Re: [RFC PATCH 2/3] kvm: x86: Use KVM_DEBUGREG_NEED_RELOAD instead of KVM_DEBUGREG_BP_ENABLED

[Sean Christopherson]

On Thu, Apr 16, 2020 at 06:15:08PM +0800, Xiaoyao Li wrote:
> Once any #BP enabled in DR7, it will set KVM_DEBUGREG_BP_ENABLED, which
> leads to reload DRn before every VM entry even if none of DRn changed.
> 
> Drop KVM_DEBUGREG_BP_ENABLED flag and set KVM_DEBUGREG_NEED_RELOAD flag
> for the cases that DRn need to be reloaded instead, to avoid unnecessary
> DRn reload.

Loading DRs on every VM-Enter _is_ necessary if there are breakpoints
enabled for the guest.  The hardware DR values are not "stable", e.g. they
are loaded with the host's values immediately after saving the guest's
value (if DR_EXITING is disabled) in vcpu_enter_guest(), notably iff the
host has an active/enabled breakpoint.  My bet is that DRs can be changed
from interrupt context as well.

Loading DRs for the guest (not necessarily the same as the guest's DRs) is
necessary if a breakpoint is enabled so that the #DB is actually hit in
guest.  It's a similar concept to instructions that consume MSR values,
e.g. SYSCALL, RDTSCP, etc..., even if KVM intercepts the MSR/DR, hardware
still needs the correct value so that the guest behavior is correct.

> Signed-off-by: Xiaoyao Li <xiaoyao.li@xxxxxxxxx>
> ---
>  arch/x86/include/asm/kvm_host.h | 3 +--
>  arch/x86/kvm/x86.c              | 4 ++--
>  2 files changed, 3 insertions(+), 4 deletions(-)
> 
> diff --git a/arch/x86/include/asm/kvm_host.h b/arch/x86/include/asm/kvm_host.h
> index f465c76e6e5a..87e2d020351e 100644
> --- a/arch/x86/include/asm/kvm_host.h
> +++ b/arch/x86/include/asm/kvm_host.h
> @@ -509,9 +509,8 @@ struct kvm_pmu {
>  struct kvm_pmu_ops;
>  
>  enum {
> -	KVM_DEBUGREG_BP_ENABLED = 1,
> +	KVM_DEBUGREG_NEED_RELOAD = 1,
>  	KVM_DEBUGREG_WONT_EXIT = 2,
> -	KVM_DEBUGREG_NEED_RELOAD = 4,
>  };
>  
>  struct kvm_mtrr_range {
> diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
> index cce926658d10..71264df64001 100644
> --- a/arch/x86/kvm/x86.c
> +++ b/arch/x86/kvm/x86.c
> @@ -1086,9 +1086,8 @@ static void kvm_update_dr7(struct kvm_vcpu *vcpu)
>  	else
>  		dr7 = vcpu->arch.dr7;
>  	kvm_x86_ops.set_dr7(vcpu, dr7);
> -	vcpu->arch.switch_db_regs &= ~KVM_DEBUGREG_BP_ENABLED;
>  	if (dr7 & DR7_BP_EN_MASK)
> -		vcpu->arch.switch_db_regs |= KVM_DEBUGREG_BP_ENABLED;
> +		vcpu->arch.switch_db_regs |= KVM_DEBUGREG_NEED_RELOAD;
>  }
>  
>  static u64 kvm_dr6_fixed(struct kvm_vcpu *vcpu)
> @@ -1128,6 +1127,7 @@ static int __kvm_set_dr(struct kvm_vcpu *vcpu, int dr, unsigned long val)
>  		break;
>  	}
>  
> +	vcpu->arch.switch_db_regs |= KVM_DEBUGREG_NEED_RELOAD;
>  	return 0;
>  }
>  
> -- 
> 2.20.1
>