[U-Boot] [PATCH] arm: Disable the strict alignment of data on armv7

[U-Boot] [PATCH] arm: Disable the strict alignment of data on armv7

[Michal Simek]

From: Nitin Jain <nitin.jain@xilinx.com>

This patch is used for disable the strict alignment of data
to avoid the memory alignment issues.

Also setup this option for Xilinx Zynq.

Signed-off-by: Nitin Jain <nitinj@xilinx.com>
Signed-off-by: Siva Durga Prasad Paladugu <sivadur@xilinx.com>
Signed-off-by: Michal Simek <michal.simek@xilinx.com>
---

Not sure if there is any side effects but our tests don't show up any
issue with disabling this bit.

---
 arch/arm/Kconfig           | 1 +
 arch/arm/cpu/armv7/Kconfig | 6 ++++++
 arch/arm/cpu/armv7/start.S | 2 ++
 3 files changed, 9 insertions(+)

diff --git a/arch/arm/Kconfig b/arch/arm/Kconfig
index a66d04eadfcb..4b5c64c8ba8b 100644
--- a/arch/arm/Kconfig
+++ b/arch/arm/Kconfig
@@ -778,6 +778,7 @@ config ARCH_ZYNQ
 	imply CMD_CLK
 	imply FAT_WRITE
 	imply CMD_SPL
+	imply ARMV7_MEM_ALIGN_DISABLE
 
 config ARCH_ZYNQMP
 	bool "Xilinx ZynqMP based platform"
diff --git a/arch/arm/cpu/armv7/Kconfig b/arch/arm/cpu/armv7/Kconfig
index b9c4f4e79b9b..d5c0f0ebab17 100644
--- a/arch/arm/cpu/armv7/Kconfig
+++ b/arch/arm/cpu/armv7/Kconfig
@@ -58,4 +58,10 @@ config ARMV7_LPAE
 	Say Y here to use the long descriptor page table format. This is
 	required if U-Boot runs in HYP mode.
 
+config ARMV7_MEM_ALIGN_DISABLE
+	bool "Disable strict alignment of data"
+	help
+	 Enabling this option disables strict alignment for armv7 by
+	 setting the alignment bit in system control register of cp15.
+
 endif
diff --git a/arch/arm/cpu/armv7/start.S b/arch/arm/cpu/armv7/start.S
index 7e2695761e98..795b702a5f9c 100644
--- a/arch/arm/cpu/armv7/start.S
+++ b/arch/arm/cpu/armv7/start.S
@@ -150,7 +150,9 @@ ENTRY(cpu_init_cp15)
 	mrc	p15, 0, r0, c1, c0, 0
 	bic	r0, r0, #0x00002000	@ clear bits 13 (--V-)
 	bic	r0, r0, #0x00000007	@ clear bits 2:0 (-CAM)
+#ifndef CONFIG_ARMV7_MEM_ALIGN_DISABLE
 	orr	r0, r0, #0x00000002	@ set bit 1 (--A-) Align
+#endif
 	orr	r0, r0, #0x00000800	@ set bit 11 (Z---) BTB
 #ifdef CONFIG_SYS_ICACHE_OFF
 	bic	r0, r0, #0x00001000	@ clear bit 12 (I) I-cache
-- 
1.9.1

[U-Boot] [PATCH] arm: Disable the strict alignment of data on armv7

[Wolfgang Denk]

Dear Michal,

In message <029f7f8f6d89cc77c92e04223a7402376e050f56.1520433579.git.michal.simek@xilinx.com> you wrote:
> From: Nitin Jain <nitin.jain@xilinx.com>
> 
> This patch is used for disable the strict alignment of data
> to avoid the memory alignment issues.

Can you please add some comments what the consequences of this
change are?  I guess there are advantages, but I also guess these
come at a price?

Best regards,

Wolfgang Denk

-- 
DENX Software Engineering GmbH,      Managing Director: Wolfgang Denk
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de
Quote from the Boss after overriding the decision of a task force  he
created  to  find  a  solution:  "I'm  sorry  if  I ever gave you the
impression your input would have any effect on my  decision  for  the
outcome of this project!"

