Re: [PATCH 0/4] Fix broken PCIe device after migration

[Igor Mammedov <imammedo@redhat.com>]

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  
>