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

[Michael S. Tsirkin]

On Fri, Feb 25, 2022 at 02:02:31PM +0100, Igor Mammedov wrote:
> 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.

So how about when ACPI pcihp overrides native with its callbacks we also
set PCC power to on?

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