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

[Igor Mammedov <imammedo@redhat.com>]

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    
> >   
>