Re: [PATCH 8/9] KVM: x86, SVM: do not clobber guest DR6 on KVM_EXIT_DEBUG

[Peter Xu <peterx@redhat.com>]

On Wed, May 06, 2020 at 07:10:33AM -0400, Paolo Bonzini wrote:
> On Intel, #DB exceptions transmit the DR6 value via the exit qualification
> field of the VMCS, and the exit qualification only contains the description
> of the precise event that caused a vmexit.
> 
> On AMD, instead the DR6 field of the VMCB is filled in as if the #DB exception
> was to be injected into the guest.  This has two effects when guest debugging
> is in use:
> 
> * the guest DR6 is clobbered
> 
> * the kvm_run->debug.arch.dr6 field can accumulate more debug events, rather
> than just the last one that happened.
> 
> Fortunately, if guest debugging is in use we debug register reads and writes
> are always intercepted.  Now that the guest DR6 is always synchronized with
> vcpu->arch.dr6, we can just run the guest with an all-zero DR6 while guest
> debugging is enabled, and restore the guest value when it is disabled.  This
> fixes both problems.
> 
> A testcase for the second issue is added in the next patch.

Is there supposed to be another test after this one, or the GD test?

> 
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
>  arch/x86/kvm/svm/svm.c |  2 ++
>  arch/x86/kvm/x86.c     | 12 ++++++++----
>  arch/x86/kvm/x86.h     |  2 ++
>  3 files changed, 12 insertions(+), 4 deletions(-)
> 
> diff --git a/arch/x86/kvm/svm/svm.c b/arch/x86/kvm/svm/svm.c
> index f03bffafd9e6..29dc7311dbb1 100644
> --- a/arch/x86/kvm/svm/svm.c
> +++ b/arch/x86/kvm/svm/svm.c
> @@ -1750,6 +1750,8 @@ static int db_interception(struct vcpu_svm *svm)
>  		kvm_run->exit_reason = KVM_EXIT_DEBUG;
>  		kvm_run->debug.arch.dr6 = svm->vmcb->save.dr6;
>  		kvm_run->debug.arch.dr7 = svm->vmcb->save.dr7;

[1]

> +		/* This restores DR6 to all zeros.  */
> +		kvm_update_dr6(vcpu);

I feel like it won't work as expected for KVM_GUESTDBG_SINGLESTEP, because at
[2] below it'll go to the "else" instead so dr6 seems won't be cleared in that
case.

Another concern I have is that, I mostly read kvm_update_dr6() as "apply the
dr6 memory cache --> VMCB".  I'm worried this might confuse people (at least I
used quite a few minutes to digest...) here because latest data should already
be in the VMCB.

Also, IMHO it would be fine to have invalid dr6 values during
KVM_SET_GUEST_DEBUG.  I'm not sure whether my understanding is correct, but I
see KVM_SET_GUEST_DEBUG needs to override the in-guest debug completely.  If we
worry about dr6 being incorrect after KVM_SET_GUEST_DEBUG is disabled, IMHO we
can reset dr6 in kvm_arch_vcpu_ioctl_set_guest_debug() properly before we
return the debug registers to the guest.

PS. I cannot see above lines [1] in my local tree (which seems to be really a
bugfix...).  I tried to use kvm/queue just in case I missed some patches, but I
still didn't see them.  So am I reading the wrong tree here?

Thanks,

>  		kvm_run->debug.arch.pc =
>  			svm->vmcb->save.cs.base + svm->vmcb->save.rip;
>  		kvm_run->debug.arch.exception = DB_VECTOR;
> diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
> index f4254d716b10..1b5e0fc346bb 100644
> --- a/arch/x86/kvm/x86.c
> +++ b/arch/x86/kvm/x86.c
> @@ -104,7 +104,6 @@ static u64 __read_mostly cr4_reserved_bits = CR4_RESERVED_BITS;
>                                      KVM_X2APIC_API_DISABLE_BROADCAST_QUIRK)
>  
>  static void update_cr8_intercept(struct kvm_vcpu *vcpu);
> -static void kvm_update_dr6(struct kvm_vcpu *vcpu);
>  static void process_nmi(struct kvm_vcpu *vcpu);
>  static void enter_smm(struct kvm_vcpu *vcpu);
>  static void __kvm_set_rflags(struct kvm_vcpu *vcpu, unsigned long rflags);
> @@ -1048,10 +1047,14 @@ static void kvm_update_dr0123(struct kvm_vcpu *vcpu)
>  	}
>  }
>  
> -static void kvm_update_dr6(struct kvm_vcpu *vcpu)
> +void kvm_update_dr6(struct kvm_vcpu *vcpu)
>  {
> -	if (!(vcpu->guest_debug & KVM_GUESTDBG_USE_HW_BP) &&
> -	    kvm_x86_ops.set_dr6)
> +	if (!kvm_x86_ops.set_dr6)
> +		return;
> +
> +	if (vcpu->guest_debug & KVM_GUESTDBG_USE_HW_BP)

[2]

> +		kvm_x86_ops.set_dr6(vcpu, DR6_FIXED_1 | DR6_RTM);
> +	else
>  		kvm_x86_ops.set_dr6(vcpu, vcpu->arch.dr6);
>  }
>  
> @@ -9154,6 +9157,7 @@ int kvm_arch_vcpu_ioctl_set_guest_debug(struct kvm_vcpu *vcpu,
>  		for (i = 0; i < KVM_NR_DB_REGS; i++)
>  			vcpu->arch.eff_db[i] = vcpu->arch.db[i];
>  	}
> +	kvm_update_dr6(vcpu);
>  	kvm_update_dr7(vcpu);
>  
>  	if (vcpu->guest_debug & KVM_GUESTDBG_SINGLESTEP)
> diff --git a/arch/x86/kvm/x86.h b/arch/x86/kvm/x86.h
> index b968acc0516f..a4c950ad4d60 100644
> --- a/arch/x86/kvm/x86.h
> +++ b/arch/x86/kvm/x86.h
> @@ -240,6 +240,8 @@ static inline bool kvm_vcpu_latch_init(struct kvm_vcpu *vcpu)
>  	return is_smm(vcpu) || kvm_x86_ops.apic_init_signal_blocked(vcpu);
>  }
>  
> +void kvm_update_dr6(struct kvm_vcpu *vcpu);
> +
>  void kvm_set_pending_timer(struct kvm_vcpu *vcpu);
>  void kvm_inject_realmode_interrupt(struct kvm_vcpu *vcpu, int irq, int inc_eip);
>  
> -- 
> 2.18.2
> 
> 

-- 
Peter Xu