* [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 a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox