[Qemu-devel] Re: SVM emulation: EVENTINJ marked valid when a pagefault happens while issuing a software interrupt

[Jan Kiszka <jan.kiszka@web.de>]

[-- Attachment #1: Type: text/plain, Size: 3671 bytes --]

Roedel, Joerg wrote:
> On Thu, May 27, 2010 at 06:20:00PM -0400, Jan Kiszka wrote:
>> Erik van der Kouwe wrote:
>>> Problem is: I'm compiling in Linux and testing in MINIX. Testing on the
>>> real hardware would require a reboot everytime. Moreover, it might screw
>>> up my system if I make bad mistakes (the MINIX filesystem is easily
>>> corrupted).
>> Use Linux+KVM as host OS, it can also run VMMs as guests (aka nested
>> SVM). And you could even debug those guests just like when you would run
>> QEMU in emulation mode. In contrast to SVM emulation, nesting is fairly
>> stable AFAIK. And it is faster.
> 
> At least it is more stable than any other nested-svm implementation I
> know of ;-)
> There are issues with kvmclock when you run kvm-on-kvm and you should
> not expect windows-based hypervisors to run without problems. Beside
> that, for running kvm-on-kvm and xen-on-kvm it is indeed fairly
> stable :-)
> 
>>> Linux source tree (2.6.31-ubuntu), arch/x86/kvm/svm.c, end of function
>>> nested_svm_vmrun. Here event_inj and event_inj_err are copied from a
>>> different VMCB, effectively clearing the value set by the CPU. Maybe
>>> this isn't were I should have been looking though?
> 
> The interesting part is in nested_svm_vmexit. There you have this piece
> of code:
> 
>         /*
>          * If we emulate a VMRUN/#VMEXIT in the same host #vmexit cycle we have
>          * to make sure that we do not lose injected events. So check event_inj
>          * here and copy it to exit_int_info if it is valid.
>          * Exit_int_info and event_inj can't be both valid because the case
>          * below only happens on a VMRUN instruction intercept which has
>          * no valid exit_int_info set.
>          */
>         if (vmcb->control.event_inj & SVM_EVTINJ_VALID) {
>                 struct vmcb_control_area *nc = &nested_vmcb->control;
> 
>                 nc->exit_int_info     = vmcb->control.event_inj;
>                 nc->exit_int_info_err = vmcb->control.event_inj_err;
>         }
> 
> and a few lines later:
> 
>         nested_vmcb->control.event_inj         = 0;
>         nested_vmcb->control.event_inj_err     = 0;
> 
> ... which takes care of this situation. The vmcb.eventinf field is _defined_ to
> be zero on a #vmexit.

(Hmm, must have missed that line in the spec.)

In that case something like

diff --git a/target-i386/op_helper.c b/target-i386/op_helper.c
index dcbdfe7..caabdb4 100644
--- a/target-i386/op_helper.c
+++ b/target-i386/op_helper.c
@@ -1263,13 +1263,6 @@ void do_interrupt(int intno, int is_int, int error_code,
 #endif
         do_interrupt_real(intno, is_int, error_code, next_eip);
     }
-
-#if !defined(CONFIG_USER_ONLY)
-    if (env->hflags & HF_SVMI_MASK) {
-	    uint32_t event_inj = ldl_phys(env->vm_vmcb + offsetof(struct vmcb, control.event_inj));
-	    stl_phys(env->vm_vmcb + offsetof(struct vmcb, control.event_inj), event_inj & ~SVM_EVTINJ_VALID);
-    }
-#endif
 }
 
 /* This should come from sysemu.h - if we could include it here... */
@@ -5388,6 +5381,7 @@ void helper_vmexit(uint32_t exit_code, uint64_t exit_info_1)
              ldl_phys(env->vm_vmcb + offsetof(struct vmcb, control.event_inj)));
     stl_phys(env->vm_vmcb + offsetof(struct vmcb, control.exit_int_info_err),
              ldl_phys(env->vm_vmcb + offsetof(struct vmcb, control.event_inj_err)));
+    stl_phys(env->vm_vmcb + offsetof(struct vmcb, control.event_inj), 0);
 
     env->hflags2 &= ~HF2_GIF_MASK;
     /* FIXME: Resets the current ASID register to zero (host ASID). */

should resolve the QEMU issue, right?

Jan


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 257 bytes --]