Hi Måns,
On Mon, 14 Oct 2013 13:19:27 +0100, Måns Rullgård mans@mansr.com wrote:
Albert ARIBAUD albert.u.boot@aribaud.net writes:
Hi Måns,
On Mon, 14 Oct 2013 11:50:42 +0100, Måns Rullgård mans@mansr.com wrote:
Piotr Wilczek p.wilczek@samsung.com writes:
-----Original Message----- From: Måns Rullgård [mailto:mans@mansr.com] Sent: Saturday, October 12, 2013 1:29 AM To: Piotr Wilczek Cc: u-boot@lists.denx.de; Tom Rini; Kyungmin Park Subject: Re: [PATCH] disk:efi: avoid unaligned access on efi partition
Piotr Wilczek p.wilczek@samsung.com writes:
In this patch static variable and memcpy instead of an assignment are used to avoid unaligned access exception on some ARM platforms.
Signed-off-by: Piotr Wilczek p.wilczek@samsung.com Signed-off-by: Kyungmin Park kyungmin.park@samsung.com CC: Tom Rini trini@ti.com
disk/part_efi.c | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-)
diff --git a/disk/part_efi.c b/disk/part_efi.c index b7524d6..303b8af 100644 --- a/disk/part_efi.c +++ b/disk/part_efi.c @@ -224,7 +224,8 @@ static int set_protective_mbr(block_dev_desc_t
*dev_desc)
p_mbr->signature = MSDOS_MBR_SIGNATURE; p_mbr->partition_record[0].sys_ind = EFI_PMBR_OSTYPE_EFI_GPT; p_mbr->partition_record[0].start_sect = 1;
- p_mbr->partition_record[0].nr_sects = (u32) dev_desc->lba;
- memcpy(&p_mbr->partition_record[0].nr_sects, &dev_desc->lba,
sizeof(dev_desc->lba));Why is this assignment problematic? Note that the compiler may optimise the memcpy() call into a plain assignment including any alignment assumptions it was making in the original code.
The correct fix is either to ensure that pointers are properly aligned or that things are annotated as potentially unaligned, whichever is more appropriate.
Problem is that the legacy_mbr structure consists 'le16 unknown' field before 'partition_record' filed and the structure is packed. As a result the address of 'partition_record' filed is halfword aligned. The compiler uses str/ldr instructions (address must be 4-byte aligned) to copy u32 'lba' data thus causing unaligned access exception.
If the struct has __attribute__((packed)), gcc should do the right thing already. Note that on ARMv6 and later ldr/str support unaligned addresses unless this is explicitly disabled in the system control register. If you do this, you _MUST_ compile with -mno-unaligned-access. Otherwise you will get problems.
Please do not advise using native unaligned accesses on code that is not strictly used by ARMv6+ architectures: the present code, for instance, might be run on pre-ARMv6 or non-ARM platforms, and thus, should never assume ability to perform unaligned accesses natively.
I'm advising no such thing. I said two things:
Declaring a struct with the 'packed' attribute makes gcc automatically generate correct code for all targets. _IF_ the selected target supports unaligned ldr/str, these might get used.
If your target is ARMv6 or later _AND_ you enable strict alignment checking in the system control register, you _MUST_ build with the -mno-unaligned-access flag.
Then I apologize; I had read "Note that on ARMv6 and later ldr/str support unaligned addresses unless this is explicitly disabled in the system control register" as a suggestion to use that capability.
Amicalement,