[v2,2/4] x86/vvmx: fix VM_EXIT_ACK_INTR_ON_EXIT handling
diff mbox series

Message ID 20200203121919.15748-3-roger.pau@citrix.com
State New, archived
Headers show
Series
  • x86/vvmx: fixes to interrupt injection
Related show

Commit Message

Roger Pau Monné Feb. 3, 2020, 12:19 p.m. UTC
When VM_EXIT_ACK_INTR_ON_EXIT is set in the vmexit control vmcs
register the bit 31 of VM_EXIT_INTR_INFO must be 0, in order to denote
that the field doesn't contain any interrupt information. This is not
currently acknowledged as the field always get filled with valid
interrupt information, regardless of whether VM_EXIT_ACK_INTR_ON_EXIT
is set.

Fix this and only fill VM_EXIT_INTR_INFO when VM_EXIT_ACK_INTR_ON_EXIT
is not set. Note that this requires one minor change in
nvmx_update_apicv in order to obtain the interrupt information from
the internal state rather than the nested vmcs register.

Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
---
Changes since v1:
 - New in this version.
---
 xen/arch/x86/hvm/vmx/vvmx.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

Comments

Tian, Kevin Feb. 4, 2020, 1:39 a.m. UTC | #1
> From: Roger Pau Monne <roger.pau@citrix.com>
> Sent: Monday, February 3, 2020 8:19 PM
> 
> When VM_EXIT_ACK_INTR_ON_EXIT is set in the vmexit control vmcs

set->cleared

> register the bit 31 of VM_EXIT_INTR_INFO must be 0, in order to denote
> that the field doesn't contain any interrupt information. This is not
> currently acknowledged as the field always get filled with valid
> interrupt information, regardless of whether VM_EXIT_ACK_INTR_ON_EXIT
> is set.
> 
> Fix this and only fill VM_EXIT_INTR_INFO when VM_EXIT_ACK_INTR_ON_EXIT
> is not set. Note that this requires one minor change in

not set -> set

> nvmx_update_apicv in order to obtain the interrupt information from
> the internal state rather than the nested vmcs register.
> 
> Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>

with above fixed, 

Reviewed-by: Kevin Tian <kevin.tian@intel.com>

