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

[Michael S. Tsirkin]

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

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

And assuming we want bug for bug compat anyway, why not just put
it here? It seems easier to reason about frankly ...

>  }
>  
>  /*
> -- 
> 2.31.1