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

[Igor Mammedov <imammedo@redhat.com>]

On Fri, 25 Feb 2022 11:12:59 +0100
Gerd Hoffmann <kraxel@redhat.com> wrote:

>   Hi,
> 
> >    pcie_cap_slot_post_load()  
> >        -> pcie_cap_update_power()
> >            -> pcie_set_power_device()
> >                -> pci_set_power()
> >                    -> pci_update_mappings()  
> 
> > Fix it by honoring PCI_EXP_SLTCAP_PCP and updating power status
> > only if capability is enabled.  
> 
> > 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);
> >  }  
> 
> The change makes sense, although I don't see how that changes qemu
> behavior.

looks like I need to fix commit message

> 
> 'power' defaults to true, so when SLTCAP_PCP is off it should never
> ever try to power off the devices.  And pci_set_power() should figure
> the state didn't change and instantly return without touching the
> device.


SLTCAP_PCP is on by default as well, so empty slot ends up with
power disabled PCC state [1]:

  sltctl & SLTCTL_PCC == 0x400

by the time machine is initialized.

Then ACPI pcihp callbacks override native hotplug ones
so PCC remains stuck in this state since all power control
is out of picture in case of ACPI based hotplug. Guest OS
doesn't use/or ignore native PCC.

After device hotplug, the device stays in has_power=on default
state as pci_qdev_realize() initialized it. [2]

However when migrated SLTCTL_PCC is also migrated, and kick in
as described in commit message and turns power off [3]

> 
> take care,
>   Gerd
> 

1)
pci_qdev_realize
pci_set_power: extra_root0, d->has_power: 0,  new power state: 1
pci_set_power: extra_root0, set has_power to: 1
pcie_cap_slot_reset
pcie_cap_update_power(extra_root0): sltcap & PCI_EXP_SLTCAP_PCP: 2,  sltctl & SLTCTL_PCC: 400
pcie_cap_update_power(extra_root0): updated power: 0  <== has no effect on children since there is none

2)
pci_qdev_realize
pci_set_power: nic, d->has_power: 0,  new power state: 1
pci_set_power: nic, set has_power to: 1


3)
pci_qdev_realize
pci_set_power: extra_root0, d->has_power: 0,  new power state: 1
pci_set_power: extra_root0, set has_power to: 1
pci_qdev_realize
pci_set_power: nic, d->has_power: 0,  new power state: 1
pci_set_power: nic, set has_power to: 1
pcie_cap_slot_reset
pcie_cap_update_power(extra_root0): sltcap & PCI_EXP_SLTCAP_PCP: 2,  sltctl & SLTCTL_PCC: 0
pcie_cap_update_power(extra_root0): updated power: 1
pci_set_power: nic, d->has_power: 1,  new power state: 1
pcie_cap_slot_post_load(extra_root0)
pcie_cap_update_power(extra_root0): sltcap & PCI_EXP_SLTCAP_PCP: 2,  sltctl & SLTCTL_PCC: 400
pcie_cap_update_power(extra_root0): updated power: 0
pci_set_power: nic, d->has_power: 1,  new power state: 0
pci_set_power: nic, set has_power to: 0