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

[Xen-devel] [PATCH v3 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/149051/

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       | 43 ++++++++++++++++++++-----------
 xen/include/asm-x86/hvm/vmx/vmx.h |  2 ++
 3 files changed, 43 insertions(+), 23 deletions(-)

-- 
2.26.0

[Xen-devel] [PATCH v3 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 v3 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>
Reviewed-by: Kevin Tian <kevin.tian@intel.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 v3 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.

When not using the Ack on exit feature preserve the previous vector in
SVI, so that it's not lost when RVI is updated to contain the pending
vector to inject.

Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
---
Changes since v2:
 - Return early if the exit reason != EXTERNAL_INTERRUPT.
 - Reduce the number of vmwrites by accumulating the changes to a
   local variable which is flushed at the end of the function.
 - Attempt to preserve the exiting SVI if Ack on exit is not enabled.
---
 xen/arch/x86/hvm/vmx/vvmx.c | 33 ++++++++++++++++++++++++---------
 1 file changed, 24 insertions(+), 9 deletions(-)

diff --git a/xen/arch/x86/hvm/vmx/vvmx.c b/xen/arch/x86/hvm/vmx/vvmx.c
index 1753005c91..39fb553590 100644
--- a/xen/arch/x86/hvm/vmx/vvmx.c
+++ b/xen/arch/x86/hvm/vmx/vvmx.c
@@ -1384,28 +1384,43 @@ 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);
+    unsigned long status;
+    int rvi;
 
-    if ( reason == EXIT_REASON_EXTERNAL_INTERRUPT &&
-         nvmx->intr.source == hvm_intsrc_lapic &&
+    if ( reason != EXIT_REASON_EXTERNAL_INTERRUPT )
+        return;
+
+    if ( 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 current 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;
+    }
+    else
+       /* Keep previous SVI if there's any. */
+       __vmread(GUEST_INTR_STATUS, &status);
 
-        __vmwrite(GUEST_INTR_STATUS, status);
+    rvi = vlapic_has_pending_irq(v);
+    if ( rvi != -1 )
+    {
+        status &= ~VMX_GUEST_INTR_STATUS_SUBFIELD_BITMASK
+        status |= rvi & VMX_GUEST_INTR_STATUS_SUBFIELD_BITMASK;
     }
+
+    if ( status )
+        __vmwrite(GUEST_INTR_STATUS, status);
 }
 
 static void virtual_vmexit(struct cpu_user_regs *regs)
-- 
2.26.0

[Xen-devel] [PATCH v3 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 v2:
 - Only update the EOI exit bitmap if GUEST_INTR_STATUS is changed.

Changes since v1:
 - Reword commit message.
---
 xen/arch/x86/hvm/vmx/intr.c       | 21 +++++++++++++--------
 xen/arch/x86/hvm/vmx/vvmx.c       |  3 +++
 xen/include/asm-x86/hvm/vmx/vmx.h |  2 ++
 3 files changed, 18 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 39fb553590..452f69e2f7 100644
--- a/xen/arch/x86/hvm/vmx/vvmx.c
+++ b/xen/arch/x86/hvm/vmx/vvmx.c
@@ -1420,7 +1420,10 @@ static void nvmx_update_apicv(struct vcpu *v)
     }
 
     if ( status )
+    {
         __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 v3 0/4] x86/nvmx: fixes for interrupt injection

[Jan Beulich]

On 26.03.2020 16:27, Roger Pau Monne wrote:
> 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/149051/
> 
> 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

You probably didn't notice that these two got committed earlier today?

Jan

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

[Roger Pau Monné]

On Thu, Mar 26, 2020 at 04:41:15PM +0100, Jan Beulich wrote:
> On 26.03.2020 16:27, Roger Pau Monne wrote:
> > 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/149051/
> > 
> > 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
> 
> You probably didn't notice that these two got committed earlier today?

Urg no, sorry. I rebased before lunch and then triggered the osstest
job. Thanks for committing those two!

Roger.

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

[Tian, Kevin]

> From: Roger Pau Monne <roger.pau@citrix.com>
> Sent: Thursday, March 26, 2020 11:27 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.
> 
> When not using the Ack on exit feature preserve the previous vector in
> SVI, so that it's not lost when RVI is updated to contain the pending
> vector to inject.
> 
> Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>

Reviewed-by: Kevin Tian <kevin.tian@intel.com>, with one small comment:

> ---
> Changes since v2:
>  - Return early if the exit reason != EXTERNAL_INTERRUPT.
>  - Reduce the number of vmwrites by accumulating the changes to a
>    local variable which is flushed at the end of the function.
>  - Attempt to preserve the exiting SVI if Ack on exit is not enabled.
> ---
>  xen/arch/x86/hvm/vmx/vvmx.c | 33 ++++++++++++++++++++++++---------
>  1 file changed, 24 insertions(+), 9 deletions(-)
> 
> diff --git a/xen/arch/x86/hvm/vmx/vvmx.c b/xen/arch/x86/hvm/vmx/vvmx.c
> index 1753005c91..39fb553590 100644
> --- a/xen/arch/x86/hvm/vmx/vvmx.c
> +++ b/xen/arch/x86/hvm/vmx/vvmx.c
> @@ -1384,28 +1384,43 @@ 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);
> +    unsigned long status;
> +    int rvi;
> 
> -    if ( reason == EXIT_REASON_EXTERNAL_INTERRUPT &&
> -         nvmx->intr.source == hvm_intsrc_lapic &&
> +    if ( reason != EXIT_REASON_EXTERNAL_INTERRUPT )
> +        return;

can we also exit if source is not lapic? as we discussed in another
thread, the whole logic here is only for lapic not others...

Thanks
Kevin

> +
> +    if ( 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 current 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;
> +    }
> +    else
> +       /* Keep previous SVI if there's any. */
> +       __vmread(GUEST_INTR_STATUS, &status);
> 
> -        __vmwrite(GUEST_INTR_STATUS, status);
> +    rvi = vlapic_has_pending_irq(v);
> +    if ( rvi != -1 )
> +    {
> +        status &= ~VMX_GUEST_INTR_STATUS_SUBFIELD_BITMASK
> +        status |= rvi & VMX_GUEST_INTR_STATUS_SUBFIELD_BITMASK;
>      }
> +
> +    if ( status )
> +        __vmwrite(GUEST_INTR_STATUS, status);
>  }
> 
>  static void virtual_vmexit(struct cpu_user_regs *regs)
> --
> 2.26.0

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

