VMX: Enhance invalid guest state emulation

- Change returned handle_invalid_guest_state() to return relevant exit codes
- Move triggering the emulation from vmx_vcpu_run() to vmx_handle_exit()
- Return to userspace instead of repeatedly trying to emulate instructions that have already failed

Signed-off-by Mohammed Gamal <[email protected]>

---
 arch/x86/kvm/vmx.c |   44 ++++++++++++++++++++------------------------
 1 files changed, 20 insertions(+), 24 deletions(-)

arch/x86/kvm/vmx.c +20 -24

index 78101dd..6265098 100644
@@ -107,7 +107,6 @@ struct vcpu_vmx {
 	} rmode;
 	int vpid;
 	bool emulation_required;
-	enum emulation_result invalid_state_emulation_result;
 
 	/* Support for vnmi-less CPUs */
 	int soft_vnmi_blocked;

@@ -3318,35 +3317,37 @@ static int handle_nmi_window(struct kvm_vcpu *vcpu)
 	return 1;
 }
 
-static void handle_invalid_guest_state(struct kvm_vcpu *vcpu)
+static int handle_invalid_guest_state(struct kvm_vcpu *vcpu)
 {
 	struct vcpu_vmx *vmx = to_vmx(vcpu);
 	enum emulation_result err = EMULATE_DONE;
-
-	local_irq_enable();
-	preempt_enable();
+	int ret = 1;
 
 	while (!guest_state_valid(vcpu)) {
 		err = emulate_instruction(vcpu, 0, 0, 0);
 
-		if (err == EMULATE_DO_MMIO)
-			break;
+		if (err == EMULATE_DO_MMIO) {
+			ret = 0;
+			goto out;
+		}
 
 		if (err != EMULATE_DONE) {
 			kvm_report_emulation_failure(vcpu, "emulation failure");
-			break;
+			vcpu->run->exit_reason = KVM_EXIT_INTERNAL_ERROR;
+			vcpu->run->internal.suberror = KVM_INTERNAL_ERROR_EMULATION;
+			ret = 0;
+			goto out;
 		}
 
 		if (signal_pending(current))
-			break;
+			goto out;
 		if (need_resched())
 			schedule();
 	}
 
-	preempt_disable();
-	local_irq_disable();
-
-	vmx->invalid_state_emulation_result = err;
+	vmx->emulation_required = 0;
+out:
+	return ret;
 }
 
 /*

@@ -3402,13 +3403,9 @@ static int vmx_handle_exit(struct kvm_vcpu *vcpu)
 
 	trace_kvm_exit(exit_reason, kvm_rip_read(vcpu));
 
-	/* If we need to emulate an MMIO from handle_invalid_guest_state
-	 * we just return 0 */
-	if (vmx->emulation_required && emulate_invalid_guest_state) {
-		if (guest_state_valid(vcpu))
-			vmx->emulation_required = 0;
-		return vmx->invalid_state_emulation_result != EMULATE_DO_MMIO;
-	}
+	/* If guest state is invalid, start emulating */
+	if (vmx->emulation_required && emulate_invalid_guest_state)
+		return handle_invalid_guest_state(vcpu);
 
 	/* Access CR3 don't cause VMExit in paging mode, so we need
 	 * to sync with guest real CR3. */

@@ -3603,11 +3600,10 @@ static void vmx_vcpu_run(struct kvm_vcpu *vcpu)
 	if (unlikely(!cpu_has_virtual_nmis() && vmx->soft_vnmi_blocked))
 		vmx->entry_time = ktime_get();
 
-	/* Handle invalid guest state instead of entering VMX */
-	if (vmx->emulation_required && emulate_invalid_guest_state) {
-		handle_invalid_guest_state(vcpu);
+	/* Don't enter VMX if guest state is invalid, let the exit handler
+	   start emulation until we arrive back to a valid state */
+	if (vmx->emulation_required && emulate_invalid_guest_state)
 		return;
-	}
 
 	if (test_bit(VCPU_REGS_RSP, (unsigned long *)&vcpu->arch.regs_dirty))
 		vmcs_writel(GUEST_RSP, vcpu->arch.regs[VCPU_REGS_RSP]);

