[PATCH 0/4] KVM: nVMX: Fixes for VMX capability MSR invariance

[PATCH 0/4] KVM: nVMX: Fixes for VMX capability MSR invariance

[Oliver Upton]

Ultimately, it is the responsibility of userspace to configure an
appropriate MSR value for the CPUID it provides its guest. However,
there are a few bits in VMX capability MSRs where KVM intervenes. The
"load IA32_PERF_GLOBAL_CTRL", "load IA32_BNDCFGS", and "clear
IA32_BNDCFGS" bits in the VMX VM-{Entry,Exit} control capability MSRs
are updated every time userspace sets the guest's CPUID. In so doing,
there is an imposed ordering between ioctls, that userspace must set MSR
values *after* setting the guest's CPUID.

Such ordering requirements should be entirely avoided. This series stops
KVM from modifying VMX VM-{Entry,Exit} control capability MSRs when the
CPUID changes. With this series applied, MSR writes from userspace
before and after KVM_SET_CPUID2 are preserved.

This series cleanly applies to 5.17-rc2. Confirmed the bug with the
included selftest, and also verified the fix. Tested with KVM selftests
on a Skylake box.

Oliver Upton (4):
  KVM: nVMX: Don't change VM-{Entry,Exit} ctrl MSRs on PMU CPUID update
  KVM: nVMX: Don't change VM-{Entry,Exit} ctrl MSRs on MPX CPUID update
  selftests: KVM: Add test for "load IA32_PERF_GLOBAL_CTRL" invariance
  selftests: KVM: Add test case for "{load/clear} IA32_BNDCFGS"
    invariance

 arch/x86/kvm/vmx/nested.c                     |  21 ----
 arch/x86/kvm/vmx/nested.h                     |   1 -
 arch/x86/kvm/vmx/pmu_intel.c                  |   2 -
 arch/x86/kvm/vmx/vmx.c                        |  21 +---
 tools/testing/selftests/kvm/.gitignore        |   1 +
 tools/testing/selftests/kvm/Makefile          |   1 +
 .../selftests/kvm/include/x86_64/vmx.h        |   2 +
 .../kvm/x86_64/vmx_capability_msrs_test.c     | 119 ++++++++++++++++++
 8 files changed, 124 insertions(+), 44 deletions(-)
 create mode 100644 tools/testing/selftests/kvm/x86_64/vmx_capability_msrs_test.c

-- 
2.35.0.rc2.247.g8bbb082509-goog

[PATCH 1/4] KVM: nVMX: Don't change VM-{Entry,Exit} ctrl MSRs on PMU CPUID update

[Oliver Upton]

Ultimately, it is up to userspace to decide what capabilities should be
advertised to the guest, including the VMX capability MSRs. Furthermore,
there shouldn't be any ordering requirements around the KVM_SET_MSR and
KVM_SET_CPUID2 ioctls. However, KVM has not respected the values written
by userspace for IA32_VMX_TRUE_{ENTRY,EXIT}_CTLS. Instead, KVM
updates the value of these MSRs when CPUID is changed. If userspace
advertises a PMU to the guest that supports IA32_PERF_GLOBAL_CTRL
(CPUID.0AH:EAX[7:0] > 1), KVM will adjust the values for
MSR_IA32_VMX_TRUE_{ENTRY,EXIT}_CTLS to unconditionally advertise the
"load IA32_PERF_GLOBAL_CTRL" control bit (or not, if the PMU doesn't
support the MSR).

The end result of these shenanigans is that VMMs that clear the "load
PERF_GLOBAL_CTRL" bits before setting CPUID will see the bits re-enabled
if it provides a supporting vPMU to the guest. Only if the MSR writes
happen after setting CPUID will userspace's intentions be upheld.

Since there are no ordering expectations around these ioctls, fix the
issue by simply not touching the VMX capability MSRs during an update
to guest CPUID. Note that the "load IA32_PERF_GLOBAL_CTRL" bits are
already set by default in the respective VMX capability MSRs,
if supported by hardware.

Fixes: 03a8871add95 ("KVM: nVMX: Expose load IA32_PERF_GLOBAL_CTRL VM-{Entry,Exit} control")
Reported-by: Jim Mattson <jmattson@google.com>
Signed-off-by: Oliver Upton <oupton@google.com>
---
 arch/x86/kvm/vmx/nested.c    | 21 ---------------------
 arch/x86/kvm/vmx/nested.h    |  1 -
 arch/x86/kvm/vmx/pmu_intel.c |  2 --
 3 files changed, 24 deletions(-)

diff --git a/arch/x86/kvm/vmx/nested.c b/arch/x86/kvm/vmx/nested.c
index ba34e94049c7..4ed2409c2bd7 100644
--- a/arch/x86/kvm/vmx/nested.c
+++ b/arch/x86/kvm/vmx/nested.c
@@ -4797,27 +4797,6 @@ int get_vmx_mem_address(struct kvm_vcpu *vcpu, unsigned long exit_qualification,
 	return 0;
 }
 
