Re: [PATCH] KVM: x86: fix root cause for missed hardware breakpoints

[Paolo Bonzini]

On 26/02/2016 13:14, Nadav Amit wrote:
> Seems correct. The code may be easier to follow if the following line:
> 
> vcpu->arch.switch_db_regs &= ~KVM_DEBUGREG_WONT_EXIT
> 
> would have been moved out of svm_sync_dirty_debug_regs and
> vmx_sync_dirty_debug_regs into the flow of vcpu_enter_guest (above).

It is there because removing the bit is tied to actually enabling debug
register exits.  On one hand it's bad to have duplicated code, on the
other hand there's a reason for it...  I'm a bit undecided on whether to
move that duplicated line to x86.c.

Paolo

Re: [PATCH] KVM: x86: fix root cause for missed hardware breakpoints

[Paolo Bonzini]

On 26/02/2016 13:14, Nadav Amit wrote:
> Seems correct. The code may be easier to follow if the following line:
> 
> vcpu->arch.switch_db_regs &= ~KVM_DEBUGREG_WONT_EXIT
> 
> would have been moved out of svm_sync_dirty_debug_regs and
> vmx_sync_dirty_debug_regs into the flow of vcpu_enter_guest (above).

It is there because removing the bit is tied to actually enabling debug
register exits.  On one hand it's bad to have duplicated code, on the
other hand there's a reason for it...  I'm a bit undecided on whether to
move that duplicated line to x86.c.

Paolo

Re: [PATCH] KVM: x86: fix root cause for missed hardware breakpoints

[Nadav Amit]

Xiao Guangrong  wrote:

> 
> 
> On 02/26/2016 07:46 PM, Paolo Bonzini wrote:
>> Commit 172b2386ed16 ("KVM: x86: fix missed hardware breakpoints",
>> 2016-02-10) worked around a case where the debug registers are not loaded
>> correctly on preemption and on the first entry to KVM_RUN.
>> 
>> However, Xiao Guangrong pointed out that the root cause must be that
>> KVM_DEBUGREG_BP_ENABLED is not being set correctly.  This can indeed
>> happen due to the lazy debug exit mechanism, which does not call
>> kvm_update_dr7.  Fix it by replacing the existing loop (more or less
>> equivalent to kvm_update_dr0123) with calls to all the kvm_update_dr*
>> functions.
>> 
>> The original patch is good enough for stable releases before 4.1, since
>> it does not have any dependencies such as commit ae561edeb421 ("KVM:
>> x86: DR0-DR3 are not clear on reset", 2015-04-02).
>> 
>> Cc: sta...@vger.kernel.org   # 4.1+
>> Fixes: 172b2386ed16a9143d9a456aae5ec87275c61489
>> Signed-off-by: Paolo Bonzini 
>> ---
>>  arch/x86/kvm/x86.c | 9 -
>>  1 file changed, 4 insertions(+), 5 deletions(-)
>> 
>> diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
>> index f4891f2ece23..eaf6ee8c28b8 100644
>> --- a/arch/x86/kvm/x86.c
>> +++ b/arch/x86/kvm/x86.c
>> @@ -2752,7 +2752,6 @@ void kvm_arch_vcpu_load(struct kvm_vcpu *vcpu, int cpu)
>>  }
>> 
>>  kvm_make_request(KVM_REQ_STEAL_UPDATE, vcpu);
>> -vcpu->arch.switch_db_regs |= KVM_DEBUGREG_RELOAD;
>>  }
>> 
>>  void kvm_arch_vcpu_put(struct kvm_vcpu *vcpu)
>> @@ -6619,12 +6618,12 @@ static int vcpu_enter_guest(struct kvm_vcpu *vcpu)
>>   * KVM_DEBUGREG_WONT_EXIT again.
>>   */
>>  if (unlikely(vcpu->arch.switch_db_regs & KVM_DEBUGREG_WONT_EXIT)) {
>> -int i;
>> -
>>  WARN_ON(vcpu->guest_debug & KVM_GUESTDBG_USE_HW_BP);
>>  kvm_x86_ops->sync_dirty_debug_regs(vcpu);
>> -for (i = 0; i < KVM_NR_DB_REGS; i++)
>> -vcpu->arch.eff_db[i] = vcpu->arch.db[i];
>> +kvm_update_dr0123(vcpu);
>> +kvm_update_dr6(vcpu);
>> +kvm_update_dr7(vcpu);
>> +vcpu->arch.switch_db_regs &= ~KVM_DEBUGREG_RELOAD;
>>  }
> 
> It is good to me now. It also can fix the case that the debug registers are
> changed by the debug-process(gdb/perf) in the context of the vcpu as i
> described in another thread.

Seems correct. The code may be easier to follow if the following line:

vcpu->arch.switch_db_regs &= ~KVM_DEBUGREG_WONT_EXIT

