In vioapic_update_EOI the irq_lock will be dropped in order to forward
the EOI to the dpci handler, so there's a window between clearing IRR
and checking if the line is asserted where IRR can change behind our
back.

Fix this by checking whether IRR is set before attempting to inject a
new interrupt.

Fixes: 06e3f8f2766 ('vt-d: Do dpci eoi outside of irq_lock.')
Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
---
 xen/arch/x86/hvm/vioapic.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/xen/arch/x86/hvm/vioapic.c b/xen/arch/x86/hvm/vioapic.c
index eb6c143f74..804bc77279 100644
--- a/xen/arch/x86/hvm/vioapic.c
+++ b/xen/arch/x86/hvm/vioapic.c
@@ -526,7 +526,7 @@ void vioapic_update_EOI(struct domain *d, u8 vector)
             }
 
             if ( (ent->fields.trig_mode == VIOAPIC_LEVEL_TRIG) &&
-                 !ent->fields.mask &&
+                 !ent->fields.mask && !ent->fields.remote_irr &&
                  hvm_irq->gsi_assert_count[vioapic->base_gsi + pin] )
             {
                 ent->fields.remote_irr = 1;
-- 
2.29.2

On 15.01.2021 15:28, Roger Pau Monne wrote:
> In vioapic_update_EOI the irq_lock will be dropped in order to forward
> the EOI to the dpci handler, so there's a window between clearing IRR
> and checking if the line is asserted where IRR can change behind our
> back.
> 
> Fix this by checking whether IRR is set before attempting to inject a
> new interrupt.
> 
> Fixes: 06e3f8f2766 ('vt-d: Do dpci eoi outside of irq_lock.')
> Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>

It's fine this way, so
Reviewed-by: Jan Beulich <jbeulich@suse.com>
but how about a slightly different change:

> --- a/xen/arch/x86/hvm/vioapic.c
> +++ b/xen/arch/x86/hvm/vioapic.c
> @@ -526,7 +526,7 @@ void vioapic_update_EOI(struct domain *d, u8 vector)
>              }
>  
>              if ( (ent->fields.trig_mode == VIOAPIC_LEVEL_TRIG) &&
> -                 !ent->fields.mask &&
> +                 !ent->fields.mask && !ent->fields.remote_irr &&
>                   hvm_irq->gsi_assert_count[vioapic->base_gsi + pin] )
>              {
>                  ent->fields.remote_irr = 1;

The check is only needed if the lock was dropped intermediately,
which happens only conditionally. So an alternative would seem
to be

            if ( is_iommu_enabled(d) )
            {
                spin_unlock(&d->arch.hvm.irq_lock);
                hvm_dpci_eoi(d, vioapic->base_gsi + pin, ent);
                spin_lock(&d->arch.hvm.irq_lock);

                if ( ent->fields.remote_irr )
                    continue;
            }

in the code immediately above. Thoughts?

Jan

On Thu, Jan 21, 2021 at 05:03:51PM +0100, Jan Beulich wrote:
> On 15.01.2021 15:28, Roger Pau Monne wrote:
> > In vioapic_update_EOI the irq_lock will be dropped in order to forward
> > the EOI to the dpci handler, so there's a window between clearing IRR
> > and checking if the line is asserted where IRR can change behind our
> > back.
> > 
> > Fix this by checking whether IRR is set before attempting to inject a
> > new interrupt.
> > 
> > Fixes: 06e3f8f2766 ('vt-d: Do dpci eoi outside of irq_lock.')
> > Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
> 
> It's fine this way, so
> Reviewed-by: Jan Beulich <jbeulich@suse.com>
> but how about a slightly different change:
> 
> > --- a/xen/arch/x86/hvm/vioapic.c
> > +++ b/xen/arch/x86/hvm/vioapic.c
> > @@ -526,7 +526,7 @@ void vioapic_update_EOI(struct domain *d, u8 vector)
> >              }
> >  
> >              if ( (ent->fields.trig_mode == VIOAPIC_LEVEL_TRIG) &&
> > -                 !ent->fields.mask &&
> > +                 !ent->fields.mask && !ent->fields.remote_irr &&
> >                   hvm_irq->gsi_assert_count[vioapic->base_gsi + pin] )
> >              {
> >                  ent->fields.remote_irr = 1;
> 
> The check is only needed if the lock was dropped intermediately,
> which happens only conditionally. So an alternative would seem
> to be
> 
>             if ( is_iommu_enabled(d) )
>             {
>                 spin_unlock(&d->arch.hvm.irq_lock);
>                 hvm_dpci_eoi(d, vioapic->base_gsi + pin, ent);
>                 spin_lock(&d->arch.hvm.irq_lock);
> 
>                 if ( ent->fields.remote_irr )
>                     continue;
>             }
> 
> in the code immediately above. Thoughts?

IMO that seems more dangerous as if new code is added below that chunk
that wouldn't depend on the value of IRR it might get skipped
unintentionally, as it's possible to oversight that the loop is
short-circuited here.

Thanks, Roger.