[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: |
Michael S. Tsirkin |
Subject: |
Re: [PATCH 2/4] pcie: update slot power status only is power control is enabled |
Date: |
Fri, 25 Feb 2022 05:05:23 -0500 |
On Thu, Feb 24, 2022 at 12:44:09PM -0500, Igor Mammedov wrote:
> on creation a PCIDevice has power turned on at the end of pci_qdev_realize()
> however later on if PCIe slot isn't populated with any children
> it's power is turned off. It's fine if native hotplug is used
> as plug callback will power slot on among other things.
> However when ACPI hotplug is enabled it replaces native PCIe plug
> callbacks with ACPI specific ones (acpi_pcihp_device_*plug_cb) and
> as result slot stays powered off. It works fine as ACPI hotplug
> on guest side takes care of enumerating/initializing hotplugged
> device. But when later guest is migrated, call chain introduced by [1]
>
> pcie_cap_slot_post_load()
> -> pcie_cap_update_power()
> -> pcie_set_power_device()
> -> pci_set_power()
> -> pci_update_mappings()
>
> will disable earlier initialized BARs for the hotplugged device
> in powered off slot due to commit [2] which disables BARs if
> power is off. As result guest OS after migration will be very
> much confused [3], still thinking that it has working device,
> which isn't true anymore due to disabled BARs.
>
> Fix it by honoring PCI_EXP_SLTCAP_PCP and updating power status
> only if capability is enabled.
> Follow up patch will disable
> PCI_EXP_SLTCAP_PCP overriding COMPAT_PROP_PCP property when
> PCIe slot is under ACPI PCI hotplug control.
>
> See [3] for reproducer.
>
> 1)
> Fixes: commit d5daff7d312 (pcie: implement slot power control for pcie root
> ports)
> 2)
> commit 23786d13441 (pci: implement power state)
> 3)
> Bugzilla: https://bugzilla.redhat.com/show_bug.cgi?id=2053584
>
> Signed-off-by: Igor Mammedov <imammedo@redhat.com>
> ---
> hw/pci/pcie.c | 5 ++---
> 1 file changed, 2 insertions(+), 3 deletions(-)
>
> 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);
Hmm I wrote I like it but now I am not sure I understand how does this
patch help fix things. here is the full function:
static void pcie_cap_update_power(PCIDevice *hotplug_dev)
{
uint8_t *exp_cap = hotplug_dev->config + hotplug_dev->exp.exp_cap;
PCIBus *sec_bus = pci_bridge_get_sec_bus(PCI_BRIDGE(hotplug_dev));
uint32_t sltcap = pci_get_long(exp_cap + PCI_EXP_SLTCAP);
uint16_t sltctl = pci_get_word(exp_cap + PCI_EXP_SLTCTL);
bool power = true;
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);
}
I understand the follow up patch, but it looks to me that without the
capability, power is always on. Why does skipping that help fix
anything?
Thanks,
> }
>
> /*
> --
> 2.31.1
- [PATCH 0/4] Fix broken PCIe device after migration, Igor Mammedov, 2022/02/24
- [PATCH 1/4] pci: expose TYPE_XIO3130_DOWNSTREAM name, Igor Mammedov, 2022/02/24
- [PATCH 2/4] pcie: update slot power status only is power control is enabled, Igor Mammedov, 2022/02/24
- Re: [PATCH 2/4] pcie: update slot power status only is power control is enabled,
Michael S. Tsirkin <=
- 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, 2022/02/25
- 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