Signature · 1 line

1.6.0.4

Quoted 5 lines · Mohammed Gamal

- Change returned handle_invalid_guest_state() to return relevant exit codes
- Move triggering the emulation from vmx_vcpu_run() to vmx_handle_exit()
- Return to userspace instead of repeatedly trying to emulate instructions that have already failed

Signed-off-by Mohammed Gamal <[email protected]>

Mohammed,

The handle_invalid_guest_state loop is potentially problematic. It would be more appropriate to use the __vcpu_run loop.

Can't you set vmx->emulation_required depending on the result of one call to emulate_instruction and get rid of the while (!guest_state_valid(vcpu)) loop?

Quoted 7 lines · Marcelo Tosatti

Quoted 5 lines · Mohammed Gamal

- Change returned handle_invalid_guest_state() to return relevant exit codes
- Move triggering the emulation from vmx_vcpu_run() to vmx_handle_exit()
- Return to userspace instead of repeatedly trying to emulate instructions that have already failed

Signed-off-by Mohammed Gamal <[email protected]>

Quoted 9 lines

Mohammed,

The handle_invalid_guest_state loop is potentially problematic. It would be more appropriate to use the __vcpu_run loop.

Can't you set vmx->emulation_required depending on the result of one call to emulate_instruction and get rid of the while (!guest_state_valid(vcpu)) loop?

Invalid state emulation is VMX-specfic, while the __vcpu_run loop is independent of the virtualization extension (defined in x86.c), no? AMD SVM can comforably run hosts in big-real mode and thus it doesn't have the notion of a guest going to an invalid state because of mode switching, so I don't think it'd be a good idea to move emulation into a generic layer. Please correct me if I am wrong

Quoted 18 lines · Mohammed Gamal

Quoted 7 lines · Marcelo Tosatti

Quoted 5 lines · Mohammed Gamal

- Change returned handle_invalid_guest_state() to return relevant exit codes
- Move triggering the emulation from vmx_vcpu_run() to vmx_handle_exit()
- Return to userspace instead of repeatedly trying to emulate instructions that have already failed

Signed-off-by Mohammed Gamal <[email protected]>

Quoted 9 lines

Mohammed,

The handle_invalid_guest_state loop is potentially problematic. It would be more appropriate to use the __vcpu_run loop.

Can't you set vmx->emulation_required depending on the result of one call to emulate_instruction and get rid of the while (!guest_state_valid(vcpu)) loop?

Quoted 6 lines

Invalid state emulation is VMX-specfic, while the __vcpu_run loop is independent of the virtualization extension (defined in x86.c), no? AMD SVM can comforably run hosts in big-real mode and thus it doesn't have the notion of a guest going to an invalid state because of mode switching, so I don't think it'd be a good idea to move emulation into a generic layer. Please correct me if I am wrong

Right. But all i am asking is to emulate one instruction at a time in handle_invalid_guest_state, instead of looping until guest_state_valid(vcpu).

So you get rid of schedule(), the check for signal_pending, etc.

Quoted 26 lines · Marcelo Tosatti

Quoted 24 lines · Mohammed Gamal

Quoted 7 lines · Marcelo Tosatti

Quoted 5 lines · Mohammed Gamal

- Change returned handle_invalid_guest_state() to return relevant exit codes
- Move triggering the emulation from vmx_vcpu_run() to vmx_handle_exit()
- Return to userspace instead of repeatedly trying to emulate instructions that have already failed

Signed-off-by Mohammed Gamal <[email protected]>

Quoted 9 lines

Mohammed,

The handle_invalid_guest_state loop is potentially problematic. It would be more appropriate to use the __vcpu_run loop.