[U-Boot] [PATCH] arm: Disable the strict alignment of data on armv7

[Michal Simek]

Hi Wolfgang,

On 8.3.2018 23:52, Wolfgang Denk wrote:
> Dear Michal,
> 
> In message <029f7f8f6d89cc77c92e04223a7402376e050f56.1520433579.git.michal.simek@xilinx.com> you wrote:
>> From: Nitin Jain <nitin.jain@xilinx.com>
>>
>> This patch is used for disable the strict alignment of data
>> to avoid the memory alignment issues.
> 
> Can you please add some comments what the consequences of this
> change are?  I guess there are advantages, but I also guess these
> come at a price?

That's something what I am expecting from this discussion if there are
any corner cases which we are not aware of.

We found this setting based on randomized testing where simply non
aligned access for memory read was causing exception.
The same tests were running fine on arm64 where non aligned accesses are
permitted.
It is hard to compare performance impact on u-boot but from
functionality point of view we are not able to see difference.
If there is any performance impact that it is quite low.

Thanks,
Michal

[U-Boot] [PATCH] arm: Disable the strict alignment of data on armv7

[Wolfgang Denk]

Dear Michal,

In message <f144e491-5ce5-2ad0-70a8-d36397752863@xilinx.com> you wrote:
> 
> > Can you please add some comments what the consequences of this
> > change are?  I guess there are advantages, but I also guess these
> > come at a price?
> 
> That's something what I am expecting from this discussion if there are
> any corner cases which we are not aware of.
> 
> We found this setting based on randomized testing where simply non
> aligned access for memory read was causing exception.

Hm... One possible position one can take is that unaligned accesses
are likely an indication for incorrect or sloppy programming.
usually we design data structures, buffers etc. such that no
unaligned accesses take place.  Most code is explicitly written in
such a way that it also runs on architectures where unaligned access
simply don't work.

I feel that providing such an option just opens the door for lousy
programmers who don't think throroughly about the code they write.

Yes, in some cases this may be conveniendt.  But there are also
cases where the problem is just papered over, and hits all the
harder later like when you also enable caching.

Thi is why I'm asking for the advantages.  If it's just is that
broken code starts working, then I'd rather see a clear error
message that forces the code to be fixed.

> The same tests were running fine on arm64 where non aligned accesses are
> permitted.

But I guess they still come under a (severe?) performance penalty?

> It is hard to compare performance impact on u-boot but from
> functionality point of view we are not able to see difference.
> If there is any performance impact that it is quite low.

Hm.... I have to admit that I don't know ARM/ARM64 that well, but I
am pretty sure that even on ARM an aligned 32 bit access on a 32 bit
bus will result in a single read cycle on that bus - while I would
expect that an unaligned 32 bit access might get broken down into 4
byte accesses?

Also, I wonder if ARM still maintains atomicity for unaligned reads/
writes ? [IIRC, x86 doesn't, while it's guaranteed for aligned
accesses.]

Best regards,

Wolfgang Denk

-- 
DENX Software Engineering GmbH,      Managing Director: Wolfgang Denk
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de
What is mind?  No matter.  What is matter?  Never mind.
                                      -- Thomas Hewitt Key, 1799-1875

[U-Boot] [PATCH] arm: Disable the strict alignment of data on armv7

[Michal Simek]

Dear Wolfgang,

On 9.3.2018 12:26, Wolfgang Denk wrote:
> Dear Michal,
> 
> In message <f144e491-5ce5-2ad0-70a8-d36397752863@xilinx.com> you wrote:
>>
>>> Can you please add some comments what the consequences of this
>>> change are?  I guess there are advantages, but I also guess these
>>> come at a price?
>>
>> That's something what I am expecting from this discussion if there are
>> any corner cases which we are not aware of.
>>
>> We found this setting based on randomized testing where simply non
>> aligned access for memory read was causing exception.
> 
> Hm... One possible position one can take is that unaligned accesses
> are likely an indication for incorrect or sloppy programming.
> usually we design data structures, buffers etc. such that no
> unaligned accesses take place.  Most code is explicitly written in
> such a way that it also runs on architectures where unaligned access
> simply don't work.
> 
> I feel that providing such an option just opens the door for lousy
> programmers who don't think throroughly about the code they write.
> 
> Yes, in some cases this may be conveniendt.  But there are also
> cases where the problem is just papered over, and hits all the
> harder later like when you also enable caching.
> 
> Thi is why I'm asking for the advantages.  If it's just is that
> broken code starts working, then I'd rather see a clear error
> message that forces the code to be fixed.

