kvmarm.lists.cs.columbia.edu archive mirror
 help / color / mirror / Atom feed
* [PATCH] KVM: arm64: nv: Work around lack of pauth support in old toolchains
@ 2024-04-22 22:48 Marc Zyngier
  2024-04-23  8:24 ` Arnd Bergmann
  2024-04-23  8:37 ` Mark Rutland
  0 siblings, 2 replies; 9+ messages in thread
From: Marc Zyngier @ 2024-04-22 22:48 UTC (permalink / raw)
  To: kvmarm, linux-arm-kernel
  Cc: James Morse, Suzuki K Poulose, Oliver Upton, Zenghui Yu,
	Arnd Bergmann, Mark Rutland, Joey Gouly, Will Deacon,
	Naresh Kamboju, Linaro Kernel Functional Testing

We still support GCC 8.x, and it appears that this toolchain
does not understand "pauth" as a valid architectural extension.
After all, it's only been 8 years since ARMv8.3 was released...

This results in the NV ERETAx code breaking the build, as it relies
on this extention to make use of the PACGA instruction.

Work around it by hand-assembling the instruction using a mind-bending
trick lifted from an old patch by Will. Magic.

Fixes: e09faab353a6 ("KVM: arm64: nv: Add emulation for ERETAx instructions")
Reported-by: Linaro Kernel Functional Testing <lkft@linaro.org>
Signed-off-by: Marc Zyngier <maz@kernel.org>
---
 arch/arm64/kvm/pauth.c | 19 +++++++++++++++++--
 1 file changed, 17 insertions(+), 2 deletions(-)

diff --git a/arch/arm64/kvm/pauth.c b/arch/arm64/kvm/pauth.c
index a3a5c404375b..8baf2a2cbdd3 100644
--- a/arch/arm64/kvm/pauth.c
+++ b/arch/arm64/kvm/pauth.c
@@ -17,6 +17,22 @@
 #include <asm/kvm_emulate.h>
 #include <asm/pointer_auth.h>
 