Can't you set vmx->emulation_required depending on the result of one call to emulate_instruction and get rid of the while (!guest_state_valid(vcpu)) loop?

Invalid state emulation is VMX-specfic, while the __vcpu_run loop is independent of the virtualization extension (defined in x86.c), no? AMD SVM can comforably run hosts in big-real mode and thus it doesn't have the notion of a guest going to an invalid state because of mode switching, so I don't think it'd be a good idea to move emulation into a generic layer. Please correct me if I am wrong

Quoted 5 lines

Right. But all i am asking is to emulate one instruction at a time in handle_invalid_guest_state, instead of looping until guest_state_valid(vcpu).

So you get rid of schedule(), the check for signal_pending, etc.

But we'll still need to enter the guest when it's in a valid state, so we need to move that loop somewhere,

and now that we still have a loop
we'll also still need to do the pending signals and scheduling checks, no?

I'd appreciate any suggestions you have to alleviate this.

Quoted 27 lines · Mohammed Gamal

Quoted 26 lines · Marcelo Tosatti

Quoted 24 lines · Mohammed Gamal

Quoted 7 lines · Marcelo Tosatti

Quoted 5 lines · Mohammed Gamal

- Change returned handle_invalid_guest_state() to return relevant exit codes
- Move triggering the emulation from vmx_vcpu_run() to vmx_handle_exit()
- Return to userspace instead of repeatedly trying to emulate instructions that have already failed

Signed-off-by Mohammed Gamal <[email protected]>

Quoted 9 lines

Mohammed,

The handle_invalid_guest_state loop is potentially problematic. It would be more appropriate to use the __vcpu_run loop.

Can't you set vmx->emulation_required depending on the result of one call to emulate_instruction and get rid of the while (!guest_state_valid(vcpu)) loop?

Invalid state emulation is VMX-specfic, while the __vcpu_run loop is independent of the virtualization extension (defined in x86.c), no? AMD SVM can comforably run hosts in big-real mode and thus it doesn't have the notion of a guest going to an invalid state because of mode switching, so I don't think it'd be a good idea to move emulation into a generic layer. Please correct me if I am wrong

Quoted 8 lines

Quoted 5 lines

Right. But all i am asking is to emulate one instruction at a time in handle_invalid_guest_state, instead of looping until guest_state_valid(vcpu).

So you get rid of schedule(), the check for signal_pending, etc.

But we'll still need to enter the guest when it's in a valid state, so we need to move that loop somewhere,

Sure, just set vmx->emulation_required = guest_state_valid(vcpu). When the state is good, the entry handler will vmentry.

Quoted 3 lines

and now that we still have a loop
we'll also still need to do the pending signals and scheduling checks, no?

Point is you can use the __vcpu_run loop.

In the latest patch you do:

+       /* Don't enter VMX if guest state is invalid, let the exit handler
+          start emulation until we arrive back to a valid state */
+       if (vmx->emulation_required && emulate_invalid_guest_state) return;

And then emulate in the exit handler.

Quoted 1 line

I'd appreciate any suggestions you have to alleviate this.

I fail to see why you need an internal loop if you can use the external (__vcpu_run) one.

Quoted 5 lines

+       if (vmx->emulation_required && emulate_invalid_guest_state) return;

And then emulate in the exit handler.

Quoted 4 lines

Quoted 1 line

I'd appreciate any suggestions you have to alleviate this.

I fail to see why you need an internal loop if you can use the external (__vcpu_run) one.

Because it's not just used by VMX. So I don't think it'd be wise to use it for something that's VMX-specific.

Quoted 5 lines

Quoted 2 lines

I'd appreciate any suggestions you have to alleviate this.

I fail to see why you need an internal loop if you can use the external (__vcpu_run) one.

Because it's not just used by VMX. So I don't think it'd be wise to use it for something that's VMX-specific.