Re: [PATCH v2 3/4] KVM: nVMX: Don't muck with allowed sec exec controls on CPUID changes

[Sean Christopherson]

On Wed, Jan 04, 2023, Sean Christopherson wrote:
> On Fri, Dec 23, 2022, Paolo Bonzini wrote:
> > On 12/13/22 07:23, Sean Christopherson wrote:
> > > Don't modify the set of allowed secondary execution controls, i.e. the
> > > virtual MSR_IA32_VMX_PROCBASED_CTLS2, in response to guest CPUID changes.
> > > To avoid breaking old userspace that never sets the VMX MSRs, i.e. relies
> > > on KVM to provide a consistent vCPU model, keep the existing behavior if
> > > userspace has never written MSR_IA32_VMX_PROCBASED_CTLS2.
> > > 
> > > KVM should not modify the VMX capabilities presented to L1 based on CPUID
> > > as doing so may discard explicit settings provided by userspace.  E.g. if
> > > userspace does KVM_SET_MSRS => KVM_SET_CPUID and disables a feature in
> > > the VMX MSRs but not CPUID (to prevent exposing the feature to L2), then
> > > stuffing the VMX MSRs during KVM_SET_CPUID will expose the feature to L2
> > > against userspace's wishes.
> > 
> > The commit message doesn't explain *why* KVM_SET_CPUID would be done before
> > KVM_SET_MSRS.
> 
> I assume you mean why KVM_SET_MSRS would be done before KVM_SET_CPUID2?
> 
> This patch is mostly paranoia, AFAIK there is no userspace that is negatively
> affected by KVM's manipulations.  The only case I can think of is if userspace
> wanted to emulate dynamic CPUID updates, e.g. set an MSR filter to intercept writes
> to MISC_ENABLES to update MONITOR/MWAIT support, but that behavior isn't allowed
> since commit feb627e8d6f6 ("KVM: x86: Forbid KVM_SET_CPUID{,2} after KVM_RUN").
> 
> There are scenarios where userspace might do KVM_SET_MSRS before KVM_SET_CPUID,
> e.g. QEMU's reuse of a vCPU for CPU hotplug, but in those cases I would expect
> userspace to follow up with another KVM_SET_MSRS.

An argument for taking this patch is that it might be necessary to disallow
KVM_SET_MSRS after KVM_RUN[*].  If KVM manipulates VMX MSRs during KVM_SET_CPUID2,
reusing a vCPU with sequence:

  SET_CPUID2 => SET_MSRS => RUN => unplug => hotplug => SET_CPUID2 => SET_MSRS

sequence will cause the second SET_MSRS to fail due to userspace "changing" the
MSR value.

[*] https://lore.kernel.org/all/20220805172945.35412-4-seanjc@xxxxxxxxxx