[PATCH] KVM: x86: Mask DR7 correctly on task-switch while debugging

[PATCH] KVM: x86: Mask DR7 correctly on task-switch while debugging

[Nadav Amit]

If the host sets hardware breakpoints to debug the guest, and a task-switch
occurs in the guest, the architectural DR7 will not be updated. The effective
DR7 would be updated instead.

This fix uses the standard DR setting mechanism instead of the one that was
previously used. As a bonus, the update of DR7 will now be effective for AMD as
well.

Signed-off-by: Nadav Amit <namit@cs.technion.ac.il>
---
 arch/x86/kvm/vmx.c | 3 ---
 arch/x86/kvm/x86.c | 3 +++
 2 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/arch/x86/kvm/vmx.c b/arch/x86/kvm/vmx.c
index f7a0a7f..8f731c0 100644
--- a/arch/x86/kvm/vmx.c
+++ b/arch/x86/kvm/vmx.c
@@ -5703,9 +5703,6 @@ static int handle_task_switch(struct kvm_vcpu *vcpu)
 		return 0;
 	}
 
-	/* clear all local breakpoint enable flags */
-	vmcs_writel(GUEST_DR7, vmcs_readl(GUEST_DR7) & ~0x155);
-
 	/*
 	 * TODO: What about debug traps on tss switch?
 	 *       Are we supposed to inject them and update dr6?
diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
index 2046be4..a170c35 100644
--- a/arch/x86/kvm/x86.c
+++ b/arch/x86/kvm/x86.c
@@ -6816,6 +6816,9 @@ int kvm_task_switch(struct kvm_vcpu *vcpu, u16 tss_selector, int idt_index,
 
 	kvm_rip_write(vcpu, ctxt->eip);
 	kvm_set_rflags(vcpu, ctxt->eflags);
+	ret = __kvm_set_dr(vcpu, 7, vcpu->arch.dr7 & ~(DR_LOCAL_ENABLE_MASK |
+						       DR_LOCAL_SLOWDOWN));
+	WARN_ON(ret != 0);
 	kvm_make_request(KVM_REQ_EVENT, vcpu);
 	return EMULATE_DONE;
 }
-- 
1.9.1

Re: [PATCH] KVM: x86: Mask DR7 correctly on task-switch while debugging

[Paolo Bonzini]

On 19/04/2015 14:18, Nadav Amit wrote:
> If the host sets hardware breakpoints to debug the guest, and a task-switch
> occurs in the guest, the architectural DR7 will not be updated. The effective
> DR7 would be updated instead.
> 
> This fix uses the standard DR setting mechanism instead of the one that was
> previously used. As a bonus, the update of DR7 will now be effective for AMD as
> well.

Is there a reason not to do it in emulate.c instead?

Paolo

> Signed-off-by: Nadav Amit <namit@cs.technion.ac.il>
> ---
>  arch/x86/kvm/vmx.c | 3 ---
>  arch/x86/kvm/x86.c | 3 +++
>  2 files changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/arch/x86/kvm/vmx.c b/arch/x86/kvm/vmx.c
> index f7a0a7f..8f731c0 100644
> --- a/arch/x86/kvm/vmx.c
> +++ b/arch/x86/kvm/vmx.c
> @@ -5703,9 +5703,6 @@ static int handle_task_switch(struct kvm_vcpu *vcpu)
>  		return 0;
>  	}
>  
> -	/* clear all local breakpoint enable flags */
> -	vmcs_writel(GUEST_DR7, vmcs_readl(GUEST_DR7) & ~0x155);
> -
>  	/*
>  	 * TODO: What about debug traps on tss switch?
>  	 *       Are we supposed to inject them and update dr6?
> diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
> index 2046be4..a170c35 100644
> --- a/arch/x86/kvm/x86.c
> +++ b/arch/x86/kvm/x86.c
> @@ -6816,6 +6816,9 @@ int kvm_task_switch(struct kvm_vcpu *vcpu, u16 tss_selector, int idt_index,
>  
>  	kvm_rip_write(vcpu, ctxt->eip);
>  	kvm_set_rflags(vcpu, ctxt->eflags);
> +	ret = __kvm_set_dr(vcpu, 7, vcpu->arch.dr7 & ~(DR_LOCAL_ENABLE_MASK |
> +						       DR_LOCAL_SLOWDOWN));
> +	WARN_ON(ret != 0);
>  	kvm_make_request(KVM_REQ_EVENT, vcpu);
>  	return EMULATE_DONE;
>  }
> 

Re: [PATCH] KVM: x86: Mask DR7 correctly on task-switch while debugging

[Nadav Amit]

Paolo Bonzini <pbonzini@redhat.com> wrote:

> 
> 
> On 19/04/2015 14:18, Nadav Amit wrote:
>> If the host sets hardware breakpoints to debug the guest, and a task-switch
>> occurs in the guest, the architectural DR7 will not be updated. The effective
>> DR7 would be updated instead.
>> 
>> This fix uses the standard DR setting mechanism instead of the one that was
>> previously used. As a bonus, the update of DR7 will now be effective for AMD as
>> well.
> 
> Is there a reason not to do it in emulate.c instead?

Not that I can think of. I will make a new version.

Nadav

[PATCH v2] KVM: x86: Fix DR7 mask on task-switch while debugging

[Nadav Amit]

If the host sets hardware breakpoints to debug the guest, and a task-switch
occurs in the guest, the architectural DR7 will not be updated. The effective
DR7 would be updated instead.

This fix puts the DR7 update during task-switch emulation, so it now uses the
standard DR setting mechanism instead of the one that was previously used. As a
bonus, the update of DR7 will now be effective for AMD as well.

Signed-off-by: Nadav Amit <namit@cs.technion.ac.il>

---

v1 -> v2:
- Move the setting to emulate.c instead of x86.c
- Shorten title
---
 arch/x86/kvm/emulate.c | 6 +++++-
 arch/x86/kvm/vmx.c     | 3 ---
 2 files changed, 5 insertions(+), 4 deletions(-)

diff --git a/arch/x86/kvm/emulate.c b/arch/x86/kvm/emulate.c
index 630bcb0..4a4555a 100644
--- a/arch/x86/kvm/emulate.c
+++ b/arch/x86/kvm/emulate.c
@@ -25,6 +25,7 @@
 #include <linux/module.h>
 #include <asm/kvm_emulate.h>
 #include <linux/stringify.h>
+#include <asm/debugreg.h>
 
 #include "x86.h"
 #include "tss.h"
@@ -2849,7 +2850,7 @@ static int emulator_do_task_switch(struct x86_emulate_ctxt *ctxt,
 	ulong old_tss_base =
 		ops->get_cached_segment_base(ctxt, VCPU_SREG_TR);
 	u32 desc_limit;
-	ulong desc_addr;
+	ulong desc_addr, dr7;
 
 	/* FIXME: old_tss_base == ~0 ? */
 
