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

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

[Piotr Wilczek]

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);
 
 #ifdef CONFIG_PARTITION_UUIDS
 		str_uuid = partitions[i].uuid;
-- 
1.7.9.5

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

[Albert ARIBAUD]

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,
-- 
Albert.

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

[Måns Rullgård]

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.

-- 
M?ns Rullg?rd
mans at mansr.com

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

[Piotr Wilczek]

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

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 at mansr.com

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

[Måns Rullgård]

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.

-- 
M?ns Rullg?rd
mans at mansr.com

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

[Albert ARIBAUD]

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.

Amicalement,
-- 
Albert.

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

[Måns Rullgård]

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.

-- 
M?ns Rullg?rd
mans at mansr.com

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

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

[Måns Rullgård]

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?

-- 
M?ns Rullg?rd
mans at mansr.com

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

[Albert ARIBAUD]

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,
-- 
Albert.

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

[Måns Rullgård]

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.

-- 
M?ns Rullg?rd
mans at mansr.com

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

[Albert ARIBAUD]

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.

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

[Måns Rullgård]

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.

-- 
M?ns Rullg?rd
mans at mansr.com

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

[Albert ARIBAUD]

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.

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

[Måns Rullgård]

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.

-- 
M?ns Rullg?rd
mans at mansr.com

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

[Albert ARIBAUD]

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.

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

[Måns Rullgård]

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".

-- 
M?ns Rullg?rd
mans at mansr.com

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

[Albert ARIBAUD]

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,
-- 
Albert.

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

[Piotr Wilczek]

Dear Albert and M?ns,

> -----Original Message-----
> From: M?ns Rullg?rd [mailto:mans at mansr.com]
> Sent: Monday, October 14, 2013 3:05 PM
> To: Albert ARIBAUD
> Cc: Piotr Wilczek; 'Tom Rini'; u-boot at 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 at mansr.com

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

[Albert ARIBAUD]

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 at mansr.com]
> > Sent: Monday, October 14, 2013 3:05 PM
> > To: Albert ARIBAUD
> > Cc: Piotr Wilczek; 'Tom Rini'; u-boot at 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,
-- 
Albert.

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

[Piotr Wilczek]

This patch fixes part_efi code 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>
CC: Albert ARIBAUD <albert.u.boot@aribaud.net>
---
Chnages for V2:
 - used put_unaligned to copy value;
 - use __aligned to align local array;

 disk/part_efi.c |    7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

