Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]
Groups > linux.kernel > #1320229 > unrolled thread
| Started by | Joshua Kinard <kumba@gentoo.org> |
|---|---|
| First post | 2016-01-28 03:30 +0100 |
| Last post | 2016-01-30 17:40 +0100 |
| Articles | 3 — 2 participants |
Back to article view | Back to linux.kernel
This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by
below is the oldest one visible, not the original post.
Re: [PATCH 1/3] MIPS: R6: Use lightweight SYNC instruction in smp_* memory barriers Joshua Kinard <kumba@gentoo.org> - 2016-01-28 03:30 +0100
Re: [PATCH 1/3] MIPS: R6: Use lightweight SYNC instruction in smp_* memory barriers "Maciej W. Rozycki" <macro@imgtec.com> - 2016-01-29 14:40 +0100
Re: [PATCH 1/3] MIPS: R6: Use lightweight SYNC instruction in smp_* memory barriers Joshua Kinard <kumba@gentoo.org> - 2016-01-30 17:40 +0100
| From | Joshua Kinard <kumba@gentoo.org> |
|---|---|
| Date | 2016-01-28 03:30 +0100 |
| Subject | Re: [PATCH 1/3] MIPS: R6: Use lightweight SYNC instruction in smp_* memory barriers |
| Message-ID | <qVKvo-4Ms-3@gated-at.bofh.it> |
On 06/05/2015 09:10, Ralf Baechle wrote:
> On Mon, Jun 01, 2015 at 05:09:34PM -0700, Leonid Yegoshin wrote:
>
> Leonid,
>
> to me the biggest technical problem with this patch is that the new Kconfig
> option is user visible. This is the kind of deeply technical options
> which exceeds the technical knowledge of most users, so it should probably
> be driven by a select.
>
> We probably also want to enquire how old CPUs from before the invention
> of the stype field are behaving. If those as I hope for all treat an
> stype != 0 as stype 0 we could simply drop the option. But we might
> simply be out of luck - dunno.
>
> Maciej,
>
> do you have an R4000 / R4600 / R5000 / R7000 / SiByte system at hand to
> test this? I think we don't need to test that SYNC actually works as
> intended but the simpler test that SYNC <stype != 0> is not causing a
> illegal instruction exception is sufficient, that is if something like
>
> int main(int argc, charg *argv[])
> {
> asm(" .set mips2 \n"
> " sync 0x10 \n"
> " sync 0x13 \n"
> " sync 0x04 \n"
> " .set mips 0 \n");
>
> return 0;
> }
>
> doesn't crash we should be ok.
>
> The kernel's SYNC emulation should already be ok. We ignore the stype
> field entirely and for a uniprocessor R2000/R3000 that should be just
> the right thing.
>
> Ralf
I tried just compiling this on my SGI O2, which has an RM7000 CPU, and it is
refusing to even compile (after fixing typos):
# gcc -O2 -pipe sync_test.c -o sync_test
{standard input}: Assembler messages:
{standard input}:19: Error: invalid operands `sync 0x10'
{standard input}:20: Error: invalid operands `sync 0x13'
{standard input}:21: Error: invalid operands `sync 0x04'
So a bit of searching landed me here:
http://stackoverflow.com/questions/3599564/gnu-assembler-for-mips-how-to-emit-sync-instructions
And I recoded the sync insns like this:
int main(int argc, char *argv[])
{
__asm__ volatile ( \
" .set mips2 \n"
" .word (0x0000000f | (0x10 << 6)) \n"
" .word (0x0000000f | (0x14 << 6)) \n"
" .word (0x0000000f | (0x04 << 6)) \n"
" .set mips0 \n"
);
return 0;
}
And the program compiles successfully and executes with no noticeable oddities
or errors. Nothing in dmesg, no crashes, booms, or disappearance of small
kittens. I did a quick disassembly to make sure all three got emitted:
004005e0 <main>:
4005e0: 27bdfff8 addiu sp,sp,-8
4005e4: afbe0004 sw s8,4(sp)
4005e8: 03a0f021 move s8,sp
4005ec: afc40008 sw a0,8(s8)
4005f0: afc5000c sw a1,12(s8)
> 4005f4: 0000040f sync.p
> 4005f8: 0000050f 0x50f
> 4005fc: 0000010f 0x10f
400600: 00001021 move v0,zero
Same effect on my Octane (IP30) w/ an R14000 CPU. Tested inside a uclibc-based
chroot, but no change. Executes successfully silently.
--
Joshua Kinard
Gentoo/MIPS
kumba@gentoo.org
6144R/F5C6C943 2015-04-27
177C 1972 1FB8 F254 BAD0 3E72 5C63 F4E3 F5C6 C943
"The past tempts us, the present confuses us, the future frightens us. And our
lives slip away, moment by moment, lost in that vast, terrible in-between."
--Emperor Turhan, Centauri Republic
[toc] | [next] | [standalone]
| From | "Maciej W. Rozycki" <macro@imgtec.com> |
|---|---|
| Date | 2016-01-29 14:40 +0100 |
| Message-ID | <qWhrj-3JY-5@gated-at.bofh.it> |
| In reply to | #1320229 |
On Wed, 27 Jan 2016, Joshua Kinard wrote:
> On 06/05/2015 09:10, Ralf Baechle wrote:
> >
> > Maciej,
> >
> > do you have an R4000 / R4600 / R5000 / R7000 / SiByte system at hand to
> > test this? I think we don't need to test that SYNC actually works as
> > intended but the simpler test that SYNC <stype != 0> is not causing a
> > illegal instruction exception is sufficient, that is if something like
> >
> > int main(int argc, charg *argv[])
> > {
> > asm(" .set mips2 \n"
> > " sync 0x10 \n"
> > " sync 0x13 \n"
> > " sync 0x04 \n"
> > " .set mips 0 \n");
> >
> > return 0;
> > }
> >
> > doesn't crash we should be ok.
No anomalies observed with these processors:
CPU0 revision is: 00000440 (R4400SC)
CPU0 revision is: 01040102 (SiByte SB1)
> I tried just compiling this on my SGI O2, which has an RM7000 CPU, and it is
> refusing to even compile (after fixing typos):
>
> # gcc -O2 -pipe sync_test.c -o sync_test
> {standard input}: Assembler messages:
> {standard input}:19: Error: invalid operands `sync 0x10'
> {standard input}:20: Error: invalid operands `sync 0x13'
> {standard input}:21: Error: invalid operands `sync 0x04'
Yeah, there is another typo there: you need to use `.set mips32' or
suchlike rather than `.set mips2' to be able to specify an operand. That
probably counts as a bug in binutils, as -- according to what I have
observed in the other thread -- the `stype' field has been defined at
least since the MIPS IV ISA.
> And the program compiles successfully and executes with no noticeable oddities
> or errors. Nothing in dmesg, no crashes, booms, or disappearance of small
You did disable SYNC emulation in the kernel though with a change like
below, did you?
> kittens. I did a quick disassembly to make sure all three got emitted:
>
> 004005e0 <main>:
> 4005e0: 27bdfff8 addiu sp,sp,-8
> 4005e4: afbe0004 sw s8,4(sp)
> 4005e8: 03a0f021 move s8,sp
> 4005ec: afc40008 sw a0,8(s8)
> 4005f0: afc5000c sw a1,12(s8)
> > 4005f4: 0000040f sync.p
> > 4005f8: 0000050f 0x50f
> > 4005fc: 0000010f 0x10f
> 400600: 00001021 move v0,zero
You could have used the -m switch to override the architecture embedded
with the ELF file, to have the instructions disassembled correctly, e.g.:
$ objdump -m mips:isa64 -d sync
[...]
00000001200008f0 <main>:
1200008f0: 0000040f sync 0x10
1200008f4: 000004cf sync 0x13
1200008f8: 0000010f sync 0x4
1200008fc: 03e00008 jr ra
120000900: 0000102d move v0,zero
...
[...]
What's interesting to note is that rev. 2.60 of the architecture did
actually change the semantics of plain SYNC (aka SYNC 0) from an ordering
barrier to a completion barrier. Previous architectural requirements for
plain SYNC were equivalent to rev. 2.60's SYNC_MB, and to implement a
completion barrier a dummy load had to follow.
Some implementers of the old plain SYNC did actually make it a completion
rather than ordering barrier and people even argued this was an
architectural requirement, as I recall from discussions in early 2000s.
On the other hand the implementations affected were UP-only processors
where about the only use for SYNC was to drain any write buffers of data
intended for MMIO accesses and such an implementation did conform even if
it was too heavyweight for architectural requirements. So I think it was
a reasonable implementation decision, saving 1-2 instructions where a
completion barrier was required. The only downside of this decision was
some programmers assumed such semantics was universally guaranteed, while
indeed it was not (before rev. 2.60).
Overall where backwards compatibility is required it looks to me like we
need to keep the implementation of any completion barriers (e.g. `iob') as
a SYNC followed by a dummy load.
Maciej
linux-mips-no-sync.diff
Index: linux/arch/mips/kernel/traps.c
===================================================================
--- linux.orig/arch/mips/kernel/traps.c
+++ linux/arch/mips/kernel/traps.c
@@ -672,6 +672,7 @@ static int simulate_rdhwr_mm(struct pt_r
return -1;
}
+#if 0
static int simulate_sync(struct pt_regs *regs, unsigned int opcode)
{
if ((opcode & OPCODE) == SPEC0 && (opcode & FUNC) == SYNC) {
@@ -682,6 +683,7 @@ static int simulate_sync(struct pt_regs
return -1; /* Must be something else ... */
}
+#endif
asmlinkage void do_ov(struct pt_regs *regs)
{
@@ -1117,8 +1119,10 @@ asmlinkage void do_ri(struct pt_regs *re
if (status < 0)
status = simulate_rdhwr_normal(regs, opcode);
+#if 0
if (status < 0)
status = simulate_sync(regs, opcode);
+#endif
if (status < 0)
status = simulate_fp(regs, opcode, old_epc, old31);
[toc] | [prev] | [next] | [standalone]
| From | Joshua Kinard <kumba@gentoo.org> |
|---|---|
| Date | 2016-01-30 17:40 +0100 |
| Message-ID | <qWGJ4-6og-3@gated-at.bofh.it> |
| In reply to | #1321753 |
On 01/29/2016 08:32, Maciej W. Rozycki wrote:
> On Wed, 27 Jan 2016, Joshua Kinard wrote:
>
>> On 06/05/2015 09:10, Ralf Baechle wrote:
>>>
>>> Maciej,
>>>
>>> do you have an R4000 / R4600 / R5000 / R7000 / SiByte system at hand to
>>> test this? I think we don't need to test that SYNC actually works as
>>> intended but the simpler test that SYNC <stype != 0> is not causing a
>>> illegal instruction exception is sufficient, that is if something like
>>>
>>> int main(int argc, charg *argv[])
>>> {
>>> asm(" .set mips2 \n"
>>> " sync 0x10 \n"
>>> " sync 0x13 \n"
>>> " sync 0x04 \n"
>>> " .set mips 0 \n");
>>>
>>> return 0;
>>> }
>>>
>>> doesn't crash we should be ok.
>
> No anomalies observed with these processors:
>
> CPU0 revision is: 00000440 (R4400SC)
> CPU0 revision is: 01040102 (SiByte SB1)
>
>> I tried just compiling this on my SGI O2, which has an RM7000 CPU, and it is
>> refusing to even compile (after fixing typos):
>>
>> # gcc -O2 -pipe sync_test.c -o sync_test
>> {standard input}: Assembler messages:
>> {standard input}:19: Error: invalid operands `sync 0x10'
>> {standard input}:20: Error: invalid operands `sync 0x13'
>> {standard input}:21: Error: invalid operands `sync 0x04'
>
> Yeah, there is another typo there: you need to use `.set mips32' or
> suchlike rather than `.set mips2' to be able to specify an operand. That
> probably counts as a bug in binutils, as -- according to what I have
> observed in the other thread -- the `stype' field has been defined at
> least since the MIPS IV ISA.
>
>> And the program compiles successfully and executes with no noticeable oddities
>> or errors. Nothing in dmesg, no crashes, booms, or disappearance of small
>
> You did disable SYNC emulation in the kernel though with a change like
> below, did you?
>
I did not...wasn't aware that the kernel emulates a lot of non-compatible
instructions in traps.c. I applied the patch to both my IP30 and IP32 kernels,
rebooted, and tested both forms of the test program (using the '.word' form and
'.set mips32' variants with operands to 'sync'), and the binary appears to run
w/o incident, as far as I can tell. Looks like at least the RM7K and R1x000
families handle operands to 'sync' w/o issue.
I might be able to test an RM5200 box out within the next two weeks, once I
finish building a netboot image to boot up a spare O2.
>> kittens. I did a quick disassembly to make sure all three got emitted:
>>
>> 004005e0 <main>:
>> 4005e0: 27bdfff8 addiu sp,sp,-8
>> 4005e4: afbe0004 sw s8,4(sp)
>> 4005e8: 03a0f021 move s8,sp
>> 4005ec: afc40008 sw a0,8(s8)
>> 4005f0: afc5000c sw a1,12(s8)
>>> 4005f4: 0000040f sync.p
>>> 4005f8: 0000050f 0x50f
>>> 4005fc: 0000010f 0x10f
>> 400600: 00001021 move v0,zero
>
> You could have used the -m switch to override the architecture embedded
> with the ELF file, to have the instructions disassembled correctly, e.g.:
>
> $ objdump -m mips:isa64 -d sync
> [...]
> 00000001200008f0 <main>:
> 1200008f0: 0000040f sync 0x10
> 1200008f4: 000004cf sync 0x13
> 1200008f8: 0000010f sync 0x4
> 1200008fc: 03e00008 jr ra
> 120000900: 0000102d move v0,zero
> ...
> [...]
>
> What's interesting to note is that rev. 2.60 of the architecture did
> actually change the semantics of plain SYNC (aka SYNC 0) from an ordering
> barrier to a completion barrier. Previous architectural requirements for
> plain SYNC were equivalent to rev. 2.60's SYNC_MB, and to implement a
> completion barrier a dummy load had to follow.
>
> Some implementers of the old plain SYNC did actually make it a completion
> rather than ordering barrier and people even argued this was an
> architectural requirement, as I recall from discussions in early 2000s.
> On the other hand the implementations affected were UP-only processors
> where about the only use for SYNC was to drain any write buffers of data
> intended for MMIO accesses and such an implementation did conform even if
> it was too heavyweight for architectural requirements. So I think it was
> a reasonable implementation decision, saving 1-2 instructions where a
> completion barrier was required. The only downside of this decision was
> some programmers assumed such semantics was universally guaranteed, while
> indeed it was not (before rev. 2.60).
>
> Overall where backwards compatibility is required it looks to me like we
> need to keep the implementation of any completion barriers (e.g. `iob') as
> a SYNC followed by a dummy load.
>
> Maciej
>
> linux-mips-no-sync.diff
> Index: linux/arch/mips/kernel/traps.c
> ===================================================================
> --- linux.orig/arch/mips/kernel/traps.c
> +++ linux/arch/mips/kernel/traps.c
> @@ -672,6 +672,7 @@ static int simulate_rdhwr_mm(struct pt_r
> return -1;
> }
>
> +#if 0
> static int simulate_sync(struct pt_regs *regs, unsigned int opcode)
> {
> if ((opcode & OPCODE) == SPEC0 && (opcode & FUNC) == SYNC) {
> @@ -682,6 +683,7 @@ static int simulate_sync(struct pt_regs
>
> return -1; /* Must be something else ... */
> }
> +#endif
>
> asmlinkage void do_ov(struct pt_regs *regs)
> {
> @@ -1117,8 +1119,10 @@ asmlinkage void do_ri(struct pt_regs *re
> if (status < 0)
> status = simulate_rdhwr_normal(regs, opcode);
>
> +#if 0
> if (status < 0)
> status = simulate_sync(regs, opcode);
> +#endif
>
> if (status < 0)
> status = simulate_fp(regs, opcode, old_epc, old31);
>
>
--
Joshua Kinard
Gentoo/MIPS
kumba@gentoo.org
6144R/F5C6C943 2015-04-27
177C 1972 1FB8 F254 BAD0 3E72 5C63 F4E3 F5C6 C943
"The past tempts us, the present confuses us, the future frightens us. And our
lives slip away, moment by moment, lost in that vast, terrible in-between."
--Emperor Turhan, Centauri Republic
[toc] | [prev] | [standalone]
Back to top | Article view | linux.kernel
csiph-web