x86/PCI: Don't alloc pcibios-irq when MSI is enabled
Joerg Roedel
maintainerPATCH 1/1 x86/PCI: Don't alloc pcibios-irq when MSI is enabled
From: Joerg Roedel <[email protected]>
The pcibios-irq and MSI both use dev->irq to store the IRQ number. While the MSI code checks for that and frees the pcibios-irq before overwriting dev->irq, the pcibios_alloc_irq function does not.
Usually this is not a problem, as the pcibios-irq is allocated before probe time of the device and the MSI irq is allocted from the drivers probe path.
But there are PCI devices handled by the core kernel and not by a standard pci driver, like the AMD IOMMU for example. For the AMD IOMMU a normal pci device driver does not make sense, because a driver can be forcibly unbound from its device, which is not a good idea for an IOMMU.
Nevertheless the PCI core code tries to match the PCI device implementing the AMD IOMMU against drivers, and allocates/frees a pcibios IRQ every time it tries out a new driver. This overwrites the dev->irq field set by pci_enable_msi() and sets it to 0 in the end (because the probe fails and the pcibios-irq is freed again).
On suspend/resume this breaks the kernel, because the irq descriptor for irq 0 is NULL.
Fix this by not allocating a pcibios-irq when MSI is already active. This also has the benefit, that a device claimed by the core kernel can not be probed by a pci driver later.
--- arch/x86/pci/common.c | 8 ++++++++ 1 file changed, 8 insertions(+)
arch/x86/pci/common.c +8 -0
@@ -675,6 +675,14 @@ int pcibios_add_device(struct pci_dev *dev)int pcibios_alloc_irq (struct pci_dev *dev) { +/* +* If the PCI device was already claimed by core code and has +* MSI enabled, probing of the pcibios irq will overwrite +* dev->irq. So bail out if MSI is already enabled. +*/ +if (pci_dev_msi_enabled (dev)) +return -EBUSY ; +return pcibios_enable_irq (dev); }
Signature · 1 line
1.9.1
Quoted 27 lines · Joerg Roedel
From: Joerg Roedel <[email protected]>
The pcibios-irq and MSI both use
dev->irqto store the IRQ number. While the MSI code checks for that and frees the pcibios-irq before overwritingdev->irq, thepcibios_alloc_irqfunction does not.Usually this is not a problem, as the pcibios-irq is allocated before probe time of the device and the MSI irq is allocted from the drivers probe path.
But there are PCI devices handled by the core kernel and not by a standard pci driver, like the AMD IOMMU for example. For the AMD IOMMU a normal pci device driver does not make sense, because a driver can be forcibly unbound from its device, which is not a good idea for an IOMMU.
Nevertheless the PCI core code tries to match the PCI device implementing the AMD IOMMU against drivers, and allocates/frees a pcibios IRQ every time it tries out a new driver. This overwrites the
dev->irqfield set bypci_enable_msi()and sets it to 0 in the end (because the probe fails and the pcibios-irq is freed again).On suspend/resume this breaks the kernel, because the irq descriptor for irq 0 is NULL.
Quoted 8 lines
Fix this by not allocating a pcibios-irq when MSI is already active. This also has the benefit, that a device claimed by the core kernel can not be probed by a pci driver later.
Cc Jiang Liu <[email protected]>Reported-by Borislav Petkov <[email protected]>Signed-off-by Joerg Roedel <[email protected]>
Quoted 27 lines
--- arch/x86/pci/common.c | 8 ++++++++ 1 file changed, 8 insertions(+)arch/x86/pci/common.c +8
-0@@ -675,6 +675,14 @@ int pcibios_add_device(struct pci_dev *dev)int pcibios_alloc_irq (struct pci_dev *dev) { +/* +* If the PCI device was already claimed by core code and has +* MSI enabled, probing of the pcibios irq will overwrite +* dev->irq. So bail out if MSI is already enabled. +*/ +if (pci_dev_msi_enabled (dev)) +return -EBUSY ; +return pcibios_enable_irq (dev); }Signature · 1 line
1.9.1
v4 cover Prevent binding PCI drivers to PCI devices used by non-pci drivers
In reply to Joerg RoedelHi Joerg,
I prepared this patchset yesterday but forgot to send them out.
I think it still worth sending out for review, so here we go:)
---
Commit 991de2e59090 ("PCI, x86: Implement pcibios_alloc_irq() and pcibios_free_irq()") breaks suspend/resume on some AMD platforms. The root cause is:
1) AMD IOMMU drivers enables MSI on IOMMU PCI devices at early boot stage
2) PCI driver binding code tries to allocate/free PCI legacy IRQ when binding other PCI drivers to IOMMU PCI devices at later boot stage, and breaks PCI MSI allocation information.
3) System resume breaks when restoring PCI MSI state using the damaged data.
We have tried on solution to detect that a PCI is in use by IOMMU driver by checking pci_msi_enabled(). But that's too specific, actually we should prevent binding PCI drivers to PCI devices used by non-PCI drivers.
Fortunately, we could prevent binding PCI drivers to PCI devices by setting pci_dev->match_driver to false. If needed, we could implement a helper function to manipulate pci_dev->match_driver.
Jiang Liu (2):
iommu/amd: Prevent binding other PCI drivers to IOMMU PCI devices ACPI, PCI: Prevent binding other PCI drivers to IOAPIC PCI devices
drivers/acpi/ioapic.c | 7 +++++-- drivers/iommu/amd_iommu_init.c | 3 +++ 2 files changed, 8 insertions(+), 2 deletions(-)
Signature · 1 line
1.7.10.4
PATCH v4 1/2 iommu/amd: Prevent binding other PCI drivers to IOMMU PCI devices
AMD IOMMU driver makes use of IOMMU PCI devices, so prevent binding other PCI drivers to IOMMU PCI devices.
This fixes a bug reported by Boris that system suspend/resume gets broken on AMD platforms. For more information, please refer to:
https://lkml.org/lkml/2015/9/26/89
--- drivers/iommu/amd_iommu_init.c | 3 +++ 1 file changed, 3 insertions(+)
drivers/iommu/amd_iommu_init.c +3 -0
@@ -1256,6 +1256,9 @@ static int iommu_init_pci(struct amd_iommu *iommu)if (!iommu->dev)return -ENODEV ; +/* Prevent binding other PCI device drivers to IOMMU devices */ + iommu->dev->match_driver = false; +pci_read_config_dword (iommu->dev, cap_ptr +MMIO_CAP_HDR_OFFSET , &iommu->cap); pci_read_config_dword(iommu->dev, cap_ptr + MMIO_RANGE_OFFSET,
Signature · 1 line
1.7.10.4
Re: [Bugfix v4 1/2] iommu/amd: Prevent binding other PCI drivers to IOMMU PCI devices
Quoted 10 lines · Jiang Liu
AMD IOMMU driver makes use of IOMMU PCI devices, so prevent binding other PCI drivers to IOMMU PCI devices.
This fixes a bug reported by Boris that system suspend/resume gets broken on AMD platforms. For more information, please refer to:
https://lkml.org/lkml/2015/9/26/89Fixes 991de2e59090 ("PCI, x86: Implement pcibios_alloc_irq() and pcibios_free_irq()")Signed-off-by Jiang Liu <[email protected]>Cc Borislav Petkov <[email protected]>
Applied to iommu/fixes, thanks Jiang.
PATCH v4 2/2 ACPI, PCI: Prevent binding other PCI drivers to IOAPIC PCI devices
In reply to Jiang LiuThe ACPI IOAPIC driver makes use of IOAPIC PCI devices, so prevent binding other PCI drivers to IOAPIC PCI devices used by ACPI IOAPIC driver.
--- drivers/acpi/ioapic.c | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-)
drivers/acpi/ioapic.c +5 -2
@@ -162,12 +162,14 @@ done:list_add (&ioapic->list, &ioapic_list);mutex_unlock (&ioapic_list_lock); -if (dev) +if (dev) { + dev->match_driver = false;dev_info (&dev->dev,"%s at %pR, GSI %u\n" , type, res, (u32 )gsi_base); -else + }else {acpi_handle_info (handle,"%s at %pR, GSI %u\n" , type, res, (u32 )gsi_base); + }return AE_OK ;
@@ -216,6 +218,7 @@ int acpi_ioapic_remove(struct acpi_pci_root *root)if (ioapic->pdev) {pci_release_region (ioapic->pdev,0 );pci_disable_device (ioapic->pdev); + ioapic->pdev->match_driver = true;pci_dev_put (ioapic->pdev); }else if (ioapic->res.flags && ioapic->res.parent) {release_resource (&ioapic->res);
Signature · 1 line
1.7.10.4
Re: [Bugfix v4 2/2] ACPI, PCI: Prevent binding other PCI drivers to IOAPIC PCI devices
Quoted 8 lines · Jiang Liu
The ACPI IOAPIC driver makes use of IOAPIC PCI devices, so prevent binding other PCI drivers to IOAPIC PCI devices used by ACPI IOAPIC driver.
Signed-off-by Jiang Liu <[email protected]>--- drivers/acpi/ioapic.c | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-)
Quoted 27 lines · Joerg Roedel
From: Joerg Roedel <[email protected]>
The pcibios-irq and MSI both use
dev->irqto store the IRQ number. While the MSI code checks for that and frees the pcibios-irq before overwritingdev->irq, thepcibios_alloc_irqfunction does not.Usually this is not a problem, as the pcibios-irq is allocated before probe time of the device and the MSI irq is allocted from the drivers probe path.
But there are PCI devices handled by the core kernel and not by a standard pci driver, like the AMD IOMMU for example. For the AMD IOMMU a normal pci device driver does not make sense, because a driver can be forcibly unbound from its device, which is not a good idea for an IOMMU.
Nevertheless the PCI core code tries to match the PCI device implementing the AMD IOMMU against drivers, and allocates/frees a pcibios IRQ every time it tries out a new driver. This overwrites the
dev->irqfield set bypci_enable_msi()and sets it to 0 in the end (because the probe fails and the pcibios-irq is freed again).On suspend/resume this breaks the kernel, because the irq descriptor for irq 0 is NULL.
Quoted 8 lines
Fix this by not allocating a pcibios-irq when MSI is already active. This also has the benefit, that a device claimed by the core kernel can not be probed by a pci driver later.
Cc Jiang Liu <[email protected]>Reported-by Borislav Petkov <[email protected]>Signed-off-by Joerg Roedel <[email protected]>
Applied with Thomas' reviewed-by to pci/msi for v4.4, thanks, Joerg!
Quoted 31 lines
--- arch/x86/pci/common.c | 8 ++++++++ 1 file changed, 8 insertions(+)arch/x86/pci/common.c +8
-0@@ -675,6 +675,14 @@ int pcibios_add_device(struct pci_dev *dev)int pcibios_alloc_irq (struct pci_dev *dev) { +/* +* If the PCI device was already claimed by core code and has +* MSI enabled, probing of the pcibios irq will overwrite +* dev->irq. So bail out if MSI is already enabled. +*/ +if (pci_dev_msi_enabled (dev)) +return -EBUSY ; +return pcibios_enable_irq (dev); }Signature · 7 lines
1.9.1 -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Quoted 33 lines · Bjorn Helgaas
Quoted 32 lines · Joerg Roedel
From: Joerg Roedel <[email protected]>
The pcibios-irq and MSI both use
dev->irqto store the IRQ number. While the MSI code checks for that and frees the pcibios-irq before overwritingdev->irq, thepcibios_alloc_irqfunction does not.Usually this is not a problem, as the pcibios-irq is allocated before probe time of the device and the MSI irq is allocted from the drivers probe path.
But there are PCI devices handled by the core kernel and not by a standard pci driver, like the AMD IOMMU for example. For the AMD IOMMU a normal pci device driver does not make sense, because a driver can be forcibly unbound from its device, which is not a good idea for an IOMMU.
Nevertheless the PCI core code tries to match the PCI device implementing the AMD IOMMU against drivers, and allocates/frees a pcibios IRQ every time it tries out a new driver. This overwrites the
dev->irqfield set bypci_enable_msi()and sets it to 0 in the end (because the probe fails and the pcibios-irq is freed again).On suspend/resume this breaks the kernel, because the irq descriptor for irq 0 is NULL.
Fix this by not allocating a pcibios-irq when MSI is already active. This also has the benefit, that a device claimed by the core kernel can not be probed by a pci driver later.
Quoted 5 lines
Quoted 3 lines
Cc Jiang Liu <[email protected]>Reported-by Borislav Petkov <[email protected]>Signed-off-by Joerg Roedel <[email protected]>Applied with Thomas' reviewed-by to pci/msi for v4.4, thanks, Joerg!
Hi Bjorn,
There's another patch already merged into mainstream kernel,
which solves this issue in another way by making use of pci_dev->match_driver flag. Please refer to:cbbc00be2ce3 ("iommu/amd: Prevent binding other PCI drivers to IOMMU PCI devices")
So I think this patch is redundant now.
Thanks,
Gerry
Quoted 32 lines
Quoted 32 lines
--- arch/x86/pci/common.c | 8 ++++++++ 1 file changed, 8 insertions(+)arch/x86/pci/common.c +8
-0@@ -675,6 +675,14 @@ int pcibios_add_device(struct pci_dev *dev)int pcibios_alloc_irq (struct pci_dev *dev) { +/* +* If the PCI device was already claimed by core code and has +* MSI enabled, probing of the pcibios irq will overwrite +* dev->irq. So bail out if MSI is already enabled. +*/ +if (pci_dev_msi_enabled (dev)) +return -EBUSY ; +return pcibios_enable_irq (dev); }Signature · 7 lines
1.9.1 -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Joerg Roedel
maintainerQuoted 6 lines · Jiang Liu
There's another patch already merged into mainstream kernel,
which solves this issue in another way by making use ofpci_dev->match_driverflag. Please refer to:cbbc00be2ce3("iommu/amd: Prevent binding other PCI drivers to IOMMU PCI devices")
So I think this patch is redundant now.
Yeah, it fixes the same problem already fixed by your match_driver fix. But I think this fix makes still sense as it avoids that the problem can reappear in the future with other drivers. Setting pci_dev->match_driver could be easily forgotten when writing new code.
Joerg