Fix bugs in KVM's (mis)handling of secondary execution controls.

KVM overrides the secondary execution control VMX MSR during KVM_SET_CPUID.
Similar to the somewhat recent reverts

  8805875aa473 ("Revert "KVM: nVMX: Do not expose MPX VMX controls when guest MPX disabled"")
  9389d5774aca ("Revert "KVM: nVMX: Expose load IA32_PERF_GLOBAL_CTRL VM-{Entry,Exit} control"")

undo misguided KVM behavior where KVM overrides allowed-1 settings in the
secondary execution controls in response to changes to the guest's CPUID
model.  To avoid breaking userspace that doesn't take ownership of the
VMX MSRs, go hands off if and only if userpace sets the MSR in question.

Before fixing that, fix another bug it was hiding where the umwait/tpause
control was being exposed to L1 for nVMX only after KVM_SET_CPUID, and
harden KVM against similar bugs in the future.

v2: Fix the ENABLE_USR_WAIT_PAUSE bug too. [Aaron]

v1: https://lore.kernel.org/all/20221110005706.1064832-1-seanjc@google.com

Sean Christopherson (4):
  KVM: nVMX: Properly expose ENABLE_USR_WAIT_PAUSE control to L1
  KVM: nVMX: Don't stuff secondary execution control if it's not
    supported
  KVM: nVMX: Don't muck with allowed sec exec controls on CPUID changes
  KVM: selftests: Test KVM's handling of VMX's sec exec MSR on
    KVM_SET_CPUID

 arch/x86/kvm/vmx/capabilities.h               |  1 +
 arch/x86/kvm/vmx/nested.c                     |  6 +-
 arch/x86/kvm/vmx/vmx.c                        | 17 +++-
 .../selftests/kvm/include/x86_64/processor.h  |  1 +
 .../selftests/kvm/include/x86_64/vmx.h        |  4 +-
 .../selftests/kvm/x86_64/vmx_msrs_test.c      | 92 +++++++++++++++++++
 6 files changed, 116 insertions(+), 5 deletions(-)


base-commit: 02076de83f4de19a045227b9d44084a30e936c26
-- 
2.39.0.rc1.256.g54fd8350bd-goog

On Tue, Dec 13, 2022 at 06:23:02AM +0000, Sean Christopherson wrote:
> Fix bugs in KVM's (mis)handling of secondary execution controls.
> 
> KVM overrides the secondary execution control VMX MSR during KVM_SET_CPUID.
> Similar to the somewhat recent reverts
> 
>   8805875aa473 ("Revert "KVM: nVMX: Do not expose MPX VMX controls when guest MPX disabled"")
>   9389d5774aca ("Revert "KVM: nVMX: Expose load IA32_PERF_GLOBAL_CTRL VM-{Entry,Exit} control"")
> 
> undo misguided KVM behavior where KVM overrides allowed-1 settings in the
> secondary execution controls in response to changes to the guest's CPUID
> model.  To avoid breaking userspace that doesn't take ownership of the
> VMX MSRs, go hands off if and only if userpace sets the MSR in question.
> 
> Before fixing that, fix another bug it was hiding where the umwait/tpause
> control was being exposed to L1 for nVMX only after KVM_SET_CPUID, and
> harden KVM against similar bugs in the future.
> 
> v2: Fix the ENABLE_USR_WAIT_PAUSE bug too. [Aaron]
> 
> v1: https://lore.kernel.org/all/20221110005706.1064832-1-seanjc@google.com
> 
> Sean Christopherson (4):
>   KVM: nVMX: Properly expose ENABLE_USR_WAIT_PAUSE control to L1
>   KVM: nVMX: Don't stuff secondary execution control if it's not
>     supported
>   KVM: nVMX: Don't muck with allowed sec exec controls on CPUID changes
>   KVM: selftests: Test KVM's handling of VMX's sec exec MSR on
>     KVM_SET_CPUID

BTW, we may need another patch to remove the obsolete comments in
nested_vmx_setup_ctls_msrs():