-void nested_vmx_pmu_entry_exit_ctls_update(struct kvm_vcpu *vcpu)
-{
-	struct vcpu_vmx *vmx;
-
-	if (!nested_vmx_allowed(vcpu))
-		return;
-
-	vmx = to_vmx(vcpu);
-	if (kvm_x86_ops.pmu_ops->is_valid_msr(vcpu, MSR_CORE_PERF_GLOBAL_CTRL)) {
-		vmx->nested.msrs.entry_ctls_high |=
-				VM_ENTRY_LOAD_IA32_PERF_GLOBAL_CTRL;
-		vmx->nested.msrs.exit_ctls_high |=
-				VM_EXIT_LOAD_IA32_PERF_GLOBAL_CTRL;
-	} else {
-		vmx->nested.msrs.entry_ctls_high &=
-				~VM_ENTRY_LOAD_IA32_PERF_GLOBAL_CTRL;
-		vmx->nested.msrs.exit_ctls_high &=
-				~VM_EXIT_LOAD_IA32_PERF_GLOBAL_CTRL;
-	}
-}
-
 static int nested_vmx_get_vmptr(struct kvm_vcpu *vcpu, gpa_t *vmpointer,
 				int *ret)
 {
diff --git a/arch/x86/kvm/vmx/nested.h b/arch/x86/kvm/vmx/nested.h
index b69a80f43b37..14ad756aac46 100644
--- a/arch/x86/kvm/vmx/nested.h
+++ b/arch/x86/kvm/vmx/nested.h
@@ -32,7 +32,6 @@ int vmx_set_vmx_msr(struct kvm_vcpu *vcpu, u32 msr_index, u64 data);
 int vmx_get_vmx_msr(struct nested_vmx_msrs *msrs, u32 msr_index, u64 *pdata);
 int get_vmx_mem_address(struct kvm_vcpu *vcpu, unsigned long exit_qualification,
 			u32 vmx_instruction_info, bool wr, int len, gva_t *ret);
-void nested_vmx_pmu_entry_exit_ctls_update(struct kvm_vcpu *vcpu);
 void nested_mark_vmcs12_pages_dirty(struct kvm_vcpu *vcpu);
 bool nested_vmx_check_io_bitmaps(struct kvm_vcpu *vcpu, unsigned int port,
 				 int size);
diff --git a/arch/x86/kvm/vmx/pmu_intel.c b/arch/x86/kvm/vmx/pmu_intel.c
index 466d18fc0c5d..ad1adbaa7d9e 100644
--- a/arch/x86/kvm/vmx/pmu_intel.c
+++ b/arch/x86/kvm/vmx/pmu_intel.c
@@ -541,8 +541,6 @@ static void intel_pmu_refresh(struct kvm_vcpu *vcpu)
 	bitmap_set(pmu->all_valid_pmc_idx,
 		INTEL_PMC_MAX_GENERIC, pmu->nr_arch_fixed_counters);
 
-	nested_vmx_pmu_entry_exit_ctls_update(vcpu);
-
 	if (intel_pmu_lbr_is_compatible(vcpu))
 		x86_perf_get_lbr(&lbr_desc->records);
 	else
-- 
2.35.0.rc2.247.g8bbb082509-goog

[PATCH 2/4] KVM: nVMX: Don't change VM-{Entry,Exit} ctrl MSRs on MPX CPUID update

[Oliver Upton]

KVM does not respect userspace's configuration of the "{load,clear}
IA32_BNDCFGS" bit in the IA32_VMX_TRUE_{ENTRY,EXIT}_CTLS MSRs, depending
on the order in which it sets the MSR and configures the guest's CPUID.
When the guest's CPUID is set, KVM will expose the "load IA32_BNDCFGS"
bits if MPX is exposed in the guest CPUID. In order to clear the bit,
userspace would need to write the VMX capability MSRs after updating
CPUID.

There are no ordering requirements on these ioctls. Fix the issue by
simply not updating the "{load,clear} IA32_BNDCFGS" bits on CPUID
update. Note that these bits are already exposed by default in the
respective VMX capability MSRs, if supported by hardware.

Fixes: 5f76f6f5ff96 ("KVM: nVMX: Do not expose MPX VMX controls when guest MPX disabled")
Signed-off-by: Oliver Upton <oupton@google.com>
---
 arch/x86/kvm/vmx/vmx.c | 21 +--------------------
 1 file changed, 1 insertion(+), 20 deletions(-)

diff --git a/arch/x86/kvm/vmx/vmx.c b/arch/x86/kvm/vmx/vmx.c
index aca3ae2a02f3..8cf58ba60b01 100644
--- a/arch/x86/kvm/vmx/vmx.c
+++ b/arch/x86/kvm/vmx/vmx.c
@@ -7227,23 +7227,6 @@ static void nested_vmx_cr_fixed1_bits_update(struct kvm_vcpu *vcpu)
 #undef cr4_fixed1_update
 }
 
