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

Re: [PATCH v2 2/4] x86/vioapic: issue EOI to dpci when switching pin to edge trigger mode


  • To: Jan Beulich <jbeulich@xxxxxxxx>
  • From: Roger Pau Monné <roger.pau@xxxxxxxxxx>
  • Date: Thu, 21 Jan 2021 18:45:54 +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=ChX1/SvbxOz/orZGYfaGZm60eftD+Fv4pS0hWjcpRpc=; b=T93lprlBWEcXRXrYRFqzyvQEVerw56tb8zkBqRr7QheGDvDTWU1uq6i18VdcGTY6yy+5NwmiLF6LXvHcVUfjapzqErABh3prk7FvR83AGMOoGuYyjem5yR00C19lkU/B5K9wTTN6v67BCqOijmiM3aUksfnk6DOEIbzxD+Wg7GF8WNmHPHFlQDOZzgEqBhyiD/6VXgv5wv2X0IexU57Ov42CLCeurSdMXzh4Pzcmkkb1h/gZX7PgoWvxY2Df2X/5MKXOb0aEi4wcfRsWAYI996BfnojCqq+Oyh8bNFqfd6D8vmMEelHDqg3nvnj5GiR8ilckFkfPJHlSrKfYbivGWA==
  • Arc-seal: i=1; a=rsa-sha256; s=arcselector9901; d=microsoft.com; cv=none; b=TfkLZ2xQ2/Cpr5lmU95x5HEqQDT1VKvxgOsCVWNf9enn6Z+Ve0jRff4Pke0fM4hdtdMtx3onqptqpgYVn0Xkz/+hKBtXCnAdd0WZy0z4JmzbFhTZmNxu7Bt/aTqb7TaFoFbOBhiGvU4VvLJ+nrO1OSNZ6OLqMxEz3Gfqfvq7C3AD0dTR/N2BUVrzfC8tyaxhRUu3XA7y1tLETj+CFE3XTzQP27KnhDkg2a1Xw6UuvUl6AR13OKr0m3p2aiht46QkE0EijO9xa72u4RuYRK1kEiCxLWCmWBaGM2Iwcn4gbGlG67hrblTROwLh5vF4rp/QGWyBfF1TXVrVPU+4GSo/+Q==
  • Authentication-results: esa3.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:46:15 +0000
  • Ironport-sdr: 2YKz+afmc0wiNkcd48AftRlI8n5xxblBJZUOm1xCn2+HkZnwPfgEPxWig1Po3g/nASm6rqEc4r umIJpBdONue0E7Sci3Q6PvObAnAcJUqkLPp9r9qeooPOKgdkKW6V0RNLt9sBLUH4+HwVRinx74 WL23QrhuhFuk8HLFe/jaDwgs1UIoZAnFOh7Dj5V2ET7l4kA7Zrtwd8utNql2ka+uOcklfizK+J dmiDtopd1QCHLiok3vS+2CXgdSa3xGrBUPbvP5/+EMpon/aTjGKg2a7XB28FchMJJwbo7hAFID zbI=
  • List-id: Xen developer discussion <xen-devel.lists.xenproject.org>

On Thu, Jan 21, 2021 at 05:23:04PM +0100, Jan Beulich wrote:
> On 15.01.2021 15:28, Roger Pau Monne wrote:
> > --- a/xen/arch/x86/hvm/vioapic.c
> > +++ b/xen/arch/x86/hvm/vioapic.c
> > @@ -268,6 +268,17 @@ static void vioapic_write_redirent(
> >  
> >      spin_unlock(&d->arch.hvm.irq_lock);
> >  
> > +    if ( ent.fields.trig_mode == VIOAPIC_EDGE_TRIG &&
> > +         ent.fields.remote_irr && is_iommu_enabled(d) )
> > +            /*
> > +             * Since IRR has been cleared and further interrupts can be
> > +             * injected also attempt to deassert any virtual line of passed
> > +             * through devices using this pin. Switching a pin from level 
> > to
> > +             * trigger mode can be used as a way to EOI an interrupt at the
> > +             * IO-APIC level.
> > +             */
> > +            hvm_dpci_eoi(d, gsi);
> > +
> >      if ( is_hardware_domain(d) && unmasked )
> >      {
> >          /*
> 
> I assume in the comment you mean "... from level to edge
> mode ...".

Yes, that's right, I completely missed it, sorry.

> But this isn't reflected in the if() you add -
> you do the same also when the mode doesn't change. Or do
> you build on the assumption that ent.fields.remote_irr can
> only be set if the prior mode was "level" (in which case
> an assertion may be warranted, as I don't think this is
> overly obvious)?

Yes, IRR is only set for level triggered interrupts, so it's indeed
build on the assumption that a pin can only have had IRR set when in
edge mode when it's being switched from level to edge.

I can add an assertion.

> Also, looking at this code, is it correct to trigger an IRQ
> upon the guest writing the upper half of an unmasked RTE
> with remote_irr clear? I'd assume this needs to be strictly
> limited to a 1->0 transition of the mask bit. If other code
> indeed guarantees this in all cases, perhaps another place
> where an assertion would be warranted?

Indeed. I don't think it should be possible for a write to the upper
half to trigger the injection of an interrupt, as having
gsi_assert_count > 0 would imply that either IRR is already set, or
that the pin is masked when processing an upper write.

I can add that a pre-patch if you agree.

In fact we could almost short-circuit the logic after the *pent = ent;
line for upper writes if it wasn't for the call to
vlapic_adjust_i8259_target, the rest of the code there shouldn't
matter for upper writes. And the i8259 target logic that we have is
very dodgy I would say. I have plans to fix it at some point, but
that requires fixing the virtual periodic timers logic first, which I
didn't get around to re-posting.

Thanks, Roger.



 


Rackspace

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