Re: [PATCH v6 06/64] KVM: arm64: nv: Add nested virt VCPU primitives for vEL2 VCPU state

Marc Zyngier
2022-02-14T14:22:22Z

On 2022-02-14 12:39, Miguel Luis wrote:
> Hi Marc,
>
>> On 28 Jan 2022, at 11:18, Marc Zyngier <maz@kernel.org> wrote:
>>
>> From: Christoffer Dall <christoffer.dall@arm.com>
>>
>> When running a nested hypervisor we commonly have to figure out if
>> the VCPU mode is running in the context of a guest hypervisor or guest
>> guest, or just a normal guest.
>>
>> Add convenient primitives for this.
>>
>> Reviewed-by: Russell King (Oracle) <rmk+kernel@armlinux.org.uk>
>> Signed-off-by: Christoffer Dall <christoffer.dall@arm.com>
>> Signed-off-by: Marc Zyngier <maz@kernel.org>
>> ---
>> arch/arm64/include/asm/kvm_emulate.h | 53 ++++++++++++++++++++++++++++
>> 1 file changed, 53 insertions(+)
>>
>> diff --git a/arch/arm64/include/asm/kvm_emulate.h
>> b/arch/arm64/include/asm/kvm_emulate.h
>> index d62405ce3e6d..ea9a130c4b6a 100644
>> --- a/arch/arm64/include/asm/kvm_emulate.h
>> +++ b/arch/arm64/include/asm/kvm_emulate.h
>> @@ -178,6 +178,59 @@ static __always_inline void vcpu_set_reg(struct
>> kvm_vcpu *vcpu, u8 reg_num,
>> vcpu_gp_regs(vcpu)->regs[reg_num] = val;
>> }
>>
>> +static inline bool vcpu_is_el2_ctxt(const struct kvm_cpu_context
>> *ctxt)
>> +{
>> + switch (ctxt->regs.pstate & (PSR_MODE32_BIT | PSR_MODE_MASK)) {
>> + case PSR_MODE_EL2h:
>> + case PSR_MODE_EL2t:
>> + return true;
>> + default:
>> + return false;
>> + }
>> +}
>
> PSR_MODE_EL2{h,t} values the least significant nibble, so why the
> PSR_MODE32_BIT in the condition?

Because that's part of the M bits in SPSR, and this has to be
valid on any code path, no matter what execution state the
guest is in. You can't evaluate the M[3:0] on their own *unless*
you have already checked that M[4] is 0 (an AArch32 guest would
have M[4]==1).

Yes, we are so far lucky that AArch32 and AArch64 don't overlap
in their values of SPSR_EL2.M[3:0]. We may run out of luck at
some point.

>
> For the scope of this function as is, may I suggest:
>
> switch (ctxt->regs.pstate & PSR_MODE_MASK) {
>
> which should be sufficient to check if vcpu_is_el2_ctx.

I don't think this is wise. It makes the code more fragile,
and harder to reason about.

Thanks,

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