On 29/01/2025 04:07, Gavin Shan wrote:
> On 12/13/24 1:55 AM, Steven Price wrote:
>> Previously machine type was used purely for specifying the physical
>> address size of the guest. Reserve the higher bits to specify an ARM
>> specific machine type and declare a new type 'KVM_VM_TYPE_ARM_REALM'
>> used to create a realm guest.
>>
>> Reviewed-by: Suzuki K Poulose <suzuki.poulose@xxxxxxx>
>> Signed-off-by: Steven Price <steven.price@xxxxxxx>
>> ---
>> arch/arm64/kvm/arm.c | 17 +++++++++++++++++
>> arch/arm64/kvm/mmu.c | 3 ---
>> include/uapi/linux/kvm.h | 19 +++++++++++++++----
>> 3 files changed, 32 insertions(+), 7 deletions(-)
>>
>> diff --git a/arch/arm64/kvm/arm.c b/arch/arm64/kvm/arm.c
>> index c505ec61180a..73016e1e0067 100644
>> --- a/arch/arm64/kvm/arm.c
>> +++ b/arch/arm64/kvm/arm.c
>> @@ -207,6 +207,23 @@ int kvm_arch_init_vm(struct kvm *kvm, unsigned
>> long type)
>> mutex_unlock(&kvm->lock);
>> #endif
>> + if (type & ~(KVM_VM_TYPE_ARM_MASK |
>> KVM_VM_TYPE_ARM_IPA_SIZE_MASK))
>> + return -EINVAL;
>> +
>> + switch (type & KVM_VM_TYPE_ARM_MASK) {
>> + case KVM_VM_TYPE_ARM_NORMAL:
>> + break;
>> + case KVM_VM_TYPE_ARM_REALM:
>> + kvm->arch.is_realm = true;
>> + if (!kvm_is_realm(kvm)) {
>> + /* Realm support unavailable */
>> + return -EINVAL;
>> + }
>> + break;
>> + default:
>> + return -EINVAL;
>> + }
>> +
>> kvm_init_nested(kvm);
>> ret = kvm_share_hyp(kvm, kvm + 1);
>
> Corresponding to comments for PATCH[6], the block of the code can be
> modified
> to avoid using kvm_is_realm() here. In this way, kvm_is_realm() can be
> simplifed
> as I commented for PATCH[6].
>
> case KVM_VM_TYPE_ARM_REALM:
> if (static_branch_unlikely(&kvm_rme_is_available))
> return -EPERM; /* -EPERM may be more suitable than -
> EINVAL */
>
> kvm->arch.is_realm = true;
> break;
Yes that's more readable. I'd used kvm_is_realm() because I wanted to
keep the check on kvm_rme_is_available to one place, but coming back to
the code there's definitely a "Huh?" moment from setting 'is_realm' and
then testing if it's a realm!
I also agree -EPERM is probably better to signify that the kernel
supports realms but the hardware doesn't.
Thanks,
Steve
