Re: [ipxe-devel] [PATCH 1/1] [arm] add -mno-unaligned-access compiler flag

2018-04-19 Thread Michael Brown 

On 19/04/18 16:26, Mark Rutland wrote:

For better or worse, I don't think that it is sufficient to pass the
compiler -mno-unaligned-accesses, even if it happens to mask the problem
in this specific case. The compiler can validly assume structures have
their usual alignment, and hence the LDRD should not be misaligned.

>
> 


... whereas using packed (rather than aligned(1), which I was wrong
about):

>
> 


... which should avoid alignment traps so long as SCTLR.A is clear to
permit unaligned LDR.


Fantastic; thank you for such a detailed response!

I've pushed a fix to add the missing __attribute__((packed)):

  http://git.ipxe.org/ipxe.git/commitdiff/e901e6b73

Heinrich: does this fix the problem?

Thanks,

Michael
___
ipxe-devel mailing list
ipxe-devel@lists.ipxe.org
https://lists.ipxe.org/mailman/listinfo.cgi/ipxe-devel

Re: [ipxe-devel] [PATCH 1/1] [arm] add -mno-unaligned-access compiler flag

2018-04-19 Thread Mark Rutland 
On Thu, Apr 19, 2018 at 04:00:47PM +0100, Mark Rutland wrote:
> On Thu, Apr 19, 2018 at 12:33:10AM +0100, Michael Brown wrote:
> > On 18/04/18 20:21, Heinrich Schuchardt wrote:
> For better or worse, I don't think that it is sufficient to pass the
> compiler -mno-unaligned-accesses, even if it happens to mask the problem
> in this specific case. The compiler can validly assume structures have
> their usual alignment, and hence the LDRD should not be misaligned.

I've just verified that this is the case:


struct foo {
int a;
int b;
};

int foo_cmp (struct foo *f1)
{
return (f1->a == f1->b);
};


... when compiled with:

  arm-linux-gnueabihf-gcc -mcpu=cortex-a15 -mabi=aapcs -mno-unaligned-access 
-O2 -c test.c

... yields:

 :
   0:   e9d0 2000   ldrdr2, r0, [r0]
   4:   eba2    sub.w   r0, r2, r0
   8:   fab0 f080   clz r0, r0
   c:   ea4f 1050   mov.w   r0, r0, lsr #5
  10:   4770bx  lr
  12:   bf00nop


... whereas using packed (rather than aligned(1), which I was wrong
about):


struct foo {
int a;
int b;
} __attribute__ ((packed));

int foo_cmp (struct foo *f1)
{
return (f1->a == f1->b);
};


... when compiled allowing unaligned accesses with:

  arm-linux-gnueabihf-gcc -mcpu=cortex-a15 -mabi=aapcs -O2 -c test.c 

