Re: Re: [PATCH v3 2/2] RISC-V: KVM: add KVM_CAP_RISCV_SET_HGATP_MODE

[fangyu.yu@linux.alibaba.com]

>> From: Fangyu Yu <fangyu.yu@linux.alibaba.com>
>>
>> This capability allows userspace to explicitly select the HGATP mode
>> for the VM. The selected mode must be less than or equal to the max
>> HGATP mode supported by the hardware. This capability must be enabled
>> before creating any vCPUs, and can only be set once per VM.
>>
>> Signed-off-by: Fangyu Yu <fangyu.yu@linux.alibaba.com>
>> ---
>>  Documentation/virt/kvm/api.rst | 18 ++++++++++++++++++
>>  arch/riscv/kvm/vm.c            | 26 ++++++++++++++++++++++++--
>>  include/uapi/linux/kvm.h       |  1 +
>>  3 files changed, 43 insertions(+), 2 deletions(-)
>>
>> diff --git a/Documentation/virt/kvm/api.rst b/Documentation/virt/kvm/api.rst
>> @@ -8765,6 +8765,24 @@ helpful if user space wants to emulate instructions which are not
>> +7.47 KVM_CAP_RISCV_SET_HGATP_MODE
>> +---------------------------------
>> +
>> +:Architectures: riscv
>> +:Type: VM
>> +:Parameters: args[0] contains the requested HGATP mode
>> +:Returns:
>> +  - 0 on success.
>> +  - -EINVAL if args[0] is outside the range of HGATP modes supported by the
>> +    hardware.
>> +  - -EBUSY if vCPUs have already been created for the VM, if the VM has any
>> +    non-empty memslots, or if the capability has already been set for the VM.
>> +
>> +This capability allows userspace to explicitly select the HGATP mode for
>> +the VM. The selected mode must be less than or equal to the maximum HGATP
>> +mode supported by the hardware.
>
>"The selected mode must be supported by both KVM and hardware."

This description is clear, Agreed.

>(The comparison is a technical detail, and incorrect too since the value
> is bouded from the bottom as well.)
>
>>                                  This capability must be enabled before
>> +creating any vCPUs, and can only be set once per VM.
>
>                     ^ "or memslots"

Right, thanks for catching that.

>
>> diff --git a/arch/riscv/kvm/vm.c b/arch/riscv/kvm/vm.c
>> @@ -202,6 +202,9 @@ int kvm_vm_ioctl_check_extension(struct kvm *kvm, long ext)
>>  	case KVM_CAP_VM_GPA_BITS:
>>  		r = kvm_riscv_gstage_gpa_bits(&kvm->arch);
>>  		break;
>> +	case KVM_CAP_RISCV_SET_HGATP_MODE:
>> +		r = IS_ENABLED(CONFIG_64BIT) ? 1 : 0;
>
>Maybe we can return the currently selected mode for a bit of extra info?
>Another nice option would be to return a bitmask of all supported modes.
>
>I think userspace has otherwise no reason to call it, since it's fine to
>just try enable and handle the -EINVAL as "don't care".
>1 syscall instead of 2.

I’d prefer to keep an explicit KVM_CHECK_EXTENSION implementation for
KVM_CAP_RISCV_SET_HGATP_MODE. Userspace commonly uses CHECK_EXTENSION for
capability discovery, and returning 0 would make it assume the capability is
unsupported even though KVM_ENABLE_CAP works.

Returning 1/0 should be sufficient here, as the actual mode support is
validated by KVM_ENABLE_CAP itself (with -EINVAL for unsupported modes).

>> @@ -212,12 +215,31 @@ int kvm_vm_ioctl_check_extension(struct kvm *kvm, long ext)
>>  
>>  int kvm_vm_ioctl_enable_cap(struct kvm *kvm, struct kvm_enable_cap *cap)
>>  {
>> +	case KVM_CAP_RISCV_SET_HGATP_MODE:
>> +#ifdef CONFIG_64BIT
>> +		if (cap->args[0] < HGATP_MODE_SV39X4 ||
>> +		    cap->args[0] > kvm_riscv_gstage_mode(kvm_riscv_gstage_max_pgd_levels))
>> +			return -EINVAL;
>> +
>> +		if (kvm->arch.gstage_mode_user_initialized || kvm->created_vcpus ||
>> +		    !kvm_are_all_memslots_empty(kvm))
>> +			return -EBUSY;
>> +
>> +		kvm->arch.gstage_mode_user_initialized = true;
>
>No need to have gstage_mode_user_initialized, since if the user could
>have changed it once, there shouldn't be an issue in changing it again.
>It's the other protections that must work.

Agreed — I'll drop gstage_mode_user_initialized. Userspace can change the
mode multiple times before the VM is committed, and updates will be gated
by the existing protections (i.e. once <vcpus created/ran | memslots active>,
the mode change will be rejected).

>> +		kvm->arch.kvm_riscv_gstage_pgd_levels =
>> +				3 + cap->args[0] - HGATP_MODE_SV39X4;
>> +		kvm_debug("VM (vmid:%lu) using SV%lluX4 G-stage page table format\n",
>> +			  kvm->arch.vmid.vmid,
>> +			  39 + (cap->args[0] - HGATP_MODE_SV39X4) * 9);
>
>(I don't think this debug message is going to be useful after a short
> debugging period, and it would clog the log on each VM launch, so I'd
> rather get rid of it.)

I'll drop this kvm_debug() from the capability path to avoid spamming the
log on each VM creation.

>Thanks.

Thanks,
Fangyu

_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv