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

From: Paolo Bonzini <pbonzini@redhat.com>
To: Peter Xu <peterx@redhat.com>
Cc: linux-kernel@vger.kernel.org, kvm@vger.kernel.org,
	Sean Christopherson <sean.j.christopherson@intel.com>
Subject: Re: [PATCH 8/9] KVM: x86, SVM: do not clobber guest DR6 on KVM_EXIT_DEBUG
Date: Wed, 6 May 2020 22:07:15 +0200	[thread overview]
Message-ID: <8f7f319c-4093-0ddc-f9f5-002c41d5622c@redhat.com> (raw)
In-Reply-To: <20200506181515.GR6299@xz-x1>

On 06/05/20 20:15, Peter Xu wrote:
> 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?

It's the GD test.
>> +		/* 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.

You're right, I need to cover both cases that trigger #DB.

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

No, the latest guest register is always in vcpu->arch.dr6.  It's only
because of KVM_DEBUGREG_WONT_EXIT that kvm_update_dr6() needs to pass
vcpu->arch.dr6 to kvm_x86_ops.set_dr6.  Actually this patch could even
check KVM_DEBUGREG_WONT_EXIT instead of vcpu->guest_debug.  I'll take a
look tomorrow.

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

Sort of, userspace can try to juggle host and guest debugging (this is
why you have KVM_GUESTDBG_INJECT_DB and KVM_GUESTDBG_INJECT_BP).

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

The patch is based on kvm/master, and indeed that line is from a bugfix
that I've posted yesterday ("KVM: SVM: fill in
kvm_run->debug.arch.dr[67]"). I had pushed that one right away, because
it was quite obviously suitable for 5.7.

Paolo