All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH 1/3] KVM: SVM: Check EFER.SVME before updating MSR_VM_CR on guest writes
@ 2026-10-29 20:28 Jim Mattson
  2026-10-29 20:28 ` [PATCH 2/3] KVM: selftests: Rename svm_nested_efer_test to svm_msrs_test Jim Mattson
                   ` (2 more replies)
  0 siblings, 3 replies; 8+ messages in thread
From: Jim Mattson @ 2026-10-29 20:28 UTC (permalink / raw)
  To: Sean Christopherson, Paolo Bonzini
  Cc: Shuah Khan, kvm, linux-kselftest, linux-kernel, Yosry Ahmed,
	Jim Mattson

Per the AMD APM, setting VM_CR.SVMDIS while EFER.SVME is set raises a #GP.

In svm_set_vm_cr(), check whether EFER.SVME is set and the value being
written sets SVMDIS *before* modifying svm->nested.vm_cr_msr. (Host-
initiated writes are allowed to set any valid MSR_VM_CR bits.)

Fixes: 4a810181c8bc ("KVM: SVM: Implement emulation of vm_cr msr")
Signed-off-by: Jim Mattson <jmattson@google.com>
---
 arch/x86/kvm/svm/svm.c | 22 +++++++++++++---------
 1 file changed, 13 insertions(+), 9 deletions(-)

diff --git a/arch/x86/kvm/svm/svm.c b/arch/x86/kvm/svm/svm.c
index 136069dfcd44..3a0a507c94b2 100644
--- a/arch/x86/kvm/svm/svm.c
+++ b/arch/x86/kvm/svm/svm.c
@@ -3015,14 +3015,24 @@ static int svm_complete_emulated_msr(struct kvm_vcpu *vcpu, int err)
 	return 1;
 }
 
-static int svm_set_vm_cr(struct kvm_vcpu *vcpu, u64 data)
+static int svm_set_vm_cr(struct kvm_vcpu *vcpu, struct msr_data *msr)
 {
 	struct vcpu_svm *svm = to_svm(vcpu);
-	int svm_dis, chg_mask;
+	u64 data = msr->data;
+	int chg_mask;
 
 	if (data & ~SVM_VM_CR_VALID_MASK)
 		return 1;
 
+	if (msr->host_initiated) {
+		svm->nested.vm_cr_msr = data;
+		return 0;
+	}
+
+	/* check for svm_disable while efer.svme is set */
+	if ((data & SVM_VM_CR_SVM_DIS_MASK) && (vcpu->arch.efer & EFER_SVME))
+		return 1;
+
 	chg_mask = SVM_VM_CR_VALID_MASK;
 
 	if (svm->nested.vm_cr_msr & SVM_VM_CR_SVM_DIS_MASK)
@@ -3031,12 +3041,6 @@ static int svm_set_vm_cr(struct kvm_vcpu *vcpu, u64 data)
 	svm->nested.vm_cr_msr &= ~chg_mask;
 	svm->nested.vm_cr_msr |= (data & chg_mask);
 
-	svm_dis = svm->nested.vm_cr_msr & SVM_VM_CR_SVM_DIS_MASK;
-
-	/* check for svm_disable while efer.svme is set */
-	if (svm_dis && (vcpu->arch.efer & EFER_SVME))
-		return 1;
-
 	return 0;
 }
 
@@ -3262,7 +3266,7 @@ static int svm_set_msr(struct kvm_vcpu *vcpu, struct msr_data *msr)
 		svm->nested.hsave_msr = data & PAGE_MASK;
 		break;
 	case MSR_VM_CR:
-		return svm_set_vm_cr(vcpu, data);
+		return svm_set_vm_cr(vcpu, msr);
 	case MSR_VM_IGNNE:
 		kvm_pr_unimpl_wrmsr(vcpu, ecx, data);
 		break;
-- 
2.56.0.317.g454f8302e1-goog


^ permalink raw reply related	[flat|nested] 8+ messages in thread

* [PATCH 2/3] KVM: selftests: Rename svm_nested_efer_test to svm_msrs_test
  2026-10-29 20:28 [PATCH 1/3] KVM: SVM: Check EFER.SVME before updating MSR_VM_CR on guest writes Jim Mattson