diff --git a/disk/part_efi.c b/disk/part_efi.c
index 9c33ae7..eb2cd57 100644
--- a/disk/part_efi.c
+++ b/disk/part_efi.c
@@ -225,7 +225,7 @@ 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;
+	put_unaligned(dev_desc->lba, &p_mbr->partition_record[0].nr_sects);
 
 	/* Write MBR sector to the MMC device */
 	if (dev_desc->block_write(dev_desc->dev, 0, 1, p_mbr) != 1) {
@@ -361,6 +361,8 @@ int gpt_fill_pte(gpt_header *gpt_h, gpt_entry *gpt_e,
 #ifdef CONFIG_PARTITION_UUIDS
 	char *str_uuid;
 #endif
+	static efi_guid_t basic_guid __aligned(0x04) =
+		PARTITION_BASIC_DATA_GUID;
 
 	for (i = 0; i < parts; i++) {
 		/* partition starting lba */
@@ -388,8 +390,7 @@ 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 */
-		memcpy(gpt_e[i].partition_type_guid.b,
-			&PARTITION_BASIC_DATA_GUID, 16);
+		gpt_e[i].partition_type_guid = basic_guid;
 
 #ifdef CONFIG_PARTITION_UUIDS
 		str_uuid = partitions[i].uuid;
-- 
1.7.9.5

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

[Tom Rini]

On Tue, Jan 28, 2014 at 01:46:52PM +0100, Piotr Wilczek wrote:

> This patch fixes part_efi code 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>
> CC: Albert ARIBAUD <albert.u.boot@aribaud.net>
> ---
> Chnages for V2:
>  - used put_unaligned to copy value;
>  - use __aligned to align local array;
> 
>  disk/part_efi.c |    7 ++++---
>  1 file changed, 4 insertions(+), 3 deletions(-)

Sorry.  After re-reading the thread here again I've posted a patch to
globally switch to -mno-unaligned-access for our armv7 tunes instead.
Thanks for your effort however!

-- 
Tom
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 836 bytes
Desc: Digital signature
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20140129/d60169f3/attachment.pgp>

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

[Hector Palacios]

On 01/28/2014 01:46 PM, Piotr Wilczek wrote:
> This patch fixes part_efi code 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>
> CC: Albert ARIBAUD <albert.u.boot@aribaud.net>
> ---
> Chnages for V2:
>   - used put_unaligned to copy value;
>   - use __aligned to align local array;
>
>   disk/part_efi.c |    7 ++++---
>   1 file changed, 4 insertions(+), 3 deletions(-)
>
> diff --git a/disk/part_efi.c b/disk/part_efi.c
> index 9c33ae7..eb2cd57 100644
> --- a/disk/part_efi.c
> +++ b/disk/part_efi.c
> @@ -225,7 +225,7 @@ 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;
> +	put_unaligned(dev_desc->lba, &p_mbr->partition_record[0].nr_sects);
>
>   	/* Write MBR sector to the MMC device */
>   	if (dev_desc->block_write(dev_desc->dev, 0, 1, p_mbr) != 1) {
> @@ -361,6 +361,8 @@ int gpt_fill_pte(gpt_header *gpt_h, gpt_entry *gpt_e,
>   #ifdef CONFIG_PARTITION_UUIDS
>   	char *str_uuid;
>   #endif
> +	static efi_guid_t basic_guid __aligned(0x04) =
> +		PARTITION_BASIC_DATA_GUID;
>
>   	for (i = 0; i < parts; i++) {
>   		/* partition starting lba */
> @@ -388,8 +390,7 @@ 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 */
> -		memcpy(gpt_e[i].partition_type_guid.b,
> -			&PARTITION_BASIC_DATA_GUID, 16);
> +		gpt_e[i].partition_type_guid = basic_guid;
>
>   #ifdef CONFIG_PARTITION_UUIDS
>   		str_uuid = partitions[i].uuid;
>

Sorry, I sent my tested-by on a different thread:
http://patchwork.ozlabs.org/patch/319649/

Tested on i.MX6 (armv7) custom board and it is working fine without
the -mno-unaligned-access flag.

Tested-by: Hector Palacios <hector.palacios@digi.com>

Replying to Tom's question in that other thread, regarding the issue:

 > Can you give me some steps on how to hit this bug?  I believe it's a bug
 > and I believe we need to fix it, I just want to investigate a few things
 > while we've got a trigger case right now.  Thanks!

GPT partition table needs to write a protective MBR to sector 0.
The MBR structure has four partition entries (each occupying 16 bytes) at unaligned 
offsets +1BEh, +1CEh, +1DEh, +1EEh (see [1]). The structure of each partition entry is 
defined at include/part_efi.h

struct partition {
	u8 boot_ind;		/* 0x80 - active */
	u8 head;		/* starting head */
	u8 sector;		/* starting sector */
	u8 cyl;			/* starting cylinder */
	u8 sys_ind;		/* What partition type */
	u8 end_head;		/* end head */
	u8 end_sector;		/* end sector */
	u8 end_cyl;		/* end cylinder */
	__le32 start_sect;	/* starting sector counting from 0 */
	__le32 nr_sects;	/* nr of sectors in partition */
} __packed;

showing eight 1-byte fields and two 4-byte fields. Since the offsets for each 
partition entry are unaligned, the last two fields (which are 32bit) are unaligned as 
well. But it's not an error, it's just the specification of the MBR requires these 
fields to be at those exact offsets. So the usage of unaligned macros for accessing 
those fields is justified.

[1] http://en.wikipedia.org/wiki/Master_boot_record#Sector_layout

Best regards,
--
Hector Palacios

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

[Tom Rini]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

On 02/19/2014 09:56 AM, Hector Palacios wrote:
> On 01/28/2014 01:46 PM, Piotr Wilczek wrote:
>> This patch fixes part_efi code 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>
>> CC: Albert ARIBAUD <albert.u.boot@aribaud.net>
>> ---
>> Chnages for V2:
>>   - used put_unaligned to copy value;
>>   - use __aligned to align local array;
>>
>>   disk/part_efi.c |    7 ++++---
>>   1 file changed, 4 insertions(+), 3 deletions(-)
>>
>> diff --git a/disk/part_efi.c b/disk/part_efi.c
>> index 9c33ae7..eb2cd57 100644
>> --- a/disk/part_efi.c
>> +++ b/disk/part_efi.c
>> @@ -225,7 +225,7 @@ 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;
>> +    put_unaligned(dev_desc->lba, &p_mbr->partition_record[0].nr_sects);
>>
>>       /* Write MBR sector to the MMC device */
>>       if (dev_desc->block_write(dev_desc->dev, 0, 1, p_mbr) != 1) {
>> @@ -361,6 +361,8 @@ int gpt_fill_pte(gpt_header *gpt_h, gpt_entry *gpt_e,
>>   #ifdef CONFIG_PARTITION_UUIDS
>>       char *str_uuid;
>>   #endif
>> +    static efi_guid_t basic_guid __aligned(0x04) =
>> +        PARTITION_BASIC_DATA_GUID;
>>
>>       for (i = 0; i < parts; i++) {
>>           /* partition starting lba */
>> @@ -388,8 +390,7 @@ 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 */
>> -        memcpy(gpt_e[i].partition_type_guid.b,
>> -            &PARTITION_BASIC_DATA_GUID, 16);
>> +        gpt_e[i].partition_type_guid = basic_guid;
>>
>>   #ifdef CONFIG_PARTITION_UUIDS
>>           str_uuid = partitions[i].uuid;
>>
> 
> Sorry, I sent my tested-by on a different thread:
> http://patchwork.ozlabs.org/patch/319649/
> 
> Tested on i.MX6 (armv7) custom board and it is working fine without
> the -mno-unaligned-access flag.
> 
> Tested-by: Hector Palacios <hector.palacios@digi.com>
> 
> Replying to Tom's question in that other thread, regarding the issue:
> 
>> Can you give me some steps on how to hit this bug?  I believe it's a bug
>> and I believe we need to fix it, I just want to investigate a few things
>> while we've got a trigger case right now.  Thanks!
> 
> GPT partition table needs to write a protective MBR to sector 0.
> The MBR structure has four partition entries (each occupying 16 bytes)
> at unaligned offsets +1BEh, +1CEh, +1DEh, +1EEh (see [1]). The structure
> of each partition entry is defined at include/part_efi.h
> 
> struct partition {
>     u8 boot_ind;        /* 0x80 - active */
>     u8 head;        /* starting head */
>     u8 sector;        /* starting sector */
>     u8 cyl;            /* starting cylinder */
>     u8 sys_ind;        /* What partition type */
>     u8 end_head;        /* end head */
>     u8 end_sector;        /* end sector */
>     u8 end_cyl;        /* end cylinder */
>     __le32 start_sect;    /* starting sector counting from 0 */
>     __le32 nr_sects;    /* nr of sectors in partition */
> } __packed;
> 
> showing eight 1-byte fields and two 4-byte fields. Since the offsets for
> each partition entry are unaligned, the last two fields (which are
> 32bit) are unaligned as well. But it's not an error, it's just the
> specification of the MBR requires these fields to be at those exact
> offsets. So the usage of unaligned macros for accessing those fields is
> justified.

Right.  I would have sworn I used the GPT commands since we've dropped
- -mno-unaligned-access but I'll just go re-test locally now then, thanks.

- -- 
Tom
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.11 (GNU/Linux)
Comment: Using GnuPG with Thunderbird - http://www.enigmail.net/

iQIcBAEBAgAGBQJTBMfWAAoJENk4IS6UOR1Wm/IP/1sOKxdJvLljPxOQ5myKhT0k
PWFMVuN9ZomSkr9+yYt87cVtz28OV42dqVIig7i96TYbzA19AxWY6jWVlPxlmbAZ
0qIwILgogtytJPDad/4NMWXy8GyHP6f9iYIui/DRImsZaErSek+egEshf46M5oyH
0Fd6H/HcpLlJ1QydN1jQzKSppS3HadlwVKEQqC2vN2WjKv3li/m84xoSdiNty2Lj
WgqEBOTfbS5dYQub72WiiaCvs2QVBAP2jRk1Lmtu/gj/L5Av0uc+J8JFV6+Un+Tt
VNUKigG8PdLonex6gCOwpULX6Lft+9x0YWJXIp0ow3JFStpxbHPgMqqKkVyC7Jgp
n14dHO4mG5ZdT2m0y/Tyu1sg7tllTPtW2k+GBsRa8vnOzHq11I6/wQ712S6BK4aB
jjuQsxOoxbGSzMEpMrotf4knc0tuL72Lh46Lz1ksRZNdu7NGOAO+DjGuXnzKn8H4
oEB6aFEewaMV8o2XBNaMddMg8MyPtVC4qZiN2uuMMKjxkTukKUCV9xCl9YkJLAV2
oXdVtEoczjWY6made42olvTeev5R1NDD1ex63q0sZIylJQrQSjMju+1PpQQxAwAh
/Kv5xn+13JtX7eaHY2oogHRLmLB5Cp2EUwg/ZVzArNTnlVdzOsr72v7BLi/fz6T2
NA5qIFUntQunN2OweEE+
=32uL
-----END PGP SIGNATURE-----

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

[Tom Rini]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

On 02/19/2014 10:03 AM, Tom Rini wrote:
> On 02/19/2014 09:56 AM, Hector Palacios wrote:
>> On 01/28/2014 01:46 PM, Piotr Wilczek wrote:
>>> This patch fixes part_efi code 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>
>>> CC: Albert ARIBAUD <albert.u.boot@aribaud.net>
>>> ---
>>> Chnages for V2:
>>>   - used put_unaligned to copy value;
>>>   - use __aligned to align local array;
>>>
>>>   disk/part_efi.c |    7 ++++---
>>>   1 file changed, 4 insertions(+), 3 deletions(-)
>>>
>>> diff --git a/disk/part_efi.c b/disk/part_efi.c
>>> index 9c33ae7..eb2cd57 100644
>>> --- a/disk/part_efi.c
>>> +++ b/disk/part_efi.c
>>> @@ -225,7 +225,7 @@ 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;
>>> +    put_unaligned(dev_desc->lba, &p_mbr->partition_record[0].nr_sects);
>>>
>>>       /* Write MBR sector to the MMC device */
>>>       if (dev_desc->block_write(dev_desc->dev, 0, 1, p_mbr) != 1) {
>>> @@ -361,6 +361,8 @@ int gpt_fill_pte(gpt_header *gpt_h, gpt_entry *gpt_e,
>>>   #ifdef CONFIG_PARTITION_UUIDS
>>>       char *str_uuid;
>>>   #endif
>>> +    static efi_guid_t basic_guid __aligned(0x04) =
>>> +        PARTITION_BASIC_DATA_GUID;
>>>
>>>       for (i = 0; i < parts; i++) {
>>>           /* partition starting lba */
>>> @@ -388,8 +390,7 @@ 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 */
>>> -        memcpy(gpt_e[i].partition_type_guid.b,
>>> -            &PARTITION_BASIC_DATA_GUID, 16);
>>> +        gpt_e[i].partition_type_guid = basic_guid;
>>>
>>>   #ifdef CONFIG_PARTITION_UUIDS
>>>           str_uuid = partitions[i].uuid;
>>>
> 
>> Sorry, I sent my tested-by on a different thread:
>> http://patchwork.ozlabs.org/patch/319649/
> 
>> Tested on i.MX6 (armv7) custom board and it is working fine without
>> the -mno-unaligned-access flag.
> 
>> Tested-by: Hector Palacios <hector.palacios@digi.com>
> 
>> Replying to Tom's question in that other thread, regarding the issue:
> 
>>> Can you give me some steps on how to hit this bug?  I believe it's a bug
>>> and I believe we need to fix it, I just want to investigate a few things
>>> while we've got a trigger case right now.  Thanks!
> 
>> GPT partition table needs to write a protective MBR to sector 0.
>> The MBR structure has four partition entries (each occupying 16 bytes)
>> at unaligned offsets +1BEh, +1CEh, +1DEh, +1EEh (see [1]). The structure
>> of each partition entry is defined at include/part_efi.h
> 
>> struct partition {
>>     u8 boot_ind;        /* 0x80 - active */
>>     u8 head;        /* starting head */
>>     u8 sector;        /* starting sector */
>>     u8 cyl;            /* starting cylinder */
>>     u8 sys_ind;        /* What partition type */
>>     u8 end_head;        /* end head */
>>     u8 end_sector;        /* end sector */
>>     u8 end_cyl;        /* end cylinder */
>>     __le32 start_sect;    /* starting sector counting from 0 */
>>     __le32 nr_sects;    /* nr of sectors in partition */
>> } __packed;
> 
>> showing eight 1-byte fields and two 4-byte fields. Since the offsets for
>> each partition entry are unaligned, the last two fields (which are
>> 32bit) are unaligned as well. But it's not an error, it's just the
>> specification of the MBR requires these fields to be at those exact
>> offsets. So the usage of unaligned macros for accessing those fields is
>> justified.
> 
> Right.  I would have sworn I used the GPT commands since we've dropped
> -mno-unaligned-access but I'll just go re-test locally now then, thanks.

Indeed I hadn't re-tested recently enough, thanks.

- -- 
Tom
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.11 (GNU/Linux)
Comment: Using GnuPG with Thunderbird - http://www.enigmail.net/

iQIcBAEBAgAGBQJTBMxeAAoJENk4IS6UOR1WmKAP/05tEc0Ix4vrbI+WEYv4s26v
Up3Uj1yMHBgHhCmdUops6C6eNjoSX2Z2TzfFx8jQ1YCTrGXO229CSi0U/oXkTWCB
lbdMxQ6IyKh1v+Z4MdEJBq43XxcGMVE8g75Z8Eb/7B74kHRvsLeLGIDMg4A3z1yB
b95HOCaifhSykBELsrqWXpMhJr4KMkn+GT9lNA5umLJdHT/1A2pEglbkZWAu4tAt
ybDpdpXI3Ai8eVg9NuMJXEAcYEGezlg55esFv57gJfLfBPM/e0WLF4MtZYyFJmC/
0SLe46OG19E6JzHMKrHngZbyVSC+Iwzh5Mw6vY40IyVxA6fpXZEZ119AyxLtP4m8
BiWjjRAO65KC6qzRJJYzXKoXSD8Ky+VJ3ATWzUhVSeLQRHeE2am8JsT8ENj5dngC
f93pzqTmOp9LPqOB81dlIbu+R8QoruX0mOQxygNy+7tpTTijLn+UUu23z165j42b
qsBzrAddgFZzUxqfyJLHIr/bvrVqBpEP6BGv2i+5eA6Q/dPHMvVLV3hTZjnxtK52
I6RVfK2R4uTX8mK+yN2HzBdqZhCJqDgxbWYUMuDIkVq7QeY5rtPBaFlOguLPqwx2
+i38rtGj5abZcqr9ASDcmrfIoe1mIAj2NPuJejNLzbwyipyqRVO0CT+GG/FwXLLG
LFWWTaNc/X4FMjGctcyr
=1et2
-----END PGP SIGNATURE-----

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

[Lukasz Majewski]

Hi Tom,

> -----BEGIN PGP SIGNED MESSAGE-----
> Hash: SHA1
> 
> On 02/19/2014 10:03 AM, Tom Rini wrote:
> > On 02/19/2014 09:56 AM, Hector Palacios wrote:
> >> On 01/28/2014 01:46 PM, Piotr Wilczek wrote:
> >>> This patch fixes part_efi code 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>
> >>> CC: Albert ARIBAUD <albert.u.boot@aribaud.net>
> >>> ---
> >>> Chnages for V2:
> >>>   - used put_unaligned to copy value;
> >>>   - use __aligned to align local array;
> >>>
> >>>   disk/part_efi.c |    7 ++++---
> >>>   1 file changed, 4 insertions(+), 3 deletions(-)
> >>>
> >>> diff --git a/disk/part_efi.c b/disk/part_efi.c
> >>> index 9c33ae7..eb2cd57 100644
> >>> --- a/disk/part_efi.c
> >>> +++ b/disk/part_efi.c
> >>> @@ -225,7 +225,7 @@ 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;
> >>> +    put_unaligned(dev_desc->lba,
> >>> &p_mbr->partition_record[0].nr_sects);
> >>>
> >>>       /* Write MBR sector to the MMC device */
> >>>       if (dev_desc->block_write(dev_desc->dev, 0, 1, p_mbr) != 1)
> >>> { @@ -361,6 +361,8 @@ int gpt_fill_pte(gpt_header *gpt_h,
> >>> gpt_entry *gpt_e, #ifdef CONFIG_PARTITION_UUIDS
> >>>       char *str_uuid;
> >>>   #endif
> >>> +    static efi_guid_t basic_guid __aligned(0x04) =
> >>> +        PARTITION_BASIC_DATA_GUID;
> >>>
> >>>       for (i = 0; i < parts; i++) {
> >>>           /* partition starting lba */
> >>> @@ -388,8 +390,7 @@ 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 */
> >>> -        memcpy(gpt_e[i].partition_type_guid.b,
> >>> -            &PARTITION_BASIC_DATA_GUID, 16);
> >>> +        gpt_e[i].partition_type_guid = basic_guid;
> >>>
> >>>   #ifdef CONFIG_PARTITION_UUIDS
> >>>           str_uuid = partitions[i].uuid;
> >>>
> > 
> >> Sorry, I sent my tested-by on a different thread:
> >> http://patchwork.ozlabs.org/patch/319649/
> > 
> >> Tested on i.MX6 (armv7) custom board and it is working fine without
> >> the -mno-unaligned-access flag.
> > 
> >> Tested-by: Hector Palacios <hector.palacios@digi.com>
> > 
> >> Replying to Tom's question in that other thread, regarding the
> >> issue:
> > 
> >>> Can you give me some steps on how to hit this bug?  I believe
> >>> it's a bug and I believe we need to fix it, I just want to
> >>> investigate a few things while we've got a trigger case right
> >>> now.  Thanks!
> > 
> >> GPT partition table needs to write a protective MBR to sector 0.
> >> The MBR structure has four partition entries (each occupying 16
> >> bytes) at unaligned offsets +1BEh, +1CEh, +1DEh, +1EEh (see [1]).
> >> The structure of each partition entry is defined at
> >> include/part_efi.h
> > 
> >> struct partition {
> >>     u8 boot_ind;        /* 0x80 - active */
> >>     u8 head;        /* starting head */
> >>     u8 sector;        /* starting sector */
> >>     u8 cyl;            /* starting cylinder */
> >>     u8 sys_ind;        /* What partition type */
> >>     u8 end_head;        /* end head */
> >>     u8 end_sector;        /* end sector */
> >>     u8 end_cyl;        /* end cylinder */
> >>     __le32 start_sect;    /* starting sector counting from 0 */
> >>     __le32 nr_sects;    /* nr of sectors in partition */
> >> } __packed;
> > 
> >> showing eight 1-byte fields and two 4-byte fields. Since the
> >> offsets for each partition entry are unaligned, the last two
> >> fields (which are 32bit) are unaligned as well. But it's not an
> >> error, it's just the specification of the MBR requires these
> >> fields to be at those exact offsets. So the usage of unaligned
> >> macros for accessing those fields is justified.
> > 
> > Right.  I would have sworn I used the GPT commands since we've
> > dropped -mno-unaligned-access but I'll just go re-test locally now
> > then, thanks.
> 
> Indeed I hadn't re-tested recently enough, thanks.

Have you managed to reproduce the problem on your setup?

I've reproduced it on Trats/Trats2 with ELDK 5.4 (gcc 4.7.2 armv7a
toolchain).

This patch fixes the problem.

> 
> - -- 
> Tom
> -----BEGIN PGP SIGNATURE-----
> Version: GnuPG v1.4.11 (GNU/Linux)
> Comment: Using GnuPG with Thunderbird - http://www.enigmail.net/
> 
> iQIcBAEBAgAGBQJTBMxeAAoJENk4IS6UOR1WmKAP/05tEc0Ix4vrbI+WEYv4s26v
> Up3Uj1yMHBgHhCmdUops6C6eNjoSX2Z2TzfFx8jQ1YCTrGXO229CSi0U/oXkTWCB
> lbdMxQ6IyKh1v+Z4MdEJBq43XxcGMVE8g75Z8Eb/7B74kHRvsLeLGIDMg4A3z1yB
> b95HOCaifhSykBELsrqWXpMhJr4KMkn+GT9lNA5umLJdHT/1A2pEglbkZWAu4tAt
> ybDpdpXI3Ai8eVg9NuMJXEAcYEGezlg55esFv57gJfLfBPM/e0WLF4MtZYyFJmC/
> 0SLe46OG19E6JzHMKrHngZbyVSC+Iwzh5Mw6vY40IyVxA6fpXZEZ119AyxLtP4m8
> BiWjjRAO65KC6qzRJJYzXKoXSD8Ky+VJ3ATWzUhVSeLQRHeE2am8JsT8ENj5dngC
> f93pzqTmOp9LPqOB81dlIbu+R8QoruX0mOQxygNy+7tpTTijLn+UUu23z165j42b
> qsBzrAddgFZzUxqfyJLHIr/bvrVqBpEP6BGv2i+5eA6Q/dPHMvVLV3hTZjnxtK52
> I6RVfK2R4uTX8mK+yN2HzBdqZhCJqDgxbWYUMuDIkVq7QeY5rtPBaFlOguLPqwx2
> +i38rtGj5abZcqr9ASDcmrfIoe1mIAj2NPuJejNLzbwyipyqRVO0CT+GG/FwXLLG
> LFWWTaNc/X4FMjGctcyr
> =1et2
> -----END PGP SIGNATURE-----



-- 
Best regards,

Lukasz Majewski

Samsung R&D Institute Poland (SRPOL) | Linux Platform Group

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

[Tom Rini]

On Mon, Feb 24, 2014 at 04:56:57PM +0100, Lukasz Majewski wrote:
> Hi Tom,
> 
> > -----BEGIN PGP SIGNED MESSAGE-----
> > Hash: SHA1
> > 
> > On 02/19/2014 10:03 AM, Tom Rini wrote:
> > > On 02/19/2014 09:56 AM, Hector Palacios wrote:
> > >> On 01/28/2014 01:46 PM, Piotr Wilczek wrote:
> > >>> This patch fixes part_efi code 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>
> > >>> CC: Albert ARIBAUD <albert.u.boot@aribaud.net>
> > >>> ---
> > >>> Chnages for V2:
> > >>>   - used put_unaligned to copy value;
> > >>>   - use __aligned to align local array;
> > >>>
> > >>>   disk/part_efi.c |    7 ++++---
> > >>>   1 file changed, 4 insertions(+), 3 deletions(-)
> > >>>
> > >>> diff --git a/disk/part_efi.c b/disk/part_efi.c
> > >>> index 9c33ae7..eb2cd57 100644
> > >>> --- a/disk/part_efi.c
> > >>> +++ b/disk/part_efi.c
> > >>> @@ -225,7 +225,7 @@ 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;
> > >>> +    put_unaligned(dev_desc->lba,
> > >>> &p_mbr->partition_record[0].nr_sects);
> > >>>
> > >>>       /* Write MBR sector to the MMC device */
> > >>>       if (dev_desc->block_write(dev_desc->dev, 0, 1, p_mbr) != 1)
> > >>> { @@ -361,6 +361,8 @@ int gpt_fill_pte(gpt_header *gpt_h,
> > >>> gpt_entry *gpt_e, #ifdef CONFIG_PARTITION_UUIDS
> > >>>       char *str_uuid;
> > >>>   #endif
> > >>> +    static efi_guid_t basic_guid __aligned(0x04) =
> > >>> +        PARTITION_BASIC_DATA_GUID;
> > >>>
> > >>>       for (i = 0; i < parts; i++) {
> > >>>           /* partition starting lba */
> > >>> @@ -388,8 +390,7 @@ 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 */
> > >>> -        memcpy(gpt_e[i].partition_type_guid.b,
> > >>> -            &PARTITION_BASIC_DATA_GUID, 16);
> > >>> +        gpt_e[i].partition_type_guid = basic_guid;
> > >>>
> > >>>   #ifdef CONFIG_PARTITION_UUIDS
> > >>>           str_uuid = partitions[i].uuid;
> > >>>
> > > 
> > >> Sorry, I sent my tested-by on a different thread:
> > >> http://patchwork.ozlabs.org/patch/319649/
> > > 
> > >> Tested on i.MX6 (armv7) custom board and it is working fine without
> > >> the -mno-unaligned-access flag.
> > > 
> > >> Tested-by: Hector Palacios <hector.palacios@digi.com>
> > > 
> > >> Replying to Tom's question in that other thread, regarding the
> > >> issue:
> > > 
> > >>> Can you give me some steps on how to hit this bug?  I believe
> > >>> it's a bug and I believe we need to fix it, I just want to
> > >>> investigate a few things while we've got a trigger case right
> > >>> now.  Thanks!
> > > 
> > >> GPT partition table needs to write a protective MBR to sector 0.
> > >> The MBR structure has four partition entries (each occupying 16
> > >> bytes) at unaligned offsets +1BEh, +1CEh, +1DEh, +1EEh (see [1]).
> > >> The structure of each partition entry is defined at
> > >> include/part_efi.h
> > > 
> > >> struct partition {
> > >>     u8 boot_ind;        /* 0x80 - active */
> > >>     u8 head;        /* starting head */
> > >>     u8 sector;        /* starting sector */
> > >>     u8 cyl;            /* starting cylinder */
> > >>     u8 sys_ind;        /* What partition type */
> > >>     u8 end_head;        /* end head */
> > >>     u8 end_sector;        /* end sector */
> > >>     u8 end_cyl;        /* end cylinder */
> > >>     __le32 start_sect;    /* starting sector counting from 0 */
> > >>     __le32 nr_sects;    /* nr of sectors in partition */
> > >> } __packed;
> > > 
> > >> showing eight 1-byte fields and two 4-byte fields. Since the
> > >> offsets for each partition entry are unaligned, the last two
> > >> fields (which are 32bit) are unaligned as well. But it's not an
> > >> error, it's just the specification of the MBR requires these
> > >> fields to be at those exact offsets. So the usage of unaligned
> > >> macros for accessing those fields is justified.
> > > 
> > > Right.  I would have sworn I used the GPT commands since we've
> > > dropped -mno-unaligned-access but I'll just go re-test locally now
> > > then, thanks.
> > 
> > Indeed I hadn't re-tested recently enough, thanks.
> 
> Have you managed to reproduce the problem on your setup?
> 
> I've reproduced it on Trats/Trats2 with ELDK 5.4 (gcc 4.7.2 armv7a
> toolchain).
> 
> This patch fixes the problem.

Yes, which is why I've renewed my effort to correct our behavior with
respect to gcc generating unaligned access faults where we don't need
them, and this shall be resolved for v2014.04-rc2.

-- 
Tom
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 836 bytes
Desc: Digital signature
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20140224/8b7bf53e/attachment.pgp>