[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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


  • To: Jan Beulich <jbeulich@xxxxxxxx>
  • From: Roger Pau Monné <roger.pau@xxxxxxxxxx>
  • Date: Thu, 21 Jan 2021 18:27:16 +0100
  • Arc-authentication-results: i=1; mx.microsoft.com 1; spf=pass smtp.mailfrom=citrix.com; dmarc=pass action=none header.from=citrix.com; dkim=pass header.d=citrix.com; arc=none
  • Arc-message-signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=microsoft.com; s=arcselector9901; h=From:Date:Subject:Message-ID:Content-Type:MIME-Version:X-MS-Exchange-SenderADCheck; bh=6x/nKLlVZNIv+D59+I1UEtjPy9lAlVvCZLxweSM1qnk=; b=E8oj1XQFornmLeC6G7qECCNb2uTDmEFZjcLFt7qLRriHFWYtBoo3QQn+RwzaLMAM5IdIdnXljlWC//HoxJpX7wnjOoi/0bRgLSewmnm3dGBewU70x/TCQ66MmsvEZ4X+kR0rZuTg/AaU3hryxun44gdvbwv43q3NnbdBslpS1zNG7krMGoKbvtok4z4aTkNLxubo1QD0IqaAX7/5BeNRRJ8Imi5vRVuxHLXTKzowajiH80xCkj81SohHO73YC51t8mUg3asvZtPQrgHr35JhXVIV8O1Ojx7YGE4kUOvE9GVgIaX02fLSZSvbWLFyMU6SWEqWw2mk9s8tgZ+92/VLEA==
  • Arc-seal: i=1; a=rsa-sha256; s=arcselector9901; d=microsoft.com; cv=none; b=Qu5mEVLggGInWJmsB0S0Pksb4ZREfZz3hP79TAPt7Kz/FLWWYQRCC3avXa3oC5J0JHQLSYqCmdbWWLSKYWzhXM/P/SQOBichrdXs7mKyvJAhRz985Z2ddfMorEgSTahdoXnPfCgOuOPjb2VhBQFPu+NcGjumDsOdEG35goaTMTJAhfcgUpbCAhnPPFByDgD7NLAub12DS8a1ysGSvw8nxh2g0Y9SRHuFwcqza/I5k6zkrMpZh6BK6ucunnIfxZ8DIkYb4+4j0pkTsvfuxtE7HQIHJ+gu+3BA/iCFUxj1zRqG0/lH2VmffY5Pby8EOlL+GGmqhhhs28FGfASe4zID/w==
  • Authentication-results: esa4.hc3370-68.iphmx.com; dkim=pass (signature verified) header.i=@citrix.onmicrosoft.com
  • Cc: Andrew Cooper <andrew.cooper3@xxxxxxxxxx>, Wei Liu <wl@xxxxxxx>, <xen-devel@xxxxxxxxxxxxxxxxxxxx>
  • Delivery-date: Thu, 21 Jan 2021 17:27:28 +0000
  • Ironport-sdr: hdnBSiTDYhxK8ASIB/EWfaJSIJKeY8asVCJ/W6TEVb1t2TCZ9IYF+soJzoyyRCvsNrvvlGkinY P7rj3NH/KiRyOKTlP97lM6eof0yPJgNSIfTWD3N6zp//AaSuYT4iOfBUNC4+5W6AIkQoYHfokN nm+KKrs0PuGTdo45nPNuBD1oK8fwdaTsfAri1M0yulS+YYIvUNCUJ6QPCLH6MECYXuOmHYDuM9 F/BUR89NIUnYWfRGFwR7Ni0Kp2ziN/m5EcOEKL5Old3jboAOVNMP3pz+0gafJv13TQw4ioeEOy 1SE=
  • List-id: Xen developer discussion <xen-devel.lists.xenproject.org>

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@xxxxxxxxxx>
> 
> It's fine this way, so
> Reviewed-by: Jan Beulich <jbeulich@xxxxxxxx>
> 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.



 


Rackspace

Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.