On Fri, Apr 21 2023, Jean-Philippe Brucker <jean-philippe@xxxxxxxxxx> wrote:
> The KVM_CHECK_EXTENSION ioctl can be issued either on the global fd
> (/dev/kvm), or on the VM fd obtained with KVM_CREATE_VM. For most
> extensions, KVM returns the same value with either method, but for some
> of them it can refine the returned value depending on the VM type. The
> KVM documentation [1] advises to use the VM fd:
>
> Based on their initialization different VMs may have different
> capabilities. It is thus encouraged to use the vm ioctl to query for
> capabilities (available with KVM_CAP_CHECK_EXTENSION_VM on the vm fd)
>
> Ongoing work on Arm confidential VMs confirms this, as some capabilities
> become unavailable to confidential VMs, requiring changes in QEMU to use
> kvm_vm_check_extension() instead of kvm_check_extension() [2]. Rather
> than changing each check one by one, change kvm_check_extension() to
> always issue the ioctl on the VM fd when available, and remove
> kvm_vm_check_extension().
>
> Fall back to the global fd when the VM check is unavailable:
>
> * Ancient kernels do not support KVM_CHECK_EXTENSION on the VM fd, since
> it was added by commit 92b591a4c46b ("KVM: Allow KVM_CHECK_EXTENSION
> on the vm fd") in Linux 3.17 [3]. Support for Linux 3.16 ended only in
> June 2020, but there may still be old images around.
>
> * A couple of calls must be issued before the VM fd is available, since
> they determine the VM type: KVM_CAP_MIPS_VZ and KVM_CAP_ARM_VM_IPA_SIZE
>
> Does any user actually depend on the check being done on the global fd
> instead of the VM fd? I surveyed all cases where KVM can return
> different values depending on the query method. Luckily QEMU already
> calls kvm_vm_check_extension() for most of those. Only three of them are
> ambiguous, because currently done on the global fd:
>
> * KVM_CAP_MAX_VCPUS and KVM_CAP_MAX_VCPU_ID on Arm, changes value if the
> user requests a vGIC different from the default. But QEMU queries this
> before vGIC configuration, so the reported value will be the same.
>
> * KVM_CAP_SW_TLB on PPC. When issued on the global fd, returns false if
> the kvm-hv module is loaded; when issued on the VM fd, returns false
> only if the VM type is HV instead of PR. If this returns false, then
> QEMU will fail to initialize a BOOKE206 MMU model.
>
> So this patch supposedly improves things, as it allows to run this
> type of vCPU even when both KVM modules are loaded.
>
> * KVM_CAP_PPC_SECURE_GUEST. Similarly, doing this check on a VM fd
> refines the returned value, and ensures that SVM is actually
> supported. Since QEMU follows the check with kvm_vm_enable_cap(), this
> patch should only provide better error reporting.
>
> [1] https://www.kernel.org/doc/html/latest/virt/kvm/api.html#kvm-check-extension
> [2] https://lore.kernel.org/kvm/875ybi0ytc.fsf@xxxxxxxxxx/
> [3] https://github.com/torvalds/linux/commit/92b591a4c46b
>
> Suggested-by: Cornelia Huck <cohuck@xxxxxxxxxx>
> Signed-off-by: Jean-Philippe Brucker <jean-philippe@xxxxxxxxxx>
> ---
> include/sysemu/kvm.h | 2 --
> include/sysemu/kvm_int.h | 1 +
> accel/kvm/kvm-all.c | 26 +++++++++-----------------
> target/i386/kvm/kvm.c | 6 +++---
> target/ppc/kvm.c | 34 +++++++++++++++++-----------------
> 5 files changed, 30 insertions(+), 39 deletions(-)
>
(...)
> diff --git a/accel/kvm/kvm-all.c b/accel/kvm/kvm-all.c
> index cf3a88d90e..eca815e763 100644
> --- a/accel/kvm/kvm-all.c
> +++ b/accel/kvm/kvm-all.c
> @@ -1109,22 +1109,13 @@ int kvm_check_extension(KVMState *s, unsigned int extension)
> {
> int ret;
>
> - ret = kvm_ioctl(s, KVM_CHECK_EXTENSION, extension);
> - if (ret < 0) {
> - ret = 0;
> + if (!s->check_extension_vm) {
> + ret = kvm_ioctl(s, KVM_CHECK_EXTENSION, extension);
> + } else {
> + ret = kvm_vm_ioctl(s, KVM_CHECK_EXTENSION, extension);
> }
> -
> - return ret;
> -}
> -
> -int kvm_vm_check_extension(KVMState *s, unsigned int extension)
> -{
> - int ret;
> -
> - ret = kvm_vm_ioctl(s, KVM_CHECK_EXTENSION, extension);
> if (ret < 0) {
> - /* VM wide version not implemented, use global one instead */
> - ret = kvm_check_extension(s, extension);
> + ret = 0;
> }
>
> return ret;
> @@ -2328,7 +2319,7 @@ static void kvm_irqchip_create(KVMState *s)
> */
> static int kvm_recommended_vcpus(KVMState *s)
> {
> - int ret = kvm_vm_check_extension(s, KVM_CAP_NR_VCPUS);
> + int ret = kvm_check_extension(s, KVM_CAP_NR_VCPUS);
> return (ret) ? ret : 4;
> }
>
> @@ -2480,6 +2471,7 @@ static int kvm_init(MachineState *ms)
> }
>
> s->vmfd = ret;
> + s->check_extension_vm = kvm_check_extension(s, KVM_CAP_CHECK_EXTENSION_VM);
Hm, it's a bit strange to set s->check_extension_vm by calling a
function that already checks for the value of
s->check_extension_vm... would it be better to call kvm_ioctl() directly
on this line?
I think it would be good if some ppc folks could give this a look, but
in general, it looks fine to me.
