[RFC PATCH 0/3] kvm: x86: Cleanup and optimazation of switch_db_regs

[RFC PATCH 0/3] kvm: x86: Cleanup and optimazation of switch_db_regs

[Xiaoyao Li]

Through reading debug related codes in kvm/x86, I found the flags of
switch_db_regs is a little confusing (at least for me) and there is
something to improve. So this patchset comes. But it only go througg
compilation.

Xiaoyao Li (3):
  kvm: x86: Rename KVM_DEBUGREG_RELOAD to KVM_DEBUGREG_NEED_RELOAD
  kvm: x86: Use KVM_DEBUGREG_NEED_RELOAD instead of
    KVM_DEBUGREG_BP_ENABLED
  kvm: x86: skip DRn reload if previous VM exit is DR access VM exit

 arch/x86/include/asm/kvm_host.h |  3 +--
 arch/x86/kvm/svm/svm.c          |  8 +++++---
 arch/x86/kvm/vmx/vmx.c          |  8 +++++---
 arch/x86/kvm/x86.c              | 12 ++++++------
 4 files changed, 17 insertions(+), 14 deletions(-)

-- 
2.20.1

[RFC PATCH 1/3] kvm: x86: Rename KVM_DEBUGREG_RELOAD to KVM_DEBUGREG_NEED_RELOAD

[Xiaoyao Li]

To make it more clear that the flag means DRn (except DR7) need to be
reloaded before vm entry.

Signed-off-by: Xiaoyao Li <xiaoyao.li@intel.com>
---
 arch/x86/include/asm/kvm_host.h | 2 +-
 arch/x86/kvm/x86.c              | 6 +++---
 2 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/arch/x86/include/asm/kvm_host.h b/arch/x86/include/asm/kvm_host.h
index c7da23aed79a..f465c76e6e5a 100644
--- a/arch/x86/include/asm/kvm_host.h
+++ b/arch/x86/include/asm/kvm_host.h
@@ -511,7 +511,7 @@ struct kvm_pmu_ops;
 enum {
 	KVM_DEBUGREG_BP_ENABLED = 1,
 	KVM_DEBUGREG_WONT_EXIT = 2,
-	KVM_DEBUGREG_RELOAD = 4,
+	KVM_DEBUGREG_NEED_RELOAD = 4,
 };
 
 struct kvm_mtrr_range {
diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
index de77bc9bd0d7..cce926658d10 100644
--- a/arch/x86/kvm/x86.c
+++ b/arch/x86/kvm/x86.c
@@ -1067,7 +1067,7 @@ static void kvm_update_dr0123(struct kvm_vcpu *vcpu)
 	if (!(vcpu->guest_debug & KVM_GUESTDBG_USE_HW_BP)) {
 		for (i = 0; i < KVM_NR_DB_REGS; i++)
 			vcpu->arch.eff_db[i] = vcpu->arch.db[i];
-		vcpu->arch.switch_db_regs |= KVM_DEBUGREG_RELOAD;
+		vcpu->arch.switch_db_regs |= KVM_DEBUGREG_NEED_RELOAD;
 	}
 }
 
@@ -8407,7 +8407,7 @@ static int vcpu_enter_guest(struct kvm_vcpu *vcpu)
 		set_debugreg(vcpu->arch.eff_db[2], 2);
 		set_debugreg(vcpu->arch.eff_db[3], 3);
 		set_debugreg(vcpu->arch.dr6, 6);
-		vcpu->arch.switch_db_regs &= ~KVM_DEBUGREG_RELOAD;
+		vcpu->arch.switch_db_regs &= ~KVM_DEBUGREG_NEED_RELOAD;
 	}
 
 	kvm_x86_ops.run(vcpu);
@@ -8424,7 +8424,7 @@ static int vcpu_enter_guest(struct kvm_vcpu *vcpu)
 		kvm_update_dr0123(vcpu);
 		kvm_update_dr6(vcpu);
 		kvm_update_dr7(vcpu);
