[PATCH] ARM: refine compiler target architecture

[PATCH] ARM: refine compiler target architecture

[Andre Przywara]

Currently we compile the tools just with -marm. This breaks
compilation when the toolchain default is less than ARMv7, because
we require the "dmb" instruction in xenstored.
One possible (and working) fix would be to just adjust that for the
tools, but in fact the same rationale is true for the hypervisor.
So lets mandate ARMv7-A as the minimum for both Xen and the tools.

Unfortunately for some reasons -mcpu=cortex-a15 does not go together
with this -march, so lets be more generic and explicitly specify the
architecture extensions we actually need for the hypervisor.

This fixes native tool compilation on my Slackware system.

Signed-off-by: Andre Przywara <andre.przywara@linaro.org>
---

Not sure if this is safe enough for 4.4. On normal build environemnts
this shouldn't change anything, but one never knows.
In any case I want to drop it here for reference. I will try to do some
tests to prove that it's harmless enough.

Regards,
Andre.

 config/arm32.mk       | 4 ++--
 xen/arch/arm/Rules.mk | 3 ++-
 2 files changed, 4 insertions(+), 3 deletions(-)

diff --git a/config/arm32.mk b/config/arm32.mk
index aa79d22..c5eb30e 100644
--- a/config/arm32.mk
+++ b/config/arm32.mk
@@ -6,8 +6,8 @@ CONFIG_XEN_INSTALL_SUFFIX :=
 
 # -march= -mcpu=
 
-# Explicitly specifiy 32-bit ARM ISA since toolchain default can be -mthumb:
-CFLAGS += -marm
+# Explicitly specifiy 32-bit ARMv7-A ISA since toolchain default can be less:
+CFLAGS += -marm -march=armv7-a
 
 HAS_PL011 := y
 HAS_EXYNOS4210 := y
diff --git a/xen/arch/arm/Rules.mk b/xen/arch/arm/Rules.mk
index aaa203e..891df25 100644
--- a/xen/arch/arm/Rules.mk
+++ b/xen/arch/arm/Rules.mk
@@ -20,7 +20,8 @@ arm := y
 ifeq ($(TARGET_SUBARCH),arm32)
 # Prevent floating-point variables from creeping into Xen.
 CFLAGS += -msoft-float
-CFLAGS += -mcpu=cortex-a15
+# allow assembly of virtualization extension instructions and smc for PSCI
+CFLAGS += -Wa,-march=armv7-a+sec+virt
 arm32 := y
 arm64 := n
 endif
-- 
1.7.12.1

Re: [PATCH] ARM: refine compiler target architecture

[Ian Campbell]

On Tue, 2013-12-17 at 10:34 +0100, Andre Przywara wrote:
> Currently we compile the tools just with -marm. This breaks
> compilation when the toolchain default is less than ARMv7, because
> we require the "dmb" instruction in xenstored.
> One possible (and working) fix would be to just adjust that for the
> tools, but in fact the same rationale is true for the hypervisor.
> So lets mandate ARMv7-A as the minimum for both Xen and the tools.
> 
> Unfortunately for some reasons -mcpu=cortex-a15 does not go together
> with this -march, so lets be more generic and explicitly specify the
> architecture extensions we actually need for the hypervisor.
> 
> This fixes native tool compilation on my Slackware system.
> 
> Signed-off-by: Andre Przywara <andre.przywara@linaro.org>
> ---
> 
> Not sure if this is safe enough for 4.4. On normal build environemnts
> this shouldn't change anything, but one never knows.
> In any case I want to drop it here for reference. I will try to do some
> tests to prove that it's harmless enough.

My major concern here is that even if the tools build on an older
toolchain we require EABI layouts for shared structs (xenstore rings ,
hypercall arguments etc) and a <v7 toolchain will often be using the
OABI, which differs in this regard.

That said at least the second hunk looks like a good cleanup, but one
for post 4.4 IMHO.

