linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH 1/2] ARM: kvm: fix building with gcc-8
@ 2018-02-02 15:07 Arnd Bergmann
  2018-02-02 15:07 ` [PATCH 2/2] ARM: xscale: fix gcc-8 build Arnd Bergmann
                   ` (2 more replies)
  0 siblings, 3 replies; 10+ messages in thread
From: Arnd Bergmann @ 2018-02-02 15:07 UTC (permalink / raw)
  To: Christoffer Dall, Marc Zyngier, Russell King
  Cc: Nicolas Pitre, Andi Kleen, Richard Earnshaw, Tamar Christina,
	Arnd Bergmann, stable, Julien Thierry, linux-arm-kernel, kvmarm,
	linux-kernel

In banked-sr.c, we use a top-level '__asm__(".arch_extension virt")'
statement to allow compilation of a multi-CPU kernel for ARMv6
and older ARMv7-A that don't normally support access to the banked
registers.

This is considered to be a programming error by the gcc developers
and will no longer work in gcc-8, where we now get a build error:

/tmp/cc4Qy7GR.s:34: Error: Banked registers are not available with this architecture. -- `mrs r3,SP_usr'
/tmp/cc4Qy7GR.s:41: Error: Banked registers are not available with this architecture. -- `mrs r3,ELR_hyp'
/tmp/cc4Qy7GR.s:55: Error: Banked registers are not available with this architecture. -- `mrs r3,SP_svc'
/tmp/cc4Qy7GR.s:62: Error: Banked registers are not available with this architecture. -- `mrs r3,LR_svc'
/tmp/cc4Qy7GR.s:69: Error: Banked registers are not available with this architecture. -- `mrs r3,SPSR_svc'
/tmp/cc4Qy7GR.s:76: Error: Banked registers are not available with this architecture. -- `mrs r3,SP_abt'

Passign the '-march-armv7ve' flag to gcc works, and is ok here, because
we know the functions won't ever be called on pre-ARMv7VE machines.
Unfortunately, older compiler versions (4.8 and earlier) do not understand
that flag, so we still need to keep the asm around.

Backporting to stable kernels (4.6+) is needed to allow those to be built
with future compilers as well.

Link: https://gcc.gnu.org/bugzilla/show_bug.cgi?id=84129
Fixes: 33280b4cd1dc ("ARM: KVM: Add banked registers save/restore")
Cc: stable@vger.kernel.org
Signed-off-by: Arnd Bergmann <arnd@arndb.de>
---
 arch/arm/kvm/hyp/Makefile    | 5 +++++
 arch/arm/kvm/hyp/banked-sr.c | 4 ++++
 2 files changed, 9 insertions(+)

diff --git a/arch/arm/kvm/hyp/Makefile b/arch/arm/kvm/hyp/Makefile
index 5638ce0c9524..63d6b404d88e 100644
--- a/arch/arm/kvm/hyp/Makefile
+++ b/arch/arm/kvm/hyp/Makefile
@@ -7,6 +7,8 @@ ccflags-y += -fno-stack-protector -DDISABLE_BRANCH_PROFILING
 
 KVM=../../../../virt/kvm
 
+CFLAGS_ARMV7VE		   :=$(call cc-option, -march=armv7ve)
+
 obj-$(CONFIG_KVM_ARM_HOST) += $(KVM)/arm/hyp/vgic-v2-sr.o
 obj-$(CONFIG_KVM_ARM_HOST) += $(KVM)/arm/hyp/vgic-v3-sr.o
 obj-$(CONFIG_KVM_ARM_HOST) += $(KVM)/arm/hyp/timer-sr.o
@@ -15,7 +17,10 @@ obj-$(CONFIG_KVM_ARM_HOST) += tlb.o
 obj-$(CONFIG_KVM_ARM_HOST) += cp15-sr.o
 obj-$(CONFIG_KVM_ARM_HOST) += vfp.o
 obj-$(CONFIG_KVM_ARM_HOST) += banked-sr.o
+CFLAGS_banked-sr.o	   += $(CFLAGS_ARMV7VE)
+
 obj-$(CONFIG_KVM_ARM_HOST) += entry.o
 obj-$(CONFIG_KVM_ARM_HOST) += hyp-entry.o
 obj-$(CONFIG_KVM_ARM_HOST) += switch.o
+CFLAGS_switch.o		   += $(CFLAGS_ARMV7VE)
 obj-$(CONFIG_KVM_ARM_HOST) += s2-setup.o
diff --git a/arch/arm/kvm/hyp/banked-sr.c b/arch/arm/kvm/hyp/banked-sr.c
index 111bda8cdebd..be4b8b0a40ad 100644
--- a/arch/arm/kvm/hyp/banked-sr.c
+++ b/arch/arm/kvm/hyp/banked-sr.c
@@ -20,6 +20,10 @@
 
 #include <asm/kvm_hyp.h>
 
+/*
+ * gcc before 4.9 doesn't understand -march=armv7ve, so we have to
+ * trick the assembler.
+ */
 __asm__(".arch_extension     virt");
 
 void __hyp_text __banked_save_state(struct kvm_cpu_context *ctxt)
-- 
2.9.0

^ permalink raw reply related	[flat|nested] 10+ messages in thread

* [PATCH 2/2] ARM: xscale: fix gcc-8 build
  2018-02-02 15:07 [PATCH 1/2] ARM: kvm: fix building with gcc-8 Arnd Bergmann
