[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [PATCH 2/4] pcie: update slot power status only is power control is
From: |
Igor Mammedov |
Subject: |
Re: [PATCH 2/4] pcie: update slot power status only is power control is enabled |
Date: |
Fri, 25 Feb 2022 16:39:17 +0100 |
On Fri, 25 Feb 2022 08:48:13 -0500
"Michael S. Tsirkin" <mst@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" <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:
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.
- Re: [PATCH 2/4] pcie: update slot power status only is power control is enabled, (continued)
- Re: [PATCH 2/4] pcie: update slot power status only is power control is enabled, Michael S. Tsirkin, 2022/02/25
- Re: [PATCH 2/4] pcie: update slot power status only is power control is enabled, Gerd Hoffmann, 2022/02/25
- Re: [PATCH 2/4] pcie: update slot power status only is power control is enabled, Michael S. Tsirkin, 2022/02/25
- Re: [PATCH 2/4] pcie: update slot power status only is power control is enabled, Igor Mammedov, 2022/02/25
- Re: [PATCH 2/4] pcie: update slot power status only is power control is enabled, Michael S. Tsirkin, 2022/02/25
- Re: [PATCH 2/4] pcie: update slot power status only is power control is enabled, Igor Mammedov, 2022/02/25
- Re: [PATCH 2/4] pcie: update slot power status only is power control is enabled, Michael S. Tsirkin, 2022/02/25
- Re: [PATCH 2/4] pcie: update slot power status only is power control is enabled,
Igor Mammedov <=
- Re: [PATCH 2/4] pcie: update slot power status only is power control is enabled, Gerd Hoffmann, 2022/02/28
- Re: [PATCH 2/4] pcie: update slot power status only is power control is enabled, Igor Mammedov, 2022/02/28
[PATCH 3/4] acpi: pcihp: disable power control on PCIe slot, Igor Mammedov, 2022/02/24
[PATCH 4/4] q35: compat: keep hotplugged PCIe device broken after migration for 6.2-older machine types, Igor Mammedov, 2022/02/24
Re: [PATCH 0/4] Fix broken PCIe device after migration, Michael S. Tsirkin, 2022/02/24
Re: [PATCH 0/4] Fix broken PCIe device after migration, Michael S. Tsirkin, 2022/02/25