@ 2026-10-29 20:28 ` Jim Mattson
  2026-10-29 20:28 ` [PATCH 3/3] KVM: selftests: Add MSR_VM_CR tests " Jim Mattson
  2026-10-29 21:19 ` [PATCH 1/3] KVM: SVM: Check EFER.SVME before updating MSR_VM_CR on guest writes Sean Christopherson
  2 siblings, 0 replies; 8+ messages in thread
From: Jim Mattson @ 2026-10-29 20:28 UTC (permalink / raw)
  To: Sean Christopherson, Paolo Bonzini
  Cc: Shuah Khan, kvm, linux-kselftest, linux-kernel, Yosry Ahmed,
	Jim Mattson

Rename svm_nested_efer_test.c to svm_msrs_test.c (matching
vmx_msrs_test.c), in preparation for testing other SVM MSRs.

No functional change intended.

Signed-off-by: Jim Mattson <jmattson@google.com>
---
 tools/testing/selftests/kvm/Makefile.kvm                        | 2 +-
 .../kvm/x86/{svm_nested_efer_test.c => svm_msrs_test.c}         | 0
 2 files changed, 1 insertion(+), 1 deletion(-)
 rename tools/testing/selftests/kvm/x86/{svm_nested_efer_test.c => svm_msrs_test.c} (100%)

diff --git a/tools/testing/selftests/kvm/Makefile.kvm b/tools/testing/selftests/kvm/Makefile.kvm
index 888a7530a280..d1ddce584efe 100644
--- a/tools/testing/selftests/kvm/Makefile.kvm
+++ b/tools/testing/selftests/kvm/Makefile.kvm
@@ -122,7 +122,7 @@ TEST_GEN_PROGS_x86 += x86/state_test
 TEST_GEN_PROGS_x86 += x86/vmx_preemption_timer_test
 TEST_GEN_PROGS_x86 += x86/svm_vmcall_test
 TEST_GEN_PROGS_x86 += x86/svm_int_ctl_test
-TEST_GEN_PROGS_x86 += x86/svm_nested_efer_test
+TEST_GEN_PROGS_x86 += x86/svm_msrs_test
 TEST_GEN_PROGS_x86 += x86/svm_nested_shutdown_test
 TEST_GEN_PROGS_x86 += x86/svm_nested_soft_inject_test
 TEST_GEN_PROGS_x86 += x86/svm_nested_vmcb12_gpa
diff --git a/tools/testing/selftests/kvm/x86/svm_nested_efer_test.c b/tools/testing/selftests/kvm/x86/svm_msrs_test.c
similarity index 100%
rename from tools/testing/selftests/kvm/x86/svm_nested_efer_test.c
rename to tools/testing/selftests/kvm/x86/svm_msrs_test.c
-- 
2.56.0.317.g454f8302e1-goog


^ permalink raw reply related	[flat|nested] 8+ messages in thread

* [PATCH 3/3] KVM: selftests: Add MSR_VM_CR tests to svm_msrs_test
  2026-10-29 20:28 [PATCH 1/3] KVM: SVM: Check EFER.SVME before updating MSR_VM_CR on guest writes Jim Mattson
  2026-10-29 20:28 ` [PATCH 2/3] KVM: selftests: Rename svm_nested_efer_test to svm_msrs_test Jim Mattson
@ 2026-10-29 20:28 ` Jim Mattson
  2026-10-29 21:19 ` [PATCH 1/3] KVM: SVM: Check EFER.SVME before updating MSR_VM_CR on guest writes Sean Christopherson
  2 siblings, 0 replies; 8+ messages in thread
From: Jim Mattson @ 2026-10-29 20:28 UTC (permalink / raw)
  To: Sean Christopherson, Paolo Bonzini
  Cc: Shuah Khan, kvm, linux-kselftest, linux-kernel, Yosry Ahmed,
	Jim Mattson

