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)