+/*
+ * "// This is some of my finest work" (Will Deacon, 2019-02-12)
+ *
+ * The jury is still out on that one.
+ */
+#define REG(r)	"(0%x[" #r "] - ((0%x[" #r "] >> 4) * 6))"
+
+/* PACGA Xd, Xn, Xm */
+#define PACGA(d,n,m)						\
+	asm volatile(".inst 0x9AC03000   |"			\
+		     "(" REG(Rd) "<< 0)  |"			\
+		     "(" REG(Rn) "<< 5)  |"			\
+		     "(" REG(Rm) "<< 16)\n"			\
+		     : [Rd] "=r" ((d))				\
+		     : [Rn] "r" ((n)), [Rm] "r" ((m)))
+
 static u64 compute_pac(struct kvm_vcpu *vcpu, u64 ptr,
 		       struct ptrauth_key ikey)
 {
@@ -36,8 +52,7 @@ static u64 compute_pac(struct kvm_vcpu *vcpu, u64 ptr,
 	__ptrauth_key_install_nosync(APGA, ikey);
 	isb();
 
-	asm volatile(ARM64_ASM_PREAMBLE ".arch_extension pauth\n"
-		     "pacga %0, %1, %2" : "=r" (pac) : "r" (ptr), "r" (mod));
+	PACGA(pac, ptr, mod);
 	isb();
 
 	__ptrauth_key_install_nosync(APGA, gkey);
-- 
2.39.2


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

* Re: [PATCH] KVM: arm64: nv: Work around lack of pauth support in old toolchains
  2024-04-22 22:48 [PATCH] KVM: arm64: nv: Work around lack of pauth support in old toolchains Marc Zyngier
@ 2024-04-23  8:24 ` Arnd Bergmann
  2024-04-23 12:00   ` Aiqun Yu (Maria)
  2024-04-23  8:37 ` Mark Rutland
  1 sibling, 1 reply; 9+ messages in thread
From: Arnd Bergmann @ 2024-04-23  8:24 UTC (permalink / raw)
  To: Marc Zyngier, kvmarm, linux-arm-kernel
  Cc: James Morse, Suzuki K Poulose, Oliver Upton, Zenghui Yu,
	Mark Rutland, Joey Gouly, Will Deacon, Naresh Kamboju,
	Linaro Kernel Functional Testing

On Tue, Apr 23, 2024, at 00:48, Marc Zyngier wrote:
> We still support GCC 8.x, and it appears that this toolchain
> does not understand "pauth" as a valid architectural extension.
> After all, it's only been 8 years since ARMv8.3 was released...

Just to clarify: I'm fairly sure that all supported toolchains
support ARMv8.3 and PACGA, the problem with ".arch_extension pauth\n"
seems to be that it was retroactively made an optional
feature for earlier architecture versions a few years after
ARMv8.3, so most binutils versions we support understand
pacga as an armv8.3 feature but reject the pauth name for the
extension.

> This results in the NV ERETAx code breaking the build, as it relies
> on this extention to make use of the PACGA instruction.
>
> Work around it by hand-assembling the instruction using a mind-bending
> trick lifted from an old patch by Will. Magic.
>
> Fixes: e09faab353a6 ("KVM: arm64: nv: Add emulation for ERETAx instructions")
> Reported-by: Linaro Kernel Functional Testing <lkft@linaro.org>
> Signed-off-by: Marc Zyngier <maz@kernel.org>

I usually try to enclose these in a version check like
"#if CONFIG_AS_VERSION >= 22800" so we can be sure we clean them up
whenever the minimum version changes, but I assume you'll remember
this one.

Acked-by: Arnd Bergmann <arnd@arndb.de>

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

* Re: [PATCH] KVM: arm64: nv: Work around lack of pauth support in old toolchains
  2024-04-22 22:48 [PATCH] KVM: arm64: nv: Work around lack of pauth support in old toolchains Marc Zyngier
  2024-04-23  8:24 ` Arnd Bergmann
@ 2024-04-23  8:37 ` Mark Rutland
  2024-04-23 11:33   ` Marc Zyngier
  1 sibling, 1 reply; 9+ messages in thread
From: Mark Rutland @ 2024-04-23  8:37 UTC (permalink / raw)
  To: Marc Zyngier
  Cc: kvmarm, linux-arm-kernel, James Morse, Suzuki K Poulose,
	Oliver Upton, Zenghui Yu, Arnd Bergmann, Joey Gouly, Will Deacon,
	Naresh Kamboju, Linaro Kernel Functional Testing

On Mon, Apr 22, 2024 at 11:48:49PM +0100, Marc Zyngier wrote:
> We still support GCC 8.x, and it appears that this toolchain
> does not understand "pauth" as a valid architectural extension.
> After all, it's only been 8 years since ARMv8.3 was released...
> 
> This results in the NV ERETAx code breaking the build, as it relies
> on this extention to make use of the PACGA instruction.
> 
> Work around it by hand-assembling the instruction using a mind-bending
> trick lifted from an old patch by Will. Magic.
> 
> Fixes: e09faab353a6 ("KVM: arm64: nv: Add emulation for ERETAx instructions")
> Reported-by: Linaro Kernel Functional Testing <lkft@linaro.org>
> Signed-off-by: Marc Zyngier <maz@kernel.org>
> ---
>  arch/arm64/kvm/pauth.c | 19 +++++++++++++++++--
>  1 file changed, 17 insertions(+), 2 deletions(-)
> 
> diff --git a/arch/arm64/kvm/pauth.c b/arch/arm64/kvm/pauth.c
> index a3a5c404375b..8baf2a2cbdd3 100644
> --- a/arch/arm64/kvm/pauth.c
> +++ b/arch/arm64/kvm/pauth.c
> @@ -17,6 +17,22 @@
>  #include <asm/kvm_emulate.h>
>  #include <asm/pointer_auth.h>
>  
> +/*
> + * "// This is some of my finest work" (Will Deacon, 2019-02-12)
> + *
> + * The jury is still out on that one.
> + */
> +#define REG(r)	"(0%x[" #r "] - ((0%x[" #r "] >> 4) * 6))"
> +
> +/* PACGA Xd, Xn, Xm */
> +#define PACGA(d,n,m)						\
> +	asm volatile(".inst 0x9AC03000   |"			\
> +		     "(" REG(Rd) "<< 0)  |"			\
> +		     "(" REG(Rn) "<< 5)  |"			\
> +		     "(" REG(Rm) "<< 16)\n"			\
> +		     : [Rd] "=r" ((d))				\
> +		     : [Rn] "r" ((n)), [Rm] "r" ((m)))

Can you please use <asm/gpr-num.h> rather than open-coding this new REG()
helper? That way this'll be consistent with the way we assemble MRS/MSR in
<asm/sysreg.h>, and it avoids the few seconds of confusion trying to figure out
the maths in the REG() helper (neat trick though!).

Fixlet for that below; I've build tested this and the resulting object file is
the same before and after (diff shows that only the name of the file changed):

| [mark@lakrids:~/src/linux]% usekorg 13.2.0 aarch64-linux-objdump -d pauth-maz.o | grep pacga
|  204:   9ac03335        pacga   x21, x25, x0
| [mark@lakrids:~/src/linux]% usekorg 13.2.0 aarch64-linux-objdump -d pauth-rutland.o | grep pacga
|  204:   9ac03335        pacga   x21, x25, x0
| [mark@lakrids:~/src/linux]% diff <(usekorg 13.2.0 aarch64-linux-objdump -d pauth-maz.o) <(usekorg 13.2.0 aarch64-linux-objdump -d pauth-rutland.o)
| 2c2
| < pauth-maz.o:     file format elf64-littleaarch64
| ---
| > pauth-rutland.o:     file format elf64-littleaarch64

If you'd prefer I can send this as a patch.

Otherwise, this looks good to me; thanks for the fix!

Mark.

---->8----
diff --git a/arch/arm64/kvm/pauth.c b/arch/arm64/kvm/pauth.c
index 8baf2a2cbdd32..e518baf570c86 100644
--- a/arch/arm64/kvm/pauth.c
+++ b/arch/arm64/kvm/pauth.c
@@ -14,24 +14,20 @@
 
 #include <linux/kvm_host.h>
 
+#include <asm/gpr-num.h>
 #include <asm/kvm_emulate.h>
 #include <asm/pointer_auth.h>
 
-/*
- * "// This is some of my finest work" (Will Deacon, 2019-02-12)
- *
- * The jury is still out on that one.
- */
-#define REG(r)	"(0%x[" #r "] - ((0%x[" #r "] >> 4) * 6))"
-
 /* PACGA Xd, Xn, Xm */
-#define PACGA(d,n,m)						\
-	asm volatile(".inst 0x9AC03000   |"			\
-		     "(" REG(Rd) "<< 0)  |"			\
-		     "(" REG(Rn) "<< 5)  |"			\
-		     "(" REG(Rm) "<< 16)\n"			\
-		     : [Rd] "=r" ((d))				\
-		     : [Rn] "r" ((n)), [Rm] "r" ((m)))
+#define PACGA(d,n,m)				\
+	asm volatile(				\
+	__DEFINE_ASM_GPR_NUMS			\
+	"	.inst 0x9AC03000          |"	\
+	"	(.L__gpr_num_%[Rd] << 0)  |"	\
+	"	(.L__gpr_num_%[Rn] << 5)  |"	\
+	"	(.L__gpr_num_%[Rm] << 16)\n"	\
+	: [Rd] "=r" ((d))			\
+	: [Rn] "r" ((n)), [Rm] "r" ((m)))
 
 static u64 compute_pac(struct kvm_vcpu *vcpu, u64 ptr,
 		       struct ptrauth_key ikey)

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

* Re: [PATCH] KVM: arm64: nv: Work around lack of pauth support in old toolchains
  2024-04-23  8:37 ` Mark Rutland
@ 2024-04-23 11:33   ` Marc Zyngier
  0 siblings, 0 replies; 9+ messages in thread
From: Marc Zyngier @ 2024-04-23 11:33 UTC (permalink / raw)
  To: Mark Rutland
  Cc: kvmarm, linux-arm-kernel, James Morse, Suzuki K Poulose,
	Oliver Upton, Zenghui Yu, Arnd Bergmann, Joey Gouly, Will Deacon,
	Naresh Kamboju, Linaro Kernel Functional Testing

On Tue, 23 Apr 2024 09:37:04 +0100,
Mark Rutland <mark.rutland@arm.com> wrote:
> 
> On Mon, Apr 22, 2024 at 11:48:49PM +0100, Marc Zyngier wrote:
> > We still support GCC 8.x, and it appears that this toolchain
> > does not understand "pauth" as a valid architectural extension.
> > After all, it's only been 8 years since ARMv8.3 was released...
> > 
> > This results in the NV ERETAx code breaking the build, as it relies
> > on this extention to make use of the PACGA instruction.
> > 
> > Work around it by hand-assembling the instruction using a mind-bending
> > trick lifted from an old patch by Will. Magic.
> > 
> > Fixes: e09faab353a6 ("KVM: arm64: nv: Add emulation for ERETAx instructions")
> > Reported-by: Linaro Kernel Functional Testing <lkft@linaro.org>
> > Signed-off-by: Marc Zyngier <maz@kernel.org>
> > ---
> >  arch/arm64/kvm/pauth.c | 19 +++++++++++++++++--
> >  1 file changed, 17 insertions(+), 2 deletions(-)
> > 
> > diff --git a/arch/arm64/kvm/pauth.c b/arch/arm64/kvm/pauth.c
> > index a3a5c404375b..8baf2a2cbdd3 100644
> > --- a/arch/arm64/kvm/pauth.c
> > +++ b/arch/arm64/kvm/pauth.c
> > @@ -17,6 +17,22 @@
> >  #include <asm/kvm_emulate.h>
> >  #include <asm/pointer_auth.h>
> >  
> > +/*
> > + * "// This is some of my finest work" (Will Deacon, 2019-02-12)
> > + *
> > + * The jury is still out on that one.
> > + */
> > +#define REG(r)	"(0%x[" #r "] - ((0%x[" #r "] >> 4) * 6))"
> > +
> > +/* PACGA Xd, Xn, Xm */
> > +#define PACGA(d,n,m)						\
> > +	asm volatile(".inst 0x9AC03000   |"			\
> > +		     "(" REG(Rd) "<< 0)  |"			\
> > +		     "(" REG(Rn) "<< 5)  |"			\
> > +		     "(" REG(Rm) "<< 16)\n"			\
> > +		     : [Rd] "=r" ((d))				\
> > +		     : [Rn] "r" ((n)), [Rm] "r" ((m)))
> 
> Can you please use <asm/gpr-num.h> rather than open-coding this new REG()
> helper? That way this'll be consistent with the way we assemble MRS/MSR in
> <asm/sysreg.h>, and it avoids the few seconds of confusion trying to figure out
> the maths in the REG() helper (neat trick though!).

Ah, I had completely forgot about this guy. Good call. I'll fold that
patchlet in and re-push the result.

Thanks,

	M.

-- 
Without deviation from the norm, progress is not possible.

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

* Re: [PATCH] KVM: arm64: nv: Work around lack of pauth support in old toolchains
  2024-04-23  8:24 ` Arnd Bergmann
@ 2024-04-23 12:00   ` Aiqun Yu (Maria)
  2024-04-23 12:06     ` Marc Zyngier
  0 siblings, 1 reply; 9+ messages in thread
From: Aiqun Yu (Maria) @ 2024-04-23 12:00 UTC (permalink / raw)
  To: Arnd Bergmann, Marc Zyngier, kvmarm, linux-arm-kernel
  Cc: James Morse, Suzuki K Poulose, Oliver Upton, Zenghui Yu,
	Mark Rutland, Joey Gouly, Will Deacon, Naresh Kamboju,
	Linaro Kernel Functional Testing



On 4/23/2024 4:24 PM, Arnd Bergmann wrote:
> On Tue, Apr 23, 2024, at 00:48, Marc Zyngier wrote:
>> We still support GCC 8.x, and it appears that this toolchain
>> does not understand "pauth" as a valid architectural extension.
>> After all, it's only been 8 years since ARMv8.3 was released...
> 
> Just to clarify: I'm fairly sure that all supported toolchains
> support ARMv8.3 and PACGA, the problem with ".arch_extension pauth\n"
> seems to be that it was retroactively made an optional
> feature for earlier architecture versions a few years after
> ARMv8.3, so most binutils versions we support understand
> pacga as an armv8.3 feature but reject the pauth name for the
> extension.
Kind of agree with Arnd here.
Shall the fix just remove the ".arch_extension pauth"?

I've tried gcc 7 failed with the pauth name for the extension.
After I remove the ".arch_extension pauth" and use "pacga" instruction
directly pass the gcc 7 compilation.

By the way, have a glance for gcc7/8/9/10 manual, from the gcc 10 manual
[1], seems gcc 10 was the first introduced the pauth as the extension
name support.
https://gcc.gnu.org/onlinedocs/gcc-10.5.0/gcc/AArch64-Options.html#g_t-march-and--mcpu-Feature-Modifiers
> 
>> This results in the NV ERETAx code breaking the build, as it relies
>> on this extention to make use of the PACGA instruction.
>>
>> Work around it by hand-assembling the instruction using a mind-bending
>> trick lifted from an old patch by Will. Magic.
>>
>> Fixes: e09faab353a6 ("KVM: arm64: nv: Add emulation for ERETAx instructions")
>> Reported-by: Linaro Kernel Functional Testing <lkft@linaro.org>
>> Signed-off-by: Marc Zyngier <maz@kernel.org>
> 
> I usually try to enclose these in a version check like
> "#if CONFIG_AS_VERSION >= 22800" so we can be sure we clean them up
> whenever the minimum version changes, but I assume you'll remember
> this one.
> 
> Acked-by: Arnd Bergmann <arnd@arndb.de>
> 
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

-- 
Thx and BRs,
Aiqun(Maria) Yu

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

* Re: [PATCH] KVM: arm64: nv: Work around lack of pauth support in old toolchains
  2024-04-23 12:00   ` Aiqun Yu (Maria)
@ 2024-04-23 12:06     ` Marc Zyngier
  2024-04-23 12:37       ` Arnd Bergmann
  0 siblings, 1 reply; 9+ messages in thread
From: Marc Zyngier @ 2024-04-23 12:06 UTC (permalink / raw)
  To: Aiqun Yu (Maria)
  Cc: Arnd Bergmann, kvmarm, linux-arm-kernel, James Morse,
	Suzuki K Poulose, Oliver Upton, Zenghui Yu, Mark Rutland,
	Joey Gouly, Will Deacon, Naresh Kamboju,
	Linaro Kernel Functional Testing

On Tue, 23 Apr 2024 13:00:55 +0100,
"Aiqun Yu (Maria)" <quic_aiquny@quicinc.com> wrote:
> 
> 
> 
> On 4/23/2024 4:24 PM, Arnd Bergmann wrote:
> > On Tue, Apr 23, 2024, at 00:48, Marc Zyngier wrote:
> >> We still support GCC 8.x, and it appears that this toolchain
> >> does not understand "pauth" as a valid architectural extension.
> >> After all, it's only been 8 years since ARMv8.3 was released...
> > 
> > Just to clarify: I'm fairly sure that all supported toolchains
> > support ARMv8.3 and PACGA, the problem with ".arch_extension pauth\n"
> > seems to be that it was retroactively made an optional
> > feature for earlier architecture versions a few years after
> > ARMv8.3, so most binutils versions we support understand
> > pacga as an armv8.3 feature but reject the pauth name for the
> > extension.
> Kind of agree with Arnd here.
> Shall the fix just remove the ".arch_extension pauth"?
> 
> I've tried gcc 7 failed with the pauth name for the extension.
> After I remove the ".arch_extension pauth" and use "pacga" instruction
> directly pass the gcc 7 compilation.

And breaks with LLVM:

  CC      arch/arm64/kvm/pauth.o
arch/arm64/kvm/pauth.c:40:9: error: instruction requires: pauth
                     "pacga %0, %1, %2" : "=r" (pac) : "r" (ptr), "r" (mod));
                      ^
<inline asm>:2:1: note: instantiated into assembly here
pacga x19, x1, x9
^
1 error generated.


	M.

-- 
Without deviation from the norm, progress is not possible.

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

* Re: [PATCH] KVM: arm64: nv: Work around lack of pauth support in old toolchains
  2024-04-23 12:06     ` Marc Zyngier
@ 2024-04-23 12:37       ` Arnd Bergmann
  2024-04-23 16:15         ` Marc Zyngier
  0 siblings, 1 reply; 9+ messages in thread
From: Arnd Bergmann @ 2024-04-23 12:37 UTC (permalink / raw)
  To: Marc Zyngier, Maria Yu
  Cc: kvmarm, linux-arm-kernel, James Morse, Suzuki K Poulose,
	Oliver Upton, Zenghui Yu, Mark Rutland, Joey Gouly, Will Deacon,
	Naresh Kamboju, Linaro Kernel Functional Testing

On Tue, Apr 23, 2024, at 14:06, Marc Zyngier wrote:
> On Tue, 23 Apr 2024 13:00:55 +0100,
> "Aiqun Yu (Maria)" <quic_aiquny@quicinc.com> wrote:
>> On 4/23/2024 4:24 PM, Arnd Bergmann wrote:
>> > On Tue, Apr 23, 2024, at 00:48, Marc Zyngier wrote:
>> >> We still support GCC 8.x, and it appears that this toolchain
>> >> does not understand "pauth" as a valid architectural extension.
>> >> After all, it's only been 8 years since ARMv8.3 was released...
>> > 
>> > Just to clarify: I'm fairly sure that all supported toolchains
>> > support ARMv8.3 and PACGA, the problem with ".arch_extension pauth\n"
>> > seems to be that it was retroactively made an optional
>> > feature for earlier architecture versions a few years after
>> > ARMv8.3, so most binutils versions we support understand
>> > pacga as an armv8.3 feature but reject the pauth name for the
>> > extension.
>> Kind of agree with Arnd here.
>> Shall the fix just remove the ".arch_extension pauth"?
>> 
>> I've tried gcc 7 failed with the pauth name for the extension.
>> After I remove the ".arch_extension pauth" and use "pacga" instruction
>> directly pass the gcc 7 compilation.

It really depends on the binutils version, not gcc of course.

> And breaks with LLVM:
>
>   CC      arch/arm64/kvm/pauth.o
> arch/arm64/kvm/pauth.c:40:9: error: instruction requires: pauth
>                      "pacga %0, %1, %2" : "=r" (pac) : "r" (ptr), "r" (mod));
>                       ^
> <inline asm>:2:1: note: instantiated into assembly here
> pacga x19, x1, x9
> ^

It works when building with LLVM_IAS=0, which we obviously don't
want to mandate here. The variant below works for both clang+ias
(including all still supported versions) and gcc+binutils, but at
that point it gets obscure enough that your .inst version is easier
to understand.

    Arnd

--- a/arch/arm64/kvm/pauth.c
+++ b/arch/arm64/kvm/pauth.c
@@ -36,7 +36,12 @@ static u64 compute_pac(struct kvm_vcpu *vcpu, u64 ptr,
        __ptrauth_key_install_nosync(APGA, ikey);
        isb();
 
-       asm volatile(ARM64_ASM_PREAMBLE ".arch_extension pauth\n"
+       asm volatile(ARM64_ASM_PREAMBLE
+#ifdef CONFIG_AS_IS_LLVM
+                    ".arch_extension pauth\n"
+#else
+                    ".arch armv8.3-a\n"
+#endif
                     "pacga %0, %1, %2" : "=r" (pac) : "r" (ptr), "r" (mod));
        isb();
 

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

* Re: [PATCH] KVM: arm64: nv: Work around lack of pauth support in old toolchains
  2024-04-23 12:37       ` Arnd Bergmann
@ 2024-04-23 16:15         ` Marc Zyngier
  2024-04-24  1:54           ` Aiqun Yu (Maria)
  0 siblings, 1 reply; 9+ messages in thread
From: Marc Zyngier @ 2024-04-23 16:15 UTC (permalink / raw)
  To: Arnd Bergmann
  Cc: Maria Yu, kvmarm, linux-arm-kernel, James Morse,
	Suzuki K Poulose, Oliver Upton, Zenghui Yu, Mark Rutland,
	Joey Gouly, Will Deacon, Naresh Kamboju,
	Linaro Kernel Functional Testing

On Tue, 23 Apr 2024 13:37:09 +0100,
"Arnd Bergmann" <arnd@arndb.de> wrote:
> 
> On Tue, Apr 23, 2024, at 14:06, Marc Zyngier wrote:
> > On Tue, 23 Apr 2024 13:00:55 +0100,
> > "Aiqun Yu (Maria)" <quic_aiquny@quicinc.com> wrote:
> >> On 4/23/2024 4:24 PM, Arnd Bergmann wrote:
> >> > On Tue, Apr 23, 2024, at 00:48, Marc Zyngier wrote:
> >> >> We still support GCC 8.x, and it appears that this toolchain
> >> >> does not understand "pauth" as a valid architectural extension.
> >> >> After all, it's only been 8 years since ARMv8.3 was released...
> >> > 
> >> > Just to clarify: I'm fairly sure that all supported toolchains
> >> > support ARMv8.3 and PACGA, the problem with ".arch_extension pauth\n"
> >> > seems to be that it was retroactively made an optional
> >> > feature for earlier architecture versions a few years after
> >> > ARMv8.3, so most binutils versions we support understand
> >> > pacga as an armv8.3 feature but reject the pauth name for the
> >> > extension.
> >> Kind of agree with Arnd here.
> >> Shall the fix just remove the ".arch_extension pauth"?
> >> 
> >> I've tried gcc 7 failed with the pauth name for the extension.
> >> After I remove the ".arch_extension pauth" and use "pacga" instruction
> >> directly pass the gcc 7 compilation.
> 
> It really depends on the binutils version, not gcc of course.

Right. I'll amend the commit message to reflect that.

> 
> > And breaks with LLVM:
> >
> >   CC      arch/arm64/kvm/pauth.o
> > arch/arm64/kvm/pauth.c:40:9: error: instruction requires: pauth
> >                      "pacga %0, %1, %2" : "=r" (pac) : "r" (ptr), "r" (mod));
> >                       ^
> > <inline asm>:2:1: note: instantiated into assembly here
> > pacga x19, x1, x9
> > ^
> 
> It works when building with LLVM_IAS=0, which we obviously don't
> want to mandate here. The variant below works for both clang+ias
> (including all still supported versions) and gcc+binutils, but at
> that point it gets obscure enough that your .inst version is easier
> to understand.