Add sub-tests to svm_msrs_test to verify KVM's emulation of guest and
host-initiated writes to MSR_VM_CR:

  - Writing reserved bits 63:5 faults with #GP(0) (or fails KVM_SET_MSRS)
    without modifying MSR_VM_CR.
  - Guest WRMSR setting SVMDIS while EFER.SVME=1 faults with #GP(0)
    without modifying MSR_VM_CR.
  - Guest WRMSR setting SVMDIS while EFER.SVME=0 succeeds, after which
    guest writes to LOCK and SVMDIS are silently ignored while bits 2:0
    remain writable.
  - Host-initiated writes via KVM_SET_MSRS can set any valid MSR_VM_CR
    bit regardless of EFER.SVME and can clear SVMDIS and LOCK.

Signed-off-by: Jim Mattson <jmattson@google.com>
---
 .../testing/selftests/kvm/x86/svm_msrs_test.c | 167 ++++++++++++++++--
 1 file changed, 153 insertions(+), 14 deletions(-)

diff --git a/tools/testing/selftests/kvm/x86/svm_msrs_test.c b/tools/testing/selftests/kvm/x86/svm_msrs_test.c
index fa0d76a9c390..e1f386ee0e7e 100644
--- a/tools/testing/selftests/kvm/x86/svm_msrs_test.c
+++ b/tools/testing/selftests/kvm/x86/svm_msrs_test.c
@@ -1,6 +1,6 @@
 // SPDX-License-Identifier: GPL-2.0-only
 /*
- * Tests for KVM's handling of EFER bits whose behavior is tied to nested SVM.
+ * Tests for KVM's handling of SVM MSRs (EFER and VM_CR).
  *
  * Copyright (C) 2026, Google LLC.
  */
@@ -33,6 +33,74 @@ static void l1_clear_efer_svme(struct svm_test_data *svm)
 	GUEST_ASSERT(0);
 }
 
+static void guest_wrmsr_vm_cr(u64 val, u64 expected_val, bool expect_gp)
+{
+	u8 expected_vec = expect_gp ? GP_VECTOR : 0;
+	u64 actual_val;
+	u8 vec;
+
+	vec = wrmsr_safe(MSR_VM_CR, val);
+	__GUEST_ASSERT(vec == expected_vec,
+		       "Wanted %s on WRMSR(MSR_VM_CR, 0x%lx), got %s",
+		       ex_str(expected_vec), val, ex_str(vec));
+
+	actual_val = rdmsr(MSR_VM_CR);
+	__GUEST_ASSERT(actual_val == expected_val,
+		       "Wanted MSR_VM_CR == 0x%lx after WRMSR(0x%lx), got 0x%lx",
+		       expected_val, val, actual_val);
+}
+
+static void guest_vm_cr(void)
+{
+	const u64 svme_valid_mask = SVM_VM_CR_VALID_MASK & ~SVM_VM_CR_SVM_DIS_MASK;
+	const u64 unlocked_mask = SVM_VM_CR_VALID_MASK &
+				  ~(SVM_VM_CR_SVM_LOCK_MASK | SVM_VM_CR_SVM_DIS_MASK);
+	u64 efer, val, expected = 0;
+	bool valid;
+	int bit;
+
+	GUEST_ASSERT_EQ(rdmsr(MSR_VM_CR), 0);
+
+	efer = rdmsr(MSR_EFER);
+	wrmsr(MSR_EFER, efer | EFER_SVME);
+
+	/*
+	 * With EFER.SVME=1, setting non-SVMDIS bits in MSR_VM_CR is allowed,
+	 * whereas setting SVMDIS or reserved bits must fault with #GP(0)
+	 * without modifying MSR_VM_CR.
+	 */
+	guest_wrmsr_vm_cr(SVM_VM_CR_VALID_MASK, 0, true);
+
+	for (bit = 0; bit < BITS_PER_LONG; bit++) {
+		val = BIT_ULL(bit);
+		valid = val & svme_valid_mask;
+		if (valid)
+			expected = val;
+
+		guest_wrmsr_vm_cr(val, expected, !valid);
+	}
+
+	/*
+	 * Verify that prior faulting attempts to set SVMDIS while EFER.SVME=1
+	 * did not latch SVMDIS and cause subsequent valid writes to fault.
+	 */
+	guest_wrmsr_vm_cr(0, 0, false);
+
+	/*
+	 * With EFER.SVME=0, setting SVMDIS succeeds.  Once set, guest writes
+	 * to LOCK and SVMDIS are silently ignored while bits 2:0 remain
+	 * writable.
+	 */
+	wrmsr(MSR_EFER, efer & ~EFER_SVME);
+
+	guest_wrmsr_vm_cr(SVM_VM_CR_SVM_DIS_MASK, SVM_VM_CR_SVM_DIS_MASK, false);
+	guest_wrmsr_vm_cr(svme_valid_mask,
+			  SVM_VM_CR_SVM_DIS_MASK | unlocked_mask, false);
+	guest_wrmsr_vm_cr(0, SVM_VM_CR_SVM_DIS_MASK, false);
+
+	GUEST_DONE();
+}
+
 static void l2_lmsle(void)
 {
 	GUEST_ASSERT(rdmsr(MSR_EFER) & EFER_LMSLE);
@@ -144,17 +212,10 @@ static void test_clear_efer_svme(void)
 	ksft_test_result_pass("L2 clearing EFER.SVME shuts down L1\n");
 }
 
