| Message ID | 20200622080643.171651-6-maz@kernel.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | KVM/arm64: Enable PtrAuth on non-VHE KVM | expand |
On Mon, Jun 22, 2020 at 09:06:43AM +0100, Marc Zyngier wrote: > We currently decide to execute the PtrAuth save/restore code based > on a set of branches that evaluate as (ARM64_HAS_ADDRESS_AUTH_ARCH || > ARM64_HAS_ADDRESS_AUTH_IMP_DEF). This can be easily replaced by > a much simpler test as the ARM64_HAS_ADDRESS_AUTH capability is > exactly this expression. > > Suggested-by: Mark Rutland <mark.rutland@arm.com> > Signed-off-by: Marc Zyngier <maz@kernel.org> Looks good to me. One minor suggestion below, but either way: Acked-by: Mark Rutland <mark.rutland@arm.com> > --- > arch/arm64/include/asm/kvm_ptrauth.h | 26 +++++++++----------------- > 1 file changed, 9 insertions(+), 17 deletions(-) > > diff --git a/arch/arm64/include/asm/kvm_ptrauth.h b/arch/arm64/include/asm/kvm_ptrauth.h > index f1830173fa9e..7a72508a841b 100644 > --- a/arch/arm64/include/asm/kvm_ptrauth.h > +++ b/arch/arm64/include/asm/kvm_ptrauth.h > @@ -61,44 +61,36 @@ > > /* > * Both ptrauth_switch_to_guest and ptrauth_switch_to_host macros will > - * check for the presence of one of the cpufeature flag > - * ARM64_HAS_ADDRESS_AUTH_ARCH or ARM64_HAS_ADDRESS_AUTH_IMP_DEF and > + * check for the presence ARM64_HAS_ADDRESS_AUTH, which is defined as > + * (ARM64_HAS_ADDRESS_AUTH_ARCH || ARM64_HAS_ADDRESS_AUTH_IMP_DEF) and > * then proceed ahead with the save/restore of Pointer Authentication > - * key registers. > + * key registers if enabled for the guest. > */ > .macro ptrauth_switch_to_guest g_ctxt, reg1, reg2, reg3 > -alternative_if ARM64_HAS_ADDRESS_AUTH_ARCH > +alternative_if_not ARM64_HAS_ADDRESS_AUTH > b 1000f > alternative_else_nop_endif > -alternative_if_not ARM64_HAS_ADDRESS_AUTH_IMP_DEF > - b 1001f > -alternative_else_nop_endif > -1000: > mrs \reg1, hcr_el2 > and \reg1, \reg1, #(HCR_API | HCR_APK) > - cbz \reg1, 1001f > + cbz \reg1, 1000f > add \reg1, \g_ctxt, #CPU_APIAKEYLO_EL1 > ptrauth_restore_state \reg1, \reg2, \reg3 > -1001: > +1000: > .endm Since these are in macros, we could use \@ to generate a macro-specific lavel rather than a magic number, which would be less likely to conflict with the surrounding environment and would be more descriptive. We do that in a few places already, and here it could look something like: | alternative_if_not ARM64_HAS_ADDRESS_AUTH | b .L__skip_pauth_switch\@ | alternative_else_nop_endif | | ... | | .L__skip_pauth_switch\@: Per the gas documentation | \@ | | as maintains a counter of how many macros it has executed in this | pseudo-variable; you can copy that number to your output with ‘\@’, | but only within a macro definition. No worries if you don't want to change that now; the Acked-by stands either way. Mark. > > .macro ptrauth_switch_to_host g_ctxt, h_ctxt, reg1, reg2, reg3 > -alternative_if ARM64_HAS_ADDRESS_AUTH_ARCH > +alternative_if_not ARM64_HAS_ADDRESS_AUTH > b 2000f > alternative_else_nop_endif > -alternative_if_not ARM64_HAS_ADDRESS_AUTH_IMP_DEF > - b 2001f > -alternative_else_nop_endif > -2000: > mrs \reg1, hcr_el2 > and \reg1, \reg1, #(HCR_API | HCR_APK) > - cbz \reg1, 2001f > + cbz \reg1, 2000f > add \reg1, \g_ctxt, #CPU_APIAKEYLO_EL1 > ptrauth_save_state \reg1, \reg2, \reg3 > add \reg1, \h_ctxt, #CPU_APIAKEYLO_EL1 > ptrauth_restore_state \reg1, \reg2, \reg3 > isb > -2001: > +2000: > .endm > > #else /* !CONFIG_ARM64_PTR_AUTH */ > -- > 2.27.0 >
Hi Mark, On 2020-06-22 10:15, Mark Rutland wrote: > On Mon, Jun 22, 2020 at 09:06:43AM +0100, Marc Zyngier wrote: >> We currently decide to execute the PtrAuth save/restore code based >> on a set of branches that evaluate as (ARM64_HAS_ADDRESS_AUTH_ARCH || >> ARM64_HAS_ADDRESS_AUTH_IMP_DEF). This can be easily replaced by >> a much simpler test as the ARM64_HAS_ADDRESS_AUTH capability is >> exactly this expression. >> >> Suggested-by: Mark Rutland <mark.rutland@arm.com> >> Signed-off-by: Marc Zyngier <maz@kernel.org> > > Looks good to me. One minor suggestion below, but either way: > > Acked-by: Mark Rutland <mark.rutland@arm.com> > >> --- >> arch/arm64/include/asm/kvm_ptrauth.h | 26 +++++++++----------------- >> 1 file changed, 9 insertions(+), 17 deletions(-) >> >> diff --git a/arch/arm64/include/asm/kvm_ptrauth.h >> b/arch/arm64/include/asm/kvm_ptrauth.h >> index f1830173fa9e..7a72508a841b 100644 >> --- a/arch/arm64/include/asm/kvm_ptrauth.h >> +++ b/arch/arm64/include/asm/kvm_ptrauth.h >> @@ -61,44 +61,36 @@ >> >> /* >> * Both ptrauth_switch_to_guest and ptrauth_switch_to_host macros >> will >> - * check for the presence of one of the cpufeature flag >> - * ARM64_HAS_ADDRESS_AUTH_ARCH or ARM64_HAS_ADDRESS_AUTH_IMP_DEF and >> + * check for the presence ARM64_HAS_ADDRESS_AUTH, which is defined as >> + * (ARM64_HAS_ADDRESS_AUTH_ARCH || ARM64_HAS_ADDRESS_AUTH_IMP_DEF) >> and >> * then proceed ahead with the save/restore of Pointer Authentication >> - * key registers. >> + * key registers if enabled for the guest. >> */ >> .macro ptrauth_switch_to_guest g_ctxt, reg1, reg2, reg3 >> -alternative_if ARM64_HAS_ADDRESS_AUTH_ARCH >> +alternative_if_not ARM64_HAS_ADDRESS_AUTH >> b 1000f >> alternative_else_nop_endif >> -alternative_if_not ARM64_HAS_ADDRESS_AUTH_IMP_DEF >> - b 1001f >> -alternative_else_nop_endif >> -1000: >> mrs \reg1, hcr_el2 >> and \reg1, \reg1, #(HCR_API | HCR_APK) >> - cbz \reg1, 1001f >> + cbz \reg1, 1000f >> add \reg1, \g_ctxt, #CPU_APIAKEYLO_EL1 >> ptrauth_restore_state \reg1, \reg2, \reg3 >> -1001: >> +1000: >> .endm > > Since these are in macros, we could use \@ to generate a macro-specific > lavel rather than a magic number, which would be less likely to > conflict > with the surrounding environment and would be more descriptive. We do > that in a few places already, and here it could look something like: > > | alternative_if_not ARM64_HAS_ADDRESS_AUTH > | b .L__skip_pauth_switch\@ > | alternative_else_nop_endif > | > | ... > | > | .L__skip_pauth_switch\@: > > Per the gas documentation > > | \@ > | > | as maintains a counter of how many macros it has executed in this > | pseudo-variable; you can copy that number to your output with > ‘\@’, > | but only within a macro definition. > > No worries if you don't want to change that now; the Acked-by stands > either way. I have folded in the following patch: diff --git a/arch/arm64/include/asm/kvm_ptrauth.h b/arch/arm64/include/asm/kvm_ptrauth.h index 7a72508a841b..0ddf98c3ba9f 100644 --- a/arch/arm64/include/asm/kvm_ptrauth.h +++ b/arch/arm64/include/asm/kvm_ptrauth.h @@ -68,29 +68,29 @@ */ .macro ptrauth_switch_to_guest g_ctxt, reg1, reg2, reg3 alternative_if_not ARM64_HAS_ADDRESS_AUTH - b 1000f + b .L__skip_switch\@ alternative_else_nop_endif mrs \reg1, hcr_el2 and \reg1, \reg1, #(HCR_API | HCR_APK) - cbz \reg1, 1000f + cbz \reg1, .L__skip_switch\@ add \reg1, \g_ctxt, #CPU_APIAKEYLO_EL1 ptrauth_restore_state \reg1, \reg2, \reg3 -1000: +.L__skip_switch\@: .endm .macro ptrauth_switch_to_host g_ctxt, h_ctxt, reg1, reg2, reg3 alternative_if_not ARM64_HAS_ADDRESS_AUTH - b 2000f + b .L__skip_switch\@ alternative_else_nop_endif mrs \reg1, hcr_el2 and \reg1, \reg1, #(HCR_API | HCR_APK) - cbz \reg1, 2000f + cbz \reg1, .L__skip_switch\@ add \reg1, \g_ctxt, #CPU_APIAKEYLO_EL1 ptrauth_save_state \reg1, \reg2, \reg3 add \reg1, \h_ctxt, #CPU_APIAKEYLO_EL1 ptrauth_restore_state \reg1, \reg2, \reg3 isb -2000: +.L__skip_switch\@: .endm #else /* !CONFIG_ARM64_PTR_AUTH */ Thanks, M.
On Mon, Jun 22, 2020 at 11:25:41AM +0100, Marc Zyngier wrote: > On 2020-06-22 10:15, Mark Rutland wrote: > > On Mon, Jun 22, 2020 at 09:06:43AM +0100, Marc Zyngier wrote: > I have folded in the following patch: > > diff --git a/arch/arm64/include/asm/kvm_ptrauth.h > b/arch/arm64/include/asm/kvm_ptrauth.h > index 7a72508a841b..0ddf98c3ba9f 100644 > --- a/arch/arm64/include/asm/kvm_ptrauth.h > +++ b/arch/arm64/include/asm/kvm_ptrauth.h > @@ -68,29 +68,29 @@ > */ > .macro ptrauth_switch_to_guest g_ctxt, reg1, reg2, reg3 > alternative_if_not ARM64_HAS_ADDRESS_AUTH > - b 1000f > + b .L__skip_switch\@ > alternative_else_nop_endif > mrs \reg1, hcr_el2 > and \reg1, \reg1, #(HCR_API | HCR_APK) > - cbz \reg1, 1000f > + cbz \reg1, .L__skip_switch\@ > add \reg1, \g_ctxt, #CPU_APIAKEYLO_EL1 > ptrauth_restore_state \reg1, \reg2, \reg3 > -1000: > +.L__skip_switch\@: > .endm > > .macro ptrauth_switch_to_host g_ctxt, h_ctxt, reg1, reg2, reg3 > alternative_if_not ARM64_HAS_ADDRESS_AUTH > - b 2000f > + b .L__skip_switch\@ > alternative_else_nop_endif > mrs \reg1, hcr_el2 > and \reg1, \reg1, #(HCR_API | HCR_APK) > - cbz \reg1, 2000f > + cbz \reg1, .L__skip_switch\@ > add \reg1, \g_ctxt, #CPU_APIAKEYLO_EL1 > ptrauth_save_state \reg1, \reg2, \reg3 > add \reg1, \h_ctxt, #CPU_APIAKEYLO_EL1 > ptrauth_restore_state \reg1, \reg2, \reg3 > isb > -2000: > +.L__skip_switch\@: > .endm Looks good to me; thanks! Mark.
On Mon, Jun 22, 2020 at 10:15:08AM +0100, Mark Rutland wrote: > On Mon, Jun 22, 2020 at 09:06:43AM +0100, Marc Zyngier wrote: > > --- a/arch/arm64/include/asm/kvm_ptrauth.h > > +++ b/arch/arm64/include/asm/kvm_ptrauth.h > > @@ -61,44 +61,36 @@ > > > > /* > > * Both ptrauth_switch_to_guest and ptrauth_switch_to_host macros will > > - * check for the presence of one of the cpufeature flag > > - * ARM64_HAS_ADDRESS_AUTH_ARCH or ARM64_HAS_ADDRESS_AUTH_IMP_DEF and > > + * check for the presence ARM64_HAS_ADDRESS_AUTH, which is defined as > > + * (ARM64_HAS_ADDRESS_AUTH_ARCH || ARM64_HAS_ADDRESS_AUTH_IMP_DEF) and > > * then proceed ahead with the save/restore of Pointer Authentication > > - * key registers. > > + * key registers if enabled for the guest. > > */ > > .macro ptrauth_switch_to_guest g_ctxt, reg1, reg2, reg3 > > -alternative_if ARM64_HAS_ADDRESS_AUTH_ARCH > > +alternative_if_not ARM64_HAS_ADDRESS_AUTH > > b 1000f > > alternative_else_nop_endif > > -alternative_if_not ARM64_HAS_ADDRESS_AUTH_IMP_DEF > > - b 1001f > > -alternative_else_nop_endif > > -1000: > > mrs \reg1, hcr_el2 > > and \reg1, \reg1, #(HCR_API | HCR_APK) > > - cbz \reg1, 1001f > > + cbz \reg1, 1000f > > add \reg1, \g_ctxt, #CPU_APIAKEYLO_EL1 > > ptrauth_restore_state \reg1, \reg2, \reg3 > > -1001: > > +1000: > > .endm > > Since these are in macros, we could use \@ to generate a macro-specific > lavel rather than a magic number, which would be less likely to conflict > with the surrounding environment and would be more descriptive. We do > that in a few places already, and here it could look something like: > > | alternative_if_not ARM64_HAS_ADDRESS_AUTH > | b .L__skip_pauth_switch\@ > | alternative_else_nop_endif > | > | ... > | > | .L__skip_pauth_switch\@: > > Per the gas documentation > > | \@ > | > | as maintains a counter of how many macros it has executed in this > | pseudo-variable; you can copy that number to your output with ‘\@’, > | but only within a macro definition. Is this relibale for this sort of application? The description just sounds like a counter of macros rather than specifically a unique label generator. It may work most of the time but also seems that it has the potential to be more fragile given that it would change based on the rest of the code in the file to potentially conflict with something it didn't previously conflict with.
On Mon, Jun 22, 2020 at 11:39:32AM +0100, Andrew Scull wrote: > On Mon, Jun 22, 2020 at 10:15:08AM +0100, Mark Rutland wrote: > > On Mon, Jun 22, 2020 at 09:06:43AM +0100, Marc Zyngier wrote: > > > > > --- a/arch/arm64/include/asm/kvm_ptrauth.h > > > +++ b/arch/arm64/include/asm/kvm_ptrauth.h > > > @@ -61,44 +61,36 @@ > > > > > > /* > > > * Both ptrauth_switch_to_guest and ptrauth_switch_to_host macros will > > > - * check for the presence of one of the cpufeature flag > > > - * ARM64_HAS_ADDRESS_AUTH_ARCH or ARM64_HAS_ADDRESS_AUTH_IMP_DEF and > > > + * check for the presence ARM64_HAS_ADDRESS_AUTH, which is defined as > > > + * (ARM64_HAS_ADDRESS_AUTH_ARCH || ARM64_HAS_ADDRESS_AUTH_IMP_DEF) and > > > * then proceed ahead with the save/restore of Pointer Authentication > > > - * key registers. > > > + * key registers if enabled for the guest. > > > */ > > > .macro ptrauth_switch_to_guest g_ctxt, reg1, reg2, reg3 > > > -alternative_if ARM64_HAS_ADDRESS_AUTH_ARCH > > > +alternative_if_not ARM64_HAS_ADDRESS_AUTH > > > b 1000f > > > alternative_else_nop_endif > > > -alternative_if_not ARM64_HAS_ADDRESS_AUTH_IMP_DEF > > > - b 1001f > > > -alternative_else_nop_endif > > > -1000: > > > mrs \reg1, hcr_el2 > > > and \reg1, \reg1, #(HCR_API | HCR_APK) > > > - cbz \reg1, 1001f > > > + cbz \reg1, 1000f > > > add \reg1, \g_ctxt, #CPU_APIAKEYLO_EL1 > > > ptrauth_restore_state \reg1, \reg2, \reg3 > > > -1001: > > > +1000: > > > .endm > > > > Since these are in macros, we could use \@ to generate a macro-specific > > lavel rather than a magic number, which would be less likely to conflict > > with the surrounding environment and would be more descriptive. We do > > that in a few places already, and here it could look something like: > > > > | alternative_if_not ARM64_HAS_ADDRESS_AUTH > > | b .L__skip_pauth_switch\@ > > | alternative_else_nop_endif > > | > > | ... > > | > > | .L__skip_pauth_switch\@: > > > > Per the gas documentation > > > > | \@ > > | > > | as maintains a counter of how many macros it has executed in this > > | pseudo-variable; you can copy that number to your output with ‘\@’, > > | but only within a macro definition. > > Is this relibale for this sort of application? The description just > sounds like a counter of macros rather than specifically a unique label > generator. It may work most of the time but also seems that it has the > potential to be more fragile given that it would change based on the > rest of the code in the file to potentially conflict with something it > didn't previously conflict with. Ah, you invoke a macro in order for the label to be generated so it will increment and the label is namespaced by the prefix. I see.
diff --git a/arch/arm64/include/asm/kvm_ptrauth.h b/arch/arm64/include/asm/kvm_ptrauth.h index f1830173fa9e..7a72508a841b 100644 --- a/arch/arm64/include/asm/kvm_ptrauth.h +++ b/arch/arm64/include/asm/kvm_ptrauth.h @@ -61,44 +61,36 @@ /* * Both ptrauth_switch_to_guest and ptrauth_switch_to_host macros will - * check for the presence of one of the cpufeature flag - * ARM64_HAS_ADDRESS_AUTH_ARCH or ARM64_HAS_ADDRESS_AUTH_IMP_DEF and + * check for the presence ARM64_HAS_ADDRESS_AUTH, which is defined as + * (ARM64_HAS_ADDRESS_AUTH_ARCH || ARM64_HAS_ADDRESS_AUTH_IMP_DEF) and * then proceed ahead with the save/restore of Pointer Authentication - * key registers. + * key registers if enabled for the guest. */ .macro ptrauth_switch_to_guest g_ctxt, reg1, reg2, reg3 -alternative_if ARM64_HAS_ADDRESS_AUTH_ARCH +alternative_if_not ARM64_HAS_ADDRESS_AUTH b 1000f alternative_else_nop_endif -alternative_if_not ARM64_HAS_ADDRESS_AUTH_IMP_DEF - b 1001f -alternative_else_nop_endif -1000: mrs \reg1, hcr_el2 and \reg1, \reg1, #(HCR_API | HCR_APK) - cbz \reg1, 1001f + cbz \reg1, 1000f add \reg1, \g_ctxt, #CPU_APIAKEYLO_EL1 ptrauth_restore_state \reg1, \reg2, \reg3 -1001: +1000: .endm .macro ptrauth_switch_to_host g_ctxt, h_ctxt, reg1, reg2, reg3 -alternative_if ARM64_HAS_ADDRESS_AUTH_ARCH +alternative_if_not ARM64_HAS_ADDRESS_AUTH b 2000f alternative_else_nop_endif -alternative_if_not ARM64_HAS_ADDRESS_AUTH_IMP_DEF - b 2001f -alternative_else_nop_endif -2000: mrs \reg1, hcr_el2 and \reg1, \reg1, #(HCR_API | HCR_APK) - cbz \reg1, 2001f + cbz \reg1, 2000f add \reg1, \g_ctxt, #CPU_APIAKEYLO_EL1 ptrauth_save_state \reg1, \reg2, \reg3 add \reg1, \h_ctxt, #CPU_APIAKEYLO_EL1 ptrauth_restore_state \reg1, \reg2, \reg3 isb -2001: +2000: .endm #else /* !CONFIG_ARM64_PTR_AUTH */
We currently decide to execute the PtrAuth save/restore code based on a set of branches that evaluate as (ARM64_HAS_ADDRESS_AUTH_ARCH || ARM64_HAS_ADDRESS_AUTH_IMP_DEF). This can be easily replaced by a much simpler test as the ARM64_HAS_ADDRESS_AUTH capability is exactly this expression. Suggested-by: Mark Rutland <mark.rutland@arm.com> Signed-off-by: Marc Zyngier <maz@kernel.org> --- arch/arm64/include/asm/kvm_ptrauth.h | 26 +++++++++----------------- 1 file changed, 9 insertions(+), 17 deletions(-)