-static void nested_vmx_entry_exit_ctls_update(struct kvm_vcpu *vcpu)
-{
-	struct vcpu_vmx *vmx = to_vmx(vcpu);
-
-	if (kvm_mpx_supported()) {
-		bool mpx_enabled = guest_cpuid_has(vcpu, X86_FEATURE_MPX);
-
-		if (mpx_enabled) {
-			vmx->nested.msrs.entry_ctls_high |= VM_ENTRY_LOAD_BNDCFGS;
-			vmx->nested.msrs.exit_ctls_high |= VM_EXIT_CLEAR_BNDCFGS;
-		} else {
-			vmx->nested.msrs.entry_ctls_high &= ~VM_ENTRY_LOAD_BNDCFGS;
-			vmx->nested.msrs.exit_ctls_high &= ~VM_EXIT_CLEAR_BNDCFGS;
-		}
-	}
-}
-
 static void update_intel_pt_cfg(struct kvm_vcpu *vcpu)
 {
 	struct vcpu_vmx *vmx = to_vmx(vcpu);
@@ -7335,10 +7318,8 @@ static void vmx_vcpu_after_set_cpuid(struct kvm_vcpu *vcpu)
 			~(FEAT_CTL_VMX_ENABLED_INSIDE_SMX |
 			  FEAT_CTL_VMX_ENABLED_OUTSIDE_SMX);
 
-	if (nested_vmx_allowed(vcpu)) {
+	if (nested_vmx_allowed(vcpu))
 		nested_vmx_cr_fixed1_bits_update(vcpu);
-		nested_vmx_entry_exit_ctls_update(vcpu);
-	}
 
 	if (boot_cpu_has(X86_FEATURE_INTEL_PT) &&
 			guest_cpuid_has(vcpu, X86_FEATURE_INTEL_PT))
-- 
2.35.0.rc2.247.g8bbb082509-goog

[PATCH 3/4] selftests: KVM: Add test for "load IA32_PERF_GLOBAL_CTRL" invariance

[Oliver Upton]

Assert that clearing the "load IA32_PERF_GLOBAL_CTRL" VM-{Entry,Exit}
capability bits is preserved across KVM_SET_CPUID2.

Signed-off-by: Oliver Upton <oupton@google.com>
---
 tools/testing/selftests/kvm/.gitignore        |  1 +
 tools/testing/selftests/kvm/Makefile          |  1 +
 .../kvm/x86_64/vmx_capability_msrs_test.c     | 82 +++++++++++++++++++
 3 files changed, 84 insertions(+)
 create mode 100644 tools/testing/selftests/kvm/x86_64/vmx_capability_msrs_test.c

diff --git a/tools/testing/selftests/kvm/.gitignore b/tools/testing/selftests/kvm/.gitignore
index dce7de7755e6..d1e6757aed2f 100644
--- a/tools/testing/selftests/kvm/.gitignore
+++ b/tools/testing/selftests/kvm/.gitignore
@@ -36,6 +36,7 @@
 /x86_64/userspace_io_test
 /x86_64/userspace_msr_exit_test
 /x86_64/vmx_apic_access_test
+/x86_64/vmx_capability_msrs_test
 /x86_64/vmx_close_while_nested_test
 /x86_64/vmx_dirty_log_test
 /x86_64/vmx_exception_with_invalid_guest_state
diff --git a/tools/testing/selftests/kvm/Makefile b/tools/testing/selftests/kvm/Makefile
index 0e4926bc9a58..083c437a852f 100644
--- a/tools/testing/selftests/kvm/Makefile
+++ b/tools/testing/selftests/kvm/Makefile
@@ -68,6 +68,7 @@ TEST_GEN_PROGS_x86_64 += x86_64/sync_regs_test
 TEST_GEN_PROGS_x86_64 += x86_64/userspace_io_test
 TEST_GEN_PROGS_x86_64 += x86_64/userspace_msr_exit_test
 TEST_GEN_PROGS_x86_64 += x86_64/vmx_apic_access_test
+TEST_GEN_PROGS_x86_64 += x86_64/vmx_capability_msrs_test
 TEST_GEN_PROGS_x86_64 += x86_64/vmx_close_while_nested_test
 TEST_GEN_PROGS_x86_64 += x86_64/vmx_dirty_log_test
 TEST_GEN_PROGS_x86_64 += x86_64/vmx_exception_with_invalid_guest_state
diff --git a/tools/testing/selftests/kvm/x86_64/vmx_capability_msrs_test.c b/tools/testing/selftests/kvm/x86_64/vmx_capability_msrs_test.c
new file mode 100644
index 000000000000..8a1a545e658b
--- /dev/null
+++ b/tools/testing/selftests/kvm/x86_64/vmx_capability_msrs_test.c
@@ -0,0 +1,82 @@
+// SPDX-License-Identifier: GPL-2.0-only
+/*
+ * VMX capability MSRs test
+ *
+ * Copyright (C) 2022 Google LLC
+ *
+ * Regression tests to check that updates to guest CPUID do not affect the
+ * values of VMX capability MSRs.
+ */
+
+#include "kvm_util.h"
+#include "vmx.h"
+
+#define VCPU_ID 0
+
+static void get_vmx_capability_msr(struct kvm_vm *vm, uint32_t msr_index,
+				   uint32_t *low, uint32_t *high)
+{
+	uint64_t val;
+
+	val = vcpu_get_msr(vm, VCPU_ID, msr_index);
+	*low = val;
+	*high = val >> 32;
+}
+
+static void set_vmx_capability_msr(struct kvm_vm *vm, uint32_t msr_index,
+				   uint32_t low, uint32_t high)
+{
+	uint64_t val = (((uint64_t) high) << 32) | low;
+
+	vcpu_set_msr(vm, VCPU_ID, msr_index, val);
+}
+
+/*
+ * Test to assert that clearing the "load IA32_PERF_GLOBAL_CTRL" VM-{Entry,Exit}
+ * control capability bits is preserved across a KVM_SET_CPUID2.
+ */
+static void load_perf_global_ctrl_test(struct kvm_vm *vm)
+{
+	uint32_t entry_low, entry_high, exit_low, exit_high;
+	struct kvm_cpuid2 *cpuid;
+
+	get_vmx_capability_msr(vm, MSR_IA32_VMX_TRUE_ENTRY_CTLS, &entry_low, &entry_high);
+	get_vmx_capability_msr(vm, MSR_IA32_VMX_TRUE_EXIT_CTLS, &exit_low, &exit_high);
+
+	if (!(entry_high & VM_ENTRY_LOAD_IA32_PERF_GLOBAL_CTRL) ||
+	    !(exit_high & VM_EXIT_LOAD_IA32_PERF_GLOBAL_CTRL)) {
+		print_skip("\"load IA32_PERF_GLOBAL_CTRL\" VM-{Entry,Exit} control not supported");
+		return;
+	}
+
+	entry_high &= ~VM_ENTRY_LOAD_IA32_PERF_GLOBAL_CTRL;
+	exit_high &= ~VM_EXIT_LOAD_IA32_PERF_GLOBAL_CTRL;
+
+	set_vmx_capability_msr(vm, MSR_IA32_VMX_TRUE_ENTRY_CTLS, entry_low, entry_high);
+	set_vmx_capability_msr(vm, MSR_IA32_VMX_TRUE_EXIT_CTLS, exit_low, exit_high);
+
+	cpuid = kvm_get_supported_cpuid();
+	vcpu_set_cpuid(vm, VCPU_ID, cpuid);
+
+	get_vmx_capability_msr(vm, MSR_IA32_VMX_TRUE_ENTRY_CTLS, &entry_low, &entry_high);
+	get_vmx_capability_msr(vm, MSR_IA32_VMX_TRUE_EXIT_CTLS, &exit_low, &exit_high);
+
+	TEST_ASSERT(!(entry_high & VM_ENTRY_LOAD_IA32_PERF_GLOBAL_CTRL),
+		    "\"load IA32_PERF_GLOBAL_CTRL\" VM-Entry bit set");
+	TEST_ASSERT(!(exit_high & VM_EXIT_LOAD_IA32_PERF_GLOBAL_CTRL),
+		    "\"load IA32_PERF_GLOBAL_CTRL\" VM-Exit bit set");
+}
+
+int main(void)
+{
+	struct kvm_vm *vm;
+
+	nested_vmx_check_supported();
+
+	/* No need to run a guest for these tests */
+	vm = vm_create_default(VCPU_ID, 0, NULL);
+
+	load_perf_global_ctrl_test(vm);
+
+	kvm_vm_free(vm);
+}
-- 
2.35.0.rc2.247.g8bbb082509-goog

[PATCH 4/4] selftests: KVM: Add test case for "{load/clear} IA32_BNDCFGS" invariance

[Oliver Upton]

Assert that clearing the "{load/clear IA32_BNDCFGS" bits is preserved
across KVM_SET_CPUID2.

Signed-off-by: Oliver Upton <oupton@google.com>
---
 .../selftests/kvm/include/x86_64/vmx.h        |  2 +
 .../kvm/x86_64/vmx_capability_msrs_test.c     | 37 +++++++++++++++++++
 2 files changed, 39 insertions(+)

diff --git a/tools/testing/selftests/kvm/include/x86_64/vmx.h b/tools/testing/selftests/kvm/include/x86_64/vmx.h
index 583ceb0d1457..811c66d9be74 100644
--- a/tools/testing/selftests/kvm/include/x86_64/vmx.h
+++ b/tools/testing/selftests/kvm/include/x86_64/vmx.h
@@ -80,6 +80,7 @@
 #define VM_EXIT_SAVE_IA32_EFER			0x00100000
 #define VM_EXIT_LOAD_IA32_EFER			0x00200000
 #define VM_EXIT_SAVE_VMX_PREEMPTION_TIMER	0x00400000
+#define VM_EXIT_CLEAR_BNDCFGS			0x00800000
 
 #define VM_EXIT_ALWAYSON_WITHOUT_TRUE_MSR	0x00036dff
 
@@ -90,6 +91,7 @@
 #define VM_ENTRY_LOAD_IA32_PERF_GLOBAL_CTRL	0x00002000
 #define VM_ENTRY_LOAD_IA32_PAT			0x00004000
 #define VM_ENTRY_LOAD_IA32_EFER			0x00008000
+#define VM_ENTRY_LOAD_BNDCFGS			0x00010000
 
 #define VM_ENTRY_ALWAYSON_WITHOUT_TRUE_MSR	0x000011ff
 
diff --git a/tools/testing/selftests/kvm/x86_64/vmx_capability_msrs_test.c b/tools/testing/selftests/kvm/x86_64/vmx_capability_msrs_test.c
index 8a1a545e658b..a0851b1224aa 100644
--- a/tools/testing/selftests/kvm/x86_64/vmx_capability_msrs_test.c
+++ b/tools/testing/selftests/kvm/x86_64/vmx_capability_msrs_test.c
@@ -67,6 +67,42 @@ static void load_perf_global_ctrl_test(struct kvm_vm *vm)
 		    "\"load IA32_PERF_GLOBAL_CTRL\" VM-Exit bit set");
 }
 
