Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1450544 > unrolled thread

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

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: [PATCH] genirq/msi: Make sure PCI MSIs are activated early Thomas Gleixner <tglx@linutronix.de> - 2016-07-26 13:50 +0200
    Re: [PATCH] genirq/msi: Make sure PCI MSIs are activated early Thomas Gleixner <tglx@linutronix.de> - 2016-07-26 15:10 +0200
      Re: [PATCH] genirq/msi: Make sure PCI MSIs are activated early Thomas Gleixner <tglx@linutronix.de> - 2016-07-26 16:10 +0200
        Re: [PATCH] genirq/msi: Make sure PCI MSIs are activated early Thomas Gleixner <tglx@linutronix.de> - 2016-07-28 17:10 +0200
          Re: [PATCH] genirq/msi: Make sure PCI MSIs are activated early Bjorn Helgaas <helgaas@kernel.org> - 2016-07-28 18:50 +0200

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

On Mon, 25 Jul 2016, Bjorn Helgaas wrote:
> On Mon, Jul 25, 2016 at 09:45:13AM +0200, Thomas Gleixner wrote:
> 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.

Well. That's only a problem if the PCI device does not support masking. But
yes, we missed that case back then.
 
> 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?

I thought about that, but that gets ugly pretty fast. Here is an alternative
solution.

I think that's the proper place to do it _AFTER_ the hierarchical allocation
took place. On x86 Marc's ACTIVATE_EARLY flag would not work because the
message is not yet ready to be assembled.

Thanks,

	tglx
---
diff --git a/drivers/pci/msi.c b/drivers/pci/msi.c
index a080f4496fe2..142341f8331b 100644
--- a/drivers/pci/msi.c
+++ b/drivers/pci/msi.c
@@ -645,6 +645,15 @@ static int msi_capability_init(struct pci_dev *dev, int nvec)
 		return ret;
 	}
 
+	/*
+	 * The mask can be ignored and PCI 2.3 does not specify mask bits for
+	 * each MSI interrupt. So in case of hierarchical irqdomains we need
+	 * to make sure that if masking is not available that the msi message
+	 * is written prior to setting the MSI enable bit in the device.
+	 */
+	if (pci_msi_ignore_mask || !entry->msi_attrib.maskbit)
+		irq_domain_activate_irq(irq_get_irq_data(entry->irq));
+
 	/* Set MSI enabled bits	 */
 	pci_intx_for_msi(dev, 0);
 	pci_msi_set_enable(dev, 1);

[toc] | [next] | [standalone]

#1450597

On Tue, 26 Jul 2016, Thomas Gleixner wrote:
> On Mon, 25 Jul 2016, Bjorn Helgaas wrote:
> > On Mon, Jul 25, 2016 at 09:45:13AM +0200, Thomas Gleixner wrote:
> > 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.
> 
> Well. That's only a problem if the PCI device does not support masking. But
> yes, we missed that case back then.
>  
> > 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?
> 
> I thought about that, but that gets ugly pretty fast. Here is an alternative
> solution.
> 
> I think that's the proper place to do it _AFTER_ the hierarchical allocation
> took place. On x86 Marc's ACTIVATE_EARLY flag would not work because the
> message is not yet ready to be assembled.

Actually it works, because the MSI domain is the last one which is running the
allocation function. So everything else is initialized already.

I'll take Marc's patch with some additional commentry as it turned out to be a
workaround for the reported VMware issues with PCI/MSI-X pass through.

Thanks,

	tglx

[toc] | [prev] | [next] | [standalone]

#1450611

On Tue, 26 Jul 2016, Thomas Gleixner wrote:
> On Tue, 26 Jul 2016, Thomas Gleixner wrote:
> > On Mon, 25 Jul 2016, Bjorn Helgaas wrote:
> > > On Mon, Jul 25, 2016 at 09:45:13AM +0200, Thomas Gleixner wrote:
> > > 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.
> > 
> > Well. That's only a problem if the PCI device does not support masking. But
> > yes, we missed that case back then.
> >  
> > > 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?
> > 
> > I thought about that, but that gets ugly pretty fast. Here is an alternative
> > solution.
> > 
> > I think that's the proper place to do it _AFTER_ the hierarchical allocation
> > took place. On x86 Marc's ACTIVATE_EARLY flag would not work because the
> > message is not yet ready to be assembled.
> 
> Actually it works, because the MSI domain is the last one which is running the
> allocation function. So everything else is initialized already.
> 
> I'll take Marc's patch with some additional commentry as it turned out to be a
> workaround for the reported VMware issues with PCI/MSI-X pass through.