We are not seeing any issue from u-boot code itself and I can believe
that structures and accesses are aligned properly.

The initial reason was that u-boot allows you to run for example command
md 1 and it ends up in panic. Which is something what should be possible
to run or code should check that architecture has alignment restriction.

Definitely panic is not proper reaction on command like this.
It means we have two options:
1. enable non aligned accesses
2. fix code/commands which enables to pass these non aligned addresses
which ends up in panic

The patch is targeting option 1 because on arm64 it is permitted.

>> The same tests were running fine on arm64 where non aligned accesses are
>> permitted.
> 
> But I guess they still come under a (severe?) performance penalty?

It is really a question where that penalty is coming from and when it
can happen.

>> It is hard to compare performance impact on u-boot but from
>> functionality point of view we are not able to see difference.
>> If there is any performance impact that it is quite low.
> 
> Hm.... I have to admit that I don't know ARM/ARM64 that well, but I
> am pretty sure that even on ARM an aligned 32 bit access on a 32 bit
> bus will result in a single read cycle on that bus - while I would
> expect that an unaligned 32 bit access might get broken down into 4
> byte accesses?

I would expect that there will be more cycles for read as you mentioned.

But again the code is align and there is not an issue in code that's why
this penalty is not happening. And non align accesses are performed when
user asks for this.

> 
> Also, I wonder if ARM still maintains atomicity for unaligned reads/
> writes ? [IIRC, x86 doesn't, while it's guaranteed for aligned
> accesses.]

Don't know.

Thanks,
Michal

[U-Boot] [PATCH] arm: Disable the strict alignment of data on armv7

[Wolfgang Denk]

Dear Michal,

In message <fc0a315b-4e51-a009-05e3-1a140bf82155@xilinx.com> you wrote:
> 
> We are not seeing any issue from u-boot code itself and I can believe
> that structures and accesses are aligned properly.
> 
> The initial reason was that u-boot allows you to run for example command
> md 1 and it ends up in panic. Which is something what should be possible
> to run or code should check that architecture has alignment restriction.

I'm not sure this needs any "fixing".  Actually I use this on a
regular base as a simple means to find out if an architecutre allos
unaligned accesses...  U-Boot is just a boot loader.  It has no
"normal users" - users are usually developers who know what they are
doing.  I think this is an area where this old quote applies nicely:

"UNIX was not designed to stop you from doing stupid things,  because
that would also stop you from doing clever things."       - Doug Gwyn

> Definitely panic is not proper reaction on command like this.

Why not?  Yes, we can blow up the code to handle this in a better
way, and add some kB of stricgs for nice and user-friendy warnings
not to do this or that.  But is it really worth the effort?
I consider this a pretty useful feature and would be sad if it went
missing. If someone runs into it by accident, he learns a lesson -
but htere is no real damage done.  Or am I missing anything?

> It means we have two options:
> 1. enable non aligned accesses
> 2. fix code/commands which enables to pass these non aligned addresses
> which ends up in panic

My vorte is for option 3:

3. Leave as is.  It's a boot loader, and like other sharp tools it
can be dangerous if improperly used.

> > But I guess they still come under a (severe?) performance penalty?
> 
> It is really a question where that penalty is coming from and when it
> can happen.

It comes from the memory and cache architecture.  Just assume an
unaligned read of 2 bytes that happen to cross a cacheline boundary.
This access causes a double cache miss; in the worst case this
causes two cache lines to be flushed to memory and then two
cachelines to be written from memory.  Instead of just reading two
bytes, we write 128 bytes and read 128 bytes.  Yes, this is a
pathological case, but you get the idea.

