[PATCH 0/2] KVM: x86: minimal debugging support during emulation

[PATCH 0/2] KVM: x86: minimal debugging support during emulation

[Paolo Bonzini]

The switch to emulate_invalid_guest_state brought one slightly unwelcome
change; the debugging interface does not work in the initial boot phase
because KVM_SET_GUEST_DEBUG is basically ignored.

These two patches bring some initial support for debugging, namely for
hardware breakpoints and single-stepping.  Software breakpoints and
hardware watchpoints are still ignored.

Paolo

v1->v2: rename EMULATE_DO_MMIO to EMULATE_USER_EXIT;
    return EMULATE_* value via pointer-to-int argument.

Paolo Bonzini (3):
  KVM: x86: rename EMULATE_DO_MMIO
  KVM: x86: handle hardware breakpoints during emulation
  KVM: x86: handle singlestep during emulation

 arch/x86/include/asm/kvm_host.h |   4 +-
 arch/x86/kvm/mmu.c              |   2 +-
 arch/x86/kvm/vmx.c              |   2 +-
 arch/x86/kvm/x86.c              | 109 ++++++++++++++++++++++++++++++++++++++--
 4 files changed, 110 insertions(+), 7 deletions(-)

-- 
1.8.1.4

[PATCH 1/3] KVM: x86: rename EMULATE_DO_MMIO

[Paolo Bonzini]

The next patch will reuse it for other userspace exits than MMIO,
namely debug events.

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 arch/x86/include/asm/kvm_host.h | 4 ++--
 arch/x86/kvm/mmu.c              | 2 +-
 arch/x86/kvm/vmx.c              | 2 +-
 arch/x86/kvm/x86.c              | 4 ++--
 4 files changed, 6 insertions(+), 6 deletions(-)

diff --git a/arch/x86/include/asm/kvm_host.h b/arch/x86/include/asm/kvm_host.h
index 1f98c1b..b33e9dc 100644
--- a/arch/x86/include/asm/kvm_host.h
+++ b/arch/x86/include/asm/kvm_host.h
@@ -795,8 +795,8 @@ extern u32  kvm_min_guest_tsc_khz;
 extern u32  kvm_max_guest_tsc_khz;
 
 enum emulation_result {
-	EMULATE_DONE,       /* no further processing */
-	EMULATE_DO_MMIO,      /* kvm_run filled with mmio request */
+	EMULATE_DONE,         /* no further processing */
+	EMULATE_USER_EXIT,    /* kvm_run ready for userspace exit */
 	EMULATE_FAIL,         /* can't emulate this instruction */
 };
 
