[Xen-devel] [PATCH v2 0/4] x86/nvmx: fixes for interrupt injection

[Xen-devel] [PATCH v2 0/4] x86/nvmx: fixes for interrupt injection

[Roger Pau Monne]

Hello,

osstest identified a regression caused by my earlier attempt to fix
interrupt injection when using nested VMX. This series aims to fix the
regression, and should unblock several osstest branches.

The following report is from osstest with this series applied:

http://logs.test-lab.xenproject.org/osstest/logs/148979/

Note the last patch (4/4) is the one that actually fixes the issue. Xen
will always use the Ack on exit feature so patches 2/4 and 3/4 don't
change the functionality when running a nested Xen, as it always
requires SVI to be updated.

Thanks, Roger.

Roger Pau Monne (4):
  Revert "x86/vvmx: fix virtual interrupt injection when Ack on exit
    control is used"
  x86/nvmx: only update SVI when using Ack on exit
  x86/nvmx: split updating RVI from SVI in nvmx_update_apicv
  x86/nvmx: update exit bitmap when using virtual interrupt delivery

 xen/arch/x86/hvm/vmx/intr.c       | 21 ++++++++++++--------
 xen/arch/x86/hvm/vmx/vvmx.c       | 33 ++++++++++++++++++++-----------
 xen/include/asm-x86/hvm/vmx/vmx.h |  2 ++
 3 files changed, 36 insertions(+), 20 deletions(-)

-- 
2.26.0

[Xen-devel] [PATCH v2 1/4] Revert "x86/vvmx: fix virtual interrupt injection when Ack on exit control is used"

[Roger Pau Monne]

This reverts commit f96e1469ad06b61796c60193daaeb9f8a96d7458.

The commit is wrong, as the whole point of nvmx_update_apicv is to
update the guest interrupt status field when the Ack on exit VMEXIT
control feature is enabled.

Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
Reviewed-by: Kevin Tian <kevin.tian@intel.com>
---
 xen/arch/x86/hvm/vmx/vvmx.c | 7 +------
 1 file changed, 1 insertion(+), 6 deletions(-)

diff --git a/xen/arch/x86/hvm/vmx/vvmx.c b/xen/arch/x86/hvm/vmx/vvmx.c
index f049920196..1b8461ba30 100644
--- a/xen/arch/x86/hvm/vmx/vvmx.c
+++ b/xen/arch/x86/hvm/vmx/vvmx.c
@@ -1456,12 +1456,7 @@ static void virtual_vmexit(struct cpu_user_regs *regs)
     /* updating host cr0 to sync TS bit */
     __vmwrite(HOST_CR0, v->arch.hvm.vmx.host_cr0);
 
-    if ( cpu_has_vmx_virtual_intr_delivery &&
-         /*
-          * Only inject the vector if the Ack on exit bit is not set, else the
-          * interrupt will be signaled in the vmcs VM_EXIT_INTR_INFO field.
-          */
-         !(get_vvmcs(v, VM_EXIT_CONTROLS) & VM_EXIT_ACK_INTR_ON_EXIT) )
+    if ( cpu_has_vmx_virtual_intr_delivery )
         nvmx_update_apicv(v);
 
     nvcpu->nv_vmswitch_in_progress = 0;
-- 
2.26.0

[Xen-devel] [PATCH v2 2/4] x86/nvmx: only update SVI when using Ack on exit

[Roger Pau Monne]

Check whether there's a valid interrupt in VM_EXIT_INTR_INFO in order
to decide whether to update SVI in nvmx_update_apicv. If Ack on exit
is not being used VM_EXIT_INTR_INFO won't have a valid interrupt and
hence SVI shouldn't be updated to signal the interrupt is currently in
service because it won't be Acked.

Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
---
 xen/arch/x86/hvm/vmx/vvmx.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/xen/arch/x86/hvm/vmx/vvmx.c b/xen/arch/x86/hvm/vmx/vvmx.c
