Message ID | 20210115142820.35224-2-roger.pau@citrix.com (mailing list archive) |
---|---|
State | New, archived |
Headers | show |
Series | x86/intr: guest interrupt handling fixes/cleanup | expand |
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.
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;
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(-)