[PATCH v4 0/4] KVM: x86: allow DEBUGCTL.DEBUGCTLMSR_FREEZE_IN_SMM passthrough

[PATCH v4 0/4] KVM: x86: allow DEBUGCTL.DEBUGCTLMSR_FREEZE_IN_SMM passthrough

[Maxim Levitsky]

Currently KVM allows the guest to set IA32_DEBUGCTL to whatever value
the guest wants, only capped by a bitmask of allowed bits

(except in the nested entry where KVM apparently doesn't even check
this set of allowed bits - this patch series also fixes that)

However some IA32_DEBUGCTL bits can be useful for the host, e.g the
IA32_DEBUGCTL.DEBUGCTLMSR_FREEZE_IN_SMM which isolates the PMU from
the influence of the host's SMM.

Reshuffle some of the code to allow (currently only this bit) to be passed
though from its host value to the guest.

Note that host value of this bit can be toggled by writing 0 or 1 to
/sys/devices/cpu/freeze_on_smi

This was tested on a Intel(R) Xeon(R) Silver 4410Y with KVM unit tests and
kvm selftests running in parallel with tight loop writing to IO port 0xB2
which on this machine generates #SMIs.

SMI generation was also verified also by reading the MSR 0x34 which
shows the current count of #SMIs received.

Despite the flood of #SMIs, the tests survived with this patch applied.

V4: incorporated review feedback.

Best regards,
     Maxim Levitsky

Maxim Levitsky (2):
  x86: nVMX: check vmcs12->guest_ia32_debugctl value given by L2
  x86: KVM: VMX: preserve DEBUGCTLMSR_FREEZE_IN_SMM

Sean Christopherson (2):
  KVM: x86: Convert vcpu_run()'s immediate exit param into a generic
    bitmap
  KVM: x86: Drop kvm_x86_ops.set_dr6() in favor of a new KVM_RUN flag

 arch/x86/include/asm/kvm-x86-ops.h |  1 -
 arch/x86/include/asm/kvm_host.h    |  9 ++++++--
 arch/x86/kvm/svm/svm.c             | 14 +++++++-----
 arch/x86/kvm/vmx/main.c            | 15 +++----------
 arch/x86/kvm/vmx/nested.c          |  8 +++++--
 arch/x86/kvm/vmx/tdx.c             |  3 ++-
 arch/x86/kvm/vmx/vmx.c             | 36 +++++++++++++++++++++---------
 arch/x86/kvm/vmx/vmx.h             |  4 ++++
 arch/x86/kvm/vmx/x86_ops.h         |  4 ++--
 arch/x86/kvm/x86.c                 | 18 ++++++++++-----
 10 files changed, 70 insertions(+), 42 deletions(-)

-- 
2.46.0

[PATCH v4 1/4] KVM: x86: Convert vcpu_run()'s immediate exit param into a generic bitmap

[Maxim Levitsky]

From: Sean Christopherson <seanjc@google.com>

Signed-off-by: Sean Christopherson <seanjc@google.com>
---
 arch/x86/include/asm/kvm_host.h |  6 +++++-
 arch/x86/kvm/svm/svm.c          |  4 ++--
 arch/x86/kvm/vmx/main.c         |  6 +++---
 arch/x86/kvm/vmx/tdx.c          |  3 ++-
 arch/x86/kvm/vmx/vmx.c          |  3 ++-
 arch/x86/kvm/vmx/x86_ops.h      |  4 ++--
 arch/x86/kvm/x86.c              | 11 ++++++++---
 7 files changed, 24 insertions(+), 13 deletions(-)

diff --git a/arch/x86/include/asm/kvm_host.h b/arch/x86/include/asm/kvm_host.h
index ed9b65785a24..c04f73cf062f 100644
--- a/arch/x86/include/asm/kvm_host.h
+++ b/arch/x86/include/asm/kvm_host.h
@@ -1670,6 +1670,10 @@ static inline u16 kvm_lapic_irq_dest_mode(bool dest_mode_logical)
 	return dest_mode_logical ? APIC_DEST_LOGICAL : APIC_DEST_PHYSICAL;
 }
 
+enum kvm_x86_run_flags {
+	KVM_RUN_FORCE_IMMEDIATE_EXIT	= BIT(0),
+};
+
 struct kvm_x86_ops {
 	const char *name;
 
@@ -1751,7 +1755,7 @@ struct kvm_x86_ops {
 
 	int (*vcpu_pre_run)(struct kvm_vcpu *vcpu);
 	enum exit_fastpath_completion (*vcpu_run)(struct kvm_vcpu *vcpu,
-						  bool force_immediate_exit);
+						  u64 run_flags);
 	int (*handle_exit)(struct kvm_vcpu *vcpu,
 		enum exit_fastpath_completion exit_fastpath);
 	int (*skip_emulated_instruction)(struct kvm_vcpu *vcpu);
diff --git a/arch/x86/kvm/svm/svm.c b/arch/x86/kvm/svm/svm.c
index cc1c721ba067..c8b8a9947057 100644
--- a/arch/x86/kvm/svm/svm.c
+++ b/arch/x86/kvm/svm/svm.c
@@ -4259,9 +4259,9 @@ static noinstr void svm_vcpu_enter_exit(struct kvm_vcpu *vcpu, bool spec_ctrl_in
 	guest_state_exit_irqoff();
 }
 
-static __no_kcsan fastpath_t svm_vcpu_run(struct kvm_vcpu *vcpu,
-					  bool force_immediate_exit)
+static __no_kcsan fastpath_t svm_vcpu_run(struct kvm_vcpu *vcpu, u64 run_flags)
 {
+	bool force_immediate_exit = run_flags & KVM_RUN_FORCE_IMMEDIATE_EXIT;
 	struct vcpu_svm *svm = to_svm(vcpu);
 	bool spec_ctrl_intercepted = msr_write_intercepted(vcpu, MSR_IA32_SPEC_CTRL);
 
diff --git a/arch/x86/kvm/vmx/main.c b/arch/x86/kvm/vmx/main.c
index 94d5d907d37b..a8e80d66e77a 100644
--- a/arch/x86/kvm/vmx/main.c
+++ b/arch/x86/kvm/vmx/main.c
@@ -176,12 +176,12 @@ static int vt_vcpu_pre_run(struct kvm_vcpu *vcpu)
 	return vmx_vcpu_pre_run(vcpu);
 }
 
-static fastpath_t vt_vcpu_run(struct kvm_vcpu *vcpu, bool force_immediate_exit)
+static fastpath_t vt_vcpu_run(struct kvm_vcpu *vcpu, u64 run_flags)
 {
 	if (is_td_vcpu(vcpu))
-		return tdx_vcpu_run(vcpu, force_immediate_exit);
+		return tdx_vcpu_run(vcpu, run_flags);
 
-	return vmx_vcpu_run(vcpu, force_immediate_exit);
+	return vmx_vcpu_run(vcpu, run_flags);
 }
 
 static int vt_handle_exit(struct kvm_vcpu *vcpu,
diff --git a/arch/x86/kvm/vmx/tdx.c b/arch/x86/kvm/vmx/tdx.c
index b952bc673271..7dbfad28debc 100644
--- a/arch/x86/kvm/vmx/tdx.c
+++ b/arch/x86/kvm/vmx/tdx.c
@@ -1020,8 +1020,9 @@ static void tdx_load_host_xsave_state(struct kvm_vcpu *vcpu)
 				DEBUGCTLMSR_FREEZE_PERFMON_ON_PMI | \
 				DEBUGCTLMSR_FREEZE_IN_SMM)
 
-fastpath_t tdx_vcpu_run(struct kvm_vcpu *vcpu, bool force_immediate_exit)
+fastpath_t tdx_vcpu_run(struct kvm_vcpu *vcpu, u64 run_flags)
 {
+	bool force_immediate_exit = run_flags & KVM_RUN_FORCE_IMMEDIATE_EXIT;
 	struct vcpu_tdx *tdx = to_tdx(vcpu);
 	struct vcpu_vt *vt = to_vt(vcpu);
 
diff --git a/arch/x86/kvm/vmx/vmx.c b/arch/x86/kvm/vmx/vmx.c
index ef2d7208dd20..609563da270c 100644
--- a/arch/x86/kvm/vmx/vmx.c
+++ b/arch/x86/kvm/vmx/vmx.c
@@ -7324,8 +7324,9 @@ static noinstr void vmx_vcpu_enter_exit(struct kvm_vcpu *vcpu,
 	guest_state_exit_irqoff();
 }
 
-fastpath_t vmx_vcpu_run(struct kvm_vcpu *vcpu, bool force_immediate_exit)
+fastpath_t vmx_vcpu_run(struct kvm_vcpu *vcpu, u64 run_flags)
 {
+	bool force_immediate_exit = run_flags & KVM_RUN_FORCE_IMMEDIATE_EXIT;
 	struct vcpu_vmx *vmx = to_vmx(vcpu);
 	unsigned long cr3, cr4;
 
diff --git a/arch/x86/kvm/vmx/x86_ops.h b/arch/x86/kvm/vmx/x86_ops.h
index 6bf8be570b2e..e1dfacd6b41f 100644
--- a/arch/x86/kvm/vmx/x86_ops.h
+++ b/arch/x86/kvm/vmx/x86_ops.h
@@ -21,7 +21,7 @@ void vmx_vm_destroy(struct kvm *kvm);
 int vmx_vcpu_precreate(struct kvm *kvm);
 int vmx_vcpu_create(struct kvm_vcpu *vcpu);
 int vmx_vcpu_pre_run(struct kvm_vcpu *vcpu);
-fastpath_t vmx_vcpu_run(struct kvm_vcpu *vcpu, bool force_immediate_exit);
+fastpath_t vmx_vcpu_run(struct kvm_vcpu *vcpu, u64 run_flags);
 void vmx_vcpu_free(struct kvm_vcpu *vcpu);
 void vmx_vcpu_reset(struct kvm_vcpu *vcpu, bool init_event);
 void vmx_vcpu_load(struct kvm_vcpu *vcpu, int cpu);
@@ -132,7 +132,7 @@ void tdx_vcpu_reset(struct kvm_vcpu *vcpu, bool init_event);
 void tdx_vcpu_free(struct kvm_vcpu *vcpu);
 void tdx_vcpu_load(struct kvm_vcpu *vcpu, int cpu);
 int tdx_vcpu_pre_run(struct kvm_vcpu *vcpu);
-fastpath_t tdx_vcpu_run(struct kvm_vcpu *vcpu, bool force_immediate_exit);
+fastpath_t tdx_vcpu_run(struct kvm_vcpu *vcpu, u64 run_flags);
 void tdx_prepare_switch_to_guest(struct kvm_vcpu *vcpu);
 void tdx_vcpu_put(struct kvm_vcpu *vcpu);
 bool tdx_protected_apic_has_interrupt(struct kvm_vcpu *vcpu);
diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
index 844e81ee1d96..25de78cdab42 100644
--- a/arch/x86/kvm/x86.c
+++ b/arch/x86/kvm/x86.c
@@ -10752,6 +10752,7 @@ static int vcpu_enter_guest(struct kvm_vcpu *vcpu)
 		dm_request_for_irq_injection(vcpu) &&
 		kvm_cpu_accept_dm_intr(vcpu);
 	fastpath_t exit_fastpath;
+	u64 run_flags;
 
 	bool req_immediate_exit = false;
 
@@ -10996,8 +10997,11 @@ static int vcpu_enter_guest(struct kvm_vcpu *vcpu)
 		goto cancel_injection;
 	}
 
-	if (req_immediate_exit)
+	run_flags = 0;
+	if (req_immediate_exit) {
+		run_flags |= KVM_RUN_FORCE_IMMEDIATE_EXIT;
 		kvm_make_request(KVM_REQ_EVENT, vcpu);
+	}
 
 	fpregs_assert_state_consistent();
 	if (test_thread_flag(TIF_NEED_FPU_LOAD))
@@ -11034,8 +11038,7 @@ static int vcpu_enter_guest(struct kvm_vcpu *vcpu)
 		WARN_ON_ONCE((kvm_vcpu_apicv_activated(vcpu) != kvm_vcpu_apicv_active(vcpu)) &&
 			     (kvm_get_apic_mode(vcpu) != LAPIC_MODE_DISABLED));
 
-		exit_fastpath = kvm_x86_call(vcpu_run)(vcpu,
-						       req_immediate_exit);
+		exit_fastpath = kvm_x86_call(vcpu_run)(vcpu, run_flags);
 		if (likely(exit_fastpath != EXIT_FASTPATH_REENTER_GUEST))
 			break;
 
@@ -11047,6 +11050,8 @@ static int vcpu_enter_guest(struct kvm_vcpu *vcpu)
 			break;
 		}
 
+		run_flags = 0;
+
 		/* Note, VM-Exits that go down the "slow" path are accounted below. */
 		++vcpu->stat.exits;
 	}