index 1b8461ba30..1753005c91 100644
--- a/xen/arch/x86/hvm/vmx/vvmx.c
+++ b/xen/arch/x86/hvm/vmx/vvmx.c
@@ -1383,7 +1383,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 = nvmx->intr.intr_info;
+    unsigned long intr_info = get_vvmcs(v, VM_EXIT_INTR_INFO);
 
     if ( reason == EXIT_REASON_EXTERNAL_INTERRUPT &&
          nvmx->intr.source == hvm_intsrc_lapic &&
-- 
2.26.0

[Xen-devel] [PATCH v2 3/4] x86/nvmx: split updating RVI from SVI in nvmx_update_apicv

[Roger Pau Monne]

Updating SVI is required when an interrupt has been injected using the
Ack on exit VMEXIT feature, so that the in service interrupt in the
GUEST_INTR_STATUS matches the vector that is signaled in
VM_EXIT_INTR_INFO.

Updating RVI however is not tied to the Ack on exit feature, as it
signals the next vector to be injected, and hence should always be
updated to the next pending vector, regardless of whether Ack on exit
is enabled.

Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
---
 xen/arch/x86/hvm/vmx/vvmx.c | 22 +++++++++++++++++-----
 1 file changed, 17 insertions(+), 5 deletions(-)

diff --git a/xen/arch/x86/hvm/vmx/vvmx.c b/xen/arch/x86/hvm/vmx/vvmx.c
index 1753005c91..8431c912a1 100644
--- a/xen/arch/x86/hvm/vmx/vvmx.c
+++ b/xen/arch/x86/hvm/vmx/vvmx.c
@@ -1384,26 +1384,38 @@ static void nvmx_update_apicv(struct vcpu *v)
     struct nestedvmx *nvmx = &vcpu_2_nvmx(v);
     unsigned long reason = get_vvmcs(v, VM_EXIT_REASON);
     unsigned long intr_info = get_vvmcs(v, VM_EXIT_INTR_INFO);
+    int rvi;
 
     if ( reason == EXIT_REASON_EXTERNAL_INTERRUPT &&
          nvmx->intr.source == hvm_intsrc_lapic &&
          (intr_info & INTR_INFO_VALID_MASK) )
     {
         uint16_t status;
-        uint32_t rvi, ppr;
-        uint32_t vector = intr_info & 0xff;
+        uint32_t ppr;
+        unsigned int vector = intr_info & INTR_INFO_VECTOR_MASK;
         struct vlapic *vlapic = vcpu_vlapic(v);
 
+        /*
+         * Update SVI to record the currently in service interrupt that's
+         * signaled in EXIT_INTR_INFO.
+         */
         vlapic_ack_pending_irq(v, vector, 1);
 
         ppr = vlapic_set_ppr(vlapic);
         WARN_ON((ppr & 0xf0) != (vector & 0xf0));
 
         status = vector << VMX_GUEST_INTR_STATUS_SVI_OFFSET;
-        rvi = vlapic_has_pending_irq(v);
-        if ( rvi != -1 )
-            status |= rvi & VMX_GUEST_INTR_STATUS_SUBFIELD_BITMASK;
+        __vmwrite(GUEST_INTR_STATUS, status);
+    }
+
+    rvi = vlapic_has_pending_irq(v);
+    if ( rvi != -1 )
+    {
+        unsigned long status;
 
+        __vmread(GUEST_INTR_STATUS, &status);
+        status &= ~VMX_GUEST_INTR_STATUS_SUBFIELD_BITMASK;
+        status |= rvi & VMX_GUEST_INTR_STATUS_SUBFIELD_BITMASK;
         __vmwrite(GUEST_INTR_STATUS, status);
     }
 }
-- 
2.26.0

[Xen-devel] [PATCH v2 4/4] x86/nvmx: update exit bitmap when using virtual interrupt delivery

[Roger Pau Monne]

Force an update of the EOI exit bitmap in nvmx_update_apicv, because
the one performed in vmx_intr_assist might not be reached if the
interrupt is intercepted by nvmx_intr_intercept returning true.

Extract the code to update the exit bitmap from vmx_intr_assist into a
helper and use it in nvmx_update_apicv.

Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
---
Changes since v1:
 - Reword commit message.
---
 xen/arch/x86/hvm/vmx/intr.c       | 21 +++++++++++++--------
 xen/arch/x86/hvm/vmx/vvmx.c       |  2 ++
 xen/include/asm-x86/hvm/vmx/vmx.h |  2 ++
 3 files changed, 17 insertions(+), 8 deletions(-)

diff --git a/xen/arch/x86/hvm/vmx/intr.c b/xen/arch/x86/hvm/vmx/intr.c
index 49a1295f09..000e14af49 100644
--- a/xen/arch/x86/hvm/vmx/intr.c
+++ b/xen/arch/x86/hvm/vmx/intr.c
@@ -224,6 +224,18 @@ static int nvmx_intr_intercept(struct vcpu *v, struct hvm_intack intack)
     return 0;
 }
 
