Re: [PATCH v2 4/7] KVM: nVMX: Add a quirk for KVM tweaks to VMX control MSRs

[Sean Christopherson <seanjc@xxxxxxxxxx>]

On Tue, Feb 08, 2022, Oliver Upton wrote:
> On Mon, Feb 7, 2022 at 10:06 AM Sean Christopherson <seanjc@xxxxxxxxxx> wrote:
> 
> [...]
> 
> > > +#define KVM_X86_QUIRK_TWEAK_VMX_CTRL_MSRS    (1 << 5)
> >
> > I'd prefer we include msr_ia32_feature_control_valid_bits in this quirk, it should
> > be relatively easy to do since most of the modifications stem from
> > vmx_vcpu_after_set_cpuid().  vmx_setup_mce() is a bit odd, but IMO it's worth
> > excising as much crud as we can.
> >
> 
> Sure, this is a good opportunity to rip out the crud.
> msr_ia32_feature_control_valid_bits is a bit messy, since the default
> value does not contain all the bits we support. At least with
> IA32_VM_TRUE_{ENTRY,EXIT}_CTLS we slim down the hardware values to get
> the default value.
> 
> Not at all objecting, but it looks like we will need to populate some
> bits in the default value of the IA32_FEAT_CTL mask, otherwise with
> the quirk enabled guests could never set any of the bits in the MSR.

I assume you mean "quirk disabled"?  Because quirks are on by default, i.e. KVM's
default behavior will be to populate msr_ia32_feature_control_valid_bits based on
CPUID updates.

That said, after typing up what I had in mind, I don't think we need a quirk at all.
The only weird part is that KVM doesn't allow host userspace to set the MSR without
first setting CPUID.  That's trivial to fix and we can do so without impacting KVM's
modeling of WRMSR from the guest.  Modeling WRMSR is no different than KVM enforcing
CR4 bits based on CPUID.  The VMX MSRs are weird because they are technically
independent of the non-virtualization support reported in CPUID, i.e. KVM is overstepping
by manipulating the MSRs based on CPUID.

I'll send this is a formal patch, obviously with KVM_SUPPORTED_FEATURE_CONTROL
defined...

diff --git a/arch/x86/kvm/vmx/vmx.c b/arch/x86/kvm/vmx/vmx.c
index d05b4955d14f..d50ae2de8b51 100644
--- a/arch/x86/kvm/vmx/vmx.c
+++ b/arch/x86/kvm/vmx/vmx.c
@@ -1749,11 +1749,16 @@ bool nested_vmx_allowed(struct kvm_vcpu *vcpu)
 }

 static inline bool vmx_feature_control_msr_valid(struct kvm_vcpu *vcpu,
-                                                uint64_t val)
+                                                struct msr_data *msr)
 {
-       uint64_t valid_bits = to_vmx(vcpu)->msr_ia32_feature_control_valid_bits;
+       uint64_t valid_bits;

-       return !(val & ~valid_bits);
+       if (msr->host_initiated)
+               valid_bits = KVM_SUPPORTED_FEATURE_CONTROL;
+       else
+               to_vmx(vcpu)->msr_ia32_feature_control_valid_bits;
+
+       return !(msr->data & ~valid_bits);
 }

 static int vmx_get_msr_feature(struct kvm_msr_entry *msr)
@@ -2146,7 +2151,7 @@ static int vmx_set_msr(struct kvm_vcpu *vcpu, struct msr_data *msr_info)
                vcpu->arch.mcg_ext_ctl = data;
                break;
        case MSR_IA32_FEAT_CTL:
-               if (!vmx_feature_control_msr_valid(vcpu, data) ||
+               if (!vmx_feature_control_msr_valid(vcpu, msr_info) ||
                    (to_vmx(vcpu)->msr_ia32_feature_control &
                     FEAT_CTL_LOCKED && !msr_info->host_initiated))
                        return 1;