> diff --git a/config/arm32.mk b/config/arm32.mk
> index aa79d22..c5eb30e 100644
> --- a/config/arm32.mk
> +++ b/config/arm32.mk
> @@ -6,8 +6,8 @@ CONFIG_XEN_INSTALL_SUFFIX :=
>  
>  # -march= -mcpu=
>  
> -# Explicitly specifiy 32-bit ARM ISA since toolchain default can be -mthumb:
> -CFLAGS += -marm
> +# Explicitly specifiy 32-bit ARMv7-A ISA since toolchain default can be less:
> +CFLAGS += -marm -march=armv7-a

The original rationale here seems a bit odd (or at least out dated) I
don't think there is any reason why thumb(2) userspace shouldn't be OK.

You carried over the misspelling of specify.

>  
>  HAS_PL011 := y
>  HAS_EXYNOS4210 := y
> diff --git a/xen/arch/arm/Rules.mk b/xen/arch/arm/Rules.mk
> index aaa203e..891df25 100644
> --- a/xen/arch/arm/Rules.mk
> +++ b/xen/arch/arm/Rules.mk
> @@ -20,7 +20,8 @@ arm := y
>  ifeq ($(TARGET_SUBARCH),arm32)
>  # Prevent floating-point variables from creeping into Xen.
>  CFLAGS += -msoft-float
> -CFLAGS += -mcpu=cortex-a15
> +# allow assembly of virtualization extension instructions and smc for PSCI
> +CFLAGS += -Wa,-march=armv7-a+sec+virt

This looks good in principal and seems to be present at least as far
back as binutils 2.21, which is the oldest I had lying around.

I wonder why we have this odd split between config/arm*.mk and
xen/arch/arm/Rules.mk.. Probably my fault somewhere in the mists of
time...

Ian.

Re: [PATCH] ARM: refine compiler target architecture

[Andre Przywara]

On 12/17/2013 11:21 AM, Ian Campbell wrote:
> On Tue, 2013-12-17 at 10:34 +0100, Andre Przywara wrote:
>> Currently we compile the tools just with -marm. This breaks
>> compilation when the toolchain default is less than ARMv7, because
>> we require the "dmb" instruction in xenstored.
>> One possible (and working) fix would be to just adjust that for the
>> tools, but in fact the same rationale is true for the hypervisor.
>> So lets mandate ARMv7-A as the minimum for both Xen and the tools.
>>
>> Unfortunately for some reasons -mcpu=cortex-a15 does not go together
>> with this -march, so lets be more generic and explicitly specify the
>> architecture extensions we actually need for the hypervisor.
>>
>> This fixes native tool compilation on my Slackware system.
>>
>> Signed-off-by: Andre Przywara <andre.przywara@linaro.org>
>> ---
>>
>> Not sure if this is safe enough for 4.4. On normal build environemnts
>> this shouldn't change anything, but one never knows.
>> In any case I want to drop it here for reference. I will try to do some
>> tests to prove that it's harmless enough.
>
> My major concern here is that even if the tools build on an older
> toolchain we require EABI layouts for shared structs (xenstore rings ,
> hypercall arguments etc) and a <v7 toolchain will often be using the
> OABI, which differs in this regard.

But actually the ABI is orthogonal to the -march setting, right? AFAIK 
-march just controls which instruction the compiler emits and which 
instructions the assembler allows. xenstored for instance uses 
xen_*mb(), which for ARM translates into "dmb" (which is >= v7), so we 
have to tell the assembler about it.
The latest ARM cross compiler I apt-got for my amd64 Debian 7.1 for 
instance gives me:
$ arm-linux-gnueabi-gcc -v
Target: arm-linux-gnueabi
... --with-arch=armv4t ...

This is pretty conservative, but the compiler is fine and with 
-march=armv7-a -mfloat-abi=hard it even generates modern code.