+void vmx_sync_exit_bitmap(struct vcpu *v)
+{
+    const unsigned int n = ARRAY_SIZE(v->arch.hvm.vmx.eoi_exit_bitmap);
+    unsigned int i;
+
+    while ( (i = find_first_bit(&v->arch.hvm.vmx.eoi_exitmap_changed, n)) < n )
+    {
+        clear_bit(i, &v->arch.hvm.vmx.eoi_exitmap_changed);
+        __vmwrite(EOI_EXIT_BITMAP(i), v->arch.hvm.vmx.eoi_exit_bitmap[i]);
+    }
+}
+
 void vmx_intr_assist(void)
 {
     struct hvm_intack intack;
@@ -318,7 +330,6 @@ void vmx_intr_assist(void)
               intack.source != hvm_intsrc_vector )
     {
         unsigned long status;
-        unsigned int i, n;
 
        /*
         * intack.vector is the highest priority vector. So we set eoi_exit_bitmap
@@ -379,13 +390,7 @@ void vmx_intr_assist(void)
                     intack.vector;
         __vmwrite(GUEST_INTR_STATUS, status);
 
-        n = ARRAY_SIZE(v->arch.hvm.vmx.eoi_exit_bitmap);
-        while ( (i = find_first_bit(&v->arch.hvm.vmx.eoi_exitmap_changed,
-                                    n)) < n )
-        {
-            clear_bit(i, &v->arch.hvm.vmx.eoi_exitmap_changed);
-            __vmwrite(EOI_EXIT_BITMAP(i), v->arch.hvm.vmx.eoi_exit_bitmap[i]);
-        }
+        vmx_sync_exit_bitmap(v);
 
         pt_intr_post(v, intack);
     }
diff --git a/xen/arch/x86/hvm/vmx/vvmx.c b/xen/arch/x86/hvm/vmx/vvmx.c
index 8431c912a1..845dd87f75 100644
--- a/xen/arch/x86/hvm/vmx/vvmx.c
+++ b/xen/arch/x86/hvm/vmx/vvmx.c
@@ -1418,6 +1418,8 @@ static void nvmx_update_apicv(struct vcpu *v)
         status |= rvi & VMX_GUEST_INTR_STATUS_SUBFIELD_BITMASK;
         __vmwrite(GUEST_INTR_STATUS, status);
     }
+
+    vmx_sync_exit_bitmap(v);
 }
 
 static void virtual_vmexit(struct cpu_user_regs *regs)
diff --git a/xen/include/asm-x86/hvm/vmx/vmx.h b/xen/include/asm-x86/hvm/vmx/vmx.h
index b334e1ec94..111ccd7e61 100644
--- a/xen/include/asm-x86/hvm/vmx/vmx.h
+++ b/xen/include/asm-x86/hvm/vmx/vmx.h
@@ -610,6 +610,8 @@ void update_guest_eip(void);
 void vmx_pi_per_cpu_init(unsigned int cpu);
 void vmx_pi_desc_fixup(unsigned int cpu);
 
+void vmx_sync_exit_bitmap(struct vcpu *v);
+
 #ifdef CONFIG_HVM
 void vmx_pi_hooks_assign(struct domain *d);
 void vmx_pi_hooks_deassign(struct domain *d);
-- 
2.26.0

Re: [Xen-devel] [PATCH v2 2/4] x86/nvmx: only update SVI when using Ack on exit

[Tian, Kevin]

> From: Roger Pau Monne <roger.pau@citrix.com>
> Sent: Wednesday, March 25, 2020 6:19 PM
> 
> Check whether there's a valid interrupt in VM_EXIT_INTR_INFO in order
> to decide whether to update SVI in nvmx_update_apicv. If Ack on exit
> is not being used VM_EXIT_INTR_INFO won't have a valid interrupt and
> hence SVI shouldn't be updated to signal the interrupt is currently in
> service because it won't be Acked.
> 
> Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>

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

Re: [Xen-devel] [PATCH v2 3/4] x86/nvmx: split updating RVI from SVI in nvmx_update_apicv

[Tian, Kevin]

> From: Roger Pau Monne <roger.pau@citrix.com>
> Sent: Wednesday, March 25, 2020 6:19 PM
> 
> Updating SVI is required when an interrupt has been injected using the
> Ack on exit VMEXIT feature, so that the in service interrupt in the
> GUEST_INTR_STATUS matches the vector that is signaled in
> VM_EXIT_INTR_INFO.
> 
> Updating RVI however is not tied to the Ack on exit feature, as it
> signals the next vector to be injected, and hence should always be
> updated to the next pending vector, regardless of whether Ack on exit
> is enabled.
> 
> Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
> ---
>  xen/arch/x86/hvm/vmx/vvmx.c | 22 +++++++++++++++++-----
>  1 file changed, 17 insertions(+), 5 deletions(-)
> 
> diff --git a/xen/arch/x86/hvm/vmx/vvmx.c b/xen/arch/x86/hvm/vmx/vvmx.c
> index 1753005c91..8431c912a1 100644
> --- a/xen/arch/x86/hvm/vmx/vvmx.c
> +++ b/xen/arch/x86/hvm/vmx/vvmx.c
> @@ -1384,26 +1384,38 @@ static void nvmx_update_apicv(struct vcpu *v)
>      struct nestedvmx *nvmx = &vcpu_2_nvmx(v);
>      unsigned long reason = get_vvmcs(v, VM_EXIT_REASON);
>      unsigned long intr_info = get_vvmcs(v, VM_EXIT_INTR_INFO);
> +    int rvi;
> 
>      if ( reason == EXIT_REASON_EXTERNAL_INTERRUPT &&
>           nvmx->intr.source == hvm_intsrc_lapic &&
>           (intr_info & INTR_INFO_VALID_MASK) )
>      {
>          uint16_t status;
> -        uint32_t rvi, ppr;
> -        uint32_t vector = intr_info & 0xff;
> +        uint32_t ppr;
> +        unsigned int vector = intr_info & INTR_INFO_VECTOR_MASK;
>          struct vlapic *vlapic = vcpu_vlapic(v);
> 
> +        /*
> +         * Update SVI to record the currently in service interrupt that's
> +         * signaled in EXIT_INTR_INFO.
> +         */
>          vlapic_ack_pending_irq(v, vector, 1);
> 
>          ppr = vlapic_set_ppr(vlapic);
>          WARN_ON((ppr & 0xf0) != (vector & 0xf0));
> 
>          status = vector << VMX_GUEST_INTR_STATUS_SVI_OFFSET;
> -        rvi = vlapic_has_pending_irq(v);
> -        if ( rvi != -1 )
> -            status |= rvi & VMX_GUEST_INTR_STATUS_SUBFIELD_BITMASK;
> +        __vmwrite(GUEST_INTR_STATUS, status);
> +    }
> +
> +    rvi = vlapic_has_pending_irq(v);
> +    if ( rvi != -1 )
> +    {
> +        unsigned long status;
> 
> +        __vmread(GUEST_INTR_STATUS, &status);
> +        status &= ~VMX_GUEST_INTR_STATUS_SUBFIELD_BITMASK;
> +        status |= rvi & VMX_GUEST_INTR_STATUS_SUBFIELD_BITMASK;
>          __vmwrite(GUEST_INTR_STATUS, status);
>      }