diff --git a/arch/x86/kvm/mmu.c b/arch/x86/kvm/mmu.c
index 6941fa7..7ee30d4 100644
--- a/arch/x86/kvm/mmu.c
+++ b/arch/x86/kvm/mmu.c
@@ -4096,7 +4096,7 @@ int kvm_mmu_page_fault(struct kvm_vcpu *vcpu, gva_t cr2, u32 error_code,
 	switch (er) {
 	case EMULATE_DONE:
 		return 1;
-	case EMULATE_DO_MMIO:
+	case EMULATE_USER_EXIT:
 		++vcpu->stat.mmio_exits;
 		/* fall through */
 	case EMULATE_FAIL:
diff --git a/arch/x86/kvm/vmx.c b/arch/x86/kvm/vmx.c
index 260a919..0c87015 100644
--- a/arch/x86/kvm/vmx.c
+++ b/arch/x86/kvm/vmx.c
@@ -5422,7 +5422,7 @@ static int handle_invalid_guest_state(struct kvm_vcpu *vcpu)
 
 		err = emulate_instruction(vcpu, EMULTYPE_NO_REEXECUTE);
 
-		if (err == EMULATE_DO_MMIO) {
+		if (err == EMULATE_USER_EXIT) {
 			ret = 0;
 			goto out;
 		}
diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
index 737c804..c2a0674 100644
--- a/arch/x86/kvm/x86.c
+++ b/arch/x86/kvm/x86.c
@@ -5038,11 +5038,11 @@ restart:
 			writeback = false;
 			vcpu->arch.complete_userspace_io = complete_emulated_pio;
 		}
-		r = EMULATE_DO_MMIO;
+		r = EMULATE_USER_EXIT;
 	} else if (vcpu->mmio_needed) {
 		if (!vcpu->mmio_is_write)
 			writeback = false;
-		r = EMULATE_DO_MMIO;
+		r = EMULATE_USER_EXIT;
 		vcpu->arch.complete_userspace_io = complete_emulated_mmio;
 	} else if (r == EMULATION_RESTART)
 		goto restart;
-- 
1.8.1.4

[PATCH 2/3] KVM: x86: handle hardware breakpoints during emulation

[Paolo Bonzini]

This lets debugging work better during emulation of invalid
guest state.

The check is done before emulating the instruction, and (in the case
of guest debugging) reuses EMULATE_DO_MMIO to exit with KVM_EXIT_DEBUG.

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 arch/x86/kvm/x86.c | 66 ++++++++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 66 insertions(+)

diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
index c2a0674..1368cf5 100644
--- a/arch/x86/kvm/x86.c
+++ b/arch/x86/kvm/x86.c
@@ -4956,6 +4956,62 @@ static bool retry_instruction(struct x86_emulate_ctxt *ctxt,
 static int complete_emulated_mmio(struct kvm_vcpu *vcpu);
 static int complete_emulated_pio(struct kvm_vcpu *vcpu);
 
+static int kvm_vcpu_check_hw_bp(unsigned long addr, u32 type, u32 dr7,
+				unsigned long *db)
+{
+	u32 dr6 = 0;
+	int i;
+	u32 enable, rwlen;
+
+	enable = dr7;
+	rwlen = dr7 >> 16;
+	for (i = 0; i < 4; i++, enable >>= 2, rwlen >>= 4)
+		if ((enable & 3) && (rwlen & 15) == type && db[i] == addr)
+			dr6 |= (1 << i);
+	return dr6;
+}
+
+static bool kvm_vcpu_check_breakpoint(struct kvm_vcpu *vcpu, int *r)
+{
+	struct kvm_run *kvm_run = vcpu->run;
+	unsigned long eip = vcpu->arch.emulate_ctxt.eip;
+	u32 dr6 = 0;
+
+	if (unlikely(vcpu->guest_debug & KVM_GUESTDBG_USE_HW_BP) &&
+	    (vcpu->arch.guest_debug_dr7 & DR7_BP_EN_MASK)) {
+		dr6 = kvm_vcpu_check_hw_bp(eip, 0,
+					   vcpu->arch.guest_debug_dr7,
+					   vcpu->arch.eff_db);
+
+		if (dr6 != 0) {
+			kvm_run->debug.arch.dr6 = dr6 | DR6_FIXED_1;
+			kvm_run->debug.arch.pc = kvm_rip_read(vcpu) +
+				get_segment_base(vcpu, VCPU_SREG_CS);
+
+			kvm_run->debug.arch.exception = DB_VECTOR;
+			kvm_run->exit_reason = KVM_EXIT_DEBUG;
+			*r = EMULATE_USER_EXIT;
+			return true;
+		}
+	}
+
+	if (unlikely(vcpu->arch.dr7 & DR7_BP_EN_MASK)) {
+		dr6 = kvm_vcpu_check_hw_bp(eip, 0,
+					   vcpu->arch.dr7,
+					   vcpu->arch.db);
+
+		if (dr6 != 0) {
+			vcpu->arch.dr6 &= ~15;
+			vcpu->arch.dr6 |= dr6;
+			kvm_queue_exception(vcpu, DB_VECTOR);
+			*r = EMULATE_DONE;
+			return true;
+		}
+	}
+
+	return false;
+}
+
 int x86_emulate_instruction(struct kvm_vcpu *vcpu,
 			    unsigned long cr2,
 			    int emulation_type,
@@ -4976,6 +5032,16 @@ int x86_emulate_instruction(struct kvm_vcpu *vcpu,
 
 	if (!(emulation_type & EMULTYPE_NO_DECODE)) {
 		init_emulate_ctxt(vcpu);
+
+		/*
+		 * We will reenter on the same instruction since
+		 * we do not set complete_userspace_io.  This does not
+		 * handle watchpoints yet, those would be handled in
+		 * the emulate_ops.
+		 */
+		if (kvm_vcpu_check_breakpoint(vcpu, &r))
+			return r;
+
 		ctxt->interruptibility = 0;
 		ctxt->have_exception = false;
 		ctxt->perm_ok = false;
-- 
1.8.1.4

[PATCH 3/3] KVM: x86: handle singlestep during emulation

[Paolo Bonzini]

This lets debugging work better during emulation of invalid
guest state.

This time the check is done after emulation, but before writeback
of the flags; we need to check the flags *before* execution of the
instruction, we cannot check singlestep_rip because the CS base may
have already been modified.

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 arch/x86/kvm/x86.c | 39 ++++++++++++++++++++++++++++++++++++++-
 1 file changed, 38 insertions(+), 1 deletion(-)

diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
index 1368cf5..9805cfd 100644
--- a/arch/x86/kvm/x86.c
+++ b/arch/x86/kvm/x86.c
@@ -4971,6 +4971,41 @@ static int kvm_vcpu_check_hw_bp(unsigned long addr, u32 type, u32 dr7,
 	return dr6;
 }
 
+static void kvm_vcpu_check_singlestep(struct kvm_vcpu *vcpu, int *r)
+{
+	struct kvm_run *kvm_run = vcpu->run;
+
+	/*
+	 * Use the "raw" value to see if TF was passed to the processor.
+	 * Note that the new value of the flags has not been saved yet.
+	 *
+	 * This is correct even for TF set by the guest, because "the
+	 * processor will not generate this exception after the instruction
+	 * that sets the TF flag".
+	 */
+	unsigned long rflags = kvm_x86_ops->get_rflags(vcpu);
+
+	if (unlikely(rflags & X86_EFLAGS_TF)) {
+		if (vcpu->guest_debug & KVM_GUESTDBG_SINGLESTEP) {
+			kvm_run->debug.arch.dr6 = DR6_BS | DR6_FIXED_1;
+			kvm_run->debug.arch.pc = vcpu->arch.singlestep_rip;
+			kvm_run->debug.arch.exception = DB_VECTOR;
+			kvm_run->exit_reason = KVM_EXIT_DEBUG;
+			*r = EMULATE_USER_EXIT;
+		} else {
+			vcpu->arch.emulate_ctxt.eflags &= ~X86_EFLAGS_TF;
+			/*
+			 * "Certain debug exceptions may clear bit 0-3.  The
+			 * remaining contents of the DR6 register are never
+			 * cleared by the processor".
+			 */
+			vcpu->arch.dr6 &= ~15;
+			vcpu->arch.dr6 |= DR6_BS;
+			kvm_queue_exception(vcpu, DB_VECTOR);
+		}
+	}
+}
+
 static bool kvm_vcpu_check_breakpoint(struct kvm_vcpu *vcpu, int *r)
 {
 	struct kvm_run *kvm_run = vcpu->run;
@@ -5117,10 +5152,12 @@ restart:
 
 	if (writeback) {
 		toggle_interruptibility(vcpu, ctxt->interruptibility);
-		kvm_set_rflags(vcpu, ctxt->eflags);
 		kvm_make_request(KVM_REQ_EVENT, vcpu);
 		vcpu->arch.emulate_regs_need_sync_to_vcpu = false;
 		kvm_rip_write(vcpu, ctxt->eip);
+		if (r == EMULATE_DONE)
+			kvm_vcpu_check_singlestep(vcpu, &r);
+		kvm_set_rflags(vcpu, ctxt->eflags);
 	} else
 		vcpu->arch.emulate_regs_need_sync_to_vcpu = true;
 
-- 
1.8.1.4

Re: [PATCH 1/3] KVM: x86: rename EMULATE_DO_MMIO

[Gleb Natapov]

On Tue, Jul 30, 2013 at 05:11:34PM +0200, Paolo Bonzini wrote:
> The next patch will reuse it for other userspace exits than MMIO,
> namely debug events.
> 
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
>  arch/x86/include/asm/kvm_host.h | 4 ++--
>  arch/x86/kvm/mmu.c              | 2 +-
>  arch/x86/kvm/vmx.c              | 2 +-
>  arch/x86/kvm/x86.c              | 4 ++--
>  4 files changed, 6 insertions(+), 6 deletions(-)
> 
> diff --git a/arch/x86/include/asm/kvm_host.h b/arch/x86/include/asm/kvm_host.h
> index 1f98c1b..b33e9dc 100644
> --- a/arch/x86/include/asm/kvm_host.h
> +++ b/arch/x86/include/asm/kvm_host.h
> @@ -795,8 +795,8 @@ extern u32  kvm_min_guest_tsc_khz;
>  extern u32  kvm_max_guest_tsc_khz;
>  
>  enum emulation_result {
> -	EMULATE_DONE,       /* no further processing */
> -	EMULATE_DO_MMIO,      /* kvm_run filled with mmio request */
> +	EMULATE_DONE,         /* no further processing */
> +	EMULATE_USER_EXIT,    /* kvm_run ready for userspace exit */
>  	EMULATE_FAIL,         /* can't emulate this instruction */
>  };
>  
> diff --git a/arch/x86/kvm/mmu.c b/arch/x86/kvm/mmu.c
> index 6941fa7..7ee30d4 100644
> --- a/arch/x86/kvm/mmu.c
> +++ b/arch/x86/kvm/mmu.c
> @@ -4096,7 +4096,7 @@ int kvm_mmu_page_fault(struct kvm_vcpu *vcpu, gva_t cr2, u32 error_code,
>  	switch (er) {
>  	case EMULATE_DONE:
>  		return 1;
> -	case EMULATE_DO_MMIO:
> +	case EMULATE_USER_EXIT:
>  		++vcpu->stat.mmio_exits;
>  		/* fall through */
>  	case EMULATE_FAIL:
> diff --git a/arch/x86/kvm/vmx.c b/arch/x86/kvm/vmx.c
> index 260a919..0c87015 100644
> --- a/arch/x86/kvm/vmx.c
> +++ b/arch/x86/kvm/vmx.c
> @@ -5422,7 +5422,7 @@ static int handle_invalid_guest_state(struct kvm_vcpu *vcpu)
>  
>  		err = emulate_instruction(vcpu, EMULTYPE_NO_REEXECUTE);
>  
> -		if (err == EMULATE_DO_MMIO) {
> +		if (err == EMULATE_USER_EXIT) {
Not related to the patch, but here we need ++vcpu->stat.mmio_exits too.
And mmio_exits is not really about mmios any more, oh well.

The patch itself looks good.
>  			ret = 0;
>  			goto out;
>  		}
> diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
> index 737c804..c2a0674 100644
> --- a/arch/x86/kvm/x86.c
> +++ b/arch/x86/kvm/x86.c
> @@ -5038,11 +5038,11 @@ restart:
>  			writeback = false;
>  			vcpu->arch.complete_userspace_io = complete_emulated_pio;
>  		}
> -		r = EMULATE_DO_MMIO;
> +		r = EMULATE_USER_EXIT;
>  	} else if (vcpu->mmio_needed) {
>  		if (!vcpu->mmio_is_write)
>  			writeback = false;
> -		r = EMULATE_DO_MMIO;
> +		r = EMULATE_USER_EXIT;
>  		vcpu->arch.complete_userspace_io = complete_emulated_mmio;
>  	} else if (r == EMULATION_RESTART)
>  		goto restart;
> -- 
> 1.8.1.4
> 

--
			Gleb.

Re: [PATCH 3/3] KVM: x86: handle singlestep during emulation

[Gleb Natapov]

On Tue, Jul 30, 2013 at 05:11:36PM +0200, Paolo Bonzini wrote:
> This lets debugging work better during emulation of invalid
> guest state.
> 
> This time the check is done after emulation, but before writeback
> of the flags; we need to check the flags *before* execution of the
> instruction, we cannot check singlestep_rip because the CS base may
> have already been modified.
> 
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
>  arch/x86/kvm/x86.c | 39 ++++++++++++++++++++++++++++++++++++++-
>  1 file changed, 38 insertions(+), 1 deletion(-)
> 
> diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
> index 1368cf5..9805cfd 100644
> --- a/arch/x86/kvm/x86.c
> +++ b/arch/x86/kvm/x86.c
> @@ -4971,6 +4971,41 @@ static int kvm_vcpu_check_hw_bp(unsigned long addr, u32 type, u32 dr7,
>  	return dr6;
>  }
>  
> +static void kvm_vcpu_check_singlestep(struct kvm_vcpu *vcpu, int *r)
> +{
> +	struct kvm_run *kvm_run = vcpu->run;
> +
> +	/*
> +	 * Use the "raw" value to see if TF was passed to the processor.
> +	 * Note that the new value of the flags has not been saved yet.
> +	 *
> +	 * This is correct even for TF set by the guest, because "the
> +	 * processor will not generate this exception after the instruction
> +	 * that sets the TF flag".
> +	 */
> +	unsigned long rflags = kvm_x86_ops->get_rflags(vcpu);
> +
> +	if (unlikely(rflags & X86_EFLAGS_TF)) {
> +		if (vcpu->guest_debug & KVM_GUESTDBG_SINGLESTEP) {
> +			kvm_run->debug.arch.dr6 = DR6_BS | DR6_FIXED_1;
> +			kvm_run->debug.arch.pc = vcpu->arch.singlestep_rip;
> +			kvm_run->debug.arch.exception = DB_VECTOR;
> +			kvm_run->exit_reason = KVM_EXIT_DEBUG;
> +			*r = EMULATE_USER_EXIT;
> +		} else {
> +			vcpu->arch.emulate_ctxt.eflags &= ~X86_EFLAGS_TF;
> +			/*
> +			 * "Certain debug exceptions may clear bit 0-3.  The
> +			 * remaining contents of the DR6 register are never
> +			 * cleared by the processor".
> +			 */
> +			vcpu->arch.dr6 &= ~15;
> +			vcpu->arch.dr6 |= DR6_BS;
> +			kvm_queue_exception(vcpu, DB_VECTOR);
> +		}
> +	}
> +}
> +
>  static bool kvm_vcpu_check_breakpoint(struct kvm_vcpu *vcpu, int *r)
>  {
>  	struct kvm_run *kvm_run = vcpu->run;
> @@ -5117,10 +5152,12 @@ restart:
>  
>  	if (writeback) {
>  		toggle_interruptibility(vcpu, ctxt->interruptibility);
> -		kvm_set_rflags(vcpu, ctxt->eflags);
>  		kvm_make_request(KVM_REQ_EVENT, vcpu);
>  		vcpu->arch.emulate_regs_need_sync_to_vcpu = false;
>  		kvm_rip_write(vcpu, ctxt->eip);
> +		if (r == EMULATE_DONE)
Single step will not work for mmio write and pio out, we never return
into emulator for those instructions.

> +			kvm_vcpu_check_singlestep(vcpu, &r);
> +		kvm_set_rflags(vcpu, ctxt->eflags);
>  	} else
>  		vcpu->arch.emulate_regs_need_sync_to_vcpu = true;
>  
> -- 
> 1.8.1.4

--
			Gleb.

Re: [PATCH 2/3] KVM: x86: handle hardware breakpoints during emulation

[Gleb Natapov]

On Tue, Jul 30, 2013 at 05:11:35PM +0200, Paolo Bonzini wrote:
> This lets debugging work better during emulation of invalid
> guest state.
> 
> The check is done before emulating the instruction, and (in the case
> of guest debugging) reuses EMULATE_DO_MMIO to exit with KVM_EXIT_DEBUG.
> 
Not EMULATE_DO_MMIO any more. Otherwise looks good.

> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
>  arch/x86/kvm/x86.c | 66 ++++++++++++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 66 insertions(+)
> 
> diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
> index c2a0674..1368cf5 100644
> --- a/arch/x86/kvm/x86.c
> +++ b/arch/x86/kvm/x86.c
> @@ -4956,6 +4956,62 @@ static bool retry_instruction(struct x86_emulate_ctxt *ctxt,
>  static int complete_emulated_mmio(struct kvm_vcpu *vcpu);
>  static int complete_emulated_pio(struct kvm_vcpu *vcpu);
>  
> +static int kvm_vcpu_check_hw_bp(unsigned long addr, u32 type, u32 dr7,
> +				unsigned long *db)
> +{
> +	u32 dr6 = 0;
> +	int i;
> +	u32 enable, rwlen;
> +
> +	enable = dr7;
> +	rwlen = dr7 >> 16;
> +	for (i = 0; i < 4; i++, enable >>= 2, rwlen >>= 4)
> +		if ((enable & 3) && (rwlen & 15) == type && db[i] == addr)
> +			dr6 |= (1 << i);
> +	return dr6;
> +}
> +
> +static bool kvm_vcpu_check_breakpoint(struct kvm_vcpu *vcpu, int *r)
> +{
> +	struct kvm_run *kvm_run = vcpu->run;
> +	unsigned long eip = vcpu->arch.emulate_ctxt.eip;
> +	u32 dr6 = 0;
> +
> +	if (unlikely(vcpu->guest_debug & KVM_GUESTDBG_USE_HW_BP) &&
> +	    (vcpu->arch.guest_debug_dr7 & DR7_BP_EN_MASK)) {
> +		dr6 = kvm_vcpu_check_hw_bp(eip, 0,
> +					   vcpu->arch.guest_debug_dr7,
> +					   vcpu->arch.eff_db);
> +
> +		if (dr6 != 0) {
> +			kvm_run->debug.arch.dr6 = dr6 | DR6_FIXED_1;
> +			kvm_run->debug.arch.pc = kvm_rip_read(vcpu) +
> +				get_segment_base(vcpu, VCPU_SREG_CS);
> +
> +			kvm_run->debug.arch.exception = DB_VECTOR;
> +			kvm_run->exit_reason = KVM_EXIT_DEBUG;
> +			*r = EMULATE_USER_EXIT;
> +			return true;
> +		}
> +	}
> +
> +	if (unlikely(vcpu->arch.dr7 & DR7_BP_EN_MASK)) {
> +		dr6 = kvm_vcpu_check_hw_bp(eip, 0,
> +					   vcpu->arch.dr7,
> +					   vcpu->arch.db);
> +
> +		if (dr6 != 0) {
> +			vcpu->arch.dr6 &= ~15;
> +			vcpu->arch.dr6 |= dr6;
> +			kvm_queue_exception(vcpu, DB_VECTOR);
> +			*r = EMULATE_DONE;
> +			return true;
> +		}
> +	}
> +
> +	return false;
> +}
> +
>  int x86_emulate_instruction(struct kvm_vcpu *vcpu,
>  			    unsigned long cr2,
>  			    int emulation_type,
> @@ -4976,6 +5032,16 @@ int x86_emulate_instruction(struct kvm_vcpu *vcpu,
>  
>  	if (!(emulation_type & EMULTYPE_NO_DECODE)) {
>  		init_emulate_ctxt(vcpu);
> +
> +		/*
> +		 * We will reenter on the same instruction since
> +		 * we do not set complete_userspace_io.  This does not
> +		 * handle watchpoints yet, those would be handled in
> +		 * the emulate_ops.
> +		 */
> +		if (kvm_vcpu_check_breakpoint(vcpu, &r))
> +			return r;
> +
>  		ctxt->interruptibility = 0;
>  		ctxt->have_exception = false;
>  		ctxt->perm_ok = false;
> -- 
> 1.8.1.4
> 

--
			Gleb.

Re: [PATCH 3/3] KVM: x86: handle singlestep during emulation

[Paolo Bonzini]

----- Gleb Natapov <gleb@redhat.com> wrote:
> On Tue, Jul 30, 2013 at 05:11:36PM +0200, Paolo Bonzini wrote:
> > This lets debugging work better during emulation of invalid
> > guest state.
> > 
> > This time the check is done after emulation, but before writeback
> > of the flags; we need to check the flags *before* execution of the
> > instruction, we cannot check singlestep_rip because the CS base may
> > have already been modified.
> > 
> > Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> > ---
> >  arch/x86/kvm/x86.c | 39 ++++++++++++++++++++++++++++++++++++++-
> >  1 file changed, 38 insertions(+), 1 deletion(-)
> > 
> > diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
> > index 1368cf5..9805cfd 100644
> > --- a/arch/x86/kvm/x86.c
> > +++ b/arch/x86/kvm/x86.c
> > @@ -4971,6 +4971,41 @@ static int kvm_vcpu_check_hw_bp(unsigned long addr, u32 type, u32 dr7,
> >  	return dr6;
> >  }
> >  
> > +static void kvm_vcpu_check_singlestep(struct kvm_vcpu *vcpu, int *r)
> > +{
> > +	struct kvm_run *kvm_run = vcpu->run;
> > +
> > +	/*
> > +	 * Use the "raw" value to see if TF was passed to the processor.
> > +	 * Note that the new value of the flags has not been saved yet.
> > +	 *
> > +	 * This is correct even for TF set by the guest, because "the
> > +	 * processor will not generate this exception after the instruction
> > +	 * that sets the TF flag".
> > +	 */
> > +	unsigned long rflags = kvm_x86_ops->get_rflags(vcpu);
> > +
> > +	if (unlikely(rflags & X86_EFLAGS_TF)) {
> > +		if (vcpu->guest_debug & KVM_GUESTDBG_SINGLESTEP) {
> > +			kvm_run->debug.arch.dr6 = DR6_BS | DR6_FIXED_1;
> > +			kvm_run->debug.arch.pc = vcpu->arch.singlestep_rip;
> > +			kvm_run->debug.arch.exception = DB_VECTOR;
> > +			kvm_run->exit_reason = KVM_EXIT_DEBUG;
> > +			*r = EMULATE_USER_EXIT;
> > +		} else {
> > +			vcpu->arch.emulate_ctxt.eflags &= ~X86_EFLAGS_TF;
> > +			/*
> > +			 * "Certain debug exceptions may clear bit 0-3.  The
> > +			 * remaining contents of the DR6 register are never
> > +			 * cleared by the processor".
> > +			 */
> > +			vcpu->arch.dr6 &= ~15;
> > +			vcpu->arch.dr6 |= DR6_BS;
> > +			kvm_queue_exception(vcpu, DB_VECTOR);
> > +		}
> > +	}
> > +}
> > +
> >  static bool kvm_vcpu_check_breakpoint(struct kvm_vcpu *vcpu, int *r)
> >  {
> >  	struct kvm_run *kvm_run = vcpu->run;
> > @@ -5117,10 +5152,12 @@ restart:
> >  
> >  	if (writeback) {
> >  		toggle_interruptibility(vcpu, ctxt->interruptibility);
> > -		kvm_set_rflags(vcpu, ctxt->eflags);
> >  		kvm_make_request(KVM_REQ_EVENT, vcpu);
> >  		vcpu->arch.emulate_regs_need_sync_to_vcpu = false;
> >  		kvm_rip_write(vcpu, ctxt->eip);
> > +		if (r == EMULATE_DONE)
> Single step will not work for mmio write and pio out, we never return
> into emulator for those instructions.

Ok to apply the patch as is and work it out later (I suppose I need to
check for NULL complete_userspace_io, and if so set my function)?  It
is already a huge improvement in usability.

Paolo

> > +			kvm_vcpu_check_singlestep(vcpu, &r);
> > +		kvm_set_rflags(vcpu, ctxt->eflags);
> >  	} else
> >  		vcpu->arch.emulate_regs_need_sync_to_vcpu = true;
> >  
> > -- 
> > 1.8.1.4
> 
> --
> 			Gleb.

Re: [PATCH 3/3] KVM: x86: handle singlestep during emulation

[Gleb Natapov]

On Wed, Jul 31, 2013 at 06:30:31AM -0400, Paolo Bonzini wrote:
> 
> ----- Gleb Natapov <gleb@redhat.com> wrote:
> > On Tue, Jul 30, 2013 at 05:11:36PM +0200, Paolo Bonzini wrote:
> > > This lets debugging work better during emulation of invalid
> > > guest state.
> > > 
> > > This time the check is done after emulation, but before writeback
> > > of the flags; we need to check the flags *before* execution of the
> > > instruction, we cannot check singlestep_rip because the CS base may
> > > have already been modified.
> > > 
> > > Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> > > ---
> > >  arch/x86/kvm/x86.c | 39 ++++++++++++++++++++++++++++++++++++++-
> > >  1 file changed, 38 insertions(+), 1 deletion(-)
> > > 
> > > diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
> > > index 1368cf5..9805cfd 100644
> > > --- a/arch/x86/kvm/x86.c
> > > +++ b/arch/x86/kvm/x86.c
> > > @@ -4971,6 +4971,41 @@ static int kvm_vcpu_check_hw_bp(unsigned long addr, u32 type, u32 dr7,
> > >  	return dr6;
> > >  }
> > >  
> > > +static void kvm_vcpu_check_singlestep(struct kvm_vcpu *vcpu, int *r)
> > > +{
> > > +	struct kvm_run *kvm_run = vcpu->run;
> > > +
> > > +	/*
> > > +	 * Use the "raw" value to see if TF was passed to the processor.
> > > +	 * Note that the new value of the flags has not been saved yet.
> > > +	 *
> > > +	 * This is correct even for TF set by the guest, because "the
> > > +	 * processor will not generate this exception after the instruction
> > > +	 * that sets the TF flag".
> > > +	 */
> > > +	unsigned long rflags = kvm_x86_ops->get_rflags(vcpu);
> > > +
> > > +	if (unlikely(rflags & X86_EFLAGS_TF)) {
> > > +		if (vcpu->guest_debug & KVM_GUESTDBG_SINGLESTEP) {
> > > +			kvm_run->debug.arch.dr6 = DR6_BS | DR6_FIXED_1;
> > > +			kvm_run->debug.arch.pc = vcpu->arch.singlestep_rip;
> > > +			kvm_run->debug.arch.exception = DB_VECTOR;
> > > +			kvm_run->exit_reason = KVM_EXIT_DEBUG;
> > > +			*r = EMULATE_USER_EXIT;
> > > +		} else {
> > > +			vcpu->arch.emulate_ctxt.eflags &= ~X86_EFLAGS_TF;
> > > +			/*
> > > +			 * "Certain debug exceptions may clear bit 0-3.  The
> > > +			 * remaining contents of the DR6 register are never
> > > +			 * cleared by the processor".
> > > +			 */
> > > +			vcpu->arch.dr6 &= ~15;
> > > +			vcpu->arch.dr6 |= DR6_BS;
> > > +			kvm_queue_exception(vcpu, DB_VECTOR);
> > > +		}
> > > +	}
> > > +}
> > > +
> > >  static bool kvm_vcpu_check_breakpoint(struct kvm_vcpu *vcpu, int *r)
> > >  {
> > >  	struct kvm_run *kvm_run = vcpu->run;
> > > @@ -5117,10 +5152,12 @@ restart:
> > >  
> > >  	if (writeback) {
> > >  		toggle_interruptibility(vcpu, ctxt->interruptibility);
> > > -		kvm_set_rflags(vcpu, ctxt->eflags);
> > >  		kvm_make_request(KVM_REQ_EVENT, vcpu);
> > >  		vcpu->arch.emulate_regs_need_sync_to_vcpu = false;
> > >  		kvm_rip_write(vcpu, ctxt->eip);
> > > +		if (r == EMULATE_DONE)
> > Single step will not work for mmio write and pio out, we never return
> > into emulator for those instructions.
> 
> Ok to apply the patch as is and work it out later (I suppose I need to
> check for NULL complete_userspace_io, and if so set my function)?  It
> is already a huge improvement in usability.
> 
This is not worse from what we have now, so lets apply it, but please add
comment here that write case is not handled properly yet to not forget
about it.

complete_userspace_io is not null for write MMIO but the execution
never returns to emulator, so this is not so simple. May be set
vcpu->arch.io_singlestep and check it in complete_userspace_io.

--
			Gleb.