> But again the code is align and there is not an issue in code that's why
> this penalty is not happening. And non align accesses are performed when
> user asks for this.

Why not tell the user: don't perform unaligend accesses on this
platform then?  I can't see the situation where he really _needs_ to
do something which is not natively supported by his hardware?

> > Also, I wonder if ARM still maintains atomicity for unaligned reads/
> > writes ? [IIRC, x86 doesn't, while it's guaranteed for aligned
> > accesses.]
> 
> Don't know.

Well, I'm willing to bet a few beer this doesn't work at least in
pathological cases like example of crossing cache lines above.

Best regards,

Wolfgang Denk

-- 
DENX Software Engineering GmbH,      Managing Director: Wolfgang Denk
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de
He'd heard her use that sweet, innocent  tone  of  voice  before.  It
meant that, pretty soon, there was going to be trouble.
                                        - Terry Pratchett, _Truckers_

[U-Boot] [PATCH] arm: Disable the strict alignment of data on armv7

[Mark Kettenis]

> From: Wolfgang Denk <wd@denx.de>
> Date: Fri, 09 Mar 2018 12:26:01 +0100
> 
> Dear Michal,
> 
> In message <f144e491-5ce5-2ad0-70a8-d36397752863@xilinx.com> you wrote:
> > 
> > > Can you please add some comments what the consequences of this
> > > change are?  I guess there are advantages, but I also guess these
> > > come at a price?
> > 
> > That's something what I am expecting from this discussion if there are
> > any corner cases which we are not aware of.
> > 
> > We found this setting based on randomized testing where simply non
> > aligned access for memory read was causing exception.
> 
> Hm... One possible position one can take is that unaligned accesses
> are likely an indication for incorrect or sloppy programming.
> usually we design data structures, buffers etc. such that no
> unaligned accesses take place.  Most code is explicitly written in
> such a way that it also runs on architectures where unaligned access
> simply don't work.
> 
> I feel that providing such an option just opens the door for lousy
> programmers who don't think throroughly about the code they write.
> 
> Yes, in some cases this may be conveniendt.  But there are also
> cases where the problem is just papered over, and hits all the
> harder later like when you also enable caching.

That is pretty much why we run OpenBSD in strict alignment mode on as
much architectures as possible.  More strict-alignment architectures
means that bugs are found quicker.  So as long as U-Boot supports
hardware that cares about alignment (e.g. older ARM, MIPS, SuperH) I'd
say there is a clear benefit of running armv7 in strict alignment mode.

Also note that some armv7 instructions still require properly aligned
data.  And that really old ARM hardware silently fetches the wrong
data for a misaligned load instruction.

[U-Boot] [PATCH] arm: Disable the strict alignment of data on armv7

[Michal Simek]

Dear Wolfgang,

On 9.3.2018 13:02, Wolfgang Denk wrote:
> Dear Michal,
> 
> In message <fc0a315b-4e51-a009-05e3-1a140bf82155@xilinx.com> you wrote:
>>
>> We are not seeing any issue from u-boot code itself and I can believe
>> that structures and accesses are aligned properly.
>>
>> The initial reason was that u-boot allows you to run for example command
>> md 1 and it ends up in panic. Which is something what should be possible
>> to run or code should check that architecture has alignment restriction.
> 
> I'm not sure this needs any "fixing".  Actually I use this on a
> regular base as a simple means to find out if an architecutre allos
> unaligned accesses...  U-Boot is just a boot loader.  It has no
> "normal users" - users are usually developers who know what they are
> doing.  I think this is an area where this old quote applies nicely:
> 
> "UNIX was not designed to stop you from doing stupid things,  because
> that would also stop you from doing clever things."       - Doug Gwyn

I understand your opinion and no problem with it.


