Re: [PATCH v2 1/4] x86/vioapic: check IRR before attempting to inject interrupt after EOI

From: "Roger Pau Monné" <roger.pau@citrix.com>
To: Jan Beulich <jbeulich@suse.com>
Cc: Andrew Cooper <andrew.cooper3@citrix.com>, Wei Liu <wl@xen.org>,
	<xen-devel@lists.xenproject.org>
Subject: Re: [PATCH v2 1/4] x86/vioapic: check IRR before attempting to inject interrupt after EOI
Date: Thu, 21 Jan 2021 18:27:16 +0100	[thread overview]
Message-ID: <20210121172716.kdfepuukimrylyru@Air-de-Roger> (raw)
In-Reply-To: <1cdc469f-5ccf-6c0c-157c-31abca5f089d@suse.com>

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.