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

[Michael S. Tsirkin]

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

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