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

[Chao Gao <chao.gao@intel.com>]

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