[Tian, Kevin]

> From: Roger Pau Monne <roger.pau@citrix.com>
> Sent: Thursday, March 26, 2020 11:27 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>

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

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

[Roger Pau Monné]

On Fri, Mar 27, 2020 at 02:21:46AM +0000, Tian, Kevin wrote:
> > From: Roger Pau Monne <roger.pau@citrix.com>
> > Sent: Thursday, March 26, 2020 11:27 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.
> > 
> > When not using the Ack on exit feature preserve the previous vector in
> > SVI, so that it's not lost when RVI is updated to contain the pending
> > vector to inject.
> > 
> > Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
> 
> Reviewed-by: Kevin Tian <kevin.tian@intel.com>, with one small comment:
> 
> > ---
> > Changes since v2:
> >  - Return early if the exit reason != EXTERNAL_INTERRUPT.
> >  - Reduce the number of vmwrites by accumulating the changes to a
> >    local variable which is flushed at the end of the function.
> >  - Attempt to preserve the exiting SVI if Ack on exit is not enabled.
> > ---
> >  xen/arch/x86/hvm/vmx/vvmx.c | 33 ++++++++++++++++++++++++---------
> >  1 file changed, 24 insertions(+), 9 deletions(-)
> > 
> > diff --git a/xen/arch/x86/hvm/vmx/vvmx.c b/xen/arch/x86/hvm/vmx/vvmx.c
> > index 1753005c91..39fb553590 100644
> > --- a/xen/arch/x86/hvm/vmx/vvmx.c
> > +++ b/xen/arch/x86/hvm/vmx/vvmx.c
> > @@ -1384,28 +1384,43 @@ 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);
> > +    unsigned long status;
> > +    int rvi;
> > 
> > -    if ( reason == EXIT_REASON_EXTERNAL_INTERRUPT &&
> > -         nvmx->intr.source == hvm_intsrc_lapic &&
> > +    if ( reason != EXIT_REASON_EXTERNAL_INTERRUPT )
> > +        return;
> 
> can we also exit if source is not lapic? as we discussed in another
> thread, the whole logic here is only for lapic not others...

Right, in any case the code below will only update GUEST_INTR_STATUS
(either SVI or RVI) for lapic interrupts, but returning early if the
source is not the lapic will do no harm AFAICT.

Will send v4 with this changed.

Thanks, Roger.