-static void test_lmsle(bool lmsle_mbz)
+static void run_vcpu_to_done(struct kvm_vcpu *vcpu)
 {
-	struct kvm_vcpu *vcpu;
-	struct kvm_vm *vm;
 	struct ucall uc;
 
-	vcpu = create_l1_vcpu(&vm, l1_lmsle);
-
-	vcpu_set_or_clear_cpuid_feature(vcpu, X86_FEATURE_EFER_LMSLE_MBZ,
-					lmsle_mbz);
-
 	vcpu_run(vcpu);
 	TEST_ASSERT_KVM_EXIT_REASON(vcpu, KVM_EXIT_IO);
 
@@ -166,6 +227,79 @@ static void test_lmsle(bool lmsle_mbz)
 	default:
 		TEST_FAIL("Unexpected ucall: %lu", uc.cmd);
 	}
+}
+
+static void test_vm_cr(void)
+{
+	struct kvm_vcpu *vcpu;
+	struct kvm_vm *vm;
+
+	vm = vm_create_with_one_vcpu(&vcpu, guest_vm_cr);
+
+	run_vcpu_to_done(vcpu);
+
+	kvm_vm_free(vm);
+	ksft_test_result_pass("Guest writes to MSR_VM_CR\n");
+}
+
+static void test_host_initiated_vm_cr(void)
+{
+	u64 efer, val, actual, expected = 0;
+	struct kvm_vcpu *vcpu;
+	struct kvm_vm *vm;
+	int bit, rc;
+	bool valid;
+
+	vm = vm_create_with_one_vcpu(&vcpu, NULL);
+
+	/*
+	 * Host-initiated writes must be able to set SVMDIS even when
+	 * EFER.SVME=1 (e.g. when restoring MSR_VM_CR before MSR_EFER), and
+	 * must be able to modify and clear SVMDIS and LOCK after SVMDIS has
+	 * been set.
+	 */
+	TEST_ASSERT_EQ(vcpu_get_msr(vcpu, MSR_VM_CR), 0);
+
+	efer = vcpu_get_msr(vcpu, MSR_EFER);
+	vcpu_set_msr(vcpu, MSR_EFER, efer | EFER_SVME);
+
+	for (bit = 0; bit < BITS_PER_LONG; bit++) {
+		val = BIT_ULL(bit);
+		valid = val & SVM_VM_CR_VALID_MASK;
+
+		rc = _vcpu_set_msr(vcpu, MSR_VM_CR, val);
+		TEST_ASSERT(valid ? rc == 1 : !rc,
+			    "Expected KVM_SET_MSRS(MSR_VM_CR, 0x%lx) to %s, got rc=%d",
+			    val, valid ? "succeed" : "fail", rc);
+		if (valid)
+			expected = val;
+		actual = vcpu_get_msr(vcpu, MSR_VM_CR);
+		TEST_ASSERT(actual == expected,
+			    "Wanted MSR_VM_CR == 0x%lx after KVM_SET_MSRS(0x%lx), got 0x%lx",
+			    expected, val, actual);
+	}
+
+	vcpu_set_msr(vcpu, MSR_VM_CR, SVM_VM_CR_VALID_MASK);
+	TEST_ASSERT_EQ(vcpu_get_msr(vcpu, MSR_VM_CR), SVM_VM_CR_VALID_MASK);
+
+	vcpu_set_msr(vcpu, MSR_VM_CR, 0);
+	TEST_ASSERT_EQ(vcpu_get_msr(vcpu, MSR_VM_CR), 0);
+
+	kvm_vm_free(vm);
+	ksft_test_result_pass("Host-initiated writes to MSR_VM_CR\n");
+}
+
+static void test_lmsle(bool lmsle_mbz)
+{
+	struct kvm_vcpu *vcpu;
+	struct kvm_vm *vm;
+
+	vcpu = create_l1_vcpu(&vm, l1_lmsle);
+
+	vcpu_set_or_clear_cpuid_feature(vcpu, X86_FEATURE_EFER_LMSLE_MBZ,
+					lmsle_mbz);
+
+	run_vcpu_to_done(vcpu);
 
 	kvm_vm_free(vm);
 	ksft_test_result_pass("Guest EFER_LMSLE_MBZ=%d\n", lmsle_mbz);
@@ -225,15 +359,16 @@ int main(int argc, char *argv[])
 	bool has_nested_svm, has_lmsle;
 
 	ksft_print_header();
-	ksft_set_plan(6);
+	ksft_set_plan(8);
 
 	test_enumeration();
 
 	/*
-	 * The sub-tests below need to actually run a nested guest, and the
-	 * EFER.LMSLE sub-tests additionally need KVM to allow EFER.LMSLE.
-	 * It's KVM's view of the world, not raw CPUID, that dictates whether
-	 * EFER.LMSLE is allowed, i.e. whether the defeature is emulated.
+	 * The sub-tests below need to actually run a nested guest or set
+	 * EFER.SVME, and the EFER.LMSLE sub-tests additionally need KVM to
+	 * allow EFER.LMSLE.  It's KVM's view of the world, not raw CPUID, that
+	 * dictates whether EFER.LMSLE is allowed, i.e. whether the defeature is
+	 * emulated.
 	 */
 	has_nested_svm = kvm_cpu_has(X86_FEATURE_SVM);
 	has_lmsle = has_nested_svm &&
@@ -246,9 +381,13 @@ int main(int argc, char *argv[])
 
 	if (has_nested_svm) {
 		test_clear_efer_svme();
+		test_vm_cr();
+		test_host_initiated_vm_cr();
 		test_lmsle(true);
 	} else {
 		ksft_test_result_skip("L2 clearing EFER.SVME shuts down L1\n");
+		ksft_test_result_skip("Guest writes to MSR_VM_CR\n");
+		ksft_test_result_skip("Host-initiated writes to MSR_VM_CR\n");
 		ksft_test_result_skip("Guest EFER_LMSLE_MBZ=1\n");
 	}
 
-- 
2.56.0.317.g454f8302e1-goog


^ permalink raw reply related	[flat|nested] 8+ messages in thread

* Re: [PATCH 1/3] KVM: SVM: Check EFER.SVME before updating MSR_VM_CR on guest writes
  2026-10-29 20:28 [PATCH 1/3] KVM: SVM: Check EFER.SVME before updating MSR_VM_CR on guest writes Jim Mattson
  2026-10-29 20:28 ` [PATCH 2/3] KVM: selftests: Rename svm_nested_efer_test to svm_msrs_test Jim Mattson
  2026-10-29 20:28 ` [PATCH 3/3] KVM: selftests: Add MSR_VM_CR tests " Jim Mattson
@ 2026-10-29 21:19 ` Sean Christopherson
  2026-10-29 21:46   ` Jim Mattson
  2 siblings, 1 reply; 8+ messages in thread
From: Sean Christopherson @ 2026-10-29 21:19 UTC (permalink / raw)
  To: Jim Mattson
  Cc: Paolo Bonzini, Shuah Khan, kvm, linux-kselftest, linux-kernel,
	Yosry Ahmed

On Thu, Oct 29, 2026, Jim Mattson wrote:
> (Host- initiated writes are allowed to set any valid MSR_VM_CR bits.)

Eww.  Why?  If we allow this, then we'll end up with the mess where the guest
can't write back the current value (if userspace is being weird).  I get that
we want to avoid ordering issues, but there's *already* a required order here,
unless userspace ignores the failure from svm_set_vm_cr() and continues on.

KVM doesn't advertise the MSR as something to be saved/restored, and none of
QEMU, CHV, CrosVM, or our VMM appear to save/restore it.  So why make trouble
for ourselves?

> Fixes: 4a810181c8bc ("KVM: SVM: Implement emulation of vm_cr msr")
> Signed-off-by: Jim Mattson <jmattson@google.com>
> ---
>  arch/x86/kvm/svm/svm.c | 22 +++++++++++++---------
>  1 file changed, 13 insertions(+), 9 deletions(-)
> 
> diff --git a/arch/x86/kvm/svm/svm.c b/arch/x86/kvm/svm/svm.c
> index 136069dfcd44..3a0a507c94b2 100644
> --- a/arch/x86/kvm/svm/svm.c
> +++ b/arch/x86/kvm/svm/svm.c
> @@ -3015,14 +3015,24 @@ static int svm_complete_emulated_msr(struct kvm_vcpu *vcpu, int err)
>  	return 1;
>  }
>  
> -static int svm_set_vm_cr(struct kvm_vcpu *vcpu, u64 data)
> +static int svm_set_vm_cr(struct kvm_vcpu *vcpu, struct msr_data *msr)
>  {
>  	struct vcpu_svm *svm = to_svm(vcpu);
> -	int svm_dis, chg_mask;
> +	u64 data = msr->data;
> +	int chg_mask;
>  
>  	if (data & ~SVM_VM_CR_VALID_MASK)
>  		return 1;
>  
> +	if (msr->host_initiated) {
> +		svm->nested.vm_cr_msr = data;
> +		return 0;
> +	}
> +
> +	/* check for svm_disable while efer.svme is set */
> +	if ((data & SVM_VM_CR_SVM_DIS_MASK) && (vcpu->arch.efer & EFER_SVME))
> +		return 1;
> +
>  	chg_mask = SVM_VM_CR_VALID_MASK;
>  
>  	if (svm->nested.vm_cr_msr & SVM_VM_CR_SVM_DIS_MASK)
> @@ -3031,12 +3041,6 @@ static int svm_set_vm_cr(struct kvm_vcpu *vcpu, u64 data)
>  	svm->nested.vm_cr_msr &= ~chg_mask;
>  	svm->nested.vm_cr_msr |= (data & chg_mask);
>  
> -	svm_dis = svm->nested.vm_cr_msr & SVM_VM_CR_SVM_DIS_MASK;
> -
> -	/* check for svm_disable while efer.svme is set */
> -	if (svm_dis && (vcpu->arch.efer & EFER_SVME))
> -		return 1;
> -
>  	return 0;
>  }
>  
> @@ -3262,7 +3266,7 @@ static int svm_set_msr(struct kvm_vcpu *vcpu, struct msr_data *msr)
>  		svm->nested.hsave_msr = data & PAGE_MASK;
>  		break;
>  	case MSR_VM_CR:
> -		return svm_set_vm_cr(vcpu, data);
> +		return svm_set_vm_cr(vcpu, msr);
>  	case MSR_VM_IGNNE:
>  		kvm_pr_unimpl_wrmsr(vcpu, ecx, data);
>  		break;
> -- 
> 2.56.0.317.g454f8302e1-goog
> 

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH 1/3] KVM: SVM: Check EFER.SVME before updating MSR_VM_CR on guest writes
  2026-10-29 21:19 ` [PATCH 1/3] KVM: SVM: Check EFER.SVME before updating MSR_VM_CR on guest writes Sean Christopherson
@ 2026-10-29 21:46   ` Jim Mattson
  2026-10-29 21:52     ` Sean Christopherson
  0 siblings, 1 reply; 8+ messages in thread
From: Jim Mattson @ 2026-10-29 21:46 UTC (permalink / raw)
  To: Sean Christopherson
  Cc: Paolo Bonzini, Shuah Khan, kvm, linux-kselftest, linux-kernel,
	Yosry Ahmed

On Thu, Oct 29, 2026 at 2:19 PM Sean Christopherson <seanjc@google.com> wrote:
>
> On Thu, Oct 29, 2026, Jim Mattson wrote:
> > (Host- initiated writes are allowed to set any valid MSR_VM_CR bits.)
>
> Eww.  Why?  If we allow this, then we'll end up with the mess where the guest
> can't write back the current value (if userspace is being weird).  I get that
> we want to avoid ordering issues, but there's *already* a required order here,
> unless userspace ignores the failure from svm_set_vm_cr() and continues on.
>
> KVM doesn't advertise the MSR as something to be saved/restored, and none of
> QEMU, CHV, CrosVM, or our VMM appear to save/restore it.  So why make trouble
> for ourselves?

OMG! I didn't look any farther than the obviously broken svm_set_vm_cr().

It looks like svm->nested.vm_cr_msr controls nothing other than itself.

It's just 5 bits of storage with some weird locking rules, and it
resets to 0 on suspend/resume.

What is the point of such half-baked support?

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH 1/3] KVM: SVM: Check EFER.SVME before updating MSR_VM_CR on guest writes
  2026-10-29 21:46   ` Jim Mattson