... yields:

 :
   0:   6802ldr r2, [r0, #0]
   2:   6840ldr r0, [r0, #4]
   4:   eba2    sub.w   r0, r2, r0
   8:   fab0 f080   clz r0, r0
   c:   ea4f 1050   mov.w   r0, r0, lsr #5
  10:   4770bx  lr
  12:   bf00nop

... which should avoid alignment traps so long as SCTLR.A is clear to
permit unaligned LDR.

Thanks,
Mark.
___
ipxe-devel mailing list
ipxe-devel@lists.ipxe.org
https://lists.ipxe.org/mailman/listinfo.cgi/ipxe-devel

Re: [ipxe-devel] [PATCH 1/1] [arm] add -mno-unaligned-access compiler flag

2018-04-19 Thread Mark Rutland 
Hi,

On Thu, Apr 19, 2018 at 12:33:10AM +0100, Michael Brown wrote:
> On 18/04/18 20:21, Heinrich Schuchardt wrote:
> > the unaligned access problem with iPXE is still haunting me. Even with
> > SCTLR.A = 0 not all forms of unaligned access are allowable. When trying
> > to run iPXE on another board I experienced the following data abort:
> > 
> > static int tcp_rx ( struct io_buffer *iobuf,
> >  struct net_device *netdev __unused,
> >  struct sockaddr_tcpip *st_src,
> >  struct sockaddr_tcpip *st_dest __unused,
> >  uint16_t pshdr_csum ) {
> > 
> > ldr r5, [r0, #12] /* Here a value from iobuf is copied */

So that's the load of iobuf->data, since we have:

struct_iobuf {
struct list_head list;  // offset 0, 8 bytes
void *head; // offset 8, 4 bytes
void *data; // offset 12, 4 bytes

...
};

> > ...
> > ldrdr6, r3, [r5, #4] /* Here the data abort occurs */

This is a combined load of tcphdr->ack and tcphdr->seq, since we have:

struct tcp_header { 
uint16_t src;   // offset 0, 2 bytes
uint16_t dest;  // offset 2, 2 bytes
uint32_t seq;   // offset 4, 4 bytes
uint32_t ack;   // offset 4, 4 bytes
...
};

Per the ARM PCS, a uint32_t should have a minimum 4-byte alignment, and
hence the compiler can assume that any struct tcp_header must have a
minimum alignment of 4 bytes...

> > r5 : 7ceba822 /* This value is only halfword aligned */

... but evidently this is not the case, presumably because we have no
control over the alignment of iobuf->data.

> > All forms of LDM and STM, LDRD, RFE, SRS, STRD, SWP must be word aligned
> > even if bit SCTLR.A = 0.
> > See ARM Architecture Reference Manual, ARMv7-A and ARMv7-R edition
> 
> The fact that gcc is generating these instructions suggests that gcc
> believes that the instructions should be legal despite the lack of alignment
> guarantees.  This suggests that there should be some CPU/MMU configuration
> in which the instructions would work.

To the best of my knowledge, there is no such mode. The instructions
listed above always require word alignment, regardless of SCTLR.A or any
other configuration bit.

As above, I think that GCC is assuming the alignment required by the
PCS, and the problem is that we create a misaligned pointer contrary to
this requirement, without telling GCC we're doing something special.

> Mark: any suggestions?

Assuming data streams can be arbitrarily misaligned, we can tell GCC so
by making tcphdr __attribute__ ((aligned(1))).

As I understand it, within Linux, we have {get,put}_unaligned_*()
helpers to cater for cases like this.

For better or worse, I don't think that it is sufficient to pass the
compiler -mno-unaligned-accesses, even if it happens to mask the problem
in this specific case. The compiler can validly assume structures have
their usual alignment, and hence the LDRD should not be misaligned.

Sorry to be the bearer of bad news!

Mark.
___
ipxe-devel mailing list
ipxe-devel@lists.ipxe.org
https://lists.ipxe.org/mailman/listinfo.cgi/ipxe-devel

Re: [ipxe-devel] [PATCH 1/1] [arm] add -mno-unaligned-access compiler flag

2018-04-18 Thread Michael Brown 

On 18/04/18 20:21, Heinrich Schuchardt wrote:

the unaligned access problem with iPXE is still haunting me. Even with
SCTLR.A = 0 not all forms of unaligned access are allowable. When trying
to run iPXE on another board I experienced the following data abort:

static int tcp_rx ( struct io_buffer *iobuf,
 struct net_device *netdev __unused,
 struct sockaddr_tcpip *st_src,
 struct sockaddr_tcpip *st_dest __unused,
 uint16_t pshdr_csum ) {

ldr r5, [r0, #12] /* Here a value from iobuf is copied */
...
ldrdr6, r3, [r5, #4] /* Here the data abort occurs */

r5 : 7ceba822 /* This value is only halfword aligned */

All forms of LDM and STM, LDRD, RFE, SRS, STRD, SWP must be word aligned
even if bit SCTLR.A = 0.
See ARM Architecture Reference Manual, ARMv7-A and ARMv7-R edition


The fact that gcc is generating these instructions suggests that gcc 
believes that the instructions should be legal despite the lack of 
alignment guarantees.  This suggests that there should be some CPU/MMU 
configuration in which the instructions would work.


Mark: any suggestions?

Thanks,

Michael
___
ipxe-devel mailing list
ipxe-devel@lists.ipxe.org
https://lists.ipxe.org/mailman/listinfo.cgi/ipxe-devel

Re: [ipxe-devel] [PATCH 1/1] [arm] add -mno-unaligned-access compiler flag

2018-04-18 Thread Heinrich Schuchardt 
On 03/29/2018 06:08 PM, Heinrich Schuchardt wrote:
> On 03/29/2018 11:20 AM, Mark Rutland wrote:
>> Hi,
>>
>> On Wed, Mar 28, 2018 at 10:19:50PM +0200, Heinrich Schuchardt wrote:
>>> Without the flag a "data abort" occured when compiling with gcc 6.3 for
>>> armhf and running on an Allwinner A20 SOC in function efi_devpath_end().
>>
>> I think this may be a bug in the UEFI implementation (which I guess is
>> U-Boot?). In the 2.7 UEFI spec, section 2.3.5 states that for AArch32:
>>
>>   Unaligned access should be enabled if supported; Alignment faults are
>>   enabled otherwise.
>>
>> All ARMv6 and ARMv7 CPUs (including the COrtex-A7s in the A20 SoC)
>> support unaligned accesses to normal memory, as would be the case for
>> memory accessed by efi_devpath_end().
>>
>> Unless GCC is generating misaligned LDM/STM instructions, I believe this
>> is a FW bug that would be worth reporting, regardless of whether this
>> patch is taken on the IPXE side.
> 
> Hello Mark,
> 
> thank you for the information.
> 
> I have unset bit 1 (alignment bit) of the system control register in
> U-Boot. Now iPXE snp.efi works on the Allwinner A20 without using
> -mno-unaligned-access.
> 
> I will try upstreaming a patch for this:
> 
> diff --git a/arch/arm/cpu/armv7/start.S b/arch/arm/cpu/armv7/start.S
> index 7e2695761e..1771741119 100644
> --- a/arch/arm/cpu/armv7/start.S
> +++ b/arch/arm/cpu/armv7/start.S
> @@ -150,7 +150,6 @@ ENTRY(cpu_init_cp15)
> mrc p15, 0, r0, c1, c0, 0
> bic r0, r0, #0x2000 @ clear bits 13 (--V-)
> bic r0, r0, #0x0007 @ clear bits 2:0 (-CAM)
> -   orr r0, r0, #0x0002 @ set bit 1 (--A-) Align
> orr r0, r0, #0x0800 @ set bit 11 (Z---) BTB
>  #ifdef CONFIG_SYS_ICACHE_OFF
> bic r0, r0, #0x1000 @ clear bit 12 (I) I-cache
> 
> Best regards
> 
> Heinrich
> 
Hello Michael,

the unaligned access problem with iPXE is still haunting me. Even with
SCTLR.A = 0 not all forms of unaligned access are allowable. When trying
to run iPXE on another board I experienced the following data abort:

static int tcp_rx ( struct io_buffer *iobuf,
struct net_device *netdev __unused,
struct sockaddr_tcpip *st_src,
struct sockaddr_tcpip *st_dest __unused,
uint16_t pshdr_csum ) {

ldr r5, [r0, #12] /* Here a value from iobuf is copied */
...
ldrdr6, r3, [r5, #4] /* Here the data abort occurs */

r5 : 7ceba822 /* This value is only halfword aligned */

All forms of LDM and STM, LDRD, RFE, SRS, STRD, SWP must be word aligned
even if bit SCTLR.A = 0.
See ARM Architecture Reference Manual, ARMv7-A and ARMv7-R edition

Best regards

Heinrich
___
ipxe-devel mailing list
ipxe-devel@lists.ipxe.org
https://lists.ipxe.org/mailman/listinfo.cgi/ipxe-devel

Re: [ipxe-devel] [PATCH 1/1] [arm] add -mno-unaligned-access compiler flag

2018-03-29 Thread Heinrich Schuchardt 
On 03/29/2018 11:20 AM, Mark Rutland wrote:
> Hi,
> 
> On Wed, Mar 28, 2018 at 10:19:50PM +0200, Heinrich Schuchardt wrote:
>> Without the flag a "data abort" occured when compiling with gcc 6.3 for
>> armhf and running on an Allwinner A20 SOC in function efi_devpath_end().
> 
> I think this may be a bug in the UEFI implementation (which I guess is
> U-Boot?). In the 2.7 UEFI spec, section 2.3.5 states that for AArch32:
> 
>   Unaligned access should be enabled if supported; Alignment faults are
>   enabled otherwise.
> 
> All ARMv6 and ARMv7 CPUs (including the COrtex-A7s in the A20 SoC)
> support unaligned accesses to normal memory, as would be the case for
> memory accessed by efi_devpath_end().
> 
> Unless GCC is generating misaligned LDM/STM instructions, I believe this
> is a FW bug that would be worth reporting, regardless of whether this
> patch is taken on the IPXE side.

Hello Mark,

thank you for the information.

I have unset bit 1 (alignment bit) of the system control register in
U-Boot. Now iPXE snp.efi works on the Allwinner A20 without using
-mno-unaligned-access.

I will try upstreaming a patch for this:

diff --git a/arch/arm/cpu/armv7/start.S b/arch/arm/cpu/armv7/start.S
index 7e2695761e..1771741119 100644
--- a/arch/arm/cpu/armv7/start.S
+++ b/arch/arm/cpu/armv7/start.S
@@ -150,7 +150,6 @@ ENTRY(cpu_init_cp15)
mrc p15, 0, r0, c1, c0, 0
bic r0, r0, #0x2000 @ clear bits 13 (--V-)
bic r0, r0, #0x0007 @ clear bits 2:0 (-CAM)
-   orr r0, r0, #0x0002 @ set bit 1 (--A-) Align
orr r0, r0, #0x0800 @ set bit 11 (Z---) BTB
 #ifdef CONFIG_SYS_ICACHE_OFF
bic r0, r0, #0x1000 @ clear bit 12 (I) I-cache

Best regards

Heinrich
___
ipxe-devel mailing list
ipxe-devel@lists.ipxe.org
https://lists.ipxe.org/mailman/listinfo.cgi/ipxe-devel

Re: [ipxe-devel] [PATCH 1/1] [arm] add -mno-unaligned-access compiler flag

2018-03-29 Thread Michael Brown 

On 29/03/18 10:20, Mark Rutland wrote:

I think this may be a bug in the UEFI implementation (which I guess is
U-Boot?). In the 2.7 UEFI spec, section 2.3.5 states that for AArch32:

   Unaligned access should be enabled if supported; Alignment faults are
   enabled otherwise.

All ARMv6 and ARMv7 CPUs (including the COrtex-A7s in the A20 SoC)
support unaligned accesses to normal memory, as would be the case for
memory accessed by efi_devpath_end().

Unless GCC is generating misaligned LDM/STM instructions, I believe this
is a FW bug that would be worth reporting, regardless of whether this
patch is taken on the IPXE side.


Thanks for that information.  I'll leave the patch unapplied for now.

Heinrich: it sounds as though you should be able to switch on unaligned 
accesses on the Allwinner A20 SOC, and this will solve the problem.  If 
this turns out for some reason not to be possible, then we could 
potentially consider defining a bin-arm32-uboot platform which makes 
allowances for the areas where U-Boot deviates from the UEFI 
specification.  I'd prefer not to do this, if we can avoid it.


Michael
___
ipxe-devel mailing list
ipxe-devel@lists.ipxe.org
https://lists.ipxe.org/mailman/listinfo.cgi/ipxe-devel

Re: [ipxe-devel] [PATCH 1/1] [arm] add -mno-unaligned-access compiler flag

2018-03-29 Thread Mark Rutland 
Hi,

On Wed, Mar 28, 2018 at 10:19:50PM +0200, Heinrich Schuchardt wrote:
> Without the flag a "data abort" occured when compiling with gcc 6.3 for
> armhf and running on an Allwinner A20 SOC in function efi_devpath_end().

I think this may be a bug in the UEFI implementation (which I guess is
U-Boot?). In the 2.7 UEFI spec, section 2.3.5 states that for AArch32:

  Unaligned access should be enabled if supported; Alignment faults are
  enabled otherwise.

All ARMv6 and ARMv7 CPUs (including the COrtex-A7s in the A20 SoC)
support unaligned accesses to normal memory, as would be the case for
memory accessed by efi_devpath_end().

Unless GCC is generating misaligned LDM/STM instructions, I believe this
is a FW bug that would be worth reporting, regardless of whether this
patch is taken on the IPXE side.

Thanks,
Mark.

> 
> The flag is usable both with GCC and LLVM.
> 
> With some console commands enabled bin-arm32-efi/snp.efi grows from 148448
> to 154464 bytes.
> 
> Signed-off-by: Heinrich Schuchardt 
> ---
>  src/arch/arm32/Makefile | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/src/arch/arm32/Makefile b/src/arch/arm32/Makefile
> index 3a7c0923..fc72af11 100644
> --- a/src/arch/arm32/Makefile
> +++ b/src/arch/arm32/Makefile
> @@ -5,6 +5,7 @@ SRCDIRS   += arch/arm32/libgcc
>  
>  # ARM32-specific flags
>  #
> +CFLAGS   += -mno-unaligned-access
>  CFLAGS   += -mthumb -mcpu=cortex-a15 -mabi=aapcs -mfloat-abi=soft
>  CFLAGS   += -mword-relocations
>  ASFLAGS  += -mthumb -mcpu=cortex-a15
> -- 
> 2.11.0
> 
> ___
> ipxe-devel mailing list
> ipxe-devel@lists.ipxe.org
> https://lists.ipxe.org/mailman/listinfo.cgi/ipxe-devel
___
ipxe-devel mailing list
ipxe-devel@lists.ipxe.org
https://lists.ipxe.org/mailman/listinfo.cgi/ipxe-devel