🚀 go-pugleaf

Jim MacBaine <jmacbaine@gmail.com> writes:

> Hello,
>
> Using aoe on a sparc64 system gives strange results:
>
...
> The log says:
>
> Aug 31 15:18:49 sunny kernel: devfs_mk_dir: invalid argument.<6>
> etherd/e0.0: unknown partition table
> Aug 31 15:18:49 sunny kernel: aoe: 0011d8xxxxxx e0.0 v4000 has
> 67553994410557440
> sectors

OK.  67553994410557440 is 61440 byte swapped in 64 bits, and 30MB is
61440 sectors, so this should be a simple byte order fix.

> The system is an Sun Ultra 5, running 2.6.12.5/sparc64 compiled with
> gcc-3.4.2.  e0.0 is exported on a x86 system using vblade-5, and has a
> size of 30 MB.

Thanks for the report.

--
  Ed L Cashin <ecashin@coraid.com>

"David S. Miller" <davem@davemloft.net> writes:

> From: Ed L Cashin <ecashin@coraid.com>
...
>> OK.  67553994410557440 is 61440 byte swapped in 64 bits, and 30MB is
>> 61440 sectors, so this should be a simple byte order fix.
>
> More strangely, the upper and lower 32-bit words are swapped.
> The bytes within each 32-bit word are swapped correctly.
>
> So the calculation maybe should be something like:
>
> 	__le32 *p = (__le32 *) &id[100 << 1];
> 	u32 high32 = le32_to_cpup(p);
> 	u32 low32 = le32_to_cpup(p + 1);
>
> 	ssize = (((u64)high32 << 32) | (u64) low32);
>
> But that doesn't make any sense, and even ide_fix_driveid() in
> drivers/ide/ide-iops.c does a le64_to_cpu() for this value:
>
> 	id->lba_capacity_2 = __le64_to_cpu(id->lba_capacity_2);
>
> I wonder if this is some artifact of how AOE devices encode
> this field when sending it to the client.

Well, an EtherDrive blade just copies the ATA identify response data
into a network packet without looking at it.  The vblade, though, has
to set the lba_capacity and lba_capacity_2 fields itself.

The aoe driver looks OK, but it turns out there's a byte swapping bug
in the vblade that could be related if he's running the vblade on a
big endian host (even though he said it was an x86 host), but I
haven't heard back from the original poster yet.

The vblade bug was the omission of swapping the bytes in each short.
The fix below shows what I mean:

diff -urNp a-exp/ata.c b-exp/ata.c
--- a-exp/ata.c	2005-09-01 10:19:11.000000000 -0400
+++ b-exp/ata.c	2005-09-01 10:19:12.000000000 -0400
@@ -55,24 +55,29 @@ setfld(ushort *a, int idx, int len, char
 }

 static void
-setlba28(ushort *p, vlong lba)
+setlba28(ushort *ident, vlong lba)
 {
-	p += 60;
-	*p++ = lba & 0xffff;
-	*p = lba >> 16 & 0x0fffffff;
+	uchar *cp;
+
+	cp = (uchar *) &ident[60];
+	*cp++ = lba;
+	*cp++ = lba >>= 8;
+	*cp++ = lba >>= 8;
+	*cp++ = (lba >>= 8) & 0xf;
 }

 static void
-setlba48(ushort *p, vlong lba)
+setlba48(ushort *ident, vlong lba)
 {
-	p += 100;
-	*p++ = lba;
-	lba >>= 16;
-	*p++ = lba;
-	lba >>= 16;
-	*p++ = lba;
-	lba >>= 16;
-	*p = lba;
+	uchar *cp;
+
+	cp = (uchar *) &ident[100];
+	*cp++ = lba;
+	*cp++ = lba >>= 8;
+	*cp++ = lba >>= 8;
+	*cp++ = lba >>= 8;
+	*cp++ = lba >>= 8;
+	*cp++ = lba >>= 8;
 }

 void


--
  Ed L Cashin <ecashin@coraid.com>

On 9/1/05, Ed L Cashin <ecashin@coraid.com> wrote:

> The aoe driver looks OK, but it turns out there's a byte swapping bug
> in the vblade that could be related if he's running the vblade on a
> big endian host (even though he said it was an x86 host), but I
> haven't heard back from the original poster yet.

It is in fact a x86_64 kernel, but with a mostly x86 userland. Vblade
is pure x86 code.

