[Qemu-devel] [PATCH] x86: svm: Always clear event_inj on vmexit

[Qemu-devel] [PATCH] x86: svm: Always clear event_inj on vmexit

[Jan Kiszka]

We currently only clear SVM_EVTINJ_VALID after successful interrupt
delivery. This apparently does not match real hardware which clears the
whole event_inj field on every vmexit, including unsuccessful interrupt
delivery.

Reported-by: Erik van der Kouwe <vdkouwe@cs.vu.nl>
Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
---

(before it gets lost)
Erik, please confirm that this works for you.

 target-i386/op_helper.c |    8 +-------
 1 files changed, 1 insertions(+), 7 deletions(-)

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). */
-- 
1.6.0.2

[Qemu-devel] Re: [PATCH] x86: svm: Always clear event_inj on vmexit

[Erik van der Kouwe]

Hi,

 > We currently only clear SVM_EVTINJ_VALID after successful interrupt
 > delivery. This apparently does not match real hardware which clears the
 > whole event_inj field on every vmexit, including unsuccessful interrupt
 > delivery.

Thanks for the patch. It is a bit hard for me to test right now as I 
messed up my test setup, but I will do so ASAP and let you know.

However, I'm worried that this patch may introduce a new problem (I may 
be mistaken though). There is still this code to load the exit interrupt 
info:

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)));

Now that event_inj is no longer loaded, won't this mean that 
exit_int_info and exit_int_info_err also won't be loaded?

With kind regards,
Erik

Jan Kiszka wrote:
> We currently only clear SVM_EVTINJ_VALID after successful interrupt
> delivery. This apparently does not match real hardware which clears the
> whole event_inj field on every vmexit, including unsuccessful interrupt
> delivery.
> 
> Reported-by: Erik van der Kouwe <vdkouwe@cs.vu.nl>
> Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
> ---
> 
> (before it gets lost)
> Erik, please confirm that this works for you.
> 
>  target-i386/op_helper.c |    8 +-------
>  1 files changed, 1 insertions(+), 7 deletions(-)
> 
> 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). */

[Qemu-devel] [PATCH v2] x86: svm: Always clear event_inj on vmexit

[Jan Kiszka]

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

Erik van der Kouwe wrote:
> Hi,
> 
>> We currently only clear SVM_EVTINJ_VALID after successful interrupt
>> delivery. This apparently does not match real hardware which clears the
>> whole event_inj field on every vmexit, including unsuccessful interrupt
>> delivery.
> 
> Thanks for the patch. It is a bit hard for me to test right now as I
> messed up my test setup, but I will do so ASAP and let you know.
> 
> However, I'm worried that this patch may introduce a new problem (I may
> be mistaken though). There is still this code to load the exit interrupt
> info:
> 
> 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)));
> 
> Now that event_inj is no longer loaded, won't this mean that
> exit_int_info and exit_int_info_err also won't be loaded?

Sorry, can't follow this ATM. But maybe you mean this: there is indeed a
problem with removing the clearance of event_inj.invalid as it may be
later on transferred into exit_int_info. And if we succeed with
injecting the event, that field must not remaind valid.

OK, here is v2:

------->

From: Jan Kiszka <jan.kiszka@siemens.com>

We currently only clear SVM_EVTINJ_VALID after successful interrupt
delivery. This apparently does not match real hardware which clears the
whole event_inj field on every vmexit, including unsuccessful interrupt
delivery.

Reported-by: Erik van der Kouwe <vdkouwe@cs.vu.nl>
Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
---
 target-i386/op_helper.c |    1 +
 1 files changed, 1 insertions(+), 0 deletions(-)

diff --git a/target-i386/op_helper.c b/target-i386/op_helper.c
index dcbdfe7..52e8910 100644
--- a/target-i386/op_helper.c
+++ b/target-i386/op_helper.c
@@ -5388,6 +5388,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). */
-- 
1.6.0.2


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

Re: [Qemu-devel] [PATCH v2] x86: svm: Always clear event_inj on vmexit

[Erik van der Kouwe]

Hi,

> Sorry, can't follow this ATM. But maybe you mean this: there is indeed a
> problem with removing the clearance of event_inj.invalid as it may be
> later on transferred into exit_int_info. And if we succeed with
> injecting the event, that field must not remaind valid.

Correct.

> OK, here is v2:

I tested this and AFAICS this one works fine and fixes the problem.

Thanks,
Erik

Re: [Qemu-devel] [PATCH v2] x86: svm: Always clear event_inj on vmexit

[Aurelien Jarno]

On Wed, Jun 02, 2010 at 08:49:14AM +0200, Jan Kiszka wrote:
> Erik van der Kouwe wrote:
> > Hi,
> > 
> >> We currently only clear SVM_EVTINJ_VALID after successful interrupt
> >> delivery. This apparently does not match real hardware which clears the
> >> whole event_inj field on every vmexit, including unsuccessful interrupt
> >> delivery.
> > 
> > Thanks for the patch. It is a bit hard for me to test right now as I
> > messed up my test setup, but I will do so ASAP and let you know.
> > 
> > However, I'm worried that this patch may introduce a new problem (I may
> > be mistaken though). There is still this code to load the exit interrupt
> > info:
> > 
> > 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)));
> > 
> > Now that event_inj is no longer loaded, won't this mean that
> > exit_int_info and exit_int_info_err also won't be loaded?
> 
> Sorry, can't follow this ATM. But maybe you mean this: there is indeed a
> problem with removing the clearance of event_inj.invalid as it may be
> later on transferred into exit_int_info. And if we succeed with
> injecting the event, that field must not remaind valid.
> 
> OK, here is v2:
> 
> ------->
> 
> From: Jan Kiszka <jan.kiszka@siemens.com>
> 
> We currently only clear SVM_EVTINJ_VALID after successful interrupt
> delivery. This apparently does not match real hardware which clears the
> whole event_inj field on every vmexit, including unsuccessful interrupt
> delivery.
> 
> Reported-by: Erik van der Kouwe <vdkouwe@cs.vu.nl>
> Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
> ---
>  target-i386/op_helper.c |    1 +
>  1 files changed, 1 insertions(+), 0 deletions(-)

Thanks, applied.

> diff --git a/target-i386/op_helper.c b/target-i386/op_helper.c
> index dcbdfe7..52e8910 100644
> --- a/target-i386/op_helper.c
> +++ b/target-i386/op_helper.c
> @@ -5388,6 +5388,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). */
> -- 
> 1.6.0.2
> 



-- 
Aurelien Jarno                          GPG: 1024D/F1BCDB73
aurelien@aurel32.net                 http://www.aurel32.net