@ 2026-10-29 21:52     ` Sean Christopherson
  2026-10-29 22:04       ` Jim Mattson
  0 siblings, 1 reply; 8+ messages in thread
From: Sean Christopherson @ 2026-10-29 21:52 UTC (permalink / raw)
  To: Jim Mattson
  Cc: Paolo Bonzini, Shuah Khan, kvm, linux-kselftest, linux-kernel,
	Yosry Ahmed

On Thu, Oct 29, 2026, Jim Mattson wrote:
> On Thu, Oct 29, 2026 at 2:19 PM Sean Christopherson <seanjc@google.com> wrote:
> >
> > On Thu, Oct 29, 2026, Jim Mattson wrote:
> > > (Host- initiated writes are allowed to set any valid MSR_VM_CR bits.)
> >
> > Eww.  Why?  If we allow this, then we'll end up with the mess where the guest
> > can't write back the current value (if userspace is being weird).  I get that
> > we want to avoid ordering issues, but there's *already* a required order here,
> > unless userspace ignores the failure from svm_set_vm_cr() and continues on.
> >
> > KVM doesn't advertise the MSR as something to be saved/restored, and none of
> > QEMU, CHV, CrosVM, or our VMM appear to save/restore it.  So why make trouble
> > for ourselves?
> 
> OMG! I didn't look any farther than the obviously broken svm_set_vm_cr().
> 
> It looks like svm->nested.vm_cr_msr controls nothing other than itself.

