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

Albert ARIBAUD
2013-10-14T13:00:02Z

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 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@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:
>
> 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.