Re: [PATCH 9/9] KVM: VMX: pass correct DR6 for GD userspace exit

[Peter Xu]

On Thu, May 07, 2020 at 07:50:11AM -0400, Paolo Bonzini wrote:
> When KVM_EXIT_DEBUG is raised for the disabled-breakpoints case (DR7.GD),
> DR6 was incorrectly copied from the value in the VM.  Instead,
> DR6.BD should be set in order to catch this case.
> 
> On AMD this does not need any special code because the processor triggers
> a #DB exception that is intercepted.  However, the testcase would fail
> without the previous patch because both DR6.BS and DR6.BD would be set.
> 
> Signed-off-by: Paolo Bonzini <pbonzini@xxxxxxxxxx>
> ---
>  arch/x86/kvm/vmx/vmx.c                        |  2 +-
>  .../testing/selftests/kvm/x86_64/debug_regs.c | 24 ++++++++++++++++++-
>  2 files changed, 24 insertions(+), 2 deletions(-)
> 
> diff --git a/arch/x86/kvm/vmx/vmx.c b/arch/x86/kvm/vmx/vmx.c
> index e2b71b0cdfce..e45cf89c5821 100644
> --- a/arch/x86/kvm/vmx/vmx.c
> +++ b/arch/x86/kvm/vmx/vmx.c
> @@ -4927,7 +4927,7 @@ static int handle_dr(struct kvm_vcpu *vcpu)
>  		 * guest debugging itself.
>  		 */
>  		if (vcpu->guest_debug & KVM_GUESTDBG_USE_HW_BP) {
> -			vcpu->run->debug.arch.dr6 = vcpu->arch.dr6;
> +			vcpu->run->debug.arch.dr6 = DR6_BD | DR6_RTM | DR6_FIXED_1;

After a second thought I'm thinking whether it would be okay to have BS set in
that test case.  I just remembered there's a test case in the kvm-unit-test
that checks explicitly against BS leftover as long as dr6 is not cleared
explicitly by the guest code, while the spec seems to have no explicit
description on this case.

Intead of above, I'm thinking whether we should allow the userspace to also
change dr6 with the KVM_SET_GUEST_DEBUG ioctl when they wanted to (right now
iiuc dr6 from userspace is completely ignored), instead of offering a fake dr6.
Or to make it simple, maybe we can just check BD bit only?

Thanks,

>  			vcpu->run->debug.arch.dr7 = dr7;
>  			vcpu->run->debug.arch.pc = kvm_get_linear_rip(vcpu);
>  			vcpu->run->debug.arch.exception = DB_VECTOR;

-- 
Peter Xu