[PATCH] KVM: SVM: handle singlestep exception when skipping emulated instructions

[PATCH] KVM: SVM: handle singlestep exception when skipping emulated instructions

[Ladi Prosek]

kvm_skip_emulated_instruction handles the singlestep debug exception
which is something we almost always want. This commit (specifically
the change in rdmsr_interception) makes the debug.flat KVM unit test
pass on AMD.

Two call sites still call skip_emulated_instruction directly:

* In svm_queue_exception where it's used only for moving the rip forward

* In task_switch_interception which is analogous to handle_task_switch
  in VMX

Signed-off-by: Ladi Prosek <lprosek@redhat.com>
---
 arch/x86/kvm/svm.c | 24 ++++++++++++------------
 1 file changed, 12 insertions(+), 12 deletions(-)

diff --git a/arch/x86/kvm/svm.c b/arch/x86/kvm/svm.c
index 4250f9a..472a1c1 100644
--- a/arch/x86/kvm/svm.c
+++ b/arch/x86/kvm/svm.c
@@ -2278,7 +2278,7 @@ static int io_interception(struct vcpu_svm *svm)
 	port = io_info >> 16;
 	size = (io_info & SVM_IOIO_SIZE_MASK) >> SVM_IOIO_SIZE_SHIFT;
 	svm->next_rip = svm->vmcb->control.exit_info_2;
-	skip_emulated_instruction(&svm->vcpu);
+	kvm_skip_emulated_instruction(&svm->vcpu);
 
 	return in ? kvm_fast_pio_in(vcpu, size, port)
 		  : kvm_fast_pio_out(vcpu, size, port);
@@ -3063,7 +3063,7 @@ static int vmload_interception(struct vcpu_svm *svm)
 		return 1;
 
 	svm->next_rip = kvm_rip_read(&svm->vcpu) + 3;
-	skip_emulated_instruction(&svm->vcpu);
+	kvm_skip_emulated_instruction(&svm->vcpu);
 
 	nested_svm_vmloadsave(nested_vmcb, svm->vmcb);
 	nested_svm_unmap(page);
@@ -3084,7 +3084,7 @@ static int vmsave_interception(struct vcpu_svm *svm)
 		return 1;
 
 	svm->next_rip = kvm_rip_read(&svm->vcpu) + 3;
-	skip_emulated_instruction(&svm->vcpu);
+	kvm_skip_emulated_instruction(&svm->vcpu);
 
 	nested_svm_vmloadsave(svm->vmcb, nested_vmcb);
 	nested_svm_unmap(page);
@@ -3126,7 +3126,7 @@ static int stgi_interception(struct vcpu_svm *svm)
 		return 1;
 
 	svm->next_rip = kvm_rip_read(&svm->vcpu) + 3;
-	skip_emulated_instruction(&svm->vcpu);
+	kvm_skip_emulated_instruction(&svm->vcpu);
 	kvm_make_request(KVM_REQ_EVENT, &svm->vcpu);
 
 	enable_gif(svm);
@@ -3140,7 +3140,7 @@ static int clgi_interception(struct vcpu_svm *svm)
 		return 1;
 
 	svm->next_rip = kvm_rip_read(&svm->vcpu) + 3;
-	skip_emulated_instruction(&svm->vcpu);
+	kvm_skip_emulated_instruction(&svm->vcpu);
 
 	disable_gif(svm);
 
@@ -3165,7 +3165,7 @@ static int invlpga_interception(struct vcpu_svm *svm)
 	kvm_mmu_invlpg(vcpu, kvm_register_read(&svm->vcpu, VCPU_REGS_RAX));
 
 	svm->next_rip = kvm_rip_read(&svm->vcpu) + 3;
-	skip_emulated_instruction(&svm->vcpu);
+	kvm_skip_emulated_instruction(&svm->vcpu);
 	return 1;
 }
 
@@ -3189,7 +3189,7 @@ static int xsetbv_interception(struct vcpu_svm *svm)
 
 	if (kvm_set_xcr(&svm->vcpu, index, new_bv) == 0) {
 		svm->next_rip = kvm_rip_read(&svm->vcpu) + 3;
-		skip_emulated_instruction(&svm->vcpu);
+		kvm_skip_emulated_instruction(&svm->vcpu);
 	}
 
 	return 1;
@@ -3285,7 +3285,7 @@ static int invlpg_interception(struct vcpu_svm *svm)
 		return emulate_instruction(&svm->vcpu, 0) == EMULATE_DONE;
 
 	kvm_mmu_invlpg(&svm->vcpu, svm->vmcb->control.exit_info_1);
-	skip_emulated_instruction(&svm->vcpu);
+	kvm_skip_emulated_instruction(&svm->vcpu);
 	return 1;
 }
 
@@ -3436,7 +3436,7 @@ static int dr_interception(struct vcpu_svm *svm)
 		kvm_register_write(&svm->vcpu, reg, val);
 	}
 
-	skip_emulated_instruction(&svm->vcpu);
+	kvm_skip_emulated_instruction(&svm->vcpu);
 
 	return 1;
 }
@@ -3569,7 +3569,7 @@ static int rdmsr_interception(struct vcpu_svm *svm)
 		kvm_register_write(&svm->vcpu, VCPU_REGS_RDX,
 				   msr_info.data >> 32);
 		svm->next_rip = kvm_rip_read(&svm->vcpu) + 2;
-		skip_emulated_instruction(&svm->vcpu);
+		kvm_skip_emulated_instruction(&svm->vcpu);
 	}
 	return 1;
 }