>> Definitely panic is not proper reaction on command like this.
> 
> Why not?  Yes, we can blow up the code to handle this in a better
> way, and add some kB of stricgs for nice and user-friendy warnings
> not to do this or that.  But is it really worth the effort?
> I consider this a pretty useful feature and would be sad if it went
> missing. If someone runs into it by accident, he learns a lesson -
> but htere is no real damage done.  Or am I missing anything?

It is not that the patch is removing or changing current behavior.
Default option is to use strict alignment and have an option to change
this behavior.


>> It means we have two options:
>> 1. enable non aligned accesses
>> 2. fix code/commands which enables to pass these non aligned addresses
>> which ends up in panic
> 
> My vorte is for option 3:
> 
> 3. Leave as is.  It's a boot loader, and like other sharp tools it
> can be dangerous if improperly used.

Sure that's also an option but it seems to me weird that by default
armv7 has this requirement and a53 not.


>>> But I guess they still come under a (severe?) performance penalty?
>>
>> It is really a question where that penalty is coming from and when it
>> can happen.
> 
> It comes from the memory and cache architecture.  Just assume an
> unaligned read of 2 bytes that happen to cross a cacheline boundary.
> This access causes a double cache miss; in the worst case this
> causes two cache lines to be flushed to memory and then two
> cachelines to be written from memory.  Instead of just reading two
> bytes, we write 128 bytes and read 128 bytes.  Yes, this is a
> pathological case, but you get the idea.

If this is WB case that write shouldn't be there but you are right that
this could happen.

>> But again the code is align and there is not an issue in code that's why
>> this penalty is not happening. And non align accesses are performed when
>> user asks for this.
> 
> Why not tell the user: don't perform unaligend accesses on this
> platform then?  I can't see the situation where he really _needs_ to
> do something which is not natively supported by his hardware?

It is supported by hardware. U-Boot/SW just doing this configuration not
to allow it.

>>> Also, I wonder if ARM still maintains atomicity for unaligned reads/
>>> writes ? [IIRC, x86 doesn't, while it's guaranteed for aligned
>>> accesses.]
>>
>> Don't know.
> 
> Well, I'm willing to bet a few beer this doesn't work at least in
> pathological cases like example of crossing cache lines above.

:-)

Thanks,
Michal

[U-Boot] [PATCH] arm: Disable the strict alignment of data on armv7

[Michal Simek]

On 9.3.2018 13:23, Mark Kettenis wrote:
>> From: Wolfgang Denk <wd@denx.de>
>> Date: Fri, 09 Mar 2018 12:26:01 +0100
>>
>> Dear Michal,
>>
>> In message <f144e491-5ce5-2ad0-70a8-d36397752863@xilinx.com> you wrote:
>>>
>>>> Can you please add some comments what the consequences of this
>>>> change are?  I guess there are advantages, but I also guess these
>>>> come at a price?
>>>
>>> That's something what I am expecting from this discussion if there are
>>> any corner cases which we are not aware of.
>>>
>>> We found this setting based on randomized testing where simply non
>>> aligned access for memory read was causing exception.
>>
>> Hm... One possible position one can take is that unaligned accesses
>> are likely an indication for incorrect or sloppy programming.
>> usually we design data structures, buffers etc. such that no
>> unaligned accesses take place.  Most code is explicitly written in
>> such a way that it also runs on architectures where unaligned access
>> simply don't work.
>>
>> I feel that providing such an option just opens the door for lousy
>> programmers who don't think throroughly about the code they write.
>>
>> Yes, in some cases this may be conveniendt.  But there are also
>> cases where the problem is just papered over, and hits all the
>> harder later like when you also enable caching.
> 
> That is pretty much why we run OpenBSD in strict alignment mode on as
> much architectures as possible.  More strict-alignment architectures
> means that bugs are found quicker.  So as long as U-Boot supports
> hardware that cares about alignment (e.g. older ARM, MIPS, SuperH) I'd
> say there is a clear benefit of running armv7 in strict alignment mode.
> 
> Also note that some armv7 instructions still require properly aligned
> data.  And that really old ARM hardware silently fetches the wrong
> data for a misaligned load instruction.
> 

No problem with this at all. I wanted to check that this is intended setup.

Thanks,
Michal