From: Cédric Le Goater <clg@kaod.org> Date: 2020-08-07 10:25:51
When a passthrough IO adapter is removed from a pseries machine using
hash MMU and the XIVE interrupt mode, the POWER hypervisor expects the
guest OS to clear all page table entries related to the adapter. If
some are still present, the RTAS call which isolates the PCI slot
returns error 9001 "valid outstanding translations" and the removal of
the IO adapter fails. This is because when the PHBs are scanned, Linux
maps automatically the INTx interrupts in the Linux interrupt number
space but these are never removed.
To solve this problem, we introduce a PPC platform specific
pcibios_remove_bus() routine which clears all interrupt mappings when
the bus is removed. This also clears the associated page table entries
of the ESB pages when using XIVE.
For this purpose, we record the logical interrupt numbers of the
mapped interrupt under the PHB structure and let pcibios_remove_bus()
do the clean up.
Since some PCI adapters, like GPUs, use the "interrupt-map" property
to describe interrupt mappings other than the legacy INTx interrupts,
we can not restrict the size of the mapping array to PCI_NUM_INTX. The
number of interrupt mappings is computed from the "interrupt-map"
property and the mapping array is allocated accordingly.
Cc: "Oliver O'Halloran" <oohall@gmail.com>
Cc: Alexey Kardashevskiy <redacted>
Signed-off-by: Cédric Le Goater <clg@kaod.org>
---
Changes since v2:
- merged 2 patches.
arch/powerpc/include/asm/pci-bridge.h | 6 ++
arch/powerpc/kernel/pci-common.c | 114 ++++++++++++++++++++++++++
2 files changed, 120 insertions(+)
@@ -127,6 +130,9 @@ struct pci_controller {void*private_data;structnpu*npu;++unsignedintirq_count;+unsignedint*irq_map;};/* These are used for config access before all the PCI probing
@@ -353,6 +353,115 @@ struct pci_controller *pci_find_controller_for_domain(int domain_nr)returnNULL;}+/*+*Assumptionismadeontheinterruptparent.Allinterrupt-map+*entriesareconsideredtohavethesameparent.+*/+staticintpcibios_irq_map_count(structpci_controller*phb)+{+const__be32*imap;+intimaplen;+structdevice_node*parent;+u32intsize,addrsize,parintsize,paraddrsize;++if(of_property_read_u32(phb->dn,"#interrupt-cells",&intsize))+return0;+if(of_property_read_u32(phb->dn,"#address-cells",&addrsize))+return0;++imap=of_get_property(phb->dn,"interrupt-map",&imaplen);+if(!imap){+pr_debug("%pOF : no interrupt-map\n",phb->dn);+return0;+}+imaplen/=sizeof(u32);+pr_debug("%pOF : imaplen=%d\n",phb->dn,imaplen);++if(imaplen<(addrsize+intsize+1))+return0;++imap+=intsize+addrsize;+parent=of_find_node_by_phandle(be32_to_cpup(imap));+if(!parent){+pr_debug("%pOF : no imap parent found !\n",phb->dn);+return0;+}++if(of_property_read_u32(parent,"#interrupt-cells",&parintsize)){+pr_debug("%pOF : parent lacks #interrupt-cells!\n",phb->dn);+return0;+}++if(of_property_read_u32(parent,"#address-cells",¶ddrsize))+paraddrsize=0;++returnimaplen/(addrsize+intsize+1+paraddrsize+parintsize);+}++staticvoidpcibios_irq_map_init(structpci_controller*phb)+{+phb->irq_count=pcibios_irq_map_count(phb);+if(phb->irq_count<PCI_NUM_INTX)+phb->irq_count=PCI_NUM_INTX;++pr_debug("%pOF : interrupt map #%d\n",phb->dn,phb->irq_count);++phb->irq_map=kcalloc(phb->irq_count,sizeof(unsignedint),+GFP_KERNEL);+}++staticvoidpci_irq_map_register(structpci_dev*pdev,unsignedintvirq)+{+structpci_controller*phb=pci_bus_to_host(pdev->bus);+inti;++if(!phb->irq_map)+return;++for(i=0;i<phb->irq_count;i++){+/*+*Lookforanemptyoranequivalentslot,asINTx+*interruptscanbesharedbetweenadapters.+*/+if(phb->irq_map[i]==virq||!phb->irq_map[i]){+phb->irq_map[i]=virq;+break;+}+}++if(i==phb->irq_count)+pr_err("PCI:%s all platform interrupts mapped\n",+pci_name(pdev));+}++/*+*Clearingthemappedinterruptswillalsocleartheunderlying+*mappingsoftheESBpagesoftheinterruptswhenunderXIVE.Itis+*arequirementofPowerVMtoclearallmemorymappingsbefore+*removingaPHB.+*/+staticvoidpci_irq_map_dispose(structpci_bus*bus)+{+structpci_controller*phb=pci_bus_to_host(bus);+inti;++if(!phb->irq_map)+return;++pr_debug("PCI: Clearing interrupt mappings for PHB %04x:%02x...\n",+pci_domain_nr(bus),bus->number);+for(i=0;i<phb->irq_count;i++)+irq_dispose_mapping(phb->irq_map[i]);++kfree(phb->irq_map);+}++voidpcibios_remove_bus(structpci_bus*bus)+{+pci_irq_map_dispose(bus);+}+EXPORT_SYMBOL_GPL(pcibios_remove_bus);+/**Readstheinterruptpintodetermineifinterruptisusebycard.*Iftheinterruptisused,thengetstheinterruptlinefromthe
@@ -401,6 +510,8 @@ static int pci_read_irq_line(struct pci_dev *pci_dev)pci_dev->irq=virq;+/* Record all interrut mappings for later removal of a PHB */+pci_irq_map_register(pci_dev,virq);return0;}
@@ -1554,6 +1665,9 @@ void pcibios_scan_phb(struct pci_controller *hose)pr_debug("PCI: Scanning PHB %pOF\n",node);+/* Allocate interrupt mappings array */+pcibios_irq_map_init(hose);+/* Get some IO space for the new PHB */pcibios_setup_phb_io_space(hose);
When a passthrough IO adapter is removed from a pseries machine using
hash MMU and the XIVE interrupt mode, the POWER hypervisor expects the
guest OS to clear all page table entries related to the adapter. If
some are still present, the RTAS call which isolates the PCI slot
returns error 9001 "valid outstanding translations" and the removal of
the IO adapter fails. This is because when the PHBs are scanned, Linux
maps automatically the INTx interrupts in the Linux interrupt number
space but these are never removed.
To solve this problem, we introduce a PPC platform specific
pcibios_remove_bus() routine which clears all interrupt mappings when
the bus is removed. This also clears the associated page table entries
of the ESB pages when using XIVE.
For this purpose, we record the logical interrupt numbers of the
mapped interrupt under the PHB structure and let pcibios_remove_bus()
do the clean up.
Since some PCI adapters, like GPUs, use the "interrupt-map" property
to describe interrupt mappings other than the legacy INTx interrupts,
we can not restrict the size of the mapping array to PCI_NUM_INTX. The
number of interrupt mappings is computed from the "interrupt-map"
property and the mapping array is allocated accordingly.
Cc: "Oliver O'Halloran" <oohall@gmail.com>
Cc: Alexey Kardashevskiy <redacted>
Signed-off-by: Cédric Le Goater <clg@kaod.org>
I thought we could reuse some of the common OF code for the DT parsing
but we cannot (easily) so it is good as it is:
Reviewed-by: Alexey Kardashevskiy <redacted>
@@ -127,6 +130,9 @@ struct pci_controller {void*private_data;structnpu*npu;++unsignedintirq_count;+unsignedint*irq_map;};/* These are used for config access before all the PCI probing
@@ -353,6 +353,115 @@ struct pci_controller *pci_find_controller_for_domain(int domain_nr)returnNULL;}+/*+*Assumptionismadeontheinterruptparent.Allinterrupt-map+*entriesareconsideredtohavethesameparent.+*/+staticintpcibios_irq_map_count(structpci_controller*phb)+{+const__be32*imap;+intimaplen;+structdevice_node*parent;+u32intsize,addrsize,parintsize,paraddrsize;++if(of_property_read_u32(phb->dn,"#interrupt-cells",&intsize))+return0;+if(of_property_read_u32(phb->dn,"#address-cells",&addrsize))+return0;++imap=of_get_property(phb->dn,"interrupt-map",&imaplen);+if(!imap){+pr_debug("%pOF : no interrupt-map\n",phb->dn);+return0;+}+imaplen/=sizeof(u32);+pr_debug("%pOF : imaplen=%d\n",phb->dn,imaplen);++if(imaplen<(addrsize+intsize+1))+return0;++imap+=intsize+addrsize;+parent=of_find_node_by_phandle(be32_to_cpup(imap));+if(!parent){+pr_debug("%pOF : no imap parent found !\n",phb->dn);+return0;+}++if(of_property_read_u32(parent,"#interrupt-cells",&parintsize)){+pr_debug("%pOF : parent lacks #interrupt-cells!\n",phb->dn);+return0;+}++if(of_property_read_u32(parent,"#address-cells",¶ddrsize))+paraddrsize=0;++returnimaplen/(addrsize+intsize+1+paraddrsize+parintsize);+}++staticvoidpcibios_irq_map_init(structpci_controller*phb)+{+phb->irq_count=pcibios_irq_map_count(phb);+if(phb->irq_count<PCI_NUM_INTX)+phb->irq_count=PCI_NUM_INTX;++pr_debug("%pOF : interrupt map #%d\n",phb->dn,phb->irq_count);++phb->irq_map=kcalloc(phb->irq_count,sizeof(unsignedint),+GFP_KERNEL);+}++staticvoidpci_irq_map_register(structpci_dev*pdev,unsignedintvirq)+{+structpci_controller*phb=pci_bus_to_host(pdev->bus);+inti;++if(!phb->irq_map)+return;++for(i=0;i<phb->irq_count;i++){+/*+*Lookforanemptyoranequivalentslot,asINTx+*interruptscanbesharedbetweenadapters.+*/+if(phb->irq_map[i]==virq||!phb->irq_map[i]){+phb->irq_map[i]=virq;+break;+}+}++if(i==phb->irq_count)+pr_err("PCI:%s all platform interrupts mapped\n",+pci_name(pdev));+}++/*+*Clearingthemappedinterruptswillalsocleartheunderlying+*mappingsoftheESBpagesoftheinterruptswhenunderXIVE.Itis+*arequirementofPowerVMtoclearallmemorymappingsbefore+*removingaPHB.+*/+staticvoidpci_irq_map_dispose(structpci_bus*bus)+{+structpci_controller*phb=pci_bus_to_host(bus);+inti;++if(!phb->irq_map)+return;++pr_debug("PCI: Clearing interrupt mappings for PHB %04x:%02x...\n",+pci_domain_nr(bus),bus->number);+for(i=0;i<phb->irq_count;i++)+irq_dispose_mapping(phb->irq_map[i]);++kfree(phb->irq_map);+}++voidpcibios_remove_bus(structpci_bus*bus)+{+pci_irq_map_dispose(bus);+}+EXPORT_SYMBOL_GPL(pcibios_remove_bus);+/**Readstheinterruptpintodetermineifinterruptisusebycard.*Iftheinterruptisused,thengetstheinterruptlinefromthe
@@ -401,6 +510,8 @@ static int pci_read_irq_line(struct pci_dev *pci_dev)pci_dev->irq=virq;+/* Record all interrut mappings for later removal of a PHB */+pci_irq_map_register(pci_dev,virq);return0;}
@@ -1554,6 +1665,9 @@ void pcibios_scan_phb(struct pci_controller *hose)pr_debug("PCI: Scanning PHB %pOF\n",node);+/* Allocate interrupt mappings array */+pcibios_irq_map_init(hose);+/* Get some IO space for the new PHB */pcibios_setup_phb_io_space(hose);
From: Michael Ellerman <hidden> Date: 2020-09-17 12:01:21
On Fri, 7 Aug 2020 12:18:54 +0200, C��dric Le Goater wrote:
When a passthrough IO adapter is removed from a pseries machine using
hash MMU and the XIVE interrupt mode, the POWER hypervisor expects the
guest OS to clear all page table entries related to the adapter. If
some are still present, the RTAS call which isolates the PCI slot
returns error 9001 "valid outstanding translations" and the removal of
the IO adapter fails. This is because when the PHBs are scanned, Linux
maps automatically the INTx interrupts in the Linux interrupt number
space but these are never removed.
[...]
On Fri, 2020-08-07 at 12:18 +0200, Cédric Le Goater wrote:
When a passthrough IO adapter is removed from a pseries machine using
hash MMU and the XIVE interrupt mode, the POWER hypervisor expects the
guest OS to clear all page table entries related to the adapter. If
some are still present, the RTAS call which isolates the PCI slot
returns error 9001 "valid outstanding translations" and the removal of
the IO adapter fails. This is because when the PHBs are scanned, Linux
maps automatically the INTx interrupts in the Linux interrupt number
space but these are never removed.
To solve this problem, we introduce a PPC platform specific
pcibios_remove_bus() routine which clears all interrupt mappings when
the bus is removed. This also clears the associated page table entries
of the ESB pages when using XIVE.
For this purpose, we record the logical interrupt numbers of the
mapped interrupt under the PHB structure and let pcibios_remove_bus()
do the clean up.
Since some PCI adapters, like GPUs, use the "interrupt-map" property
to describe interrupt mappings other than the legacy INTx interrupts,
we can not restrict the size of the mapping array to PCI_NUM_INTX. The
number of interrupt mappings is computed from the "interrupt-map"
property and the mapping array is allocated accordingly.
Cc: "Oliver O'Halloran" <oohall@gmail.com>
Cc: Alexey Kardashevskiy <redacted>
Signed-off-by: Cédric Le Goater <clg@kaod.org>
@@ -127,6 +130,9 @@ struct pci_controller {void*private_data;structnpu*npu;++unsignedintirq_count;+unsignedint*irq_map;};/* These are used for config access before all the PCI probing
*pci_find_controller_for_domain(int domain_nr)
return NULL;
}
+/*
+ * Assumption is made on the interrupt parent. All interrupt-map
+ * entries are considered to have the same parent.
+ */
+static int pcibios_irq_map_count(struct pci_controller *phb)
+{
+ const __be32 *imap;
+ int imaplen;
+ struct device_node *parent;
+ u32 intsize, addrsize, parintsize, paraddrsize;
+
+ if (of_property_read_u32(phb->dn, "#interrupt-cells", &intsize))
+ return 0;
+ if (of_property_read_u32(phb->dn, "#address-cells", &addrsize))
+ return 0;
+
+ imap = of_get_property(phb->dn, "interrupt-map", &imaplen);
+ if (!imap) {
+ pr_debug("%pOF : no interrupt-map\n", phb->dn);
+ return 0;
+ }
+ imaplen /= sizeof(u32);
+ pr_debug("%pOF : imaplen=%d\n", phb->dn, imaplen);
+
+ if (imaplen < (addrsize + intsize + 1))
+ return 0;
+
+ imap += intsize + addrsize;
+ parent = of_find_node_by_phandle(be32_to_cpup(imap));
+ if (!parent) {
+ pr_debug("%pOF : no imap parent found !\n", phb->dn);
+ return 0;
+ }
+
+ if (of_property_read_u32(parent, "#interrupt-cells", &parintsize)) {
+ pr_debug("%pOF : parent lacks #interrupt-cells!\n", phb->dn);
+ return 0;
+ }
+
+ if (of_property_read_u32(parent, "#address-cells", ¶ddrsize))
+ paraddrsize = 0;
+
+ return imaplen / (addrsize + intsize + 1 + paraddrsize + parintsize);
+}
+
+static void pcibios_irq_map_init(struct pci_controller *phb)
+{
+ phb->irq_count = pcibios_irq_map_count(phb);
+ if (phb->irq_count < PCI_NUM_INTX)
+ phb->irq_count = PCI_NUM_INTX;
+
+ pr_debug("%pOF : interrupt map #%d\n", phb->dn, phb->irq_count);
+
+ phb->irq_map = kcalloc(phb->irq_count, sizeof(unsigned int),
+ GFP_KERNEL);
+}
+
+static void pci_irq_map_register(struct pci_dev *pdev, unsigned int virq)
+{
+ struct pci_controller *phb = pci_bus_to_host(pdev->bus);
+ int i;
+
+ if (!phb->irq_map)
+ return;
+
+ for (i = 0; i < phb->irq_count; i++) {
+ /*
+ * Look for an empty or an equivalent slot, as INTx
+ * interrupts can be shared between adapters.
+ */
+ if (phb->irq_map[i] == virq || !phb->irq_map[i]) {
+ phb->irq_map[i] = virq;
+ break;
+ }
+ }
+
+ if (i == phb->irq_count)
+ pr_err("PCI:%s all platform interrupts mapped\n",
+ pci_name(pdev));
+}
+
+/*
+ * Clearing the mapped interrupts will also clear the underlying
+ * mappings of the ESB pages of the interrupts when under XIVE. It is
+ * a requirement of PowerVM to clear all memory mappings before
+ * removing a PHB.
+ */
+static void pci_irq_map_dispose(struct pci_bus *bus)
+{
+ struct pci_controller *phb = pci_bus_to_host(bus);
+ int i;
+
+ if (!phb->irq_map)
+ return;
+
+ pr_debug("PCI: Clearing interrupt mappings for PHB %04x:%02x...\n",
+ pci_domain_nr(bus), bus->number);
+ for (i = 0; i < phb->irq_count; i++)
+ irq_dispose_mapping(phb->irq_map[i]);
+
+ kfree(phb->irq_map);
+}
+
+void pcibios_remove_bus(struct pci_bus *bus)
+{
+ pci_irq_map_dispose(bus);
+}
+EXPORT_SYMBOL_GPL(pcibios_remove_bus);
+
/*
* Reads the interrupt pin to determine if interrupt is use by card.
* If the interrupt is used, then gets the interrupt line from the
@@ -401,6 +510,8 @@ static int pci_read_irq_line(struct pci_dev *pci_dev) pci_dev->irq = virq;+ /* Record all interrut mappings for later removal of a PHB */+ pci_irq_map_register(pci_dev, virq); return 0; }
@@ -1554,6 +1665,9 @@ void pcibios_scan_phb(struct pci_controller *hose) pr_debug("PCI: Scanning PHB %pOF\n", node);+ /* Allocate interrupt mappings array */+ pcibios_irq_map_init(hose);+ /* Get some IO space for the new PHB */ pcibios_setup_phb_io_space(hose);
From: Cédric Le Goater <clg@kaod.org> Date: 2020-09-23 07:14:33
On 9/23/20 2:33 AM, Qian Cai wrote:
On Fri, 2020-08-07 at 12:18 +0200, Cédric Le Goater wrote:
quoted
When a passthrough IO adapter is removed from a pseries machine using
hash MMU and the XIVE interrupt mode, the POWER hypervisor expects the
guest OS to clear all page table entries related to the adapter. If
some are still present, the RTAS call which isolates the PCI slot
returns error 9001 "valid outstanding translations" and the removal of
the IO adapter fails. This is because when the PHBs are scanned, Linux
maps automatically the INTx interrupts in the Linux interrupt number
space but these are never removed.
To solve this problem, we introduce a PPC platform specific
pcibios_remove_bus() routine which clears all interrupt mappings when
the bus is removed. This also clears the associated page table entries
of the ESB pages when using XIVE.
For this purpose, we record the logical interrupt numbers of the
mapped interrupt under the PHB structure and let pcibios_remove_bus()
do the clean up.
Since some PCI adapters, like GPUs, use the "interrupt-map" property
to describe interrupt mappings other than the legacy INTx interrupts,
we can not restrict the size of the mapping array to PCI_NUM_INTX. The
number of interrupt mappings is computed from the "interrupt-map"
property and the mapping array is allocated accordingly.
Cc: "Oliver O'Halloran" <oohall@gmail.com>
Cc: Alexey Kardashevskiy <redacted>
Signed-off-by: Cédric Le Goater <clg@kaod.org>
OK. The patch is missing a NULL assignement after kfree() and that
might be the issue.
I did try PHB removal under PowerNV, so I would like to understand
how we managed to remove twice the PCI bus and possibly reproduce.
Any chance we could grab what the syscall fuzzer (syzkaller) did ?
Thanks,
C.
@@ -127,6 +130,9 @@ struct pci_controller {void*private_data;structnpu*npu;++unsignedintirq_count;+unsignedint*irq_map;};/* These are used for config access before all the PCI probing
*pci_find_controller_for_domain(int domain_nr)
return NULL;
}
+/*
+ * Assumption is made on the interrupt parent. All interrupt-map
+ * entries are considered to have the same parent.
+ */
+static int pcibios_irq_map_count(struct pci_controller *phb)
+{
+ const __be32 *imap;
+ int imaplen;
+ struct device_node *parent;
+ u32 intsize, addrsize, parintsize, paraddrsize;
+
+ if (of_property_read_u32(phb->dn, "#interrupt-cells", &intsize))
+ return 0;
+ if (of_property_read_u32(phb->dn, "#address-cells", &addrsize))
+ return 0;
+
+ imap = of_get_property(phb->dn, "interrupt-map", &imaplen);
+ if (!imap) {
+ pr_debug("%pOF : no interrupt-map\n", phb->dn);
+ return 0;
+ }
+ imaplen /= sizeof(u32);
+ pr_debug("%pOF : imaplen=%d\n", phb->dn, imaplen);
+
+ if (imaplen < (addrsize + intsize + 1))
+ return 0;
+
+ imap += intsize + addrsize;
+ parent = of_find_node_by_phandle(be32_to_cpup(imap));
+ if (!parent) {
+ pr_debug("%pOF : no imap parent found !\n", phb->dn);
+ return 0;
+ }
+
+ if (of_property_read_u32(parent, "#interrupt-cells", &parintsize)) {
+ pr_debug("%pOF : parent lacks #interrupt-cells!\n", phb->dn);
+ return 0;
+ }
+
+ if (of_property_read_u32(parent, "#address-cells", ¶ddrsize))
+ paraddrsize = 0;
+
+ return imaplen / (addrsize + intsize + 1 + paraddrsize + parintsize);
+}
+
+static void pcibios_irq_map_init(struct pci_controller *phb)
+{
+ phb->irq_count = pcibios_irq_map_count(phb);
+ if (phb->irq_count < PCI_NUM_INTX)
+ phb->irq_count = PCI_NUM_INTX;
+
+ pr_debug("%pOF : interrupt map #%d\n", phb->dn, phb->irq_count);
+
+ phb->irq_map = kcalloc(phb->irq_count, sizeof(unsigned int),
+ GFP_KERNEL);
+}
+
+static void pci_irq_map_register(struct pci_dev *pdev, unsigned int virq)
+{
+ struct pci_controller *phb = pci_bus_to_host(pdev->bus);
+ int i;
+
+ if (!phb->irq_map)
+ return;
+
+ for (i = 0; i < phb->irq_count; i++) {
+ /*
+ * Look for an empty or an equivalent slot, as INTx
+ * interrupts can be shared between adapters.
+ */
+ if (phb->irq_map[i] == virq || !phb->irq_map[i]) {
+ phb->irq_map[i] = virq;
+ break;
+ }
+ }
+
+ if (i == phb->irq_count)
+ pr_err("PCI:%s all platform interrupts mapped\n",
+ pci_name(pdev));
+}
+
+/*
+ * Clearing the mapped interrupts will also clear the underlying
+ * mappings of the ESB pages of the interrupts when under XIVE. It is
+ * a requirement of PowerVM to clear all memory mappings before
+ * removing a PHB.
+ */
+static void pci_irq_map_dispose(struct pci_bus *bus)
+{
+ struct pci_controller *phb = pci_bus_to_host(bus);
+ int i;
+
+ if (!phb->irq_map)
+ return;
+
+ pr_debug("PCI: Clearing interrupt mappings for PHB %04x:%02x...\n",
+ pci_domain_nr(bus), bus->number);
+ for (i = 0; i < phb->irq_count; i++)
+ irq_dispose_mapping(phb->irq_map[i]);
+
+ kfree(phb->irq_map);
+}
+
+void pcibios_remove_bus(struct pci_bus *bus)
+{
+ pci_irq_map_dispose(bus);
+}
+EXPORT_SYMBOL_GPL(pcibios_remove_bus);
+
/*
* Reads the interrupt pin to determine if interrupt is use by card.
* If the interrupt is used, then gets the interrupt line from the
@@ -401,6 +510,8 @@ static int pci_read_irq_line(struct pci_dev *pci_dev) pci_dev->irq = virq;+ /* Record all interrut mappings for later removal of a PHB */+ pci_irq_map_register(pci_dev, virq); return 0; }
@@ -1554,6 +1665,9 @@ void pcibios_scan_phb(struct pci_controller *hose) pr_debug("PCI: Scanning PHB %pOF\n", node);+ /* Allocate interrupt mappings array */+ pcibios_irq_map_init(hose);+ /* Get some IO space for the new PHB */ pcibios_setup_phb_io_space(hose);
On Fri, 2020-08-07 at 12:18 +0200, Cédric Le Goater wrote:
quoted
When a passthrough IO adapter is removed from a pseries machine using
hash MMU and the XIVE interrupt mode, the POWER hypervisor expects the
guest OS to clear all page table entries related to the adapter. If
some are still present, the RTAS call which isolates the PCI slot
returns error 9001 "valid outstanding translations" and the removal of
the IO adapter fails. This is because when the PHBs are scanned, Linux
maps automatically the INTx interrupts in the Linux interrupt number
space but these are never removed.
To solve this problem, we introduce a PPC platform specific
pcibios_remove_bus() routine which clears all interrupt mappings when
the bus is removed. This also clears the associated page table entries
of the ESB pages when using XIVE.
For this purpose, we record the logical interrupt numbers of the
mapped interrupt under the PHB structure and let pcibios_remove_bus()
do the clean up.
Since some PCI adapters, like GPUs, use the "interrupt-map" property
to describe interrupt mappings other than the legacy INTx interrupts,
we can not restrict the size of the mapping array to PCI_NUM_INTX. The
number of interrupt mappings is computed from the "interrupt-map"
property and the mapping array is allocated accordingly.
Cc: "Oliver O'Halloran" <oohall@gmail.com>
Cc: Alexey Kardashevskiy <redacted>
Signed-off-by: Cédric Le Goater <clg@kaod.org>
OK. The patch is missing a NULL assignement after kfree() and that
might be the issue.
I did try PHB removal under PowerNV, so I would like to understand
how we managed to remove twice the PCI bus and possibly reproduce.
Any chance we could grab what the syscall fuzzer (syzkaller) did ?
My guess would be it is doing this in parallel to provoke races.
--
Alexey
From: Cédric Le Goater <clg@kaod.org> Date: 2020-09-24 10:31:52
On 9/24/20 7:11 AM, Alexey Kardashevskiy wrote:
On 23/09/2020 17:06, Cédric Le Goater wrote:
quoted
On 9/23/20 2:33 AM, Qian Cai wrote:
quoted
On Fri, 2020-08-07 at 12:18 +0200, Cédric Le Goater wrote:
quoted
When a passthrough IO adapter is removed from a pseries machine using
hash MMU and the XIVE interrupt mode, the POWER hypervisor expects the
guest OS to clear all page table entries related to the adapter. If
some are still present, the RTAS call which isolates the PCI slot
returns error 9001 "valid outstanding translations" and the removal of
the IO adapter fails. This is because when the PHBs are scanned, Linux
maps automatically the INTx interrupts in the Linux interrupt number
space but these are never removed.
To solve this problem, we introduce a PPC platform specific
pcibios_remove_bus() routine which clears all interrupt mappings when
the bus is removed. This also clears the associated page table entries
of the ESB pages when using XIVE.
For this purpose, we record the logical interrupt numbers of the
mapped interrupt under the PHB structure and let pcibios_remove_bus()
do the clean up.
Since some PCI adapters, like GPUs, use the "interrupt-map" property
to describe interrupt mappings other than the legacy INTx interrupts,
we can not restrict the size of the mapping array to PCI_NUM_INTX. The
number of interrupt mappings is computed from the "interrupt-map"
property and the mapping array is allocated accordingly.
Cc: "Oliver O'Halloran" <oohall@gmail.com>
Cc: Alexey Kardashevskiy <redacted>
Signed-off-by: Cédric Le Goater <clg@kaod.org>
OK. The patch is missing a NULL assignement after kfree() and that
might be the issue.
I did try PHB removal under PowerNV, so I would like to understand
how we managed to remove twice the PCI bus and possibly reproduce.
Any chance we could grab what the syscall fuzzer (syzkaller) did ?
My guess would be it is doing this in parallel to provoke races.
Concurrency removal and rescan should be controlled by :
pci_stop_and_remove_bus_device_locked()
pci_lock_rescan_remove()
And, in the report, the stack traces are on the same CPU and PID.
What I think is happening is that we did a couple of remove/rescan
of the same PHB. The problem is that ->irq_map is not reallocated
the second time because the PHB is re-scanned differently on the
PowerNV platform. At the second remove, the ->irq_map being not NULL,
we try to kfree it again and the allocator warns of a double free :/
This works fine on pseries/KVM because the PHB is never removed and
on pseries/pHyp, pcibios_scan_phb() is called at PHB hotplug. But on
PowerNV, pcibios_scan_phb() is only called at probe/boot time and
not at hotplug time :/
I was a good thing to spot that before merge. But I need to revise
that patch again.
Thanks,
C.
On Wed, 2020-09-23 at 09:06 +0200, Cédric Le Goater wrote:
On 9/23/20 2:33 AM, Qian Cai wrote:
quoted
On Fri, 2020-08-07 at 12:18 +0200, Cédric Le Goater wrote:
quoted
When a passthrough IO adapter is removed from a pseries machine using
hash MMU and the XIVE interrupt mode, the POWER hypervisor expects the
guest OS to clear all page table entries related to the adapter. If
some are still present, the RTAS call which isolates the PCI slot
returns error 9001 "valid outstanding translations" and the removal of
the IO adapter fails. This is because when the PHBs are scanned, Linux
maps automatically the INTx interrupts in the Linux interrupt number
space but these are never removed.
To solve this problem, we introduce a PPC platform specific
pcibios_remove_bus() routine which clears all interrupt mappings when
the bus is removed. This also clears the associated page table entries
of the ESB pages when using XIVE.
For this purpose, we record the logical interrupt numbers of the
mapped interrupt under the PHB structure and let pcibios_remove_bus()
do the clean up.
Since some PCI adapters, like GPUs, use the "interrupt-map" property
to describe interrupt mappings other than the legacy INTx interrupts,
we can not restrict the size of the mapping array to PCI_NUM_INTX. The
number of interrupt mappings is computed from the "interrupt-map"
property and the mapping array is allocated accordingly.
Cc: "Oliver O'Halloran" <oohall@gmail.com>
Cc: Alexey Kardashevskiy <redacted>
Signed-off-by: Cédric Le Goater <clg@kaod.org>
OK. The patch is missing a NULL assignement after kfree() and that
might be the issue.
I did try PHB removal under PowerNV, so I would like to understand
how we managed to remove twice the PCI bus and possibly reproduce.
Any chance we could grab what the syscall fuzzer (syzkaller) did ?
From: Michael Ellerman <mpe@ellerman.id.au> Date: 2020-10-14 09:30:50
Qian Cai [off-list ref] writes:
On Wed, 2020-09-23 at 09:06 +0200, Cédric Le Goater wrote:
quoted
On 9/23/20 2:33 AM, Qian Cai wrote:
quoted
On Fri, 2020-08-07 at 12:18 +0200, Cédric Le Goater wrote:
quoted
When a passthrough IO adapter is removed from a pseries machine using
hash MMU and the XIVE interrupt mode, the POWER hypervisor expects the
guest OS to clear all page table entries related to the adapter. If
some are still present, the RTAS call which isolates the PCI slot
returns error 9001 "valid outstanding translations" and the removal of
the IO adapter fails. This is because when the PHBs are scanned, Linux
maps automatically the INTx interrupts in the Linux interrupt number
space but these are never removed.
To solve this problem, we introduce a PPC platform specific
pcibios_remove_bus() routine which clears all interrupt mappings when
the bus is removed. This also clears the associated page table entries
of the ESB pages when using XIVE.
For this purpose, we record the logical interrupt numbers of the
mapped interrupt under the PHB structure and let pcibios_remove_bus()
do the clean up.
Since some PCI adapters, like GPUs, use the "interrupt-map" property
to describe interrupt mappings other than the legacy INTx interrupts,
we can not restrict the size of the mapping array to PCI_NUM_INTX. The
number of interrupt mappings is computed from the "interrupt-map"
property and the mapping array is allocated accordingly.
Cc: "Oliver O'Halloran" <oohall@gmail.com>
Cc: Alexey Kardashevskiy <redacted>
Signed-off-by: Cédric Le Goater <clg@kaod.org>
OK. The patch is missing a NULL assignement after kfree() and that
might be the issue.
I did try PHB removal under PowerNV, so I would like to understand
how we managed to remove twice the PCI bus and possibly reproduce.
Any chance we could grab what the syscall fuzzer (syzkaller) did ?
Any update on this? Maybe Michael or Stephen could drop this for now, so our
fuzzing could continue to find something else new?
On Fri, 2020-08-07 at 12:18 +0200, Cédric Le Goater wrote:
quoted
When a passthrough IO adapter is removed from a pseries machine using
hash MMU and the XIVE interrupt mode, the POWER hypervisor expects the
guest OS to clear all page table entries related to the adapter. If
some are still present, the RTAS call which isolates the PCI slot
returns error 9001 "valid outstanding translations" and the removal of
the IO adapter fails. This is because when the PHBs are scanned, Linux
maps automatically the INTx interrupts in the Linux interrupt number
space but these are never removed.
To solve this problem, we introduce a PPC platform specific
pcibios_remove_bus() routine which clears all interrupt mappings when
the bus is removed. This also clears the associated page table entries
of the ESB pages when using XIVE.
For this purpose, we record the logical interrupt numbers of the
mapped interrupt under the PHB structure and let pcibios_remove_bus()
do the clean up.
Since some PCI adapters, like GPUs, use the "interrupt-map" property
to describe interrupt mappings other than the legacy INTx interrupts,
we can not restrict the size of the mapping array to PCI_NUM_INTX. The
number of interrupt mappings is computed from the "interrupt-map"
property and the mapping array is allocated accordingly.
Cc: "Oliver O'Halloran" <oohall@gmail.com>
Cc: Alexey Kardashevskiy <redacted>
Signed-off-by: Cédric Le Goater <clg@kaod.org>
OK. The patch is missing a NULL assignement after kfree() and that
might be the issue.
I did try PHB removal under PowerNV, so I would like to understand
how we managed to remove twice the PCI bus and possibly reproduce.
Any chance we could grab what the syscall fuzzer (syzkaller) did ?
How do you remove PHBs exactly? There is no such thing in the powernv
platform, I thought someone added this and you are fixing it but no.
PHBs on powernv are created at the boot time and there is no way to
remove them, you can only try removing all the bridges.
So what exactly are you doing?
--
Alexey
From: Cédric Le Goater <clg@kaod.org> Date: 2020-11-02 17:04:56
On 10/14/20 4:55 AM, Alexey Kardashevskiy wrote:
On 23/09/2020 17:06, Cédric Le Goater wrote:
quoted
On 9/23/20 2:33 AM, Qian Cai wrote:
quoted
On Fri, 2020-08-07 at 12:18 +0200, Cédric Le Goater wrote:
quoted
When a passthrough IO adapter is removed from a pseries machine using
hash MMU and the XIVE interrupt mode, the POWER hypervisor expects the
guest OS to clear all page table entries related to the adapter. If
some are still present, the RTAS call which isolates the PCI slot
returns error 9001 "valid outstanding translations" and the removal of
the IO adapter fails. This is because when the PHBs are scanned, Linux
maps automatically the INTx interrupts in the Linux interrupt number
space but these are never removed.
To solve this problem, we introduce a PPC platform specific
pcibios_remove_bus() routine which clears all interrupt mappings when
the bus is removed. This also clears the associated page table entries
of the ESB pages when using XIVE.
For this purpose, we record the logical interrupt numbers of the
mapped interrupt under the PHB structure and let pcibios_remove_bus()
do the clean up.
Since some PCI adapters, like GPUs, use the "interrupt-map" property
to describe interrupt mappings other than the legacy INTx interrupts,
we can not restrict the size of the mapping array to PCI_NUM_INTX. The
number of interrupt mappings is computed from the "interrupt-map"
property and the mapping array is allocated accordingly.
Cc: "Oliver O'Halloran" <oohall@gmail.com>
Cc: Alexey Kardashevskiy <redacted>
Signed-off-by: Cédric Le Goater <clg@kaod.org>
OK. The patch is missing a NULL assignement after kfree() and that
might be the issue.
I did try PHB removal under PowerNV, so I would like to understand
how we managed to remove twice the PCI bus and possibly reproduce.
Any chance we could grab what the syscall fuzzer (syzkaller) did ?
How do you remove PHBs exactly? There is no such thing in the powernv platform, I thought someone added this and you are fixing it but no. PHBs on powernv are created at the boot time and there is no way to remove them, you can only try removing all the bridges.
yes. I noticed that later when proposing the fix for the double
free.
So what exactly are you doing?
What you just said above, with the commands :
echo 1 > /sys/devices/pci0031\:00/0031\:00\:00.0/remove
echo 1 > /sys/devices/pci0031\:00/pci_bus/0031\:00/rescan
C.
On Tue, Nov 3, 2020 at 1:39 AM Cédric Le Goater [off-list ref] wrote:
On 10/14/20 4:55 AM, Alexey Kardashevskiy wrote:
quoted
How do you remove PHBs exactly? There is no such thing in the powernv platform, I thought someone added this and you are fixing it but no. PHBs on powernv are created at the boot time and there is no way to remove them, you can only try removing all the bridges.
yes. I noticed that later when proposing the fix for the double
free.
quoted
So what exactly are you doing?
What you just said above, with the commands :
echo 1 > /sys/devices/pci0031\:00/0031\:00\:00.0/remove
echo 1 > /sys/devices/pci0031\:00/pci_bus/0031\:00/rescan
Right, so that'll remove the root port device (and Bus 01 beneath it),
but the PHB itself is still there. If it was removed the root bus
would also disappear.