fdt_support: fix unaligned u64 accesses in fdt_fixup_memory_banks
PATCH 1/1 fdt_support: fix unaligned u64 accesses in fdt_fixup_memory_banks
From: Rob Herring <[email protected]>
Commit f18295d3837c282f (fdt_support: fix an endian bug of fdt_fixup_memory_banks) changed fdt_fixup_memory_banks cell writing from a byte at a time to casting the buffer pointer to a 64-bit pointer. This can result in unaligned accesses when there is a mixture of cell sizes of 1 and 2.
--- common/fdt_support.c | 11 +++++++---- 1 file changed, 7 insertions(+), 4 deletions(-)
common/fdt_support.c +7 -4
@@ -388,19 +388,22 @@ static int fdt_pack_reg(const void *fdt, void *buf, uint64_t *address,int i;int address_len =get_cells_len (fdt,"#address-cells" );int size_len =get_cells_len (fdt,"#size-cells" ); +u64 cell;char *p = buf;for (i =0 ; i < n; i++) {if (address_len ==8 ) - *(fdt64_t *)p =cpu_to_fdt64 (address[i]); + cell =cpu_to_fdt64 (address[i]);else - *(fdt32_t *)p =cpu_to_fdt32 (address[i]); + cell =cpu_to_fdt32 (address[i]); +memcpy (p, &cell, address_len); p += address_len;if (size_len ==8 ) - *(fdt64_t *)p =cpu_to_fdt64 (size[i]); + cell =cpu_to_fdt64 (size[i]);else - *(fdt32_t *)p =cpu_to_fdt32 (size[i]); + cell =cpu_to_fdt32 (size[i]); +memcpy (p, &cell, size_len); p += size_len; }
Signature · 1 line
1.9.1
PATCH 1/1 fdt_support: fix unaligned u64 accesses in fdt_fixup_memory_banks
+Tom
Hi Rob,
Quoted 7 lines · Rob Herring
From: Rob Herring <[email protected]>
Commit
f18295d3837c282f(fdt_support: fix an endian bug offdt_fixup_memory_banks) changedfdt_fixup_memory_bankscell writing from a byte at a time to casting the buffer pointer to a 64-bit pointer. This can result in unaligned accesses when there is a mixture of cell sizes of 1 and 2.
Should that be 739a01ed8e0?
Quoted 25 lines
Cc Masahiro Yamada <[email protected]>Signed-off-by Rob Herring <[email protected]>--- common/fdt_support.c | 11 +++++++---- 1 file changed, 7 insertions(+), 4 deletions(-)common/fdt_support.c +3
-2@@ -388,19 +388,22 @@ static int fdt_pack_reg(const void *fdt, void *buf, uint64_t *address,int i;int address_len =get_cells_len (fdt,"#address-cells" );int size_len =get_cells_len (fdt,"#size-cells" ); +u64 cell;char *p = buf;for (i =0 ; i < n; i++) {if (address_len ==8 ) - *(fdt64_t *)p =cpu_to_fdt64 (address[i]); + cell =cpu_to_fdt64 (address[i]);else - *(fdt32_t *)p =cpu_to_fdt32 (address[i]); + cell =cpu_to_fdt32 (address[i]);
common/fdt_support.c static int fdt_pack_reg(const void *fdt, void *buf, uint64_t *address, · line 406
Quoted 10 lines
common/fdt_support.c +4
-2+memcpy (p, &cell, address_len); p += address_len;if (size_len ==8 ) - *(fdt64_t *)p =cpu_to_fdt64 (size[i]); + cell =cpu_to_fdt64 (size[i]);else - *(fdt32_t *)p =cpu_to_fdt32 (size[i]); + cell =cpu_to_fdt32 (size[i]); +memcpy (p, &cell, size_len);
What will this do with 32-bit values? Aren't use assuming that the first 32-bit word of 'cell' will contain the least significant 32 bits? Is that always true? I'm not quite sure.
Also I really thing this needs a test. It's a pretty simple function so a unit test would be easy to write.
common/fdt_support.c static int fdt_pack_reg(const void *fdt, void *buf, uint64_t *address, · line 408
Quoted 2 lines
common/fdt_support.c +0
-0p += size_len; }
Regards,
Simon
PATCH 1/1 fdt_support: fix unaligned u64 accesses in fdt_fixup_memory_banks
Quoted 7 lines · Simon Glass
+Tom
Hi Rob,
Quoted 2 lines · Rob Herring
From: Rob Herring <[email protected]>
Quoted 7 lines
Quoted 5 lines
Commit
f18295d3837c282f(fdt_support: fix an endian bug offdt_fixup_memory_banks) changedfdt_fixup_memory_bankscell writing from a byte at a time to casting the buffer pointer to a 64-bit pointer. This can result in unaligned accesses when there is a mixture of cell sizes of 1 and 2.Should that be
739a01ed8e0?
Uhh, yes.
Quoted 29 lines
Quoted 29 lines
Cc Masahiro Yamada <[email protected]>Signed-off-by Rob Herring <[email protected]>--- common/fdt_support.c | 11 +++++++---- 1 file changed, 7 insertions(+), 4 deletions(-)common/fdt_support.c +4
-2@@ -388,19 +388,22 @@ static int fdt_pack_reg(const void *fdt, void *buf, uint64_t *address,int i;int address_len =get_cells_len (fdt,"#address-cells" );int size_len =get_cells_len (fdt,"#size-cells" ); +u64 cell;char *p = buf;for (i =0 ; i < n; i++) {if (address_len ==8 ) - *(fdt64_t *)p =cpu_to_fdt64 (address[i]); + cell =cpu_to_fdt64 (address[i]);else - *(fdt32_t *)p =cpu_to_fdt32 (address[i]); + cell =cpu_to_fdt32 (address[i]); +memcpy (p, &cell, address_len); p += address_len;
Quoted 11 lines
common/fdt_support.c static int fdt_pack_reg(const void *fdt, void *buf, uint64_t *address, · line 406
Quoted 7 lines
common/fdt_support.c +3
-2if (size_len ==8 ) - *(fdt64_t *)p =cpu_to_fdt64 (size[i]); + cell =cpu_to_fdt64 (size[i]);else - *(fdt32_t *)p =cpu_to_fdt32 (size[i]); + cell =cpu_to_fdt32 (size[i]); +memcpy (p, &cell, size_len);What will this do with 32-bit values? Aren't use assuming that the first 32-bit word of 'cell' will contain the least significant 32 bits? Is that always true? I'm not quite sure.
We've already done the endian conversion, so we're just copying a string of bytes only changing the alignment potentially.
Quoted 3 lines
Also I really thing this needs a test. It's a pretty simple function so a unit test would be easy to write.
I'll look into that.
Rob
Quoted 6 lines
common/fdt_support.c static int fdt_pack_reg(const void *fdt, void *buf, uint64_t *address, · line 408
Quoted 3 lines
common/fdt_support.c +0
-0p += size_len; }Regards,
Simon
PATCH 1/1 fdt_support: fix unaligned u64 accesses in fdt_fixup_memory_banks
Hi Rob,
Quoted 59 lines · Rob Herring
Quoted 7 lines · Simon Glass
+Tom
Hi Rob,
Quoted 2 lines · Rob Herring
From: Rob Herring <[email protected]>
Quoted 7 lines
Quoted 5 lines
Commit
f18295d3837c282f(fdt_support: fix an endian bug offdt_fixup_memory_banks) changedfdt_fixup_memory_bankscell writing from a byte at a time to casting the buffer pointer to a 64-bit pointer. This can result in unaligned accesses when there is a mixture of cell sizes of 1 and 2.Should that be
739a01ed8e0?Uhh, yes.
Quoted 29 lines
Quoted 29 lines
Cc Masahiro Yamada <[email protected]>Signed-off-by Rob Herring <[email protected]>--- common/fdt_support.c | 11 +++++++---- 1 file changed, 7 insertions(+), 4 deletions(-)common/fdt_support.c +4
-2@@ -388,19 +388,22 @@ static int fdt_pack_reg(const void *fdt, void *buf, uint64_t *address,int i;int address_len =get_cells_len (fdt,"#address-cells" );int size_len =get_cells_len (fdt,"#size-cells" ); +u64 cell;char *p = buf;for (i =0 ; i < n; i++) {if (address_len ==8 ) - *(fdt64_t *)p =cpu_to_fdt64 (address[i]); + cell =cpu_to_fdt64 (address[i]);else - *(fdt32_t *)p =cpu_to_fdt32 (address[i]); + cell =cpu_to_fdt32 (address[i]); +memcpy (p, &cell, address_len); p += address_len;Quoted 11 lines
common/fdt_support.c static int fdt_pack_reg(const void *fdt, void *buf, uint64_t *address, · line 406
Quoted 7 lines
common/fdt_support.c +3
-2if (size_len ==8 ) - *(fdt64_t *)p =cpu_to_fdt64 (size[i]); + cell =cpu_to_fdt64 (size[i]);else - *(fdt32_t *)p =cpu_to_fdt32 (size[i]); + cell =cpu_to_fdt32 (size[i]); +memcpy (p, &cell, size_len);What will this do with 32-bit values? Aren't use assuming that the first 32-bit word of 'cell' will contain the least significant 32 bits? Is that always true? I'm not quite sure.
Quoted 2 lines
We've already done the endian conversion, so we're just copying a string of bytes only changing the alignment potentially.
Yes I think you are right. Then I suggest adding a comment here, as memcpy() from a native type is unusual.
Quoted 6 lines
Quoted 4 lines
Also I really thing this needs a test. It's a pretty simple function so a unit test would be easy to write.
I'll look into that.
See test/command_ut.c for one way to do this with sandbox by adding a new command.
Regards,
Simon
PATCH 1/1 fdt_support: fix unaligned u64 accesses in fdt_fixup_memory_banks
Quoted 62 lines · Simon Glass
Hi Rob,
Quoted 59 lines · Rob Herring
Quoted 7 lines · Simon Glass
+Tom
Hi Rob,
Quoted 2 lines · Rob Herring
From: Rob Herring <[email protected]>
Quoted 7 lines
Quoted 5 lines
Commit
f18295d3837c282f(fdt_support: fix an endian bug offdt_fixup_memory_banks) changedfdt_fixup_memory_bankscell writing from a byte at a time to casting the buffer pointer to a 64-bit pointer. This can result in unaligned accesses when there is a mixture of cell sizes of 1 and 2.Should that be
739a01ed8e0?Uhh, yes.
Quoted 40 lines
Quoted 26 lines
Cc Masahiro Yamada <[email protected]>Signed-off-by Rob Herring <[email protected]>--- common/fdt_support.c | 11 +++++++---- 1 file changed, 7 insertions(+), 4 deletions(-)common/fdt_support.c +3
-2@@ -388,19 +388,22 @@ static int fdt_pack_reg(const void *fdt, void *buf, uint64_t *address,int i;int address_len =get_cells_len (fdt,"#address-cells" );int size_len =get_cells_len (fdt,"#size-cells" ); +u64 cell;char *p = buf;for (i =0 ; i < n; i++) {if (address_len ==8 ) - *(fdt64_t *)p =cpu_to_fdt64 (address[i]); + cell =cpu_to_fdt64 (address[i]);else - *(fdt32_t *)p =cpu_to_fdt32 (address[i]); + cell =cpu_to_fdt32 (address[i]);common/fdt_support.c static int fdt_pack_reg(const void *fdt, void *buf, uint64_t *address, · line 406
Quoted 10 lines
common/fdt_support.c +4
-2+memcpy (p, &cell, address_len); p += address_len;if (size_len ==8 ) - *(fdt64_t *)p =cpu_to_fdt64 (size[i]); + cell =cpu_to_fdt64 (size[i]);else - *(fdt32_t *)p =cpu_to_fdt32 (size[i]); + cell =cpu_to_fdt32 (size[i]); +memcpy (p, &cell, size_len);What will this do with 32-bit values? Aren't use assuming that the first 32-bit word of 'cell' will contain the least significant 32 bits? Is that always true? I'm not quite sure.
Quoted 5 lines
Quoted 2 lines
We've already done the endian conversion, so we're just copying a string of bytes only changing the alignment potentially.
Yes I think you are right. Then I suggest adding a comment here, as
memcpy()from a native type is unusual.
Better yet, use put_unaligned().
Signature · 2 lines
M?ns Rullg?rd mans at mansr.com