| Message ID | 1381498270-24342-1-git-send-email-p.wilczek@samsung.com |
|---|---|
| State | Changes Requested |
| Delegated to: | Tom Rini |
| Headers | show |
Hi Piotr, On Fri, 11 Oct 2013 15:31:10 +0200, Piotr Wilczek <p.wilczek@samsung.com> wrote: > 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)); > > /* Write MBR sector to the MMC device */ > if (dev_desc->block_write(dev_desc->dev, 0, 1, p_mbr) != 1) { > @@ -387,8 +388,9 @@ int gpt_fill_pte(gpt_header *gpt_h, gpt_entry *gpt_e, > gpt_e[i].ending_lba = cpu_to_le64(offset - 1); > > /* partition type GUID */ > + static efi_guid_t basic_guid = PARTITION_BASIC_DATA_GUID; > memcpy(gpt_e[i].partition_type_guid.b, > - &PARTITION_BASIC_DATA_GUID, 16); > + &basic_guid, 16); Usually, an all-caps symbol is a macro, which makes taking its address a no-no. Besides, doing a memcpy() for 32-bit quantities seems like overkill for me. Any reason you cannot simply use asm/unaligned.h and either get_unaligned or put_unaligned depending on where the alignment issue lies? > #ifdef CONFIG_PARTITION_UUIDS > str_uuid = partitions[i].uuid; Amicalement,
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.
> -----Original Message----- > From: Måns Rullgård [mailto:mans@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.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. The best option would be to rearrange field in the structure but for other reasons I cannot do that. I will use put/get_unaligned as Albert suggested. Best regards Piotr Wilczek > -- > Måns Rullgård > mans@mansr.com
Piotr Wilczek <p.wilczek@samsung.com> writes: >> -----Original Message----- >> From: Måns Rullgård [mailto:mans@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.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.
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@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.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. Amicalement,
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@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.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.
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@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.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 ARIBAUD <albert.u.boot@aribaud.net> writes: >> > 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. If building for ARMv6 or later, I do suggest allowing unaligned accesses. The moment you add -march=armv6 (or equivalent), you allow for a number of things not supported by older versions, so why not unaligned memory accesses?
Hi Måns, On Mon, 14 Oct 2013 14:05:13 +0100, Måns Rullgård <mans@mansr.com> wrote: > Albert ARIBAUD <albert.u.boot@aribaud.net> writes: > > >> > 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. > > If building for ARMv6 or later, I do suggest allowing unaligned > accesses. The moment you add -march=armv6 (or equivalent), you allow > for a number of things not supported by older versions, so why not > unaligned memory accesses? doc/README.arm-unaligned-accesses probably has the answer to your question, especially from line 21 onward. If not, I'll be happy to provide further clarification. Amicalement,
Dear Albert and Måns, > -----Original Message----- > From: Måns Rullgård [mailto:mans@mansr.com] > Sent: Monday, October 14, 2013 3:05 PM > To: Albert ARIBAUD > Cc: Piotr Wilczek; 'Tom Rini'; u-boot@lists.denx.de; 'Kyungmin Park' > Subject: Re: [U-Boot] [PATCH] disk:efi: avoid unaligned access on efi > partition > > Albert ARIBAUD <albert.u.boot@aribaud.net> writes: > > >> > 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. > > If building for ARMv6 or later, I do suggest allowing unaligned > accesses. The moment you add -march=armv6 (or equivalent), you allow > for a number of things not supported by older versions, so why not > unaligned memory accesses? > Thank you for your comments, I will follow your advice. Best regards Piotr Wilczek > -- > Måns Rullgård > mans@mansr.com
Albert ARIBAUD <albert.u.boot@aribaud.net> writes: > Hi Måns, > > On Mon, 14 Oct 2013 14:05:13 +0100, Måns Rullgård <mans@mansr.com> > wrote: > >> Albert ARIBAUD <albert.u.boot@aribaud.net> writes: >> >> >> > 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. >> >> If building for ARMv6 or later, I do suggest allowing unaligned >> accesses. The moment you add -march=armv6 (or equivalent), you allow >> for a number of things not supported by older versions, so why not >> unaligned memory accesses? > > doc/README.arm-unaligned-accesses probably has the answer to your > question, especially from line 21 onward. If not, I'll be happy to > provide further clarification. That is about as backwards as it can get. By adding -munaligned-access you are telling gcc that unaligned accesses are supported and welcome. With this setting enabled, gcc can and will generate unaligned accesses just about anywhere. This setting is NOT compatible with the SCTRL.A bit being set (strict hardware alignment checking). You cannot enable strict alignment checking in hardware, tell the compiler unaligned accesses are OK, and then expect not to have problems. This is no more a valid combination than allowing floating-point instructions when the target has no FPU.
Hi Piotr, On Mon, 14 Oct 2013 15:49:43 +0200, Piotr Wilczek <p.wilczek@samsung.com> wrote: > Dear Albert and Måns, > > > -----Original Message----- > > From: Måns Rullgård [mailto:mans@mansr.com] > > Sent: Monday, October 14, 2013 3:05 PM > > To: Albert ARIBAUD > > Cc: Piotr Wilczek; 'Tom Rini'; u-boot@lists.denx.de; 'Kyungmin Park' > > Subject: Re: [U-Boot] [PATCH] disk:efi: avoid unaligned access on efi > > partition > > > > Albert ARIBAUD <albert.u.boot@aribaud.net> writes: > > > > >> > 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. > > > > If building for ARMv6 or later, I do suggest allowing unaligned > > accesses. The moment you add -march=armv6 (or equivalent), you allow > > for a number of things not supported by older versions, so why not > > unaligned memory accesses? > > > > Thank you for your comments, I will follow your advice. I will NAK building part_efi.c with options to allow native unaligned access. I will, OTOH, be ok with explicit unaligned reads/writes. > Best regards > Piotr Wilczek Amicalement,
Hi Måns, On Mon, 14 Oct 2013 15:09:39 +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 14:05:13 +0100, Måns Rullgård <mans@mansr.com> > > wrote: > > > >> Albert ARIBAUD <albert.u.boot@aribaud.net> writes: > >> > >> >> > 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. > >> > >> If building for ARMv6 or later, I do suggest allowing unaligned > >> accesses. The moment you add -march=armv6 (or equivalent), you allow > >> for a number of things not supported by older versions, so why not > >> unaligned memory accesses? > > > > doc/README.arm-unaligned-accesses probably has the answer to your > > question, especially from line 21 onward. If not, I'll be happy to > > provide further clarification. > > That is about as backwards as it can get. By adding -munaligned-access > you are telling gcc that unaligned accesses are supported and welcome. > With this setting enabled, gcc can and will generate unaligned accesses > just about anywhere. This setting is NOT compatible with the SCTRL.A > bit being set (strict hardware alignment checking). > > You cannot enable strict alignment checking in hardware, tell the > compiler unaligned accesses are OK, and then expect not to have > problems. This is no more a valid combination than allowing > floating-point instructions when the target has no FPU. I sense that you have not understood the reason why I want alignment checking enabled in ARM yet also want ARMv6+ builds to emit native unaligned accesses if they consider it needed. The reason is, if we prevent ARMv6 builds from using native unaligned accesses, they would replace *all* such accesses with smaller, aligned, ones, which would not trigger a data abort; even those unaligned accesses cased by programming errors. Whereas if we allow ARMv6+ builds to use native unaligned accesses, yet enable alignment checks, then any native unaligned access will be caught as early as possible, and we'll find and cure the issue faster. This is, of course, assuming we don't voluntarily do native unaligned accesses, and in U-Boot, we indeed don't: in U-Boot, accesses are done on natural alignments. Since I have set up this policy, experience (and it has been a while) shows that very few problems arise from alignment checks + native unaligned accesses. These roughly come from hardware- or standards- mandated unaligned fields, in which case they are worth explicitly accessing with "unaligned" macros, or from programming errors, which should be fixed. Another benefit of it is, if the code builds and runs in mainline with alignment checks *and* native unaligned accesses enabled, then it builds and runs also if you disable either one; whereas code that builds and runs with alignment checks or native unaligned accesses disabled might fail if both are enabled. And I don't see what we would gain by going from a strict "natural alignment only" policy to a relaxed "unalignment allowed" one. Amicalement,
Albert ARIBAUD <albert.u.boot@aribaud.net> writes: > Hi Måns, > > On Mon, 14 Oct 2013 15:09:39 +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 14:05:13 +0100, Måns Rullgård <mans@mansr.com> >> > wrote: >> > >> >> Albert ARIBAUD <albert.u.boot@aribaud.net> writes: >> >> >> >> >> > 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. >> >> >> >> If building for ARMv6 or later, I do suggest allowing unaligned >> >> accesses. The moment you add -march=armv6 (or equivalent), you allow >> >> for a number of things not supported by older versions, so why not >> >> unaligned memory accesses? >> > >> > doc/README.arm-unaligned-accesses probably has the answer to your >> > question, especially from line 21 onward. If not, I'll be happy to >> > provide further clarification. >> >> That is about as backwards as it can get. By adding -munaligned-access >> you are telling gcc that unaligned accesses are supported and welcome. >> With this setting enabled, gcc can and will generate unaligned accesses >> just about anywhere. This setting is NOT compatible with the SCTRL.A >> bit being set (strict hardware alignment checking). >> >> You cannot enable strict alignment checking in hardware, tell the >> compiler unaligned accesses are OK, and then expect not to have >> problems. This is no more a valid combination than allowing >> floating-point instructions when the target has no FPU. > > I sense that you have not understood the reason why I want alignment > checking enabled in ARM yet also want ARMv6+ builds to emit native > unaligned accesses if they consider it needed. Your wishes are mutually exclusive. You cannot both allow hardware unaligned access AND at the same time trap them. > The reason is, if we prevent ARMv6 builds from using native unaligned > accesses, they would replace *all* such accesses with smaller, aligned, > ones, which would not trigger a data abort; even those unaligned > accesses cased by programming errors. If you disable unaligned accesses in hardware (as u-boot does), you have no option but doing them a byte at a time. > Whereas if we allow ARMv6+ builds to use native unaligned accesses, yet > enable alignment checks, then any native unaligned access will be > caught as early as possible, and we'll find and cure the issue faster. > > This is, of course, assuming we don't voluntarily do native unaligned > accesses, and in U-Boot, we indeed don't: in U-Boot, accesses are done > on natural alignments. The hardware does not differentiate between intentional and unintentional unaligned accesses. Unlike some architectures, which have dedicated instructions for unaligned accesses, ARM uses the same instructions in both cases (with some limitations). > Since I have set up this policy, experience (and it has been a while) > shows that very few problems arise from alignment checks + native > unaligned accesses. These roughly come from hardware- or standards- > mandated unaligned fields, in which case they are worth explicitly > accessing with "unaligned" macros, or from programming errors, which > should be fixed. The problem is that when you tell gcc (using -munaligned-access) that hardware unaligned accesses are supported, you give it permission to compile even get/put_unaligned() calls (or otherwise annotated accesses) using regular LDR/STR instructions. If this code runs with strict checking enabled in hardware (SCTRL.A set), it will trap. What you probably want is to build with -mno-unaligned-access and enable strict hardware alignment checking. This ensures that any deliberate unaligned accesses (e.g. through get_unaligned) are split into multiple smaller accesses while trapping any (unintentional) unaligned word accesses. The most common alignment-related programming mistake is to dereference a pointer with insufficient alignment. It is far less common for pointers to be marked as unaligned when they do not need to be. > Another benefit of it is, if the code builds and runs in mainline with > alignment checks *and* native unaligned accesses enabled, then it > builds and runs also if you disable either one; whereas code that > builds and runs with alignment checks or native unaligned accesses > disabled might fail if both are enabled. > > And I don't see what we would gain by going from a strict "natural > alignment only" policy to a relaxed "unalignment allowed" one. The benefit of allowing hardware unaligned accesses when supported is that code which for some reason must do these things (as you said, sometimes this is unavoidable) will be faster.
Hi Måns, On Mon, 14 Oct 2013 16:59:56 +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 15:09:39 +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 14:05:13 +0100, Måns Rullgård <mans@mansr.com> > >> > wrote: > >> > > >> >> Albert ARIBAUD <albert.u.boot@aribaud.net> writes: > >> >> > >> >> >> > 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. > >> >> > >> >> If building for ARMv6 or later, I do suggest allowing unaligned > >> >> accesses. The moment you add -march=armv6 (or equivalent), you allow > >> >> for a number of things not supported by older versions, so why not > >> >> unaligned memory accesses? > >> > > >> > doc/README.arm-unaligned-accesses probably has the answer to your > >> > question, especially from line 21 onward. If not, I'll be happy to > >> > provide further clarification. > >> > >> That is about as backwards as it can get. By adding -munaligned-access > >> you are telling gcc that unaligned accesses are supported and welcome. > >> With this setting enabled, gcc can and will generate unaligned accesses > >> just about anywhere. This setting is NOT compatible with the SCTRL.A > >> bit being set (strict hardware alignment checking). > >> > >> You cannot enable strict alignment checking in hardware, tell the > >> compiler unaligned accesses are OK, and then expect not to have > >> problems. This is no more a valid combination than allowing > >> floating-point instructions when the target has no FPU. > > > > I sense that you have not understood the reason why I want alignment > > checking enabled in ARM yet also want ARMv6+ builds to emit native > > unaligned accesses if they consider it needed. > > Your wishes are mutually exclusive. You cannot both allow hardware > unaligned access AND at the same time trap them. These are not wishes, there are actual settings chosen for the reason already laid out. They do appear contradictory if your goal is to use ARMv6+ features to their maximum, but this is not the goal here. > > The reason is, if we prevent ARMv6 builds from using native unaligned > > accesses, they would replace *all* such accesses with smaller, aligned, > > ones, which would not trigger a data abort; even those unaligned > > accesses cased by programming errors. > > If you disable unaligned accesses in hardware (as u-boot does), you have > no option but doing them a byte at a time. Indeed, but I do *not* *disable* native unaligned accesses, I *allow* them; and I do not *want* them to be replaced by byte-by-byte emulation. > > Whereas if we allow ARMv6+ builds to use native unaligned accesses, yet > > enable alignment checks, then any native unaligned access will be > > caught as early as possible, and we'll find and cure the issue faster. > > > > This is, of course, assuming we don't voluntarily do native unaligned > > accesses, and in U-Boot, we indeed don't: in U-Boot, accesses are done > > on natural alignments. > > The hardware does not differentiate between intentional and > unintentional unaligned accesses. Unlike some architectures, which have > dedicated instructions for unaligned accesses, ARM uses the same > instructions in both cases (with some limitations). Indeed, the hardware does not differentiate between intentional vs unintentional unaligned accesses, which is why I decided that rather than suffer the latter, we should trap them both and fix them according to their nature: intentional ones get replaced by emulation, and the cause of unintentional ones gets fixed. > > Since I have set up this policy, experience (and it has been a while) > > shows that very few problems arise from alignment checks + native > > unaligned accesses. These roughly come from hardware- or standards- > > mandated unaligned fields, in which case they are worth explicitly > > accessing with "unaligned" macros, or from programming errors, which > > should be fixed. > > The problem is that when you tell gcc (using -munaligned-access) that > hardware unaligned accesses are supported, you give it permission to > compile even get/put_unaligned() calls (or otherwise annotated accesses) > using regular LDR/STR instructions. If this code runs with strict > checking enabled in hardware (SCTRL.A set), it will trap. This is not "the problem", this is "the intended effect": I *want* it to generate native unaligned accesses when conditions allow it to, because such conditions (except one single known, identified, case [1]) indicate that there is an actual unaligned access statement in the source code which was not fixed or made explicit, and trapping the native access will lead us immediately to the corresponding source code point. > What you probably want is to build with -mno-unaligned-access and enable > strict hardware alignment checking. This ensures that any deliberate > unaligned accesses (e.g. through get_unaligned) are split into multiple > smaller accesses while trapping any (unintentional) unaligned word > accesses. No, I don't want this. I don't want to escape the alignment check trap I also set up. I want to get caught. > The most common alignment-related programming mistake is to dereference > a pointer with insufficient alignment. It is far less common for > pointers to be marked as unaligned when they do not need to be. Possibly, but a typology of possible causes is slightly beyond the point of this discussion. > > Another benefit of it is, if the code builds and runs in mainline with > > alignment checks *and* native unaligned accesses enabled, then it > > builds and runs also if you disable either one; whereas code that > > builds and runs with alignment checks or native unaligned accesses > > disabled might fail if both are enabled. > > > > And I don't see what we would gain by going from a strict "natural > > alignment only" policy to a relaxed "unalignment allowed" one. > > The benefit of allowing hardware unaligned accesses when supported is > that code which for some reason must do these things (as you said, > sometimes this is unavoidable) will be faster. Which actual use case in U-Boot would show a substantial speed increase from allowing native unaligned accesses? I don't consider accessing a few fields occasionally as 'substantial'. And I don't see any large operation, e.g. a memory transfer, requiring unaligned accesses. [1] This case is when a local char array is initialized with a string constant the size of which is a multiple of 4; then gcc uses native ldr/str instructions even though the string literal is not aligned. The workarounds for this are documented. Amicalement,
Albert ARIBAUD <albert.u.boot@aribaud.net> writes: >> > I sense that you have not understood the reason why I want alignment >> > checking enabled in ARM yet also want ARMv6+ builds to emit native >> > unaligned accesses if they consider it needed. >> >> Your wishes are mutually exclusive. You cannot both allow hardware >> unaligned access AND at the same time trap them. > > These are not wishes, there are actual settings chosen for the reason > already laid out. They do appear contradictory if your goal is to use > ARMv6+ features to their maximum, but this is not the goal here. > >> > The reason is, if we prevent ARMv6 builds from using native unaligned >> > accesses, they would replace *all* such accesses with smaller, aligned, >> > ones, which would not trigger a data abort; even those unaligned >> > accesses cased by programming errors. >> >> If you disable unaligned accesses in hardware (as u-boot does), you have >> no option but doing them a byte at a time. > > Indeed, but I do *not* *disable* native unaligned accesses, I *allow* > them; and I do not *want* them to be replaced by byte-by-byte emulation. Let's go back to the basics. In ARMv6 and later there is a bit in the system control register (SCTLR.A) which decides whether or not unaligned memory accesses are allowed. The reset value of this bit allows unaligned accesses. When unaligned accesses are allowed, word and halfword load/store instructions (LDR, STR, LDRH, LDRSH, STRH) with an unaligned address simply perform the requested memory operation. When unaligned accesses are disallowed (SCTLR.A set), these instructions cause an alignment fault if used with an unaligned address. The load/store double and multiple instructions (LDRD, STRD, LDM, STM) always trap on unaligned addresses. This is all described in the ARM Architecture Reference Manual (DDI0406C) section A3.2. That's the hardware side. On the compiler side, gcc traditionally did not issue unaligned load/store instructions on ARM. Since version 4.7, gcc does issue unaligned accesses when the target is ARMv6 or later. This makes sense since a hardware unaligned access is faster than doing it byte-wise in software, and the default for the CPU is to permit unaligned accesses. Needless to say, a potentially unaligned address will only be accessed using the subset of load/store instructions for which this is supported. To support configurations where SCTR.A is set (disallowing unaligned accesses), gcc 4.7 also adds a flag (-mno-unaligned-access) causing it to never emit potentially unaligned loads or stores. The compiler behaviour described above is true only for well-behaved code. In standard C, pointers must always be aligned according to their target type. For instance, a pointer to a 32-bit integer type must typically be 32-bit aligned. Thus, if a pointer is constructed with incorrect alignment, any attempt to use it may result in invalid memory access instructions being executed. In practice, various situations arise where there is a need to work with unaligned data, for example when parsing some communication protocols. To simplify such code, most compilers provide some language extension allowing the programmer to annotate a type definition or pointer as being potentially unaligned. In gcc, the 'packed' attribute on struct and union types serves this purpose. Any access to a member of a 'packed' struct/union is assumed to be potentially unaligned, and the instructions selection is limited accordingly. When -munaligned-access is in effect, unaligned word or halfword load/store instructions may be used here. When this feature is disabled (-mno-unaligned-access), only aligned loads and stores (typically bytes) are permitted. The situations where the compiler will issue an unaligned memory access are generally not predictable. Currently, they tend to occur in struct/array assignment (including initialisation), inline expansion of memcpy/memset, and accesses to 'packed' struct members. As compiler optimisations improve, these cases will likely increase in number. As we can see, enabling the -munaligned-access flag results in load/store instructions occasionally accessing unaligned memory, and the precise places where this happens are not predictable. It is thus a requirement that SCTLR.A be clear when this compiler flag is set. Otherwise alignment faults will occur. If for whatever reason SCTLR.A is set, it is required to use the -mno-unaligned-access compiler flag in order for the code to run cleanly. Failure to do so will result in alignment faults when the code is executed. One reason for setting SCTLR.A is to aid in catching programming errors whereby a normal pointer is assigned an unaligned value. Since these pointers are assumed by the compiler to be correctly aligned, accesses through them are unaffected by the -m[no-]unaligned-access flag, and any such errors will thus trigger an alignment fault. If any of the above is unclear, please let me know, and I will try to explain it better.
Hi Måns, On Tue, 15 Oct 2013 16:23:44 +0100, Måns Rullgård <mans@mansr.com> wrote: > Albert ARIBAUD <albert.u.boot@aribaud.net> writes: > > >> > I sense that you have not understood the reason why I want alignment > >> > checking enabled in ARM yet also want ARMv6+ builds to emit native > >> > unaligned accesses if they consider it needed. > >> > >> Your wishes are mutually exclusive. You cannot both allow hardware > >> unaligned access AND at the same time trap them. > > > > These are not wishes, there are actual settings chosen for the reason > > already laid out. They do appear contradictory if your goal is to use > > ARMv6+ features to their maximum, but this is not the goal here. > > > >> > The reason is, if we prevent ARMv6 builds from using native unaligned > >> > accesses, they would replace *all* such accesses with smaller, aligned, > >> > ones, which would not trigger a data abort; even those unaligned > >> > accesses cased by programming errors. > >> > >> If you disable unaligned accesses in hardware (as u-boot does), you have > >> no option but doing them a byte at a time. > > > > Indeed, but I do *not* *disable* native unaligned accesses, I *allow* > > them; and I do not *want* them to be replaced by byte-by-byte emulation. > > Let's go back to the basics. > > In ARMv6 and later there is a bit in the system control register > (SCTLR.A) which decides whether or not unaligned memory accesses are > allowed. The reset value of this bit allows unaligned accesses. > > When unaligned accesses are allowed, word and halfword load/store > instructions (LDR, STR, LDRH, LDRSH, STRH) with an unaligned address > simply perform the requested memory operation. When unaligned accesses > are disallowed (SCTLR.A set), these instructions cause an alignment > fault if used with an unaligned address. The load/store double and > multiple instructions (LDRD, STRD, LDM, STM) always trap on unaligned > addresses. > > This is all described in the ARM Architecture Reference Manual (DDI0406C) > section A3.2. > > That's the hardware side. Your description is correct, although this bit is not specific to "ARMv6 and later", since ARMv5 has alignment checks too. > On the compiler side, gcc traditionally did not issue unaligned > load/store instructions on ARM. Please be specific: gcc did not emit *native* unaligned accesses. > Since version 4.7, gcc does issue > unaligned accesses when the target is ARMv6 or later. This makes sense > since a hardware unaligned access is faster than doing it byte-wise in > software, and the default for the CPU is to permit unaligned accesses. > Needless to say, a potentially unaligned address will only be accessed > using the subset of load/store instructions for which this is supported. Indeed. Note that this is stated in doc/README.arm-unaligned-accesses. > To support configurations where SCTR.A is set (disallowing unaligned > accesses), gcc 4.7 also adds a flag (-mno-unaligned-access) causing it > to never emit potentially unaligned loads or stores. Maybe the intent which governed the addition of this option was indeed to support configurations where alignment check is enabled; what we can tell is what this option does, and yes, it controls whether the compiler will use native unaligned accesses. > The compiler behaviour described above is true only for well-behaved > code. In standard C, pointers must always be aligned according to their > target type. For instance, a pointer to a 32-bit integer type must > typically be 32-bit aligned. Thus, if a pointer is constructed with > incorrect alignment, any attempt to use it may result in invalid memory > access instructions being executed. Correct, although I'm not sure why you're mentioning this (and, strictly speaking, all of the compiler's behavior is defined only for 'well-behaved' C code). > In practice, various situations arise where there is a need to work with > unaligned data, for example when parsing some communication protocols. > To simplify such code, most compilers provide some language extension > allowing the programmer to annotate a type definition or pointer as > being potentially unaligned. In gcc, the 'packed' attribute on struct > and union types serves this purpose. Correct, and again, I fail to see why you mention this. > Any access to a member of a 'packed' struct/union is assumed to be > potentially unaligned, and the instructions selection is limited > accordingly. When -munaligned-access is in effect, unaligned word or > halfword load/store instructions may be used here. When this feature is > disabled (-mno-unaligned-access), only aligned loads and stores > (typically bytes) are permitted. Ditto. > The situations where the compiler will issue an unaligned memory access > are generally not predictable. Currently, they tend to occur in > struct/array assignment (including initialisation), inline expansion of > memcpy/memset, and accesses to 'packed' struct members. As compiler > optimisations improve, these cases will likely increase in number. This last statement has no solid foundation. On the contrary, there is no reason that a compiler emit unaligned accesses when by default it is expected to align data to their natural boundaries. > As we can see, enabling the -munaligned-access flag results in > load/store instructions occasionally accessing unaligned memory, and the > precise places where this happens are not predictable. It is thus a > requirement that SCTLR.A be clear when this compiler flag is set. > Otherwise alignment faults will occur. As I said, and as documented in doc/README.arm-unaligned-accesses for a whole year now, the only case where native unaligned accesses are emitted is with string literals. Even char arrays are aligned. There's actually a case to be made that string literals should be aligned, too, like all other strings are. > If for whatever reason SCTLR.A is set, it is required to use the > -mno-unaligned-access compiler flag in order for the code to run > cleanly. Failure to do so will result in alignment faults when the code > is executed. It is never *required* to use -mno-unaligned-access unless your hardware is unable to perform unaligned accesses for some reason external to the CPU. > One reason for setting SCTLR.A is to aid in catching programming errors > whereby a normal pointer is assigned an unaligned value. Since these > pointers are assumed by the compiler to be correctly aligned, accesses > through them are unaffected by the -m[no-]unaligned-access flag, and > any such errors will thus trigger an alignment fault. This is one use of alignment checks. Another use is to catch code which intends to do unaligned accesses even though the policy of the project says otherwise. > If any of the above is unclear, please let me know, and I will try to > explain it better. All this is perfectly clear and essentially valid... if your goal is to use all the features of ARMv6+ to simplify the developer's life under the assumption that the code is well-behaved (and the compiler is error-free, for that matter). It is also still not valid with respect to my goal, which is to make sure that the actual code of a multi-architecture project running on a variety of compilers will be as correct as possible. Let me state this again: while the approach you describe is the logical one to make life as easy as possible for the developer (and compiler write) on ARMv6+ architectures, it is not *mandated* in any sense, and it is not the approach I have chosen. I suggest now that you leave aside any assumptions on "how things must be done", then read my answers again in the context I have just described, and fin "why things are done this way" in ARM U-Boot. Amicalement,
Albert ARIBAUD <albert.u.boot@aribaud.net> writes: > Hi Måns, > > On Tue, 15 Oct 2013 16:23:44 +0100, Måns Rullgård <mans@mansr.com> > wrote: > >> On the compiler side, gcc traditionally did not issue unaligned >> load/store instructions on ARM. > > Please be specific: gcc did not emit *native* unaligned accesses. Please explain what you mean by "native unaligned access".
Hi Måns, On Tue, 15 Oct 2013 17:29:24 +0100, Måns Rullgård <mans@mansr.com> wrote: > Albert ARIBAUD <albert.u.boot@aribaud.net> writes: > > > Hi Måns, > > > > On Tue, 15 Oct 2013 16:23:44 +0100, Måns Rullgård <mans@mansr.com> > > wrote: > > > >> On the compiler side, gcc traditionally did not issue unaligned > >> load/store instructions on ARM. > > > > Please be specific: gcc did not emit *native* unaligned accesses. > > Please explain what you mean by "native unaligned access". By this I mean an unaligned access performed with a single transfer, initiated by a single processor instruction, as opposed to an unaligned access performed with a series of smaller, aligned, transfers (and possibly additional operations) initiated by a sequence of processor instructions (emulated, if you will). Prior to 4.7, gcc did emit unaligned accesses if instructed to at the source code level; they were however emulated. From 4.7 onward, it was possible to choose whether unaligned transfers should be performed native or emulated, the default being native for ARMv6+, and emulated for the rest. Amicalement,
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)); /* Write MBR sector to the MMC device */ if (dev_desc->block_write(dev_desc->dev, 0, 1, p_mbr) != 1) { @@ -387,8 +388,9 @@ int gpt_fill_pte(gpt_header *gpt_h, gpt_entry *gpt_e, gpt_e[i].ending_lba = cpu_to_le64(offset - 1); /* partition type GUID */ + static efi_guid_t basic_guid = PARTITION_BASIC_DATA_GUID; memcpy(gpt_e[i].partition_type_guid.b, - &PARTITION_BASIC_DATA_GUID, 16); + &basic_guid, 16); #ifdef CONFIG_PARTITION_UUIDS str_uuid = partitions[i].uuid;