-		vcpu->arch.switch_db_regs &= ~KVM_DEBUGREG_RELOAD;
+		vcpu->arch.switch_db_regs &= ~KVM_DEBUGREG_NEED_RELOAD;
 	}
 
 	/*
-- 
2.20.1

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

[Xiaoyao Li]

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.

Signed-off-by: Xiaoyao Li <xiaoyao.li@intel.com>
---
 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

[RFC PATCH 3/3] kvm: x86: skip DRn reload if previous VM exit is DR access VM exit

[Xiaoyao Li]

When DR access vm exit, there is no DRn change throughout VM exit to
next VM enter. Skip the DRn reload in this case and fix the comments.

Signed-off-by: Xiaoyao Li <xiaoyao.li@intel.com>
---
 arch/x86/kvm/svm/svm.c | 8 +++++---
 arch/x86/kvm/vmx/vmx.c | 8 +++++---
 arch/x86/kvm/x86.c     | 2 +-
 3 files changed, 11 insertions(+), 7 deletions(-)

diff --git a/arch/x86/kvm/svm/svm.c b/arch/x86/kvm/svm/svm.c
index 66123848448d..c6883a0bf8c3 100644
--- a/arch/x86/kvm/svm/svm.c
+++ b/arch/x86/kvm/svm/svm.c
@@ -2287,9 +2287,11 @@ static int dr_interception(struct vcpu_svm *svm)
 
 	if (svm->vcpu.guest_debug == 0) {
 		/*
-		 * No more DR vmexits; force a reload of the debug registers
-		 * and reenter on this instruction.  The next vmexit will
-		 * retrieve the full state of the debug registers.
+		 * No more DR vmexits and reenter on this instruction.
+		 * The next vmexit will retrieve the full state of the debug
+		 * registers and re-enable DR vmexits.
+		 * No need to set KVM_DEBUGREG_NEED_RELOAD because no DRn change
+		 * since this DR vmexit.
 		 */
 		clr_dr_intercepts(svm);
 		svm->vcpu.arch.switch_db_regs |= KVM_DEBUGREG_WONT_EXIT;
diff --git a/arch/x86/kvm/vmx/vmx.c b/arch/x86/kvm/vmx/vmx.c
index aa1b8cf7c915..22eff8503048 100644
--- a/arch/x86/kvm/vmx/vmx.c
+++ b/arch/x86/kvm/vmx/vmx.c
@@ -4967,9 +4967,11 @@ static int handle_dr(struct kvm_vcpu *vcpu)
 		exec_controls_clearbit(to_vmx(vcpu), CPU_BASED_MOV_DR_EXITING);
 
 		/*
-		 * No more DR vmexits; force a reload of the debug registers
-		 * and reenter on this instruction.  The next vmexit will
-		 * retrieve the full state of the debug registers.
+		 * No more DR vmexits and reenter on this instruction.
+		 * The next vmexit will retrieve the full state of the debug
+		 * registers and re-enable DR vmexits.
+		 * No need to set KVM_DEBUGREG_NEED_RELOAD because no DRn change
+		 * since this DR vmexit.
 		 */
 		vcpu->arch.switch_db_regs |= KVM_DEBUGREG_WONT_EXIT;
 		return 1;
diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
index 71264df64001..8983848cbf45 100644
--- a/arch/x86/kvm/x86.c
+++ b/arch/x86/kvm/x86.c
@@ -8400,7 +8400,7 @@ static int vcpu_enter_guest(struct kvm_vcpu *vcpu)
 	if (test_thread_flag(TIF_NEED_FPU_LOAD))
 		switch_fpu_return();
 