diff --git a/arch/x86/kvm/vmx/nested.c b/arch/x86/kvm/vmx/nested.c
index b6f4411b613e..42ceddcafd3e 100644
--- a/arch/x86/kvm/vmx/nested.c
+++ b/arch/x86/kvm/vmx/nested.c
@@ -6854,11 +6854,6 @@ void nested_vmx_setup_ctls_msrs(struct vmcs_config *vmcs_conf, u32 ept_caps)
        msrs->procbased_ctls_low &=
                ~(CPU_BASED_CR3_LOAD_EXITING | CPU_BASED_CR3_STORE_EXITING);

-       /*
-        * secondary cpu-based controls.  Do not include those that
-        * depend on CPUID bits, they are added later by
-        * vmx_vcpu_after_set_cpuid.
-        */
        msrs->secondary_ctls_low = 0;

        msrs->secondary_ctls_high = vmcs_conf->cpu_based_2nd_exec_ctrl;

B.R.
Yu

On Wed, Dec 14, 2022, Yu Zhang wrote:
> On Tue, Dec 13, 2022 at 06:23:02AM +0000, Sean Christopherson wrote:
> > Fix bugs in KVM's (mis)handling of secondary execution controls.
> > 
> > KVM overrides the secondary execution control VMX MSR during KVM_SET_CPUID.
> > Similar to the somewhat recent reverts
> > 
> >   8805875aa473 ("Revert "KVM: nVMX: Do not expose MPX VMX controls when guest MPX disabled"")
> >   9389d5774aca ("Revert "KVM: nVMX: Expose load IA32_PERF_GLOBAL_CTRL VM-{Entry,Exit} control"")
> > 
> > undo misguided KVM behavior where KVM overrides allowed-1 settings in the
> > secondary execution controls in response to changes to the guest's CPUID
> > model.  To avoid breaking userspace that doesn't take ownership of the
> > VMX MSRs, go hands off if and only if userpace sets the MSR in question.
> > 
> > Before fixing that, fix another bug it was hiding where the umwait/tpause
> > control was being exposed to L1 for nVMX only after KVM_SET_CPUID, and
> > harden KVM against similar bugs in the future.
> > 
> > v2: Fix the ENABLE_USR_WAIT_PAUSE bug too. [Aaron]
> > 
> > v1: https://lore.kernel.org/all/20221110005706.1064832-1-seanjc@google.com
> > 
> > Sean Christopherson (4):
> >   KVM: nVMX: Properly expose ENABLE_USR_WAIT_PAUSE control to L1
> >   KVM: nVMX: Don't stuff secondary execution control if it's not
> >     supported
> >   KVM: nVMX: Don't muck with allowed sec exec controls on CPUID changes
> >   KVM: selftests: Test KVM's handling of VMX's sec exec MSR on
> >     KVM_SET_CPUID
> 
> BTW, we may need another patch to remove the obsolete comments in
> nested_vmx_setup_ctls_msrs():

Ouch, indeed.  Want to send a proper patch?  Or provide your SoB and I'll write
a changelog?

The comment was added by commit 80154d77c922 ("KVM: VMX: cache secondary exec controls"),
but arguably the below is the appropriate Fixes, as it's the commit that fixed the
existing cases where KVM didn't enumerate supported-but-conditional controls.

Fixes: 6defc591846d ("KVM: nVMX: include conditional controls in /dev/kvm KVM_GET_MSRS")

> diff --git a/arch/x86/kvm/vmx/nested.c b/arch/x86/kvm/vmx/nested.c
> index b6f4411b613e..42ceddcafd3e 100644
> --- a/arch/x86/kvm/vmx/nested.c
> +++ b/arch/x86/kvm/vmx/nested.c
> @@ -6854,11 +6854,6 @@ void nested_vmx_setup_ctls_msrs(struct vmcs_config *vmcs_conf, u32 ept_caps)
>         msrs->procbased_ctls_low &=
>                 ~(CPU_BASED_CR3_LOAD_EXITING | CPU_BASED_CR3_STORE_EXITING);
> 
> -       /*
> -        * secondary cpu-based controls.  Do not include those that
> -        * depend on CPUID bits, they are added later by
> -        * vmx_vcpu_after_set_cpuid.
> -        */
>         msrs->secondary_ctls_low = 0;
> 
>         msrs->secondary_ctls_high = vmcs_conf->cpu_based_2nd_exec_ctrl;
> 
> B.R.
> Yu

