Re: [PATCH v2 3/3] q35: compat: keep hotplugged PCIe device broken after migration for 6.2 and older machine types

[Michael S. Tsirkin]

On Tue, Mar 01, 2022 at 10:12:00AM -0500, Igor Mammedov wrote:
> Q35 switched to ACPI PCI hotplug by default in since 6.1
> machine type and migration worked as expected (with BARs
> on target being the same as on source)
> 
> However native PCIe fixes [1] merged in 6.2 time, regressed
> migration part, resulting in disabled BARs on target. The
> issue affects pc-q35-6.2 and pc-q35-6.1 machine types (and
> older if qemu-6.2 binary is used on source with manually
> enabled ACPI PCI hotplug).
> 
> Introduce x-pcihp-disable-pcie-slot-power-on-fixup compat
> property to keep 6.2 and older machine types in broken state
> when ACPI PCI hotplug is enabled to make sure that guest does
> see the same PCIe device and slot on src & dst.
> 
> 1)
> Fixes: d5daff7d312 (pcie: implement slot power control for pcie root ports)
> Signed-off-by: Igor Mammedov <imammedo@redhat.com>

I am not sure why we need this one. What's the scenario that's broken
otherwise?


> ---
>  include/hw/acpi/pcihp.h |  1 +
>  hw/acpi/ich9.c          | 20 ++++++++++++++++++++
>  hw/acpi/pcihp.c         | 11 +++++++----
>  hw/core/machine.c       |  4 +++-
>  4 files changed, 31 insertions(+), 5 deletions(-)
> 
> diff --git a/include/hw/acpi/pcihp.h b/include/hw/acpi/pcihp.h
> index af1a169fc3..2436151678 100644
> --- a/include/hw/acpi/pcihp.h
> +++ b/include/hw/acpi/pcihp.h
> @@ -52,6 +52,7 @@ typedef struct AcpiPciHpState {
>      bool legacy_piix;
>      uint16_t io_base;
>      uint16_t io_len;
> +    bool disable_pcie_slot_power_on_fixup;
>  } AcpiPciHpState;
>  
>  void acpi_pcihp_init(Object *owner, AcpiPciHpState *, PCIBus *root,
> diff --git a/hw/acpi/ich9.c b/hw/acpi/ich9.c
> index bd9bbade70..e3bffdef71 100644
> --- a/hw/acpi/ich9.c
> +++ b/hw/acpi/ich9.c
> @@ -430,6 +430,23 @@ static void ich9_pm_set_keep_pci_slot_hpc(Object *obj, 
> bool value, Error **errp)
>      s->pm.keep_pci_slot_hpc = value;
>  }
>  
> +static bool ich9_pm_get_disable_pcie_slot_power_on_fixup(Object *obj,
> +                                                         Error **errp)
> +{
> +    ICH9LPCState *s = ICH9_LPC_DEVICE(obj);
> +
> +    return s->pm.acpi_pci_hotplug.disable_pcie_slot_power_on_fixup;
> +}
> +
> +static void ich9_pm_set_disable_pcie_slot_power_on_fixup(Object *obj,
> +                                                         bool value,
> +                                                         Error **errp)
> +{
> +    ICH9LPCState *s = ICH9_LPC_DEVICE(obj);
> +
> +    s->pm.acpi_pci_hotplug.disable_pcie_slot_power_on_fixup = value;
> +}
> +
>  void ich9_pm_add_properties(Object *obj, ICH9LPCPMRegs *pm)
>  {
>      static const uint32_t gpe0_len = ICH9_PMIO_GPE0_LEN;
> @@ -469,6 +486,9 @@ void ich9_pm_add_properties(Object *obj, ICH9LPCPMRegs 
> *pm)
>      object_property_add_bool(obj, "x-keep-pci-slot-hpc",
>                               ich9_pm_get_keep_pci_slot_hpc,
>                               ich9_pm_set_keep_pci_slot_hpc);
> +    object_property_add_bool(obj, "x-pcihp-disable-pcie-slot-power-on-fixup",
> +                             ich9_pm_get_disable_pcie_slot_power_on_fixup,
> +                             ich9_pm_set_disable_pcie_slot_power_on_fixup);
>  }
>  
>  void ich9_pm_device_pre_plug_cb(HotplugHandler *hotplug_dev, DeviceState 
> *dev,
> diff --git a/hw/acpi/pcihp.c b/hw/acpi/pcihp.c
> index 6351bd3424..4c06caf4a9 100644
> --- a/hw/acpi/pcihp.c
> +++ b/hw/acpi/pcihp.c
> @@ -369,10 +369,13 @@ void acpi_pcihp_device_plug_cb(HotplugHandler 
> *hotplug_dev, AcpiPciHpState *s,
>      }
>  
>      bus = pci_get_bus(pdev);
> -    bridge = pci_bridge_get_device(bus);
> -    if (object_dynamic_cast(OBJECT(bridge), TYPE_PCIE_ROOT_PORT) ||
> -        object_dynamic_cast(OBJECT(bridge), TYPE_XIO3130_DOWNSTREAM)) {
> -        pcie_cap_slot_enable_power(bridge);
> +    /* compat knob to preserve pci_config as in 6.2 & older when pcihp in 
> use */
> +    if (s->disable_pcie_slot_power_on_fixup == false) {
> +        bridge = pci_bridge_get_device(bus);
> +        if (object_dynamic_cast(OBJECT(bridge), TYPE_PCIE_ROOT_PORT) ||
> +            object_dynamic_cast(OBJECT(bridge), TYPE_XIO3130_DOWNSTREAM)) {
> +            pcie_cap_slot_enable_power(bridge);
> +        }
>      }
>  
>      bsel = acpi_pcihp_get_bsel(bus);
> diff --git a/hw/core/machine.c b/hw/core/machine.c
> index d856485cb4..1758b49c2f 100644
> --- a/hw/core/machine.c
> +++ b/hw/core/machine.c
> @@ -37,7 +37,9 @@
>  #include "hw/virtio/virtio.h"
>  #include "hw/virtio/virtio-pci.h"
>  
> -GlobalProperty hw_compat_6_2[] = {};
> +GlobalProperty hw_compat_6_2[] = {
> +    { "ICH9-LPC", "x-pcihp-disable-pcie-slot-power-on-fixup", "on" },
> +};
>  const size_t hw_compat_6_2_len = G_N_ELEMENTS(hw_compat_6_2);
>  
>  GlobalProperty hw_compat_6_1[] = {
> -- 
> 2.31.1