LOL, nice.

> It's just 5 bits of storage with some weird locking rules, and it
> resets to 0 on suspend/resume.
> 
> What is the point of such half-baked support?

More than likely, so that older kernels didn't crash when writing an MSR KVM
didn't emulate. :-(

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH 1/3] KVM: SVM: Check EFER.SVME before updating MSR_VM_CR on guest writes
  2026-10-29 21:52     ` Sean Christopherson
@ 2026-10-29 22:04       ` Jim Mattson
  2026-10-30  0:00         ` Jim Mattson
  0 siblings, 1 reply; 8+ messages in thread
From: Jim Mattson @ 2026-10-29 22:04 UTC (permalink / raw)
  To: Sean Christopherson
  Cc: Paolo Bonzini, Shuah Khan, kvm, linux-kselftest, linux-kernel,
	Yosry Ahmed

On Thu, Oct 29, 2026 at 2:52 PM Sean Christopherson <seanjc@google.com> wrote:
>
> On Thu, Oct 29, 2026, Jim Mattson wrote:
> > On Thu, Oct 29, 2026 at 2:19 PM Sean Christopherson <seanjc@google.com> wrote:
> > >
> > > On Thu, Oct 29, 2026, Jim Mattson wrote:
> > > > (Host- initiated writes are allowed to set any valid MSR_VM_CR bits.)
> > >
> > > Eww.  Why?  If we allow this, then we'll end up with the mess where the guest
> > > can't write back the current value (if userspace is being weird).  I get that
> > > we want to avoid ordering issues, but there's *already* a required order here,
> > > unless userspace ignores the failure from svm_set_vm_cr() and continues on.
> > >
> > > KVM doesn't advertise the MSR as something to be saved/restored, and none of
> > > QEMU, CHV, CrosVM, or our VMM appear to save/restore it.  So why make trouble
> > > for ourselves?
> >
> > OMG! I didn't look any farther than the obviously broken svm_set_vm_cr().
> >
> > It looks like svm->nested.vm_cr_msr controls nothing other than itself.
>
> LOL, nice.
>
> > It's just 5 bits of storage with some weird locking rules, and it
> > resets to 0 on suspend/resume.
> >
> > What is the point of such half-baked support?
>
> More than likely, so that older kernels didn't crash when writing an MSR KVM
> didn't emulate. :-(

You're not going to let me get rid of it, are you?

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH 1/3] KVM: SVM: Check EFER.SVME before updating MSR_VM_CR on guest writes
  2026-10-29 22:04       ` Jim Mattson
