On 02/02/2025 06:00, Gavin Shan wrote:
> On 12/13/24 1:55 AM, Steven Price wrote:
>> From: Suzuki K Poulose <suzuki.poulose@xxxxxxx>
>>
>> Given we have different types of VMs supported, check the
>> support for SVE for the given instance of the VM to accurately
>> report the status.
>>
>> Signed-off-by: Suzuki K Poulose <suzuki.poulose@xxxxxxx>
>> Signed-off-by: Steven Price <steven.price@xxxxxxx>
>> ---
>> arch/arm64/include/asm/kvm_rme.h | 2 ++
>> arch/arm64/kvm/arm.c | 5 ++++-
>> arch/arm64/kvm/rme.c | 5 +++++
>> 3 files changed, 11 insertions(+), 1 deletion(-)
>>
>> diff --git a/arch/arm64/include/asm/kvm_rme.h b/arch/arm64/include/
>> asm/kvm_rme.h
>> index 90a4537ad38d..0d89ab1645c1 100644
>> --- a/arch/arm64/include/asm/kvm_rme.h
>> +++ b/arch/arm64/include/asm/kvm_rme.h
>> @@ -85,6 +85,8 @@ void kvm_init_rme(void);
>> u32 kvm_realm_ipa_limit(void);
>> u32 kvm_realm_vgic_nr_lr(void);
>> +bool kvm_rme_supports_sve(void);
>> +
>> 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);
>> diff --git a/arch/arm64/kvm/arm.c b/arch/arm64/kvm/arm.c
>> index 134acb4ee26f..6f7f96ab781d 100644
>> --- a/arch/arm64/kvm/arm.c
>> +++ b/arch/arm64/kvm/arm.c
>> @@ -456,7 +456,10 @@ int kvm_vm_ioctl_check_extension(struct kvm *kvm,
>> long ext)
>> r = get_kvm_ipa_limit();
>> break;
>> case KVM_CAP_ARM_SVE:
>> - r = system_supports_sve();
>> + if (kvm_is_realm(kvm))
>> + r = kvm_rme_supports_sve();
>> + else
>> + r = system_supports_sve();
>> break;
>
> kvm_vm_ioctl_check_extension() can be called by
> ioctl(KVM_CHECK_EXTENSION) on the
> file descriptor of '/dev/kvm'. kvm is NULL and kvm_is_realm() returns
> false in
> this case.
>
> kvm_dev_ioctl
> kvm_vm_ioctl_check_extension_generic // kvm is NULL
> kvm_vm_ioctl_check_extension
See my reply in patch 25
>> case KVM_CAP_ARM_PTRAUTH_ADDRESS:
>> case KVM_CAP_ARM_PTRAUTH_GENERIC:
>> diff --git a/arch/arm64/kvm/rme.c b/arch/arm64/kvm/rme.c
>> index 5831d379760a..27a479feb907 100644
>> --- a/arch/arm64/kvm/rme.c
>> +++ b/arch/arm64/kvm/rme.c
>> @@ -20,6 +20,11 @@ static bool rme_supports(unsigned long feature)
>> return !!u64_get_bits(rmm_feat_reg0, feature);
>> }
>> +bool kvm_rme_supports_sve(void)
>> +{
>> + return rme_supports(RMI_FEATURE_REGISTER_0_SVE_EN);
>> +}
>> +
>
> If rme_supports() becomes a public helper, it can be directly used. In
> turn,
> kvm_rme_supports_sve() can be dropped. RMI_FEATURE_REGISTER_0_SVE_EN is
> obvious
> to indicate the corresponding feature.
I agree this seem reasonable. Sadly the use of u64_get_bits() is
assuming that the 'feature' parameter is constant at runtime. I could
rework it to not require a constant, but considering this is the only
function which is exposing rme_supports() without any further checks I
feel it's better to leave the code as it is.
Obviously if we get more feature bits like this in the future then it
would be worth revisiting.
Thanks,
Steve
>> static int rmi_check_version(void)
>> {
>> struct arm_smccc_res res;
>
> Thanks,
> Gavin
>
