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

[Igor Mammedov <imammedo@redhat.com>]

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  
>