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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <aCbf6JikteXzb0gB@intel.com>
Date: Fri, 16 May 2025 14:49:12 +0800
From: Chao Gao <chao.gao@...el.com>
To: Maxim Levitsky <mlevitsk@...hat.com>
CC: <kvm@...r.kernel.org>, Thomas Gleixner <tglx@...utronix.de>, "Sean
 Christopherson" <seanjc@...gle.com>, Borislav Petkov <bp@...en8.de>,
	<x86@...nel.org>, Ingo Molnar <mingo@...hat.com>,
	<linux-kernel@...r.kernel.org>, "H. Peter Anvin" <hpa@...or.com>, "Paolo
 Bonzini" <pbonzini@...hat.com>, Dave Hansen <dave.hansen@...ux.intel.com>
Subject: Re: [PATCH v4 2/4] KVM: x86: Drop kvm_x86_ops.set_dr6() in favor of
 a new KVM_RUN flag

>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
>
>