I have two minor comments. First, original code requires only one
__vmwrite but now needs two writes and one read for Ack-on-exit
is set. Can we optimize to sustain the original behavior? Second,
although I didn't come up a case where always updating RVI (w/
pending interrupt) may bring some bad effect, it is anyway safer
to put the whole logic within the check of vmexit reason and intr 
source.

Thanks
Kevin

Re: [Xen-devel] [PATCH v2 4/4] x86/nvmx: update exit bitmap when using virtual interrupt delivery

[Tian, Kevin]

> From: Roger Pau Monne <roger.pau@citrix.com>
> Sent: Wednesday, March 25, 2020 6:19 PM
> 
> Force an update of the EOI exit bitmap in nvmx_update_apicv, because
> the one performed in vmx_intr_assist might not be reached if the
> interrupt is intercepted by nvmx_intr_intercept returning true.
> 
> Extract the code to update the exit bitmap from vmx_intr_assist into a
> helper and use it in nvmx_update_apicv.
> 
> Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
> ---
> Changes since v1:
>  - Reword commit message.
> ---
>  xen/arch/x86/hvm/vmx/intr.c       | 21 +++++++++++++--------
>  xen/arch/x86/hvm/vmx/vvmx.c       |  2 ++
>  xen/include/asm-x86/hvm/vmx/vmx.h |  2 ++
>  3 files changed, 17 insertions(+), 8 deletions(-)
> 
> diff --git a/xen/arch/x86/hvm/vmx/intr.c b/xen/arch/x86/hvm/vmx/intr.c
> index 49a1295f09..000e14af49 100644
> --- a/xen/arch/x86/hvm/vmx/intr.c
> +++ b/xen/arch/x86/hvm/vmx/intr.c
> @@ -224,6 +224,18 @@ static int nvmx_intr_intercept(struct vcpu *v, struct
> hvm_intack intack)
>      return 0;
>  }
> 
> +void vmx_sync_exit_bitmap(struct vcpu *v)
> +{
> +    const unsigned int n = ARRAY_SIZE(v->arch.hvm.vmx.eoi_exit_bitmap);
> +    unsigned int i;
> +
> +    while ( (i = find_first_bit(&v->arch.hvm.vmx.eoi_exitmap_changed, n)) <
> n )
> +    {
> +        clear_bit(i, &v->arch.hvm.vmx.eoi_exitmap_changed);
> +        __vmwrite(EOI_EXIT_BITMAP(i), v->arch.hvm.vmx.eoi_exit_bitmap[i]);
> +    }
> +}
> +
>  void vmx_intr_assist(void)
>  {
>      struct hvm_intack intack;
> @@ -318,7 +330,6 @@ void vmx_intr_assist(void)
>                intack.source != hvm_intsrc_vector )
>      {
>          unsigned long status;
> -        unsigned int i, n;
> 
>         /*
>          * intack.vector is the highest priority vector. So we set eoi_exit_bitmap
> @@ -379,13 +390,7 @@ void vmx_intr_assist(void)
>                      intack.vector;
>          __vmwrite(GUEST_INTR_STATUS, status);
> 
> -        n = ARRAY_SIZE(v->arch.hvm.vmx.eoi_exit_bitmap);
> -        while ( (i = find_first_bit(&v->arch.hvm.vmx.eoi_exitmap_changed,
> -                                    n)) < n )
> -        {
> -            clear_bit(i, &v->arch.hvm.vmx.eoi_exitmap_changed);
> -            __vmwrite(EOI_EXIT_BITMAP(i), v->arch.hvm.vmx.eoi_exit_bitmap[i]);
> -        }
> +        vmx_sync_exit_bitmap(v);
> 
>          pt_intr_post(v, intack);
>      }
> diff --git a/xen/arch/x86/hvm/vmx/vvmx.c b/xen/arch/x86/hvm/vmx/vvmx.c
> index 8431c912a1..845dd87f75 100644
> --- a/xen/arch/x86/hvm/vmx/vvmx.c
> +++ b/xen/arch/x86/hvm/vmx/vvmx.c
> @@ -1418,6 +1418,8 @@ static void nvmx_update_apicv(struct vcpu *v)
>          status |= rvi & VMX_GUEST_INTR_STATUS_SUBFIELD_BITMASK;
>          __vmwrite(GUEST_INTR_STATUS, status);
>      }
> +
> +    vmx_sync_exit_bitmap(v);

Similarly, I'd like to do the sync within the conditional block, when intr
status is actually changed. Otherwise, it becomes checking bitmap change
in every vmentry when apicv is enabled.

>  }
> 
>  static void virtual_vmexit(struct cpu_user_regs *regs)
> diff --git a/xen/include/asm-x86/hvm/vmx/vmx.h b/xen/include/asm-
> x86/hvm/vmx/vmx.h
> index b334e1ec94..111ccd7e61 100644
> --- a/xen/include/asm-x86/hvm/vmx/vmx.h
> +++ b/xen/include/asm-x86/hvm/vmx/vmx.h
> @@ -610,6 +610,8 @@ void update_guest_eip(void);
>  void vmx_pi_per_cpu_init(unsigned int cpu);
>  void vmx_pi_desc_fixup(unsigned int cpu);
> 
> +void vmx_sync_exit_bitmap(struct vcpu *v);
> +
>  #ifdef CONFIG_HVM
>  void vmx_pi_hooks_assign(struct domain *d);
>  void vmx_pi_hooks_deassign(struct domain *d);
> --
> 2.26.0

Re: [Xen-devel] [PATCH v2 3/4] x86/nvmx: split updating RVI from SVI in nvmx_update_apicv

[Roger Pau Monné]

On Thu, Mar 26, 2020 at 03:13:56AM +0000, Tian, Kevin wrote:
> > From: Roger Pau Monne <roger.pau@citrix.com>
> > Sent: Wednesday, March 25, 2020 6:19 PM
> > 
> > Updating SVI is required when an interrupt has been injected using the
> > Ack on exit VMEXIT feature, so that the in service interrupt in the
> > GUEST_INTR_STATUS matches the vector that is signaled in
> > VM_EXIT_INTR_INFO.
> > 
> > Updating RVI however is not tied to the Ack on exit feature, as it
> > signals the next vector to be injected, and hence should always be
> > updated to the next pending vector, regardless of whether Ack on exit
> > is enabled.
> > 
> > Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
> > ---
> >  xen/arch/x86/hvm/vmx/vvmx.c | 22 +++++++++++++++++-----
> >  1 file changed, 17 insertions(+), 5 deletions(-)
> > 
> > diff --git a/xen/arch/x86/hvm/vmx/vvmx.c b/xen/arch/x86/hvm/vmx/vvmx.c
> > index 1753005c91..8431c912a1 100644
> > --- a/xen/arch/x86/hvm/vmx/vvmx.c
> > +++ b/xen/arch/x86/hvm/vmx/vvmx.c
> > @@ -1384,26 +1384,38 @@ static void nvmx_update_apicv(struct vcpu *v)
> >      struct nestedvmx *nvmx = &vcpu_2_nvmx(v);
> >      unsigned long reason = get_vvmcs(v, VM_EXIT_REASON);
> >      unsigned long intr_info = get_vvmcs(v, VM_EXIT_INTR_INFO);
> > +    int rvi;
> > 
> >      if ( reason == EXIT_REASON_EXTERNAL_INTERRUPT &&
> >           nvmx->intr.source == hvm_intsrc_lapic &&
> >           (intr_info & INTR_INFO_VALID_MASK) )
> >      {
> >          uint16_t status;
> > -        uint32_t rvi, ppr;
> > -        uint32_t vector = intr_info & 0xff;
> > +        uint32_t ppr;
> > +        unsigned int vector = intr_info & INTR_INFO_VECTOR_MASK;
> >          struct vlapic *vlapic = vcpu_vlapic(v);
> > 
> > +        /*
> > +         * Update SVI to record the currently in service interrupt that's
> > +         * signaled in EXIT_INTR_INFO.
> > +         */
> >          vlapic_ack_pending_irq(v, vector, 1);
> > 
> >          ppr = vlapic_set_ppr(vlapic);
> >          WARN_ON((ppr & 0xf0) != (vector & 0xf0));
> > 
> >          status = vector << VMX_GUEST_INTR_STATUS_SVI_OFFSET;
> > -        rvi = vlapic_has_pending_irq(v);
> > -        if ( rvi != -1 )
> > -            status |= rvi & VMX_GUEST_INTR_STATUS_SUBFIELD_BITMASK;
> > +        __vmwrite(GUEST_INTR_STATUS, status);
> > +    }
> > +
> > +    rvi = vlapic_has_pending_irq(v);
> > +    if ( rvi != -1 )
> > +    {
> > +        unsigned long status;
> > 
> > +        __vmread(GUEST_INTR_STATUS, &status);
> > +        status &= ~VMX_GUEST_INTR_STATUS_SUBFIELD_BITMASK;
> > +        status |= rvi & VMX_GUEST_INTR_STATUS_SUBFIELD_BITMASK;
> >          __vmwrite(GUEST_INTR_STATUS, status);
> >      }
> 
> I have two minor comments. First, original code requires only one
> __vmwrite but now needs two writes and one read for Ack-on-exit
> is set. Can we optimize to sustain the original behavior?

