Re: [PATCH v6 12/43] arm64: RME: Allocate/free RECs to match vCPUs

[Steven Price <steven.price@arm.com>]

On 29/01/2025 04:50, Gavin Shan wrote:
> On 12/13/24 1:55 AM, Steven Price wrote:
>> The RMM maintains a data structure known as the Realm Execution Context
>> (or REC). It is similar to struct kvm_vcpu and tracks the state of the
>> virtual CPUs. KVM must delegate memory and request the structures are
>> created when vCPUs are created, and suitably tear down on destruction.
>>
>> RECs must also be supplied with addition pages - auxiliary (or AUX)
>> granules - for storing the larger registers state (e.g. for SVE). The
>> number of AUX granules for a REC depends on the parameters with which
>> the Realm was created - the RMM makes this information available via the
>> RMI_REC_AUX_COUNT call performed after creating the Realm Descriptor
>> (RD).
>>
>> Note that only some of register state for the REC can be set by KVM, the
>> rest is defined by the RMM (zeroed). The register state then cannot be
>> changed by KVM after the REC is created (except when the guest
>> explicitly requests this e.g. by performing a PSCI call). The RMM also
>> requires that the VMM creates RECs in ascending order of the MPIDR.
>>
>> See Realm Management Monitor specification (DEN0137) for more
>> information:
>> https://developer.arm.com/documentation/den0137/
>>
>> Signed-off-by: Steven Price <steven.price@arm.com>
>> ---
>> Changes since v5:
>>   * Separate the concept of vcpu_is_rec() and
>>     kvm_arm_vcpu_rec_finalized() by using the KVM_ARM_VCPU_REC feature as
>>     the indication that the VCPU is a REC.
>> Changes since v2:
>>   * Free rec->run earlier in kvm_destroy_realm() and adapt to previous
>> patches.
>> ---
>>   arch/arm64/include/asm/kvm_emulate.h |   7 ++
>>   arch/arm64/include/asm/kvm_host.h    |   3 +
>>   arch/arm64/include/asm/kvm_rme.h     |  18 ++++
>>   arch/arm64/kvm/arm.c                 |   9 ++
>>   arch/arm64/kvm/reset.c               |  11 ++
>>   arch/arm64/kvm/rme.c                 | 144 +++++++++++++++++++++++++++
>>   6 files changed, 192 insertions(+)
>>
>> diff --git a/arch/arm64/include/asm/kvm_emulate.h b/arch/arm64/
>> include/asm/kvm_emulate.h
>> index 27f54a7778aa..ec2b6d9c9c07 100644
>> --- a/arch/arm64/include/asm/kvm_emulate.h
>> +++ b/arch/arm64/include/asm/kvm_emulate.h
>> @@ -722,7 +722,14 @@ static inline bool kvm_realm_is_created(struct
>> kvm *kvm)
>>     static inline bool vcpu_is_rec(struct kvm_vcpu *vcpu)
>>   {
>> +    if (static_branch_unlikely(&kvm_rme_is_available))
>> +        return vcpu_has_feature(vcpu, KVM_ARM_VCPU_REC);
>>       return false;
>>   }
>>   
> 
> It seems the check on kvm_rme_is_available is unnecessary because
> KVM_ARM_VCPU_REC
> is possible to be true only when kvm_rme_is_available is true.

Similar to a previous patch - the check of the static key is to avoid
overhead on systems without RME.

>> +static inline bool kvm_arm_vcpu_rec_finalized(struct kvm_vcpu *vcpu)
>> +{
>> +    return vcpu->arch.rec.mpidr != INVALID_HWID;
>> +}
>> +
>>   #endif /* __ARM64_KVM_EMULATE_H__ */
> 
> I would suggest to rename to kvm_arm_rec_finalized() since vCPU and REC are
> similar objects at the same level. It'd better to avoid duplicate object
> name reference in the function name.

Ack

