[RFC] Getting correct eip in skip_emulated_instruction()

[RFC] Getting correct eip in skip_emulated_instruction()

[Guillaume Thouvenin]

Hello,

 Presently when we want to skip an emulated instruction we update the
eip by reading the instruction length from VMCS structure and we add
this value to the current eip. It gives us the new eip. A problem occurs
when the guest state doesn't allow us to use VT because in that case,
the values in VMCS structure are not up to date. That means that if we
call skip_emulated_instruction() while guest state is invalid, the
computed eip will be false.

 To fix the problem I introduced a new field in the structure kvm_io to
store the eip value if guest state is invalid. If the stored value is
equal to 0 we read the length of the current instruction in VMCS and if
not, we use it as the new eip. It works but I'm not really happy with
that hack and I don't see how to solve the problem nicely.

 I attached the patch, thanks for comments,
 
Regards,
Guillaume

---
diff --git a/arch/x86/kvm/svm.c b/arch/x86/kvm/svm.c
index 05efc4e..315e70d 100644
--- a/arch/x86/kvm/svm.c
+++ b/arch/x86/kvm/svm.c
@@ -1099,7 +1099,7 @@ static int io_interception(struct vcpu_svm *svm, struct kvm_run *kvm_run)
 	rep = (io_info & SVM_IOIO_REP_MASK) != 0;
 	down = (svm->vmcb->save.rflags & X86_EFLAGS_DF) != 0;
 
-	return kvm_emulate_pio(&svm->vcpu, kvm_run, in, size, port);
+	return kvm_emulate_pio(&svm->vcpu, kvm_run, in, size, port, 0);
 }
 
 static int nmi_interception(struct vcpu_svm *svm, struct kvm_run *kvm_run)
diff --git a/arch/x86/kvm/vmx.c b/arch/x86/kvm/vmx.c
index 64e2439..9ba6583 100644
--- a/arch/x86/kvm/vmx.c
+++ b/arch/x86/kvm/vmx.c
@@ -726,9 +726,19 @@ static void skip_emulated_instruction(struct kvm_vcpu *vcpu)
 {
 	unsigned long rip;
 	u32 interruptibility;
+	struct vcpu_vmx *vmx = to_vmx(vcpu);
 
-	rip = kvm_rip_read(vcpu);
-	rip += vmcs_read32(VM_EXIT_INSTRUCTION_LEN);
+	/*
+	 * Not all cases receive valid value in the VM-exit instruction
+	 * length field.
+	 */
+	if (vcpu->run->io.rip && vmx->emulation_required && emulate_invalid_guest_state)
+		rip = vcpu->run->io.rip;
+	else {
+		rip = kvm_rip_read(vcpu);
+		rip += vmcs_read32(VM_EXIT_INSTRUCTION_LEN);
+	}
+	vcpu->run->io.rip = 0;
 	kvm_rip_write(vcpu, rip);
 
 	/*
@@ -2687,7 +2697,7 @@ static int handle_io(struct kvm_vcpu *vcpu, struct kvm_run *kvm_run)
 	rep = (exit_qualification & 32) != 0;
 	port = exit_qualification >> 16;
 
-	return kvm_emulate_pio(vcpu, kvm_run, in, size, port);
+	return kvm_emulate_pio(vcpu, kvm_run, in, size, port, 0);
 }
 
 static void
diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
index 883c137..29dd988 100644
--- a/arch/x86/kvm/x86.c
+++ b/arch/x86/kvm/x86.c
@@ -2451,7 +2451,7 @@ static struct kvm_io_device *vcpu_find_pio_dev(struct kvm_vcpu *vcpu,
 }
 
 int kvm_emulate_pio(struct kvm_vcpu *vcpu, struct kvm_run *run, int in,
-		  int size, unsigned port)
+		  int size, unsigned port, unsigned long rip)
 {
 	struct kvm_io_device *pio_dev;
 	unsigned long val;
@@ -2462,6 +2462,7 @@ int kvm_emulate_pio(struct kvm_vcpu *vcpu, struct kvm_run *run, int in,
 	vcpu->run->io.data_offset = KVM_PIO_PAGE_OFFSET * PAGE_SIZE;
 	vcpu->run->io.count = vcpu->arch.pio.count = vcpu->arch.pio.cur_count = 1;
 	vcpu->run->io.port = vcpu->arch.pio.port = port;
+	vcpu->run->io.rip = rip;
 	vcpu->arch.pio.in = in;
 	vcpu->arch.pio.string = 0;
 	vcpu->arch.pio.down = 0;
diff --git a/arch/x86/kvm/x86_emulate.c b/arch/x86/kvm/x86_emulate.c
index a391e21..da3a990 100644
--- a/arch/x86/kvm/x86_emulate.c
+++ b/arch/x86/kvm/x86_emulate.c
@@ -1768,7 +1768,7 @@ special_insn:
 		io_dir_in = 0;
 	do_io:	if (kvm_emulate_pio(ctxt->vcpu, NULL, io_dir_in,
 				   (c->d & ByteOp) ? 1 : c->op_bytes,
-				   port) != 0) {
+				   port, c->eip) != 0) {
 			c->eip = saved_eip;
 			goto cannot_emulate;
 		}
diff --git a/include/asm-x86/kvm_host.h b/include/asm-x86/kvm_host.h
index c4fbd70..042b335 100644
--- a/include/asm-x86/kvm_host.h
+++ b/include/asm-x86/kvm_host.h
@@ -542,7 +542,7 @@ int kvm_set_msr(struct kvm_vcpu *vcpu, u32 msr_index, u64 data);
 struct x86_emulate_ctxt;
 
 int kvm_emulate_pio(struct kvm_vcpu *vcpu, struct kvm_run *run, int in,
-		     int size, unsigned port);
+		     int size, unsigned port, unsigned long rip);
 int kvm_emulate_pio_string(struct kvm_vcpu *vcpu, struct kvm_run *run, int in,
 			   int size, unsigned long count, int down,
 			    gva_t address, int rep, unsigned port);
diff --git a/include/linux/kvm.h b/include/linux/kvm.h
index 44fd7fa..1a00719 100644
--- a/include/linux/kvm.h
+++ b/include/linux/kvm.h
@@ -127,6 +127,7 @@ struct kvm_run {
 			__u16 port;
 			__u32 count;
 			__u64 data_offset; /* relative to kvm_run start */
+			__u64 rip;
 		} io;
 		struct {
 		} debug;

Re: [RFC] Getting correct eip in skip_emulated_instruction()

[Avi Kivity]

Guillaume Thouvenin wrote:
> Hello,
>
>  Presently when we want to skip an emulated instruction we update the
> eip by reading the instruction length from VMCS structure and we add
> this value to the current eip. It gives us the new eip. A problem occurs
> when the guest state doesn't allow us to use VT because in that case,
> the values in VMCS structure are not up to date. That means that if we
> call skip_emulated_instruction() while guest state is invalid, the
> computed eip will be false.
>
>  To fix the problem I introduced a new field in the structure kvm_io to
> store the eip value if guest state is invalid. If the stored value is
> equal to 0 we read the length of the current instruction in VMCS and if
> not, we use it as the new eip. It works but I'm not really happy with
> that hack and I don't see how to solve the problem nicely.
>   

If we call the emulator we shouldn't call skip_emulated_instruction() in 
the first place, since the emulator already computes the next rip for 
us. I suggest moving ->skip_emulated_instruction() out of 
kvm_emulate_pio() and into handle_io() (and the svm equivalent).

-- 
error compiling committee.c: too many arguments to function