Re: [PATCH v3 01/26] kvm: Merge kvm_check_extension() and kvm_vm_check_extension()

[Jean-Philippe Brucker]

On Tue, Nov 26, 2024 at 12:29:35PM +0000, Daniel P. Berrangé wrote:
> On Mon, Nov 25, 2024 at 07:56:00PM +0000, Jean-Philippe Brucker 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().
> 
> The downside I see of this approach is that it can potentially
> mask mistakes / unexpected behaviour.
> 
> eg, consider you are in a code path where you /think/ the VM fd
> is available, but for some unexpected reason it is NOT in fact
> available. The code silently falls back to the global FD, thus
> giving a potentially incorrect extension check answer.
> 
> Having separate check methods with no fallback ensures that we
> are checking exactly what we /intend/ to be checking, or will
> see an error

Yes I see your point, and I'm happy dropping this patch since I'm less
familiar with the other archs.

The alternative is replacing kvm_check_extension() with
kvm_vm_check_extension() wherever the Arm ioctl handler behaves
differently depending on the VM type. Simple enough though it does affect
kvm-all.c too:

diff --git a/accel/kvm/kvm-all.c b/accel/kvm/kvm-all.c
index 801cff16a5..a56b943f31 100644
--- a/accel/kvm/kvm-all.c
+++ b/accel/kvm/kvm-all.c
@@ -2410,13 +2410,13 @@ static int kvm_recommended_vcpus(KVMState *s)
 
 static int kvm_max_vcpus(KVMState *s)
 {
-    int ret = kvm_check_extension(s, KVM_CAP_MAX_VCPUS);
+    int ret = kvm_vm_check_extension(s, KVM_CAP_MAX_VCPUS);
     return (ret) ? ret : kvm_recommended_vcpus(s);
 }
 
 static int kvm_max_vcpu_id(KVMState *s)
 {
-    int ret = kvm_check_extension(s, KVM_CAP_MAX_VCPU_ID);
+    int ret = kvm_vm_check_extension(s, KVM_CAP_MAX_VCPU_ID);
     return (ret) ? ret : kvm_max_vcpus(s);
 }
 
@@ -2693,7 +2693,7 @@ static int kvm_init(MachineState *ms)
 
 #ifdef TARGET_KVM_HAVE_GUEST_DEBUG
     kvm_has_guest_debug =
-        (kvm_check_extension(s, KVM_CAP_SET_GUEST_DEBUG) > 0);
+        (kvm_vm_check_extension(s, KVM_CAP_SET_GUEST_DEBUG) > 0);
 #endif
 
     kvm_sstep_flags = 0;
diff --git a/target/arm/kvm.c b/target/arm/kvm.c
index 7b6812c0de..609c6d4e7a 100644
--- a/target/arm/kvm.c
+++ b/target/arm/kvm.c
@@ -618,11 +618,11 @@ int kvm_arch_init(MachineState *ms, KVMState *s)
         }
     }
 
-    max_hw_wps = kvm_check_extension(s, KVM_CAP_GUEST_DEBUG_HW_WPS);
+    max_hw_wps = kvm_vm_check_extension(s, KVM_CAP_GUEST_DEBUG_HW_WPS);
     hw_watchpoints = g_array_sized_new(true, true,
                                        sizeof(HWWatchpoint), max_hw_wps);
 
-    max_hw_bps = kvm_check_extension(s, KVM_CAP_GUEST_DEBUG_HW_BPS);
+    max_hw_bps = kvm_vm_check_extension(s, KVM_CAP_GUEST_DEBUG_HW_BPS);
     hw_breakpoints = g_array_sized_new(true, true,
                                        sizeof(HWBreakpoint), max_hw_bps);
 
@@ -1764,7 +1764,7 @@ void kvm_arm_pvtime_init(ARMCPU *cpu, uint64_t ipa)
 
 void kvm_arm_steal_time_finalize(ARMCPU *cpu, Error **errp)
 {
-    bool has_steal_time = kvm_check_extension(kvm_state, KVM_CAP_STEAL_TIME);
+    bool has_steal_time = kvm_vm_check_extension(kvm_state, 
KVM_CAP_STEAL_TIME);
 
     if (cpu->kvm_steal_time == ON_OFF_AUTO_AUTO) {
         if (!has_steal_time || !arm_feature(&cpu->env, ARM_FEATURE_AARCH64)) {
@@ -1799,7 +1799,7 @@ bool kvm_arm_aarch32_supported(void)
 
 bool kvm_arm_sve_supported(void)
 {
-    return kvm_check_extension(kvm_state, KVM_CAP_ARM_SVE);
+    return kvm_vm_check_extension(kvm_state, KVM_CAP_ARM_SVE);
 }
 
 bool kvm_arm_mte_supported(void)