would have been moved out of svm_sync_dirty_debug_regs and
vmx_sync_dirty_debug_regs into the flow of vcpu_enter_guest (above).

Nadav

Re: [PATCH] KVM: x86: fix root cause for missed hardware breakpoints

[Nadav Amit]

Xiao Guangrong  wrote:

> 
> 
> On 02/26/2016 07:46 PM, Paolo Bonzini wrote:
>> Commit 172b2386ed16 ("KVM: x86: fix missed hardware breakpoints",
>> 2016-02-10) worked around a case where the debug registers are not loaded
>> correctly on preemption and on the first entry to KVM_RUN.
>> 
>> However, Xiao Guangrong pointed out that the root cause must be that
>> KVM_DEBUGREG_BP_ENABLED is not being set correctly.  This can indeed
>> happen due to the lazy debug exit mechanism, which does not call
>> kvm_update_dr7.  Fix it by replacing the existing loop (more or less
>> equivalent to kvm_update_dr0123) with calls to all the kvm_update_dr*
>> functions.
>> 
>> The original patch is good enough for stable releases before 4.1, since
>> it does not have any dependencies such as commit ae561edeb421 ("KVM:
>> x86: DR0-DR3 are not clear on reset", 2015-04-02).
>> 
>> Cc: sta...@vger.kernel.org   # 4.1+
>> Fixes: 172b2386ed16a9143d9a456aae5ec87275c61489
>> Signed-off-by: Paolo Bonzini 
>> ---
>>  arch/x86/kvm/x86.c | 9 -
>>  1 file changed, 4 insertions(+), 5 deletions(-)
>> 
>> diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
>> index f4891f2ece23..eaf6ee8c28b8 100644
>> --- a/arch/x86/kvm/x86.c
>> +++ b/arch/x86/kvm/x86.c
>> @@ -2752,7 +2752,6 @@ void kvm_arch_vcpu_load(struct kvm_vcpu *vcpu, int cpu)
>>  }
>> 
>>  kvm_make_request(KVM_REQ_STEAL_UPDATE, vcpu);
>> -vcpu->arch.switch_db_regs |= KVM_DEBUGREG_RELOAD;
>>  }
>> 
>>  void kvm_arch_vcpu_put(struct kvm_vcpu *vcpu)
>> @@ -6619,12 +6618,12 @@ static int vcpu_enter_guest(struct kvm_vcpu *vcpu)
>>   * KVM_DEBUGREG_WONT_EXIT again.
>>   */
>>  if (unlikely(vcpu->arch.switch_db_regs & KVM_DEBUGREG_WONT_EXIT)) {
>> -int i;
>> -
>>  WARN_ON(vcpu->guest_debug & KVM_GUESTDBG_USE_HW_BP);
>>  kvm_x86_ops->sync_dirty_debug_regs(vcpu);
>> -for (i = 0; i < KVM_NR_DB_REGS; i++)
>> -vcpu->arch.eff_db[i] = vcpu->arch.db[i];
>> +kvm_update_dr0123(vcpu);
>> +kvm_update_dr6(vcpu);
>> +kvm_update_dr7(vcpu);
>> +vcpu->arch.switch_db_regs &= ~KVM_DEBUGREG_RELOAD;
>>  }
> 
> It is good to me now. It also can fix the case that the debug registers are
> changed by the debug-process(gdb/perf) in the context of the vcpu as i
> described in another thread.

Seems correct. The code may be easier to follow if the following line:

vcpu->arch.switch_db_regs &= ~KVM_DEBUGREG_WONT_EXIT

would have been moved out of svm_sync_dirty_debug_regs and
vmx_sync_dirty_debug_regs into the flow of vcpu_enter_guest (above).

Nadav

Re: [PATCH] KVM: x86: fix root cause for missed hardware breakpoints

[Xiao Guangrong]

On 02/26/2016 07:46 PM, Paolo Bonzini wrote:

Commit 172b2386ed16 ("KVM: x86: fix missed hardware breakpoints",
2016-02-10) worked around a case where the debug registers are not loaded
correctly on preemption and on the first entry to KVM_RUN.

However, Xiao Guangrong pointed out that the root cause must be that
KVM_DEBUGREG_BP_ENABLED is not being set correctly.  This can indeed
happen due to the lazy debug exit mechanism, which does not call
kvm_update_dr7.  Fix it by replacing the existing loop (more or less
equivalent to kvm_update_dr0123) with calls to all the kvm_update_dr*
functions.