+/*
+ * Test to assert that clearing the "load IA32_BNDCFGS" and "clear IA32_BNDCFGS"
+ * control capability bits is preserved across a KVM_SET_CPUID2.
+ */
+static void bndcfgs_ctrl_test(struct kvm_vm *vm)
+{
+	uint32_t entry_low, entry_high, exit_low, exit_high;
+	struct kvm_cpuid2 *cpuid;
+
+	get_vmx_capability_msr(vm, MSR_IA32_VMX_TRUE_ENTRY_CTLS, &entry_low, &entry_high);
+	get_vmx_capability_msr(vm, MSR_IA32_VMX_TRUE_EXIT_CTLS, &exit_low, &exit_high);
+
+	if (!(entry_high & VM_ENTRY_LOAD_BNDCFGS) || !(exit_high & VM_EXIT_CLEAR_BNDCFGS)) {
+		print_skip("\"{load,clear} IA32_BNDCFGS\" controls not supported");
+		return;
+	}
+
+	entry_high &= ~VM_ENTRY_LOAD_BNDCFGS;
+	exit_high &= ~VM_EXIT_CLEAR_BNDCFGS;
+
+	set_vmx_capability_msr(vm, MSR_IA32_VMX_TRUE_ENTRY_CTLS, entry_low, entry_high);
+	set_vmx_capability_msr(vm, MSR_IA32_VMX_TRUE_EXIT_CTLS, exit_low, exit_high);
+
+	cpuid = kvm_get_supported_cpuid();
+	vcpu_set_cpuid(vm, VCPU_ID, cpuid);
+
+	get_vmx_capability_msr(vm, MSR_IA32_VMX_TRUE_ENTRY_CTLS, &entry_low, &entry_high);
+	get_vmx_capability_msr(vm, MSR_IA32_VMX_TRUE_EXIT_CTLS, &exit_low, &exit_high);
+
+	TEST_ASSERT(!(entry_high & VM_ENTRY_LOAD_BNDCFGS),
+		    "\"load IA32_BNDCFGS\" VM-Entry bit set");
+	TEST_ASSERT(!(exit_high & VM_EXIT_CLEAR_BNDCFGS),
+		    "\"clear IA32_BNDCFGS\" VM-Exit bit set");
+}
+
+
 int main(void)
 {
 	struct kvm_vm *vm;
@@ -77,6 +113,7 @@ int main(void)
 	vm = vm_create_default(VCPU_ID, 0, NULL);
 
 	load_perf_global_ctrl_test(vm);
+	bndcfgs_ctrl_test(vm);
 
 	kvm_vm_free(vm);
 }
