From: Ed L Cashin <[email protected]>
Date: Fri, 16 Sep 2005 09:36:51 -0400
> I've been working with Jim MacBaine, and he reports that the patch
> below gets rid of the problem. I don't know why. When I test
> le64_to_cpup by itself, it works as expected.
This reminds me of a known UltraSPARC chip bug. There is a bug
wherein if you do a 64-bit endianness swapped load instruction, it
only endian swaps within each 32-bit word, not throughout the entire
64-bit word as it should.
But this is weird, because the errata description for this chip bug
says that it only applies to the deprecated 32-bit mode "ldd/std"
instructions, whereas the compiler emits the 64-bit "ldx/stx"
instructions for sparc64 kernel builds.
So, first, let's sanity check the ___arch__swab64p() implementation:
#include <stdlib.h>
#include <stdio.h>
#define ASI_PL 0x88 /* Primary, implicit, l-endian */
typedef unsigned long __u64;
static __inline__ __u64 ___arch__swab64p(const __u64 *addr)
{
__u64 ret;
__asm__ __volatile__ ("ldxa [%1] %2, %0"
: "=r" (ret)
: "r" (addr), "i" (ASI_PL));
return ret;
}
int main(void)
{
unsigned long tval = 0x123456789abcdef0;
unsigned long *p, v;
p = &tval;
v = ___arch__swab64p(p);
printf("%016lx --> %016lx\n",
tval, v);
exit(0);
}
Running this on my Ultra-IIIi workstation (this chip doesn't have
said errata) gives the desired result:
davem@sunset:~/src/GIT/sparc-2.6$ gcc -m64 -O2 -o x x.c
davem@sunset:~/src/GIT/sparc-2.6$ ./x
123456789abcdef0 --> f0debc9a78563412
davem@sunset:~/src/GIT/sparc-2.6$
So it looks sane. Let's try this on a chip that has the errata:
? ./x
123456789abcdef0 --> f0debc9a78563412
?
That's fine too, so this isn't what is causing the problems.
Wait, I think I see the problem. Is that "&id[100<<1]" pointer
aligned properly on a 64-bit boundary? I bet it isn't, and the
sparc64 unaligned load/store trap handler doesn't check whether one of
the endian swapping load/store attributes are being used.
Looking at the assembler generated for aoecmd.s, it isn't aligned:
add %l1, 236, %g2
...
ldxa [%g2] 136, %g7
That's aligned on a 4-byte boundary, but not an 8-byte one.
So this is what the bug is.
From: Ed L Cashin <[email protected]>
Date: Fri, 16 Sep 2005 09:36:51 -0400
> I've been working with Jim MacBaine, and he reports that the patch
> below gets rid of the problem. I don't know why. When I test
> le64_to_cpup by itself, it works as expected.
This patch should fix the bug.
diff --git a/arch/sparc64/kernel/una_asm.S b/arch/sparc64/kernel/una_asm.S
--- a/arch/sparc64/kernel/una_asm.S
+++ b/arch/sparc64/kernel/una_asm.S
@@ -17,7 +17,7 @@ kernel_unaligned_trap_fault:
__do_int_store:
rd %asi, %o4
wr %o3, 0, %asi
- ldx [%o2], %g3
+ mov %o2, %g3
cmp %o1, 2
be,pn %icc, 2f
cmp %o1, 4
diff --git a/arch/sparc64/kernel/unaligned.c b/arch/sparc64/kernel/unaligned.c
--- a/arch/sparc64/kernel/unaligned.c
+++ b/arch/sparc64/kernel/unaligned.c
@@ -184,13 +184,14 @@ extern void do_int_load(unsigned long *d
unsigned long *saddr, int is_signed, int asi);
extern void __do_int_store(unsigned long *dst_addr, int size,
- unsigned long *src_val, int asi);
+ unsigned long src_val, int asi);
static inline void do_int_store(int reg_num, int size, unsigned long *dst_addr,
- struct pt_regs *regs, int asi)
+ struct pt_regs *regs, int asi, int orig_asi)
{
unsigned long zero = 0;
- unsigned long *src_val = &zero;
+ unsigned long *src_val_p = &zero;
+ unsigned long src_val;
if (size == 16) {
size = 8;
@@ -198,7 +199,25 @@ static inline void do_int_store(int reg_
(unsigned)fetch_reg(reg_num, regs) : 0)) << 32) |
(unsigned)fetch_reg(reg_num + 1, regs);
} else if (reg_num) {
- src_val = fetch_reg_addr(reg_num, regs);
+ src_val_p = fetch_reg_addr(reg_num, regs);
+ }
+ src_val = *src_val_p;
+ if (unlikely(asi != orig_asi)) {
+ switch (size) {
+ case 2:
+ src_val = swab16(src_val);
+ break;
+ case 4:
+ src_val = swab32(src_val);
+ break;
+ case 8:
+ src_val = swab64(src_val);
+ break;
+ case 16:
+ default:
+ BUG();
+ break;
+ };
}
__do_int_store(dst_addr, size, src_val, asi);
}
@@ -276,6 +295,7 @@ asmlinkage void kernel_unaligned_trap(st
kernel_mna_trap_fault();
} else {
unsigned long addr;
+ int orig_asi, asi;
addr = compute_effective_address(regs, insn,
((insn >> 25) & 0x1f));
@@ -285,18 +305,48 @@ asmlinkage void kernel_unaligned_trap(st
regs->tpc, dirstrings[dir], addr, size,
regs->u_regs[UREG_RETPC]);
#endif
+ orig_asi = asi = decode_asi(insn, regs);
+ switch (asi) {
+ case ASI_NL:
+ case ASI_AIUPL:
+ case ASI_AIUSL:
+ case ASI_PL:
+ case ASI_SL:
+ case ASI_PNFL:
+ case ASI_SNFL:
+ asi &= ~0x08;
+ break;
+ };
switch (dir) {
case load:
do_int_load(fetch_reg_addr(((insn>>25)&0x1f), regs),
size, (unsigned long *) addr,
- decode_signedness(insn),
- decode_asi(insn, regs));
+ decode_signedness(insn), asi);
+ if (unlikely(asi != orig_asi)) {
+ unsigned long val_in = *(unsigned long *) addr;
+ switch (size) {
+ case 2:
+ val_in = swab16(val_in);
+ break;
+ case 4:
+ val_in = swab32(val_in);
+ break;
+ case 8:
+ val_in = swab64(val_in);
+ break;
+ case 16:
+ default:
+ BUG();
+ break;
+ };
+ *(unsigned long *) addr = val_in;
+ }
break;
case store:
do_int_store(((insn>>25)&0x1f), size,
(unsigned long *) addr, regs,
- decode_asi(insn, regs));
+ asi, orig_asi);
break;
default:
On 9/17/05, David S. Miller <[email protected]> wrote:
> This patch should fix the bug.
>
> diff --git a/arch/sparc64/kernel/una_asm.S b/arch/sparc64/kernel/una_asm.S
[..]
> diff --git a/arch/sparc64/kernel/unaligned.c b/arch/sparc64/kernel/unaligned.c
[..]
Was this patch meant to be applied to a fresh 2.6.13 kernel without
any of Ed's patches? If so, I cannot confirm that this patch works.
The aoe driver still reports a wrong size:
sunny:~# modprobe aoe
aoe: aoe_init: AoE v2.6-10 initialised.
etherd/e0.0: unknown partition table
aoe: 0011xxxxxxxx e0.0 v4000 has 7441392446501552128 sectors
The exported file has got a size of 19088743 sectors.
Regards,
Jim
From: Jim MacBaine <[email protected]>
Date: Sat, 17 Sep 2005 12:10:17 +0200
> Was this patch meant to be applied to a fresh 2.6.13 kernel without
> any of Ed's patches? If so, I cannot confirm that this patch works.
> The aoe driver still reports a wrong size:
Please double check that you really ran with the patch
applied. I even wrote a test kernel module that verified
that the bug was fixed by doing various unaligned 64-bit
loads and stores, both with and without endianness swapping.
It definitely failed before the patch, and definitely worked
with the patch.
"David S. Miller" <[email protected]> writes:
> From: Ed L Cashin <[email protected]>
> Date: Fri, 16 Sep 2005 09:36:51 -0400
>
>> I've been working with Jim MacBaine, and he reports that the patch
>> below gets rid of the problem. I don't know why. When I test
>> le64_to_cpup by itself, it works as expected.
>
> This patch should fix the bug.
>
> diff --git a/arch/sparc64/kernel/una_asm.S b/arch/sparc64/kernel/una_asm.S
> --- a/arch/sparc64/kernel/una_asm.S
> +++ b/arch/sparc64/kernel/una_asm.S
So it's OK to use the "...._to_cpup" macros with unaligned pointers?
I'm asking whether ...
1) Passing le64_to_cpup an unaligned pointer is "OK" and within the
intended use of the function. I'm having trouble finding whether
this is documented somewhere.
2) These new changes to the sparc64 unaligned access fault handling
will make it OK to leave the aoe driver the way it is in the
mainline kernel.
...
> diff --git a/arch/sparc64/kernel/unaligned.c b/arch/sparc64/kernel/unaligned.c
--
Ed L Cashin <[email protected]>
From: Ed L Cashin <[email protected]>
Date: Mon, 19 Sep 2005 10:24:00 -0400
> 1) Passing le64_to_cpup an unaligned pointer is "OK" and within the
> intended use of the function. I'm having trouble finding whether
> this is documented somewhere.
>
> 2) These new changes to the sparc64 unaligned access fault handling
> will make it OK to leave the aoe driver the way it is in the
> mainline kernel.
Both #1 and #2 are true.
Although it's very much discouraged to dereference unaligned pointers,
especially in performance critical code (which this AOE case is not,
thankfully), because performance will be really bad as the trap
handler has to fix up the access on RISC platforms.
David> Although it's very much discouraged to dereference
David> unaligned pointers, especially in performance critical code
David> (which this AOE case is not, thankfully), because
David> performance will be really bad as the trap handler has to
David> fix up the access on RISC platforms.
Also, ia64 has a tendency to print an ugly message in the kernel log
for unaligned accesses. Has anyone tried AoE on ia64?
It might be better to change the AoE code to use get_unaligned(), just
to document what's going on. Although clearly the sparc64 patch is
correct as well -- we should never silently return the wrong data.
- R.
"David S. Miller" <[email protected]> writes:
> From: Ed L Cashin <[email protected]>
> Date: Mon, 19 Sep 2005 10:24:00 -0400
>
>> 1) Passing le64_to_cpup an unaligned pointer is "OK" and within the
>> intended use of the function. I'm having trouble finding whether
>> this is documented somewhere.
>>
>> 2) These new changes to the sparc64 unaligned access fault handling
>> will make it OK to leave the aoe driver the way it is in the
>> mainline kernel.
>
> Both #1 and #2 are true.
That's interesting. I think I'll send a patch documenting #1.
> Although it's very much discouraged to dereference unaligned pointers,
> especially in performance critical code (which this AOE case is not,
> thankfully), because performance will be really bad as the trap
> handler has to fix up the access on RISC platforms.
Yes, this only happens when per AoE device when the AoE device is
discovered. Still, I might submit a patch that reverts the aoe driver
to getting the ATA identify values byte by byte as it used to do.
--
Ed L Cashin <[email protected]>