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").
