On Fri, Feb 25, 2022 at 09:18:30AM +0100, Igor Mammedov wrote:
> On Thu, 24 Feb 2022 13:05:07 -0500
> "Michael S. Tsirkin" <m...@redhat.com> wrote:
>
> > On Thu, Feb 24, 2022 at 12:44:09PM -0500, Igor Mammedov wrote:
> > > on creation a PCIDevice has power turned on at the end of
> > > pci_qdev_realize()
> > > however later on if PCIe slot isn't populated with any children
> > > it's power is turned off. It's fine if native hotplug is used
> > > as plug callback will power slot on among other things.
> > > However when ACPI hotplug is enabled it replaces native PCIe plug
> > > callbacks with ACPI specific ones (acpi_pcihp_device_*plug_cb) and
> > > as result slot stays powered off. It works fine as ACPI hotplug
> > > on guest side takes care of enumerating/initializing hotplugged
> > > device. But when later guest is migrated, call chain introduced by [1]
> > >
> > > pcie_cap_slot_post_load()
> > > -> pcie_cap_update_power()
> > > -> pcie_set_power_device()
> > > -> pci_set_power()
> > > -> pci_update_mappings()
> > >
> > > will disable earlier initialized BARs for the hotplugged device
> > > in powered off slot due to commit [2] which disables BARs if
> > > power is off. As result guest OS after migration will be very
> > > much confused [3], still thinking that it has working device,
> > > which isn't true anymore due to disabled BARs.
> > >
> > > Fix it by honoring PCI_EXP_SLTCAP_PCP and updating power status
> > > only if capability is enabled. Follow up patch will disable
> > > PCI_EXP_SLTCAP_PCP overriding COMPAT_PROP_PCP property when
> > > PCIe slot is under ACPI PCI hotplug control.
> > >
> > > See [3] for reproducer.
> > >
> > > 1)
> > > Fixes: commit d5daff7d312 (pcie: implement slot power control for pcie
> > > root ports)
> > > 2)
> > > commit 23786d13441 (pci: implement power state)
> > > 3)
> > > Bugzilla: https://bugzilla.redhat.com/show_bug.cgi?id=2053584
> > >
> >
> >
> > Correct format for the last paragraph:
> >
> >
> > Fixes: d5daff7d312 ("pcie: implement slot power control for pcie root
> > ports")
> > Fixes: 23786d13441 ("pci: implement power state")
> > Fixes: https://bugzilla.redhat.com/show_bug.cgi?id=2053584
>
> ok, will fix it up on respin like this to have references:
>
> 1)
> Fixes: d5daff7d312 ("pcie: implement slot power control for pcie root ports")
> 2)
> Fixes: 23786d13441 ("pci: implement power state")
> Fixes: https://bugzilla.redhat.com/show_bug.cgi?id=2053584
Just drop references, a bit of duplication is not a problem. E.g.
in powered off slot due to commit 23786d13441 ("pci: implement power state")
which disables BARs if
Trailer tags belong in a group at the end with no interruptions, not all
tools handle them otherwise.
> >
> > > Signed-off-by: Igor Mammedov <imamm...@redhat.com>
> > > ---
> > > hw/pci/pcie.c | 5 ++---
> > > 1 file changed, 2 insertions(+), 3 deletions(-)
> > >
> > > diff --git a/hw/pci/pcie.c b/hw/pci/pcie.c
> > > index d7d73a31e4..2339729a7c 100644
> > > --- a/hw/pci/pcie.c
> > > +++ b/hw/pci/pcie.c
> > > @@ -383,10 +383,9 @@ static void pcie_cap_update_power(PCIDevice
> > > *hotplug_dev)
> > >
> > > if (sltcap & PCI_EXP_SLTCAP_PCP) {
> > > power = (sltctl & PCI_EXP_SLTCTL_PCC) == PCI_EXP_SLTCTL_PWR_ON;
> > > + pci_for_each_device(sec_bus, pci_bus_num(sec_bus),
> > > + pcie_set_power_device, &power);
> > > }
> > > -
> > > - pci_for_each_device(sec_bus, pci_bus_num(sec_bus),
> > > - pcie_set_power_device, &power);
> >
> > I think this is correct. However, I wonder whether for 6.2 compatiblity
> > as a hack we should sometimes skip the power update even when
> > PCI_EXP_SLTCAP_PCP exists. Will that not work around the issue for
> > these machine types?
>
> pc-q35-6.2 is broken utterly.
> With pc-q35-6.1, it's a mess. Here is a ping-pong migration matrix for it
>
> v6.1 | v6.2 | Fix
> v6.1 ok | broken | ok (#1)
> v6.2 | broken | broken (#2)
>
> [1] has PCI_EXP_SLTCAP_PCP due to x-pcihp-enable-pcie-pcp-cap=on
> i.e. pci_config is exactly the same as in qemu-v6.1
> [2] PCI_EXP_SLTCAP_PCP is enabled + empty slot is powered off
> (+ state is migrated)
>
> there are some invariants that might work in one direction,
> but it won't survive ping-pong migration. And more importantly
> for upstream we care mostly care for old -> new working,
> and it's direction that is broken in v6.2.
>
> > And assuming we want bug for bug compat anyway, why not just put
> > it here? It seems easier to reason about frankly ...
>
> It should be possible hack PCI core to fixup broken power state
> on incoming migration at (at postload time), but that would just
> create more confusion, where in some cases migration would work
> and in some would not (depending on used qemu versions).
>
> Lets just declare v6.2 qemu broken, with upgrade/downgrade to
> (7.0/6.1) as suggested solution.
>
> PS:
> I'd very much prefer avoid adding hacks for ACPI pcihp sake to
> PCI core, and let PCI code behave as it's supposed to per spec.
> It's already bad enough with pcihp layered on top of PCI,
> making PCI code depend on pcihp will just make it more fragile.
>
> > > }
> > >
> > > /*
> > > --
> > > 2.31.1
> >