> ---
> Changes since v1:
>  - New in this version.
> ---
>  xen/arch/x86/hvm/vmx/vvmx.c | 6 ++++--
>  1 file changed, 4 insertions(+), 2 deletions(-)
> 
> diff --git a/xen/arch/x86/hvm/vmx/vvmx.c b/xen/arch/x86/hvm/vmx/vvmx.c
> index 3d97a293b2..47eee1e5b9 100644
> --- a/xen/arch/x86/hvm/vmx/vvmx.c
> +++ b/xen/arch/x86/hvm/vmx/vvmx.c
> @@ -1283,6 +1283,7 @@ static void load_vvmcs_host_state(struct vcpu *v)
>  static void sync_exception_state(struct vcpu *v)
>  {
>      struct nestedvmx *nvmx = &vcpu_2_nvmx(v);
> +    uint32_t exit_ctrl = get_vvmcs(v, VM_EXIT_CONTROLS);
> 
>      if ( !(nvmx->intr.intr_info & INTR_INFO_VALID_MASK) )
>          return;
> @@ -1294,7 +1295,8 @@ static void sync_exception_state(struct vcpu *v)
>          set_vvmcs(v, VM_EXIT_REASON, EXIT_REASON_EXTERNAL_INTERRUPT);
>          set_vvmcs(v, EXIT_QUALIFICATION, 0);
>          set_vvmcs(v, VM_EXIT_INTR_INFO,
> -                    nvmx->intr.intr_info);
> +                  (exit_ctrl & VM_EXIT_ACK_INTR_ON_EXIT) ? nvmx->intr.intr_info
> +                                                         : 0);
>          break;
> 
>      case X86_EVENTTYPE_HW_EXCEPTION:
> @@ -1320,7 +1322,7 @@ static void nvmx_update_apicv(struct vcpu *v)
>  {
>      struct nestedvmx *nvmx = &vcpu_2_nvmx(v);
>      unsigned long reason = get_vvmcs(v, VM_EXIT_REASON);
> -    uint32_t intr_info = get_vvmcs(v, VM_EXIT_INTR_INFO);
> +    uint32_t intr_info = nvmx->intr.intr_info;
> 
>      if ( reason == EXIT_REASON_EXTERNAL_INTERRUPT &&
>           nvmx->intr.source == hvm_intsrc_lapic &&
> --
> 2.25.0
Roger Pau Monné Feb. 4, 2020, 9:26 a.m. UTC | #2
On Tue, Feb 04, 2020 at 01:39:44AM +0000, Tian, Kevin wrote:
> > From: Roger Pau Monne <roger.pau@citrix.com>
> > Sent: Monday, February 3, 2020 8:19 PM
> > 
> > When VM_EXIT_ACK_INTR_ON_EXIT is set in the vmexit control vmcs
> 
> set->cleared
> 
> > register the bit 31 of VM_EXIT_INTR_INFO must be 0, in order to denote
> > that the field doesn't contain any interrupt information. This is not
> > currently acknowledged as the field always get filled with valid
> > interrupt information, regardless of whether VM_EXIT_ACK_INTR_ON_EXIT
> > is set.
> > 
> > Fix this and only fill VM_EXIT_INTR_INFO when VM_EXIT_ACK_INTR_ON_EXIT
> > is not set. Note that this requires one minor change in
> 
> not set -> set
> 
> > nvmx_update_apicv in order to obtain the interrupt information from
> > the internal state rather than the nested vmcs register.
> > 
> > Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
> 
> with above fixed, 

Ouch yes, I've inverted the conditions. Thanks for noticing.

> 
> Reviewed-by: Kevin Tian <kevin.tian@intel.com>

To whoever picks this patch for committing: let me know if you would
like me to resend with the commit log fixed.

Roger.

Patch
diff mbox series

diff --git a/xen/arch/x86/hvm/vmx/vvmx.c b/xen/arch/x86/hvm/vmx/vvmx.c
index 3d97a293b2..47eee1e5b9 100644
--- a/xen/arch/x86/hvm/vmx/vvmx.c
+++ b/xen/arch/x86/hvm/vmx/vvmx.c
@@ -1283,6 +1283,7 @@  static void load_vvmcs_host_state(struct vcpu *v)
 static void sync_exception_state(struct vcpu *v)
 {
     struct nestedvmx *nvmx = &vcpu_2_nvmx(v);
+    uint32_t exit_ctrl = get_vvmcs(v, VM_EXIT_CONTROLS);
 
     if ( !(nvmx->intr.intr_info & INTR_INFO_VALID_MASK) )
         return;
@@ -1294,7 +1295,8 @@  static void sync_exception_state(struct vcpu *v)
         set_vvmcs(v, VM_EXIT_REASON, EXIT_REASON_EXTERNAL_INTERRUPT);
         set_vvmcs(v, EXIT_QUALIFICATION, 0);
         set_vvmcs(v, VM_EXIT_INTR_INFO,
-                    nvmx->intr.intr_info);
+                  (exit_ctrl & VM_EXIT_ACK_INTR_ON_EXIT) ? nvmx->intr.intr_info
+                                                         : 0);
         break;
 
     case X86_EVENTTYPE_HW_EXCEPTION:
@@ -1320,7 +1322,7 @@  static void nvmx_update_apicv(struct vcpu *v)
 {
     struct nestedvmx *nvmx = &vcpu_2_nvmx(v);
     unsigned long reason = get_vvmcs(v, VM_EXIT_REASON);
-    uint32_t intr_info = get_vvmcs(v, VM_EXIT_INTR_INFO);
+    uint32_t intr_info = nvmx->intr.intr_info;
 
     if ( reason == EXIT_REASON_EXTERNAL_INTERRUPT &&
          nvmx->intr.source == hvm_intsrc_lapic &&