[U-Boot] [PATCH] arm: Disable the strict alignment of data on armv7
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
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
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
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
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
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:
- enable non aligned accesses
- 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
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:
- enable non aligned accesses
- fix code/commands which enables to pass these non aligned addresses
which ends up in panic
My vorte is for option 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
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.
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
participants (3)
-
Mark Kettenis -
Michal Simek -
Wolfgang Denk