[PATCH 0/5] zfs: Fix zfs support on aarch64

[PATCH 0/5] zfs: Fix zfs support on aarch64

[mwleeds]

This patch series is needed to get U-Boot to boot from a ZFS filesystem
on an aarch64 computer. Some of the patches are not architecture specific
and would be needed to boot ZFS on other platforms as well. The ZFS
support in U-Boot hasn't been substantively touched in several years and
to me it seems like it must have been broken for a long time on all
platforms, but I have only tested on aarch64.

Since there doesn't seem to be a mantainer for this area who I can cc,
I'm hoping these patches get seen and pulled in by a general U-Boot
maintainer.

Phaedrus Leeds (5):
  zfs: Fix malloc() success check
  zfs: Add a comment to clarify nvlist memory layout
  zfs: Fix unaligned read of uint64
  zfs: Fix return value of fs_devread()
  zfs: Fix zfs_read() to actually work

 fs/zfs/dev.c |  2 +-
 fs/zfs/zfs.c | 30 ++++++++++++++++++++++++++----
 2 files changed, 27 insertions(+), 5 deletions(-)

-- 
2.44.0

[PATCH 1/5] zfs: Fix malloc() success check

[mwleeds]

This code was hitting the error code path whenever malloc() succeeded
rather than when it failed, so presumably this part of the code hasn't
been tested. I had to apply this fix (and others) to get U-Boot to boot
from ZFS on an Nvidia Jetson TX2 NX SoM (an aarch64 computer).

Signed-off-by: Phaedrus Leeds <mwleeds@mailtundra.com>
Tested-by: Phaedrus Leeds <mwleeds@mailtundra.com>
---
 fs/zfs/zfs.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/fs/zfs/zfs.c b/fs/zfs/zfs.c
index 1fec96cd5c..14779dee32 100644
--- a/fs/zfs/zfs.c
+++ b/fs/zfs/zfs.c
@@ -648,21 +648,21 @@ dmu_read(dnode_end_t *dn, uint64_t blkid, void **buf,
 		if (bp_array != dn->dn.dn_blkptr) {
 			free(bp_array);
 			bp_array = 0;
 		}
 
 		if (BP_IS_HOLE(bp)) {
 			size_t size = zfs_to_cpu16(dn->dn.dn_datablkszsec,
 											dn->endian)
 				<< SPA_MINBLOCKSHIFT;
 			*buf = malloc(size);
-			if (*buf) {
+			if (!*buf) {
 				err = ZFS_ERR_OUT_OF_MEMORY;
 				break;
 			}
 			memset(*buf, 0, size);
 			endian = (zfs_to_cpu64(bp->blk_prop, endian) >> 63) & 1;
 			break;
 		}
 		if (level == 0) {
 			err = zio_read(bp, endian, buf, 0, data);
 			endian = (zfs_to_cpu64(bp->blk_prop, endian) >> 63) & 1;
-- 
2.44.0

[PATCH 2/5] zfs: Add a comment to clarify nvlist memory layout

[mwleeds]

Signed-off-by: Phaedrus Leeds <mwleeds@mailtundra.com>
Tested-by: Phaedrus Leeds <mwleeds@mailtundra.com>
---
 fs/zfs/zfs.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/fs/zfs/zfs.c b/fs/zfs/zfs.c
index 14779dee32..61d58fce68 100644
--- a/fs/zfs/zfs.c
+++ b/fs/zfs/zfs.c
@@ -1610,20 +1610,25 @@ zfs_nvlist_lookup_nvlist(char *nvlist, char *name)
 {
 	char *nvpair;
 	char *ret;
 	size_t size;
 	int found;
 
 	found = nvlist_find_value(nvlist, name, DATA_TYPE_NVLIST, &nvpair,
 							  &size, 0);
 	if (!found)
 		return 0;
+
+	/* Allocate 12 bytes in addition to the nvlist size: One uint32 before the
+	 * nvlist to hold the encoding method, and two zero uint32's after the
+	 * nvlist as the NULL terminator.
+	 */
 	ret = calloc(1, size + 3 * sizeof(uint32_t));
 	if (!ret)
 		return 0;
 	memcpy(ret, nvlist, sizeof(uint32_t));
 
 	memcpy(ret + sizeof(uint32_t), nvpair, size);
 	return ret;
 }
 
 int
-- 
2.44.0

[PATCH 3/5] zfs: Fix unaligned read of uint64

[mwleeds]

Without this patch, when trying to boot zfs using U-Boot on a Jetson TX2
NX (which is aarch64), I get a CPU reset error like so:

"Synchronous Abort" handler, esr 0x96000021
elr: 00000000800c9000 lr : 00000000800c8ffc (reloc)
elr: 00000000fff77000 lr : 00000000fff76ffc
x0 : 00000000ffb40f04 x1 : 0000000000000000
x2 : 000000000000000a x3 : 0000000003100000
x4 : 0000000003100000 x5 : 0000000000000034
x6 : 00000000fff9cc6e x7 : 000000000000000f
x8 : 00000000ff7f84a0 x9 : 0000000000000008
x10: 00000000ffb40f04 x11: 0000000000000006
x12: 000000000001869f x13: 0000000000000001
x14: 00000000ff7f84bc x15: 0000000000000010
x16: 0000000000002080 x17: 00000000001fffff
x18: 00000000ff7fbdd8 x19: 00000000ffb405f8
x20: 00000000ffb40dd0 x21: 00000000fffabe5e
x22: 000000ea77940000 x23: 00000000ffb42090
x24: 0000000000000000 x25: 0000000000000000
x26: 0000000000000000 x27: 0000000000000000
x28: 0000000000bab10c x29: 00000000ff7f85f0

Code: d00001a0 9103a000 94006ac6 f9401ba0 (f9400000)
Resetting CPU ...

This happens when be64_to_cpu() is called on a value that exists at a
memory address that's 4 byte aligned but not 8 byte aligned (e.g. an
address ending in 04). The call stack where that happens is:
check_pool_label() ->
zfs_nvlist_lookup_uint64(vdevnvlist, ZPOOL_CONFIG_ASHIFT,...) ->
be64_to_cpu()

Signed-off-by: Phaedrus Leeds <mwleeds@mailtundra.com>
Tested-by: Phaedrus Leeds <mwleeds@mailtundra.com>
---
 fs/zfs/zfs.c | 18 ++++++++++++++++++
 1 file changed, 18 insertions(+)

diff --git a/fs/zfs/zfs.c b/fs/zfs/zfs.c
index 61d58fce68..9a50deac18 100644
--- a/fs/zfs/zfs.c
+++ b/fs/zfs/zfs.c
@@ -1552,35 +1552,53 @@ nvlist_find_value(char *nvlist, char *name, int valtype, char **val,
 			if (nelm_out)
 				*nelm_out = nelm;
 			return 1;
 		}
 
 		nvlist += encode_size;	/* goto the next nvpair */
 	}
 	return 0;
 }
 
