Re: [PATCH v9 1/5] KVM: arm64: Add a vcpu flag to control ptrauth for guest

[Marc Zyngier <marc.zyngier@arm.com>]

On 17/04/2019 14:08, Amit Daniel Kachhap wrote:
> Hi,
> 
> On 4/17/19 2:05 PM, Marc Zyngier wrote:
>> On 12/04/2019 04:20, Amit Daniel Kachhap wrote:
>>> A per vcpu flag is added to check if pointer authentication is
>>> enabled for the vcpu or not. This flag may be enabled according to
>>> the necessary user policies and host capabilities.
>>>
>>> This patch also adds a helper to check the flag.
>>>
>>> Signed-off-by: Amit Daniel Kachhap <amit.kachhap@arm.com>
>>> Cc: Mark Rutland <mark.rutland@arm.com>
>>> Cc: Marc Zyngier <marc.zyngier@arm.com>
>>> Cc: Christoffer Dall <christoffer.dall@arm.com>
>>> Cc: kvmarm@lists.cs.columbia.edu
>>> ---
>>>
>>> Changes since v8:
>>> * Added a new per vcpu flag which will store Pointer Authentication enable
>>>    status instead of checking them again. [Dave Martin]
>>>
>>>   arch/arm64/include/asm/kvm_host.h | 4 ++++
>>>   1 file changed, 4 insertions(+)
>>>
>>> diff --git a/arch/arm64/include/asm/kvm_host.h b/arch/arm64/include/asm/kvm_host.h
>>> index 9d57cf8..31dbc7c 100644
>>> --- a/arch/arm64/include/asm/kvm_host.h
>>> +++ b/arch/arm64/include/asm/kvm_host.h
>>> @@ -355,10 +355,14 @@ struct kvm_vcpu_arch {
>>>   #define KVM_ARM64_HOST_SVE_ENABLED	(1 << 4) /* SVE enabled for EL0 */
>>>   #define KVM_ARM64_GUEST_HAS_SVE		(1 << 5) /* SVE exposed to guest */
>>>   #define KVM_ARM64_VCPU_SVE_FINALIZED	(1 << 6) /* SVE config completed */
>>> +#define KVM_ARM64_GUEST_HAS_PTRAUTH	(1 << 7) /* PTRAUTH exposed to guest */
>>>   
>>>   #define vcpu_has_sve(vcpu) (system_supports_sve() && \
>>>   			    ((vcpu)->arch.flags & KVM_ARM64_GUEST_HAS_SVE))
>>>   
>>> +#define vcpu_has_ptrauth(vcpu)	\
>>> +			((vcpu)->arch.flags & KVM_ARM64_GUEST_HAS_PTRAUTH)
>>> +
>>
>> Just as for SVE, please first check that the system has PTRAUTH.
>> Something like:
>>
>> 		(cpus_have_const_cap(ARM64_HAS_GENERIC_AUTH_ARCH) && \
>> 		 ((vcpu)->arch.flags & KVM_ARM64_GUEST_HAS_PTRAUTH))
> 
> In the subsequent patches, vcpu->arch.flags is only set to 
> KVM_ARM64_GUEST_HAS_PTRAUTH when all host capability check conditions 
> matches such as system_supports_address_auth(), 
> system_supports_generic_auth() so doing them again is repetitive in my view.

It isn't the setting of the flag I care about, but the check of that
flag. Checking a flag for a feature that cannot be used on the running
system should have a zero cost, which isn't the case here.

Granted, the impact should be minimal and it looks like it mostly happen
on the slow path, but at the very least it would be consistent. So even
if you don't buy my argument about efficiency, please change it in the
name of consistency.

Thanks,

	M.
-- 
Jazz is not dead. It just smells funny...