-- 
2.35.0.rc2.247.g8bbb082509-goog

Re: [PATCH 0/4] KVM: nVMX: Fixes for VMX capability MSR invariance

[Jim Mattson]

On Wed, Feb 2, 2022 at 3:04 PM Oliver Upton <oupton@google.com> wrote:
>
> Ultimately, it is the responsibility of userspace to configure an
> appropriate MSR value for the CPUID it provides its guest. However,
> there are a few bits in VMX capability MSRs where KVM intervenes. The
> "load IA32_PERF_GLOBAL_CTRL", "load IA32_BNDCFGS", and "clear
> IA32_BNDCFGS" bits in the VMX VM-{Entry,Exit} control capability MSRs
> are updated every time userspace sets the guest's CPUID. In so doing,
> there is an imposed ordering between ioctls, that userspace must set MSR
> values *after* setting the guest's CPUID.

 Do you mean *before*?

Re: [PATCH 0/4] KVM: nVMX: Fixes for VMX capability MSR invariance

[Sean Christopherson]

On Wed, Feb 02, 2022, Jim Mattson wrote:
> On Wed, Feb 2, 2022 at 3:04 PM Oliver Upton <oupton@google.com> wrote:
> >
> > Ultimately, it is the responsibility of userspace to configure an
> > appropriate MSR value for the CPUID it provides its guest. However,
> > there are a few bits in VMX capability MSRs where KVM intervenes. The
> > "load IA32_PERF_GLOBAL_CTRL", "load IA32_BNDCFGS", and "clear
> > IA32_BNDCFGS" bits in the VMX VM-{Entry,Exit} control capability MSRs
> > are updated every time userspace sets the guest's CPUID. In so doing,
> > there is an imposed ordering between ioctls, that userspace must set MSR
> > values *after* setting the guest's CPUID.
> 
>  Do you mean *before*?