@ 2018-02-02 15:07 ` Arnd Bergmann
  2018-02-02 16:39   ` Nicolas Pitre
  2018-02-02 15:55 ` [PATCH 1/2] ARM: kvm: fix building with gcc-8 Robin Murphy
  2018-02-04 18:45 ` Christoffer Dall
  2 siblings, 1 reply; 10+ messages in thread
From: Arnd Bergmann @ 2018-02-02 15:07 UTC (permalink / raw)
  To: Russell King
  Cc: Nicolas Pitre, Andi Kleen, Richard Earnshaw, Tamar Christina,
	Arnd Bergmann, linux-arm-kernel, linux-kernel

We use a hack in xscale-cp0.c to allow building it for ARMv4 while
also using ARMv5TE and iWMMXt specific inline assembly, by
adding a top-level asm statement.

Unfortunately that hack no longer works with gcc-8, since it will
revert back to the normal architecture. The recommended way of
handling this is to use __attribute__((target("armv5te"))) on the
functions that need it, or #pragma GCC target("arch=armv5te").
Either of those work with gcc-8, but not earlier versions, and
it seems worse to combine that with the old hack.

Instead, this adds the .arch statement to each inline assembler
statement that needs it individually. That is also slightly uglier
than the previous hack, but it works with all compiler versions
and documents better why we need the override in the first place.

Link: https://gcc.gnu.org/bugzilla/show_bug.cgi?id=84129
Signed-off-by: Arnd Bergmann <arnd@arndb.de>
---
 arch/arm/kernel/xscale-cp0.c | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

diff --git a/arch/arm/kernel/xscale-cp0.c b/arch/arm/kernel/xscale-cp0.c
index 77a2eef72115..e06a2f6dac4f 100644
--- a/arch/arm/kernel/xscale-cp0.c
+++ b/arch/arm/kernel/xscale-cp0.c
@@ -17,11 +17,10 @@
 #include <asm/thread_notify.h>
 #include <asm/cputype.h>
 
-asm("	.arch armv5te\n");
-
 static inline void dsp_save_state(u32 *state)
 {
 	__asm__ __volatile__ (
+		".arch	armv5te\n\t"
 		"mrrc	p0, 0, %0, %1, c0\n"
 		: "=r" (state[0]), "=r" (state[1]));
 }
@@ -29,6 +28,7 @@ static inline void dsp_save_state(u32 *state)
 static inline void dsp_load_state(u32 *state)
 {
 	__asm__ __volatile__ (
+		".arch	armv5te\n\t"
 		"mcrr	p0, 0, %0, %1, c0\n"
 		: : "r" (state[0]), "r" (state[1]));
 }
@@ -134,7 +134,8 @@ static int __init cpu_has_iwmmxt(void)
 	 *	tmrrc	%0, %1, wR0
 	 */
 	__asm__ __volatile__ (
-		"mcrr	p0, 0, %2, %3, c0\n"
+		".arch	armv5te\n\t"
+		"mcrr	p0, 0, %2, %3, c0\n\t"
 		"mrrc	p0, 0, %0, %1, c0\n"
 		: "=r" (lo), "=r" (hi)
 		: "r" (0), "r" (0x100));
-- 
2.9.0

^ permalink raw reply related	[flat|nested] 10+ messages in thread

* Re: [PATCH 1/2] ARM: kvm: fix building with gcc-8
  2018-02-02 15:07 [PATCH 1/2] ARM: kvm: fix building with gcc-8 Arnd Bergmann
  2018-02-02 15:07 ` [PATCH 2/2] ARM: xscale: fix gcc-8 build Arnd Bergmann
@ 2018-02-02 15:55 ` Robin Murphy
  2018-02-02 16:23   ` Robin Murphy
  2018-02-04 18:45 ` Christoffer Dall
  2 siblings, 1 reply; 10+ messages in thread
From: Robin Murphy @ 2018-02-02 15:55 UTC (permalink / raw)
  To: Arnd Bergmann, Christoffer Dall, Marc Zyngier, Russell King
  Cc: Andi Kleen, Nicolas Pitre, Julien Thierry, linux-kernel, stable,
	kvmarm, linux-arm-kernel, Richard Earnshaw, Tamar Christina

On 02/02/18 15:07, Arnd Bergmann wrote:
> In banked-sr.c, we use a top-level '__asm__(".arch_extension virt")'
> statement to allow compilation of a multi-CPU kernel for ARMv6
> and older ARMv7-A that don't normally support access to the banked
> registers.
> 
> This is considered to be a programming error by the gcc developers
> and will no longer work in gcc-8, where we now get a build error:
> 
> /tmp/cc4Qy7GR.s:34: Error: Banked registers are not available with this architecture. -- `mrs r3,SP_usr'
> /tmp/cc4Qy7GR.s:41: Error: Banked registers are not available with this architecture. -- `mrs r3,ELR_hyp'
> /tmp/cc4Qy7GR.s:55: Error: Banked registers are not available with this architecture. -- `mrs r3,SP_svc'
> /tmp/cc4Qy7GR.s:62: Error: Banked registers are not available with this architecture. -- `mrs r3,LR_svc'
> /tmp/cc4Qy7GR.s:69: Error: Banked registers are not available with this architecture. -- `mrs r3,SPSR_svc'
> /tmp/cc4Qy7GR.s:76: Error: Banked registers are not available with this architecture. -- `mrs r3,SP_abt'
> 
> Passign the '-march-armv7ve' flag to gcc works, and is ok here, because
> we know the functions won't ever be called on pre-ARMv7VE machines.
> Unfortunately, older compiler versions (4.8 and earlier) do not understand
> that flag, so we still need to keep the asm around.
> 
> Backporting to stable kernels (4.6+) is needed to allow those to be built
> with future compilers as well.