-- 
2.46.0

[PATCH v4 2/4] KVM: x86: Drop kvm_x86_ops.set_dr6() in favor of a new KVM_RUN flag

[Maxim Levitsky]

From: Sean Christopherson <seanjc@google.com>

Instruct vendor code to load the guest's DR6 into hardware via a new
KVM_RUN flag, and remove kvm_x86_ops.set_dr6(), whose sole purpose was to
load vcpu->arch.dr6 into hardware when DR6 can be read/written directly
by the guest.

Signed-off-by: Sean Christopherson <seanjc@google.com>
---
 arch/x86/include/asm/kvm-x86-ops.h |  1 -
 arch/x86/include/asm/kvm_host.h    |  2 +-
 arch/x86/kvm/svm/svm.c             | 10 ++++++----
 arch/x86/kvm/vmx/main.c            |  9 ---------
 arch/x86/kvm/vmx/vmx.c             |  9 +++------
 arch/x86/kvm/x86.c                 |  2 +-
 6 files changed, 11 insertions(+), 22 deletions(-)

diff --git a/arch/x86/include/asm/kvm-x86-ops.h b/arch/x86/include/asm/kvm-x86-ops.h
index 79406bf07a1c..a2248817470c 100644
--- a/arch/x86/include/asm/kvm-x86-ops.h
+++ b/arch/x86/include/asm/kvm-x86-ops.h
@@ -49,7 +49,6 @@ KVM_X86_OP(set_idt)
 KVM_X86_OP(get_gdt)
 KVM_X86_OP(set_gdt)
 KVM_X86_OP(sync_dirty_debug_regs)
-KVM_X86_OP(set_dr6)
 KVM_X86_OP(set_dr7)
 KVM_X86_OP(cache_reg)
 KVM_X86_OP(get_rflags)