> The vblade bug was the omission of swapping the bytes in each short.
> The fix below shows what I mean:

Unfortunately it doesn't fix anything here. The client still reports
the same wrong size as before.  The dmesg output is identical, too.

Regards,
Jim

Jim MacBaine <jmacbaine@gmail.com> writes:

> On 9/1/05, Ed L Cashin <ecashin@coraid.com> wrote:
>
>> The aoe driver looks OK, but it turns out there's a byte swapping bug
>> in the vblade that could be related if he's running the vblade on a
>> big endian host (even though he said it was an x86 host), but I
>> haven't heard back from the original poster yet.
>
> It is in fact a x86_64 kernel, but with a mostly x86 userland. Vblade
> is pure x86 code.
>
>> The vblade bug was the omission of swapping the bytes in each short.
>> The fix below shows what I mean:
>
> Unfortunately it doesn't fix anything here. The client still reports
> the same wrong size as before.  The dmesg output is identical, too.

Let's take this discussion off the lkml, because I doubt there's a
problem with the aoe driver in the kernel, and I can easily follow up
to the lkml with a synopsis if it turns out I'm wrong.

Jim MacBaine, I'm going to ask for more details in a separate email.

--
  Ed L Cashin <ecashin@coraid.com>

Ed L Cashin <ecashin@coraid.com> writes:

...
> Let's take this discussion off the lkml, because I doubt there's a
> problem with the aoe driver in the kernel, and I can easily follow up
> to the lkml with a synopsis if it turns out I'm wrong.

It looks like I was probably wrong.  I need to do some debugging, but
the only sparc64 machine here at hand is in use.

If anybody would be up for running 2.6.13 on a sparc64 host and
running tests with a patched aoe driver, please let me know.  A test
would look something like this, using an x86 host and a sparc64 host
on the same LAN.

  x86$ dd if=/dev/zero of=/tmp/0x1234567 bs=1k count=1 seek088742
  x86$ vblade 0 1 eth0 /tmp/0x1234567

  sparc64$ rmmod aoe
  sparc64$ cd ~/linux-2.6.13
  sparc64$ patch -p1 < aoe.diff
  sparc64$ make && make modules_install
  sparc64$ modprobe aoe

I'd email you patches, and you'd email me the printk messages that
show up in the logs.  Such help would be much appreciated.

--
  Ed L Cashin <ecashin@coraid.com>

--=-=-=

Jim MacBaine <jmacbaine@gmail.com> writes:

> Hello,
>
> Using aoe on a sparc64 system gives strange results:
>
> sunny:/dev/etherd# echo >discover
> sunny:/dev/etherd# mke2fs e0.0
> mke2fs 1.37 (21-Mar-2005)
> mke2fs: File too large while trying to determine filesystem size
> sunny:/dev/etherd# blockdev --getsz e0.0
> -4503599627370496
>
> The log says:
>
> Aug 31 15:18:49 sunny kernel: devfs_mk_dir: invalid argument.<6>
> etherd/e0.0: unknown partition table
> Aug 31 15:18:49 sunny kernel: aoe: 0011d8xxxxxx e0.0 v4000 has
> 67553994410557440
> sectors
>
> The system is an Sun Ultra 5, running 2.6.12.5/sparc64 compiled with
> gcc-3.4.2.  e0.0 is exported on a x86 system using vblade-5, and has a
> size of 30 MB.

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.


--=-=-=
Content-Disposition: inline; filename=diff

Index: linux-2.6.13/drivers/block/aoe/aoecmd.c
===================================================================
--- linux-2.6.13.orig/drivers/block/aoe/aoecmd.c	2005-08-31 17:03:52.000000000 -0400
+++ linux-2.6.13/drivers/block/aoe/aoecmd.c	2005-09-15 15:44:41.000000000 -0400
@@ -320,7 +320,8 @@
 		d->flags |= DEVFL_EXT;
 
 		/* word 100: number lba48 sectors */
-		ssize = le64_to_cpup((__le64 *) &id[100<<1]);
+		ssize = *((u64 *) &id[100<<1]);
+		ssize = le64_to_cpu(ssize);
 
 		/* set as in ide-disk.c:init_idedisk_capacity */
 		d->geo.cylinders = ssize;

--=-=-=



-- 
  Ed L Cashin <ecashin@coraid.com>

--=-=-=--

From: Ed L Cashin <ecashin@coraid.com>
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 <ecashin@coraid.com>
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 <davem@davemloft.net> 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