Is "-Wa,arch=armv7-a+virt" (as we appear to do for a couple of files 
already) viable as a possibly cleaner alternative, or is GCC itself now 
policing the contents of inline asms?

> Link: https://gcc.gnu.org/bugzilla/show_bug.cgi?id=84129
> Fixes: 33280b4cd1dc ("ARM: KVM: Add banked registers save/restore")
> Cc: stable@vger.kernel.org
> Signed-off-by: Arnd Bergmann <arnd@arndb.de>
> ---
>   arch/arm/kvm/hyp/Makefile    | 5 +++++
>   arch/arm/kvm/hyp/banked-sr.c | 4 ++++
>   2 files changed, 9 insertions(+)
> 
> diff --git a/arch/arm/kvm/hyp/Makefile b/arch/arm/kvm/hyp/Makefile
> index 5638ce0c9524..63d6b404d88e 100644
> --- a/arch/arm/kvm/hyp/Makefile
> +++ b/arch/arm/kvm/hyp/Makefile
> @@ -7,6 +7,8 @@ ccflags-y += -fno-stack-protector -DDISABLE_BRANCH_PROFILING
>   
>   KVM=../../../../virt/kvm
>   
> +CFLAGS_ARMV7VE		   :=$(call cc-option, -march=armv7ve)
> +
>   obj-$(CONFIG_KVM_ARM_HOST) += $(KVM)/arm/hyp/vgic-v2-sr.o
>   obj-$(CONFIG_KVM_ARM_HOST) += $(KVM)/arm/hyp/vgic-v3-sr.o
>   obj-$(CONFIG_KVM_ARM_HOST) += $(KVM)/arm/hyp/timer-sr.o
> @@ -15,7 +17,10 @@ obj-$(CONFIG_KVM_ARM_HOST) += tlb.o
>   obj-$(CONFIG_KVM_ARM_HOST) += cp15-sr.o
>   obj-$(CONFIG_KVM_ARM_HOST) += vfp.o
>   obj-$(CONFIG_KVM_ARM_HOST) += banked-sr.o
> +CFLAGS_banked-sr.o	   += $(CFLAGS_ARMV7VE)
> +
>   obj-$(CONFIG_KVM_ARM_HOST) += entry.o
>   obj-$(CONFIG_KVM_ARM_HOST) += hyp-entry.o
>   obj-$(CONFIG_KVM_ARM_HOST) += switch.o
> +CFLAGS_switch.o		   += $(CFLAGS_ARMV7VE)
>   obj-$(CONFIG_KVM_ARM_HOST) += s2-setup.o
> diff --git a/arch/arm/kvm/hyp/banked-sr.c b/arch/arm/kvm/hyp/banked-sr.c
> index 111bda8cdebd..be4b8b0a40ad 100644
> --- a/arch/arm/kvm/hyp/banked-sr.c
> +++ b/arch/arm/kvm/hyp/banked-sr.c
> @@ -20,6 +20,10 @@
>   
>   #include <asm/kvm_hyp.h>
>   
> +/*
> + * gcc before 4.9 doesn't understand -march=armv7ve, so we have to
> + * trick the assembler.
> + */
>   __asm__(".arch_extension     virt");

Would it be worth wrapping this in a preprocessor check for compilers 
that won't understand the command-line flag? I believe LLVM tends to 
choke on these global asm statements entirely, so minimising exposure 
might be a good thing to do in general.

Robin.