The original patch is good enough for stable releases before 4.1, since
it does not have any dependencies such as commit ae561edeb421 ("KVM:
x86: DR0-DR3 are not clear on reset", 2015-04-02).

Cc: sta...@vger.kernel.org   # 4.1+
Fixes: 172b2386ed16a9143d9a456aae5ec87275c61489
Signed-off-by: Paolo Bonzini 
---
  arch/x86/kvm/x86.c | 9 -
  1 file changed, 4 insertions(+), 5 deletions(-)

diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
index f4891f2ece23..eaf6ee8c28b8 100644
--- a/arch/x86/kvm/x86.c
+++ b/arch/x86/kvm/x86.c
@@ -2752,7 +2752,6 @@ void kvm_arch_vcpu_load(struct kvm_vcpu *vcpu, int cpu)
}

kvm_make_request(KVM_REQ_STEAL_UPDATE, vcpu);
-   vcpu->arch.switch_db_regs |= KVM_DEBUGREG_RELOAD;
  }

  void kvm_arch_vcpu_put(struct kvm_vcpu *vcpu)
@@ -6619,12 +6618,12 @@ static int vcpu_enter_guest(struct kvm_vcpu *vcpu)
 * KVM_DEBUGREG_WONT_EXIT again.
 */
if (unlikely(vcpu->arch.switch_db_regs & KVM_DEBUGREG_WONT_EXIT)) {
-   int i;
-
WARN_ON(vcpu->guest_debug & KVM_GUESTDBG_USE_HW_BP);
kvm_x86_ops->sync_dirty_debug_regs(vcpu);
-   for (i = 0; i < KVM_NR_DB_REGS; i++)
-   vcpu->arch.eff_db[i] = vcpu->arch.db[i];
+   kvm_update_dr0123(vcpu);
+   kvm_update_dr6(vcpu);
+   kvm_update_dr7(vcpu);
+   vcpu->arch.switch_db_regs &= ~KVM_DEBUGREG_RELOAD;
}


It is good to me now. It also can fix the case that the debug registers are
changed by the debug-process(gdb/perf) in the context of the vcpu as i
described in another thread.

Reviewed-by: Xiao Guangrong 

Re: [PATCH] KVM: x86: fix root cause for missed hardware breakpoints

[Xiao Guangrong]

On 02/26/2016 07:46 PM, Paolo Bonzini wrote:

Commit 172b2386ed16 ("KVM: x86: fix missed hardware breakpoints",
2016-02-10) worked around a case where the debug registers are not loaded
correctly on preemption and on the first entry to KVM_RUN.

However, Xiao Guangrong pointed out that the root cause must be that
KVM_DEBUGREG_BP_ENABLED is not being set correctly.  This can indeed
happen due to the lazy debug exit mechanism, which does not call
kvm_update_dr7.  Fix it by replacing the existing loop (more or less
equivalent to kvm_update_dr0123) with calls to all the kvm_update_dr*
functions.

The original patch is good enough for stable releases before 4.1, since
it does not have any dependencies such as commit ae561edeb421 ("KVM:
x86: DR0-DR3 are not clear on reset", 2015-04-02).

Cc: sta...@vger.kernel.org   # 4.1+
Fixes: 172b2386ed16a9143d9a456aae5ec87275c61489
Signed-off-by: Paolo Bonzini 
---
  arch/x86/kvm/x86.c | 9 -
  1 file changed, 4 insertions(+), 5 deletions(-)

diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
index f4891f2ece23..eaf6ee8c28b8 100644
--- a/arch/x86/kvm/x86.c
+++ b/arch/x86/kvm/x86.c
@@ -2752,7 +2752,6 @@ void kvm_arch_vcpu_load(struct kvm_vcpu *vcpu, int cpu)
}

kvm_make_request(KVM_REQ_STEAL_UPDATE, vcpu);
-   vcpu->arch.switch_db_regs |= KVM_DEBUGREG_RELOAD;
  }

  void kvm_arch_vcpu_put(struct kvm_vcpu *vcpu)
@@ -6619,12 +6618,12 @@ static int vcpu_enter_guest(struct kvm_vcpu *vcpu)
 * KVM_DEBUGREG_WONT_EXIT again.
 */
if (unlikely(vcpu->arch.switch_db_regs & KVM_DEBUGREG_WONT_EXIT)) {
-   int i;
-
WARN_ON(vcpu->guest_debug & KVM_GUESTDBG_USE_HW_BP);
kvm_x86_ops->sync_dirty_debug_regs(vcpu);
-   for (i = 0; i < KVM_NR_DB_REGS; i++)
-   vcpu->arch.eff_db[i] = vcpu->arch.db[i];
+   kvm_update_dr0123(vcpu);
+   kvm_update_dr6(vcpu);
+   kvm_update_dr7(vcpu);
+   vcpu->arch.switch_db_regs &= ~KVM_DEBUGREG_RELOAD;
}


It is good to me now. It also can fix the case that the debug registers are
changed by the debug-process(gdb/perf) in the context of the vcpu as i
described in another thread.

Reviewed-by: Xiao Guangrong