From mboxrd@z Thu Jan 1 00:00:00 1970 From: Albert ARIBAUD Date: Mon, 14 Oct 2013 15:00:02 +0200 Subject: [U-Boot] [PATCH] disk:efi: avoid unaligned access on efi partition In-Reply-To: References: <1381498270-24342-1-git-send-email-p.wilczek@samsung.com> <000601cec8b7$ae803050$0b8090f0$%wilczek@samsung.com> <20131014134626.7e691591@lilith> Message-ID: <20131014150002.2b1cfe82@lilith> List-Id: MIME-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit To: u-boot@lists.denx.de Hi M?ns, On Mon, 14 Oct 2013 13:19:27 +0100, M?ns Rullg?rd wrote: > Albert ARIBAUD writes: > > > Hi M?ns, > > > > On Mon, 14 Oct 2013 11:50:42 +0100, M?ns Rullg?rd > > wrote: > > > >> Piotr Wilczek 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 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 > >> >> > Signed-off-by: Kyungmin Park > >> >> > CC: Tom Rini > >> >> > --- > >> >> > 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.