>   
>   void __hyp_text __banked_save_state(struct kvm_cpu_context *ctxt)
> 

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH 1/2] ARM: kvm: fix building with gcc-8
  2018-02-02 15:55 ` [PATCH 1/2] ARM: kvm: fix building with gcc-8 Robin Murphy
@ 2018-02-02 16:23   ` Robin Murphy
  2018-02-02 16:29     ` Arnd Bergmann
  0 siblings, 1 reply; 10+ messages in thread
From: Robin Murphy @ 2018-02-02 16:23 UTC (permalink / raw)
  To: Arnd Bergmann, Christoffer Dall, Marc Zyngier, Russell King
  Cc: Andi Kleen, Julien Thierry, Nicolas Pitre, linux-kernel, stable,
	Richard Earnshaw, Tamar Christina, kvmarm, linux-arm-kernel

On 02/02/18 15:55, Robin Murphy wrote:
> On 02/02/18 15:07, Arnd Bergmann wrote:
>> In banked-sr.c, we use a top-level '__asm__(".arch_extension virt")'
>> statement to allow compilation of a multi-CPU kernel for ARMv6
>> and older ARMv7-A that don't normally support access to the banked
>> registers.
>>
>> This is considered to be a programming error by the gcc developers
>> and will no longer work in gcc-8, where we now get a build error:
>>
>> /tmp/cc4Qy7GR.s:34: Error: Banked registers are not available with 
>> this architecture. -- `mrs r3,SP_usr'
>> /tmp/cc4Qy7GR.s:41: Error: Banked registers are not available with 
>> this architecture. -- `mrs r3,ELR_hyp'
>> /tmp/cc4Qy7GR.s:55: Error: Banked registers are not available with 
>> this architecture. -- `mrs r3,SP_svc'
>> /tmp/cc4Qy7GR.s:62: Error: Banked registers are not available with 
>> this architecture. -- `mrs r3,LR_svc'
>> /tmp/cc4Qy7GR.s:69: Error: Banked registers are not available with 
>> this architecture. -- `mrs r3,SPSR_svc'
>> /tmp/cc4Qy7GR.s:76: Error: Banked registers are not available with 
>> this architecture. -- `mrs r3,SP_abt'
>>
>> Passign the '-march-armv7ve' flag to gcc works, and is ok here, because
>> we know the functions won't ever be called on pre-ARMv7VE machines.
>> Unfortunately, older compiler versions (4.8 and earlier) do not 
>> understand
>> that flag, so we still need to keep the asm around.
>>
>> Backporting to stable kernels (4.6+) is needed to allow those to be built
>> with future compilers as well.
> 
> Is "-Wa,arch=armv7-a+virt" (as we appear to do for a couple of files 
> already) viable as a possibly cleaner alternative, or is GCC itself now 
> policing the contents of inline asms?

In fact, looking at the binutils history, any version capable of 
assembling this file should understand that (modulo my typo), so 
hopefully it ought to be feasible to replace these global asms with 
assembler flags entirely.

Robin.

>> Link: https://gcc.gnu.org/bugzilla/show_bug.cgi?id=84129
>> Fixes: 33280b4cd1dc ("ARM: KVM: Add banked registers save/restore")
>> Cc: stable@vger.kernel.org
>> Signed-off-by: Arnd Bergmann <arnd@arndb.de>
>> ---
>>   arch/arm/kvm/hyp/Makefile    | 5 +++++
>>   arch/arm/kvm/hyp/banked-sr.c | 4 ++++
>>   2 files changed, 9 insertions(+)
>>
>> diff --git a/arch/arm/kvm/hyp/Makefile b/arch/arm/kvm/hyp/Makefile
>> index 5638ce0c9524..63d6b404d88e 100644
>> --- a/arch/arm/kvm/hyp/Makefile
>> +++ b/arch/arm/kvm/hyp/Makefile
>> @@ -7,6 +7,8 @@ ccflags-y += -fno-stack-protector 
>> -DDISABLE_BRANCH_PROFILING
>>   KVM=../../../../virt/kvm
>> +CFLAGS_ARMV7VE           :=$(call cc-option, -march=armv7ve)
>> +
>>   obj-$(CONFIG_KVM_ARM_HOST) += $(KVM)/arm/hyp/vgic-v2-sr.o
>>   obj-$(CONFIG_KVM_ARM_HOST) += $(KVM)/arm/hyp/vgic-v3-sr.o
>>   obj-$(CONFIG_KVM_ARM_HOST) += $(KVM)/arm/hyp/timer-sr.o
>> @@ -15,7 +17,10 @@ obj-$(CONFIG_KVM_ARM_HOST) += tlb.o
>>   obj-$(CONFIG_KVM_ARM_HOST) += cp15-sr.o
>>   obj-$(CONFIG_KVM_ARM_HOST) += vfp.o
>>   obj-$(CONFIG_KVM_ARM_HOST) += banked-sr.o
>> +CFLAGS_banked-sr.o       += $(CFLAGS_ARMV7VE)
>> +
>>   obj-$(CONFIG_KVM_ARM_HOST) += entry.o
>>   obj-$(CONFIG_KVM_ARM_HOST) += hyp-entry.o
>>   obj-$(CONFIG_KVM_ARM_HOST) += switch.o
>> +CFLAGS_switch.o           += $(CFLAGS_ARMV7VE)
>>   obj-$(CONFIG_KVM_ARM_HOST) += s2-setup.o
>> diff --git a/arch/arm/kvm/hyp/banked-sr.c b/arch/arm/kvm/hyp/banked-sr.c
>> index 111bda8cdebd..be4b8b0a40ad 100644
>> --- a/arch/arm/kvm/hyp/banked-sr.c
>> +++ b/arch/arm/kvm/hyp/banked-sr.c
>> @@ -20,6 +20,10 @@
>>   #include <asm/kvm_hyp.h>
>> +/*
>> + * gcc before 4.9 doesn't understand -march=armv7ve, so we have to
>> + * trick the assembler.
>> + */
>>   __asm__(".arch_extension     virt");
> 
> Would it be worth wrapping this in a preprocessor check for compilers 
> that won't understand the command-line flag? I believe LLVM tends to 
> choke on these global asm statements entirely, so minimising exposure 
> might be a good thing to do in general.
> 
> Robin.
> 
>>   void __hyp_text __banked_save_state(struct kvm_cpu_context *ctxt)
>>
> 
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH 1/2] ARM: kvm: fix building with gcc-8
  2018-02-02 16:23   ` Robin Murphy
@ 2018-02-02 16:29     ` Arnd Bergmann
  2018-02-02 16:30       ` Robin Murphy
  0 siblings, 1 reply; 10+ messages in thread
From: Arnd Bergmann @ 2018-02-02 16:29 UTC (permalink / raw)
  To: Robin Murphy
  Cc: Christoffer Dall, Marc Zyngier, Russell King, Andi Kleen,
	Julien Thierry, Nicolas Pitre, Linux Kernel Mailing List,
	# 3.4.x, Richard Earnshaw, Tamar Christina, kvmarm, Linux ARM

