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

[Igor Mammedov]

On Fri, 25 Feb 2022 08:08:57 -0500
"Michael S. Tsirkin" <mst@redhat.com> wrote:

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

with some reworks it should work (i.e. adding an extra knob that will tell
PCI core not to power off when it should, looks fragile and very hacky).
It has the same migration implications as this patch, so I'd rather go
after disabling whole SLTCAP_PCP thing to be correct and keeping PCI
code free from ACPI hacks.

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