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 <imamm...@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
