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

Re: [PATCH v2 3/4] x86/vpic: issue dpci EOI for cleared pins at ICW1


  • To: Jan Beulich <jbeulich@xxxxxxxx>
  • From: Roger Pau Monné <roger.pau@xxxxxxxxxx>
  • Date: Fri, 22 Jan 2021 10:53: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=StaYakex4ChFniSU5+J55tJVinzfJJSjV/QM0hY5Ctc=; b=KXKoihm/oFo7iiZBLtZoO7Bx17bzqDgi4KgkIXQOXy2zZOPKHTjquvvB7s5P81yVYQ76wl6EpBniclPw13bgaFcZVZv6HRlElgqm9A4Cz5slfGLB9wBeG9cvQglOxUQebe0Xdby98Rk+83FGyCdRnJiRbyPYHbDF/789GHTgX1Gh64eHAVqUG/pkIROwCTc/Gk9Ol/M1R7deqfqXi7n48hwgJPAy8LvfavdKLZ4vY9f/3H6Mu2XhlfCFSZstvWGKOfo9IQo67/AjkzuoMY4D0aKmQ3N200PCEnbTJ6Yv8bN9ZE67tgfxLvT9y4GKvZE4Fz+EGte8KEXfncJD0vrS8g==
  • Arc-seal: i=1; a=rsa-sha256; s=arcselector9901; d=microsoft.com; cv=none; b=J6JHjZoUPX5yjfKtqTkQmqsWN+fIqGA2MoH6S7ODGAMJHdl2Sjw7tHhwGXx5GGQmDZNne3PiWkxZRGu4TZjHN3Z+w21E9nbVQgvJUWq5cd2B9rVN2U5V8s/Z7RdyM+GfUiX8Bhp13ZGmeRfl7VFwV5GGTjlVMyNfJracbIq0fb69av0U8LnYh4STPnBLb9XLzi+VTJwiqOogp3YDy+jbQxafqQRR1zVjrT4wl/0RswSI2IMiQNrIgRcESXoQLHfPR64O9OM16vzsc7tlK/Y0GrUDg12qLOPkmg1pr5J7LweJvUTXTb8Ic3i3sBPZleSlCiEQdyB/BHWUxcw2XshwWQ==
  • Authentication-results: esa2.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: Fri, 22 Jan 2021 09:54:20 +0000
  • Ironport-sdr: hPSdI10242pBuke8azkkDUqtxupHxuqXPEGpUJyDnzL1h5ZuadHaY72v4pZ9HagJ2cLJqj1sti HP7UyauyIYo9Umiwvkx8sj2KHSjXK32K3f0vSu3Y5GQg54/+tWP9NbaWyh3W+u8mLBl8HV2P4O Y8q+iK9sk20rrOpMPxdJopfEQyh9L6sXuZ6S6jmHbu8qIPunnatH+IAg8PVmbiHoSUjmv2jlHH YreCAqOeEcQQ9CSK7vwDKW+2wucEoRrv4TeGUgjFFJETjXjx6ka7++St/7Wglw2q1ARwn/IMaA McQ=
  • List-id: Xen developer discussion <xen-devel.lists.xenproject.org>

On Fri, Jan 22, 2021 at 10:02:15AM +0100, Jan Beulich wrote:
> On 15.01.2021 15:28, Roger Pau Monne wrote:
> > When pins are cleared from either ISR or IRR as part of the
> > initialization sequence forward the clearing of those pins to the dpci
> > EOI handler, as it is equivalent to an EOI. Not doing so can bring the
> > interrupt controller state out of sync with the dpci handling logic,
> > that expects a notification when a pin has been EOI'ed.
> 
> The question though is what this clearing of ISR and some of
> IRR during ICW1 is based upon: Going through various manuals
> (up-to-date from Nov 2020, intermediate, and all the way
> through to an old hard copy from 1993) I can't find a single
> mention of ICW1 having any effect on ISR or IRR, despite both
> soft copy ones being detailed about other state getting
> changed.
> 
> There is "Following initialization, an interrupt request (IRQ)
> input must make a low-to-high transition to generate an
> interrupt", but I'm afraid this can be read to mean different
> things. And if it was meant to describe an effect on ISR
> and/or IRR, it would imo be in conflict with us keeping IRR
> bits of level triggered interrupts.

I have an old copy of the 8259A spec, and it does mention the same
quote that you have above. I also wondered while I was adjusting this
code whether what we do is fine. I have to admit I haven't considered
changing this logic much because I don't have an effective way to test
it.

I've also taken a look at what others do, QEMU for example will do
exactly the same logic as Xen during ICW1, bhyve OTOH will fully zero
IRR and leave ISR as is.

Doing a bit of archaeology in QEMU I've found the following change:

commit aa24822bdc7c4e74afbc6fa1324b01cf067da7cb
Author: Jan Kiszka <jan.kiszka@xxxxxxxxxxx>
Date:   Tue Jan 24 16:29:29 2012 +0100

    i8259: Do not clear level-triggered lines in IRR on init

    When an input line is handled as level-triggered, it will immediately
    raise an IRQ on the output of a PIC again that goes through an init
    reset. So only clear the edge-triggered inputs from IRR in that
    scenario.

    Signed-off-by: Jan Kiszka <jan.kiszka@xxxxxxxxxxx>
    Signed-off-by: Anthony Liguori <aliguori@xxxxxxxxxx>

Which seems to point out there's a reasoning behind what it's
currently implemented. This seems to be against the spec as there's no
low-to-high transition?

> > @@ -217,6 +219,24 @@ static void vpic_ioport_write(
> >              }
> >  
> >              vpic->init_state = ((val & 3) << 2) | 1;
> > +            vpic_update_int_output(vpic);
> > +            vpic_unlock(vpic);
> > +
> > +            /*
> > +             * Forward the EOI of any pending or in service interrupt that 
> > has
> > +             * been cleared from IRR or ISR, or else the dpci logic will 
> > get
> > +             * out of sync with the state of the interrupt controller.
> > +             */
> > +            while ( pending )
> > +            {
> > +                unsigned int pin = __scanbit(pending, 8);
> > +
> > +                ASSERT(pin < 8);
> > +                hvm_dpci_eoi(current->domain,
> > +                             hvm_isa_irq_to_gsi((addr >> 7) ? (pin | 8) : 
> > pin));
> > +                __clear_bit(pin, &pending);
> > +            }
> > +            goto unmask;
> 
> A similar consideration applies here (albeit just as an
> observation, as being orthogonal to your change): A PIC
> becomes ready for handling interrupts only at the end of the
> ICWx sequence. Hence it would seem to me that invoking
> pt_may_unmask_irq()

Right, it might be best to force unmask = 1 when init_state gets set
to 0. A spurious call to pt_may_unmask_irq won't be harmful anyway,
even if no pins have been actually unmasked.

> (maybe also vpic_update_int_output()) at
> ICW1 is premature. To me this seems particularly relevant
> considering that ICW1 clears IMR, and hence an interrupt
> becoming pending between ICW1 and ICW2 wouldn't know which
> vector to use.
> 
> Or maybe on that side of things, vpic_update_int_output()
> should really do
> 
>     if ( vpic->int_output == (!vpic->init_state && irq >= 0) )
>         return;
> 
>     /* INT line transition L->H or H->L. */
>     vpic->int_output = !vpic->init_state && !vpic->int_output;
> 
> ?

So to force int_output = false when in init state. Seems reasonable, I
can implement this either as a pre-patch or a followup, but again I'm
not sure I have means to properly test it.

Thanks, Roger.



 


Rackspace

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