@@ -2934,6 +2935,9 @@ static int emulator_do_task_switch(struct x86_emulate_ctxt *ctxt,
 		ret = em_push(ctxt);
 	}
 
+	ops->get_dr(ctxt, 7, &dr7);
+	ops->set_dr(ctxt, 7, dr7 & ~(DR_LOCAL_ENABLE_MASK | DR_LOCAL_SLOWDOWN));
+
 	return ret;
 }
 
diff --git a/arch/x86/kvm/vmx.c b/arch/x86/kvm/vmx.c
index f7a0a7f..8f731c0 100644
--- a/arch/x86/kvm/vmx.c
+++ b/arch/x86/kvm/vmx.c
@@ -5703,9 +5703,6 @@ static int handle_task_switch(struct kvm_vcpu *vcpu)
 		return 0;
 	}
 
-	/* clear all local breakpoint enable flags */
-	vmcs_writel(GUEST_DR7, vmcs_readl(GUEST_DR7) & ~0x155);
-
 	/*
 	 * TODO: What about debug traps on tss switch?
 	 *       Are we supposed to inject them and update dr6?
-- 
1.9.1

Re: [PATCH v2] KVM: x86: Fix DR7 mask on task-switch while debugging

[Nadav Amit]

Paolo,

Please note that the following patch, as well as the patch-set "KVM: x86:
Fix REP-string effect on RCX/RSI/RDI” are still pending.

I still owe you a fix for guest DEBUGCTL, and some cleanup for vmx cleanup
code. I will send them once you finish reviewing the previous ones.

Thanks,
Nadav

Nadav Amit <namit@cs.technion.ac.il> wrote:

> If the host sets hardware breakpoints to debug the guest, and a task-switch
> occurs in the guest, the architectural DR7 will not be updated. The effective
> DR7 would be updated instead.
> 
> This fix puts the DR7 update during task-switch emulation, so it now uses the
> standard DR setting mechanism instead of the one that was previously used. As a
> bonus, the update of DR7 will now be effective for AMD as well.
> 
> Signed-off-by: Nadav Amit <namit@cs.technion.ac.il>
> 
> ---
> 
> v1 -> v2:
> - Move the setting to emulate.c instead of x86.c
> - Shorten title
> ---
> arch/x86/kvm/emulate.c | 6 +++++-
> arch/x86/kvm/vmx.c     | 3 ---
> 2 files changed, 5 insertions(+), 4 deletions(-)
> 
> diff --git a/arch/x86/kvm/emulate.c b/arch/x86/kvm/emulate.c
> index 630bcb0..4a4555a 100644
> --- a/arch/x86/kvm/emulate.c
> +++ b/arch/x86/kvm/emulate.c
> @@ -25,6 +25,7 @@
> #include <linux/module.h>
> #include <asm/kvm_emulate.h>
> #include <linux/stringify.h>
> +#include <asm/debugreg.h>
> 
> #include "x86.h"
> #include "tss.h"
> @@ -2849,7 +2850,7 @@ static int emulator_do_task_switch(struct x86_emulate_ctxt *ctxt,
> 	ulong old_tss_base =
> 		ops->get_cached_segment_base(ctxt, VCPU_SREG_TR);
> 	u32 desc_limit;
> -	ulong desc_addr;
> +	ulong desc_addr, dr7;
> 
> 	/* FIXME: old_tss_base == ~0 ? */
> 
> @@ -2934,6 +2935,9 @@ static int emulator_do_task_switch(struct x86_emulate_ctxt *ctxt,
> 		ret = em_push(ctxt);
> 	}
> 
> +	ops->get_dr(ctxt, 7, &dr7);
> +	ops->set_dr(ctxt, 7, dr7 & ~(DR_LOCAL_ENABLE_MASK | DR_LOCAL_SLOWDOWN));
> +
> 	return ret;
> }
> 
> diff --git a/arch/x86/kvm/vmx.c b/arch/x86/kvm/vmx.c
> index f7a0a7f..8f731c0 100644
> --- a/arch/x86/kvm/vmx.c
> +++ b/arch/x86/kvm/vmx.c
> @@ -5703,9 +5703,6 @@ static int handle_task_switch(struct kvm_vcpu *vcpu)
> 		return 0;
> 	}
> 
> -	/* clear all local breakpoint enable flags */
> -	vmcs_writel(GUEST_DR7, vmcs_readl(GUEST_DR7) & ~0x155);
> -
> 	/*
> 	 * TODO: What about debug traps on tss switch?
> 	 *       Are we supposed to inject them and update dr6?
> -- 
> 1.9.1

Re: [PATCH v2] KVM: x86: Fix DR7 mask on task-switch while debugging

[Paolo Bonzini]

On 19/04/2015 20:12, Nadav Amit wrote:
> If the host sets hardware breakpoints to debug the guest, and a task-switch
> occurs in the guest, the architectural DR7 will not be updated. The effective
> DR7 would be updated instead.
> 
> This fix puts the DR7 update during task-switch emulation, so it now uses the
> standard DR setting mechanism instead of the one that was previously used. As a
> bonus, the update of DR7 will now be effective for AMD as well.
> 
> Signed-off-by: Nadav Amit <namit@cs.technion.ac.il>
> 
> ---
> 
> v1 -> v2:
> - Move the setting to emulate.c instead of x86.c
> - Shorten title
> ---
>  arch/x86/kvm/emulate.c | 6 +++++-
>  arch/x86/kvm/vmx.c     | 3 ---
>  2 files changed, 5 insertions(+), 4 deletions(-)
> 
> diff --git a/arch/x86/kvm/emulate.c b/arch/x86/kvm/emulate.c
> index 630bcb0..4a4555a 100644
> --- a/arch/x86/kvm/emulate.c
> +++ b/arch/x86/kvm/emulate.c
> @@ -25,6 +25,7 @@
>  #include <linux/module.h>
>  #include <asm/kvm_emulate.h>
>  #include <linux/stringify.h>
> +#include <asm/debugreg.h>
>  
>  #include "x86.h"
>  #include "tss.h"
> @@ -2849,7 +2850,7 @@ static int emulator_do_task_switch(struct x86_emulate_ctxt *ctxt,
>  	ulong old_tss_base =
>  		ops->get_cached_segment_base(ctxt, VCPU_SREG_TR);
>  	u32 desc_limit;
> -	ulong desc_addr;
> +	ulong desc_addr, dr7;
>  
>  	/* FIXME: old_tss_base == ~0 ? */
>  
> @@ -2934,6 +2935,9 @@ static int emulator_do_task_switch(struct x86_emulate_ctxt *ctxt,
>  		ret = em_push(ctxt);
>  	}
>  
> +	ops->get_dr(ctxt, 7, &dr7);
> +	ops->set_dr(ctxt, 7, dr7 & ~(DR_LOCAL_ENABLE_MASK | DR_LOCAL_SLOWDOWN));
> +
>  	return ret;
>  }
>  
> diff --git a/arch/x86/kvm/vmx.c b/arch/x86/kvm/vmx.c
> index f7a0a7f..8f731c0 100644
> --- a/arch/x86/kvm/vmx.c
> +++ b/arch/x86/kvm/vmx.c
> @@ -5703,9 +5703,6 @@ static int handle_task_switch(struct kvm_vcpu *vcpu)
>  		return 0;
>  	}
>  
> -	/* clear all local breakpoint enable flags */
> -	vmcs_writel(GUEST_DR7, vmcs_readl(GUEST_DR7) & ~0x155);
> -
>  	/*
>  	 * TODO: What about debug traps on tss switch?
>  	 *       Are we supposed to inject them and update dr6?
> 

Applied, thanks.

Paolo