| Message ID | 20220224174411.3296848-1-imammedo@redhat.com (mailing list archive) |
|---|---|
| Headers | show |
| Series | Fix broken PCIe device after migration | expand |
On Thu, Feb 24, 2022 at 12:44:07PM -0500, Igor Mammedov wrote: > Currently ACPI PCI hotplug is enabled by default for Q35 machine > type and overrides native PCIe hotplug. It works as expected when > a PCIe device is hotplugged into slot, however the device becomes > in-operational after migration. Which is caused by BARs being > disabled on target due to powered off status of the slot. > > Proposed fix disables power control on PCIe slot when ACPI pcihp > takes over hotplug control and makes PCIe slot check if power > control is enabled before trying to change slot's power. Which > leaves slot always powered on and that makes PCI core keep BARs > enabled. > > PS: > it's still hacky approach as all ACPI PCI hotplug is, but it's > the least intrusive one. Alternative, I've considered, could be > chaining hotplug callbacks and making pcihp ones call overriden > native callbacks while inhibiting hotplug event in native callbacks > somehow. But that were a bit more intrusive and spills over to SHPC > if implemented systematically, so I ditched that for now. It could > be resurrected later on if current approach turns out to be > insufficient. Yes, I am wondering about this myself. Replace callbacks with some kind of notifier, so instead of overrides we add things? I will ponder this over the weekend. > RHBZ: https://bugzilla.redhat.com/show_bug.cgi?id=2053584 > CC: mst@redhat.com > CC: kraxel@redhat.com > > Igor Mammedov (4): > pci: expose TYPE_XIO3130_DOWNSTREAM name > pcie: update slot power status only is power control is enabled > acpi: pcihp: disable power control on PCIe slot > q35: compat: keep hotplugged PCIe device broken after migration for > 6.2-older machine types > > include/hw/acpi/pcihp.h | 4 +++- > include/hw/pci-bridge/xio3130_downstream.h | 15 +++++++++++++++ > hw/acpi/acpi-pci-hotplug-stub.c | 3 ++- > hw/acpi/ich9.c | 21 ++++++++++++++++++++- > hw/acpi/pcihp.c | 16 +++++++++++++++- > hw/acpi/piix4.c | 3 ++- > hw/core/machine.c | 4 +++- > hw/pci-bridge/xio3130_downstream.c | 3 ++- > hw/pci/pcie.c | 5 ++--- > 9 files changed, 64 insertions(+), 10 deletions(-) > create mode 100644 include/hw/pci-bridge/xio3130_downstream.h > > -- > 2.31.1
On Thu, 24 Feb 2022 13:08:11 -0500 "Michael S. Tsirkin" <mst@redhat.com> wrote: > On Thu, Feb 24, 2022 at 12:44:07PM -0500, Igor Mammedov wrote: > > Currently ACPI PCI hotplug is enabled by default for Q35 machine > > type and overrides native PCIe hotplug. It works as expected when > > a PCIe device is hotplugged into slot, however the device becomes > > in-operational after migration. Which is caused by BARs being > > disabled on target due to powered off status of the slot. > > > > Proposed fix disables power control on PCIe slot when ACPI pcihp > > takes over hotplug control and makes PCIe slot check if power > > control is enabled before trying to change slot's power. Which > > leaves slot always powered on and that makes PCI core keep BARs > > enabled. > > > > PS: > > it's still hacky approach as all ACPI PCI hotplug is, but it's > > the least intrusive one. Alternative, I've considered, could be > > chaining hotplug callbacks and making pcihp ones call overriden > > native callbacks while inhibiting hotplug event in native callbacks > > somehow. But that were a bit more intrusive and spills over to SHPC > > if implemented systematically, so I ditched that for now. It could > > be resurrected later on if current approach turns out to be > > insufficient. > > Yes, I am wondering about this myself. Replace callbacks with > some kind of notifier, so instead of overrides we add things? > I will ponder this over the weekend. Callback overrides with optional chaining worked fine so far, Chaining is just a bit more complicated as often one need to refactor old code on pre and plug stages and think about how to partition job between involved components. But they are very explicit about what's supposed to call what and in what order and graceful error handling. And I dislike notifiers exactly for the lack of those properties and more difficult from review pov. I think [ATM] for this issue chaining callbacks is overkill, but maybe in future it can be done if an idea of having PCI slots described in ACPI + native hotplug proves to be a viable. > > RHBZ: https://bugzilla.redhat.com/show_bug.cgi?id=2053584 > > CC: mst@redhat.com > > CC: kraxel@redhat.com > > > > Igor Mammedov (4): > > pci: expose TYPE_XIO3130_DOWNSTREAM name > > pcie: update slot power status only is power control is enabled > > acpi: pcihp: disable power control on PCIe slot > > q35: compat: keep hotplugged PCIe device broken after migration for > > 6.2-older machine types > > > > include/hw/acpi/pcihp.h | 4 +++- > > include/hw/pci-bridge/xio3130_downstream.h | 15 +++++++++++++++ > > hw/acpi/acpi-pci-hotplug-stub.c | 3 ++- > > hw/acpi/ich9.c | 21 ++++++++++++++++++++- > > hw/acpi/pcihp.c | 16 +++++++++++++++- > > hw/acpi/piix4.c | 3 ++- > > hw/core/machine.c | 4 +++- > > hw/pci-bridge/xio3130_downstream.c | 3 ++- > > hw/pci/pcie.c | 5 ++--- > > 9 files changed, 64 insertions(+), 10 deletions(-) > > create mode 100644 include/hw/pci-bridge/xio3130_downstream.h > > > > -- > > 2.31.1 >
On Thu, Feb 24, 2022 at 12:44:07PM -0500, Igor Mammedov wrote: > Currently ACPI PCI hotplug is enabled by default for Q35 machine > type and overrides native PCIe hotplug. It works as expected when > a PCIe device is hotplugged into slot, however the device becomes > in-operational after migration. Which is caused by BARs being > disabled on target due to powered off status of the slot. > > Proposed fix disables power control on PCIe slot when ACPI pcihp > takes over hotplug control and makes PCIe slot check if power > control is enabled before trying to change slot's power. Which > leaves slot always powered on and that makes PCI core keep BARs > enabled. I thought some more about this. One of the reasons we did not remove the hotplug capability is really so it's easier to layer acpi on top of pcihp if we want to do it down the road. And it would be quite annoying if we had to add more hack to go back to having capability. How about instead of patch 3 we call pci_set_power(dev, true) for all devices where ACPI is managing hotplug immediately on plug? This will fix the bug avoiding headache with migration. Patch 2 does seem like a good idea. > PS: > it's still hacky approach as all ACPI PCI hotplug is, but it's > the least intrusive one. Alternative, I've considered, could be > chaining hotplug callbacks and making pcihp ones call overriden > native callbacks while inhibiting hotplug event in native callbacks > somehow. But that were a bit more intrusive and spills over to SHPC > if implemented systematically, so I ditched that for now. It could > be resurrected later on if current approach turns out to be > insufficient. > > RHBZ: https://bugzilla.redhat.com/show_bug.cgi?id=2053584 > CC: mst@redhat.com > CC: kraxel@redhat.com > > Igor Mammedov (4): > pci: expose TYPE_XIO3130_DOWNSTREAM name > pcie: update slot power status only is power control is enabled > acpi: pcihp: disable power control on PCIe slot > q35: compat: keep hotplugged PCIe device broken after migration for > 6.2-older machine types > > include/hw/acpi/pcihp.h | 4 +++- > include/hw/pci-bridge/xio3130_downstream.h | 15 +++++++++++++++ > hw/acpi/acpi-pci-hotplug-stub.c | 3 ++- > hw/acpi/ich9.c | 21 ++++++++++++++++++++- > hw/acpi/pcihp.c | 16 +++++++++++++++- > hw/acpi/piix4.c | 3 ++- > hw/core/machine.c | 4 +++- > hw/pci-bridge/xio3130_downstream.c | 3 ++- > hw/pci/pcie.c | 5 ++--- > 9 files changed, 64 insertions(+), 10 deletions(-) > create mode 100644 include/hw/pci-bridge/xio3130_downstream.h > > -- > 2.31.1
On Fri, 25 Feb 2022 04:58:46 -0500 "Michael S. Tsirkin" <mst@redhat.com> wrote: > On Thu, Feb 24, 2022 at 12:44:07PM -0500, Igor Mammedov wrote: > > Currently ACPI PCI hotplug is enabled by default for Q35 machine > > type and overrides native PCIe hotplug. It works as expected when > > a PCIe device is hotplugged into slot, however the device becomes > > in-operational after migration. Which is caused by BARs being > > disabled on target due to powered off status of the slot. > > > > Proposed fix disables power control on PCIe slot when ACPI pcihp > > takes over hotplug control and makes PCIe slot check if power > > control is enabled before trying to change slot's power. Which > > leaves slot always powered on and that makes PCI core keep BARs > > enabled. > > > I thought some more about this. One of the reasons we > did not remove the hotplug capability is really so > it's easier to layer acpi on top of pcihp if we > want to do it down the road. And it would be quite annoying > if we had to add more hack to go back to having capability. > > How about instead of patch 3 we call pci_set_power(dev, true) for all > devices where ACPI is managing hotplug immediately on plug? This will > fix the bug avoiding headache with migration. true it would be more migration friendly (v6.2 still broken but that can't be helped), since we won't alter pci_config at all. Although it's still more hackish compared to disabling SLTCAP_PCP (though it seems guest OSes have no issues with SLTCAP_PCP being present but not really operational, at least for ~6months the thing was released (6.1-6.2-now)). Let me play with this idea and see if it works and at what cost, though I still prefer cleaner SLTCAP_PCP disabling to make sure guest OS won't get wrong idea about power control being present when it's not actually not. > Patch 2 does seem like a good idea. > > > PS: > > it's still hacky approach as all ACPI PCI hotplug is, but it's > > the least intrusive one. Alternative, I've considered, could be > > chaining hotplug callbacks and making pcihp ones call overriden > > native callbacks while inhibiting hotplug event in native callbacks > > somehow. But that were a bit more intrusive and spills over to SHPC > > if implemented systematically, so I ditched that for now. It could > > be resurrected later on if current approach turns out to be > > insufficient. > > > > RHBZ: https://bugzilla.redhat.com/show_bug.cgi?id=2053584 > > CC: mst@redhat.com > > CC: kraxel@redhat.com > > > > Igor Mammedov (4): > > pci: expose TYPE_XIO3130_DOWNSTREAM name > > pcie: update slot power status only is power control is enabled > > acpi: pcihp: disable power control on PCIe slot > > q35: compat: keep hotplugged PCIe device broken after migration for > > 6.2-older machine types > > > > include/hw/acpi/pcihp.h | 4 +++- > > include/hw/pci-bridge/xio3130_downstream.h | 15 +++++++++++++++ > > hw/acpi/acpi-pci-hotplug-stub.c | 3 ++- > > hw/acpi/ich9.c | 21 ++++++++++++++++++++- > > hw/acpi/pcihp.c | 16 +++++++++++++++- > > hw/acpi/piix4.c | 3 ++- > > hw/core/machine.c | 4 +++- > > hw/pci-bridge/xio3130_downstream.c | 3 ++- > > hw/pci/pcie.c | 5 ++--- > > 9 files changed, 64 insertions(+), 10 deletions(-) > > create mode 100644 include/hw/pci-bridge/xio3130_downstream.h > > > > -- > > 2.31.1 >
On Fri, Feb 25, 2022 at 02:18:23PM +0100, Igor Mammedov wrote: > On Fri, 25 Feb 2022 04:58:46 -0500 > "Michael S. Tsirkin" <mst@redhat.com> wrote: > > > On Thu, Feb 24, 2022 at 12:44:07PM -0500, Igor Mammedov wrote: > > > Currently ACPI PCI hotplug is enabled by default for Q35 machine > > > type and overrides native PCIe hotplug. It works as expected when > > > a PCIe device is hotplugged into slot, however the device becomes > > > in-operational after migration. Which is caused by BARs being > > > disabled on target due to powered off status of the slot. > > > > > > Proposed fix disables power control on PCIe slot when ACPI pcihp > > > takes over hotplug control and makes PCIe slot check if power > > > control is enabled before trying to change slot's power. Which > > > leaves slot always powered on and that makes PCI core keep BARs > > > enabled. > > > > > > I thought some more about this. One of the reasons we > > did not remove the hotplug capability is really so > > it's easier to layer acpi on top of pcihp if we > > want to do it down the road. And it would be quite annoying > > if we had to add more hack to go back to having capability. > > > > How about instead of patch 3 we call pci_set_power(dev, true) for all > > devices where ACPI is managing hotplug immediately on plug? This will > > fix the bug avoiding headache with migration. > > true it would be more migration friendly (v6.2 still broken > but that can't be helped), since we won't alter pci_config at all. > Although it's still more hackish compared to disabling SLTCAP_PCP > (though it seems guest OSes have no issues with SLTCAP_PCP being > present but not really operational, at least for ~6months the thing > was released (6.1-6.2-now)). > > Let me play with this idea and see if it works and at what cost, > though I still prefer cleaner SLTCAP_PCP disabling to make sure > guest OS won't get wrong idea about power control being present > when it's not actually not. Well the control is present, isn't it? Can be used to e.g. reset the device behind the bridge. > > > Patch 2 does seem like a good idea. > > > > > PS: > > > it's still hacky approach as all ACPI PCI hotplug is, but it's > > > the least intrusive one. Alternative, I've considered, could be > > > chaining hotplug callbacks and making pcihp ones call overriden > > > native callbacks while inhibiting hotplug event in native callbacks > > > somehow. But that were a bit more intrusive and spills over to SHPC > > > if implemented systematically, so I ditched that for now. It could > > > be resurrected later on if current approach turns out to be > > > insufficient. > > > > > > RHBZ: https://bugzilla.redhat.com/show_bug.cgi?id=2053584 > > > CC: mst@redhat.com > > > CC: kraxel@redhat.com > > > > > > Igor Mammedov (4): > > > pci: expose TYPE_XIO3130_DOWNSTREAM name > > > pcie: update slot power status only is power control is enabled > > > acpi: pcihp: disable power control on PCIe slot > > > q35: compat: keep hotplugged PCIe device broken after migration for > > > 6.2-older machine types > > > > > > include/hw/acpi/pcihp.h | 4 +++- > > > include/hw/pci-bridge/xio3130_downstream.h | 15 +++++++++++++++ > > > hw/acpi/acpi-pci-hotplug-stub.c | 3 ++- > > > hw/acpi/ich9.c | 21 ++++++++++++++++++++- > > > hw/acpi/pcihp.c | 16 +++++++++++++++- > > > hw/acpi/piix4.c | 3 ++- > > > hw/core/machine.c | 4 +++- > > > hw/pci-bridge/xio3130_downstream.c | 3 ++- > > > hw/pci/pcie.c | 5 ++--- > > > 9 files changed, 64 insertions(+), 10 deletions(-) > > > create mode 100644 include/hw/pci-bridge/xio3130_downstream.h > > > > > > -- > > > 2.31.1 > >
On Fri, 25 Feb 2022 14:18:23 +0100 Igor Mammedov <imammedo@redhat.com> wrote: > On Fri, 25 Feb 2022 04:58:46 -0500 > "Michael S. Tsirkin" <mst@redhat.com> wrote: > > > On Thu, Feb 24, 2022 at 12:44:07PM -0500, Igor Mammedov wrote: > > > Currently ACPI PCI hotplug is enabled by default for Q35 machine > > > type and overrides native PCIe hotplug. It works as expected when > > > a PCIe device is hotplugged into slot, however the device becomes > > > in-operational after migration. Which is caused by BARs being > > > disabled on target due to powered off status of the slot. > > > > > > Proposed fix disables power control on PCIe slot when ACPI pcihp > > > takes over hotplug control and makes PCIe slot check if power > > > control is enabled before trying to change slot's power. Which > > > leaves slot always powered on and that makes PCI core keep BARs > > > enabled. > > > > > > I thought some more about this. One of the reasons we > > did not remove the hotplug capability is really so > > it's easier to layer acpi on top of pcihp if we > > want to do it down the road. And it would be quite annoying > > if we had to add more hack to go back to having capability. > > > > How about instead of patch 3 we call pci_set_power(dev, true) for all > > devices where ACPI is managing hotplug immediately on plug? This will > > fix the bug avoiding headache with migration. > > true it would be more migration friendly (v6.2 still broken > but that can't be helped), since we won't alter pci_config at all. > Although it's still more hackish compared to disabling SLTCAP_PCP > (though it seems guest OSes have no issues with SLTCAP_PCP being > present but not really operational, at least for ~6months the thing > was released (6.1-6.2-now)). > > Let me play with this idea and see if it works and at what cost, > though I still prefer cleaner SLTCAP_PCP disabling to make sure > guest OS won't get wrong idea about power control being present > when it's not actually not. It's not going to work, plug time is not the problem here. The hot-plugged device already ends up in power on state by default. It's later on target pcie_cap_slot_post_load() updates its power to 'off' due to sltctl & SLTCTL_PCC == 400 options if we go fixup route are 1) from ich9_pm_post_load() scan PCI hierarchy and do fixup it's currently called after root port pcie_cap_slot_reset() in simple test case. It's sketchy and fragile to rely on particular load() order, if it's somehow changed in future it will (silently) break this dependency. 2) do fixup from pcie_cap_slot_post_load(), means polluting generic PCI code with APCI hotplug idiosyncrasies. I thought you would be the first one to shoot that down. I guess, I pretty much convinced myself that instead of fixing up broken SLTCTL_PCC on the fly, it's more correct to fix the root issue (non functional power control) on source by disabling it completely. Quick smoke testing with Linux(FC14) and Windows (2012r2) shows it works as expected. Alternative idea of chaining native pcie callbacks into acpi ones, looks inferior as well since we would have pollute native ones with ACPI logic to inhibit native hotplug event while keeping its power mgmt part working. > > Patch 2 does seem like a good idea. > > > > > PS: > > > it's still hacky approach as all ACPI PCI hotplug is, but it's > > > the least intrusive one. Alternative, I've considered, could be > > > chaining hotplug callbacks and making pcihp ones call overriden > > > native callbacks while inhibiting hotplug event in native callbacks > > > somehow. But that were a bit more intrusive and spills over to SHPC > > > if implemented systematically, so I ditched that for now. It could > > > be resurrected later on if current approach turns out to be > > > insufficient. > > > > > > RHBZ: https://bugzilla.redhat.com/show_bug.cgi?id=2053584 > > > CC: mst@redhat.com > > > CC: kraxel@redhat.com > > > > > > Igor Mammedov (4): > > > pci: expose TYPE_XIO3130_DOWNSTREAM name > > > pcie: update slot power status only is power control is enabled > > > acpi: pcihp: disable power control on PCIe slot > > > q35: compat: keep hotplugged PCIe device broken after migration for > > > 6.2-older machine types > > > > > > include/hw/acpi/pcihp.h | 4 +++- > > > include/hw/pci-bridge/xio3130_downstream.h | 15 +++++++++++++++ > > > hw/acpi/acpi-pci-hotplug-stub.c | 3 ++- > > > hw/acpi/ich9.c | 21 ++++++++++++++++++++- > > > hw/acpi/pcihp.c | 16 +++++++++++++++- > > > hw/acpi/piix4.c | 3 ++- > > > hw/core/machine.c | 4 +++- > > > hw/pci-bridge/xio3130_downstream.c | 3 ++- > > > hw/pci/pcie.c | 5 ++--- > > > 9 files changed, 64 insertions(+), 10 deletions(-) > > > create mode 100644 include/hw/pci-bridge/xio3130_downstream.h > > > > > > -- > > > 2.31.1 > > >
On Fri, 25 Feb 2022 08:50:43 -0500 "Michael S. Tsirkin" <mst@redhat.com> wrote: > On Fri, Feb 25, 2022 at 02:18:23PM +0100, Igor Mammedov wrote: > > On Fri, 25 Feb 2022 04:58:46 -0500 > > "Michael S. Tsirkin" <mst@redhat.com> wrote: > > > > > On Thu, Feb 24, 2022 at 12:44:07PM -0500, Igor Mammedov wrote: > > > > Currently ACPI PCI hotplug is enabled by default for Q35 machine > > > > type and overrides native PCIe hotplug. It works as expected when > > > > a PCIe device is hotplugged into slot, however the device becomes > > > > in-operational after migration. Which is caused by BARs being > > > > disabled on target due to powered off status of the slot. > > > > > > > > Proposed fix disables power control on PCIe slot when ACPI pcihp > > > > takes over hotplug control and makes PCIe slot check if power > > > > control is enabled before trying to change slot's power. Which > > > > leaves slot always powered on and that makes PCI core keep BARs > > > > enabled. > > > > > > > > > I thought some more about this. One of the reasons we > > > did not remove the hotplug capability is really so > > > it's easier to layer acpi on top of pcihp if we > > > want to do it down the road. And it would be quite annoying > > > if we had to add more hack to go back to having capability. > > > > > > How about instead of patch 3 we call pci_set_power(dev, true) for all > > > devices where ACPI is managing hotplug immediately on plug? This will > > > fix the bug avoiding headache with migration. > > > > true it would be more migration friendly (v6.2 still broken > > but that can't be helped), since we won't alter pci_config at all. > > Although it's still more hackish compared to disabling SLTCAP_PCP > > (though it seems guest OSes have no issues with SLTCAP_PCP being > > present but not really operational, at least for ~6months the thing > > was released (6.1-6.2-now)). > > > > Let me play with this idea and see if it works and at what cost, > > though I still prefer cleaner SLTCAP_PCP disabling to make sure > > guest OS won't get wrong idea about power control being present > > when it's not actually not. > > Well the control is present, isn't it? Can be used to e.g. reset the > device behind the bridge. can you point to how reset is supposed to work? > > > > > > Patch 2 does seem like a good idea. > > > > > > > PS: > > > > it's still hacky approach as all ACPI PCI hotplug is, but it's > > > > the least intrusive one. Alternative, I've considered, could be > > > > chaining hotplug callbacks and making pcihp ones call overriden > > > > native callbacks while inhibiting hotplug event in native callbacks > > > > somehow. But that were a bit more intrusive and spills over to SHPC > > > > if implemented systematically, so I ditched that for now. It could > > > > be resurrected later on if current approach turns out to be > > > > insufficient. > > > > > > > > RHBZ: https://bugzilla.redhat.com/show_bug.cgi?id=2053584 > > > > CC: mst@redhat.com > > > > CC: kraxel@redhat.com > > > > > > > > Igor Mammedov (4): > > > > pci: expose TYPE_XIO3130_DOWNSTREAM name > > > > pcie: update slot power status only is power control is enabled > > > > acpi: pcihp: disable power control on PCIe slot > > > > q35: compat: keep hotplugged PCIe device broken after migration for > > > > 6.2-older machine types > > > > > > > > include/hw/acpi/pcihp.h | 4 +++- > > > > include/hw/pci-bridge/xio3130_downstream.h | 15 +++++++++++++++ > > > > hw/acpi/acpi-pci-hotplug-stub.c | 3 ++- > > > > hw/acpi/ich9.c | 21 ++++++++++++++++++++- > > > > hw/acpi/pcihp.c | 16 +++++++++++++++- > > > > hw/acpi/piix4.c | 3 ++- > > > > hw/core/machine.c | 4 +++- > > > > hw/pci-bridge/xio3130_downstream.c | 3 ++- > > > > hw/pci/pcie.c | 5 ++--- > > > > 9 files changed, 64 insertions(+), 10 deletions(-) > > > > create mode 100644 include/hw/pci-bridge/xio3130_downstream.h > > > > > > > > -- > > > > 2.31.1 > > > >
On Fri, Feb 25, 2022 at 04:50:54PM +0100, Igor Mammedov wrote: > On Fri, 25 Feb 2022 08:50:43 -0500 > "Michael S. Tsirkin" <mst@redhat.com> wrote: > > > On Fri, Feb 25, 2022 at 02:18:23PM +0100, Igor Mammedov wrote: > > > On Fri, 25 Feb 2022 04:58:46 -0500 > > > "Michael S. Tsirkin" <mst@redhat.com> wrote: > > > > > > > On Thu, Feb 24, 2022 at 12:44:07PM -0500, Igor Mammedov wrote: > > > > > Currently ACPI PCI hotplug is enabled by default for Q35 machine > > > > > type and overrides native PCIe hotplug. It works as expected when > > > > > a PCIe device is hotplugged into slot, however the device becomes > > > > > in-operational after migration. Which is caused by BARs being > > > > > disabled on target due to powered off status of the slot. > > > > > > > > > > Proposed fix disables power control on PCIe slot when ACPI pcihp > > > > > takes over hotplug control and makes PCIe slot check if power > > > > > control is enabled before trying to change slot's power. Which > > > > > leaves slot always powered on and that makes PCI core keep BARs > > > > > enabled. > > > > > > > > > > > > I thought some more about this. One of the reasons we > > > > did not remove the hotplug capability is really so > > > > it's easier to layer acpi on top of pcihp if we > > > > want to do it down the road. And it would be quite annoying > > > > if we had to add more hack to go back to having capability. > > > > > > > > How about instead of patch 3 we call pci_set_power(dev, true) for all > > > > devices where ACPI is managing hotplug immediately on plug? This will > > > > fix the bug avoiding headache with migration. > > > > > > true it would be more migration friendly (v6.2 still broken > > > but that can't be helped), since we won't alter pci_config at all. > > > Although it's still more hackish compared to disabling SLTCAP_PCP > > > (though it seems guest OSes have no issues with SLTCAP_PCP being > > > present but not really operational, at least for ~6months the thing > > > was released (6.1-6.2-now)). > > > > > > Let me play with this idea and see if it works and at what cost, > > > though I still prefer cleaner SLTCAP_PCP disabling to make sure > > > guest OS won't get wrong idea about power control being present > > > when it's not actually not. > > > > Well the control is present, isn't it? Can be used to e.g. reset the > > device behind the bridge. > > can you point to how reset is supposed to work? Well, I am alluding to this code in linux static const struct pci_reset_fn_method pci_reset_fn_methods[] = { { }, { pci_dev_specific_reset, .name = "device_specific" }, { pci_dev_acpi_reset, .name = "acpi" }, { pcie_reset_flr, .name = "flr" }, { pci_af_flr, .name = "af_flr" }, { pci_pm_reset, .name = "pm" }, { pci_reset_bus_function, .name = "bus" }, }; Thinkably down the road linux could add a method powering the secondary bus off then back on as a way to reset devices behind it. There are plenty of other ways so it's not that I can say why that specific way of doing it is useful. > > > > > > > > > Patch 2 does seem like a good idea. > > > > > > > > > PS: > > > > > it's still hacky approach as all ACPI PCI hotplug is, but it's > > > > > the least intrusive one. Alternative, I've considered, could be > > > > > chaining hotplug callbacks and making pcihp ones call overriden > > > > > native callbacks while inhibiting hotplug event in native callbacks > > > > > somehow. But that were a bit more intrusive and spills over to SHPC > > > > > if implemented systematically, so I ditched that for now. It could > > > > > be resurrected later on if current approach turns out to be > > > > > insufficient. > > > > > > > > > > RHBZ: https://bugzilla.redhat.com/show_bug.cgi?id=2053584 > > > > > CC: mst@redhat.com > > > > > CC: kraxel@redhat.com > > > > > > > > > > Igor Mammedov (4): > > > > > pci: expose TYPE_XIO3130_DOWNSTREAM name > > > > > pcie: update slot power status only is power control is enabled > > > > > acpi: pcihp: disable power control on PCIe slot > > > > > q35: compat: keep hotplugged PCIe device broken after migration for > > > > > 6.2-older machine types > > > > > > > > > > include/hw/acpi/pcihp.h | 4 +++- > > > > > include/hw/pci-bridge/xio3130_downstream.h | 15 +++++++++++++++ > > > > > hw/acpi/acpi-pci-hotplug-stub.c | 3 ++- > > > > > hw/acpi/ich9.c | 21 ++++++++++++++++++++- > > > > > hw/acpi/pcihp.c | 16 +++++++++++++++- > > > > > hw/acpi/piix4.c | 3 ++- > > > > > hw/core/machine.c | 4 +++- > > > > > hw/pci-bridge/xio3130_downstream.c | 3 ++- > > > > > hw/pci/pcie.c | 5 ++--- > > > > > 9 files changed, 64 insertions(+), 10 deletions(-) > > > > > create mode 100644 include/hw/pci-bridge/xio3130_downstream.h > > > > > > > > > > -- > > > > > 2.31.1 > > > > > >
Hi, > Well the control is present, isn't it? Can be used to e.g. reset the > device behind the bridge. Well, not right now b/c poweroff ejects the device. Would need a patch like this ... --- a/hw/pci/pcie.c +++ b/hw/pci/pcie.c @@ -755,7 +755,8 @@ void pcie_cap_slot_write_config(PCIDevice *dev, if ((sltsta & PCI_EXP_SLTSTA_PDS) && (val & PCI_EXP_SLTCTL_PCC) && (val & PCI_EXP_SLTCTL_PIC_OFF) == PCI_EXP_SLTCTL_PIC_OFF && (!(old_slt_ctl & PCI_EXP_SLTCTL_PCC) || - (old_slt_ctl & PCI_EXP_SLTCTL_PIC_OFF) != PCI_EXP_SLTCTL_PIC_OFF)) { + (old_slt_ctl & PCI_EXP_SLTCTL_PIC_OFF) != PCI_EXP_SLTCTL_PIC_OFF) && + dev->pending_deleted_event) { pcie_cap_slot_do_unplug(dev); } pcie_cap_update_power(dev); ... so pcie unplugs on poweroff only in case there is a pending unplug request. But with that the guest wouldn't be able to unplug devices on its own. take care, Gerd
Currently ACPI PCI hotplug is enabled by default for Q35 machine type and overrides native PCIe hotplug. It works as expected when a PCIe device is hotplugged into slot, however the device becomes in-operational after migration. Which is caused by BARs being disabled on target due to powered off status of the slot. Proposed fix disables power control on PCIe slot when ACPI pcihp takes over hotplug control and makes PCIe slot check if power control is enabled before trying to change slot's power. Which leaves slot always powered on and that makes PCI core keep BARs enabled. PS: it's still hacky approach as all ACPI PCI hotplug is, but it's the least intrusive one. Alternative, I've considered, could be chaining hotplug callbacks and making pcihp ones call overriden native callbacks while inhibiting hotplug event in native callbacks somehow. But that were a bit more intrusive and spills over to SHPC if implemented systematically, so I ditched that for now. It could be resurrected later on if current approach turns out to be insufficient. RHBZ: https://bugzilla.redhat.com/show_bug.cgi?id=2053584 CC: mst@redhat.com CC: kraxel@redhat.com Igor Mammedov (4): pci: expose TYPE_XIO3130_DOWNSTREAM name pcie: update slot power status only is power control is enabled acpi: pcihp: disable power control on PCIe slot q35: compat: keep hotplugged PCIe device broken after migration for 6.2-older machine types include/hw/acpi/pcihp.h | 4 +++- include/hw/pci-bridge/xio3130_downstream.h | 15 +++++++++++++++ hw/acpi/acpi-pci-hotplug-stub.c | 3 ++- hw/acpi/ich9.c | 21 ++++++++++++++++++++- hw/acpi/pcihp.c | 16 +++++++++++++++- hw/acpi/piix4.c | 3 ++- hw/core/machine.c | 4 +++- hw/pci-bridge/xio3130_downstream.c | 3 ++- hw/pci/pcie.c | 5 ++--- 9 files changed, 64 insertions(+), 10 deletions(-) create mode 100644 include/hw/pci-bridge/xio3130_downstream.h