* Re: [PATCH] powerpc/pci: Fix PHB removal/rescan on PowerNV [not found] <20200925092258.525079-1-clg@kaod.org> @ 2020-10-08 2:23 ` Oliver O'Halloran 2020-10-08 4:37 ` Cédric Le Goater 0 siblings, 1 reply; 3+ messages in thread From: Oliver O'Halloran @ 2020-10-08 2:23 UTC (permalink / raw) To: Cédric Le Goater Cc: Michael Ellerman, linuxppc-dev, Alexey Kardashevskiy, linux-pci On Fri, Sep 25, 2020 at 7:23 PM Cédric Le Goater <clg@kaod.org> wrote: > > To fix an issue with PHB hotplug on pSeries machine (HPT/XIVE), commit > 3a3181e16fbd introduced a PPC specific pcibios_remove_bus() routine to > clear all interrupt mappings when the bus is removed. This routine > frees an array allocated in pcibios_scan_phb(). > > This broke PHB hotplug on PowerNV because, when a PHB is removed and > re-scanned through sysfs, the PCI layer un-assigns and re-assigns > resources to the PHB but does not destroy and recreate the PCI > controller structure. Since pcibios_remove_bus() does not clear the > 'irq_map' array pointer, a second removal of the PHB will try to free > the array a second time and corrupt memory. "PHB hotplug" and "hot-plugging devices under a PHB" are different things. What you're saying here doesn't make a whole lot of sense to me unless you're conflating the two. The distinction is important since on pseries we can use DLPAR to add and remove actual PHBs (i.e. the pci_controller) at runtime, but there's no corresponding mechanism on PowerNV. > Free the 'irq_map' array in pcibios_free_controller() to fix > corruption and clear interrupt mapping after it has been > disposed. This to avoid filling up the array with successive > remove/rescan of a bus. Even with this patch I think we're still broken. With this patch applied the init path is something like: per-phb init: allocate phb->irq_map array per-bus init: nothing per-device init: pcibios_bus_add_device() pci_read_irq_line() pci_irq_map_register(pci_dev, virq) *record the device's interrupt in phb->irq_map* And the teardown path: per-device teardown: nothing per-bus teardown: pcibios_remove_bus() pci_irq_map_dispose() *walk phb->irq_map and dispose of each mapped interrupt* per-phb teardown: free(phb->irq_map) There's a lot of asymmetry here, which is a problem in itself, but the real issue is that when removing *any* pci_bus under a PHB we dispose of the LSI\ for *every* device under that PHB. Not good. Ideally we should be fixing this by having the per-device teardown handle disposing the mapping. Unfortunately, there's no pcibios hook that's called when removing a pci_dev. However, we can register a bus notifier which will be called when the pci_dev is removed from its bus and we already do that for the per-device EEH teardown and to handle IOMMU TCE invalidation when the device is removed. ^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [PATCH] powerpc/pci: Fix PHB removal/rescan on PowerNV 2020-10-08 2:23 ` [PATCH] powerpc/pci: Fix PHB removal/rescan on PowerNV Oliver O'Halloran @ 2020-10-08 4:37 ` Cédric Le Goater 2020-10-14 14:24 ` Greg Kurz 0 siblings, 1 reply; 3+ messages in thread From: Cédric Le Goater @ 2020-10-08 4:37 UTC (permalink / raw) To: Oliver O'Halloran Cc: Michael Ellerman, linuxppc-dev, Alexey Kardashevskiy, linux-pci On 10/8/20 4:23 AM, Oliver O'Halloran wrote: > On Fri, Sep 25, 2020 at 7:23 PM Cédric Le Goater <clg@kaod.org> wrote: >> >> To fix an issue with PHB hotplug on pSeries machine (HPT/XIVE), commit >> 3a3181e16fbd introduced a PPC specific pcibios_remove_bus() routine to >> clear all interrupt mappings when the bus is removed. This routine >> frees an array allocated in pcibios_scan_phb(). >> >> This broke PHB hotplug on PowerNV because, when a PHB is removed and >> re-scanned through sysfs, the PCI layer un-assigns and re-assigns >> resources to the PHB but does not destroy and recreate the PCI >> controller structure. Since pcibios_remove_bus() does not clear the >> 'irq_map' array pointer, a second removal of the PHB will try to free >> the array a second time and corrupt memory. > > "PHB hotplug" and "hot-plugging devices under a PHB" are different > things. What you're saying here doesn't make a whole lot of sense to > me unless you're conflating the two. The distinction is important > since on pseries we can use DLPAR to add and remove actual PHBs (i.e. > the pci_controller) at runtime, but there's no corresponding mechanism > on PowerNV. And it's even different on QEMU. >> Free the 'irq_map' array in pcibios_free_controller() to fix >> corruption and clear interrupt mapping after it has been >> disposed. This to avoid filling up the array with successive >> remove/rescan of a bus. > > Even with this patch I think we're still broken. With this patch > applied the init path is something like: > > per-phb init: > allocate phb->irq_map array > per-bus init: > nothing > per-device init: > pcibios_bus_add_device() > pci_read_irq_line() > pci_irq_map_register(pci_dev, virq) > *record the device's interrupt in phb->irq_map* > > And the teardown path: > > per-device teardown: > nothing > per-bus teardown: > pcibios_remove_bus() > pci_irq_map_dispose() > *walk phb->irq_map and dispose of each mapped interrupt* > per-phb teardown: > free(phb->irq_map) > > There's a lot of asymmetry here, which is a problem in itself, but the > real issue is that when removing *any* pci_bus under a PHB we dispose > of the LSI\ for *every* device under that PHB. Not good. > > Ideally we should be fixing this by having the per-device teardown > handle disposing the mapping. Unfortunately, there's no pcibios hook > that's called when removing a pci_dev. However, we can register a bus > notifier which will be called when the pci_dev is removed from its bus > and we already do that for the per-device EEH teardown and to handle > IOMMU TCE invalidation when the device is removed. I lack the knowledge here and I think some else should take over, as I am not doing a good job. Michael, can you drop the initial patch again :/ It is better not to merge anything. Thanks, C. ^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [PATCH] powerpc/pci: Fix PHB removal/rescan on PowerNV 2020-10-08 4:37 ` Cédric Le Goater @ 2020-10-14 14:24 ` Greg Kurz 0 siblings, 0 replies; 3+ messages in thread From: Greg Kurz @ 2020-10-14 14:24 UTC (permalink / raw) To: Cédric Le Goater Cc: Oliver O'Halloran, Alexey Kardashevskiy, linuxppc-dev, linux-pci On Thu, 8 Oct 2020 06:37:02 +0200 Cédric Le Goater <clg@kaod.org> wrote: > On 10/8/20 4:23 AM, Oliver O'Halloran wrote: > > On Fri, Sep 25, 2020 at 7:23 PM Cédric Le Goater <clg@kaod.org> wrote: > >> > >> To fix an issue with PHB hotplug on pSeries machine (HPT/XIVE), commit > >> 3a3181e16fbd introduced a PPC specific pcibios_remove_bus() routine to > >> clear all interrupt mappings when the bus is removed. This routine > >> frees an array allocated in pcibios_scan_phb(). > >> > >> This broke PHB hotplug on PowerNV because, when a PHB is removed and > >> re-scanned through sysfs, the PCI layer un-assigns and re-assigns > >> resources to the PHB but does not destroy and recreate the PCI > >> controller structure. Since pcibios_remove_bus() does not clear the > >> 'irq_map' array pointer, a second removal of the PHB will try to free > >> the array a second time and corrupt memory. > > > > "PHB hotplug" and "hot-plugging devices under a PHB" are different > > things. What you're saying here doesn't make a whole lot of sense to > > me unless you're conflating the two. The distinction is important > > since on pseries we can use DLPAR to add and remove actual PHBs (i.e. > > the pci_controller) at runtime, but there's no corresponding mechanism > > on PowerNV. > > And it's even different on QEMU. > If the real HW doesn't have the notion of adding/removing a PHB at runtime, then QEMU should stick to that, ie. setting dc->hotpluggable to false for PNV PHB device types. > >> Free the 'irq_map' array in pcibios_free_controller() to fix > >> corruption and clear interrupt mapping after it has been > >> disposed. This to avoid filling up the array with successive > >> remove/rescan of a bus. > > > > Even with this patch I think we're still broken. With this patch > > applied the init path is something like: > > > > per-phb init: > > allocate phb->irq_map array > > per-bus init: > > nothing > > per-device init: > > pcibios_bus_add_device() > > pci_read_irq_line() > > pci_irq_map_register(pci_dev, virq) > > *record the device's interrupt in phb->irq_map* > > > > And the teardown path: > > > > per-device teardown: > > nothing > > per-bus teardown: > > pcibios_remove_bus() > > pci_irq_map_dispose() > > *walk phb->irq_map and dispose of each mapped interrupt* > > per-phb teardown: > > free(phb->irq_map) > > > > There's a lot of asymmetry here, which is a problem in itself, but the > > real issue is that when removing *any* pci_bus under a PHB we dispose > > of the LSI\ for *every* device under that PHB. Not good. > > > > Ideally we should be fixing this by having the per-device teardown > > handle disposing the mapping. Unfortunately, there's no pcibios hook > > that's called when removing a pci_dev. However, we can register a bus > > notifier which will be called when the pci_dev is removed from its bus > > and we already do that for the per-device EEH teardown and to handle > > IOMMU TCE invalidation when the device is removed. > > I lack the knowledge here and I think some else should take over, > as I am not doing a good job. > > Michael, can you drop the initial patch again :/ It is better not > to merge anything. > > Thanks, > > C. > > ^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2020-10-14 14:31 UTC | newest] Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- [not found] <20200925092258.525079-1-clg@kaod.org> 2020-10-08 2:23 ` [PATCH] powerpc/pci: Fix PHB removal/rescan on PowerNV Oliver O'Halloran 2020-10-08 4:37 ` Cédric Le Goater 2020-10-14 14:24 ` Greg Kurz
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).