Sean Christopherson <seanjc@xxxxxxxxxx> writes: > 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. Is this a good thing or a bad one? :-) I'm not against being extra verbose but adding a new feature to EVMCS1_SUPPORTED_* (even when there is a corresponding field) requires testing or a evmcs_has_perf_global_ctrl()-like story may happen and such testing would require access to Windows/Hyper-V images. This sounds like an extra burden for contributors. IMO it's OK if new features are mechanically added to EVMCS1_UNSUPPORTED_* on the grounds that it wasn't tested but then it's not much different from "unsupported by default" (my approach). So I'm on the fence here. > > 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 | \ > -- Vitaly
