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:35:28PM +0100, Igor Mammedov wrote:
> 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.

Hmm I don't get it.  I literally mean this:


diff --git a/hw/pci/pcie.c b/hw/pci/pcie.c
index d7d73a31e4..72de72ce7a 100644
--- a/hw/pci/pcie.c
+++ b/hw/pci/pcie.c
@@ -389,6 +389,17 @@ static void pcie_cap_update_power(PCIDevice *hotplug_dev)
                         pcie_set_power_device, &power);
 }
 
+void pcie_cap_enable_power(PCIDevice *hotplug_dev)
+{
+    uint8_t *exp_cap = hotplug_dev->config + hotplug_dev->exp.exp_cap;
+    uint32_t sltcap = pci_get_long(exp_cap + PCI_EXP_SLTCAP);
+
+    if (sltcap & PCI_EXP_SLTCAP_PCP) {
+        pci_set_word_by_mask(exp_cap + PCI_EXP_SLTCTL,
+                             PCI_EXP_SLTCTL_PCC, PCI_EXP_SLTCTL_PWR_ON);
+    }
+}
+
 /*
  * A PCI Express Hot-Plug Event has occurred, so update slot status register
  * and notify OS of the event if necessary.

Then call this from ACPI.  How would this have any migration
implications at all?  And why do we need a knob not to power off then?
Power will just stay on since there's nothing turning it off.

-- 
MST