Re: [PATCH V2 3/3] KVM: X86: Reset DR6 only when KVM_DEBUGREG_WONT_EXIT

[Lai Jiangshan]

On 2021/8/10 18:14, Paolo Bonzini wrote:
> On 09/08/21 19:43, Lai Jiangshan wrote:
> > From: Lai Jiangshan <laijs@xxxxxxxxxxxxxxxxx>
> >
> > The commit efdab992813fb ("KVM: x86: fix escape of guest dr6 to the host")
> > fixed a bug by reseting DR6 unconditionally when the vcpu being scheduled out.
> >
> > But writing to debug registers is slow, and it can be shown in perf results
> > sometimes even neither the host nor the guest activate breakpoints.
> >
> > It'd be better to reset it conditionally and this patch moves the code of
> > reseting DR6 to the path of VM-exit and only reset it when
> > KVM_DEBUGREG_WONT_EXIT which is the only case that DR6 is guest value.
> >
> > Signed-off-by: Lai Jiangshan <laijs@xxxxxxxxxxxxxxxxx>
> > ---
> >   arch/x86/kvm/x86.c | 8 ++------
> >   1 file changed, 2 insertions(+), 6 deletions(-)
> >
> > diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
> > index d2aa49722064..f40cdd7687d8 100644
> > --- a/arch/x86/kvm/x86.c
> > +++ b/arch/x86/kvm/x86.c
> > @@ -4309,12 +4309,6 @@ void kvm_arch_vcpu_put(struct kvm_vcpu *vcpu)
> >       static_call(kvm_x86_vcpu_put)(vcpu);
> >       vcpu->arch.last_host_tsc = rdtsc();
> > -    /*
> > -     * If userspace has set any breakpoints or watchpoints, dr6 is restored
> > -     * on every vmexit, but if not, we might have a stale dr6 from the
> > -     * guest. do_debug expects dr6 to be cleared after it runs, do the same.
> > -     */
> > -    set_debugreg(0, 6);
> >   }
> >   static int kvm_vcpu_ioctl_get_lapic(struct kvm_vcpu *vcpu,
> > @@ -9630,6 +9624,8 @@ static int vcpu_enter_guest(struct kvm_vcpu *vcpu)
> >           static_call(kvm_x86_sync_dirty_debug_regs)(vcpu);
> >           kvm_update_dr0123(vcpu);
> >           kvm_update_dr7(vcpu);
> > +        /* Reset Dr6 which is guest value. */
> > +        set_debugreg(DR6_RESERVED, 6);
> >       }
> >       /*
>
> ... and this should also be done exclusively for VMX, in vmx_sync_dirty_debug_regs:
>
>      KVM: VMX: Reset DR6 only when KVM_DEBUGREG_WONT_EXIT
>      The commit efdab992813fb ("KVM: x86: fix escape of guest dr6 to the host")
>      fixed a bug by resetting DR6 unconditionally when the vcpu being scheduled out.
>      But writing to debug registers is slow, and it can be visible in perf results
>      sometimes, even if neither the host nor the guest activate breakpoints.
>      Since KVM_DEBUGREG_WONT_EXIT on Intel processors is the only case
>      where DR6 gets the guest value, and it never happens at all on SVM,
>      the register can be cleared in vmx.c right after reading it.
>      Reported-by: Lai Jiangshan <laijs@xxxxxxxxxxxxxxxxx>
>      Signed-off-by: Paolo Bonzini <pbonzini@xxxxxxxxxx>
>
> diff --git a/arch/x86/kvm/vmx/vmx.c b/arch/x86/kvm/vmx/vmx.c
> index 21a3ef3012cf..3a91302d05c0 100644
> --- a/arch/x86/kvm/vmx/vmx.c
> +++ b/arch/x86/kvm/vmx/vmx.c
> @@ -5110,6 +5110,12 @@ static void vmx_sync_dirty_debug_regs(struct kvm_vcpu *vcpu)
>
>       vcpu->arch.switch_db_regs &= ~KVM_DEBUGREG_WONT_EXIT;
>       exec_controls_setbit(to_vmx(vcpu), CPU_BASED_MOV_DR_EXITING);
> +
> +    /*
> +     * do_debug expects dr6 to be cleared after it runs, avoid that it sees
> +     * a stale dr6 from the guest.
> +     */


do_debug() is renamed. Maybe you can use "The host kernel #DB handler".


> +    set_debugreg(DR6_RESERVED, 6);
>   }
>
>   static void vmx_set_dr7(struct kvm_vcpu *vcpu, unsigned long val)
> diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
> index fbc536b21585..04c393551fb0 100644
> --- a/arch/x86/kvm/x86.c
> +++ b/arch/x86/kvm/x86.c
> @@ -4313,12 +4313,6 @@ void kvm_arch_vcpu_put(struct kvm_vcpu *vcpu)
>
>       static_call(kvm_x86_vcpu_put)(vcpu);
>       vcpu->arch.last_host_tsc = rdtsc();
> -    /*
> -     * If userspace has set any breakpoints or watchpoints, dr6 is restored
> -     * on every vmexit, but if not, we might have a stale dr6 from the
> -     * guest. do_debug expects dr6 to be cleared after it runs, do the same.
> -     */
> -    set_debugreg(0, 6);
>   }
>
>   static int kvm_vcpu_ioctl_get_lapic(struct kvm_vcpu *vcpu,
>
>
> Paolo