On Fri, Feb 2, 2018 at 5:23 PM, Robin Murphy <robin.murphy@arm.com> wrote:
> On 02/02/18 15:55, Robin Murphy wrote:
>>
>> On 02/02/18 15:07, Arnd Bergmann wrote:
>>>
>>> In banked-sr.c, we use a top-level '__asm__(".arch_extension virt")'
>>> statement to allow compilation of a multi-CPU kernel for ARMv6
>>> and older ARMv7-A that don't normally support access to the banked
>>> registers.
>>>
>>> This is considered to be a programming error by the gcc developers
>>> and will no longer work in gcc-8, where we now get a build error:
>>>
>>> /tmp/cc4Qy7GR.s:34: Error: Banked registers are not available with this
>>> architecture. -- `mrs r3,SP_usr'
>>> /tmp/cc4Qy7GR.s:41: Error: Banked registers are not available with this
>>> architecture. -- `mrs r3,ELR_hyp'
>>> /tmp/cc4Qy7GR.s:55: Error: Banked registers are not available with this
>>> architecture. -- `mrs r3,SP_svc'
>>> /tmp/cc4Qy7GR.s:62: Error: Banked registers are not available with this
>>> architecture. -- `mrs r3,LR_svc'
>>> /tmp/cc4Qy7GR.s:69: Error: Banked registers are not available with this
>>> architecture. -- `mrs r3,SPSR_svc'
>>> /tmp/cc4Qy7GR.s:76: Error: Banked registers are not available with this
>>> architecture. -- `mrs r3,SP_abt'
>>>
>>> Passign the '-march-armv7ve' flag to gcc works, and is ok here, because
>>> we know the functions won't ever be called on pre-ARMv7VE machines.
>>> Unfortunately, older compiler versions (4.8 and earlier) do not
>>> understand
>>> that flag, so we still need to keep the asm around.
>>>
>>> Backporting to stable kernels (4.6+) is needed to allow those to be built
>>> with future compilers as well.
>>
>>
>> Is "-Wa,arch=armv7-a+virt" (as we appear to do for a couple of files
>> already) viable as a possibly cleaner alternative, or is GCC itself now
>> policing the contents of inline asms?
>
>
> In fact, looking at the binutils history, any version capable of assembling
> this file should understand that (modulo my typo), so hopefully it ought to
> be feasible to replace these global asms with assembler flags entirely.

No, this only works for .S files, not .c, since gcc starts the output with
an explicit .arch setting that overrides the command line. I think this
was done intentionally to prevent such a hack from working, and have
more reliable checks on the validity of the assembler instruction in
inline asm statements (which we try to circumvent here).

      Arnd

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH 1/2] ARM: kvm: fix building with gcc-8
  2018-02-02 16:29     ` Arnd Bergmann
@ 2018-02-02 16:30       ` Robin Murphy
  0 siblings, 0 replies; 10+ messages in thread
From: Robin Murphy @ 2018-02-02 16:30 UTC (permalink / raw)
  To: Arnd Bergmann
  Cc: Christoffer Dall, Marc Zyngier, Russell King, Andi Kleen,
	Julien Thierry, Nicolas Pitre, Linux Kernel Mailing List,
	# 3.4.x, Richard Earnshaw, Tamar Christina, kvmarm, Linux ARM

On 02/02/18 16:29, Arnd Bergmann wrote:
> On Fri, Feb 2, 2018 at 5:23 PM, Robin Murphy <robin.murphy@arm.com> wrote:
>> On 02/02/18 15:55, Robin Murphy wrote:
>>>
>>> On 02/02/18 15:07, Arnd Bergmann wrote:
>>>>
>>>> In banked-sr.c, we use a top-level '__asm__(".arch_extension virt")'
>>>> statement to allow compilation of a multi-CPU kernel for ARMv6
>>>> and older ARMv7-A that don't normally support access to the banked
>>>> registers.
>>>>
>>>> This is considered to be a programming error by the gcc developers
>>>> and will no longer work in gcc-8, where we now get a build error:
>>>>
>>>> /tmp/cc4Qy7GR.s:34: Error: Banked registers are not available with this
>>>> architecture. -- `mrs r3,SP_usr'
>>>> /tmp/cc4Qy7GR.s:41: Error: Banked registers are not available with this
>>>> architecture. -- `mrs r3,ELR_hyp'
>>>> /tmp/cc4Qy7GR.s:55: Error: Banked registers are not available with this
>>>> architecture. -- `mrs r3,SP_svc'
>>>> /tmp/cc4Qy7GR.s:62: Error: Banked registers are not available with this
>>>> architecture. -- `mrs r3,LR_svc'
>>>> /tmp/cc4Qy7GR.s:69: Error: Banked registers are not available with this
>>>> architecture. -- `mrs r3,SPSR_svc'
>>>> /tmp/cc4Qy7GR.s:76: Error: Banked registers are not available with this
>>>> architecture. -- `mrs r3,SP_abt'
>>>>
>>>> Passign the '-march-armv7ve' flag to gcc works, and is ok here, because
>>>> we know the functions won't ever be called on pre-ARMv7VE machines.
>>>> Unfortunately, older compiler versions (4.8 and earlier) do not
>>>> understand
>>>> that flag, so we still need to keep the asm around.
>>>>
>>>> Backporting to stable kernels (4.6+) is needed to allow those to be built
>>>> with future compilers as well.
>>>
>>>
>>> Is "-Wa,arch=armv7-a+virt" (as we appear to do for a couple of files
>>> already) viable as a possibly cleaner alternative, or is GCC itself now
>>> policing the contents of inline asms?
>>
>>
>> In fact, looking at the binutils history, any version capable of assembling
>> this file should understand that (modulo my typo), so hopefully it ought to
>> be feasible to replace these global asms with assembler flags entirely.
> 
> No, this only works for .S files, not .c, since gcc starts the output with
> an explicit .arch setting that overrides the command line. I think this
> was done intentionally to prevent such a hack from working, and have
> more reliable checks on the validity of the assembler instruction in
> inline asm statements (which we try to circumvent here).