On Thu, Dec 15, 2022 at 12:18:33AM +0000, Sean Christopherson wrote:
> > 
> > BTW, we may need another patch to remove the obsolete comments in
> > nested_vmx_setup_ctls_msrs():
> 
> Ouch, indeed.  Want to send a proper patch?  Or provide your SoB and I'll write
> a changelog?
> 
> The comment was added by commit 80154d77c922 ("KVM: VMX: cache secondary exec controls"),
> but arguably the below is the appropriate Fixes, as it's the commit that fixed the
> existing cases where KVM didn't enumerate supported-but-conditional controls.
> 
> Fixes: 6defc591846d ("KVM: nVMX: include conditional controls in /dev/kvm KVM_GET_MSRS")
> 

Thanks a lot, Sean, especially for sharing the commit history.

And I just sent out a patch to fix it.

One question is about the process of small cleanup patches like
this: would it be better off to include the cleanup patches as
part of a larger submission, or is it OK to be sent seperately?

Previously I submitted some small patches(e.g. [1] & [2]) but
have not received any reply. So I am just wondering, maybe those
patches are too trivial and sometimes time-wasting for the reviewers?

Any suggestion? Thanks!

B.R.
Yu

[1]: [PATCH] KVM: MMU: Add wrapper to check whether MMU is in direct mode
https://www.spinics.net/lists/kvm/msg297583.html
[2]: [PATCH v2 0/2] Cleanup VMFUNC handling in KVM. 
https://www.spinics.net/lists/kernel/msg4582139.html

On Thu, Dec 15, 2022, Yu Zhang wrote:
> On Thu, Dec 15, 2022 at 12:18:33AM +0000, Sean Christopherson wrote:
> > > 
> > > BTW, we may need another patch to remove the obsolete comments in
> > > nested_vmx_setup_ctls_msrs():
> > 
> > Ouch, indeed.  Want to send a proper patch?  Or provide your SoB and I'll write
> > a changelog?
> > 
> > The comment was added by commit 80154d77c922 ("KVM: VMX: cache secondary exec controls"),
> > but arguably the below is the appropriate Fixes, as it's the commit that fixed the
> > existing cases where KVM didn't enumerate supported-but-conditional controls.
> > 
> > Fixes: 6defc591846d ("KVM: nVMX: include conditional controls in /dev/kvm KVM_GET_MSRS")
> > 
> 
> Thanks a lot, Sean, especially for sharing the commit history.
> 
> And I just sent out a patch to fix it.
> 
> One question is about the process of small cleanup patches like
> this: would it be better off to include the cleanup patches as
> part of a larger submission, or is it OK to be sent seperately?

In this case, it's ok to be sent separately.  There are no code dependencies, and
the changelog can be written to say "this comment is no longer accurate", even if
there is still broken code in KVM.

> Previously I submitted some small patches(e.g. [1] & [2]) but
> have not received any reply. So I am just wondering, maybe those
> patches are too trivial and sometimes time-wasting for the reviewers?

They're definitely not too trivial.  This is just an especially rough time of
year for reviews, e.g. end of year corporate stuff, merge window, holidays, etc.

Part of why I haven't provided reviews is that the patches _aren't_ super trivial,
e.g. I'm on the fence on whether mmu_is_direct() should take @vcpu or @mmu, and if
I vote to have it take @mmu, then that'll conflict with mmu_is_nested().  So I end
up staying silent until I can come back to it with fresh eyes to see if there's a
better alternative, or if I'm just being nitpicky.

> 
> They're definitely not too trivial.  This is just an especially rough time of
> year for reviews, e.g. end of year corporate stuff, merge window, holidays, etc.
> 

Glad to know that. Thanks!

> Part of why I haven't provided reviews is that the patches _aren't_ super trivial,
> e.g. I'm on the fence on whether mmu_is_direct() should take @vcpu or @mmu, and if
> I vote to have it take @mmu, then that'll conflict with mmu_is_nested().  So I end
> up staying silent until I can come back to it with fresh eyes to see if there's a
> better alternative, or if I'm just being nitpicky.
> 
Well, though I would prefer mmu_is_direct(), I appreciate for
being considerate. We can discuss it later. :)

B.R.
Yu