No, after, otherwise the CPUID updates will override the MSR updates.

MSR_IA32_FEAT_CTL has this same issue.  But that mess also highlights an issue
with this series: if userspace relies on KVM to do the updates, it will break the
existing ABI, e.g. I'm pretty sure older versions of QEMU rely on KVM to adjust
the MSRs.

I agree that KVM should keep its nose out of this stuff, especially since most
VMX controls are technically not architecturally tied to CPUID.  But we probably
need an opt-in from userspace to stop mucking with the MSRs.

Re: [PATCH 0/4] KVM: nVMX: Fixes for VMX capability MSR invariance

[Jim Mattson]

On Wed, Feb 2, 2022 at 4:33 PM Sean Christopherson <seanjc@google.com> wrote:
>
> On Wed, Feb 02, 2022, Jim Mattson wrote:
> > On Wed, Feb 2, 2022 at 3:04 PM Oliver Upton <oupton@google.com> wrote:
> > >
> > > Ultimately, it is the responsibility of userspace to configure an
> > > appropriate MSR value for the CPUID it provides its guest. However,
> > > there are a few bits in VMX capability MSRs where KVM intervenes. The
> > > "load IA32_PERF_GLOBAL_CTRL", "load IA32_BNDCFGS", and "clear
> > > IA32_BNDCFGS" bits in the VMX VM-{Entry,Exit} control capability MSRs
> > > are updated every time userspace sets the guest's CPUID. In so doing,
> > > there is an imposed ordering between ioctls, that userspace must set MSR
> > > values *after* setting the guest's CPUID.
> >
> >  Do you mean *before*?
>
> No, after, otherwise the CPUID updates will override the MSR updates.

Wasn't that the intention behind this code in the first place (to
override KVM_SET_MSR based on CPUID bits)? If not, what was the
intention behind this code?

Re: [PATCH 0/4] KVM: nVMX: Fixes for VMX capability MSR invariance

[Oliver Upton]

On Wed, Feb 2, 2022 at 4:38 PM Jim Mattson <jmattson@google.com> wrote:
>
> On Wed, Feb 2, 2022 at 4:33 PM Sean Christopherson <seanjc@google.com> wrote:
> >
> > On Wed, Feb 02, 2022, Jim Mattson wrote:
> > > On Wed, Feb 2, 2022 at 3:04 PM Oliver Upton <oupton@google.com> wrote:
> > > >
> > > > Ultimately, it is the responsibility of userspace to configure an
> > > > appropriate MSR value for the CPUID it provides its guest. However,
> > > > there are a few bits in VMX capability MSRs where KVM intervenes. The
> > > > "load IA32_PERF_GLOBAL_CTRL", "load IA32_BNDCFGS", and "clear
> > > > IA32_BNDCFGS" bits in the VMX VM-{Entry,Exit} control capability MSRs
> > > > are updated every time userspace sets the guest's CPUID. In so doing,
> > > > there is an imposed ordering between ioctls, that userspace must set MSR
> > > > values *after* setting the guest's CPUID.
> > >
> > >  Do you mean *before*?
> >
> > No, after, otherwise the CPUID updates will override the MSR updates.
>
> Wasn't that the intention behind this code in the first place (to
> override KVM_SET_MSR based on CPUID bits)? If not, what was the
> intention behind this code?

Suppose a VMM desperately wants to hide the "load
IA32_PERF_GLOBAL_CTRL" bits, in spite of providing a supporting vPMU.
The only way to do so at the moment is to write the control MSR after
the CPUID write.