I think I can manage to do a single write by using a global status
variable initialized to 0 and only do the vmwrite if the value is != 0
by the end of the function.

> Second,
> although I didn't come up a case where always updating RVI (w/
> pending interrupt) may bring some bad effect, it is anyway safer
> to put the whole logic within the check of vmexit reason and intr 
> source.

I also considered this and decided there was no harm in always
updating RVI if there's a pending interrupt, it could even prevent a
future VMEXIT to update RVI AFAICT?

If you prefer I can return early from the function if reason !=
EXIT_REASON_EXTERNAL_INTERRUPT.

Thanks, Roger.

Re: [Xen-devel] [PATCH v2 4/4] x86/nvmx: update exit bitmap when using virtual interrupt delivery

[Roger Pau Monné]

On Thu, Mar 26, 2020 at 03:17:59AM +0000, Tian, Kevin wrote:
> > From: Roger Pau Monne <roger.pau@citrix.com>
> > Sent: Wednesday, March 25, 2020 6:19 PM
> > 
> > Force an update of the EOI exit bitmap in nvmx_update_apicv, because
> > the one performed in vmx_intr_assist might not be reached if the
> > interrupt is intercepted by nvmx_intr_intercept returning true.
> > 
> > Extract the code to update the exit bitmap from vmx_intr_assist into a
> > helper and use it in nvmx_update_apicv.
> > 
> > Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
> > ---
> > Changes since v1:
> >  - Reword commit message.
> > ---
> >  xen/arch/x86/hvm/vmx/intr.c       | 21 +++++++++++++--------
> >  xen/arch/x86/hvm/vmx/vvmx.c       |  2 ++
> >  xen/include/asm-x86/hvm/vmx/vmx.h |  2 ++
> >  3 files changed, 17 insertions(+), 8 deletions(-)
> > 
> > diff --git a/xen/arch/x86/hvm/vmx/intr.c b/xen/arch/x86/hvm/vmx/intr.c
> > index 49a1295f09..000e14af49 100644
> > --- a/xen/arch/x86/hvm/vmx/intr.c
> > +++ b/xen/arch/x86/hvm/vmx/intr.c
> > @@ -224,6 +224,18 @@ static int nvmx_intr_intercept(struct vcpu *v, struct
> > hvm_intack intack)
> >      return 0;
> >  }
> > 
> > +void vmx_sync_exit_bitmap(struct vcpu *v)
> > +{
> > +    const unsigned int n = ARRAY_SIZE(v->arch.hvm.vmx.eoi_exit_bitmap);
> > +    unsigned int i;
> > +
> > +    while ( (i = find_first_bit(&v->arch.hvm.vmx.eoi_exitmap_changed, n)) <
> > n )
> > +    {
> > +        clear_bit(i, &v->arch.hvm.vmx.eoi_exitmap_changed);
> > +        __vmwrite(EOI_EXIT_BITMAP(i), v->arch.hvm.vmx.eoi_exit_bitmap[i]);
> > +    }
> > +}
> > +
> >  void vmx_intr_assist(void)
> >  {
> >      struct hvm_intack intack;
> > @@ -318,7 +330,6 @@ void vmx_intr_assist(void)
> >                intack.source != hvm_intsrc_vector )
> >      {
> >          unsigned long status;
> > -        unsigned int i, n;
> > 
> >         /*
> >          * intack.vector is the highest priority vector. So we set eoi_exit_bitmap
> > @@ -379,13 +390,7 @@ void vmx_intr_assist(void)
> >                      intack.vector;
> >          __vmwrite(GUEST_INTR_STATUS, status);
> > 
> > -        n = ARRAY_SIZE(v->arch.hvm.vmx.eoi_exit_bitmap);
> > -        while ( (i = find_first_bit(&v->arch.hvm.vmx.eoi_exitmap_changed,
> > -                                    n)) < n )
> > -        {
> > -            clear_bit(i, &v->arch.hvm.vmx.eoi_exitmap_changed);
> > -            __vmwrite(EOI_EXIT_BITMAP(i), v->arch.hvm.vmx.eoi_exit_bitmap[i]);
> > -        }
> > +        vmx_sync_exit_bitmap(v);
> > 
> >          pt_intr_post(v, intack);
> >      }
> > diff --git a/xen/arch/x86/hvm/vmx/vvmx.c b/xen/arch/x86/hvm/vmx/vvmx.c
> > index 8431c912a1..845dd87f75 100644
> > --- a/xen/arch/x86/hvm/vmx/vvmx.c
> > +++ b/xen/arch/x86/hvm/vmx/vvmx.c
> > @@ -1418,6 +1418,8 @@ static void nvmx_update_apicv(struct vcpu *v)
> >          status |= rvi & VMX_GUEST_INTR_STATUS_SUBFIELD_BITMASK;
> >          __vmwrite(GUEST_INTR_STATUS, status);
> >      }
> > +
> > +    vmx_sync_exit_bitmap(v);
> 
> Similarly, I'd like to do the sync within the conditional block, when intr
> status is actually changed. Otherwise, it becomes checking bitmap change
> in every vmentry when apicv is enabled.

No - it will only check the bitmap when there's a virtual vmexit
(which is where nvmx_update_apicv gets called), not on every vmentry.
I can try to do this conditionally on whether GUEST_INTR_STATUS is
actually changed.

Thanks, Roger.

Re: [Xen-devel] [PATCH v2 3/4] x86/nvmx: split updating RVI from SVI in nvmx_update_apicv

[Tian, Kevin]

> From: Roger Pau Monné <roger.pau@citrix.com>
> Sent: Thursday, March 26, 2020 5:20 PM
> 
> On Thu, Mar 26, 2020 at 03:13:56AM +0000, Tian, Kevin wrote:
> > > From: Roger Pau Monne <roger.pau@citrix.com>
> > > Sent: Wednesday, March 25, 2020 6:19 PM
> > >
> > > Updating SVI is required when an interrupt has been injected using the
> > > Ack on exit VMEXIT feature, so that the in service interrupt in the
> > > GUEST_INTR_STATUS matches the vector that is signaled in
> > > VM_EXIT_INTR_INFO.
> > >
> > > Updating RVI however is not tied to the Ack on exit feature, as it
> > > signals the next vector to be injected, and hence should always be
> > > updated to the next pending vector, regardless of whether Ack on exit
> > > is enabled.
> > >
> > > Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
> > > ---
> > >  xen/arch/x86/hvm/vmx/vvmx.c | 22 +++++++++++++++++-----
> > >  1 file changed, 17 insertions(+), 5 deletions(-)
> > >
> > > diff --git a/xen/arch/x86/hvm/vmx/vvmx.c
> b/xen/arch/x86/hvm/vmx/vvmx.c
> > > index 1753005c91..8431c912a1 100644
> > > --- a/xen/arch/x86/hvm/vmx/vvmx.c
> > > +++ b/xen/arch/x86/hvm/vmx/vvmx.c
> > > @@ -1384,26 +1384,38 @@ static void nvmx_update_apicv(struct vcpu
> *v)
> > >      struct nestedvmx *nvmx = &vcpu_2_nvmx(v);
> > >      unsigned long reason = get_vvmcs(v, VM_EXIT_REASON);
> > >      unsigned long intr_info = get_vvmcs(v, VM_EXIT_INTR_INFO);
> > > +    int rvi;
> > >
> > >      if ( reason == EXIT_REASON_EXTERNAL_INTERRUPT &&
> > >           nvmx->intr.source == hvm_intsrc_lapic &&
> > >           (intr_info & INTR_INFO_VALID_MASK) )
> > >      {
> > >          uint16_t status;
> > > -        uint32_t rvi, ppr;
> > > -        uint32_t vector = intr_info & 0xff;
> > > +        uint32_t ppr;
> > > +        unsigned int vector = intr_info & INTR_INFO_VECTOR_MASK;
> > >          struct vlapic *vlapic = vcpu_vlapic(v);
> > >
> > > +        /*
> > > +         * Update SVI to record the currently in service interrupt that's
> > > +         * signaled in EXIT_INTR_INFO.
> > > +         */
> > >          vlapic_ack_pending_irq(v, vector, 1);
> > >
> > >          ppr = vlapic_set_ppr(vlapic);
> > >          WARN_ON((ppr & 0xf0) != (vector & 0xf0));
> > >
> > >          status = vector << VMX_GUEST_INTR_STATUS_SVI_OFFSET;
> > > -        rvi = vlapic_has_pending_irq(v);
> > > -        if ( rvi != -1 )
> > > -            status |= rvi & VMX_GUEST_INTR_STATUS_SUBFIELD_BITMASK;
> > > +        __vmwrite(GUEST_INTR_STATUS, status);
> > > +    }
> > > +
> > > +    rvi = vlapic_has_pending_irq(v);
> > > +    if ( rvi != -1 )
> > > +    {
> > > +        unsigned long status;
> > >
> > > +        __vmread(GUEST_INTR_STATUS, &status);
> > > +        status &= ~VMX_GUEST_INTR_STATUS_SUBFIELD_BITMASK;
> > > +        status |= rvi & VMX_GUEST_INTR_STATUS_SUBFIELD_BITMASK;
> > >          __vmwrite(GUEST_INTR_STATUS, status);
> > >      }
> >
> > I have two minor comments. First, original code requires only one
> > __vmwrite but now needs two writes and one read for Ack-on-exit
> > is set. Can we optimize to sustain the original behavior?
> 
> I think I can manage to do a single write by using a global status
> variable initialized to 0 and only do the vmwrite if the value is != 0
> by the end of the function.
> 
> > Second,
> > although I didn't come up a case where always updating RVI (w/
> > pending interrupt) may bring some bad effect, it is anyway safer
> > to put the whole logic within the check of vmexit reason and intr
> > source.
> 
> I also considered this and decided there was no harm in always
> updating RVI if there's a pending interrupt, it could even prevent a
> future VMEXIT to update RVI AFAICT?
> 
> If you prefer I can return early from the function if reason !=
> EXIT_REASON_EXTERNAL_INTERRUPT.
> 

