Re: [PATCH 2/4] pcie: update slot power status only is power control is enabled

[Igor Mammedov]

On Thu, 24 Feb 2022 13:05:07 -0500
"Michael S. Tsirkin" <mst@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

> 
> > Signed-off-by: Igor Mammedov <imammedo@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  
>