@ 2026-10-30  0:00         ` Jim Mattson
  0 siblings, 0 replies; 8+ messages in thread
From: Jim Mattson @ 2026-10-30  0:00 UTC (permalink / raw)
  To: Sean Christopherson
  Cc: Paolo Bonzini, Shuah Khan, kvm, linux-kselftest, linux-kernel,
	Yosry Ahmed

On Thu, Oct 29, 2026 at 3:04 PM Jim Mattson <jmattson@google.com> wrote:
>
> On Thu, Oct 29, 2026 at 2:52 PM Sean Christopherson <seanjc@google.com> wrote:
> >
> > On Thu, Oct 29, 2026, Jim Mattson wrote:
> > > On Thu, Oct 29, 2026 at 2:19 PM Sean Christopherson <seanjc@google.com> wrote:
> > > >
> > > > On Thu, Oct 29, 2026, Jim Mattson wrote:
> > > > > (Host- initiated writes are allowed to set any valid MSR_VM_CR bits.)
> > > >
> > > > Eww.  Why?  If we allow this, then we'll end up with the mess where the guest
> > > > can't write back the current value (if userspace is being weird).  I get that
> > > > we want to avoid ordering issues, but there's *already* a required order here,
> > > > unless userspace ignores the failure from svm_set_vm_cr() and continues on.
> > > >
> > > > KVM doesn't advertise the MSR as something to be saved/restored, and none of
> > > > QEMU, CHV, CrosVM, or our VMM appear to save/restore it.  So why make trouble
> > > > for ourselves?
> > >
> > > OMG! I didn't look any farther than the obviously broken svm_set_vm_cr().
> > >
> > > It looks like svm->nested.vm_cr_msr controls nothing other than itself.
> >
> > LOL, nice.
> >
> > > It's just 5 bits of storage with some weird locking rules, and it
> > > resets to 0 on suspend/resume.
> > >
> > > What is the point of such half-baked support?
> >
> > More than likely, so that older kernels didn't crash when writing an MSR KVM
> > didn't emulate. :-(
>
> You're not going to let me get rid of it, are you?

LOL! Even the 'LOCK' check is wrong:

 if (svm->nested.vm_cr_msr & SVM_VM_CR_SVM_DIS_MASK)
        chg_mask &= ~(SVM_VM_CR_SVM_LOCK_MASK | SVM_VM_CR_SVM_DIS_MASK);

The condition should be checking SVM_VM_CR_SVM_LOCK_MASK.

Well, at least it can be used as a sentinel you can poll to see if
your VM has live-migrated. :)

^ permalink raw reply	[flat|nested] 8+ messages in thread

end of thread, other threads:[~2026-10-30  0:00 UTC | newest]

Thread overview: 8+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2026-10-29 20:28 [PATCH 1/3] KVM: SVM: Check EFER.SVME before updating MSR_VM_CR on guest writes Jim Mattson
2026-10-29 20:28 ` [PATCH 2/3] KVM: selftests: Rename svm_nested_efer_test to svm_msrs_test Jim Mattson
2026-10-29 20:28 ` [PATCH 3/3] KVM: selftests: Add MSR_VM_CR tests " Jim Mattson
2026-10-29 21:19 ` [PATCH 1/3] KVM: SVM: Check EFER.SVME before updating MSR_VM_CR on guest writes Sean Christopherson
2026-10-29 21:46   ` Jim Mattson
2026-10-29 21:52     ` Sean Christopherson
2026-10-29 22:04       ` Jim Mattson
2026-10-30  0:00         ` Jim Mattson

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.