Now I digged a little bit deeper into all that PCI/MSI maze.

When a interrupt is freed, then we write the msi message to 0, but the
PCI_MSI_FLAGS_ENABLE flag is still set. That makes me wonder ...

Thanks,

	tglx

[toc] | [prev] | [next] | [standalone]

#1451972

On Tue, 26 Jul 2016, Thomas Gleixner wrote:
> On Tue, 26 Jul 2016, Thomas Gleixner wrote:
> > On Tue, 26 Jul 2016, Thomas Gleixner wrote:
> > > On Mon, 25 Jul 2016, Bjorn Helgaas wrote:
> > > > On Mon, Jul 25, 2016 at 09:45:13AM +0200, Thomas Gleixner wrote:
> > > > 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.
> > > 
> > > Well. That's only a problem if the PCI device does not support masking. But
> > > yes, we missed that case back then.
> > >  
> > > > 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?
> > > 
> > > I thought about that, but that gets ugly pretty fast. Here is an alternative
> > > solution.
> > > 
> > > I think that's the proper place to do it _AFTER_ the hierarchical allocation
> > > took place. On x86 Marc's ACTIVATE_EARLY flag would not work because the
> > > message is not yet ready to be assembled.
> > 
> > Actually it works, because the MSI domain is the last one which is running the
> > allocation function. So everything else is initialized already.
> > 
> > I'll take Marc's patch with some additional commentry as it turned out to be a
> > workaround for the reported VMware issues with PCI/MSI-X pass through.
> 
> Now I digged a little bit deeper into all that PCI/MSI maze.
> 
> When a interrupt is freed, then we write the msi message to 0, but the
> PCI_MSI_FLAGS_ENABLE flag is still set. That makes me wonder ...

Bjorn, any opinion on that?

Thanks,

	tglx

[toc] | [prev] | [next] | [standalone]

#1452011

On Thu, Jul 28, 2016 at 05:03:30PM +0200, Thomas Gleixner wrote:
> On Tue, 26 Jul 2016, Thomas Gleixner wrote:
> > On Tue, 26 Jul 2016, Thomas Gleixner wrote:
> > > On Tue, 26 Jul 2016, Thomas Gleixner wrote:
> > > > On Mon, 25 Jul 2016, Bjorn Helgaas wrote:
> > > > > On Mon, Jul 25, 2016 at 09:45:13AM +0200, Thomas Gleixner wrote:
> > > > > 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.
> > > > 
> > > > Well. That's only a problem if the PCI device does not support masking. But
> > > > yes, we missed that case back then.
> > > >  
> > > > > 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?
> > > > 
> > > > I thought about that, but that gets ugly pretty fast. Here is an alternative
> > > > solution.
> > > > 
> > > > I think that's the proper place to do it _AFTER_ the hierarchical allocation
> > > > took place. On x86 Marc's ACTIVATE_EARLY flag would not work because the
> > > > message is not yet ready to be assembled.
> > > 
> > > Actually it works, because the MSI domain is the last one which is running the
> > > allocation function. So everything else is initialized already.
> > > 
> > > I'll take Marc's patch with some additional commentry as it turned out to be a
> > > workaround for the reported VMware issues with PCI/MSI-X pass through.
> > 
> > Now I digged a little bit deeper into all that PCI/MSI maze.
> > 
> > When a interrupt is freed, then we write the msi message to 0, but the
> > PCI_MSI_FLAGS_ENABLE flag is still set. That makes me wonder ...
> 
> Bjorn, any opinion on that?

I assume you mean we write 0 to PCI_MSI_ADDRESS_LO, PCI_MSI_DATA_32,
and similar registers in the MSI Capability structure.

It doesn't sound safe to me to do that while PCI_MSI_FLAGS_ENABLE is
still set.  I don't see anything in the spec that constrains when a
device latches the values from those registers.  It seems legal to do
it on PCI_MSI_FLAGS_ENABLE transitions, but it also seems legal to do
it whenever the device needs to signal an interrupt.

If a device does the latter, it seems like clearing PCI_MSI_ADDRESS_LO
while PCI_MSI_FLAGS_ENABLE is set could lead to stray DMA writes if
the device for some reason signals an interrupt later.

Bjorn

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web