linux-kernel - Re: MSI irqchip configured as IRQCHIP_ONESHOT

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <87pnfjwxtx.fsf@nanos.tec.linutronix.de>
Date:   Thu, 16 Jan 2020 10:32:42 +0100
From:   Thomas Gleixner <tglx@...utronix.de>
To:     Ramon Fried <rfried.dev@...il.com>
Cc:     hkallweit1@...il.com, Bjorn Helgaas <bhelgaas@...gle.com>,
        maz@...nel.org, lorenzo.pieralisi@....com,
        linux-pci@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: MSI irqchip configured as IRQCHIP_ONESHOT_SAFE causes spurious IRQs

Ramon,

Ramon Fried <rfried.dev@...il.com> writes:
> On Thu, Jan 16, 2020 at 3:39 AM Thomas Gleixner <tglx@...utronix.de> wrote:
>> Ramon Fried <rfried.dev@...il.com> writes:
>
> Basically, 32 MSI vectors are represented by a single GIC irq.
> There's a status registers which every bit correspond to an MSI vector, and
> individual MSI needs to be acked on that registers. in any case where
> there's asserted bit the GIC IRQ level is high.

Which is not that bad. 

>> > It's configured with handle_level_irq() as the GIC is level IRQ.
>>
>> Which is completely bonkers. MSI has edge semantics and sharing an
>> interrupt line for edge type interrupts is broken by design, unless the
>> hardware which handles the incoming MSIs and forwards them to the level
>> type interrupt line is designed properly and the driver does the right
>> thing.
>
> Yes, the design of the HW is sort of broken. I concur.

As you describe it, it's not that bad.

>> > The ack callback acks the GIC irq.  the mask/unmask calls
>> > pci_msi_mask_irq() / pci_msi_unmask_irq()
>>
>> What? How is that supposed to work with multiple MSIs?
> Acking is per MSI vector as I described above, so it should work.

No. This is the wrong approach. Lets look at the hardware:

| GIC   line X |------| MSI block | <--- Messages from devices

The MSI block latches the incoming message up to the point where it is
acknowledged in the MSI block. This makes sure that the level semantics
of the GIC are met.

So from a software perspective you want to do something like this:

gic_irq_handler()
{
   mask_ack(gic_irqX);

   pending = read(msi_status);
   for_each_bit(bit, pending) {
       ack(msi_status, bit);  // This clears the latch in the MSI block
       handle_irq(irqof(bit));
   }
   unmask(gic_irqX);
}

And that works perfectly correct without masking the MSI interrupt at
the PCI level for a threaded handler simply because the PCI device will
not send another interrupt until the previous one has been handled by
the driver unless the PCI device is broken.

If that's the case, then you have to work around that at the device
driver level, not at the irq chip level, by installing a primary handler
which quiesces the device (not the MSI part).

Thanks,

        tglx