On Fri, 25 Feb 2022 08:48:13 -0500 "Michael S. Tsirkin" <m...@redhat.com> wrote:
> 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" <m...@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 <kra...@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: I was thinking about the time when we do override native callbacks. which happens for every root port at its realize time. Start up sequence on src: acpi_pcihp_device_pre_plug_cb(dev: extra_root0) pci_qdev_realize(extra_root0) pci_set_power: extra_root0, d->has_power: 0, new power state: 1 pci_set_power: extra_root0, set has_power to: 1 acpi_pcihp_device_plug_cb(dev: extra_root0) <== lets assume we call pcie_cap_enable_power(dev) here it's all good wrt layering as we are rewiring being initialized root port to another hp controller from context of its parent device pcie_cap_slot_reset <== then here we hit "if (populated) {} else {}" which kills whatever above has done since slot is not populated and a knob would be needed to prevent reset (i.e. don't touch power state as it's 'managed' by ACPI) 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 Though I haven't thought about end-device hotplug time: (qemu) device_add e1000e,bus=extra_root0,id=nic acpi_pcihp_device_pre_plug_cb(dev: nic) pci_qdev_realize(nic) pci_set_power: nic, d->has_power: 0, new power state: 1 pci_set_power: nic, set has_power to: 1 acpi_pcihp_device_plug_cb(dev: nic) <== here we have a chance to power on no longer empty slot pcie_cap_enable_power(hotplug_dev) then when target loads state it will see SLTCTL_PCC: 0 and keep slot powered on. pci_set_power: nic, d->has_power: 1, new power state: 1 This where I wasn't comfortable with idea of calling random PCIe code chunks and thought about chaining callbacks so that pcie_cap_slot_[pre_]plug_cb() would do necessary PCIe steps and acpi_pcihp_device_[pre_]plug_cb() do ACPI specific things not intruding on each other, but that requires telling PCIe code that it should not issue native hotplug event to guest. > 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. It still changes pci_config, the similar to disabling SLTCAP_PCP, so I think we still need migration compat knob to have the same device state in cross version migration case.