Re: [PATCH] genirq/msi: Make sure PCI MSIs are activated early

From: Bjorn Helgaas
Date: Mon Jul 25 2016 - 10:47:20 EST

On Mon, Jul 25, 2016 at 09:45:13AM +0200, Thomas Gleixner wrote:
> On Fri, 22 Jul 2016, Bjorn Helgaas wrote:
> > On Wed, Jul 13, 2016 at 05:18:33PM +0100, Marc Zyngier wrote:
> > > and it turns out that end-points are allowed to latch the content
> > > of the MSI configuration registers as soon as MSIs are enabled.
> > > In Bharat's case, the end-point ends up using whatever was there
> > > already, which is not what you want.
> > >
> > > In order to make things converge, we introduce a new MSI domain
> > > flag (MSI_FLAG_ACTIVATE_EARLY) that is unconditionally set for
> > > PCI/MSI. When set, this flag forces the programming of the end-point
> > > as soon as the MSIs are allocated.
> > >
> > > A consequence of this is that we have an extra activate in
> > > irq_startup, but that should be without much consequence.
> > >
> > > Reported-by: Bharat Kumar Gogada <bharat.kumar.gogada@xxxxxxxxxx>
> > > Tested-by: Bharat Kumar Gogada <bharat.kumar.gogada@xxxxxxxxxx>
> > > Signed-off-by: Marc Zyngier <marc.zyngier@xxxxxxx>
> >
> > Acked-by: Bjorn Helgaas <bhelgaas@xxxxxxxxxx>
> >
> > Thomas, let me know if you'd like me to take this. It looks like the
> > real smarts here are in kernel/irq, so I assume you'll take it unless
> > I hear otherwise.
> I'll take it. Though I have second thoughts about the whole issue.
> We deliberately made the allocation sequence of interrupts in a way that we
> can easily rollback in case of failure.
> We achieved that by activating the interrupts only at request time and not
> somewhere in the middle of the allocation sequence. That makes the whole
> hierarchical allocation more robust and avoids complex rollbacks.
> Now that new flag is basically torpedoing that approach.
> What I really wonder is why that is only an issue with that particular xilinx
> hardware/IP block. I'm aware that up to PCI 2.3 the mask bit for MSI
> interrupts is optional or in really old versions not even specified. So only
> if that mask bit is missing the above described issue can happen.
> If not, then we might have a general issue that we don't mask the entry before
> we call pci_msi_set_enable().

Good question. I haven't followed this thread in detail, so my ack
meant "I'm OK with this if you are," not "I've reviewed this and
think it's great."

I thought the original issue [1] was that PCI_MSI_FLAGS_ENABLE was being
written before PCI_MSI_ADDRESS_LO. That doesn't sound like a good
idea to me.

I don't understand the whole flow. Here's what I've gleaned so far:

domain = pci_msi_get_domain(dev)
if (domain)
# this seems like the problem case
pci_msi_domain_alloc_irqs(domain, dev, nvec)
# this case apparently works fine
for_each_pci_msi_entry(entry, dev)
xilinx_pcie_msi_setup_irq # xilinx_pcie_msi_chip.setup_irq
pci_msi_set_enable(dev, 1)
pci_write_config_word(PCI_MSI_FLAGS, PCI_MSI_FLAGS_ENABLE)

I assume the problem is that in the MSI domain case, we don't call the
chip->setup_irq method until later. I gave up trying to figure out
where that happens. Is it something like the following?

?? chip->setup_irq ??

That does seem like a problem. Maybe it would be better to delay
setting PCI_MSI_FLAGS_ENABLE until after the MSI address & data bits
have been set?