Re: [PATCH 0/4] KVM: nVMX: Fixes for VMX capability MSR invariance

[Sean Christopherson]

On Wed, Feb 02, 2022, Jim Mattson wrote:
> On Wed, Feb 2, 2022 at 4:33 PM Sean Christopherson <seanjc@google.com> wrote:
> >
> > On Wed, Feb 02, 2022, Jim Mattson wrote:
> > > On Wed, Feb 2, 2022 at 3:04 PM Oliver Upton <oupton@google.com> wrote:
> > > >
> > > > Ultimately, it is the responsibility of userspace to configure an
> > > > appropriate MSR value for the CPUID it provides its guest. However,
> > > > there are a few bits in VMX capability MSRs where KVM intervenes. The
> > > > "load IA32_PERF_GLOBAL_CTRL", "load IA32_BNDCFGS", and "clear
> > > > IA32_BNDCFGS" bits in the VMX VM-{Entry,Exit} control capability MSRs
> > > > are updated every time userspace sets the guest's CPUID. In so doing,
> > > > there is an imposed ordering between ioctls, that userspace must set MSR
> > > > values *after* setting the guest's CPUID.
> > >
> > >  Do you mean *before*?
> >
> > No, after, otherwise the CPUID updates will override the MSR updates.
> 
> Wasn't that the intention behind this code in the first place (to
> override KVM_SET_MSR based on CPUID bits)? If not, what was the
> intention behind this code?