Ah, I see, that is unfortunate. Thanks for clarifying.

Robin.

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH 2/2] ARM: xscale: fix gcc-8 build
  2018-02-02 15:07 ` [PATCH 2/2] ARM: xscale: fix gcc-8 build Arnd Bergmann
@ 2018-02-02 16:39   ` Nicolas Pitre
  0 siblings, 0 replies; 10+ messages in thread
From: Nicolas Pitre @ 2018-02-02 16:39 UTC (permalink / raw)
  To: Arnd Bergmann
  Cc: Russell King, Andi Kleen, Richard Earnshaw, Tamar Christina,
	linux-arm-kernel, linux-kernel

On Fri, 2 Feb 2018, Arnd Bergmann wrote:

> We use a hack in xscale-cp0.c to allow building it for ARMv4 while
> also using ARMv5TE and iWMMXt specific inline assembly, by
> adding a top-level asm statement.
> 
> Unfortunately that hack no longer works with gcc-8, since it will
> revert back to the normal architecture. The recommended way of
> handling this is to use __attribute__((target("armv5te"))) on the
> functions that need it, or #pragma GCC target("arch=armv5te").
> Either of those work with gcc-8, but not earlier versions, and
> it seems worse to combine that with the old hack.
> 
> Instead, this adds the .arch statement to each inline assembler
> statement that needs it individually. That is also slightly uglier
> than the previous hack, but it works with all compiler versions
> and documents better why we need the override in the first place.
> 
> Link: https://gcc.gnu.org/bugzilla/show_bug.cgi?id=84129
> Signed-off-by: Arnd Bergmann <arnd@arndb.de>

Acked-by: Nicolas Pitre <nico@linaro.org>


> ---
>  arch/arm/kernel/xscale-cp0.c | 7 ++++---
>  1 file changed, 4 insertions(+), 3 deletions(-)
> 
> diff --git a/arch/arm/kernel/xscale-cp0.c b/arch/arm/kernel/xscale-cp0.c
> index 77a2eef72115..e06a2f6dac4f 100644
> --- a/arch/arm/kernel/xscale-cp0.c
> +++ b/arch/arm/kernel/xscale-cp0.c
> @@ -17,11 +17,10 @@
>  #include <asm/thread_notify.h>
>  #include <asm/cputype.h>
>  
> -asm("	.arch armv5te\n");
> -
>  static inline void dsp_save_state(u32 *state)
>  {
>  	__asm__ __volatile__ (
> +		".arch	armv5te\n\t"
>  		"mrrc	p0, 0, %0, %1, c0\n"
>  		: "=r" (state[0]), "=r" (state[1]));
>  }
> @@ -29,6 +28,7 @@ static inline void dsp_save_state(u32 *state)
>  static inline void dsp_load_state(u32 *state)
>  {
>  	__asm__ __volatile__ (
> +		".arch	armv5te\n\t"
>  		"mcrr	p0, 0, %0, %1, c0\n"
>  		: : "r" (state[0]), "r" (state[1]));
>  }
> @@ -134,7 +134,8 @@ static int __init cpu_has_iwmmxt(void)
>  	 *	tmrrc	%0, %1, wR0
>  	 */
>  	__asm__ __volatile__ (
> -		"mcrr	p0, 0, %2, %3, c0\n"
> +		".arch	armv5te\n\t"
> +		"mcrr	p0, 0, %2, %3, c0\n\t"
>  		"mrrc	p0, 0, %0, %1, c0\n"
>  		: "=r" (lo), "=r" (hi)
>  		: "r" (0), "r" (0x100));
> -- 
> 2.9.0
> 
> 

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH 1/2] ARM: kvm: fix building with gcc-8
  2018-02-02 15:07 [PATCH 1/2] ARM: kvm: fix building with gcc-8 Arnd Bergmann
  2018-02-02 15:07 ` [PATCH 2/2] ARM: xscale: fix gcc-8 build Arnd Bergmann
  2018-02-02 15:55 ` [PATCH 1/2] ARM: kvm: fix building with gcc-8 Robin Murphy
@ 2018-02-04 18:45 ` Christoffer Dall
  2018-02-04 20:57   ` Arnd Bergmann
  2 siblings, 1 reply; 10+ messages in thread
From: Christoffer Dall @ 2018-02-04 18:45 UTC (permalink / raw)
  To: Arnd Bergmann
  Cc: Marc Zyngier, Russell King, Nicolas Pitre, Andi Kleen,
	Richard Earnshaw, Tamar Christina, stable, Julien Thierry,
	linux-arm-kernel, kvmarm, linux-kernel

Hi Arnd,

On Fri, Feb 02, 2018 at 04:07:34PM +0100, Arnd Bergmann wrote:
> In banked-sr.c, we use a top-level '__asm__(".arch_extension virt")'
> statement to allow compilation of a multi-CPU kernel for ARMv6
> and older ARMv7-A that don't normally support access to the banked
> registers.
> 
> This is considered to be a programming error by the gcc developers
> and will no longer work in gcc-8, where we now get a build error:
> 
> /tmp/cc4Qy7GR.s:34: Error: Banked registers are not available with this architecture. -- `mrs r3,SP_usr'
> /tmp/cc4Qy7GR.s:41: Error: Banked registers are not available with this architecture. -- `mrs r3,ELR_hyp'
> /tmp/cc4Qy7GR.s:55: Error: Banked registers are not available with this architecture. -- `mrs r3,SP_svc'
> /tmp/cc4Qy7GR.s:62: Error: Banked registers are not available with this architecture. -- `mrs r3,LR_svc'
> /tmp/cc4Qy7GR.s:69: Error: Banked registers are not available with this architecture. -- `mrs r3,SPSR_svc'
> /tmp/cc4Qy7GR.s:76: Error: Banked registers are not available with this architecture. -- `mrs r3,SP_abt'
> 
> Passign the '-march-armv7ve' flag to gcc works, and is ok here, because
> we know the functions won't ever be called on pre-ARMv7VE machines.
> Unfortunately, older compiler versions (4.8 and earlier) do not understand
> that flag, so we still need to keep the asm around.