Are there really people out there still using OABI? Do we care?
Both compilers I tested (Slackware and Debian cross) are purely EABI, 
but have less than armv7 as their default.

Regards,
Andre.

>
> That said at least the second hunk looks like a good cleanup, but one
> for post 4.4 IMHO.
>
>> diff --git a/config/arm32.mk b/config/arm32.mk
>> index aa79d22..c5eb30e 100644
>> --- a/config/arm32.mk
>> +++ b/config/arm32.mk
>> @@ -6,8 +6,8 @@ CONFIG_XEN_INSTALL_SUFFIX :=
>>
>>   # -march= -mcpu=
>>
>> -# Explicitly specifiy 32-bit ARM ISA since toolchain default can be -mthumb:
>> -CFLAGS += -marm
>> +# Explicitly specifiy 32-bit ARMv7-A ISA since toolchain default can be less:
>> +CFLAGS += -marm -march=armv7-a
>
> The original rationale here seems a bit odd (or at least out dated) I
> don't think there is any reason why thumb(2) userspace shouldn't be OK.
>
> You carried over the misspelling of specify.
>
>>
>>   HAS_PL011 := y
>>   HAS_EXYNOS4210 := y
>> diff --git a/xen/arch/arm/Rules.mk b/xen/arch/arm/Rules.mk
>> index aaa203e..891df25 100644
>> --- a/xen/arch/arm/Rules.mk
>> +++ b/xen/arch/arm/Rules.mk
>> @@ -20,7 +20,8 @@ arm := y
>>   ifeq ($(TARGET_SUBARCH),arm32)
>>   # Prevent floating-point variables from creeping into Xen.
>>   CFLAGS += -msoft-float
>> -CFLAGS += -mcpu=cortex-a15
>> +# allow assembly of virtualization extension instructions and smc for PSCI
>> +CFLAGS += -Wa,-march=armv7-a+sec+virt
>
> This looks good in principal and seems to be present at least as far
> back as binutils 2.21, which is the oldest I had lying around.
>
> I wonder why we have this odd split between config/arm*.mk and
> xen/arch/arm/Rules.mk.. Probably my fault somewhere in the mists of
> time...
>
> Ian.
>

Re: [PATCH] ARM: refine compiler target architecture

[Ian Campbell]

On Wed, 2013-12-18 at 08:48 +0100, Andre Przywara wrote:
> But actually the ABI is orthogonal to the -march setting, right? AFAIK 
> -march just controls which instruction the compiler emits and which 
> instructions the assembler allows.

I thought they were subtly intertwined, even if technically they are
orthogonal.

> $ arm-linux-gnueabi-gcc -v
> Target: arm-linux-gnueabi
> ... --with-arch=armv4t ...
> 
> This is pretty conservative, but the compiler is fine and with 
> -march=armv7-a -mfloat-abi=hard it even generates modern code.

I think we can't force float-abi=hard unless the rest of the libraries
on the system use it? Which won't be the case on armel.

> Are there really people out there still using OABI?

I thought the Debian armel port was OABI, it seems I was mistaken,
sorry. From the native Debian armel compiler:

$ gcc -v
[...]
Target: arm-linux-gnueabi
Configured with: [...]
 --with-arch=armv4t --with-float=soft --enable-checking=release
--build=arm-linux-gnueabi --host=arm-linux-gnueabi
--target=arm-linux-gnueabi
[...]
gcc version 4.8.2 (Debian 4.8.2-7) 

https://wiki.debian.org/ArmEabiPort confirms that I was wrong.

The armhf port is EABI and v7:
$ gcc -v
[...]
Target: arm-linux-gnueabihf
Configured with: [...]
 --with-arch=armv7-a --with-fpu=vfpv3-d16 --with-float=hard
--with-mode=thumb --enable-checking=release --build=arm-linux-gnueabihf
--host=arm-linux-gnueabihf --target=arm-linux-gnueabihf
[...]
gcc version 4.6.3 (Debian 4.6.3-14) 