Exactly. Either we have a good way to abstract this behind the scenes
(which I don't see right now), or we just assume control of the
instruction generation, which is what my patch does.

In general, I question the value of the ".arch_extension" requirement
for something like Linux, where we already have a pretty fine grained
control of what we want to see being output by the compiler. but that
ship has sailed long ago.

Thanks,

	M.

> 
>     arnd
> 
> --- a/arch/arm64/kvm/pauth.c
> +++ b/arch/arm64/kvm/pauth.c
> @@ -36,7 +36,12 @@ static u64 compute_pac(struct kvm_vcpu *vcpu, u64 ptr,
>         __ptrauth_key_install_nosync(APGA, ikey);
>         isb();
>  
> -       asm volatile(ARM64_ASM_PREAMBLE ".arch_extension pauth\n"
> +       asm volatile(ARM64_ASM_PREAMBLE
> +#ifdef CONFIG_AS_IS_LLVM
> +                    ".arch_extension pauth\n"
> +#else
> +                    ".arch armv8.3-a\n"
> +#endif
>                      "pacga %0, %1, %2" : "=r" (pac) : "r" (ptr), "r" (mod));
>         isb();
>  
> 

-- 
Without deviation from the norm, progress is not possible.

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

* Re: [PATCH] KVM: arm64: nv: Work around lack of pauth support in old toolchains
  2024-04-23 16:15         ` Marc Zyngier
@ 2024-04-24  1:54           ` Aiqun Yu (Maria)
  0 siblings, 0 replies; 9+ messages in thread
From: Aiqun Yu (Maria) @ 2024-04-24  1:54 UTC (permalink / raw)
  To: Marc Zyngier, Arnd Bergmann
  Cc: kvmarm, linux-arm-kernel, James Morse, Suzuki K Poulose,
	Oliver Upton, Zenghui Yu, Mark Rutland, Joey Gouly, Will Deacon,
	Naresh Kamboju, Linaro Kernel Functional Testing



On 4/24/2024 12:15 AM, Marc Zyngier wrote:
> On Tue, 23 Apr 2024 13:37:09 +0100,
> "Arnd Bergmann" <arnd@arndb.de> wrote:
>>
>> On Tue, Apr 23, 2024, at 14:06, Marc Zyngier wrote:
>>> On Tue, 23 Apr 2024 13:00:55 +0100,
>>> "Aiqun Yu (Maria)" <quic_aiquny@quicinc.com> wrote:
>>>> On 4/23/2024 4:24 PM, Arnd Bergmann wrote:
>>>>> On Tue, Apr 23, 2024, at 00:48, Marc Zyngier wrote:
>>>>>> We still support GCC 8.x, and it appears that this toolchain
>>>>>> does not understand "pauth" as a valid architectural extension.
>>>>>> After all, it's only been 8 years since ARMv8.3 was released...
>>>>>
>>>>> Just to clarify: I'm fairly sure that all supported toolchains
>>>>> support ARMv8.3 and PACGA, the problem with ".arch_extension pauth\n"
>>>>> seems to be that it was retroactively made an optional
>>>>> feature for earlier architecture versions a few years after
>>>>> ARMv8.3, so most binutils versions we support understand
>>>>> pacga as an armv8.3 feature but reject the pauth name for the
>>>>> extension.
>>>> Kind of agree with Arnd here.
>>>> Shall the fix just remove the ".arch_extension pauth"?
>>>>
>>>> I've tried gcc 7 failed with the pauth name for the extension.
>>>> After I remove the ".arch_extension pauth" and use "pacga" instruction
>>>> directly pass the gcc 7 compilation.
>>
>> It really depends on the binutils version, not gcc of course.
> 
> Right. I'll amend the commit message to reflect that.
> 
>>
>>> And breaks with LLVM:
>>>
>>>   CC      arch/arm64/kvm/pauth.o
>>> arch/arm64/kvm/pauth.c:40:9: error: instruction requires: pauth
>>>                      "pacga %0, %1, %2" : "=r" (pac) : "r" (ptr), "r" (mod));
>>>                       ^
>>> <inline asm>:2:1: note: instantiated into assembly here
>>> pacga x19, x1, x9
>>> ^
>>
>> It works when building with LLVM_IAS=0, which we obviously don't
>> want to mandate here. The variant below works for both clang+ias
>> (including all still supported versions) and gcc+binutils, but at
>> that point it gets obscure enough that your .inst version is easier
>> to understand.
> 
> Exactly. Either we have a good way to abstract this behind the scenes
> (which I don't see right now), or we just assume control of the
> instruction generation, which is what my patch does.
Ack.
> 
> In general, I question the value of the ".arch_extension" requirement
> for something like Linux, where we already have a pretty fine grained
> control of what we want to see being output by the compiler. but that
> ship has sailed long ago.
> 
> Thanks,
> 
> 	M.
> 
>>
>>     arnd
>>
>> --- a/arch/arm64/kvm/pauth.c
>> +++ b/arch/arm64/kvm/pauth.c
>> @@ -36,7 +36,12 @@ static u64 compute_pac(struct kvm_vcpu *vcpu, u64 ptr,
>>         __ptrauth_key_install_nosync(APGA, ikey);
>>         isb();
>>  
>> -       asm volatile(ARM64_ASM_PREAMBLE ".arch_extension pauth\n"
>> +       asm volatile(ARM64_ASM_PREAMBLE
>> +#ifdef CONFIG_AS_IS_LLVM
>> +                    ".arch_extension pauth\n"
>> +#else
>> +                    ".arch armv8.3-a\n"
>> +#endif
>>                      "pacga %0, %1, %2" : "=r" (pac) : "r" (ptr), "r" (mod));
>>         isb();
>>  
>>
> 

-- 
Thx and BRs,
Aiqun(Maria) Yu

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

end of thread, other threads:[~2024-04-24  1:54 UTC | newest]

Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2024-04-22 22:48 [PATCH] KVM: arm64: nv: Work around lack of pauth support in old toolchains Marc Zyngier
2024-04-23  8:24 ` Arnd Bergmann
2024-04-23 12:00   ` Aiqun Yu (Maria)
2024-04-23 12:06     ` Marc Zyngier
2024-04-23 12:37       ` Arnd Bergmann
2024-04-23 16:15         ` Marc Zyngier
2024-04-24  1:54           ` Aiqun Yu (Maria)
2024-04-23  8:37 ` Mark Rutland
2024-04-23 11:33   ` Marc Zyngier

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).