Does "not understand" mean "ignores" or do we get an error?

> 
> Backporting to stable kernels (4.6+) is needed to allow those to be built
> with future compilers as well.

This builds on the toolchains I have on my machine, so:

Acked-by: Christoffer Dall <christoffer.dall@linaro.org>

Are you applying this via a tree with other fixes or would you like me
to carry it in the kvmarm tree?

Thanks,
-Christoffer

> 
> Link: https://gcc.gnu.org/bugzilla/show_bug.cgi?id=84129
> Fixes: 33280b4cd1dc ("ARM: KVM: Add banked registers save/restore")
> Cc: stable@vger.kernel.org
> Signed-off-by: Arnd Bergmann <arnd@arndb.de>
> ---
>  arch/arm/kvm/hyp/Makefile    | 5 +++++
>  arch/arm/kvm/hyp/banked-sr.c | 4 ++++
>  2 files changed, 9 insertions(+)
> 
> diff --git a/arch/arm/kvm/hyp/Makefile b/arch/arm/kvm/hyp/Makefile
> index 5638ce0c9524..63d6b404d88e 100644
> --- a/arch/arm/kvm/hyp/Makefile
> +++ b/arch/arm/kvm/hyp/Makefile
> @@ -7,6 +7,8 @@ ccflags-y += -fno-stack-protector -DDISABLE_BRANCH_PROFILING
>  
>  KVM=../../../../virt/kvm
>  
> +CFLAGS_ARMV7VE		   :=$(call cc-option, -march=armv7ve)
> +
>  obj-$(CONFIG_KVM_ARM_HOST) += $(KVM)/arm/hyp/vgic-v2-sr.o
>  obj-$(CONFIG_KVM_ARM_HOST) += $(KVM)/arm/hyp/vgic-v3-sr.o
>  obj-$(CONFIG_KVM_ARM_HOST) += $(KVM)/arm/hyp/timer-sr.o
> @@ -15,7 +17,10 @@ obj-$(CONFIG_KVM_ARM_HOST) += tlb.o
>  obj-$(CONFIG_KVM_ARM_HOST) += cp15-sr.o
>  obj-$(CONFIG_KVM_ARM_HOST) += vfp.o
>  obj-$(CONFIG_KVM_ARM_HOST) += banked-sr.o
> +CFLAGS_banked-sr.o	   += $(CFLAGS_ARMV7VE)
> +
>  obj-$(CONFIG_KVM_ARM_HOST) += entry.o
>  obj-$(CONFIG_KVM_ARM_HOST) += hyp-entry.o
>  obj-$(CONFIG_KVM_ARM_HOST) += switch.o
> +CFLAGS_switch.o		   += $(CFLAGS_ARMV7VE)
>  obj-$(CONFIG_KVM_ARM_HOST) += s2-setup.o
> diff --git a/arch/arm/kvm/hyp/banked-sr.c b/arch/arm/kvm/hyp/banked-sr.c
> index 111bda8cdebd..be4b8b0a40ad 100644
> --- a/arch/arm/kvm/hyp/banked-sr.c
> +++ b/arch/arm/kvm/hyp/banked-sr.c
> @@ -20,6 +20,10 @@
>  
>  #include <asm/kvm_hyp.h>
>  
> +/*
> + * gcc before 4.9 doesn't understand -march=armv7ve, so we have to
> + * trick the assembler.
> + */
>  __asm__(".arch_extension     virt");
>  
>  void __hyp_text __banked_save_state(struct kvm_cpu_context *ctxt)
> -- 
> 2.9.0
> 

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH 1/2] ARM: kvm: fix building with gcc-8
  2018-02-04 18:45 ` Christoffer Dall
@ 2018-02-04 20:57   ` Arnd Bergmann
  2018-02-05  8:33     ` Christoffer Dall
  0 siblings, 1 reply; 10+ messages in thread
From: Arnd Bergmann @ 2018-02-04 20:57 UTC (permalink / raw)
  To: Christoffer Dall
  Cc: Marc Zyngier, Russell King, Nicolas Pitre, Andi Kleen,
	Richard Earnshaw, Tamar Christina, # 3.4.x, Julien Thierry,
	Linux ARM, kvmarm, Linux Kernel Mailing List

On Sun, Feb 4, 2018 at 7:45 PM, Christoffer Dall
<christoffer.dall@linaro.org> wrote:
> Hi Arnd,
>
> On Fri, Feb 02, 2018 at 04:07:34PM +0100, Arnd Bergmann wrote:
>> In banked-sr.c, we use a top-level '__asm__(".arch_extension virt")'
>> statement to allow compilation of a multi-CPU kernel for ARMv6
>> and older ARMv7-A that don't normally support access to the banked
>> registers.
>>
>> This is considered to be a programming error by the gcc developers
>> and will no longer work in gcc-8, where we now get a build error:
>>
>> /tmp/cc4Qy7GR.s:34: Error: Banked registers are not available with this architecture. -- `mrs r3,SP_usr'
>> /tmp/cc4Qy7GR.s:41: Error: Banked registers are not available with this architecture. -- `mrs r3,ELR_hyp'
>> /tmp/cc4Qy7GR.s:55: Error: Banked registers are not available with this architecture. -- `mrs r3,SP_svc'
>> /tmp/cc4Qy7GR.s:62: Error: Banked registers are not available with this architecture. -- `mrs r3,LR_svc'
>> /tmp/cc4Qy7GR.s:69: Error: Banked registers are not available with this architecture. -- `mrs r3,SPSR_svc'
>> /tmp/cc4Qy7GR.s:76: Error: Banked registers are not available with this architecture. -- `mrs r3,SP_abt'
>>
>> Passign the '-march-armv7ve' flag to gcc works, and is ok here, because
>> we know the functions won't ever be called on pre-ARMv7VE machines.
>> Unfortunately, older compiler versions (4.8 and earlier) do not understand
>> that flag, so we still need to keep the asm around.
>
> Does "not understand" mean "ignores" or do we get an error?

We get an error, which is why I used the $(call cc-option) Makefile
helper to check if the compiler supports it.

>> Backporting to stable kernels (4.6+) is needed to allow those to be built
>> with future compilers as well.
>
> This builds on the toolchains I have on my machine, so:
>
> Acked-by: Christoffer Dall <christoffer.dall@linaro.org>
>
> Are you applying this via a tree with other fixes or would you like me
> to carry it in the kvmarm tree?

Please pick it up in your tree.

Thanks,

       Arnd

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH 1/2] ARM: kvm: fix building with gcc-8
  2018-02-04 20:57   ` Arnd Bergmann
