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

> 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

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.

> 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