diff --git a/arch/x86/include/asm/kvm_host.h b/arch/x86/include/asm/kvm_host.h
index c04f73cf062f..d2ad31a1628e 100644
--- a/arch/x86/include/asm/kvm_host.h
+++ b/arch/x86/include/asm/kvm_host.h
@@ -1672,6 +1672,7 @@ static inline u16 kvm_lapic_irq_dest_mode(bool dest_mode_logical)
 
 enum kvm_x86_run_flags {
 	KVM_RUN_FORCE_IMMEDIATE_EXIT	= BIT(0),
+	KVM_RUN_LOAD_GUEST_DR6		= BIT(1),
 };
 
 struct kvm_x86_ops {
@@ -1724,7 +1725,6 @@ struct kvm_x86_ops {
 	void (*get_gdt)(struct kvm_vcpu *vcpu, struct desc_ptr *dt);
 	void (*set_gdt)(struct kvm_vcpu *vcpu, struct desc_ptr *dt);
 	void (*sync_dirty_debug_regs)(struct kvm_vcpu *vcpu);
-	void (*set_dr6)(struct kvm_vcpu *vcpu, unsigned long value);
 	void (*set_dr7)(struct kvm_vcpu *vcpu, unsigned long value);
 	void (*cache_reg)(struct kvm_vcpu *vcpu, enum kvm_reg reg);
 	unsigned long (*get_rflags)(struct kvm_vcpu *vcpu);
diff --git a/arch/x86/kvm/svm/svm.c b/arch/x86/kvm/svm/svm.c
index c8b8a9947057..026b28051fff 100644
--- a/arch/x86/kvm/svm/svm.c
+++ b/arch/x86/kvm/svm/svm.c
@@ -4308,10 +4308,13 @@ static __no_kcsan fastpath_t svm_vcpu_run(struct kvm_vcpu *vcpu, u64 run_flags)
 	svm_hv_update_vp_id(svm->vmcb, vcpu);
 
 	/*
-	 * Run with all-zero DR6 unless needed, so that we can get the exact cause
-	 * of a #DB.
+	 * Run with all-zero DR6 unless the guest can write DR6 freely, so that
+	 * KVM can get the exact cause of a #DB.  Note, loading guest DR6 from
+	 * KVM's snapshot is only necessary when DR accesses won't exit.
 	 */
-	if (likely(!(vcpu->arch.switch_db_regs & KVM_DEBUGREG_WONT_EXIT)))
+	if (unlikely(run_flags & KVM_RUN_LOAD_GUEST_DR6))
+		svm_set_dr6(vcpu, vcpu->arch.dr6);
+	else if (likely(!(vcpu->arch.switch_db_regs & KVM_DEBUGREG_WONT_EXIT)))
 		svm_set_dr6(vcpu, DR6_ACTIVE_LOW);
 
 	clgi();
@@ -5119,7 +5122,6 @@ static struct kvm_x86_ops svm_x86_ops __initdata = {
 	.set_idt = svm_set_idt,
 	.get_gdt = svm_get_gdt,
 	.set_gdt = svm_set_gdt,
-	.set_dr6 = svm_set_dr6,
 	.set_dr7 = svm_set_dr7,
 	.sync_dirty_debug_regs = svm_sync_dirty_debug_regs,
 	.cache_reg = svm_cache_reg,
diff --git a/arch/x86/kvm/vmx/main.c b/arch/x86/kvm/vmx/main.c
index a8e80d66e77a..28f854055e2c 100644
--- a/arch/x86/kvm/vmx/main.c
+++ b/arch/x86/kvm/vmx/main.c
@@ -498,14 +498,6 @@ static void vt_set_gdt(struct kvm_vcpu *vcpu, struct desc_ptr *dt)
 	vmx_set_gdt(vcpu, dt);
 }
 
-static void vt_set_dr6(struct kvm_vcpu *vcpu, unsigned long val)
-{
-	if (is_td_vcpu(vcpu))
-		return;
-
-	vmx_set_dr6(vcpu, val);
-}
-
 static void vt_set_dr7(struct kvm_vcpu *vcpu, unsigned long val)
 {
 	if (is_td_vcpu(vcpu))
@@ -945,7 +937,6 @@ struct kvm_x86_ops vt_x86_ops __initdata = {
 	.set_idt = vt_set_idt,
 	.get_gdt = vt_get_gdt,
 	.set_gdt = vt_set_gdt,
-	.set_dr6 = vt_set_dr6,
 	.set_dr7 = vt_set_dr7,
 	.sync_dirty_debug_regs = vt_sync_dirty_debug_regs,
 	.cache_reg = vt_cache_reg,
diff --git a/arch/x86/kvm/vmx/vmx.c b/arch/x86/kvm/vmx/vmx.c
index 609563da270c..9953de0cb32a 100644
--- a/arch/x86/kvm/vmx/vmx.c
+++ b/arch/x86/kvm/vmx/vmx.c
@@ -5611,12 +5611,6 @@ void vmx_sync_dirty_debug_regs(struct kvm_vcpu *vcpu)
 	set_debugreg(DR6_RESERVED, 6);
 }
 
-void vmx_set_dr6(struct kvm_vcpu *vcpu, unsigned long val)
-{
-	lockdep_assert_irqs_disabled();
-	set_debugreg(vcpu->arch.dr6, 6);
-}
-
 void vmx_set_dr7(struct kvm_vcpu *vcpu, unsigned long val)
 {
 	vmcs_writel(GUEST_DR7, val);
@@ -7371,6 +7365,9 @@ fastpath_t vmx_vcpu_run(struct kvm_vcpu *vcpu, u64 run_flags)
 		vmcs_writel(GUEST_RIP, vcpu->arch.regs[VCPU_REGS_RIP]);
 	vcpu->arch.regs_dirty = 0;
 
+	if (run_flags & KVM_RUN_LOAD_GUEST_DR6)
+		set_debugreg(vcpu->arch.dr6, 6);
+
 	/*
 	 * Refresh vmcs.HOST_CR3 if necessary.  This must be done immediately
 	 * prior to VM-Enter, as the kernel may load a new ASID (PCID) any time
diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
index 25de78cdab42..684b8047e0f2 100644
--- a/arch/x86/kvm/x86.c
+++ b/arch/x86/kvm/x86.c
@@ -11019,7 +11019,7 @@ static int vcpu_enter_guest(struct kvm_vcpu *vcpu)
 		set_debugreg(vcpu->arch.eff_db[3], 3);
 		/* When KVM_DEBUGREG_WONT_EXIT, dr6 is accessible in guest. */
 		if (unlikely(vcpu->arch.switch_db_regs & KVM_DEBUGREG_WONT_EXIT))
-			kvm_x86_call(set_dr6)(vcpu, vcpu->arch.dr6);
+			run_flags |= KVM_RUN_LOAD_GUEST_DR6;
 	} else if (unlikely(hw_breakpoint_active())) {
 		set_debugreg(0, 7);
 	}
-- 
2.46.0

[PATCH v4 3/4] x86: nVMX: check vmcs12->guest_ia32_debugctl value given by L2

[Maxim Levitsky]

Check the vmcs12 guest_ia32_debugctl value before loading it, to avoid L2
being able to load arbitrary values to hardware IA32_DEBUGCTL.

Signed-off-by: Maxim Levitsky <mlevitsk@redhat.com>
---
 arch/x86/kvm/vmx/nested.c | 4 ++++
 arch/x86/kvm/vmx/vmx.c    | 2 +-
 arch/x86/kvm/vmx/vmx.h    | 2 ++
 3 files changed, 7 insertions(+), 1 deletion(-)

diff --git a/arch/x86/kvm/vmx/nested.c b/arch/x86/kvm/vmx/nested.c
index e073e3008b16..0bda6400e30a 100644
--- a/arch/x86/kvm/vmx/nested.c
+++ b/arch/x86/kvm/vmx/nested.c
@@ -3193,6 +3193,10 @@ static int nested_vmx_check_guest_state(struct kvm_vcpu *vcpu,
 	     CC((vmcs12->guest_bndcfgs & MSR_IA32_BNDCFGS_RSVD))))
 		return -EINVAL;
 
+	if ((vmcs12->vm_entry_controls & VM_ENTRY_LOAD_DEBUG_CONTROLS) &&
+	     CC(vmcs12->guest_ia32_debugctl & ~vmx_get_supported_debugctl(vcpu, false)))
+		return -EINVAL;
+
 	if (nested_check_guest_non_reg_state(vmcs12))
 		return -EINVAL;
 
diff --git a/arch/x86/kvm/vmx/vmx.c b/arch/x86/kvm/vmx/vmx.c
index 9953de0cb32a..9046ee2e9a04 100644
--- a/arch/x86/kvm/vmx/vmx.c
+++ b/arch/x86/kvm/vmx/vmx.c
@@ -2179,7 +2179,7 @@ static u64 nested_vmx_truncate_sysenter_addr(struct kvm_vcpu *vcpu,
 	return (unsigned long)data;
 }
 
-static u64 vmx_get_supported_debugctl(struct kvm_vcpu *vcpu, bool host_initiated)
+u64 vmx_get_supported_debugctl(struct kvm_vcpu *vcpu, bool host_initiated)
 {
 	u64 debugctl = 0;
 
diff --git a/arch/x86/kvm/vmx/vmx.h b/arch/x86/kvm/vmx/vmx.h
index 6d1e40ecc024..1b80479505d3 100644
--- a/arch/x86/kvm/vmx/vmx.h
+++ b/arch/x86/kvm/vmx/vmx.h
@@ -413,7 +413,9 @@ static inline void vmx_set_intercept_for_msr(struct kvm_vcpu *vcpu, u32 msr,
 		vmx_disable_intercept_for_msr(vcpu, msr, type);
 }
 
+
 void vmx_update_cpu_dirty_logging(struct kvm_vcpu *vcpu);
+u64 vmx_get_supported_debugctl(struct kvm_vcpu *vcpu, bool host_initiated);
 
 /*
  * Note, early Intel manuals have the write-low and read-high bitmap offsets
-- 
2.46.0

[PATCH v4 4/4] x86: KVM: VMX: preserve DEBUGCTLMSR_FREEZE_IN_SMM

[Maxim Levitsky]

Pass through the host's DEBUGCTL.DEBUGCTLMSR_FREEZE_IN_SMM to the guest
GUEST_IA32_DEBUGCTL without the guest seeing this value.

Since the value of the host DEBUGCTL can in theory change between VM runs,
check if has changed, and if yes, then reload the GUEST_IA32_DEBUGCTL with
the new value.

Signed-off-by: Maxim Levitsky <mlevitsk@redhat.com>
---
 arch/x86/include/asm/kvm_host.h |  1 +
 arch/x86/kvm/vmx/nested.c       |  4 ++--
 arch/x86/kvm/vmx/vmx.c          | 22 +++++++++++++++++++---
 arch/x86/kvm/vmx/vmx.h          |  2 ++
 arch/x86/kvm/x86.c              |  7 +++++--
 5 files changed, 29 insertions(+), 7 deletions(-)

diff --git a/arch/x86/include/asm/kvm_host.h b/arch/x86/include/asm/kvm_host.h
index d2ad31a1628e..2e7e4a8b392e 100644
--- a/arch/x86/include/asm/kvm_host.h
+++ b/arch/x86/include/asm/kvm_host.h
@@ -1673,6 +1673,7 @@ static inline u16 kvm_lapic_irq_dest_mode(bool dest_mode_logical)
 enum kvm_x86_run_flags {
 	KVM_RUN_FORCE_IMMEDIATE_EXIT	= BIT(0),
 	KVM_RUN_LOAD_GUEST_DR6		= BIT(1),
+	KVM_RUN_LOAD_DEBUGCTL		= BIT(2),
 };
 
 struct kvm_x86_ops {
diff --git a/arch/x86/kvm/vmx/nested.c b/arch/x86/kvm/vmx/nested.c
index 0bda6400e30a..0a572356119f 100644
--- a/arch/x86/kvm/vmx/nested.c
+++ b/arch/x86/kvm/vmx/nested.c
@@ -2653,7 +2653,7 @@ static int prepare_vmcs02(struct kvm_vcpu *vcpu, struct vmcs12 *vmcs12,
 	if (vmx->nested.nested_run_pending &&
 	    (vmcs12->vm_entry_controls & VM_ENTRY_LOAD_DEBUG_CONTROLS)) {
 		kvm_set_dr(vcpu, 7, vmcs12->guest_dr7);
-		vmcs_write64(GUEST_IA32_DEBUGCTL, vmcs12->guest_ia32_debugctl);
+		vmx_guest_debugctl_write(vcpu, vmcs12->guest_ia32_debugctl);
 	} else {
 		kvm_set_dr(vcpu, 7, vcpu->arch.dr7);
 		vmcs_write64(GUEST_IA32_DEBUGCTL, vmx->nested.pre_vmenter_debugctl);
@@ -4792,7 +4792,7 @@ static void load_vmcs12_host_state(struct kvm_vcpu *vcpu,
 	__vmx_set_segment(vcpu, &seg, VCPU_SREG_LDTR);
 
 	kvm_set_dr(vcpu, 7, 0x400);
-	vmcs_write64(GUEST_IA32_DEBUGCTL, 0);
+	vmx_guest_debugctl_write(vcpu, 0);
 
 	if (nested_vmx_load_msr(vcpu, vmcs12->vm_exit_msr_load_addr,
 				vmcs12->vm_exit_msr_load_count))
diff --git a/arch/x86/kvm/vmx/vmx.c b/arch/x86/kvm/vmx/vmx.c
index 9046ee2e9a04..c70fe7cbede6 100644
--- a/arch/x86/kvm/vmx/vmx.c
+++ b/arch/x86/kvm/vmx/vmx.c
@@ -2154,7 +2154,7 @@ int vmx_get_msr(struct kvm_vcpu *vcpu, struct msr_data *msr_info)
 			msr_info->data = vmx->pt_desc.guest.addr_a[index / 2];
 		break;
 	case MSR_IA32_DEBUGCTLMSR:
-		msr_info->data = vmcs_read64(GUEST_IA32_DEBUGCTL);
+		msr_info->data = vmx_guest_debugctl_read();
 		break;
 	default:
 	find_uret_msr:
@@ -2194,6 +2194,17 @@ u64 vmx_get_supported_debugctl(struct kvm_vcpu *vcpu, bool host_initiated)
 	return debugctl;
 }
 
+void vmx_guest_debugctl_write(struct kvm_vcpu *vcpu, u64 val)
+{
+	val |= vcpu->arch.host_debugctl & DEBUGCTLMSR_FREEZE_IN_SMM;
+	vmcs_write64(GUEST_IA32_DEBUGCTL, val);
+}
+
+u64 vmx_guest_debugctl_read(void)
+{
+	return vmcs_read64(GUEST_IA32_DEBUGCTL) & ~DEBUGCTLMSR_FREEZE_IN_SMM;
+}
+
 /*
  * Writes msr value into the appropriate "register".
  * Returns 0 on success, non-0 otherwise.
@@ -2279,7 +2290,8 @@ int vmx_set_msr(struct kvm_vcpu *vcpu, struct msr_data *msr_info)
 						VM_EXIT_SAVE_DEBUG_CONTROLS)
 			get_vmcs12(vcpu)->guest_ia32_debugctl = data;
 
-		vmcs_write64(GUEST_IA32_DEBUGCTL, data);
+		vmx_guest_debugctl_write(vcpu, data);
+
 		if (intel_pmu_lbr_is_enabled(vcpu) && !to_vmx(vcpu)->lbr_desc.event &&
 		    (data & DEBUGCTLMSR_LBR))
 			intel_pmu_create_guest_lbr_event(vcpu);
@@ -4795,7 +4807,8 @@ static void init_vmcs(struct vcpu_vmx *vmx)
 	vmcs_write32(GUEST_SYSENTER_CS, 0);
 	vmcs_writel(GUEST_SYSENTER_ESP, 0);
 	vmcs_writel(GUEST_SYSENTER_EIP, 0);
-	vmcs_write64(GUEST_IA32_DEBUGCTL, 0);
+
+	vmx_guest_debugctl_write(&vmx->vcpu, 0);
 
 	if (cpu_has_vmx_tpr_shadow()) {
 		vmcs_write64(VIRTUAL_APIC_PAGE_ADDR, 0);
@@ -7368,6 +7381,9 @@ fastpath_t vmx_vcpu_run(struct kvm_vcpu *vcpu, u64 run_flags)
 	if (run_flags & KVM_RUN_LOAD_GUEST_DR6)
 		set_debugreg(vcpu->arch.dr6, 6);
 
+	if (run_flags & KVM_RUN_LOAD_DEBUGCTL)
+		vmx_guest_debugctl_write(vcpu, vmx_guest_debugctl_read());
+
 	/*
 	 * Refresh vmcs.HOST_CR3 if necessary.  This must be done immediately
 	 * prior to VM-Enter, as the kernel may load a new ASID (PCID) any time
diff --git a/arch/x86/kvm/vmx/vmx.h b/arch/x86/kvm/vmx/vmx.h
index 1b80479505d3..5ddedf73392b 100644
--- a/arch/x86/kvm/vmx/vmx.h
+++ b/arch/x86/kvm/vmx/vmx.h
@@ -416,6 +416,8 @@ static inline void vmx_set_intercept_for_msr(struct kvm_vcpu *vcpu, u32 msr,
 
 void vmx_update_cpu_dirty_logging(struct kvm_vcpu *vcpu);
 u64 vmx_get_supported_debugctl(struct kvm_vcpu *vcpu, bool host_initiated);
+void vmx_guest_debugctl_write(struct kvm_vcpu *vcpu, u64 val);
+u64 vmx_guest_debugctl_read(void);
 
 /*
  * Note, early Intel manuals have the write-low and read-high bitmap offsets
diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
index 684b8047e0f2..a85078dfa36d 100644
--- a/arch/x86/kvm/x86.c
+++ b/arch/x86/kvm/x86.c
@@ -10752,7 +10752,7 @@ static int vcpu_enter_guest(struct kvm_vcpu *vcpu)
 		dm_request_for_irq_injection(vcpu) &&
 		kvm_cpu_accept_dm_intr(vcpu);
 	fastpath_t exit_fastpath;
-	u64 run_flags;
+	u64 run_flags, host_debug_ctl;
 
 	bool req_immediate_exit = false;
 
@@ -11024,7 +11024,10 @@ static int vcpu_enter_guest(struct kvm_vcpu *vcpu)
 		set_debugreg(0, 7);
 	}
 
-	vcpu->arch.host_debugctl = get_debugctlmsr();
+	host_debug_ctl = get_debugctlmsr();
+	if (host_debug_ctl != vcpu->arch.host_debugctl)
+		run_flags |= KVM_RUN_LOAD_DEBUGCTL;
+	vcpu->arch.host_debugctl = host_debug_ctl;
 
 	guest_timing_enter_irqoff();
 
-- 
2.46.0

Re: [PATCH v4 3/4] x86: nVMX: check vmcs12->guest_ia32_debugctl value given by L2

[Chao Gao]

On Wed, May 14, 2025 at 08:53:52PM -0400, Maxim Levitsky wrote:
>Check the vmcs12 guest_ia32_debugctl value before loading it, to avoid L2
>being able to load arbitrary values to hardware IA32_DEBUGCTL.
>
>Signed-off-by: Maxim Levitsky <mlevitsk@redhat.com>
>---
> arch/x86/kvm/vmx/nested.c | 4 ++++
> arch/x86/kvm/vmx/vmx.c    | 2 +-
> arch/x86/kvm/vmx/vmx.h    | 2 ++
> 3 files changed, 7 insertions(+), 1 deletion(-)
>
>diff --git a/arch/x86/kvm/vmx/nested.c b/arch/x86/kvm/vmx/nested.c
>index e073e3008b16..0bda6400e30a 100644
>--- a/arch/x86/kvm/vmx/nested.c
>+++ b/arch/x86/kvm/vmx/nested.c
>@@ -3193,6 +3193,10 @@ static int nested_vmx_check_guest_state(struct kvm_vcpu *vcpu,
> 	     CC((vmcs12->guest_bndcfgs & MSR_IA32_BNDCFGS_RSVD))))
> 		return -EINVAL;
> 
>+	if ((vmcs12->vm_entry_controls & VM_ENTRY_LOAD_DEBUG_CONTROLS) &&
>+	     CC(vmcs12->guest_ia32_debugctl & ~vmx_get_supported_debugctl(vcpu, false)))
>+		return -EINVAL;
>+

How about grouping this check with the one against DR7 a few lines above?

> 	if (nested_check_guest_non_reg_state(vmcs12))
> 		return -EINVAL;
> 
>diff --git a/arch/x86/kvm/vmx/vmx.c b/arch/x86/kvm/vmx/vmx.c
>index 9953de0cb32a..9046ee2e9a04 100644
>--- a/arch/x86/kvm/vmx/vmx.c
>+++ b/arch/x86/kvm/vmx/vmx.c
>@@ -2179,7 +2179,7 @@ static u64 nested_vmx_truncate_sysenter_addr(struct kvm_vcpu *vcpu,
> 	return (unsigned long)data;
> }
> 
>-static u64 vmx_get_supported_debugctl(struct kvm_vcpu *vcpu, bool host_initiated)
>+u64 vmx_get_supported_debugctl(struct kvm_vcpu *vcpu, bool host_initiated)
> {
> 	u64 debugctl = 0;
> 
>diff --git a/arch/x86/kvm/vmx/vmx.h b/arch/x86/kvm/vmx/vmx.h
>index 6d1e40ecc024..1b80479505d3 100644
>--- a/arch/x86/kvm/vmx/vmx.h
>+++ b/arch/x86/kvm/vmx/vmx.h
>@@ -413,7 +413,9 @@ static inline void vmx_set_intercept_for_msr(struct kvm_vcpu *vcpu, u32 msr,
> 		vmx_disable_intercept_for_msr(vcpu, msr, type);
> }
> 
>+

stray newline.

> void vmx_update_cpu_dirty_logging(struct kvm_vcpu *vcpu);
>+u64 vmx_get_supported_debugctl(struct kvm_vcpu *vcpu, bool host_initiated);
> 
> /*
>  * Note, early Intel manuals have the write-low and read-high bitmap offsets
>-- 
>2.46.0
>
>

Re: [PATCH v4 4/4] x86: KVM: VMX: preserve DEBUGCTLMSR_FREEZE_IN_SMM

[Chao Gao]

>@@ -7368,6 +7381,9 @@ fastpath_t vmx_vcpu_run(struct kvm_vcpu *vcpu, u64 run_flags)
> 	if (run_flags & KVM_RUN_LOAD_GUEST_DR6)
> 		set_debugreg(vcpu->arch.dr6, 6);
> 
>+	if (run_flags & KVM_RUN_LOAD_DEBUGCTL)
>+		vmx_guest_debugctl_write(vcpu, vmx_guest_debugctl_read());

...

>+
> 	/*
> 	 * Refresh vmcs.HOST_CR3 if necessary.  This must be done immediately
> 	 * prior to VM-Enter, as the kernel may load a new ASID (PCID) any time
>diff --git a/arch/x86/kvm/vmx/vmx.h b/arch/x86/kvm/vmx/vmx.h
>index 1b80479505d3..5ddedf73392b 100644
>--- a/arch/x86/kvm/vmx/vmx.h
>+++ b/arch/x86/kvm/vmx/vmx.h
>@@ -416,6 +416,8 @@ static inline void vmx_set_intercept_for_msr(struct kvm_vcpu *vcpu, u32 msr,
> 
> void vmx_update_cpu_dirty_logging(struct kvm_vcpu *vcpu);
> u64 vmx_get_supported_debugctl(struct kvm_vcpu *vcpu, bool host_initiated);
>+void vmx_guest_debugctl_write(struct kvm_vcpu *vcpu, u64 val);
>+u64 vmx_guest_debugctl_read(void);
> 
> /*
>  * Note, early Intel manuals have the write-low and read-high bitmap offsets
>diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
>index 684b8047e0f2..a85078dfa36d 100644
>--- a/arch/x86/kvm/x86.c
>+++ b/arch/x86/kvm/x86.c
>@@ -10752,7 +10752,7 @@ static int vcpu_enter_guest(struct kvm_vcpu *vcpu)
> 		dm_request_for_irq_injection(vcpu) &&
> 		kvm_cpu_accept_dm_intr(vcpu);
> 	fastpath_t exit_fastpath;
>-	u64 run_flags;
>+	u64 run_flags, host_debug_ctl;
> 
> 	bool req_immediate_exit = false;
> 
>@@ -11024,7 +11024,10 @@ static int vcpu_enter_guest(struct kvm_vcpu *vcpu)
> 		set_debugreg(0, 7);
> 	}
> 
>-	vcpu->arch.host_debugctl = get_debugctlmsr();
>+	host_debug_ctl = get_debugctlmsr();
>+	if (host_debug_ctl != vcpu->arch.host_debugctl)
>+		run_flags |= KVM_RUN_LOAD_DEBUGCTL;
>+	vcpu->arch.host_debugctl = host_debug_ctl;

IIUC, using run_flags here may only update the GUEST_DEBUGCTL field of a vmcs02,
leaving vmcs01 not updated.

> 
> 	guest_timing_enter_irqoff();
> 
>-- 
>2.46.0
>
>

Re: [PATCH v4 2/4] KVM: x86: Drop kvm_x86_ops.set_dr6() in favor of a new KVM_RUN flag

[Chao Gao]

>diff --git a/arch/x86/kvm/svm/svm.c b/arch/x86/kvm/svm/svm.c
>index c8b8a9947057..026b28051fff 100644
>--- a/arch/x86/kvm/svm/svm.c
>+++ b/arch/x86/kvm/svm/svm.c
>@@ -4308,10 +4308,13 @@ static __no_kcsan fastpath_t svm_vcpu_run(struct kvm_vcpu *vcpu, u64 run_flags)
> 	svm_hv_update_vp_id(svm->vmcb, vcpu);
> 
> 	/*
>-	 * Run with all-zero DR6 unless needed, so that we can get the exact cause
>-	 * of a #DB.
>+	 * Run with all-zero DR6 unless the guest can write DR6 freely, so that
>+	 * KVM can get the exact cause of a #DB.  Note, loading guest DR6 from
>+	 * KVM's snapshot is only necessary when DR accesses won't exit.
> 	 */
>-	if (likely(!(vcpu->arch.switch_db_regs & KVM_DEBUGREG_WONT_EXIT)))
>+	if (unlikely(run_flags & KVM_RUN_LOAD_GUEST_DR6))
>+		svm_set_dr6(vcpu, vcpu->arch.dr6);
>+	else if (likely(!(vcpu->arch.switch_db_regs & KVM_DEBUGREG_WONT_EXIT)))
> 		svm_set_dr6(vcpu, DR6_ACTIVE_LOW);

...

> void vmx_set_dr7(struct kvm_vcpu *vcpu, unsigned long val)
> {
> 	vmcs_writel(GUEST_DR7, val);
>@@ -7371,6 +7365,9 @@ fastpath_t vmx_vcpu_run(struct kvm_vcpu *vcpu, u64 run_flags)
> 		vmcs_writel(GUEST_RIP, vcpu->arch.regs[VCPU_REGS_RIP]);
> 	vcpu->arch.regs_dirty = 0;
> 
>+	if (run_flags & KVM_RUN_LOAD_GUEST_DR6)
>+		set_debugreg(vcpu->arch.dr6, 6);
>+
> 	/*
> 	 * Refresh vmcs.HOST_CR3 if necessary.  This must be done immediately
> 	 * prior to VM-Enter, as the kernel may load a new ASID (PCID) any time
>diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
>index 25de78cdab42..684b8047e0f2 100644
>--- a/arch/x86/kvm/x86.c
>+++ b/arch/x86/kvm/x86.c
>@@ -11019,7 +11019,7 @@ static int vcpu_enter_guest(struct kvm_vcpu *vcpu)
> 		set_debugreg(vcpu->arch.eff_db[3], 3);
> 		/* When KVM_DEBUGREG_WONT_EXIT, dr6 is accessible in guest. */
> 		if (unlikely(vcpu->arch.switch_db_regs & KVM_DEBUGREG_WONT_EXIT))
>-			kvm_x86_call(set_dr6)(vcpu, vcpu->arch.dr6);
>+			run_flags |= KVM_RUN_LOAD_GUEST_DR6;

The new KVM_RUN flag isn't necessary. Vendor code can directly check
switch_db_regs to decide whether to load the guest dr6 into hardware.

In svm_vcpu_run(), referencing both run_flags and switch_db_regs is confusing.
The following logic is much clearer:

	if (likely(!(vcpu->arch.switch_db_regs & KVM_DEBUGREG_WONT_EXIT)))
 		svm_set_dr6(vcpu, DR6_ACTIVE_LOW);
	else
		svm_set_dr6(vcpu, vcpu->arch.dr6);



> 	} else if (unlikely(hw_breakpoint_active())) {
> 		set_debugreg(0, 7);
> 	}
>-- 
>2.46.0
>
>

Re: [PATCH v4 2/4] KVM: x86: Drop kvm_x86_ops.set_dr6() in favor of a new KVM_RUN flag

[Sean Christopherson]

On Fri, May 16, 2025, Chao Gao wrote:
> >diff --git a/arch/x86/kvm/svm/svm.c b/arch/x86/kvm/svm/svm.c
> >index c8b8a9947057..026b28051fff 100644
> >--- a/arch/x86/kvm/svm/svm.c
> >+++ b/arch/x86/kvm/svm/svm.c
> >@@ -4308,10 +4308,13 @@ static __no_kcsan fastpath_t svm_vcpu_run(struct kvm_vcpu *vcpu, u64 run_flags)
> > 	svm_hv_update_vp_id(svm->vmcb, vcpu);
> > 
> > 	/*
> >-	 * Run with all-zero DR6 unless needed, so that we can get the exact cause
> >-	 * of a #DB.
> >+	 * Run with all-zero DR6 unless the guest can write DR6 freely, so that
> >+	 * KVM can get the exact cause of a #DB.  Note, loading guest DR6 from
> >+	 * KVM's snapshot is only necessary when DR accesses won't exit.
> > 	 */
> >-	if (likely(!(vcpu->arch.switch_db_regs & KVM_DEBUGREG_WONT_EXIT)))
> >+	if (unlikely(run_flags & KVM_RUN_LOAD_GUEST_DR6))
> >+		svm_set_dr6(vcpu, vcpu->arch.dr6);
> >+	else if (likely(!(vcpu->arch.switch_db_regs & KVM_DEBUGREG_WONT_EXIT)))
> > 		svm_set_dr6(vcpu, DR6_ACTIVE_LOW);
> 
> ...
> 
> > void vmx_set_dr7(struct kvm_vcpu *vcpu, unsigned long val)
> > {
> > 	vmcs_writel(GUEST_DR7, val);
> >@@ -7371,6 +7365,9 @@ fastpath_t vmx_vcpu_run(struct kvm_vcpu *vcpu, u64 run_flags)
> > 		vmcs_writel(GUEST_RIP, vcpu->arch.regs[VCPU_REGS_RIP]);
> > 	vcpu->arch.regs_dirty = 0;
> > 
> >+	if (run_flags & KVM_RUN_LOAD_GUEST_DR6)
> >+		set_debugreg(vcpu->arch.dr6, 6);
> >+
> > 	/*
> > 	 * Refresh vmcs.HOST_CR3 if necessary.  This must be done immediately
> > 	 * prior to VM-Enter, as the kernel may load a new ASID (PCID) any time
> >diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
> >index 25de78cdab42..684b8047e0f2 100644
> >--- a/arch/x86/kvm/x86.c
> >+++ b/arch/x86/kvm/x86.c
> >@@ -11019,7 +11019,7 @@ static int vcpu_enter_guest(struct kvm_vcpu *vcpu)
> > 		set_debugreg(vcpu->arch.eff_db[3], 3);
> > 		/* When KVM_DEBUGREG_WONT_EXIT, dr6 is accessible in guest. */
> > 		if (unlikely(vcpu->arch.switch_db_regs & KVM_DEBUGREG_WONT_EXIT))
> >-			kvm_x86_call(set_dr6)(vcpu, vcpu->arch.dr6);
> >+			run_flags |= KVM_RUN_LOAD_GUEST_DR6;
> 
> The new KVM_RUN flag isn't necessary. Vendor code can directly check
> switch_db_regs to decide whether to load the guest dr6 into hardware.

So thought Paolo, too.

> In svm_vcpu_run(), referencing both run_flags and switch_db_regs is confusing.
> The following logic is much clearer:
> 
> 	if (likely(!(vcpu->arch.switch_db_regs & KVM_DEBUGREG_WONT_EXIT)))
>  		svm_set_dr6(vcpu, DR6_ACTIVE_LOW);
> 	else
> 		svm_set_dr6(vcpu, vcpu->arch.dr6);

And wrong :-)  vcpu->arch.dr6 is stale on fastpasth reentry, see commit c2fee09fc167
("KVM: x86: Load DR6 with guest value only before entering .vcpu_run() loop").

Re: [PATCH v4 4/4] x86: KVM: VMX: preserve DEBUGCTLMSR_FREEZE_IN_SMM

[mlevitsk]

On Fri, 2025-05-16 at 11:39 +0800, Chao Gao wrote:
> > @@ -7368,6 +7381,9 @@ fastpath_t vmx_vcpu_run(struct kvm_vcpu *vcpu, u64 run_flags)
> > 	if (run_flags & KVM_RUN_LOAD_GUEST_DR6)
> > 		set_debugreg(vcpu->arch.dr6, 6);
> > 
> > +	if (run_flags & KVM_RUN_LOAD_DEBUGCTL)
> > +		vmx_guest_debugctl_write(vcpu, vmx_guest_debugctl_read());
> 
> ...
> 
> > +
> > 	/*
> > 	 * Refresh vmcs.HOST_CR3 if necessary.  This must be done immediately
> > 	 * prior to VM-Enter, as the kernel may load a new ASID (PCID) any time
> > diff --git a/arch/x86/kvm/vmx/vmx.h b/arch/x86/kvm/vmx/vmx.h
> > index 1b80479505d3..5ddedf73392b 100644
> > --- a/arch/x86/kvm/vmx/vmx.h
> > +++ b/arch/x86/kvm/vmx/vmx.h
> > @@ -416,6 +416,8 @@ static inline void vmx_set_intercept_for_msr(struct kvm_vcpu *vcpu, u32 msr,
> > 
> > void vmx_update_cpu_dirty_logging(struct kvm_vcpu *vcpu);
> > u64 vmx_get_supported_debugctl(struct kvm_vcpu *vcpu, bool host_initiated);
> > +void vmx_guest_debugctl_write(struct kvm_vcpu *vcpu, u64 val);
> > +u64 vmx_guest_debugctl_read(void);
> > 
> > /*
> >  * Note, early Intel manuals have the write-low and read-high bitmap offsets
> > diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
> > index 684b8047e0f2..a85078dfa36d 100644
> > --- a/arch/x86/kvm/x86.c
> > +++ b/arch/x86/kvm/x86.c
> > @@ -10752,7 +10752,7 @@ static int vcpu_enter_guest(struct kvm_vcpu *vcpu)
> > 		dm_request_for_irq_injection(vcpu) &&
> > 		kvm_cpu_accept_dm_intr(vcpu);
> > 	fastpath_t exit_fastpath;
> > -	u64 run_flags;
> > +	u64 run_flags, host_debug_ctl;
> > 
> > 	bool req_immediate_exit = false;
> > 
> > @@ -11024,7 +11024,10 @@ static int vcpu_enter_guest(struct kvm_vcpu *vcpu)
> > 		set_debugreg(0, 7);
> > 	}
> > 
> > -	vcpu->arch.host_debugctl = get_debugctlmsr();
> > +	host_debug_ctl = get_debugctlmsr();
> > +	if (host_debug_ctl != vcpu->arch.host_debugctl)
> > +		run_flags |= KVM_RUN_LOAD_DEBUGCTL;
> > +	vcpu->arch.host_debugctl = host_debug_ctl;
> 
> IIUC, using run_flags here may only update the GUEST_DEBUGCTL field of a vmcs02,
> leaving vmcs01 not updated.

Hi,

Thankfully this should not be a problem because when KVM exits from vmcs02 to vmcs01 the
IA32_DEBUGCTL is reset to 0. 

If I’m not mistaken, this always happens regardless of the VM_ENTRY_LOAD_DEBUG_CONTROLS.

Now since I wrapped the write of the with a helper function (vmx_guest_debugctl_write),
it should pick up the new value.


Best regards,
	Maxim Levitsky

> 
> > 
> > 	guest_timing_enter_irqoff();
> > 
> > -- 
> > 2.46.0
> > 
> > 
> 

Re: [PATCH v4 3/4] x86: nVMX: check vmcs12->guest_ia32_debugctl value given by L2

[mlevitsk]

On Fri, 2025-05-16 at 11:31 +0800, Chao Gao wrote:
> On Wed, May 14, 2025 at 08:53:52PM -0400, Maxim Levitsky wrote:
> > Check the vmcs12 guest_ia32_debugctl value before loading it, to avoid L2
> > being able to load arbitrary values to hardware IA32_DEBUGCTL.
> > 
> > Signed-off-by: Maxim Levitsky <mlevitsk@redhat.com>
> > ---
> > arch/x86/kvm/vmx/nested.c | 4 ++++
> > arch/x86/kvm/vmx/vmx.c    | 2 +-
> > arch/x86/kvm/vmx/vmx.h    | 2 ++
> > 3 files changed, 7 insertions(+), 1 deletion(-)
> > 
> > diff --git a/arch/x86/kvm/vmx/nested.c b/arch/x86/kvm/vmx/nested.c
> > index e073e3008b16..0bda6400e30a 100644
> > --- a/arch/x86/kvm/vmx/nested.c
> > +++ b/arch/x86/kvm/vmx/nested.c
> > @@ -3193,6 +3193,10 @@ static int nested_vmx_check_guest_state(struct kvm_vcpu *vcpu,
> > 	     CC((vmcs12->guest_bndcfgs & MSR_IA32_BNDCFGS_RSVD))))
> > 		return -EINVAL;
> > 
> > +	if ((vmcs12->vm_entry_controls & VM_ENTRY_LOAD_DEBUG_CONTROLS) &&
> > +	     CC(vmcs12->guest_ia32_debugctl & ~vmx_get_supported_debugctl(vcpu, false)))
> > +		return -EINVAL;
> > +
> 
> How about grouping this check with the one against DR7 a few lines above?

Good idea, will do.
> 
> > 	if (nested_check_guest_non_reg_state(vmcs12))
> > 		return -EINVAL;
> > 
> > diff --git a/arch/x86/kvm/vmx/vmx.c b/arch/x86/kvm/vmx/vmx.c
> > index 9953de0cb32a..9046ee2e9a04 100644
> > --- a/arch/x86/kvm/vmx/vmx.c
> > +++ b/arch/x86/kvm/vmx/vmx.c
> > @@ -2179,7 +2179,7 @@ static u64 nested_vmx_truncate_sysenter_addr(struct kvm_vcpu *vcpu,
> > 	return (unsigned long)data;
> > }
> > 
> > -static u64 vmx_get_supported_debugctl(struct kvm_vcpu *vcpu, bool host_initiated)
> > +u64 vmx_get_supported_debugctl(struct kvm_vcpu *vcpu, bool host_initiated)
> > {
> > 	u64 debugctl = 0;
> > 
> > diff --git a/arch/x86/kvm/vmx/vmx.h b/arch/x86/kvm/vmx/vmx.h
> > index 6d1e40ecc024..1b80479505d3 100644
> > --- a/arch/x86/kvm/vmx/vmx.h
> > +++ b/arch/x86/kvm/vmx/vmx.h
> > @@ -413,7 +413,9 @@ static inline void vmx_set_intercept_for_msr(struct kvm_vcpu *vcpu, u32 msr,
> > 		vmx_disable_intercept_for_msr(vcpu, msr, type);
> > }
> > 
> > +
> 
> stray newline.
> 
> > void vmx_update_cpu_dirty_logging(struct kvm_vcpu *vcpu);
> > +u64 vmx_get_supported_debugctl(struct kvm_vcpu *vcpu, bool host_initiated);
> > 
> > /*
> >  * Note, early Intel manuals have the write-low and read-high bitmap offsets
> > -- 
> > 2.46.0
> > 
> > 
> 

Best regards,
	Maxim Levitsky

Re: [PATCH v4 3/4] x86: nVMX: check vmcs12->guest_ia32_debugctl value given by L2

[mlevitsk]

On Fri, 2025-05-16 at 10:50 -0400, mlevitsk@redhat.com wrote:
> On Fri, 2025-05-16 at 11:31 +0800, Chao Gao wrote:
> > On Wed, May 14, 2025 at 08:53:52PM -0400, Maxim Levitsky wrote:
> > > Check the vmcs12 guest_ia32_debugctl value before loading it, to avoid L2
> > > being able to load arbitrary values to hardware IA32_DEBUGCTL.
> > > 
> > > Signed-off-by: Maxim Levitsky <mlevitsk@redhat.com>
> > > ---
> > > arch/x86/kvm/vmx/nested.c | 4 ++++
> > > arch/x86/kvm/vmx/vmx.c    | 2 +-
> > > arch/x86/kvm/vmx/vmx.h    | 2 ++
> > > 3 files changed, 7 insertions(+), 1 deletion(-)
> > > 
> > > diff --git a/arch/x86/kvm/vmx/nested.c b/arch/x86/kvm/vmx/nested.c
> > > index e073e3008b16..0bda6400e30a 100644
> > > --- a/arch/x86/kvm/vmx/nested.c
> > > +++ b/arch/x86/kvm/vmx/nested.c
> > > @@ -3193,6 +3193,10 @@ static int nested_vmx_check_guest_state(struct kvm_vcpu *vcpu,
> > > 	     CC((vmcs12->guest_bndcfgs & MSR_IA32_BNDCFGS_RSVD))))
> > > 		return -EINVAL;
> > > 
> > > +	if ((vmcs12->vm_entry_controls & VM_ENTRY_LOAD_DEBUG_CONTROLS) &&
> > > +	     CC(vmcs12->guest_ia32_debugctl & ~vmx_get_supported_debugctl(vcpu, false)))
> > > +		return -EINVAL;
> > > +
> > 
> > How about grouping this check with the one against DR7 a few lines above?
> 
> Good idea, will do.

Besides the above change, is there anything else to change in this patchset?
If not I'll sent a new version soon.

Thanks,
Best regards,
	Maxim Levitsky




> > 
> > > 	if (nested_check_guest_non_reg_state(vmcs12))
> > > 		return -EINVAL;
> > > 
> > > diff --git a/arch/x86/kvm/vmx/vmx.c b/arch/x86/kvm/vmx/vmx.c
> > > index 9953de0cb32a..9046ee2e9a04 100644
> > > --- a/arch/x86/kvm/vmx/vmx.c
> > > +++ b/arch/x86/kvm/vmx/vmx.c
> > > @@ -2179,7 +2179,7 @@ static u64 nested_vmx_truncate_sysenter_addr(struct kvm_vcpu *vcpu,
> > > 	return (unsigned long)data;
> > > }
> > > 
> > > -static u64 vmx_get_supported_debugctl(struct kvm_vcpu *vcpu, bool host_initiated)
> > > +u64 vmx_get_supported_debugctl(struct kvm_vcpu *vcpu, bool host_initiated)
> > > {
> > > 	u64 debugctl = 0;
> > > 
> > > diff --git a/arch/x86/kvm/vmx/vmx.h b/arch/x86/kvm/vmx/vmx.h
> > > index 6d1e40ecc024..1b80479505d3 100644
> > > --- a/arch/x86/kvm/vmx/vmx.h
> > > +++ b/arch/x86/kvm/vmx/vmx.h
> > > @@ -413,7 +413,9 @@ static inline void vmx_set_intercept_for_msr(struct kvm_vcpu *vcpu, u32 msr,
> > > 		vmx_disable_intercept_for_msr(vcpu, msr, type);
> > > }
> > > 
> > > +
> > 
> > stray newline.
> > 
> > > void vmx_update_cpu_dirty_logging(struct kvm_vcpu *vcpu);
> > > +u64 vmx_get_supported_debugctl(struct kvm_vcpu *vcpu, bool host_initiated);
> > > 
> > > /*
> > >  * Note, early Intel manuals have the write-low and read-high bitmap offsets
> > > -- 
> > > 2.46.0
> > > 
> > > 
> > 
> 
> Best regards,
> 	Maxim Levitsky

Re: [PATCH v4 3/4] x86: nVMX: check vmcs12->guest_ia32_debugctl value given by L2

[Sean Christopherson]

"KVM: nVMX:" for the shortlog scope.

Re: [PATCH v4 4/4] x86: KVM: VMX: preserve DEBUGCTLMSR_FREEZE_IN_SMM

[Sean Christopherson]

KVM: VMX:

On Wed, May 14, 2025, Maxim Levitsky wrote:
> Pass through the host's DEBUGCTL.DEBUGCTLMSR_FREEZE_IN_SMM to the guest
> GUEST_IA32_DEBUGCTL without the guest seeing this value.
> 
> Since the value of the host DEBUGCTL can in theory change between VM runs,
> check if has changed, and if yes, then reload the GUEST_IA32_DEBUGCTL with
> the new value.

Please split this into two patches.  Add vmx_guest_debugctl_{read,write}(), then
land the FREEZE_IN_SMM change on top.  Adding the helpers should be a nop and
thus trivial to review, and similarly the DEBUGCTLMSR_FREEZE_IN_SMM change is
actually pretty small.  But combined, this patch is annoying to review because
there's a lot of uninteresting diff to wade through to get at the FREEZE_IN_SMM
logic.

> Signed-off-by: Maxim Levitsky <mlevitsk@redhat.com>
> ---
>  arch/x86/include/asm/kvm_host.h |  1 +
>  arch/x86/kvm/vmx/nested.c       |  4 ++--
>  arch/x86/kvm/vmx/vmx.c          | 22 +++++++++++++++++++---
>  arch/x86/kvm/vmx/vmx.h          |  2 ++
>  arch/x86/kvm/x86.c              |  7 +++++--
>  5 files changed, 29 insertions(+), 7 deletions(-)
> 
> diff --git a/arch/x86/include/asm/kvm_host.h b/arch/x86/include/asm/kvm_host.h
> index d2ad31a1628e..2e7e4a8b392e 100644
> --- a/arch/x86/include/asm/kvm_host.h
> +++ b/arch/x86/include/asm/kvm_host.h
> @@ -1673,6 +1673,7 @@ static inline u16 kvm_lapic_irq_dest_mode(bool dest_mode_logical)
>  enum kvm_x86_run_flags {
>  	KVM_RUN_FORCE_IMMEDIATE_EXIT	= BIT(0),
>  	KVM_RUN_LOAD_GUEST_DR6		= BIT(1),
> +	KVM_RUN_LOAD_DEBUGCTL		= BIT(2),
>  };
>  
>  struct kvm_x86_ops {

...

> @@ -7368,6 +7381,9 @@ fastpath_t vmx_vcpu_run(struct kvm_vcpu *vcpu, u64 run_flags)
>  	if (run_flags & KVM_RUN_LOAD_GUEST_DR6)
>  		set_debugreg(vcpu->arch.dr6, 6);
>  
> +	if (run_flags & KVM_RUN_LOAD_DEBUGCTL)
> +		vmx_guest_debugctl_write(vcpu, vmx_guest_debugctl_read());
> +
>  	/*
>  	 * Refresh vmcs.HOST_CR3 if necessary.  This must be done immediately
>  	 * prior to VM-Enter, as the kernel may load a new ASID (PCID) any time
> diff --git a/arch/x86/kvm/vmx/vmx.h b/arch/x86/kvm/vmx/vmx.h
> index 1b80479505d3..5ddedf73392b 100644
> --- a/arch/x86/kvm/vmx/vmx.h
> +++ b/arch/x86/kvm/vmx/vmx.h
> @@ -416,6 +416,8 @@ static inline void vmx_set_intercept_for_msr(struct kvm_vcpu *vcpu, u32 msr,
>  
>  void vmx_update_cpu_dirty_logging(struct kvm_vcpu *vcpu);
>  u64 vmx_get_supported_debugctl(struct kvm_vcpu *vcpu, bool host_initiated);
> +void vmx_guest_debugctl_write(struct kvm_vcpu *vcpu, u64 val);
> +u64 vmx_guest_debugctl_read(void);

I vote to make these static inlines, I don't see any reason to bury them in vmx.c

>  /*
>   * Note, early Intel manuals have the write-low and read-high bitmap offsets
> diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
> index 684b8047e0f2..a85078dfa36d 100644
> --- a/arch/x86/kvm/x86.c
> +++ b/arch/x86/kvm/x86.c
> @@ -10752,7 +10752,7 @@ static int vcpu_enter_guest(struct kvm_vcpu *vcpu)
>  		dm_request_for_irq_injection(vcpu) &&
>  		kvm_cpu_accept_dm_intr(vcpu);
>  	fastpath_t exit_fastpath;
> -	u64 run_flags;
> +	u64 run_flags, host_debug_ctl;
>  
>  	bool req_immediate_exit = false;
>  
> @@ -11024,7 +11024,10 @@ static int vcpu_enter_guest(struct kvm_vcpu *vcpu)
>  		set_debugreg(0, 7);
>  	}
>  
> -	vcpu->arch.host_debugctl = get_debugctlmsr();
> +	host_debug_ctl = get_debugctlmsr();

This can probably just be debug_ctl to shorten the lines, I don't see a strong
need to clarify it's the host's value since all accesses are clustered together.

> +	if (host_debug_ctl != vcpu->arch.host_debugctl)
> +		run_flags |= KVM_RUN_LOAD_DEBUGCTL;
> +	vcpu->arch.host_debugctl = host_debug_ctl;

Argh, the TDX series didn't get refreshed (or maybe it got poorly rebased), and
now there's a redundant and confusing "host_debugctlmsr" field in vcpu_vt.  Can
you slot in the below?  It's not urgent enough to warrant posting separately,
and handling TDX in this series would get a bit wonky if TDX uses a different
snapshot.

The reason I say that TDX will get wonky is also why I think the "are bits
changing?" check in x86.c needs to be precise.  KVM_RUN_LOAD_DEBUGCTL should
*never* be set for TDX and SVM, and so they should WARN instead of silently
doing nothing.  But to do that without generating false positives, the common
check needs to be precise.

I was going to say we could throw a mask in kvm_x86_ops, but TDX throws a wrench
in that idea.  Aha!  Actually, we can still use kvm_x86_ops.  TDX can be exempted
via guest_state_protected.  E.g. in common x86:

	debug_ctl = get_debugctlmsr();
	if (((debug_ctl ^ vcpu->arch.host_debugctl) & kvm_x86_ops.HOST_DEBUGCTL_MASK) &&
	    !vcpu->arch.guest_state_protected)
		run_flags |= KVM_RUN_LOAD_DEBUGCTL;
	vcpu->arch.host_debugctl = debug_ctl;

--
From: Sean Christopherson <seanjc@google.com>
Date: Tue, 20 May 2025 15:37:41 -0700
Subject: [PATCH] KVM: TDX: Use kvm_arch_vcpu.host_debugctl to restore the
 host's DEBUGCTL

Use the kvm_arch_vcpu.host_debugctl snapshot to restore DEBUGCTL after
running a TD vCPU.  The final TDX series rebase was mishandled, likely due
to commit fb71c7959356 ("KVM: x86: Snapshot the host's DEBUGCTL in common
x86") deleting the same line of code from vmx.h, i.e. creating a semantic
conflict of sorts, but no syntactic conflict.

Using the version in kvm_vcpu_arch picks up the ulong => u64 fix (which
isn't relevant to TDX) as well as the IRQ fix from commit 189ecdb3e112
("KVM: x86: Snapshot the host's DEBUGCTL after disabling IRQs").

Link: https://lore.kernel.org/all/20250307212053.2948340-10-pbonzini@redhat.com
Cc: Adrian Hunter <adrian.hunter@intel.com>
Fixes: 8af099037527 ("KVM: TDX: Save and restore IA32_DEBUGCTL")
Signed-off-by: Sean Christopherson <seanjc@google.com>
---
 arch/x86/kvm/vmx/common.h | 2 --
 arch/x86/kvm/vmx/tdx.c    | 6 ++----
 2 files changed, 2 insertions(+), 6 deletions(-)

diff --git a/arch/x86/kvm/vmx/common.h b/arch/x86/kvm/vmx/common.h
index 8f46a06e2c44..66454bead202 100644
--- a/arch/x86/kvm/vmx/common.h
+++ b/arch/x86/kvm/vmx/common.h
@@ -53,8 +53,6 @@ struct vcpu_vt {
 #ifdef CONFIG_X86_64
 	u64		msr_host_kernel_gs_base;
 #endif
-
-	unsigned long	host_debugctlmsr;
 };
 
 #ifdef CONFIG_KVM_INTEL_TDX
diff --git a/arch/x86/kvm/vmx/tdx.c b/arch/x86/kvm/vmx/tdx.c
index 7dbfad28debc..84b2922b8119 100644
--- a/arch/x86/kvm/vmx/tdx.c
+++ b/arch/x86/kvm/vmx/tdx.c
@@ -778,8 +778,6 @@ void tdx_prepare_switch_to_guest(struct kvm_vcpu *vcpu)
 	else
 		vt->msr_host_kernel_gs_base = read_msr(MSR_KERNEL_GS_BASE);
 
-	vt->host_debugctlmsr = get_debugctlmsr();
-
 	vt->guest_state_loaded = true;
 }
 
@@ -1056,8 +1054,8 @@ fastpath_t tdx_vcpu_run(struct kvm_vcpu *vcpu, u64 run_flags)
 
 	tdx_vcpu_enter_exit(vcpu);
 
-	if (vt->host_debugctlmsr & ~TDX_DEBUGCTL_PRESERVED)
-		update_debugctlmsr(vt->host_debugctlmsr);
+	if (vcpu->arch.host_debugctl & ~TDX_DEBUGCTL_PRESERVED)
+		update_debugctlmsr(vcpu->arch.host_debugctl);
 
 	tdx_load_host_xsave_state(vcpu);
 	tdx->guest_entered = true;

base-commit: 475a02020ac2de6b10e85de75e79833139b556e0
--

Re: [PATCH v4 3/4] x86: nVMX: check vmcs12->guest_ia32_debugctl value given by L2

[Chao Gao]

On Tue, May 20, 2025 at 05:48:44PM -0400, mlevitsk@redhat.com wrote:
>On Fri, 2025-05-16 at 10:50 -0400, mlevitsk@redhat.com wrote:
>> On Fri, 2025-05-16 at 11:31 +0800, Chao Gao wrote:
>> > On Wed, May 14, 2025 at 08:53:52PM -0400, Maxim Levitsky wrote:
>> > > Check the vmcs12 guest_ia32_debugctl value before loading it, to avoid L2
>> > > being able to load arbitrary values to hardware IA32_DEBUGCTL.
>> > > 
>> > > Signed-off-by: Maxim Levitsky <mlevitsk@redhat.com>
>> > > ---
>> > > arch/x86/kvm/vmx/nested.c | 4 ++++
>> > > arch/x86/kvm/vmx/vmx.c    | 2 +-
>> > > arch/x86/kvm/vmx/vmx.h    | 2 ++
>> > > 3 files changed, 7 insertions(+), 1 deletion(-)
>> > > 
>> > > diff --git a/arch/x86/kvm/vmx/nested.c b/arch/x86/kvm/vmx/nested.c
>> > > index e073e3008b16..0bda6400e30a 100644
>> > > --- a/arch/x86/kvm/vmx/nested.c
>> > > +++ b/arch/x86/kvm/vmx/nested.c
>> > > @@ -3193,6 +3193,10 @@ static int nested_vmx_check_guest_state(struct kvm_vcpu *vcpu,
>> > > 	     CC((vmcs12->guest_bndcfgs & MSR_IA32_BNDCFGS_RSVD))))
>> > > 		return -EINVAL;
>> > > 
>> > > +	if ((vmcs12->vm_entry_controls & VM_ENTRY_LOAD_DEBUG_CONTROLS) &&
>> > > +	     CC(vmcs12->guest_ia32_debugctl & ~vmx_get_supported_debugctl(vcpu, false)))
>> > > +		return -EINVAL;
>> > > +
>> > 
>> > How about grouping this check with the one against DR7 a few lines above?
>> 
>> Good idea, will do.
>
>Besides the above change, is there anything else to change in this patchset?
>If not I'll sent a new version soon.

...

>> > > diff --git a/arch/x86/kvm/vmx/vmx.h b/arch/x86/kvm/vmx/vmx.h
>> > > index 6d1e40ecc024..1b80479505d3 100644
>> > > --- a/arch/x86/kvm/vmx/vmx.h
>> > > +++ b/arch/x86/kvm/vmx/vmx.h
>> > > @@ -413,7 +413,9 @@ static inline void vmx_set_intercept_for_msr(struct kvm_vcpu *vcpu, u32 msr,
>> > > 		vmx_disable_intercept_for_msr(vcpu, msr, type);
>> > > }
>> > > 
>> > > +
>> > 
>> > stray newline.

Can you remove this newline? (not sure if you've already noticed this)

Also, the shortlogs for patches 3-4 don't follow the convention. They should be
"KVM: nVMX" and "KVM: VMX". With these fixed,

Reviewed-by: Chao Gao <chao.gao@intel.com>

Re: [PATCH v4 3/4] x86: nVMX: check vmcs12->guest_ia32_debugctl value given by L2

[mlevitsk]

On Wed, 2025-05-21 at 08:32 +0800, Chao Gao wrote:
> On Tue, May 20, 2025 at 05:48:44PM -0400, mlevitsk@redhat.com wrote:
> > On Fri, 2025-05-16 at 10:50 -0400, mlevitsk@redhat.com wrote:
> > > On Fri, 2025-05-16 at 11:31 +0800, Chao Gao wrote:
> > > > On Wed, May 14, 2025 at 08:53:52PM -0400, Maxim Levitsky wrote:
> > > > > Check the vmcs12 guest_ia32_debugctl value before loading it, to avoid L2
> > > > > being able to load arbitrary values to hardware IA32_DEBUGCTL.
> > > > > 
> > > > > Signed-off-by: Maxim Levitsky <mlevitsk@redhat.com>
> > > > > ---
> > > > > arch/x86/kvm/vmx/nested.c | 4 ++++
> > > > > arch/x86/kvm/vmx/vmx.c    | 2 +-
> > > > > arch/x86/kvm/vmx/vmx.h    | 2 ++
> > > > > 3 files changed, 7 insertions(+), 1 deletion(-)
> > > > > 
> > > > > diff --git a/arch/x86/kvm/vmx/nested.c b/arch/x86/kvm/vmx/nested.c
> > > > > index e073e3008b16..0bda6400e30a 100644
> > > > > --- a/arch/x86/kvm/vmx/nested.c
> > > > > +++ b/arch/x86/kvm/vmx/nested.c
> > > > > @@ -3193,6 +3193,10 @@ static int nested_vmx_check_guest_state(struct kvm_vcpu *vcpu,
> > > > > 	     CC((vmcs12->guest_bndcfgs & MSR_IA32_BNDCFGS_RSVD))))
> > > > > 		return -EINVAL;
> > > > > 
> > > > > +	if ((vmcs12->vm_entry_controls & VM_ENTRY_LOAD_DEBUG_CONTROLS) &&
> > > > > +	     CC(vmcs12->guest_ia32_debugctl & ~vmx_get_supported_debugctl(vcpu, false)))
> > > > > +		return -EINVAL;
> > > > > +
> > > > 
> > > > How about grouping this check with the one against DR7 a few lines above?
> > > 
> > > Good idea, will do.
> > 
> > Besides the above change, is there anything else to change in this patchset?
> > If not I'll sent a new version soon.
> 
> ...
> 
> > > > > diff --git a/arch/x86/kvm/vmx/vmx.h b/arch/x86/kvm/vmx/vmx.h
> > > > > index 6d1e40ecc024..1b80479505d3 100644
> > > > > --- a/arch/x86/kvm/vmx/vmx.h
> > > > > +++ b/arch/x86/kvm/vmx/vmx.h
> > > > > @@ -413,7 +413,9 @@ static inline void vmx_set_intercept_for_msr(struct kvm_vcpu *vcpu, u32 msr,
> > > > > 		vmx_disable_intercept_for_msr(vcpu, msr, type);
> > > > > }
> > > > > 
> > > > > +
> > > > 
> > > > stray newline.
> 
> Can you remove this newline? (not sure if you've already noticed this)
Sure thing!

> 
> Also, the shortlogs for patches 3-4 don't follow the convention. They should be
> "KVM: nVMX" and "KVM: VMX". With these fixed,

Sure thing - I wish it was more consistent but you are right. 



> 
> Reviewed-by: Chao Gao <chao.gao@intel.com>
> 

Thank you very much!
Best regards,
	Maxim Levitsky

Re: [PATCH v4 4/4] x86: KVM: VMX: preserve DEBUGCTLMSR_FREEZE_IN_SMM

[mlevitsk]

On Tue, 2025-05-20 at 15:57 -0700, Sean Christopherson wrote:
> KVM: VMX:
> 
> On Wed, May 14, 2025, Maxim Levitsky wrote:
> > Pass through the host's DEBUGCTL.DEBUGCTLMSR_FREEZE_IN_SMM to the guest
> > GUEST_IA32_DEBUGCTL without the guest seeing this value.
> > 
> > Since the value of the host DEBUGCTL can in theory change between VM runs,
> > check if has changed, and if yes, then reload the GUEST_IA32_DEBUGCTL with
> > the new value.
> 
> Please split this into two patches.  Add vmx_guest_debugctl_{read,write}(), then
> land the FREEZE_IN_SMM change on top.  Adding the helpers should be a nop and
> thus trivial to review, and similarly the DEBUGCTLMSR_FREEZE_IN_SMM change is
> actually pretty small.  But combined, this patch is annoying to review because
> there's a lot of uninteresting diff to wade through to get at the FREEZE_IN_SMM
> logic.

No problem.

> 
> > Signed-off-by: Maxim Levitsky <mlevitsk@redhat.com>
> > ---
> >  arch/x86/include/asm/kvm_host.h |  1 +
> >  arch/x86/kvm/vmx/nested.c       |  4 ++--
> >  arch/x86/kvm/vmx/vmx.c          | 22 +++++++++++++++++++---
> >  arch/x86/kvm/vmx/vmx.h          |  2 ++
> >  arch/x86/kvm/x86.c              |  7 +++++--
> >  5 files changed, 29 insertions(+), 7 deletions(-)
> > 
> > diff --git a/arch/x86/include/asm/kvm_host.h b/arch/x86/include/asm/kvm_host.h
> > index d2ad31a1628e..2e7e4a8b392e 100644
> > --- a/arch/x86/include/asm/kvm_host.h
> > +++ b/arch/x86/include/asm/kvm_host.h
> > @@ -1673,6 +1673,7 @@ static inline u16 kvm_lapic_irq_dest_mode(bool dest_mode_logical)
> >  enum kvm_x86_run_flags {
> >  	KVM_RUN_FORCE_IMMEDIATE_EXIT	= BIT(0),
> >  	KVM_RUN_LOAD_GUEST_DR6		= BIT(1),
> > +	KVM_RUN_LOAD_DEBUGCTL		= BIT(2),
> >  };
> >  
> >  struct kvm_x86_ops {
> 
> ...
> 
> > @@ -7368,6 +7381,9 @@ fastpath_t vmx_vcpu_run(struct kvm_vcpu *vcpu, u64 run_flags)
> >  	if (run_flags & KVM_RUN_LOAD_GUEST_DR6)
> >  		set_debugreg(vcpu->arch.dr6, 6);
> >  
> > +	if (run_flags & KVM_RUN_LOAD_DEBUGCTL)
> > +		vmx_guest_debugctl_write(vcpu, vmx_guest_debugctl_read());
> > +
> >  	/*
> >  	 * Refresh vmcs.HOST_CR3 if necessary.  This must be done immediately
> >  	 * prior to VM-Enter, as the kernel may load a new ASID (PCID) any time
> > diff --git a/arch/x86/kvm/vmx/vmx.h b/arch/x86/kvm/vmx/vmx.h
> > index 1b80479505d3..5ddedf73392b 100644
> > --- a/arch/x86/kvm/vmx/vmx.h
> > +++ b/arch/x86/kvm/vmx/vmx.h
> > @@ -416,6 +416,8 @@ static inline void vmx_set_intercept_for_msr(struct kvm_vcpu *vcpu, u32 msr,
> >  
> >  void vmx_update_cpu_dirty_logging(struct kvm_vcpu *vcpu);
> >  u64 vmx_get_supported_debugctl(struct kvm_vcpu *vcpu, bool host_initiated);
> > +void vmx_guest_debugctl_write(struct kvm_vcpu *vcpu, u64 val);
> > +u64 vmx_guest_debugctl_read(void);
> 
> I vote to make these static inlines, I don't see any reason to bury them in vmx.c

No problem as well.

> 
> >  /*
> >   * Note, early Intel manuals have the write-low and read-high bitmap offsets
> > diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
> > index 684b8047e0f2..a85078dfa36d 100644
> > --- a/arch/x86/kvm/x86.c
> > +++ b/arch/x86/kvm/x86.c
> > @@ -10752,7 +10752,7 @@ static int vcpu_enter_guest(struct kvm_vcpu *vcpu)
> >  		dm_request_for_irq_injection(vcpu) &&
> >  		kvm_cpu_accept_dm_intr(vcpu);
> >  	fastpath_t exit_fastpath;
> > -	u64 run_flags;
> > +	u64 run_flags, host_debug_ctl;
> >  
> >  	bool req_immediate_exit = false;
> >  
> > @@ -11024,7 +11024,10 @@ static int vcpu_enter_guest(struct kvm_vcpu *vcpu)
> >  		set_debugreg(0, 7);
> >  	}
> >  
> > -	vcpu->arch.host_debugctl = get_debugctlmsr();
> > +	host_debug_ctl = get_debugctlmsr();
> 
> This can probably just be debug_ctl to shorten the lines, I don't see a strong
> need to clarify it's the host's value since all accesses are clustered together.
> 
> > +	if (host_debug_ctl != vcpu->arch.host_debugctl)
> > +		run_flags |= KVM_RUN_LOAD_DEBUGCTL;
> > +	vcpu->arch.host_debugctl = host_debug_ctl;
> 
> Argh, the TDX series didn't get refreshed (or maybe it got poorly rebased), and
> now there's a redundant and confusing "host_debugctlmsr" field in vcpu_vt.  Can
> you slot in the below?  It's not urgent enough to warrant posting separately,
> and handling TDX in this series would get a bit wonky if TDX uses a different
> snapshot.
> 
> The reason I say that TDX will get wonky is also why I think the "are bits
> changing?" check in x86.c needs to be precise.  KVM_RUN_LOAD_DEBUGCTL should
> *never* be set for TDX and SVM, and so they should WARN instead of silently
> doing nothing.  But to do that without generating false positives, the common
> check needs to be precise.
> 
> I was going to say we could throw a mask in kvm_x86_ops, but TDX throws a wrench
> in that idea.  Aha!  Actually, we can still use kvm_x86_ops.  TDX can be exempted
> via guest_state_protected.  E.g. in common x86:
> 
> 	debug_ctl = get_debugctlmsr();
> 	if (((debug_ctl ^ vcpu->arch.host_debugctl) & kvm_x86_ops.HOST_DEBUGCTL_MASK) &&
> 	    !vcpu->arch.guest_state_protected)
> 		run_flags |= KVM_RUN_LOAD_DEBUGCTL;
> 	vcpu->arch.host_debugctl = debug_ctl;

Makes sense. I need to remember that TDX has arrived so I have to take it
in the account from now on.

All right, thanks for the review, I will post a new patch series soon.


Best regards,
	Maxim Levitsky

> 
> --
> From: Sean Christopherson <seanjc@google.com>
> Date: Tue, 20 May 2025 15:37:41 -0700
> Subject: [PATCH] KVM: TDX: Use kvm_arch_vcpu.host_debugctl to restore the
>  host's DEBUGCTL
> 
> Use the kvm_arch_vcpu.host_debugctl snapshot to restore DEBUGCTL after
> running a TD vCPU.  The final TDX series rebase was mishandled, likely due
> to commit fb71c7959356 ("KVM: x86: Snapshot the host's DEBUGCTL in common
> x86") deleting the same line of code from vmx.h, i.e. creating a semantic
> conflict of sorts, but no syntactic conflict.
> 
> Using the version in kvm_vcpu_arch picks up the ulong => u64 fix (which
> isn't relevant to TDX) as well as the IRQ fix from commit 189ecdb3e112
> ("KVM: x86: Snapshot the host's DEBUGCTL after disabling IRQs").
> 
> Link: https://lore.kernel.org/all/20250307212053.2948340-10-pbonzini@redhat.com
> Cc: Adrian Hunter <adrian.hunter@intel.com>
> Fixes: 8af099037527 ("KVM: TDX: Save and restore IA32_DEBUGCTL")
> Signed-off-by: Sean Christopherson <seanjc@google.com>
> ---
>  arch/x86/kvm/vmx/common.h | 2 --
>  arch/x86/kvm/vmx/tdx.c    | 6 ++----
>  2 files changed, 2 insertions(+), 6 deletions(-)
> 
> diff --git a/arch/x86/kvm/vmx/common.h b/arch/x86/kvm/vmx/common.h
> index 8f46a06e2c44..66454bead202 100644
> --- a/arch/x86/kvm/vmx/common.h
> +++ b/arch/x86/kvm/vmx/common.h
> @@ -53,8 +53,6 @@ struct vcpu_vt {
>  #ifdef CONFIG_X86_64
>  	u64		msr_host_kernel_gs_base;
>  #endif
> -
> -	unsigned long	host_debugctlmsr;
>  };
>  
>  #ifdef CONFIG_KVM_INTEL_TDX
> diff --git a/arch/x86/kvm/vmx/tdx.c b/arch/x86/kvm/vmx/tdx.c
> index 7dbfad28debc..84b2922b8119 100644
> --- a/arch/x86/kvm/vmx/tdx.c
> +++ b/arch/x86/kvm/vmx/tdx.c
> @@ -778,8 +778,6 @@ void tdx_prepare_switch_to_guest(struct kvm_vcpu *vcpu)
>  	else
>  		vt->msr_host_kernel_gs_base = read_msr(MSR_KERNEL_GS_BASE);
>  
> -	vt->host_debugctlmsr = get_debugctlmsr();
> -
>  	vt->guest_state_loaded = true;
>  }
>  
> @@ -1056,8 +1054,8 @@ fastpath_t tdx_vcpu_run(struct kvm_vcpu *vcpu, u64 run_flags)
>  
>  	tdx_vcpu_enter_exit(vcpu);
>  
> -	if (vt->host_debugctlmsr & ~TDX_DEBUGCTL_PRESERVED)
> -		update_debugctlmsr(vt->host_debugctlmsr);
> +	if (vcpu->arch.host_debugctl & ~TDX_DEBUGCTL_PRESERVED)
> +		update_debugctlmsr(vcpu->arch.host_debugctl);
>  
>  	tdx_load_host_xsave_state(vcpu);
>  	tdx->guest_entered = true;
> 
> base-commit: 475a02020ac2de6b10e85de75e79833139b556e0
> --
>