+int is_word_aligned_ptr(void *ptr) {
+	return ((uintptr_t)ptr & (sizeof(void *) - 1)) == 0;
+}
+
 int
 zfs_nvlist_lookup_uint64(char *nvlist, char *name, uint64_t *out)
 {
 	char *nvpair;
 	size_t size;
 	int found;
 
 	found = nvlist_find_value(nvlist, name, DATA_TYPE_UINT64, &nvpair, &size, 0);
 	if (!found)
 		return 0;
 	if (size < sizeof(uint64_t)) {
 		printf("invalid uint64\n");
 		return ZFS_ERR_BAD_FS;
 	}
 
+	/* On arm64, calling be64_to_cpu() on a value stored at a memory address
+	 * that's not 8-byte aligned causes the CPU to reset. Avoid that by copying the
+	 * value somewhere else if needed.
+	 */
+	if (!is_word_aligned_ptr((void *)nvpair)) {
+		uint64_t *alignedptr = malloc(sizeof(uint64_t));
+		if (!alignedptr)
+			return 0;
+		memcpy(alignedptr, nvpair, sizeof(uint64_t));
+		*out = be64_to_cpu(*alignedptr);
+		free(alignedptr);
+		return 1;
+	}
+
 	*out = be64_to_cpu(*(uint64_t *) nvpair);
 	return 1;
 }
 
 char *
 zfs_nvlist_lookup_string(char *nvlist, char *name)
 {
 	char *nvpair;
 	char *ret;
 	size_t slen;
-- 
2.44.0

[PATCH 4/5] zfs: Fix return value of fs_devread()

[mwleeds]

As evidenced by how other filesystems handle it, a return value of 0
from fs_devread() means failure; nonzero means success. The opposite
assumption was being made in zfs.c for the use of zfs_devread() so fix
the confusion by making zfs_devread() return 0 on success.

It probably doesn't make sense to change the handling of zfs_devread()
in zfs.c instead, because as it is it matches the semantics of the other
functions there.

Signed-off-by: Phaedrus Leeds <mwleeds@mailtundra.com>
Tested-by: Phaedrus Leeds <mwleeds@mailtundra.com>
---
 fs/zfs/dev.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/fs/zfs/dev.c b/fs/zfs/dev.c
index 251e7d1f74..fcd9893b3a 100644
--- a/fs/zfs/dev.c
+++ b/fs/zfs/dev.c
@@ -19,12 +19,12 @@ static struct disk_partition *part_info;
 void zfs_set_blk_dev(struct blk_desc *rbdd, struct disk_partition *info)
 {
 	zfs_blk_desc = rbdd;
 	part_info = info;
 }
 
 /* err */
 int zfs_devread(int sector, int byte_offset, int byte_len, char *buf)
 {
 	return fs_devread(zfs_blk_desc, part_info, sector, byte_offset,
-			  byte_len, buf);
+			  byte_len, buf) ? 0 : 1;
 }
-- 
2.44.0

[PATCH 5/5] zfs: Fix zfs_read() to actually work

[mwleeds]

Without this patch, the while loop being modified goes on infinitely,
but with the patch I am able to boot linux on zfs on a jetson tx2 nx.

It seems like this code was never tested because the logic is clearly
wrong. The function do_div(a,b) does a division that modifies the first
parameter to have a = a / b, and returns the remainder of the division.
So clearly in the usual case when file->offset = 0, the line
"blkid = do_div(blkid, blksz);" just results in blkid being set to zero
on every iteration of the loop, rather than being incremented as blocks
are read. Hence the zeroth block is read over and over and this becomes
an infinite loop.

So instead capture the remainder of the division in a "blkoff" variable,
and use that to properly calculate the memory address to move from in
memmove() below.

For example, if file->offset were 1337, on the first iteration of the
loop blkid would be 0 and blkoff would be 1337. If the blksz is 131072
(as it was for me), that amount of data would be copied into
data->file_buf. movesize would be 131072 - 1337 = 129735 so 129735 bytes
would be moved into buf. On the second iteration of the loop (assuming
there is one), red would be 129735, blkid would be 1, blkoff would be 0,
and 131072 bytes would be copied into buf. And so on...

Signed-off-by: Phaedrus Leeds <mwleeds@mailtundra.com>
Tested-by: Phaedrus Leeds <mwleeds@mailtundra.com>
---
 fs/zfs/zfs.c | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/fs/zfs/zfs.c b/fs/zfs/zfs.c
index 9a50deac18..bfc11fa667 100644
--- a/fs/zfs/zfs.c
+++ b/fs/zfs/zfs.c
@@ -2128,37 +2128,36 @@ zfs_read(zfs_file_t file, char *buf, uint64_t len)
 	 * now, this only reads in one data block at a time.
 	 */
 	length = len;
 	red = 0;
 	while (length) {
 		void *t;
 		/*
 		 * Find requested blkid and the offset within that block.
 		 */
 		uint64_t blkid = file->offset + red;
-		blkid = do_div(blkid, blksz);
+		uint64_t blkoff = do_div(blkid, blksz);
 		free(data->file_buf);
 		data->file_buf = 0;
 
 		err = dmu_read(&(data->dnode), blkid, &t,
 					   0, data);
 		data->file_buf = t;
 		if (err)
 			return -1;
 
 		data->file_start = blkid * blksz;
 		data->file_end = data->file_start + blksz;
 
 		movesize = min(length, data->file_end - (int)file->offset - red);
 
-		memmove(buf, data->file_buf + file->offset + red
-				- data->file_start, movesize);
+		memmove(buf, data->file_buf + blkoff, movesize);
 		buf += movesize;
 		length -= movesize;
 		red += movesize;
 	}
 
 	return len;
 }
 
 int
 zfs_close(zfs_file_t file)
-- 
2.44.0

Re: [PATCH 1/5] zfs: Fix malloc() success check

[Igor Opaniuk]

Hi Phaedrus,

On Sun, Apr 7, 2024 at 4:00 AM <mwleeds@mailtundra.com> wrote:

> This code was hitting the error code path whenever malloc() succeeded
> rather than when it failed, so presumably this part of the code hasn't
> been tested. I had to apply this fix (and others) to get U-Boot to boot
> from ZFS on an Nvidia Jetson TX2 NX SoM (an aarch64 computer).
>
> Signed-off-by: Phaedrus Leeds <mwleeds@mailtundra.com>
> Tested-by: Phaedrus Leeds <mwleeds@mailtundra.com>
>
It's an abuse of the Tested-by tag. If you are the author of the patch,
that obviously implies that you tested it before sending to ML.
Signed-off-by is enough in this case.