@ 2018-02-05  8:33     ` Christoffer Dall
  0 siblings, 0 replies; 10+ messages in thread
From: Christoffer Dall @ 2018-02-05  8:33 UTC (permalink / raw)
  To: Arnd Bergmann
  Cc: Marc Zyngier, Russell King, Nicolas Pitre, Andi Kleen,
	Richard Earnshaw, Tamar Christina, # 3.4.x, Julien Thierry,
	Linux ARM, kvmarm, Linux Kernel Mailing List

On Sun, Feb 04, 2018 at 09:57:49PM +0100, Arnd Bergmann wrote:
> On Sun, Feb 4, 2018 at 7:45 PM, Christoffer Dall
> <christoffer.dall@linaro.org> wrote:
> > Hi Arnd,
> >
> > On Fri, Feb 02, 2018 at 04:07:34PM +0100, Arnd Bergmann wrote:
> >> In banked-sr.c, we use a top-level '__asm__(".arch_extension virt")'
> >> statement to allow compilation of a multi-CPU kernel for ARMv6
> >> and older ARMv7-A that don't normally support access to the banked
> >> registers.
> >>
> >> This is considered to be a programming error by the gcc developers
> >> and will no longer work in gcc-8, where we now get a build error:
> >>
> >> /tmp/cc4Qy7GR.s:34: Error: Banked registers are not available with this architecture. -- `mrs r3,SP_usr'
> >> /tmp/cc4Qy7GR.s:41: Error: Banked registers are not available with this architecture. -- `mrs r3,ELR_hyp'
> >> /tmp/cc4Qy7GR.s:55: Error: Banked registers are not available with this architecture. -- `mrs r3,SP_svc'
> >> /tmp/cc4Qy7GR.s:62: Error: Banked registers are not available with this architecture. -- `mrs r3,LR_svc'
> >> /tmp/cc4Qy7GR.s:69: Error: Banked registers are not available with this architecture. -- `mrs r3,SPSR_svc'
> >> /tmp/cc4Qy7GR.s:76: Error: Banked registers are not available with this architecture. -- `mrs r3,SP_abt'
> >>
> >> Passign the '-march-armv7ve' flag to gcc works, and is ok here, because
> >> we know the functions won't ever be called on pre-ARMv7VE machines.
> >> Unfortunately, older compiler versions (4.8 and earlier) do not understand
> >> that flag, so we still need to keep the asm around.
> >
> > Does "not understand" mean "ignores" or do we get an error?
> 
> We get an error, which is why I used the $(call cc-option) Makefile
> helper to check if the compiler supports it.
> 

Right.

> >> Backporting to stable kernels (4.6+) is needed to allow those to be built
> >> with future compilers as well.
> >
> > This builds on the toolchains I have on my machine, so:
> >
> > Acked-by: Christoffer Dall <christoffer.dall@linaro.org>
> >
> > Are you applying this via a tree with other fixes or would you like me
> > to carry it in the kvmarm tree?
> 
> Please pick it up in your tree.
> 
Will do.

Thanks,
-Christoffer

^ permalink raw reply	[flat|nested] 10+ messages in thread

end of thread, other threads:[~2018-02-05  8:33 UTC | newest]

Thread overview: 10+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-02-02 15:07 [PATCH 1/2] ARM: kvm: fix building with gcc-8 Arnd Bergmann
2018-02-02 15:07 ` [PATCH 2/2] ARM: xscale: fix gcc-8 build Arnd Bergmann
2018-02-02 16:39   ` Nicolas Pitre
2018-02-02 15:55 ` [PATCH 1/2] ARM: kvm: fix building with gcc-8 Robin Murphy
2018-02-02 16:23   ` Robin Murphy
2018-02-02 16:29     ` Arnd Bergmann
2018-02-02 16:30       ` Robin Murphy
2018-02-04 18:45 ` Christoffer Dall
2018-02-04 20:57   ` Arnd Bergmann
2018-02-05  8:33     ` Christoffer Dall

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).