| Message ID | 20150930173619.GA3826@pd.tnic (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Wed, Sep 30, 2015 at 07:36:19PM +0200, Borislav Petkov wrote: > Right, so this fixes the issue on my box, courtesy of Joerg. WE > basically don't disable the IRQ on MSI-enabled devices. The AMD IOMMU > uses a barebones PCI device but not a PCI driver, which would be an > overkill. Well, not only overkill, but actually harmful. As I just wrote to Jiang, a device can be forcibly unbound from its driver, which is something we don't want for the IOMMU. Joerg
On 2015/10/1 1:36, Borislav Petkov wrote: > On Thu, Oct 01, 2015 at 01:00:44AM +0800, Jiang Liu wrote: >> Thanks Joerg, that makes sense. If some driver tries to binding to >> the IOMMU device, it will trigger the scenario as you described. For >> example, Xen backend driver will try to probe all PCI devices if >> enabled. I will do more investigation tomorrow. > > Right, so this fixes the issue on my box, courtesy of Joerg. WE > basically don't disable the IRQ on MSI-enabled devices. The AMD IOMMU > uses a barebones PCI device but not a PCI driver, which would be an > overkill. > > --- > diff --git a/arch/x86/pci/common.c b/arch/x86/pci/common.c > index 09d3afc..29ec2eb 100644 > --- a/arch/x86/pci/common.c > +++ b/arch/x86/pci/common.c > @@ -674,12 +674,15 @@ int pcibios_add_device(struct pci_dev *dev) > > int pcibios_alloc_irq(struct pci_dev *dev) > { > + if (pci_dev_msi_enabled(dev)) > + return 0; We may return -EBUSY here to reject the probe operation. It doesn't make sense to continue the probe if MSI is already enabled, tt also helps to avoid calling pcibios_free_irq() in function pci_device_probe(). > + > return pcibios_enable_irq(dev); > } > > void pcibios_free_irq(struct pci_dev *dev) > { > - if (pcibios_disable_irq) > + if (!pci_dev_msi_enabled(dev) && pcibios_disable_irq) The above change is not needed, pcibios_disable_irq() will first check !pci_has_managed_irq(dev) before actually freeing PCI irq. pci_has_managed_irq(dev) only returns true if pcibios_alloc_irq() succeeds. So to summary, I think we only need following change to fix the regression: int pcibios_alloc_irq(struct pci_dev *dev) { + if (pci_dev_msi_enabled(dev)) + return -EBUSY; What do you think? Thanks! Gerry > pcibios_disable_irq(dev); > } > -- >
Hi Jiang, On Sat, Oct 03, 2015 at 03:36:35PM +0800, Jiang Liu wrote: > So to summary, I think we only need following change to fix the > regression: > int pcibios_alloc_irq(struct pci_dev *dev) > { > + if (pci_dev_msi_enabled(dev)) > + return -EBUSY; > > What do you think? Yes, that works too and has the added benefit that no driver can attach to the iommu device and get in the way of the driver. Will you send the patch for this change or should I do it? Joerg
On 2015/10/5 18:03, Joerg Roedel wrote: > Hi Jiang, > > On Sat, Oct 03, 2015 at 03:36:35PM +0800, Jiang Liu wrote: >> So to summary, I think we only need following change to fix the >> regression: >> int pcibios_alloc_irq(struct pci_dev *dev) >> { >> + if (pci_dev_msi_enabled(dev)) >> + return -EBUSY; >> >> What do you think? > > Yes, that works too and has the added benefit that no driver can attach > to the iommu device and get in the way of the driver. > > Will you send the patch for this change or should I do it? Hi Joerg, We are on leave for Chinese National Holiday and has limited access to my working environment. It would be appreciated if you could help to send out a patch for it. Otherwise I will send out a patch within 2-3 days. Thanks! Gerry
On Tue, Oct 06, 2015 at 09:13:11PM +0800, Jiang Liu wrote: > We are on leave for Chinese National Holiday and has limited > access to my working environment. It would be appreciated if you could > help to send out a patch for it. Otherwise I will send out a patch > within 2-3 days. Okay, I just sent the patch. Joerg
diff --git a/arch/x86/pci/common.c b/arch/x86/pci/common.c index 09d3afc..29ec2eb 100644 --- a/arch/x86/pci/common.c +++ b/arch/x86/pci/common.c @@ -674,12 +674,15 @@ int pcibios_add_device(struct pci_dev *dev) int pcibios_alloc_irq(struct pci_dev *dev) { + if (pci_dev_msi_enabled(dev)) + return 0; + return pcibios_enable_irq(dev); } void pcibios_free_irq(struct pci_dev *dev) { - if (pcibios_disable_irq) + if (!pci_dev_msi_enabled(dev) && pcibios_disable_irq) pcibios_disable_irq(dev); } --