Re: [PATCH v2 5/5] KVM: arm64: Simplify PtrAuth alternative patching

[Marc Zyngier <maz@kernel.org>]

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.
-- 
Jazz is not dead. It just smells funny...