I agree with your but given the trickiness of nested intr handling
I prefer to a more conservative but safer approach. So yes, return
early sounds good here.

Thanks
Kevin

Re: [Xen-devel] [PATCH v2 4/4] x86/nvmx: update exit bitmap when using virtual interrupt delivery

[Tian, Kevin]

> From: Roger Pau Monné <roger.pau@citrix.com>
> Sent: Thursday, March 26, 2020 5:22 PM
> 
> On Thu, Mar 26, 2020 at 03:17:59AM +0000, Tian, Kevin wrote:
> > > From: Roger Pau Monne <roger.pau@citrix.com>
> > > Sent: Wednesday, March 25, 2020 6:19 PM
> > >
> > > Force an update of the EOI exit bitmap in nvmx_update_apicv, because
> > > the one performed in vmx_intr_assist might not be reached if the
> > > interrupt is intercepted by nvmx_intr_intercept returning true.
> > >
> > > Extract the code to update the exit bitmap from vmx_intr_assist into a
> > > helper and use it in nvmx_update_apicv.
> > >
> > > Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
> > > ---
> > > Changes since v1:
> > >  - Reword commit message.
> > > ---
> > >  xen/arch/x86/hvm/vmx/intr.c       | 21 +++++++++++++--------
> > >  xen/arch/x86/hvm/vmx/vvmx.c       |  2 ++
> > >  xen/include/asm-x86/hvm/vmx/vmx.h |  2 ++
> > >  3 files changed, 17 insertions(+), 8 deletions(-)
> > >
> > > diff --git a/xen/arch/x86/hvm/vmx/intr.c b/xen/arch/x86/hvm/vmx/intr.c
> > > index 49a1295f09..000e14af49 100644
> > > --- a/xen/arch/x86/hvm/vmx/intr.c
> > > +++ b/xen/arch/x86/hvm/vmx/intr.c
> > > @@ -224,6 +224,18 @@ static int nvmx_intr_intercept(struct vcpu *v,
> struct
> > > hvm_intack intack)
> > >      return 0;
> > >  }
> > >
> > > +void vmx_sync_exit_bitmap(struct vcpu *v)
> > > +{
> > > +    const unsigned int n = ARRAY_SIZE(v->arch.hvm.vmx.eoi_exit_bitmap);
> > > +    unsigned int i;
> > > +
> > > +    while ( (i = find_first_bit(&v->arch.hvm.vmx.eoi_exitmap_changed, n))
> <
> > > n )
> > > +    {
> > > +        clear_bit(i, &v->arch.hvm.vmx.eoi_exitmap_changed);
> > > +        __vmwrite(EOI_EXIT_BITMAP(i), v-
> >arch.hvm.vmx.eoi_exit_bitmap[i]);
> > > +    }
> > > +}
> > > +
> > >  void vmx_intr_assist(void)
> > >  {
> > >      struct hvm_intack intack;
> > > @@ -318,7 +330,6 @@ void vmx_intr_assist(void)
> > >                intack.source != hvm_intsrc_vector )
> > >      {
> > >          unsigned long status;
> > > -        unsigned int i, n;
> > >
> > >         /*
> > >          * intack.vector is the highest priority vector. So we set
> eoi_exit_bitmap
> > > @@ -379,13 +390,7 @@ void vmx_intr_assist(void)
> > >                      intack.vector;
> > >          __vmwrite(GUEST_INTR_STATUS, status);
> > >
> > > -        n = ARRAY_SIZE(v->arch.hvm.vmx.eoi_exit_bitmap);
> > > -        while ( (i = find_first_bit(&v->arch.hvm.vmx.eoi_exitmap_changed,
> > > -                                    n)) < n )
> > > -        {
> > > -            clear_bit(i, &v->arch.hvm.vmx.eoi_exitmap_changed);
> > > -            __vmwrite(EOI_EXIT_BITMAP(i), v-
> >arch.hvm.vmx.eoi_exit_bitmap[i]);
> > > -        }
> > > +        vmx_sync_exit_bitmap(v);
> > >
> > >          pt_intr_post(v, intack);
> > >      }
> > > diff --git a/xen/arch/x86/hvm/vmx/vvmx.c
> b/xen/arch/x86/hvm/vmx/vvmx.c
> > > index 8431c912a1..845dd87f75 100644
> > > --- a/xen/arch/x86/hvm/vmx/vvmx.c
> > > +++ b/xen/arch/x86/hvm/vmx/vvmx.c
> > > @@ -1418,6 +1418,8 @@ static void nvmx_update_apicv(struct vcpu *v)
> > >          status |= rvi & VMX_GUEST_INTR_STATUS_SUBFIELD_BITMASK;
> > >          __vmwrite(GUEST_INTR_STATUS, status);
> > >      }
> > > +
> > > +    vmx_sync_exit_bitmap(v);
> >
> > Similarly, I'd like to do the sync within the conditional block, when intr
> > status is actually changed. Otherwise, it becomes checking bitmap change
> > in every vmentry when apicv is enabled.
> 
> No - it will only check the bitmap when there's a virtual vmexit
> (which is where nvmx_update_apicv gets called), not on every vmentry.

you are right. I overlooked it. but still it's unnecessary to do it for
every virtual vmexit. 😊

> I can try to do this conditionally on whether GUEST_INTR_STATUS is
> actually changed.
> 

yes.

Thanks
Kevin