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

[Albert ARIBAUD]

Hi Måns,

On Mon, 14 Oct 2013 13:19:27 +0100, Måns Rullgård <mans at mansr.com>
wrote:

> Albert ARIBAUD <albert.u.boot at aribaud.net> writes:
> 
> > 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.
> 
> I'm advising no such thing.  I said two things:
> 
> 1.  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.
> 
> 2.  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,
-- 
Albert.