The MPX side is from commit 5f76f6f5ff96 ("KVM: nVMX: Do not expose MPX VMX controls
when guest MPX disabled"), which was a miguided "fix" to workaround an L1 KVM bug in L0.
And also to workaround an L0 userspace bug (failure to set VMX MSRs).

The PMU bug looks to be a similar snafu, it appears to workaround a userspace bug
(again, failure to set VMX MSRs) in KVM.

But once userspace started taking ownership of VMX MSRs, KVM's hack-a-fixes just got
in the way :-/

Re: [PATCH 0/4] KVM: nVMX: Fixes for VMX capability MSR invariance

[Oliver Upton]

On Wed, Feb 2, 2022 at 4:33 PM Sean Christopherson <seanjc@google.com> wrote:
>
> On Wed, Feb 02, 2022, Jim Mattson wrote:
> > On Wed, Feb 2, 2022 at 3:04 PM Oliver Upton <oupton@google.com> wrote:
> > >
> > > Ultimately, it is the responsibility of userspace to configure an
> > > appropriate MSR value for the CPUID it provides its guest. However,
> > > there are a few bits in VMX capability MSRs where KVM intervenes. The
> > > "load IA32_PERF_GLOBAL_CTRL", "load IA32_BNDCFGS", and "clear
> > > IA32_BNDCFGS" bits in the VMX VM-{Entry,Exit} control capability MSRs
> > > are updated every time userspace sets the guest's CPUID. In so doing,
> > > there is an imposed ordering between ioctls, that userspace must set MSR
> > > values *after* setting the guest's CPUID.
> >
> >  Do you mean *before*?
>
> No, after, otherwise the CPUID updates will override the MSR updates.
>
> MSR_IA32_FEAT_CTL has this same issue.  But that mess also highlights an issue
> with this series: if userspace relies on KVM to do the updates, it will break the
> existing ABI, e.g. I'm pretty sure older versions of QEMU rely on KVM to adjust
> the MSRs.

I realize I failed to add a note about exactly this in the cover
letter. It seems, based on the commit 5f76f6f5ff96 ("KVM: nVMX: Do not
expose MPX VMX controls when guest MPX disabled") we opted to handle
the VMX capability MSR in-kernel rather than expecting userspace to
pick a sane value that matches the set CPUID. So what really has
become ABI here? It seems as though one could broadly state that KVM
owns VMX VM-{Entry,Exit} control MSRs without opt-in, or narrowly
assert that only the bits in this series are in fact ABI.

Regardless, since we must uphold this misbehavior as ABI, we have a
regression since KVM doesn't override the MSR write if it comes after
the CPUID write.

> I agree that KVM should keep its nose out of this stuff, especially since most
> VMX controls are technically not architecturally tied to CPUID.  But we probably
> need an opt-in from userspace to stop mucking with the MSRs.

Bleh, I wanted to avoid the age-old problem of naming, but alas...

--
Thanks,
Oliver

Re: [PATCH 0/4] KVM: nVMX: Fixes for VMX capability MSR invariance

[Sean Christopherson]

On Wed, Feb 02, 2022, Oliver Upton wrote:
> On Wed, Feb 2, 2022 at 4:33 PM Sean Christopherson <seanjc@google.com> wrote:
> > MSR_IA32_FEAT_CTL has this same issue.  But that mess also highlights an issue
> > with this series: if userspace relies on KVM to do the updates, it will break the
> > existing ABI, e.g. I'm pretty sure older versions of QEMU rely on KVM to adjust
> > the MSRs.
> 
> I realize I failed to add a note about exactly this in the cover
> letter. It seems, based on the commit 5f76f6f5ff96 ("KVM: nVMX: Do not
> expose MPX VMX controls when guest MPX disabled") we opted to handle
> the VMX capability MSR in-kernel rather than expecting userspace to
> pick a sane value that matches the set CPUID. So what really has
> become ABI here? It seems as though one could broadly state that KVM
> owns VMX VM-{Entry,Exit} control MSRs without opt-in, or narrowly
> assert that only the bits in this series are in fact ABI.

I don't know Paolo's position, but personally I feel quite strongly that KVM should
not manipulate the guest vCPU model.  KVM should reject changes that put the kernel
at risk, but otherwise userspace should have full control.

> Regardless, since we must uphold this misbehavior as ABI, we have a
> regression since KVM doesn't override the MSR write if it comes after
> the CPUID write.
> 
> > I agree that KVM should keep its nose out of this stuff, especially since most
> > VMX controls are technically not architecturally tied to CPUID.  But we probably
> > need an opt-in from userspace to stop mucking with the MSRs.
> 
> Bleh, I wanted to avoid the age-old problem of naming, but alas...

I think a single quirk would suffice, e.g. KVM_X86_QUIRK_KVM_DOESNT_LIKE_TO_SHARE.

Re: [PATCH 0/4] KVM: nVMX: Fixes for VMX capability MSR invariance

[Oliver Upton]

On Wed, Feb 2, 2022 at 4:55 PM Sean Christopherson <seanjc@google.com> wrote:
>
> On Wed, Feb 02, 2022, Oliver Upton wrote:
> > On Wed, Feb 2, 2022 at 4:33 PM Sean Christopherson <seanjc@google.com> wrote:
> > > MSR_IA32_FEAT_CTL has this same issue.  But that mess also highlights an issue
> > > with this series: if userspace relies on KVM to do the updates, it will break the
> > > existing ABI, e.g. I'm pretty sure older versions of QEMU rely on KVM to adjust
> > > the MSRs.
> >
> > I realize I failed to add a note about exactly this in the cover
> > letter. It seems, based on the commit 5f76f6f5ff96 ("KVM: nVMX: Do not
> > expose MPX VMX controls when guest MPX disabled") we opted to handle
> > the VMX capability MSR in-kernel rather than expecting userspace to
> > pick a sane value that matches the set CPUID. So what really has
> > become ABI here? It seems as though one could broadly state that KVM
> > owns VMX VM-{Entry,Exit} control MSRs without opt-in, or narrowly
> > assert that only the bits in this series are in fact ABI.
>
> I don't know Paolo's position, but personally I feel quite strongly that KVM should
> not manipulate the guest vCPU model.  KVM should reject changes that put the kernel
> at risk, but otherwise userspace should have full control.

Ditto, just want to make sure there's agreement that such a quirk only
applies to these bits and not the whole pile.

Re: [PATCH 0/4] KVM: nVMX: Fixes for VMX capability MSR invariance

[Jim Mattson]

On Wed, Feb 2, 2022 at 4:55 PM Sean Christopherson <seanjc@google.com> wrote:
>
> On Wed, Feb 02, 2022, Oliver Upton wrote:
> > On Wed, Feb 2, 2022 at 4:33 PM Sean Christopherson <seanjc@google.com> wrote:
> > > MSR_IA32_FEAT_CTL has this same issue.  But that mess also highlights an issue
> > > with this series: if userspace relies on KVM to do the updates, it will break the
> > > existing ABI, e.g. I'm pretty sure older versions of QEMU rely on KVM to adjust
> > > the MSRs.
> >
> > I realize I failed to add a note about exactly this in the cover
> > letter. It seems, based on the commit 5f76f6f5ff96 ("KVM: nVMX: Do not
> > expose MPX VMX controls when guest MPX disabled") we opted to handle
> > the VMX capability MSR in-kernel rather than expecting userspace to
> > pick a sane value that matches the set CPUID. So what really has
> > become ABI here? It seems as though one could broadly state that KVM
> > owns VMX VM-{Entry,Exit} control MSRs without opt-in, or narrowly
> > assert that only the bits in this series are in fact ABI.
>
> I don't know Paolo's position, but personally I feel quite strongly that KVM should
> not manipulate the guest vCPU model.  KVM should reject changes that put the kernel
> at risk, but otherwise userspace should have full control.

I agree wholeheartedly. Userspace should not have to do KVM_GET_CPUID2
after KVM_SET_CPUID2 to find out what the guest's actual CPUID table
looks like. However, today, it does.

Similarly, for read-only MSRs, userspace should be able to assume that
KVM_GET_MSRS will always return the same value that was passed to
KVM_SET_MSRS. However, that is not the case today, either.