>> diff --git a/arch/arm64/include/asm/kvm_host.h b/arch/arm64/include/
>> asm/kvm_host.h
>> index 8482638dce3b..220195c727ef 100644
>> --- a/arch/arm64/include/asm/kvm_host.h
>> +++ b/arch/arm64/include/asm/kvm_host.h
>> @@ -789,6 +789,9 @@ struct kvm_vcpu_arch {
>>         /* Per-vcpu CCSIDR override or NULL */
>>       u32 *ccsidr;
>> +
>> +    /* Realm meta data */
>> +    struct realm_rec rec;
>>   };
>>     /*
>> diff --git a/arch/arm64/include/asm/kvm_rme.h b/arch/arm64/include/
>> asm/kvm_rme.h
>> index 32bdedf1d866..62d4a63d3035 100644
>> --- a/arch/arm64/include/asm/kvm_rme.h
>> +++ b/arch/arm64/include/asm/kvm_rme.h
>> @@ -6,6 +6,7 @@
>>   #ifndef __ASM_KVM_RME_H
>>   #define __ASM_KVM_RME_H
>>   +#include <asm/rmi_smc.h>
>>   #include <uapi/linux/kvm.h>
>>     /**
>> @@ -65,6 +66,21 @@ struct realm {
>>       unsigned int ia_bits;
>>   };
>>   +/**
>> + * struct realm_rec - Additional per VCPU data for a Realm
>> + *
>> + * @mpidr: MPIDR (Multiprocessor Affinity Register) value to identify
>> this VCPU
>> + * @rec_page: Kernel VA of the RMM's private page for this REC
>> + * @aux_pages: Additional pages private to the RMM for this REC
>> + * @run: Kernel VA of the RmiRecRun structure shared with the RMM
>> + */
>> +struct realm_rec {
>> +    unsigned long mpidr;
>> +    void *rec_page;
>> +    struct page *aux_pages[REC_PARAMS_AUX_GRANULES];
>> +    struct rec_run *run;
>> +};
>> +
>>   void kvm_init_rme(void);
>>   u32 kvm_realm_ipa_limit(void);
>>   @@ -72,6 +88,8 @@ int kvm_realm_enable_cap(struct kvm *kvm, struct
>> kvm_enable_cap *cap);
>>   int kvm_init_realm_vm(struct kvm *kvm);
>>   void kvm_destroy_realm(struct kvm *kvm);
>>   void kvm_realm_destroy_rtts(struct kvm *kvm, u32 ia_bits);
>> +int kvm_create_rec(struct kvm_vcpu *vcpu);
>> +void kvm_destroy_rec(struct kvm_vcpu *vcpu);
>>     #define RMM_RTT_BLOCK_LEVEL    2
>>   #define RMM_RTT_MAX_LEVEL    3
>> diff --git a/arch/arm64/kvm/arm.c b/arch/arm64/kvm/arm.c
>> index 73016e1e0067..2d97147651be 100644
>> --- a/arch/arm64/kvm/arm.c
>> +++ b/arch/arm64/kvm/arm.c
>> @@ -525,6 +525,8 @@ int kvm_arch_vcpu_create(struct kvm_vcpu *vcpu)
>>       /* Force users to call KVM_ARM_VCPU_INIT */
>>       vcpu_clear_flag(vcpu, VCPU_INITIALIZED);
>>   +    vcpu->arch.rec.mpidr = INVALID_HWID;
>> +
>>       vcpu->arch.mmu_page_cache.gfp_zero = __GFP_ZERO;
>>         /* Set up the timer */
>> @@ -1467,6 +1469,9 @@ static unsigned long
>> system_supported_vcpu_features(void)
>>       if (!cpus_have_final_cap(ARM64_HAS_NESTED_VIRT))
>>           clear_bit(KVM_ARM_VCPU_HAS_EL2, &features);
>>   +    if (!static_branch_unlikely(&kvm_rme_is_available))
>> +        clear_bit(KVM_ARM_VCPU_REC, &features);
>> +
>>       return features;
>>   }
>>   @@ -1506,6 +1511,10 @@ static int
>> kvm_vcpu_init_check_features(struct kvm_vcpu *vcpu,
>>       if (test_bit(KVM_ARM_VCPU_HAS_EL2, &features))
>>           return -EINVAL;
>>   +    /* RME is incompatible with AArch32 */
>> +    if (test_bit(KVM_ARM_VCPU_REC, &features))
>> +        return -EINVAL;
>> +
>>       return 0;
>>   }
>>   
> 
> One corner case seems missed in kvm_vcpu_init_check_features(). It's
> allowed to
> initialize a vCPU with REC feature even kvm_is_realm(kvm) is false.
> Hopefully,
> I didn't miss something.

Ah, yes good point. I'll pass a kvm pointer to
kvm_vcpu_init_check_features() and use kvm_is_realm() in there.

Thanks,

Steve

> [...]
> 
> Thanks,
> Gavin
>