Re: [PATCH 2/4] KVM: nVMX: Invert 'unsupported by eVMCSv1' check
From: Sean Christopherson <seanjc@google.com>
Date: 2022-10-26 23:21:02
Also in:
kvm, lkml
Subsystem:
kernel virtual machine for x86 (kvm/x86), the rest, x86 architecture (32-bit and 64-bit) · Maintainers:
Sean Christopherson, Paolo Bonzini, Linus Torvalds, Thomas Gleixner, Ingo Molnar, Borislav Petkov, Dave Hansen
On Tue, Oct 18, 2022, Vitaly Kuznetsov wrote:
When a new feature gets implemented in KVM, EVMCS1_UNSUPPORTED_* defines need to be adjusted to avoid the situation when the feature is exposed to the guest but there's no corresponding eVMCS field[s] for it. This is not obvious and fragile.
Eh, either way is fragile, the only difference is what goes wrong when it breaks. At the risk of making this overly verbose, what about requiring developers to explicitly define whether or not a new control is support? E.g. keep the EVMCS1_UNSUPPORTED_* and then add compile-time assertions to verify that every feature that is REQUIRED | OPTIONAL is SUPPORTED | UNSUPPORTED. That way the eVMCS "supported" controls don't need to include the ALWAYSON controls, and anytime someone adds a new control, they'll have to stop and think about eVMCS. I think we'll still want (need?) the runtime sanitization, but this might allow catching at least some cases without needing to wait until a control actually gets exposed. E.g. possibly with more macro magic to reduce the boilerplate
diff --git a/arch/x86/kvm/vmx/evmcs.c b/arch/x86/kvm/vmx/evmcs.c
index d8b23c96d627..190932edcc02 100644
--- a/arch/x86/kvm/vmx/evmcs.c
+++ b/arch/x86/kvm/vmx/evmcs.c@@ -422,6 +422,10 @@ void nested_evmcs_filter_control_msr(struct kvm_vcpu *vcpu, u32 msr_index, u64 * u32 ctl_high = (u32)(*pdata >> 32); u32 unsupported_ctrls; + BUILD_BUG_ON((EVMCS1_SUPPORTED_PINCTRL | EVMCS1_UNSUPPORTED_PINCTRL) != + (KVM_REQUIRED_VMX_PIN_BASED_VM_EXEC_CONTROL | + KVM_OPTIONAL_VMX_PIN_BASED_VM_EXEC_CONTROL)); + /* * Hyper-V 2016 and 2019 try using these features even when eVMCS * is enabled but there are no corresponding fields.
diff --git a/arch/x86/kvm/vmx/evmcs.h b/arch/x86/kvm/vmx/evmcs.h
index 6f746ef3c038..58d77afe9d57 100644
--- a/arch/x86/kvm/vmx/evmcs.h
+++ b/arch/x86/kvm/vmx/evmcs.h@@ -48,6 +48,11 @@ DECLARE_STATIC_KEY_FALSE(enable_evmcs); */ #define EVMCS1_UNSUPPORTED_PINCTRL (PIN_BASED_POSTED_INTR | \ PIN_BASED_VMX_PREEMPTION_TIMER) +#define EVMCS1_SUPPORTED_PINCTRL \ + (PIN_BASED_EXT_INTR_MASK | \ + PIN_BASED_NMI_EXITING | \ + PIN_BASED_VIRTUAL_NMIS) + #define EVMCS1_UNSUPPORTED_EXEC_CTRL (CPU_BASED_ACTIVATE_TERTIARY_CONTROLS) #define EVMCS1_UNSUPPORTED_2NDEXEC \ (SECONDARY_EXEC_VIRTUAL_INTR_DELIVERY | \