[U-Boot] [PATCH] disk:efi: avoid unaligned access on efi partition

[Albert ARIBAUD]

Hi Måns,

On Mon, 14 Oct 2013 11:50:42 +0100, Måns Rullgård <mans at mansr.com>
wrote:

> Piotr Wilczek <p.wilczek at samsung.com> writes:
> 
> >> -----Original Message-----
> >> From: Måns Rullgård [mailto:mans at mansr.com]
> >> Sent: Saturday, October 12, 2013 1:29 AM
> >> To: Piotr Wilczek
> >> Cc: u-boot at lists.denx.de; Tom Rini; Kyungmin Park
> >> Subject: Re: [PATCH] disk:efi: avoid unaligned access on efi partition
> >> 
> >> Piotr Wilczek <p.wilczek at 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 at samsung.com>
> >> > Signed-off-by: Kyungmin Park <kyungmin.park at samsung.com>
> >> > CC: Tom Rini <trini at 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.

Amicalement,
-- 
Albert.