Albert ARIBAUD <albert.u.b...@aribaud.net> writes:
> Hi Måns,
>
> On Mon, 14 Oct 2013 11:50:42 +0100, Måns Rullgård <m...@mansr.com>
> wrote:
>
>> Piotr Wilczek <p.wilc...@samsung.com> writes:
>>
>> >> -----Original Message-----
>> >> From: Måns Rullgård [mailto:m...@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.wilc...@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.wilc...@samsung.com>
>> >> > Signed-off-by: Kyungmin Park <kyungmin.p...@samsung.com>
>> >> > CC: Tom Rini <tr...@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.
--
Måns Rullgård
m...@mansr.com
_______________________________________________
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot
