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" <[email protected]> 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: [email protected] > > > CC: [email protected] > > > > > > 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 > >
