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

["Michael S. Tsirkin" <mst@redhat.com>]

On Thu, Feb 24, 2022 at 12:44:11PM -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-enable-pcie-pcp-cap 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  rc & dst (i.e. with or without
> power control present/configured).
> 
> 1) commit d5daff7d312 (pcie: implement slot power control for pcie root ports)
> 
> Signed-off-by: Igor Mammedov <imammedo@redhat.com>
> ---
>  include/hw/acpi/pcihp.h         |  4 +++-
>  hw/acpi/acpi-pci-hotplug-stub.c |  3 ++-
>  hw/acpi/ich9.c                  | 21 ++++++++++++++++++++-
>  hw/acpi/pcihp.c                 | 20 ++++++++++++--------
>  hw/acpi/piix4.c                 |  3 ++-
>  hw/core/machine.c               |  4 +++-
>  6 files changed, 42 insertions(+), 13 deletions(-)
> 
> diff --git a/include/hw/acpi/pcihp.h b/include/hw/acpi/pcihp.h
> index af1a169fc3..06466dd2a8 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 enable_pcie_pcp_cap;
>  } AcpiPciHpState;
>  
>  void acpi_pcihp_init(Object *owner, AcpiPciHpState *, PCIBus *root,
> @@ -59,7 +60,8 @@ void acpi_pcihp_init(Object *owner, AcpiPciHpState *, PCIBus *root,
>                       uint16_t io_base);
>  
>  void acpi_pcihp_device_pre_plug_cb(HotplugHandler *hotplug_dev,
> -                                   DeviceState *dev, Error **errp);
> +                                   AcpiPciHpState *s, DeviceState *dev,
> +                                   Error **errp);
>  void acpi_pcihp_device_plug_cb(HotplugHandler *hotplug_dev, AcpiPciHpState *s,
>                                 DeviceState *dev, Error **errp);
>  void acpi_pcihp_device_unplug_cb(HotplugHandler *hotplug_dev, AcpiPciHpState *s,
> diff --git a/hw/acpi/acpi-pci-hotplug-stub.c b/hw/acpi/acpi-pci-hotplug-stub.c
> index 734e4c5986..6904b772b3 100644
> --- a/hw/acpi/acpi-pci-hotplug-stub.c
> +++ b/hw/acpi/acpi-pci-hotplug-stub.c
> @@ -18,7 +18,8 @@ void acpi_pcihp_device_plug_cb(HotplugHandler *hotplug_dev, AcpiPciHpState *s,
>  }
>  
>  void acpi_pcihp_device_pre_plug_cb(HotplugHandler *hotplug_dev,
> -                                   DeviceState *dev, Error **errp)
> +                                   AcpiPciHpState *s, DeviceState *dev,
> +                                   Error **errp)
>  {
>      return;
>  }
> diff --git a/hw/acpi/ich9.c b/hw/acpi/ich9.c
> index bd9bbade70..928d5c101c 100644
> --- a/hw/acpi/ich9.c
> +++ b/hw/acpi/ich9.c
> @@ -430,6 +430,21 @@ 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_enable_pcie_pcp_cap(Object *obj, Error **errp)
> +{
> +    ICH9LPCState *s = ICH9_LPC_DEVICE(obj);
> +
> +    return s->pm.acpi_pci_hotplug.enable_pcie_pcp_cap;
> +}
> +
> +static void ich9_pm_set_enable_pcie_pcp_cap(Object *obj, bool value,
> +                                             Error **errp)
> +{
> +    ICH9LPCState *s = ICH9_LPC_DEVICE(obj);
> +
> +    s->pm.acpi_pci_hotplug.enable_pcie_pcp_cap = value;
> +}
> +
>  void ich9_pm_add_properties(Object *obj, ICH9LPCPMRegs *pm)
>  {
>      static const uint32_t gpe0_len = ICH9_PMIO_GPE0_LEN;
> @@ -469,6 +484,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-enable-pcie-pcp-cap",
> +                             ich9_pm_get_enable_pcie_pcp_cap,
> +                             ich9_pm_set_enable_pcie_pcp_cap);
>  }
>  
>  void ich9_pm_device_pre_plug_cb(HotplugHandler *hotplug_dev, DeviceState *dev,
> @@ -477,7 +495,8 @@ void ich9_pm_device_pre_plug_cb(HotplugHandler *hotplug_dev, DeviceState *dev,
>      ICH9LPCState *lpc = ICH9_LPC_DEVICE(hotplug_dev);
>  
>      if (object_dynamic_cast(OBJECT(dev), TYPE_PCI_DEVICE)) {
> -        acpi_pcihp_device_pre_plug_cb(hotplug_dev, dev, errp);
> +        acpi_pcihp_device_pre_plug_cb(hotplug_dev, &lpc->pm.acpi_pci_hotplug,
> +                                      dev, errp);
>          return;
>      }
>  
> diff --git a/hw/acpi/pcihp.c b/hw/acpi/pcihp.c
> index bc47d1bf64..4c1fdb2211 100644
> --- a/hw/acpi/pcihp.c
> +++ b/hw/acpi/pcihp.c
> @@ -291,7 +291,8 @@ void acpi_pcihp_reset(AcpiPciHpState *s, bool acpihp_root_off)
>  #define ONBOARD_INDEX_MAX (16 * 1024 - 1)
>  
>  void acpi_pcihp_device_pre_plug_cb(HotplugHandler *hotplug_dev,
> -                                   DeviceState *dev, Error **errp)
> +                                   AcpiPciHpState *s, DeviceState *dev,
> +                                   Error **errp)
>  {
>      PCIDevice *pdev = PCI_DEVICE(dev);
>  
> @@ -331,13 +332,16 @@ void acpi_pcihp_device_pre_plug_cb(HotplugHandler *hotplug_dev,
>                                   g_cmp_uint32, NULL);
>      }
>  
> -    /*
> -     * since acpi_pcihp manages hotplug, disable PCI-E power control on slot
> -     */
> -    if (object_dynamic_cast(OBJECT(dev), TYPE_PCIE_ROOT_PORT) ||
> -        object_dynamic_cast(OBJECT(dev), TYPE_XIO3130_DOWNSTREAM)) {
> -        object_property_set_bool(OBJECT(dev), COMPAT_PROP_PCP, false,
> -                                 &error_abort);
> +    /* compat knob to preserve pci_config as in 6.2 & older when pcihp in use */

This comment probably belongs where the field is defined, not where
it's used.

> +    if (s->enable_pcie_pcp_cap == false) {
> +        /*
> +         * since acpi_pcihp manages hotplug, disable PCI-E power control on slot

Do we then need to check ACPI_PM_PROP_ACPI_PCIHP_BRIDGE to make sure
acpi hotplug is actually enabled?

> +         */
> +        if (object_dynamic_cast(OBJECT(dev), TYPE_PCIE_ROOT_PORT) ||
> +            object_dynamic_cast(OBJECT(dev), TYPE_XIO3130_DOWNSTREAM)) {
> +            object_property_set_bool(OBJECT(dev), COMPAT_PROP_PCP, false,
> +                                     &error_abort);
> +        }
>      }
>  }
>  
> diff --git a/hw/acpi/piix4.c b/hw/acpi/piix4.c
> index cc37fa3416..0d25f75112 100644
> --- a/hw/acpi/piix4.c
> +++ b/hw/acpi/piix4.c
> @@ -350,7 +350,8 @@ static void piix4_device_pre_plug_cb(HotplugHandler *hotplug_dev,
>      PIIX4PMState *s = PIIX4_PM(hotplug_dev);
>  
>      if (object_dynamic_cast(OBJECT(dev), TYPE_PCI_DEVICE)) {
> -        acpi_pcihp_device_pre_plug_cb(hotplug_dev, dev, errp);
> +        acpi_pcihp_device_pre_plug_cb(hotplug_dev, &s->acpi_pci_hotplug, dev,
> +                                      errp);
>      } else if (object_dynamic_cast(OBJECT(dev), TYPE_PC_DIMM)) {
>          if (!s->acpi_memory_hotplug.is_enabled) {
>              error_setg(errp,
> diff --git a/hw/core/machine.c b/hw/core/machine.c
> index d856485cb4..48ffc85f8d 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-enable-pcie-pcp-cap", "on" },
> +};
>  const size_t hw_compat_6_2_len = G_N_ELEMENTS(hw_compat_6_2);
>  
>  GlobalProperty hw_compat_6_1[] = {
> -- 
> 2.31.1