-	if (unlikely(vcpu->arch.switch_db_regs)) {
+	if (unlikely(vcpu->arch.switch_db_regs & KVM_DEBUGREG_NEED_RELOAD)) {
 		set_debugreg(0, 7);
 		set_debugreg(vcpu->arch.eff_db[0], 0);
 		set_debugreg(vcpu->arch.eff_db[1], 1);
-- 
2.20.1

Re: [RFC PATCH 1/3] kvm: x86: Rename KVM_DEBUGREG_RELOAD to KVM_DEBUGREG_NEED_RELOAD

[Sean Christopherson]

On Thu, Apr 16, 2020 at 06:15:07PM +0800, Xiaoyao Li wrote:
> To make it more clear that the flag means DRn (except DR7) need to be
> reloaded before vm entry.
> 
> Signed-off-by: Xiaoyao Li <xiaoyao.li@intel.com>
> ---
>  arch/x86/include/asm/kvm_host.h | 2 +-
>  arch/x86/kvm/x86.c              | 6 +++---
>  2 files changed, 4 insertions(+), 4 deletions(-)
> 
> diff --git a/arch/x86/include/asm/kvm_host.h b/arch/x86/include/asm/kvm_host.h
> index c7da23aed79a..f465c76e6e5a 100644
> --- a/arch/x86/include/asm/kvm_host.h
> +++ b/arch/x86/include/asm/kvm_host.h
> @@ -511,7 +511,7 @@ struct kvm_pmu_ops;
>  enum {
>  	KVM_DEBUGREG_BP_ENABLED = 1,
>  	KVM_DEBUGREG_WONT_EXIT = 2,
> -	KVM_DEBUGREG_RELOAD = 4,
> +	KVM_DEBUGREG_NEED_RELOAD = 4,

My vote would be for KVM_DEBUGREG_DIRTY  Any bit that is set switch_db_regs
triggers a reload, whereas I would expect a RELOAD flag to be set _every_
time a load is needed and thus be the only bit that's checked

>  };
>  
>  struct kvm_mtrr_range {
> diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
> index de77bc9bd0d7..cce926658d10 100644
> --- a/arch/x86/kvm/x86.c
> +++ b/arch/x86/kvm/x86.c
> @@ -1067,7 +1067,7 @@ static void kvm_update_dr0123(struct kvm_vcpu *vcpu)
>  	if (!(vcpu->guest_debug & KVM_GUESTDBG_USE_HW_BP)) {
>  		for (i = 0; i < KVM_NR_DB_REGS; i++)
>  			vcpu->arch.eff_db[i] = vcpu->arch.db[i];
> -		vcpu->arch.switch_db_regs |= KVM_DEBUGREG_RELOAD;
> +		vcpu->arch.switch_db_regs |= KVM_DEBUGREG_NEED_RELOAD;
>  	}
>  }
>  
> @@ -8407,7 +8407,7 @@ static int vcpu_enter_guest(struct kvm_vcpu *vcpu)
>  		set_debugreg(vcpu->arch.eff_db[2], 2);
>  		set_debugreg(vcpu->arch.eff_db[3], 3);
>  		set_debugreg(vcpu->arch.dr6, 6);
> -		vcpu->arch.switch_db_regs &= ~KVM_DEBUGREG_RELOAD;
> +		vcpu->arch.switch_db_regs &= ~KVM_DEBUGREG_NEED_RELOAD;
>  	}
>  
>  	kvm_x86_ops.run(vcpu);
> @@ -8424,7 +8424,7 @@ static int vcpu_enter_guest(struct kvm_vcpu *vcpu)
>  		kvm_update_dr0123(vcpu);
>  		kvm_update_dr6(vcpu);
>  		kvm_update_dr7(vcpu);
> -		vcpu->arch.switch_db_regs &= ~KVM_DEBUGREG_RELOAD;
> +		vcpu->arch.switch_db_regs &= ~KVM_DEBUGREG_NEED_RELOAD;

This is the path that I think would really benefit from DIRTY, it took me
several reads to catch that kvm_update_dr0123() will set RELOAD.

>  	}
>  
>  	/*
> -- 
> 2.20.1
> 

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@intel.com>
> ---
>  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
> 

Re: [RFC PATCH 3/3] kvm: x86: skip DRn reload if previous VM exit is DR access VM exit

[Sean Christopherson]

On Thu, Apr 16, 2020 at 06:15:09PM +0800, Xiaoyao Li wrote:
> When DR access vm exit, there is no DRn change throughout VM exit to
> next VM enter. Skip the DRn reload in this case and fix the comments.

Same thing as the previous patch, the hardware values aren't stable.  In
this case, MOV DR won't exit so KVM needs to ensure hardware has the guest
values, irrespective of whether breakpoints are enabled.

> Signed-off-by: Xiaoyao Li <xiaoyao.li@intel.com>
> ---
>  arch/x86/kvm/svm/svm.c | 8 +++++---
>  arch/x86/kvm/vmx/vmx.c | 8 +++++---
>  arch/x86/kvm/x86.c     | 2 +-
>  3 files changed, 11 insertions(+), 7 deletions(-)
> 
> diff --git a/arch/x86/kvm/svm/svm.c b/arch/x86/kvm/svm/svm.c
> index 66123848448d..c6883a0bf8c3 100644
> --- a/arch/x86/kvm/svm/svm.c
> +++ b/arch/x86/kvm/svm/svm.c
> @@ -2287,9 +2287,11 @@ static int dr_interception(struct vcpu_svm *svm)
>  
>  	if (svm->vcpu.guest_debug == 0) {
>  		/*
> -		 * No more DR vmexits; force a reload of the debug registers
> -		 * and reenter on this instruction.  The next vmexit will
> -		 * retrieve the full state of the debug registers.
> +		 * No more DR vmexits and reenter on this instruction.
> +		 * The next vmexit will retrieve the full state of the debug
> +		 * registers and re-enable DR vmexits.
> +		 * No need to set KVM_DEBUGREG_NEED_RELOAD because no DRn change
> +		 * since this DR vmexit.
>  		 */
>  		clr_dr_intercepts(svm);
>  		svm->vcpu.arch.switch_db_regs |= KVM_DEBUGREG_WONT_EXIT;
> diff --git a/arch/x86/kvm/vmx/vmx.c b/arch/x86/kvm/vmx/vmx.c
> index aa1b8cf7c915..22eff8503048 100644
> --- a/arch/x86/kvm/vmx/vmx.c
> +++ b/arch/x86/kvm/vmx/vmx.c
> @@ -4967,9 +4967,11 @@ static int handle_dr(struct kvm_vcpu *vcpu)
>  		exec_controls_clearbit(to_vmx(vcpu), CPU_BASED_MOV_DR_EXITING);
>  
>  		/*
> -		 * No more DR vmexits; force a reload of the debug registers
> -		 * and reenter on this instruction.  The next vmexit will
> -		 * retrieve the full state of the debug registers.
> +		 * No more DR vmexits and reenter on this instruction.
> +		 * The next vmexit will retrieve the full state of the debug
> +		 * registers and re-enable DR vmexits.
> +		 * No need to set KVM_DEBUGREG_NEED_RELOAD because no DRn change
> +		 * since this DR vmexit.
>  		 */
>  		vcpu->arch.switch_db_regs |= KVM_DEBUGREG_WONT_EXIT;
>  		return 1;
> diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
> index 71264df64001..8983848cbf45 100644
> --- a/arch/x86/kvm/x86.c
> +++ b/arch/x86/kvm/x86.c
> @@ -8400,7 +8400,7 @@ static int vcpu_enter_guest(struct kvm_vcpu *vcpu)
>  	if (test_thread_flag(TIF_NEED_FPU_LOAD))
>  		switch_fpu_return();
>  
> -	if (unlikely(vcpu->arch.switch_db_regs)) {
> +	if (unlikely(vcpu->arch.switch_db_regs & KVM_DEBUGREG_NEED_RELOAD)) {
>  		set_debugreg(0, 7);
>  		set_debugreg(vcpu->arch.eff_db[0], 0);
>  		set_debugreg(vcpu->arch.eff_db[1], 1);
> -- 
> 2.20.1
> 

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

[Xiaoyao Li]

On 4/24/2020 3:29 AM, Sean Christopherson wrote:
> 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.  

May bad, bbviously I didn't think about it.

> My bet is that DRs can be changed
> from interrupt context as well.
So set KVM_DEBUGREG_NEED_RELOAD in vcpu_load won't help.

> 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@intel.com>
>> ---
>>   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
>>

Re: [RFC PATCH 1/3] kvm: x86: Rename KVM_DEBUGREG_RELOAD to KVM_DEBUGREG_NEED_RELOAD

[Xiaoyao Li]

On 4/24/2020 3:09 AM, Sean Christopherson wrote:
> On Thu, Apr 16, 2020 at 06:15:07PM +0800, Xiaoyao Li wrote:
>> To make it more clear that the flag means DRn (except DR7) need to be
>> reloaded before vm entry.
>>
>> Signed-off-by: Xiaoyao Li <xiaoyao.li@intel.com>
>> ---
>>   arch/x86/include/asm/kvm_host.h | 2 +-
>>   arch/x86/kvm/x86.c              | 6 +++---
>>   2 files changed, 4 insertions(+), 4 deletions(-)
>>
>> diff --git a/arch/x86/include/asm/kvm_host.h b/arch/x86/include/asm/kvm_host.h
>> index c7da23aed79a..f465c76e6e5a 100644
>> --- a/arch/x86/include/asm/kvm_host.h
>> +++ b/arch/x86/include/asm/kvm_host.h
>> @@ -511,7 +511,7 @@ struct kvm_pmu_ops;
>>   enum {
>>   	KVM_DEBUGREG_BP_ENABLED = 1,
>>   	KVM_DEBUGREG_WONT_EXIT = 2,
>> -	KVM_DEBUGREG_RELOAD = 4,
>> +	KVM_DEBUGREG_NEED_RELOAD = 4,
> 
> My vote would be for KVM_DEBUGREG_DIRTY  Any bit that is set switch_db_regs
> triggers a reload, whereas I would expect a RELOAD flag to be set _every_
> time a load is needed and thus be the only bit that's checked
> 

That's what I intended to do with this series, apparently I failed it. :(

>>   };
>>   
>>   struct kvm_mtrr_range {
>> diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
>> index de77bc9bd0d7..cce926658d10 100644
>> --- a/arch/x86/kvm/x86.c
>> +++ b/arch/x86/kvm/x86.c
>> @@ -1067,7 +1067,7 @@ static void kvm_update_dr0123(struct kvm_vcpu *vcpu)
>>   	if (!(vcpu->guest_debug & KVM_GUESTDBG_USE_HW_BP)) {
>>   		for (i = 0; i < KVM_NR_DB_REGS; i++)
>>   			vcpu->arch.eff_db[i] = vcpu->arch.db[i];
>> -		vcpu->arch.switch_db_regs |= KVM_DEBUGREG_RELOAD;
>> +		vcpu->arch.switch_db_regs |= KVM_DEBUGREG_NEED_RELOAD;
>>   	}
>>   }
>>   
>> @@ -8407,7 +8407,7 @@ static int vcpu_enter_guest(struct kvm_vcpu *vcpu)
>>   		set_debugreg(vcpu->arch.eff_db[2], 2);
>>   		set_debugreg(vcpu->arch.eff_db[3], 3);
>>   		set_debugreg(vcpu->arch.dr6, 6);
>> -		vcpu->arch.switch_db_regs &= ~KVM_DEBUGREG_RELOAD;
>> +		vcpu->arch.switch_db_regs &= ~KVM_DEBUGREG_NEED_RELOAD;
>>   	}
>>   
>>   	kvm_x86_ops.run(vcpu);
>> @@ -8424,7 +8424,7 @@ static int vcpu_enter_guest(struct kvm_vcpu *vcpu)
>>   		kvm_update_dr0123(vcpu);
>>   		kvm_update_dr6(vcpu);
>>   		kvm_update_dr7(vcpu);
>> -		vcpu->arch.switch_db_regs &= ~KVM_DEBUGREG_RELOAD;
>> +		vcpu->arch.switch_db_regs &= ~KVM_DEBUGREG_NEED_RELOAD;
> 
> This is the path that I think would really benefit from DIRTY, it took me
> several reads to catch that kvm_update_dr0123() will set RELOAD.
> 
>>   	}
>>   
>>   	/*
>> -- 
>> 2.20.1
>>

Re: [RFC PATCH 1/3] kvm: x86: Rename KVM_DEBUGREG_RELOAD to KVM_DEBUGREG_NEED_RELOAD

[Peter Xu]

On Thu, Apr 23, 2020 at 12:09:42PM -0700, Sean Christopherson wrote:
> On Thu, Apr 16, 2020 at 06:15:07PM +0800, Xiaoyao Li wrote:
> > To make it more clear that the flag means DRn (except DR7) need to be
> > reloaded before vm entry.
> > 
> > Signed-off-by: Xiaoyao Li <xiaoyao.li@intel.com>
> > ---
> >  arch/x86/include/asm/kvm_host.h | 2 +-
> >  arch/x86/kvm/x86.c              | 6 +++---
> >  2 files changed, 4 insertions(+), 4 deletions(-)
> > 
> > diff --git a/arch/x86/include/asm/kvm_host.h b/arch/x86/include/asm/kvm_host.h
> > index c7da23aed79a..f465c76e6e5a 100644
> > --- a/arch/x86/include/asm/kvm_host.h
> > +++ b/arch/x86/include/asm/kvm_host.h
> > @@ -511,7 +511,7 @@ struct kvm_pmu_ops;
> >  enum {
> >  	KVM_DEBUGREG_BP_ENABLED = 1,
> >  	KVM_DEBUGREG_WONT_EXIT = 2,
> > -	KVM_DEBUGREG_RELOAD = 4,
> > +	KVM_DEBUGREG_NEED_RELOAD = 4,
> 
> My vote would be for KVM_DEBUGREG_DIRTY  Any bit that is set switch_db_regs
> triggers a reload, whereas I would expect a RELOAD flag to be set _every_
> time a load is needed and thus be the only bit that's checked

But then shouldn't DIRTY be set as long as KVM_DEBUGREG_BP_ENABLED is set every
time before vmenter?  Then it'll somehow go back to switch_db_regs, iiuc...

IIUC RELOAD actually wants to say "reload only for this iteration", that's why
it's cleared after each reload.  So maybe...  RELOAD_ONCE?

(Btw, do we have debug regs tests somewhere no matter inside guest or with
 KVM_SET_GUEST_DEBUG?)

Thanks,

-- 
Peter Xu

Re: [RFC PATCH 1/3] kvm: x86: Rename KVM_DEBUGREG_RELOAD to KVM_DEBUGREG_NEED_RELOAD

[Sean Christopherson]

On Fri, Apr 24, 2020 at 04:21:03PM -0400, Peter Xu wrote:
> On Thu, Apr 23, 2020 at 12:09:42PM -0700, Sean Christopherson wrote:
> > On Thu, Apr 16, 2020 at 06:15:07PM +0800, Xiaoyao Li wrote:
> > > To make it more clear that the flag means DRn (except DR7) need to be
> > > reloaded before vm entry.
> > > 
> > > Signed-off-by: Xiaoyao Li <xiaoyao.li@intel.com>
> > > ---
> > >  arch/x86/include/asm/kvm_host.h | 2 +-
> > >  arch/x86/kvm/x86.c              | 6 +++---
> > >  2 files changed, 4 insertions(+), 4 deletions(-)
> > > 
> > > diff --git a/arch/x86/include/asm/kvm_host.h b/arch/x86/include/asm/kvm_host.h
> > > index c7da23aed79a..f465c76e6e5a 100644
> > > --- a/arch/x86/include/asm/kvm_host.h
> > > +++ b/arch/x86/include/asm/kvm_host.h
> > > @@ -511,7 +511,7 @@ struct kvm_pmu_ops;
> > >  enum {
> > >  	KVM_DEBUGREG_BP_ENABLED = 1,
> > >  	KVM_DEBUGREG_WONT_EXIT = 2,
> > > -	KVM_DEBUGREG_RELOAD = 4,
> > > +	KVM_DEBUGREG_NEED_RELOAD = 4,
> > 
> > My vote would be for KVM_DEBUGREG_DIRTY  Any bit that is set switch_db_regs
> > triggers a reload, whereas I would expect a RELOAD flag to be set _every_
> > time a load is needed and thus be the only bit that's checked
> 
> But then shouldn't DIRTY be set as long as KVM_DEBUGREG_BP_ENABLED is set every
> time before vmenter?  Then it'll somehow go back to switch_db_regs, iiuc...
> 
> IIUC RELOAD actually wants to say "reload only for this iteration", that's why
> it's cleared after each reload.  So maybe...  RELOAD_ONCE?

Or FORCE_LOAD, or FORCE_RELOAD?  Those crossed my mind as well.

> (Btw, do we have debug regs tests somewhere no matter inside guest or with
>  KVM_SET_GUEST_DEBUG?)

I don't think so?

Re: [RFC PATCH 1/3] kvm: x86: Rename KVM_DEBUGREG_RELOAD to KVM_DEBUGREG_NEED_RELOAD

[Peter Xu]

On Fri, Apr 24, 2020 at 01:29:22PM -0700, Sean Christopherson wrote:
> On Fri, Apr 24, 2020 at 04:21:03PM -0400, Peter Xu wrote:
> > On Thu, Apr 23, 2020 at 12:09:42PM -0700, Sean Christopherson wrote:
> > > On Thu, Apr 16, 2020 at 06:15:07PM +0800, Xiaoyao Li wrote:
> > > > To make it more clear that the flag means DRn (except DR7) need to be
> > > > reloaded before vm entry.
> > > > 
> > > > Signed-off-by: Xiaoyao Li <xiaoyao.li@intel.com>
> > > > ---
> > > >  arch/x86/include/asm/kvm_host.h | 2 +-
> > > >  arch/x86/kvm/x86.c              | 6 +++---
> > > >  2 files changed, 4 insertions(+), 4 deletions(-)
> > > > 
> > > > diff --git a/arch/x86/include/asm/kvm_host.h b/arch/x86/include/asm/kvm_host.h
> > > > index c7da23aed79a..f465c76e6e5a 100644
> > > > --- a/arch/x86/include/asm/kvm_host.h
> > > > +++ b/arch/x86/include/asm/kvm_host.h
> > > > @@ -511,7 +511,7 @@ struct kvm_pmu_ops;
> > > >  enum {
> > > >  	KVM_DEBUGREG_BP_ENABLED = 1,
> > > >  	KVM_DEBUGREG_WONT_EXIT = 2,
> > > > -	KVM_DEBUGREG_RELOAD = 4,
> > > > +	KVM_DEBUGREG_NEED_RELOAD = 4,
> > > 
> > > My vote would be for KVM_DEBUGREG_DIRTY  Any bit that is set switch_db_regs
> > > triggers a reload, whereas I would expect a RELOAD flag to be set _every_
> > > time a load is needed and thus be the only bit that's checked
> > 
> > But then shouldn't DIRTY be set as long as KVM_DEBUGREG_BP_ENABLED is set every
> > time before vmenter?  Then it'll somehow go back to switch_db_regs, iiuc...
> > 
> > IIUC RELOAD actually wants to say "reload only for this iteration", that's why
> > it's cleared after each reload.  So maybe...  RELOAD_ONCE?
> 
> Or FORCE_LOAD, or FORCE_RELOAD?  Those crossed my mind as well.

Yep, FORCE_RELOAD sounds better than DIRTY.

> 
> > (Btw, do we have debug regs tests somewhere no matter inside guest or with
> >  KVM_SET_GUEST_DEBUG?)
> 
> I don't think so?

OK, I'll see whether I can write some up.  Thanks,

-- 
Peter Xu

Re: [RFC PATCH 1/3] kvm: x86: Rename KVM_DEBUGREG_RELOAD to KVM_DEBUGREG_NEED_RELOAD

[Paolo Bonzini]

On 24/04/20 22:21, Peter Xu wrote:
> But then shouldn't DIRTY be set as long as KVM_DEBUGREG_BP_ENABLED is set every
> time before vmenter?  Then it'll somehow go back to switch_db_regs, iiuc...
> 
> IIUC RELOAD actually wants to say "reload only for this iteration", that's why
> it's cleared after each reload.  So maybe...  RELOAD_ONCE?
> 
> (Btw, do we have debug regs tests somewhere no matter inside guest or with
>  KVM_SET_GUEST_DEBUG?)

What about KVM_DEBUGREG_EFF_DB_DIRTY?

We have them in kvm-unit-tests for debug regs inside the guest, but no
selftests covering KVM_SET_GUEST_DEBUG.

Paolo

Re: [RFC PATCH 1/3] kvm: x86: Rename KVM_DEBUGREG_RELOAD to KVM_DEBUGREG_NEED_RELOAD

[Paolo Bonzini]

On 16/04/20 12:15, Xiaoyao Li wrote:
> To make it more clear that the flag means DRn (except DR7) need to be
> reloaded before vm entry.
> 
> Signed-off-by: Xiaoyao Li <xiaoyao.li@intel.com>

I wonder if KVM_DEBUGREG_RELOAD is needed at all.  It should be easy to
write selftests for it, using the testcase in commit message
172b2386ed16 and the information in commit ae561edeb421.

Paolo

> ---
>  arch/x86/include/asm/kvm_host.h | 2 +-
>  arch/x86/kvm/x86.c              | 6 +++---
>  2 files changed, 4 insertions(+), 4 deletions(-)
> 
> diff --git a/arch/x86/include/asm/kvm_host.h b/arch/x86/include/asm/kvm_host.h
> index c7da23aed79a..f465c76e6e5a 100644
> --- a/arch/x86/include/asm/kvm_host.h
> +++ b/arch/x86/include/asm/kvm_host.h
> @@ -511,7 +511,7 @@ struct kvm_pmu_ops;
>  enum {
>  	KVM_DEBUGREG_BP_ENABLED = 1,
>  	KVM_DEBUGREG_WONT_EXIT = 2,
> -	KVM_DEBUGREG_RELOAD = 4,
> +	KVM_DEBUGREG_NEED_RELOAD = 4,
>  };
>  
>  struct kvm_mtrr_range {
> diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
> index de77bc9bd0d7..cce926658d10 100644
> --- a/arch/x86/kvm/x86.c
> +++ b/arch/x86/kvm/x86.c
> @@ -1067,7 +1067,7 @@ static void kvm_update_dr0123(struct kvm_vcpu *vcpu)
>  	if (!(vcpu->guest_debug & KVM_GUESTDBG_USE_HW_BP)) {
>  		for (i = 0; i < KVM_NR_DB_REGS; i++)
>  			vcpu->arch.eff_db[i] = vcpu->arch.db[i];
> -		vcpu->arch.switch_db_regs |= KVM_DEBUGREG_RELOAD;
> +		vcpu->arch.switch_db_regs |= KVM_DEBUGREG_NEED_RELOAD;
>  	}
>  }
>  
> @@ -8407,7 +8407,7 @@ static int vcpu_enter_guest(struct kvm_vcpu *vcpu)
>  		set_debugreg(vcpu->arch.eff_db[2], 2);
>  		set_debugreg(vcpu->arch.eff_db[3], 3);
>  		set_debugreg(vcpu->arch.dr6, 6);
> -		vcpu->arch.switch_db_regs &= ~KVM_DEBUGREG_RELOAD;
> +		vcpu->arch.switch_db_regs &= ~KVM_DEBUGREG_NEED_RELOAD;
>  	}
>  
>  	kvm_x86_ops.run(vcpu);
> @@ -8424,7 +8424,7 @@ static int vcpu_enter_guest(struct kvm_vcpu *vcpu)
>  		kvm_update_dr0123(vcpu);
>  		kvm_update_dr6(vcpu);
>  		kvm_update_dr7(vcpu);
> -		vcpu->arch.switch_db_regs &= ~KVM_DEBUGREG_RELOAD;
> +		vcpu->arch.switch_db_regs &= ~KVM_DEBUGREG_NEED_RELOAD;
>  	}
>  
>  	/*
> 

Re: [RFC PATCH 1/3] kvm: x86: Rename KVM_DEBUGREG_RELOAD to KVM_DEBUGREG_NEED_RELOAD

[Nadav Amit]

> On Apr 25, 2020, at 1:07 AM, Paolo Bonzini <pbonzini@redhat.com> wrote:
> 
> On 16/04/20 12:15, Xiaoyao Li wrote:
>> To make it more clear that the flag means DRn (except DR7) need to be
>> reloaded before vm entry.
>> 
>> Signed-off-by: Xiaoyao Li <xiaoyao.li@intel.com>
> 
> I wonder if KVM_DEBUGREG_RELOAD is needed at all.  It should be easy to
> write selftests for it, using the testcase in commit message
> 172b2386ed16 and the information in commit ae561edeb421.

I must be missing something, since I did not follow this thread and other
KVM changes very closely.

Yet, for the record, I added KVM_DEBUGREG_RELOAD due to real experienced
issues that I had while running Intel’s fuzzing tests on KVM: IIRC, the DRs
were not reloaded after an INIT event that clears them.

Personally, I would prefer that a test for that, if added, would be added
to KVM-unit-tests, based on Liran’s INIT test. This would allow to confirm
bare-metal behaves as the VM.

Re: [RFC PATCH 1/3] kvm: x86: Rename KVM_DEBUGREG_RELOAD to KVM_DEBUGREG_NEED_RELOAD

[Paolo Bonzini]

On 25/04/20 18:54, Nadav Amit wrote:
>> I wonder if KVM_DEBUGREG_RELOAD is needed at all.  It should be easy to
>> write selftests for it, using the testcase in commit message
>> 172b2386ed16 and the information in commit ae561edeb421.
> I must be missing something, since I did not follow this thread and other
> KVM changes very closely.
> 
> Yet, for the record, I added KVM_DEBUGREG_RELOAD due to real experienced
> issues that I had while running Intel’s fuzzing tests on KVM: IIRC, the DRs
> were not reloaded after an INIT event that clears them.

Indeed, but the code has changed since then and I'm not sure it is still
needed.

> Personally, I would prefer that a test for that, if added, would be added
> to KVM-unit-tests, based on Liran’s INIT test. This would allow to confirm
> bare-metal behaves as the VM.

Yes, that would be good as well of course.

Paolo

Re: [RFC PATCH 1/3] kvm: x86: Rename KVM_DEBUGREG_RELOAD to KVM_DEBUGREG_NEED_RELOAD

[Peter Xu]

On Sat, Apr 25, 2020 at 09:48:17AM +0200, Paolo Bonzini wrote:
> On 24/04/20 22:21, Peter Xu wrote:
> > But then shouldn't DIRTY be set as long as KVM_DEBUGREG_BP_ENABLED is set every
> > time before vmenter?  Then it'll somehow go back to switch_db_regs, iiuc...
> > 
> > IIUC RELOAD actually wants to say "reload only for this iteration", that's why
> > it's cleared after each reload.  So maybe...  RELOAD_ONCE?
> > 
> > (Btw, do we have debug regs tests somewhere no matter inside guest or with
> >  KVM_SET_GUEST_DEBUG?)
> 
> What about KVM_DEBUGREG_EFF_DB_DIRTY?

The problem is iiuc we always reload eff_db[] no matter which bit in
switch_db_regs is set, so this may still not clearly identify this bit from the
rest of the two bits...

Actually I think eff_db[] is a bit confusing here in that it can be either the
host specified dbreg values or the guest specified depends on the dynamic value
of KVM_GUESTDBG_USE_HW_BP.

I am thinking maybe it's clearer to have host_db[] and guest_db[], then only
until vmenter do we load either of them by:

  if (KVM_GUESTDBG_USE_HW_BP)
    load(host_db[]);
  else
    load(gueet_db[]);

Then each db[] will be very clear on what's the data is about.  And we don't
need to check KVM_GUESTDBG_USE_HW_BP every time when accessing eff_db[].

> 
> We have them in kvm-unit-tests for debug regs inside the guest, but no
> selftests covering KVM_SET_GUEST_DEBUG.

I see!  Thanks,

-- 
Peter Xu

Re: [RFC PATCH 1/3] kvm: x86: Rename KVM_DEBUGREG_RELOAD to KVM_DEBUGREG_NEED_RELOAD

[Xiaoyao Li]

On 4/27/2020 10:37 PM, Peter Xu wrote:
> On Sat, Apr 25, 2020 at 09:48:17AM +0200, Paolo Bonzini wrote:
>> On 24/04/20 22:21, Peter Xu wrote:
>>> But then shouldn't DIRTY be set as long as KVM_DEBUGREG_BP_ENABLED is set every
>>> time before vmenter?  Then it'll somehow go back to switch_db_regs, iiuc...
>>>
>>> IIUC RELOAD actually wants to say "reload only for this iteration", that's why
>>> it's cleared after each reload.  So maybe...  RELOAD_ONCE?
>>>
>>> (Btw, do we have debug regs tests somewhere no matter inside guest or with
>>>   KVM_SET_GUEST_DEBUG?)
>>
>> What about KVM_DEBUGREG_EFF_DB_DIRTY?
> 
> The problem is iiuc we always reload eff_db[] no matter which bit in
> switch_db_regs is set, so this may still not clearly identify this bit from the
> rest of the two bits...
> 
> Actually I think eff_db[] is a bit confusing here in that it can be either the
> host specified dbreg values or the guest specified depends on the dynamic value
> of KVM_GUESTDBG_USE_HW_BP.
> 
> I am thinking maybe it's clearer to have host_db[] and guest_db[], then only
> until vmenter do we load either of them by:

host_db[] is somewhat misleading, how about user_db[] (just like user_fpu)

>    if (KVM_GUESTDBG_USE_HW_BP)
>      load(host_db[]);
>    else
>      load(gueet_db[]);
> 
> Then each db[] will be very clear on what's the data is about.  And we don't
> need to check KVM_GUESTDBG_USE_HW_BP every time when accessing eff_db[].
>