---
>  fs/zfs/zfs.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/fs/zfs/zfs.c b/fs/zfs/zfs.c
> index 1fec96cd5c..14779dee32 100644
> --- a/fs/zfs/zfs.c
> +++ b/fs/zfs/zfs.c
> @@ -648,21 +648,21 @@ dmu_read(dnode_end_t *dn, uint64_t blkid, void **buf,
>                 if (bp_array != dn->dn.dn_blkptr) {
>                         free(bp_array);
>                         bp_array = 0;
>                 }
>
>                 if (BP_IS_HOLE(bp)) {
>                         size_t size = zfs_to_cpu16(dn->dn.dn_datablkszsec,
>
>               dn->endian)
>                                 << SPA_MINBLOCKSHIFT;
>                         *buf = malloc(size);
> -                       if (*buf) {
> +                       if (!*buf) {
>                                 err = ZFS_ERR_OUT_OF_MEMORY;
>                                 break;
>                         }
>                         memset(*buf, 0, size);
>                         endian = (zfs_to_cpu64(bp->blk_prop, endian) >>
> 63) & 1;
>                         break;
>                 }
>                 if (level == 0) {
>                         err = zio_read(bp, endian, buf, 0, data);
>                         endian = (zfs_to_cpu64(bp->blk_prop, endian) >>
> 63) & 1;
> --
> 2.44.0
>
>

-- 
Best regards - Atentamente - Meilleures salutations

Igor Opaniuk

mailto: igor.opaniuk@gmail.com
skype: igor.opanyuk
https://www.linkedin.com/in/iopaniuk <http://ua.linkedin.com/in/iopaniuk>

Re: [PATCH 1/5] zfs: Fix malloc() success check

[mwleeds]

On Sunday, April 7th, 2024 at 4:22 AM, Igor Opaniuk <igor.opaniuk@gmail.com> wrote:

> Hi Phaedrus,
> 
> On Sun, Apr 7, 2024 at 4:00 AM mwleeds@mailtundra.com wrote:
> 
> > This code was hitting the error code path whenever malloc() succeeded
> > rather than when it failed, so presumably this part of the code hasn't
> > been tested. I had to apply this fix (and others) to get U-Boot to boot
> > from ZFS on an Nvidia Jetson TX2 NX SoM (an aarch64 computer).
> > 
> > Signed-off-by: Phaedrus Leeds mwleeds@mailtundra.com
> > Tested-by: Phaedrus Leeds mwleeds@mailtundra.com
> 
> It's an abuse of the Tested-by tag. If you are the author of the patch,
> that obviously implies that you tested it before sending to ML.
> Signed-off-by is enough in this case.
> 

That makes sense. Sorry I'm a bit new to this way of submitting patches and more accustomed to pull requests. It seems like a minor thing though; should I re-submit the patches?

> ---
> 
> > fs/zfs/zfs.c | 2 +-
> > 1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/fs/zfs/zfs.c b/fs/zfs/zfs.c
> > index 1fec96cd5c..14779dee32 100644
> > --- a/fs/zfs/zfs.c
> > +++ b/fs/zfs/zfs.c
> > @@ -648,21 +648,21 @@ dmu_read(dnode_end_t *dn, uint64_t blkid, void **buf,
> > if (bp_array != dn->dn.dn_blkptr) {
> > free(bp_array);
> > bp_array = 0;
> > }
> > 
> > if (BP_IS_HOLE(bp)) {
> > size_t size = zfs_to_cpu16(dn->dn.dn_datablkszsec,
> > 
> > dn->endian)
> > << SPA_MINBLOCKSHIFT;
> > *buf = malloc(size);
> > - if (*buf) {
> > + if (!*buf) {
> > err = ZFS_ERR_OUT_OF_MEMORY;
> > break;
> > }
> > memset(*buf, 0, size);
> > endian = (zfs_to_cpu64(bp->blk_prop, endian) >>
> > 63) & 1;
> > break;
> > }
> > if (level == 0) {
> > err = zio_read(bp, endian, buf, 0, data);
> > endian = (zfs_to_cpu64(bp->blk_prop, endian) >>
> > 63) & 1;
> > --
> > 2.44.0
> 
> 
> --
> Best regards - Atentamente - Meilleures salutations
> 
> Igor Opaniuk
> 
> mailto: igor.opaniuk@gmail.com
> skype: igor.opanyuk
> https://www.linkedin.com/in/iopaniuk http://ua.linkedin.com/in/iopaniuk

Re: [PATCH 3/5] zfs: Fix unaligned read of uint64

[Caleb Connolly]

Hi Phaedrus,

On 07/04/2024 03:47, mwleeds@mailtundra.com wrote:
> Without this patch, when trying to boot zfs using U-Boot on a Jetson TX2
> NX (which is aarch64), I get a CPU reset error like so:
> 
> "Synchronous Abort" handler, esr 0x96000021
> elr: 00000000800c9000 lr : 00000000800c8ffc (reloc)
> elr: 00000000fff77000 lr : 00000000fff76ffc
> x0 : 00000000ffb40f04 x1 : 0000000000000000
> x2 : 000000000000000a x3 : 0000000003100000
> x4 : 0000000003100000 x5 : 0000000000000034
> x6 : 00000000fff9cc6e x7 : 000000000000000f
> x8 : 00000000ff7f84a0 x9 : 0000000000000008
> x10: 00000000ffb40f04 x11: 0000000000000006
> x12: 000000000001869f x13: 0000000000000001
> x14: 00000000ff7f84bc x15: 0000000000000010
> x16: 0000000000002080 x17: 00000000001fffff
> x18: 00000000ff7fbdd8 x19: 00000000ffb405f8
> x20: 00000000ffb40dd0 x21: 00000000fffabe5e
> x22: 000000ea77940000 x23: 00000000ffb42090
> x24: 0000000000000000 x25: 0000000000000000
> x26: 0000000000000000 x27: 0000000000000000
> x28: 0000000000bab10c x29: 00000000ff7f85f0
> 
> Code: d00001a0 9103a000 94006ac6 f9401ba0 (f9400000)
> Resetting CPU ...
> 
> This happens when be64_to_cpu() is called on a value that exists at a
> memory address that's 4 byte aligned but not 8 byte aligned (e.g. an
> address ending in 04). The call stack where that happens is:
> check_pool_label() ->
> zfs_nvlist_lookup_uint64(vdevnvlist, ZPOOL_CONFIG_ASHIFT,...) ->
> be64_to_cpu()

This is very odd, aarch64 doesn't generally have these restrictions. I 
got a bit nerdsniped when I saw this so I did some digging and figured this:

1. Your abort exception doesn't include the FAR_ELx register (which 
should contain the address that was being accessed when the abort 
occured). This means your board is running in EL3.
2. It turns out there is an "A" flag in the SCTLR_ELx register, when set 
this flag causes a fault when trying to load from an address that isn't 
aligned to the size of the data element (see "A, bit" on 
https://developer.arm.com/documentation/ddi0595/2021-06/AArch64-Registers/SCTLR-EL3--System-Control-Register--EL3-

I'm not sure who's in the "wrong" here, maybe the driver should avoid 
unaligned accesses? But then again, I don't think you should be running 
a ZFS driver in EL3.

I'm not familiar with the Jetson Nano, but maybe there's a documented 
way to run U-Boot so that it isn't executing in EL3? Or if not you could 
also try unsetting the A flag.

If this really is something to fix in the driver, I don't think 
hotpatching every unaligned access with a malloc() is the right solution.

> 
> Signed-off-by: Phaedrus Leeds <mwleeds@mailtundra.com>
> Tested-by: Phaedrus Leeds <mwleeds@mailtundra.com>

regarding your question about re-sending to remove these tags, I'd say 
probably yes, and especially if you're going to send a new revision anyway.

fwiw you seem to have gotten pretty much everything else about the patch 
submission process spot on :)

Kind regards,
> ---
>   fs/zfs/zfs.c | 18 ++++++++++++++++++
>   1 file changed, 18 insertions(+)
> 
> diff --git a/fs/zfs/zfs.c b/fs/zfs/zfs.c
> index 61d58fce68..9a50deac18 100644
> --- a/fs/zfs/zfs.c
> +++ b/fs/zfs/zfs.c
> @@ -1552,35 +1552,53 @@ nvlist_find_value(char *nvlist, char *name, int valtype, char **val,
>   			if (nelm_out)
>   				*nelm_out = nelm;
>   			return 1;
>   		}
>   
>   		nvlist += encode_size;	/* goto the next nvpair */
>   	}
>   	return 0;
>   }
>   
> +int is_word_aligned_ptr(void *ptr) {
> +	return ((uintptr_t)ptr & (sizeof(void *) - 1)) == 0;
> +}
> +
>   int
>   zfs_nvlist_lookup_uint64(char *nvlist, char *name, uint64_t *out)
>   {
>   	char *nvpair;
>   	size_t size;
>   	int found;
>   
>   	found = nvlist_find_value(nvlist, name, DATA_TYPE_UINT64, &nvpair, &size, 0);
>   	if (!found)
>   		return 0;
>   	if (size < sizeof(uint64_t)) {
>   		printf("invalid uint64\n");
>   		return ZFS_ERR_BAD_FS;
>   	}
>   
> +	/* On arm64, calling be64_to_cpu() on a value stored at a memory address
> +	 * that's not 8-byte aligned causes the CPU to reset. Avoid that by copying the
> +	 * value somewhere else if needed.
> +	 */
> +	if (!is_word_aligned_ptr((void *)nvpair)) {
> +		uint64_t *alignedptr = malloc(sizeof(uint64_t));
> +		if (!alignedptr)
> +			return 0;
> +		memcpy(alignedptr, nvpair, sizeof(uint64_t));
> +		*out = be64_to_cpu(*alignedptr);
> +		free(alignedptr);
> +		return 1;
> +	}
> +
>   	*out = be64_to_cpu(*(uint64_t *) nvpair);
>   	return 1;
>   }
>   
>   char *
>   zfs_nvlist_lookup_string(char *nvlist, char *name)
>   {
>   	char *nvpair;
>   	char *ret;
>   	size_t slen;

-- 
// Caleb (they/them)

Re: [PATCH 3/5] zfs: Fix unaligned read of uint64

[Caleb Connolly]

On 12/04/2024 20:50, Caleb Connolly wrote:
> Hi Phaedrus,
> 
> On 07/04/2024 03:47, mwleeds@mailtundra.com wrote:
>> Without this patch, when trying to boot zfs using U-Boot on a Jetson TX2
>> NX (which is aarch64), I get a CPU reset error like so:
>>
>> "Synchronous Abort" handler, esr 0x96000021
>> elr: 00000000800c9000 lr : 00000000800c8ffc (reloc)
>> elr: 00000000fff77000 lr : 00000000fff76ffc
>> x0 : 00000000ffb40f04 x1 : 0000000000000000
>> x2 : 000000000000000a x3 : 0000000003100000
>> x4 : 0000000003100000 x5 : 0000000000000034
>> x6 : 00000000fff9cc6e x7 : 000000000000000f
>> x8 : 00000000ff7f84a0 x9 : 0000000000000008
>> x10: 00000000ffb40f04 x11: 0000000000000006
>> x12: 000000000001869f x13: 0000000000000001
>> x14: 00000000ff7f84bc x15: 0000000000000010
>> x16: 0000000000002080 x17: 00000000001fffff
>> x18: 00000000ff7fbdd8 x19: 00000000ffb405f8
>> x20: 00000000ffb40dd0 x21: 00000000fffabe5e
>> x22: 000000ea77940000 x23: 00000000ffb42090
>> x24: 0000000000000000 x25: 0000000000000000
>> x26: 0000000000000000 x27: 0000000000000000
>> x28: 0000000000bab10c x29: 00000000ff7f85f0
>>
>> Code: d00001a0 9103a000 94006ac6 f9401ba0 (f9400000)
>> Resetting CPU ...
>>
>> This happens when be64_to_cpu() is called on a value that exists at a
>> memory address that's 4 byte aligned but not 8 byte aligned (e.g. an
>> address ending in 04). The call stack where that happens is:
>> check_pool_label() ->
>> zfs_nvlist_lookup_uint64(vdevnvlist, ZPOOL_CONFIG_ASHIFT,...) ->
>> be64_to_cpu()
> 
> This is very odd, aarch64 doesn't generally have these restrictions. I 
> got a bit nerdsniped when I saw this so I did some digging and figured 
> this:
> 
> 1. Your abort exception doesn't include the FAR_ELx register (which 
> should contain the address that was being accessed when the abort 
> occured). This means your board is running in EL3.
> 2. It turns out there is an "A" flag in the SCTLR_ELx register, when set 
> this flag causes a fault when trying to load from an address that isn't 
> aligned to the size of the data element (see "A, bit" on 
> https://developer.arm.com/documentation/ddi0595/2021-06/AArch64-Registers/SCTLR-EL3--System-Control-Register--EL3-
> 
> I'm not sure who's in the "wrong" here, maybe the driver should avoid 
> unaligned accesses? But then again, I don't think you should be running 
> a ZFS driver in EL3.
> 
> I'm not familiar with the Jetson Nano, but maybe there's a documented 
> way to run U-Boot so that it isn't executing in EL3? Or if not you could 
> also try unsetting the A flag.
> 
> If this really is something to fix in the driver, I don't think 
> hotpatching every unaligned access with a malloc() is the right solution.
> 
>>
>> Signed-off-by: Phaedrus Leeds <mwleeds@mailtundra.com>
>> Tested-by: Phaedrus Leeds <mwleeds@mailtundra.com>
> 
> regarding your question about re-sending to remove these tags, I'd say 
> probably yes, and especially if you're going to send a new revision anyway.
> 
> fwiw you seem to have gotten pretty much everything else about the patch 
> submission process spot on :)

Oh, by the way, instead of Tested-by tags, can you add Fixes tags on the 
patches that contain bug fixes?

In case you're unfamiliar, you can generate them like this:

$ git config --global pretty.fixes = "Fixes: %h (\"%s\")"
$ git log --oneline --pretty=fixes <SHA>

Probably for all of yours that will be:

Fixes: 4d3c95f5ea7c ("zfs: Add ZFS filesystem support")
> 
> Kind regards,
>> ---
>>   fs/zfs/zfs.c | 18 ++++++++++++++++++
>>   1 file changed, 18 insertions(+)
>>
>> diff --git a/fs/zfs/zfs.c b/fs/zfs/zfs.c
>> index 61d58fce68..9a50deac18 100644
>> --- a/fs/zfs/zfs.c
>> +++ b/fs/zfs/zfs.c
>> @@ -1552,35 +1552,53 @@ nvlist_find_value(char *nvlist, char *name, 
>> int valtype, char **val,
>>               if (nelm_out)
>>                   *nelm_out = nelm;
>>               return 1;
>>           }
>>           nvlist += encode_size;    /* goto the next nvpair */
>>       }
>>       return 0;
>>   }
>> +int is_word_aligned_ptr(void *ptr) {
>> +    return ((uintptr_t)ptr & (sizeof(void *) - 1)) == 0;
>> +}
>> +
>>   int
>>   zfs_nvlist_lookup_uint64(char *nvlist, char *name, uint64_t *out)
>>   {
>>       char *nvpair;
>>       size_t size;
>>       int found;
>>       found = nvlist_find_value(nvlist, name, DATA_TYPE_UINT64, 
>> &nvpair, &size, 0);
>>       if (!found)
>>           return 0;
>>       if (size < sizeof(uint64_t)) {
>>           printf("invalid uint64\n");
>>           return ZFS_ERR_BAD_FS;
>>       }
>> +    /* On arm64, calling be64_to_cpu() on a value stored at a memory 
>> address
>> +     * that's not 8-byte aligned causes the CPU to reset. Avoid that 
>> by copying the
>> +     * value somewhere else if needed.
>> +     */
>> +    if (!is_word_aligned_ptr((void *)nvpair)) {
>> +        uint64_t *alignedptr = malloc(sizeof(uint64_t));
>> +        if (!alignedptr)
>> +            return 0;
>> +        memcpy(alignedptr, nvpair, sizeof(uint64_t));
>> +        *out = be64_to_cpu(*alignedptr);
>> +        free(alignedptr);
>> +        return 1;
>> +    }
>> +
>>       *out = be64_to_cpu(*(uint64_t *) nvpair);
>>       return 1;
>>   }
>>   char *
>>   zfs_nvlist_lookup_string(char *nvlist, char *name)
>>   {
>>       char *nvpair;
>>       char *ret;
>>       size_t slen;
> 

-- 
// Caleb (they/them)

Re: [PATCH 0/5] zfs: Fix zfs support on aarch64

[Tom Rini]

On Sat, 06 Apr 2024 18:47:24 -0700, mwleeds@mailtundra.com wrote:

> This patch series is needed to get U-Boot to boot from a ZFS filesystem
> on an aarch64 computer. Some of the patches are not architecture specific
> and would be needed to boot ZFS on other platforms as well. The ZFS
> support in U-Boot hasn't been substantively touched in several years and
> to me it seems like it must have been broken for a long time on all
> platforms, but I have only tested on aarch64.
> 
> [...]

Per Igor's comment and Phaedrus agreement, dropped his Tested-by on the patches
themselves. Then applied to u-boot/master, thanks!

-- 
Tom

Re: [PATCH 3/5] zfs: Fix unaligned read of uint64

[mwleeds]

Hi Caleb,

Thanks for this interesting feedback. I saw these patches were already merged
since you sent this but I added a few thoughts below anyway.

On Friday, April 12th, 2024 at 11:50 AM, Caleb Connolly <caleb.connolly@linaro.org> wrote:

> Hi Phaedrus,
> 
> On 07/04/2024 03:47, mwleeds@mailtundra.com wrote:
> 
> > Without this patch, when trying to boot zfs using U-Boot on a Jetson TX2
> > NX (which is aarch64), I get a CPU reset error like so:
> > 
> > "Synchronous Abort" handler, esr 0x96000021
> > elr: 00000000800c9000 lr : 00000000800c8ffc (reloc)
> > elr: 00000000fff77000 lr : 00000000fff76ffc
> > x0 : 00000000ffb40f04 x1 : 0000000000000000
> > x2 : 000000000000000a x3 : 0000000003100000
> > x4 : 0000000003100000 x5 : 0000000000000034
> > x6 : 00000000fff9cc6e x7 : 000000000000000f
> > x8 : 00000000ff7f84a0 x9 : 0000000000000008
> > x10: 00000000ffb40f04 x11: 0000000000000006
> > x12: 000000000001869f x13: 0000000000000001
> > x14: 00000000ff7f84bc x15: 0000000000000010
> > x16: 0000000000002080 x17: 00000000001fffff
> > x18: 00000000ff7fbdd8 x19: 00000000ffb405f8
> > x20: 00000000ffb40dd0 x21: 00000000fffabe5e
> > x22: 000000ea77940000 x23: 00000000ffb42090
> > x24: 0000000000000000 x25: 0000000000000000
> > x26: 0000000000000000 x27: 0000000000000000
> > x28: 0000000000bab10c x29: 00000000ff7f85f0
> > 
> > Code: d00001a0 9103a000 94006ac6 f9401ba0 (f9400000)
> > Resetting CPU ...
> > 
> > This happens when be64_to_cpu() is called on a value that exists at a
> > memory address that's 4 byte aligned but not 8 byte aligned (e.g. an
> > address ending in 04). The call stack where that happens is:
> > check_pool_label() ->
> > zfs_nvlist_lookup_uint64(vdevnvlist, ZPOOL_CONFIG_ASHIFT,...) ->
> > be64_to_cpu()
> 
> 
> This is very odd, aarch64 doesn't generally have these restrictions. I
> got a bit nerdsniped when I saw this so I did some digging and figured this:
> 
> 1. Your abort exception doesn't include the FAR_ELx register (which
> should contain the address that was being accessed when the abort
> occured). This means your board is running in EL3.
> 2. It turns out there is an "A" flag in the SCTLR_ELx register, when set
> this flag causes a fault when trying to load from an address that isn't
> aligned to the size of the data element (see "A, bit" on
> https://developer.arm.com/documentation/ddi0595/2021-06/AArch64-Registers/SCTLR-EL3--System-Control-Register--EL3-
> 
> I'm not sure who's in the "wrong" here, maybe the driver should avoid
> unaligned accesses? But then again, I don't think you should be running
> a ZFS driver in EL3.
> 
> I'm not familiar with the Jetson Nano, but maybe there's a documented
> way to run U-Boot so that it isn't executing in EL3? Or if not you could
> also try unsetting the A flag.

I may look into this if I get a chance. However if I write some assembly code
to change the execution level or unset the A flag, I worry that the code would
work fine on the hardware I have (Jetson TX2 NX) but behave differently on
another platform. And I don't think I can easily set up testing environments
with u-boot + zfs on different platforms to find out.

> 
> If this really is something to fix in the driver, I don't think
> hotpatching every unaligned access with a malloc() is the right solution.
> 

I'm certainly open to other ideas. The difficulty is the data structure we're
parsing in this file is read from disk and it's only 4-byte aligned.

> > Signed-off-by: Phaedrus Leeds mwleeds@mailtundra.com
> > Tested-by: Phaedrus Leeds mwleeds@mailtundra.com
> 
> 
> regarding your question about re-sending to remove these tags, I'd say
> probably yes, and especially if you're going to send a new revision anyway.
> 
> fwiw you seem to have gotten pretty much everything else about the patch
> submission process spot on :)
> 
> Kind regards,
> 
> > ---
> > fs/zfs/zfs.c | 18 ++++++++++++++++++
> > 1 file changed, 18 insertions(+)
> > 
> > diff --git a/fs/zfs/zfs.c b/fs/zfs/zfs.c
> > index 61d58fce68..9a50deac18 100644
> > --- a/fs/zfs/zfs.c
> > +++ b/fs/zfs/zfs.c
> > @@ -1552,35 +1552,53 @@ nvlist_find_value(char *nvlist, char *name, int valtype, char **val,
> > if (nelm_out)
> > *nelm_out = nelm;
> > return 1;
> > }
> > 
> > nvlist += encode_size; /* goto the next nvpair */
> > }
> > return 0;
> > }
> > 
> > +int is_word_aligned_ptr(void *ptr) {
> > + return ((uintptr_t)ptr & (sizeof(void *) - 1)) == 0;
> > +}
> > +
> > int
> > zfs_nvlist_lookup_uint64(char *nvlist, char *name, uint64_t *out)
> > {
> > char *nvpair;
> > size_t size;
> > int found;
> > 
> > found = nvlist_find_value(nvlist, name, DATA_TYPE_UINT64, &nvpair, &size, 0);
> > if (!found)
> > return 0;
> > if (size < sizeof(uint64_t)) {
> > printf("invalid uint64\n");
> > return ZFS_ERR_BAD_FS;
> > }
> > 
> > + /* On arm64, calling be64_to_cpu() on a value stored at a memory address
> > + * that's not 8-byte aligned causes the CPU to reset. Avoid that by copying the
> > + * value somewhere else if needed.
> > + */
> > + if (!is_word_aligned_ptr((void *)nvpair)) {
> > + uint64_t *alignedptr = malloc(sizeof(uint64_t));
> > + if (!alignedptr)
> > + return 0;
> > + memcpy(alignedptr, nvpair, sizeof(uint64_t));
> > + *out = be64_to_cpu(*alignedptr);
> > + free(alignedptr);
> > + return 1;
> > + }
> > +
> > out = be64_to_cpu((uint64_t *) nvpair);
> > return 1;
> > }
> > 
> > char *
> > zfs_nvlist_lookup_string(char *nvlist, char *name)
> > {
> > char *nvpair;
> > char *ret;
> > size_t slen;
> 
> 
> --
> // Caleb (they/them)

Re: [PATCH 3/5] zfs: Fix unaligned read of uint64

[Caleb Connolly]

On 25/04/2024 06:02, mwleeds@mailtundra.com wrote:
> Hi Caleb,
> 
> Thanks for this interesting feedback. I saw these patches were already merged
> since you sent this but I added a few thoughts below anyway.

Ah, a shame.. It would be good to find a scalable solution to this!
> 
> On Friday, April 12th, 2024 at 11:50 AM, Caleb Connolly <caleb.connolly@linaro.org> wrote:
> 
>> Hi Phaedrus,
>>
>> On 07/04/2024 03:47, mwleeds@mailtundra.com wrote:
>>
>>> Without this patch, when trying to boot zfs using U-Boot on a Jetson TX2
>>> NX (which is aarch64), I get a CPU reset error like so:
>>>
>>> "Synchronous Abort" handler, esr 0x96000021
>>> elr: 00000000800c9000 lr : 00000000800c8ffc (reloc)
>>> elr: 00000000fff77000 lr : 00000000fff76ffc
>>> x0 : 00000000ffb40f04 x1 : 0000000000000000
>>> x2 : 000000000000000a x3 : 0000000003100000
>>> x4 : 0000000003100000 x5 : 0000000000000034
>>> x6 : 00000000fff9cc6e x7 : 000000000000000f
>>> x8 : 00000000ff7f84a0 x9 : 0000000000000008
>>> x10: 00000000ffb40f04 x11: 0000000000000006
>>> x12: 000000000001869f x13: 0000000000000001
>>> x14: 00000000ff7f84bc x15: 0000000000000010
>>> x16: 0000000000002080 x17: 00000000001fffff
>>> x18: 00000000ff7fbdd8 x19: 00000000ffb405f8
>>> x20: 00000000ffb40dd0 x21: 00000000fffabe5e
>>> x22: 000000ea77940000 x23: 00000000ffb42090
>>> x24: 0000000000000000 x25: 0000000000000000
>>> x26: 0000000000000000 x27: 0000000000000000
>>> x28: 0000000000bab10c x29: 00000000ff7f85f0
>>>
>>> Code: d00001a0 9103a000 94006ac6 f9401ba0 (f9400000)
>>> Resetting CPU ...
>>>
>>> This happens when be64_to_cpu() is called on a value that exists at a
>>> memory address that's 4 byte aligned but not 8 byte aligned (e.g. an
>>> address ending in 04). The call stack where that happens is:
>>> check_pool_label() ->
>>> zfs_nvlist_lookup_uint64(vdevnvlist, ZPOOL_CONFIG_ASHIFT,...) ->
>>> be64_to_cpu()
>>
>>
>> This is very odd, aarch64 doesn't generally have these restrictions. I
>> got a bit nerdsniped when I saw this so I did some digging and figured this:
>>
>> 1. Your abort exception doesn't include the FAR_ELx register (which
>> should contain the address that was being accessed when the abort
>> occured). This means your board is running in EL3.
>> 2. It turns out there is an "A" flag in the SCTLR_ELx register, when set
>> this flag causes a fault when trying to load from an address that isn't
>> aligned to the size of the data element (see "A, bit" on
>> https://developer.arm.com/documentation/ddi0595/2021-06/AArch64-Registers/SCTLR-EL3--System-Control-Register--EL3-
>>
>> I'm not sure who's in the "wrong" here, maybe the driver should avoid
>> unaligned accesses? But then again, I don't think you should be running
>> a ZFS driver in EL3.
>>
>> I'm not familiar with the Jetson Nano, but maybe there's a documented
>> way to run U-Boot so that it isn't executing in EL3? Or if not you could
>> also try unsetting the A flag.
> 
> I may look into this if I get a chance. However if I write some assembly code
> to change the execution level or unset the A flag, I worry that the code would
> work fine on the hardware I have (Jetson TX2 NX) but behave differently on

Well, unsetting the A flag might arguably have some niche negative 
outcomes, but I really struggle to see how this is a driver bug to be 
honest...
> another platform. And I don't think I can easily set up testing environments
> with u-boot + zfs on different platforms to find out.

fwiw, if you do intend to investigate this further, I'd be happy to 
validate your assumptions on a Qualcomm board (where we are rather 
boring and running in EL1 or EL2).
> 
>>
>> If this really is something to fix in the driver, I don't think
>> hotpatching every unaligned access with a malloc() is the right solution.
>>
> 
> I'm certainly open to other ideas. The difficulty is the data structure we're
> parsing in this file is read from disk and it's only 4-byte aligned.

According to the ARM docs, the issue is that you're loading an 8-byte 
value from a 4-byte aligned address. If you split it into two 4-byte 
reads (for the upper and lower words) then it should work fine. This 
would avoid the malloc at any rate.

Thanks and regards,
> 
>>> Signed-off-by: Phaedrus Leeds mwleeds@mailtundra.com
>>> Tested-by: Phaedrus Leeds mwleeds@mailtundra.com
>>
>>
>> regarding your question about re-sending to remove these tags, I'd say
>> probably yes, and especially if you're going to send a new revision anyway.
>>
>> fwiw you seem to have gotten pretty much everything else about the patch
>> submission process spot on :)
>>
>> Kind regards,
>>
>>> ---
>>> fs/zfs/zfs.c | 18 ++++++++++++++++++
>>> 1 file changed, 18 insertions(+)
>>>
>>> diff --git a/fs/zfs/zfs.c b/fs/zfs/zfs.c
>>> index 61d58fce68..9a50deac18 100644
>>> --- a/fs/zfs/zfs.c
>>> +++ b/fs/zfs/zfs.c
>>> @@ -1552,35 +1552,53 @@ nvlist_find_value(char *nvlist, char *name, int valtype, char **val,
>>> if (nelm_out)
>>> *nelm_out = nelm;
>>> return 1;
>>> }
>>>
>>> nvlist += encode_size; /* goto the next nvpair */
>>> }
>>> return 0;
>>> }
>>>
>>> +int is_word_aligned_ptr(void *ptr) {
>>> + return ((uintptr_t)ptr & (sizeof(void *) - 1)) == 0;
>>> +}
>>> +
>>> int
>>> zfs_nvlist_lookup_uint64(char *nvlist, char *name, uint64_t *out)
>>> {
>>> char *nvpair;
>>> size_t size;
>>> int found;
>>>
>>> found = nvlist_find_value(nvlist, name, DATA_TYPE_UINT64, &nvpair, &size, 0);
>>> if (!found)
>>> return 0;
>>> if (size < sizeof(uint64_t)) {
>>> printf("invalid uint64\n");
>>> return ZFS_ERR_BAD_FS;
>>> }
>>>
>>> + /* On arm64, calling be64_to_cpu() on a value stored at a memory address
>>> + * that's not 8-byte aligned causes the CPU to reset. Avoid that by copying the
>>> + * value somewhere else if needed.
>>> + */
>>> + if (!is_word_aligned_ptr((void *)nvpair)) {
>>> + uint64_t *alignedptr = malloc(sizeof(uint64_t));
>>> + if (!alignedptr)
>>> + return 0;
>>> + memcpy(alignedptr, nvpair, sizeof(uint64_t));
>>> + *out = be64_to_cpu(*alignedptr);
>>> + free(alignedptr);
>>> + return 1;
>>> + }
>>> +
>>> out = be64_to_cpu((uint64_t *) nvpair);
>>> return 1;
>>> }
>>>
>>> char *
>>> zfs_nvlist_lookup_string(char *nvlist, char *name)
>>> {
>>> char *nvpair;
>>> char *ret;
>>> size_t slen;
>>
>>
>> --
>> // Caleb (they/them)

-- 
// Caleb (they/them)

Re: [PATCH 3/5] zfs: Fix unaligned read of uint64

[Phaedrus Leeds]

On Thursday, April 25th, 2024 at 4:57 AM, Caleb Connolly <caleb.connolly@linaro.org> wrote:

> 
> On 25/04/2024 06:02, mwleeds@mailtundra.com wrote:
> 
> > Hi Caleb,
> > 
> > Thanks for this interesting feedback. I saw these patches were already merged
> > since you sent this but I added a few thoughts below anyway.
> 
> 
> Ah, a shame.. It would be good to find a scalable solution to this!

While I agree your suggestion of doing two 4 byte reads is more scalable than
the malloc() patch, the code path (zfs_nvlist_lookup_uint64) is only hit when
reading the ZPool label (check_pool_label) which is a once-per-boot operation
operating on some metadata, so I don't think scalability is as much a concern
as it would be if we were hitting this code path when reading every block from
the disk for example.

That said, I may try implementing the patch as you suggested if I get a chance.
And in any case I definitely appreciate your engagement and feedback!

> 
> > On Friday, April 12th, 2024 at 11:50 AM, Caleb Connolly caleb.connolly@linaro.org wrote:
> > 
> > > Hi Phaedrus,
> > > 
> > > On 07/04/2024 03:47, mwleeds@mailtundra.com wrote:
> > > 
> > > > Without this patch, when trying to boot zfs using U-Boot on a Jetson TX2
> > > > NX (which is aarch64), I get a CPU reset error like so:
> > > > 
> > > > "Synchronous Abort" handler, esr 0x96000021
> > > > elr: 00000000800c9000 lr : 00000000800c8ffc (reloc)
> > > > elr: 00000000fff77000 lr : 00000000fff76ffc
> > > > x0 : 00000000ffb40f04 x1 : 0000000000000000
> > > > x2 : 000000000000000a x3 : 0000000003100000
> > > > x4 : 0000000003100000 x5 : 0000000000000034
> > > > x6 : 00000000fff9cc6e x7 : 000000000000000f
> > > > x8 : 00000000ff7f84a0 x9 : 0000000000000008
> > > > x10: 00000000ffb40f04 x11: 0000000000000006
> > > > x12: 000000000001869f x13: 0000000000000001
> > > > x14: 00000000ff7f84bc x15: 0000000000000010
> > > > x16: 0000000000002080 x17: 00000000001fffff
> > > > x18: 00000000ff7fbdd8 x19: 00000000ffb405f8
> > > > x20: 00000000ffb40dd0 x21: 00000000fffabe5e
> > > > x22: 000000ea77940000 x23: 00000000ffb42090
> > > > x24: 0000000000000000 x25: 0000000000000000
> > > > x26: 0000000000000000 x27: 0000000000000000
> > > > x28: 0000000000bab10c x29: 00000000ff7f85f0
> > > > 
> > > > Code: d00001a0 9103a000 94006ac6 f9401ba0 (f9400000)
> > > > Resetting CPU ...
> > > > 
> > > > This happens when be64_to_cpu() is called on a value that exists at a
> > > > memory address that's 4 byte aligned but not 8 byte aligned (e.g. an
> > > > address ending in 04). The call stack where that happens is:
> > > > check_pool_label() ->
> > > > zfs_nvlist_lookup_uint64(vdevnvlist, ZPOOL_CONFIG_ASHIFT,...) ->
> > > > be64_to_cpu()
> > > 
> > > This is very odd, aarch64 doesn't generally have these restrictions. I
> > > got a bit nerdsniped when I saw this so I did some digging and figured this:
> > > 
> > > 1. Your abort exception doesn't include the FAR_ELx register (which
> > > should contain the address that was being accessed when the abort
> > > occured). This means your board is running in EL3.
> > > 2. It turns out there is an "A" flag in the SCTLR_ELx register, when set
> > > this flag causes a fault when trying to load from an address that isn't
> > > aligned to the size of the data element (see "A, bit" on
> > > https://developer.arm.com/documentation/ddi0595/2021-06/AArch64-Registers/SCTLR-EL3--System-Control-Register--EL3-
> > > 
> > > I'm not sure who's in the "wrong" here, maybe the driver should avoid
> > > unaligned accesses? But then again, I don't think you should be running
> > > a ZFS driver in EL3.
> > > 
> > > I'm not familiar with the Jetson Nano, but maybe there's a documented
> > > way to run U-Boot so that it isn't executing in EL3? Or if not you could
> > > also try unsetting the A flag.
> > 
> > I may look into this if I get a chance. However if I write some assembly code
> > to change the execution level or unset the A flag, I worry that the code would
> > work fine on the hardware I have (Jetson TX2 NX) but behave differently on
> 
> 
> Well, unsetting the A flag might arguably have some niche negative
> outcomes, but I really struggle to see how this is a driver bug to be
> honest...
> 
> > another platform. And I don't think I can easily set up testing environments
> > with u-boot + zfs on different platforms to find out.
> 
> 
> fwiw, if you do intend to investigate this further, I'd be happy to
> validate your assumptions on a Qualcomm board (where we are rather
> boring and running in EL1 or EL2).

Thanks, I'll keep that in mind

> 
> > > If this really is something to fix in the driver, I don't think
> > > hotpatching every unaligned access with a malloc() is the right solution.
> > 
> > I'm certainly open to other ideas. The difficulty is the data structure we're
> > parsing in this file is read from disk and it's only 4-byte aligned.
> 
> 
> According to the ARM docs, the issue is that you're loading an 8-byte
> value from a 4-byte aligned address. If you split it into two 4-byte
> reads (for the upper and lower words) then it should work fine. This
> would avoid the malloc at any rate.

Makes sense

> 
> Thanks and regards,
> 
> > > > Signed-off-by: Phaedrus Leeds mwleeds@mailtundra.com
> > > > Tested-by: Phaedrus Leeds mwleeds@mailtundra.com
> > > 
> > > regarding your question about re-sending to remove these tags, I'd say
> > > probably yes, and especially if you're going to send a new revision anyway.
> > > 
> > > fwiw you seem to have gotten pretty much everything else about the patch
> > > submission process spot on :)
> > > 
> > > Kind regards,
> > > 
> > > > ---
> > > > fs/zfs/zfs.c | 18 ++++++++++++++++++
> > > > 1 file changed, 18 insertions(+)
> > > > 
> > > > diff --git a/fs/zfs/zfs.c b/fs/zfs/zfs.c
> > > > index 61d58fce68..9a50deac18 100644
> > > > --- a/fs/zfs/zfs.c
> > > > +++ b/fs/zfs/zfs.c
> > > > @@ -1552,35 +1552,53 @@ nvlist_find_value(char *nvlist, char *name, int valtype, char **val,
> > > > if (nelm_out)
> > > > *nelm_out = nelm;
> > > > return 1;
> > > > }
> > > > 
> > > > nvlist += encode_size; /* goto the next nvpair */
> > > > }
> > > > return 0;
> > > > }
> > > > 
> > > > +int is_word_aligned_ptr(void *ptr) {
> > > > + return ((uintptr_t)ptr & (sizeof(void *) - 1)) == 0;
> > > > +}
> > > > +
> > > > int
> > > > zfs_nvlist_lookup_uint64(char *nvlist, char *name, uint64_t *out)
> > > > {
> > > > char *nvpair;
> > > > size_t size;
> > > > int found;
> > > > 
> > > > found = nvlist_find_value(nvlist, name, DATA_TYPE_UINT64, &nvpair, &size, 0);
> > > > if (!found)
> > > > return 0;
> > > > if (size < sizeof(uint64_t)) {
> > > > printf("invalid uint64\n");
> > > > return ZFS_ERR_BAD_FS;
> > > > }
> > > > 
> > > > + /* On arm64, calling be64_to_cpu() on a value stored at a memory address
> > > > + * that's not 8-byte aligned causes the CPU to reset. Avoid that by copying the
> > > > + * value somewhere else if needed.
> > > > + */
> > > > + if (!is_word_aligned_ptr((void *)nvpair)) {
> > > > + uint64_t *alignedptr = malloc(sizeof(uint64_t));
> > > > + if (!alignedptr)
> > > > + return 0;
> > > > + memcpy(alignedptr, nvpair, sizeof(uint64_t));
> > > > + *out = be64_to_cpu(*alignedptr);
> > > > + free(alignedptr);
> > > > + return 1;
> > > > + }
> > > > +
> > > > out = be64_to_cpu((uint64_t *) nvpair);
> > > > return 1;
> > > > }
> > > > 
> > > > char *
> > > > zfs_nvlist_lookup_string(char *nvlist, char *name)
> > > > {
> > > > char *nvpair;
> > > > char *ret;
> > > > size_t slen;
> > > 
> > > --
> > > // Caleb (they/them)
> 
> 
> --
> // Caleb (they/them)

Re: [PATCH 3/5] zfs: Fix unaligned read of uint64

[mwleeds]

On Thursday, April 25th, 2024 at 4:57 AM, Caleb Connolly <caleb.connolly@linaro.org> wrote:

>
> On 25/04/2024 06:02, mwleeds@mailtundra.com wrote:
>
> > Hi Caleb,
> >
> > Thanks for this interesting feedback. I saw these patches were already merged
> > since you sent this but I added a few thoughts below anyway.
>
>
> Ah, a shame.. It would be good to find a scalable solution to this!

While I agree your suggestion of doing two 4 byte reads is more scalable than
the malloc() patch, the code path (zfs_nvlist_lookup_uint64) is only hit when
reading the ZPool label (check_pool_label) which is a once-per-boot operation
operating on some metadata, so I don't think scalability is as much a concern
as it would be if we were hitting this code path when reading every block from
the disk for example.

That said, I may try implementing the patch as you suggested if I get a chance.
And in any case I definitely appreciate your engagement and feedback!

>
> > On Friday, April 12th, 2024 at 11:50 AM, Caleb Connolly caleb.connolly@linaro.org wrote:
> >
> > > Hi Phaedrus,
> > >
> > > On 07/04/2024 03:47, mwleeds@mailtundra.com wrote:
> > >
> > > > Without this patch, when trying to boot zfs using U-Boot on a Jetson TX2
> > > > NX (which is aarch64), I get a CPU reset error like so:
> > > >
> > > > "Synchronous Abort" handler, esr 0x96000021
> > > > elr: 00000000800c9000 lr : 00000000800c8ffc (reloc)
> > > > elr: 00000000fff77000 lr : 00000000fff76ffc
> > > > x0 : 00000000ffb40f04 x1 : 0000000000000000
> > > > x2 : 000000000000000a x3 : 0000000003100000
> > > > x4 : 0000000003100000 x5 : 0000000000000034
> > > > x6 : 00000000fff9cc6e x7 : 000000000000000f
> > > > x8 : 00000000ff7f84a0 x9 : 0000000000000008
> > > > x10: 00000000ffb40f04 x11: 0000000000000006
> > > > x12: 000000000001869f x13: 0000000000000001
> > > > x14: 00000000ff7f84bc x15: 0000000000000010
> > > > x16: 0000000000002080 x17: 00000000001fffff
> > > > x18: 00000000ff7fbdd8 x19: 00000000ffb405f8
> > > > x20: 00000000ffb40dd0 x21: 00000000fffabe5e
> > > > x22: 000000ea77940000 x23: 00000000ffb42090
> > > > x24: 0000000000000000 x25: 0000000000000000
> > > > x26: 0000000000000000 x27: 0000000000000000
> > > > x28: 0000000000bab10c x29: 00000000ff7f85f0
> > > >
> > > > Code: d00001a0 9103a000 94006ac6 f9401ba0 (f9400000)
> > > > Resetting CPU ...
> > > >
> > > > This happens when be64_to_cpu() is called on a value that exists at a
> > > > memory address that's 4 byte aligned but not 8 byte aligned (e.g. an
> > > > address ending in 04). The call stack where that happens is:
> > > > check_pool_label() ->
> > > > zfs_nvlist_lookup_uint64(vdevnvlist, ZPOOL_CONFIG_ASHIFT,...) ->
> > > > be64_to_cpu()
> > >
> > > This is very odd, aarch64 doesn't generally have these restrictions. I
> > > got a bit nerdsniped when I saw this so I did some digging and figured this:
> > >
> > > 1. Your abort exception doesn't include the FAR_ELx register (which
> > > should contain the address that was being accessed when the abort
> > > occured). This means your board is running in EL3.
> > > 2. It turns out there is an "A" flag in the SCTLR_ELx register, when set
> > > this flag causes a fault when trying to load from an address that isn't
> > > aligned to the size of the data element (see "A, bit" on
> > > https://developer.arm.com/documentation/ddi0595/2021-06/AArch64-Registers/SCTLR-EL3--System-Control-Register--EL3-
> > >
> > > I'm not sure who's in the "wrong" here, maybe the driver should avoid
> > > unaligned accesses? But then again, I don't think you should be running
> > > a ZFS driver in EL3.
> > >
> > > I'm not familiar with the Jetson Nano, but maybe there's a documented
> > > way to run U-Boot so that it isn't executing in EL3? Or if not you could
> > > also try unsetting the A flag.
> >
> > I may look into this if I get a chance. However if I write some assembly code
> > to change the execution level or unset the A flag, I worry that the code would
> > work fine on the hardware I have (Jetson TX2 NX) but behave differently on
>
>
> Well, unsetting the A flag might arguably have some niche negative
> outcomes, but I really struggle to see how this is a driver bug to be
> honest...
>
> > another platform. And I don't think I can easily set up testing environments
> > with u-boot + zfs on different platforms to find out.
>
>
> fwiw, if you do intend to investigate this further, I'd be happy to
> validate your assumptions on a Qualcomm board (where we are rather
> boring and running in EL1 or EL2).

Thanks, I'll keep that in mind

>
> > > If this really is something to fix in the driver, I don't think
> > > hotpatching every unaligned access with a malloc() is the right solution.
> >
> > I'm certainly open to other ideas. The difficulty is the data structure we're
> > parsing in this file is read from disk and it's only 4-byte aligned.
>
>
> According to the ARM docs, the issue is that you're loading an 8-byte
> value from a 4-byte aligned address. If you split it into two 4-byte
> reads (for the upper and lower words) then it should work fine. This
> would avoid the malloc at any rate.

Makes sense

>
> Thanks and regards,
>
> > > > Signed-off-by: Phaedrus Leeds mwleeds@mailtundra.com
> > > > Tested-by: Phaedrus Leeds mwleeds@mailtundra.com
> > >
> > > regarding your question about re-sending to remove these tags, I'd say
> > > probably yes, and especially if you're going to send a new revision anyway.
> > >
> > > fwiw you seem to have gotten pretty much everything else about the patch
> > > submission process spot on :)
> > >
> > > Kind regards,
> > >
> > > > ---
> > > > fs/zfs/zfs.c | 18 ++++++++++++++++++
> > > > 1 file changed, 18 insertions(+)
> > > >
> > > > diff --git a/fs/zfs/zfs.c b/fs/zfs/zfs.c
> > > > index 61d58fce68..9a50deac18 100644
> > > > --- a/fs/zfs/zfs.c
> > > > +++ b/fs/zfs/zfs.c
> > > > @@ -1552,35 +1552,53 @@ nvlist_find_value(char *nvlist, char *name, int valtype, char **val,
> > > > if (nelm_out)
> > > > *nelm_out = nelm;
> > > > return 1;
> > > > }
> > > >
> > > > nvlist += encode_size; /* goto the next nvpair */
> > > > }
> > > > return 0;
> > > > }
> > > >
> > > > +int is_word_aligned_ptr(void *ptr) {
> > > > + return ((uintptr_t)ptr & (sizeof(void *) - 1)) == 0;
> > > > +}
> > > > +
> > > > int
> > > > zfs_nvlist_lookup_uint64(char *nvlist, char *name, uint64_t *out)
> > > > {
> > > > char *nvpair;
> > > > size_t size;
> > > > int found;
> > > >
> > > > found = nvlist_find_value(nvlist, name, DATA_TYPE_UINT64, &nvpair, &size, 0);
> > > > if (!found)
> > > > return 0;
> > > > if (size < sizeof(uint64_t)) {
> > > > printf("invalid uint64\n");
> > > > return ZFS_ERR_BAD_FS;
> > > > }
> > > >
> > > > + /* On arm64, calling be64_to_cpu() on a value stored at a memory address
> > > > + * that's not 8-byte aligned causes the CPU to reset. Avoid that by copying the
> > > > + * value somewhere else if needed.
> > > > + */
> > > > + if (!is_word_aligned_ptr((void *)nvpair)) {
> > > > + uint64_t *alignedptr = malloc(sizeof(uint64_t));
> > > > + if (!alignedptr)
> > > > + return 0;
> > > > + memcpy(alignedptr, nvpair, sizeof(uint64_t));
> > > > + *out = be64_to_cpu(*alignedptr);
> > > > + free(alignedptr);
> > > > + return 1;
> > > > + }
> > > > +
> > > > out = be64_to_cpu((uint64_t *) nvpair);
> > > > return 1;
> > > > }
> > > >
> > > > char *
> > > > zfs_nvlist_lookup_string(char *nvlist, char *name)
> > > > {
> > > > char *nvpair;
> > > > char *ret;
> > > > size_t slen;
> > >
> > > --
> > > // Caleb (they/them)
>
>
> --
> // Caleb (they/them)