@@ -3699,7 +3699,7 @@ static int wrmsr_interception(struct vcpu_svm *svm)
 		kvm_inject_gp(&svm->vcpu, 0);
 	} else {
 		trace_kvm_msr_write(ecx, data);
-		skip_emulated_instruction(&svm->vcpu);
+		kvm_skip_emulated_instruction(&svm->vcpu);
 	}
 	return 1;
 }
@@ -3730,7 +3730,7 @@ static int pause_interception(struct vcpu_svm *svm)
 
 static int nop_interception(struct vcpu_svm *svm)
 {
-	skip_emulated_instruction(&(svm->vcpu));
+	kvm_skip_emulated_instruction(&(svm->vcpu));
 	return 1;
 }
 
-- 
2.9.3

Re: [PATCH] KVM: SVM: handle singlestep exception when skipping emulated instructions

[Radim Krčmář]

2017-06-21 15:37+0200, Ladi Prosek:
> kvm_skip_emulated_instruction handles the singlestep debug exception
> which is something we almost always want. This commit (specifically
> the change in rdmsr_interception) makes the debug.flat KVM unit test
> pass on AMD.

kvm_skip_emulated_instruction() also has a return value, which says
whether the debug exception was requested by the userspace or by the
guest (userspace has priority).

This patch fixes the guest debugging, but userspace still won't receive
its events.  I think it would be better to fix both at once,

> Signed-off-by: Ladi Prosek <lprosek@redhat.com>
> ---
> diff --git a/arch/x86/kvm/svm.c b/arch/x86/kvm/svm.c
> @@ -2278,7 +2278,7 @@ static int io_interception(struct vcpu_svm *svm)
>  	port = io_info >> 16;
>  	size = (io_info & SVM_IOIO_SIZE_MASK) >> SVM_IOIO_SIZE_SHIFT;
>  	svm->next_rip = svm->vmcb->control.exit_info_2;
> -	skip_emulated_instruction(&svm->vcpu);
> +	kvm_skip_emulated_instruction(&svm->vcpu);
>  
>  	return in ? kvm_fast_pio_in(vcpu, size, port)
>  		  : kvm_fast_pio_out(vcpu, size, port);

i.e.
	ret = kvm_skip_emulated_instruction(&svm->vcpu);

	return ret && (...);

> @@ -3063,7 +3063,7 @@ static int vmload_interception(struct vcpu_svm *svm)
>  		return 1;
>  
>  	svm->next_rip = kvm_rip_read(&svm->vcpu) + 3;
> -	skip_emulated_instruction(&svm->vcpu);
> +	kvm_skip_emulated_instruction(&svm->vcpu);

	ret = kvm_skip_emulated_instruction(&svm->vcpu);
>  
>  	nested_svm_vmloadsave(nested_vmcb, svm->vmcb);
>  	nested_svm_unmap(page);

	return ret;

and so on ... thanks.

Re: [PATCH] KVM: SVM: handle singlestep exception when skipping emulated instructions

[Radim Krčmář]

2017-06-21 18:55+0200, Radim Krčmář:
> 2017-06-21 15:37+0200, Ladi Prosek:
> > kvm_skip_emulated_instruction handles the singlestep debug exception
> > which is something we almost always want. This commit (specifically
> > the change in rdmsr_interception) makes the debug.flat KVM unit test
> > pass on AMD.
> 
> kvm_skip_emulated_instruction() also has a return value, which says
> whether the debug exception was requested by the userspace or by the
> guest (userspace has priority).
> 
> This patch fixes the guest debugging, but userspace still won't receive
> its events.  I think it would be better to fix both at once,
> 
> > Signed-off-by: Ladi Prosek <lprosek@redhat.com>
> > ---
> > diff --git a/arch/x86/kvm/svm.c b/arch/x86/kvm/svm.c
> > @@ -2278,7 +2278,7 @@ static int io_interception(struct vcpu_svm *svm)
> >  	port = io_info >> 16;
> >  	size = (io_info & SVM_IOIO_SIZE_MASK) >> SVM_IOIO_SIZE_SHIFT;
> >  	svm->next_rip = svm->vmcb->control.exit_info_2;
> > -	skip_emulated_instruction(&svm->vcpu);
> > +	kvm_skip_emulated_instruction(&svm->vcpu);
> >  
> >  	return in ? kvm_fast_pio_in(vcpu, size, port)
> >  		  : kvm_fast_pio_out(vcpu, size, port);
> 
> i.e.
> 	ret = kvm_skip_emulated_instruction(&svm->vcpu);
> 
> 	return ret && (...);

Nope, the ret has to be checked afterwards ... better look at
handle_io() in vmx.c. :)

Re: [PATCH] KVM: SVM: handle singlestep exception when skipping emulated instructions

[Ladi Prosek]

On Wed, Jun 21, 2017 at 6:55 PM, Radim Krčmář <rkrcmar@redhat.com> wrote:
> 2017-06-21 15:37+0200, Ladi Prosek:
>> kvm_skip_emulated_instruction handles the singlestep debug exception
>> which is something we almost always want. This commit (specifically
>> the change in rdmsr_interception) makes the debug.flat KVM unit test
>> pass on AMD.
>
> kvm_skip_emulated_instruction() also has a return value, which says
> whether the debug exception was requested by the userspace or by the
> guest (userspace has priority).
>
> This patch fixes the guest debugging, but userspace still won't receive
> its events.  I think it would be better to fix both at once,

Will do, thanks!