| Message ID | 20240422224849.2238222-1-maz@kernel.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | KVM: arm64: nv: Work around lack of pauth support in old toolchains | expand |
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>
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)
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.
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
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.
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();
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(); > >
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(); >> >> >
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);
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(-)