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/[email protected]
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
Verify that KVM does, and does not, modify the allowed set of VMX's
secondary execution controls during KVM_SET_CPUID. Historically, KVM has
modified select bits in response to guest CPUID changes to try and force
a consistent CPU model. KVM's meddling causes problems if userspace
invokes KVM_SET_CPUID after explicitly setting the MSR, as KVM may end up
overriding a legal userspace config.
Newer, fixed KVM versions maintain the historical meddling for backwards
compatibility, but only if userspace has never set the MSR for the vCPU.
I.e. KVM transfers ownership to userspace on the first write.
Signed-off-by: Sean Christopherson <[email protected]>
---
.../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 +++++++++++++++++++
3 files changed, 95 insertions(+), 2 deletions(-)
diff --git a/tools/testing/selftests/kvm/include/x86_64/processor.h b/tools/testing/selftests/kvm/include/x86_64/processor.h
index b1a31de7108a..9314a06f56d3 100644
--- a/tools/testing/selftests/kvm/include/x86_64/processor.h
+++ b/tools/testing/selftests/kvm/include/x86_64/processor.h
@@ -109,6 +109,7 @@ struct kvm_x86_cpu_feature {
#define X86_FEATURE_INVPCID KVM_X86_CPU_FEATURE(0x7, 0, EBX, 10)
#define X86_FEATURE_RTM KVM_X86_CPU_FEATURE(0x7, 0, EBX, 11)
#define X86_FEATURE_MPX KVM_X86_CPU_FEATURE(0x7, 0, EBX, 14)
+#define X86_FEATURE_RDSEED KVM_X86_CPU_FEATURE(0x7, 0, EBX, 18)
#define X86_FEATURE_SMAP KVM_X86_CPU_FEATURE(0x7, 0, EBX, 20)
#define X86_FEATURE_PCOMMIT KVM_X86_CPU_FEATURE(0x7, 0, EBX, 22)
#define X86_FEATURE_CLFLUSHOPT KVM_X86_CPU_FEATURE(0x7, 0, EBX, 23)
diff --git a/tools/testing/selftests/kvm/include/x86_64/vmx.h b/tools/testing/selftests/kvm/include/x86_64/vmx.h
index 5f0c0a29c556..b66661ba28c8 100644
--- a/tools/testing/selftests/kvm/include/x86_64/vmx.h
+++ b/tools/testing/selftests/kvm/include/x86_64/vmx.h
@@ -61,8 +61,8 @@
#define SECONDARY_EXEC_SHADOW_VMCS 0x00004000
#define SECONDARY_EXEC_RDSEED_EXITING 0x00010000
#define SECONDARY_EXEC_ENABLE_PML 0x00020000
-#define SECONDARY_EPT_VE 0x00040000
-#define SECONDARY_ENABLE_XSAV_RESTORE 0x00100000
+#define SECONDARY_EXEC_EPT_VE 0x00040000
+#define SECONDARY_EXEC_ENABLE_XSAVES 0x00100000
#define SECONDARY_EXEC_TSC_SCALING 0x02000000
#define PIN_BASED_EXT_INTR_MASK 0x00000001
diff --git a/tools/testing/selftests/kvm/x86_64/vmx_msrs_test.c b/tools/testing/selftests/kvm/x86_64/vmx_msrs_test.c
index 90720b6205f4..d7b1a72a8912 100644
--- a/tools/testing/selftests/kvm/x86_64/vmx_msrs_test.c
+++ b/tools/testing/selftests/kvm/x86_64/vmx_msrs_test.c
@@ -12,6 +12,96 @@
#include "kvm_util.h"
#include "vmx.h"
+static void vmx_sec_exec_assert_allowed(struct kvm_vcpu *vcpu,
+ const char *name, uint64_t ctrl)
+{
+ TEST_ASSERT(vcpu_get_msr(vcpu, MSR_IA32_VMX_PROCBASED_CTLS2) & ctrl,
+ "Expected '%s' to be allowed in sec exec controls", name);
+}
+
+static void vmx_sec_exec_assert_denied(struct kvm_vcpu *vcpu,
+ const char *name, uint64_t ctrl)
+{
+ TEST_ASSERT(!(vcpu_get_msr(vcpu, MSR_IA32_VMX_PROCBASED_CTLS2) & ctrl),
+ "Expected '%s' to be denied in sec exec controls", name);
+}
+
+static void vmx_sec_exec_control_test(struct kvm_vcpu *vcpu,
+ const char *name,
+ struct kvm_x86_cpu_feature feature,
+ uint64_t ctrl, bool kvm_owned)
+{
+ /* Allowed-1 settings are in the upper 32 bits. */
+ ctrl <<= 32;
+
+ if (!this_cpu_has(feature))
+ return;
+
+ if (kvm_owned) {
+ vcpu_set_cpuid_feature(vcpu, feature);
+ vmx_sec_exec_assert_allowed(vcpu, name, ctrl);
+
+ vcpu_clear_cpuid_feature(vcpu, feature);
+ vmx_sec_exec_assert_denied(vcpu, name, ctrl);
+
+ /* Make sure KVM is actually toggling the bit. */
+ vcpu_set_cpuid_feature(vcpu, feature);
+ vmx_sec_exec_assert_allowed(vcpu, name, ctrl);
+ } else {
+ vcpu_set_msr(vcpu, MSR_IA32_VMX_PROCBASED_CTLS2,
+ vcpu_get_msr(vcpu, MSR_IA32_VMX_PROCBASED_CTLS2) | ctrl);
+ vmx_sec_exec_assert_allowed(vcpu, name, ctrl);
+
+ vcpu_set_cpuid_feature(vcpu, feature);
+ vmx_sec_exec_assert_allowed(vcpu, name, ctrl);
+
+ vcpu_clear_cpuid_feature(vcpu, feature);
+ vmx_sec_exec_assert_allowed(vcpu, name, ctrl);
+
+ vcpu_set_msr(vcpu, MSR_IA32_VMX_PROCBASED_CTLS2,
+ vcpu_get_msr(vcpu, MSR_IA32_VMX_PROCBASED_CTLS2) & ~ctrl);
+ vmx_sec_exec_assert_denied(vcpu, name, ctrl);
+
+ vcpu_set_cpuid_feature(vcpu, feature);
+ vmx_sec_exec_assert_denied(vcpu, name, ctrl);
+
+ vcpu_clear_cpuid_feature(vcpu, feature);
+ vmx_sec_exec_assert_denied(vcpu, name, ctrl);
+ }
+}
+
+#define vmx_sec_exec_feature_test(vcpu, name, kvm_owned) \
+ vmx_sec_exec_control_test(vcpu, #name, X86_FEATURE_##name, \
+ SECONDARY_EXEC_ENABLE_##name, kvm_owned)
+
+#define vmx_sec_exec_exiting_test(vcpu, name, kvm_owned) \
+ vmx_sec_exec_control_test(vcpu, #name, X86_FEATURE_##name, \
+ SECONDARY_EXEC_##name##_EXITING, kvm_owned)
+
+static void vmx_sec_exec_controls_test(struct kvm_vcpu *vcpu)
+{
+ int i;
+
+ if (this_cpu_has(X86_FEATURE_XSAVE))
+ vcpu_set_cpuid_feature(vcpu, X86_FEATURE_XSAVE);
+
+ if (this_cpu_has(X86_FEATURE_RDPID))
+ vcpu_clear_cpuid_feature(vcpu, X86_FEATURE_RDPID);
+
+ /*
+ * Verify that for features KVM has historically taken control of, KVM
+ * updates PROCBASED_CTLS2 during KVM_SET_CPUID if userspace has never
+ * set the MSR, but leaves it alone once userspace writes the MSR.
+ */
+ for (i = 0; i < 2; i++) {
+ vmx_sec_exec_feature_test(vcpu, XSAVES, !i);
+ vmx_sec_exec_feature_test(vcpu, RDTSCP, !i);
+ vmx_sec_exec_feature_test(vcpu, INVPCID, !i);
+ vmx_sec_exec_exiting_test(vcpu, RDRAND, !i);
+ vmx_sec_exec_exiting_test(vcpu, RDSEED, !i);
+ }
+}
+
static void vmx_fixed1_msr_test(struct kvm_vcpu *vcpu, uint32_t msr_index,
uint64_t mask)
{
@@ -124,6 +214,8 @@ int main(void)
/* No need to actually do KVM_RUN, thus no guest code. */
vm = vm_create_with_one_vcpu(&vcpu, NULL);
+ vmx_sec_exec_controls_test(vcpu);
+
vmx_save_restore_msrs_test(vcpu);
ia32_feature_control_msr_test(vcpu);
--
2.39.0.rc1.256.g54fd8350bd-goog
Set ENABLE_USR_WAIT_PAUSE in KVM's supported VMX MSR configuration if the
feature is supported in hardware and enabled in KVM's base, non-nested
configuration, i.e. expose ENABLE_USR_WAIT_PAUSE to L1 if it's supported.
This fixes a bug where saving/restoring, i.e. migrating, a vCPU will fail
if WAITPKG (the associated CPUID feature) is enabled for the vCPU, and
obviously allows L1 to enable the feature for L2.
KVM already effectively exposes ENABLE_USR_WAIT_PAUSE to L1 by stuffing
the allowed-1 control ina vCPU's virtual MSR_IA32_VMX_PROCBASED_CTLS2 when
updating secondary controls in response to KVM_SET_CPUID(2), but (a) that
depends on flawed code (KVM shouldn't touch VMX MSRs in response to CPUID
updates) and (b) runs afoul of vmx_restore_control_msr()'s restriction
that the guest value must be a strict subset of the supported host value.
Although no past commit explicitly enabled nested support for WAITPKG,
doing so is safe and functionally correct from an architectural
perspective as no additional KVM support is needed to virtualize TPAUSE,
UMONITOR, and UMWAIT for L2 relative to L1, and KVM already forwards
VM-Exits to L1 as necessary (commit bf653b78f960, "KVM: vmx: Introduce
handle_unexpected_vmexit and handle WAITPKG vmexit").
Note, KVM always keeps the hosts MSR_IA32_UMWAIT_CONTROL resident in
hardware, i.e. always runs both L1 and L2 with the host's power management
settings for TPAUSE and UMWAIT. See commit bf09fb6cba4f ("KVM: VMX: Stop
context switching MSR_IA32_UMWAIT_CONTROL") for more details.
Fixes: e69e72faa3a0 ("KVM: x86: Add support for user wait instructions")
Cc: [email protected]
Reported-by: Aaron Lewis <[email protected]>
Reported-by: Yu Zhang <[email protected]>
Signed-off-by: Sean Christopherson <[email protected]>
---
arch/x86/kvm/vmx/nested.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/arch/x86/kvm/vmx/nested.c b/arch/x86/kvm/vmx/nested.c
index b6f4411b613e..d131375f347a 100644
--- a/arch/x86/kvm/vmx/nested.c
+++ b/arch/x86/kvm/vmx/nested.c
@@ -6873,7 +6873,8 @@ void nested_vmx_setup_ctls_msrs(struct vmcs_config *vmcs_conf, u32 ept_caps)
SECONDARY_EXEC_ENABLE_INVPCID |
SECONDARY_EXEC_RDSEED_EXITING |
SECONDARY_EXEC_XSAVES |
- SECONDARY_EXEC_TSC_SCALING;
+ SECONDARY_EXEC_TSC_SCALING |
+ SECONDARY_EXEC_ENABLE_USR_WAIT_PAUSE;
/*
* We can emulate "VMCS shadowing," even if the hardware
--
2.39.0.rc1.256.g54fd8350bd-goog
When stuffing the allowed secondary execution controls for nested VMX in
response to CPUID updates, don't set the allowed-1 bit for a feature that
isn't supported by KVM, i.e. isn't allowed by the canonical vmcs_config.
WARN if KVM attempts to manipulate a feature that isn't supported. All
features that are currently stuffed are always advertised to L1 for
nested VMX if they are supported in KVM's base configuration, and no
additional features should ever be added to the CPUID-induced stuffing
(updating VMX MSRs in response to CPUID updates is a long-standing KVM
flaw that is slowly being fixed).
Signed-off-by: Sean Christopherson <[email protected]>
---
arch/x86/kvm/vmx/vmx.c | 10 ++++++++++
1 file changed, 10 insertions(+)
diff --git a/arch/x86/kvm/vmx/vmx.c b/arch/x86/kvm/vmx/vmx.c
index fe5615fd8295..13d3f5eb4c32 100644
--- a/arch/x86/kvm/vmx/vmx.c
+++ b/arch/x86/kvm/vmx/vmx.c
@@ -4459,6 +4459,16 @@ vmx_adjust_secondary_exec_control(struct vcpu_vmx *vmx, u32 *exec_control,
* controls for features that are/aren't exposed to the guest.
*/
if (nested) {
+ /*
+ * All features that got grandfathered into KVM's flawed CPUID-
+ * induced manipulation of VMX MSRs are unconditionally exposed
+ * to L1 if the feature is supported by KVM (for nested). I.e.
+ * KVM should never attempt to stuff a feature that isn't
+ * already exposed to L1 for nested virtualization.
+ */
+ if (WARN_ON_ONCE(!(vmcs_config.nested.secondary_ctls_high & control)))
+ enabled = false;
+
if (enabled)
vmx->nested.msrs.secondary_ctls_high |= control;
else
--
2.39.0.rc1.256.g54fd8350bd-goog
On Tue, Dec 13, 2022 at 06:23:03AM +0000, Sean Christopherson wrote:
> Set ENABLE_USR_WAIT_PAUSE in KVM's supported VMX MSR configuration if the
> feature is supported in hardware and enabled in KVM's base, non-nested
> configuration, i.e. expose ENABLE_USR_WAIT_PAUSE to L1 if it's supported.
> This fixes a bug where saving/restoring, i.e. migrating, a vCPU will fail
> if WAITPKG (the associated CPUID feature) is enabled for the vCPU, and
> obviously allows L1 to enable the feature for L2.
>
> KVM already effectively exposes ENABLE_USR_WAIT_PAUSE to L1 by stuffing
> the allowed-1 control ina vCPU's virtual MSR_IA32_VMX_PROCBASED_CTLS2 when
> updating secondary controls in response to KVM_SET_CPUID(2), but (a) that
> depends on flawed code (KVM shouldn't touch VMX MSRs in response to CPUID
> updates) and (b) runs afoul of vmx_restore_control_msr()'s restriction
> that the guest value must be a strict subset of the supported host value.
>
> Although no past commit explicitly enabled nested support for WAITPKG,
> doing so is safe and functionally correct from an architectural
> perspective as no additional KVM support is needed to virtualize TPAUSE,
> UMONITOR, and UMWAIT for L2 relative to L1, and KVM already forwards
> VM-Exits to L1 as necessary (commit bf653b78f960, "KVM: vmx: Introduce
> handle_unexpected_vmexit and handle WAITPKG vmexit").
>
> Note, KVM always keeps the hosts MSR_IA32_UMWAIT_CONTROL resident in
> hardware, i.e. always runs both L1 and L2 with the host's power management
> settings for TPAUSE and UMWAIT. See commit bf09fb6cba4f ("KVM: VMX: Stop
> context switching MSR_IA32_UMWAIT_CONTROL") for more details.
>
> Fixes: e69e72faa3a0 ("KVM: x86: Add support for user wait instructions")
> Cc: [email protected]
> Reported-by: Aaron Lewis <[email protected]>
> Reported-by: Yu Zhang <[email protected]>
> Signed-off-by: Sean Christopherson <[email protected]>
Could you please help add
"Reported-by: Yang, Lixiao <[email protected]>"
She identified the failure of vmx_msrs_test in KVM selftest first. Thanks!
B.R.
Yu
On Mon, Dec 12, 2022 at 10:23 PM Sean Christopherson <[email protected]> wrote:
>
> Set ENABLE_USR_WAIT_PAUSE in KVM's supported VMX MSR configuration if the
> feature is supported in hardware and enabled in KVM's base, non-nested
> configuration, i.e. expose ENABLE_USR_WAIT_PAUSE to L1 if it's supported.
> This fixes a bug where saving/restoring, i.e. migrating, a vCPU will fail
> if WAITPKG (the associated CPUID feature) is enabled for the vCPU, and
> obviously allows L1 to enable the feature for L2.
>
> KVM already effectively exposes ENABLE_USR_WAIT_PAUSE to L1 by stuffing
> the allowed-1 control ina vCPU's virtual MSR_IA32_VMX_PROCBASED_CTLS2 when
> updating secondary controls in response to KVM_SET_CPUID(2), but (a) that
> depends on flawed code (KVM shouldn't touch VMX MSRs in response to CPUID
> updates) and (b) runs afoul of vmx_restore_control_msr()'s restriction
> that the guest value must be a strict subset of the supported host value.
>
> Although no past commit explicitly enabled nested support for WAITPKG,
> doing so is safe and functionally correct from an architectural
> perspective as no additional KVM support is needed to virtualize TPAUSE,
> UMONITOR, and UMWAIT for L2 relative to L1, and KVM already forwards
> VM-Exits to L1 as necessary (commit bf653b78f960, "KVM: vmx: Introduce
> handle_unexpected_vmexit and handle WAITPKG vmexit").
>
> Note, KVM always keeps the hosts MSR_IA32_UMWAIT_CONTROL resident in
> hardware, i.e. always runs both L1 and L2 with the host's power management
> settings for TPAUSE and UMWAIT. See commit bf09fb6cba4f ("KVM: VMX: Stop
> context switching MSR_IA32_UMWAIT_CONTROL") for more details.
>
> Fixes: e69e72faa3a0 ("KVM: x86: Add support for user wait instructions")
> Cc: [email protected]
> Reported-by: Aaron Lewis <[email protected]>
> Reported-by: Yu Zhang <[email protected]>
> Signed-off-by: Sean Christopherson <[email protected]>
Reviewed-by: Jim Mattson <[email protected]>
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/[email protected]
>
> 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/[email protected]
> >
> > 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