> Do we care?

I care about people not getting confused into think that things will
work on such systems when it will not. The bugs resulting from an OABI
vs EABI mismatch will likely be quite subtle.

I guess this means the original Debian arm (not -el nor -hf) port which
I suppose people are less likely to have these days.

> Both compilers I tested (Slackware and Debian cross) are purely EABI, 
> but have less than armv7 as their default.

I wonder if we can either add some option which will force EABI and
therefore fail to build on an OABI system (maybe march v7 is that test
given the age of OABI?) or add a configure check for struct alignment?

In any case looking back to the original patch I am now convinced that
it would be OK. I'm not sure about for 4.4 though, I'm a bit wary of
changing compiler defaults at this stage.

Ian.

Re: [PATCH] ARM: refine compiler target architecture

[Andre Przywara]

On 12/18/2013 09:29 AM, Ian Campbell wrote:
> On Wed, 2013-12-18 at 08:48 +0100, Andre Przywara wrote:
>> But actually the ABI is orthogonal to the -march setting, right? AFAIK
>> -march just controls which instruction the compiler emits and which
>> instructions the assembler allows.
>
> I thought they were subtly intertwined, even if technically they are
> orthogonal.

eek.

>
>> $ arm-linux-gnueabi-gcc -v
>> Target: arm-linux-gnueabi
>> ... --with-arch=armv4t ...
>>
>> This is pretty conservative, but the compiler is fine and with
>> -march=armv7-a -mfloat-abi=hard it even generates modern code.
>
> I think we can't force float-abi=hard unless the rest of the libraries
> on the system use it? Which won't be the case on armel.

Right. Well, we can force it, but it will not work ;-) Not sure if it 
even links.

>> Are there really people out there still using OABI?
>
> I thought the Debian armel port was OABI, it seems I was mistaken,
> sorry. From the native Debian armel compiler:
>
> $ gcc -v
> [...]
> Target: arm-linux-gnueabi
> Configured with: [...]
>   --with-arch=armv4t --with-float=soft --enable-checking=release
> --build=arm-linux-gnueabi --host=arm-linux-gnueabi
> --target=arm-linux-gnueabi
> [...]
> gcc version 4.8.2 (Debian 4.8.2-7)
>
> https://wiki.debian.org/ArmEabiPort confirms that I was wrong.
>
> The armhf port is EABI and v7:
> $ gcc -v
> [...]
> Target: arm-linux-gnueabihf
> Configured with: [...]
>   --with-arch=armv7-a --with-fpu=vfpv3-d16 --with-float=hard
> --with-mode=thumb --enable-checking=release --build=arm-linux-gnueabihf
> --host=arm-linux-gnueabihf --target=arm-linux-gnueabihf
> [...]
> gcc version 4.6.3 (Debian 4.6.3-14)
>
>> Do we care?
>
> I care about people not getting confused into think that things will
> work on such systems when it will not. The bugs resulting from an OABI
> vs EABI mismatch will likely be quite subtle.
>
> I guess this means the original Debian arm (not -el nor -hf) port which
> I suppose people are less likely to have these days.

Right, this matches my quick research. armel should work, arm not.

>> Both compilers I tested (Slackware and Debian cross) are purely EABI,
>> but have less than armv7 as their default.
>
> I wonder if we can either add some option which will force EABI and
> therefore fail to build on an OABI system (maybe march v7 is that test
> given the age of OABI?) or add a configure check for struct alignment?

Right, that sounds like a good thing. If I can find some time still this 
week, I can try to come up with something.

> In any case looking back to the original patch I am now convinced that
> it would be OK. I'm not sure about for 4.4 though, I'm a bit wary of
> changing compiler defaults at this stage.

I agree. Lets push it out for 4.5.

I can write an email with the error message I got, the explanation and 
the fix to the list to help people with this problem meanwhile. Google 
should pick it up from the archives then ;-)

Regards,
Andre.