[PATCH v2 0/3] Fix broken PCIe device after migration

[PATCH v2 0/3] Fix broken PCIe device after migration

[Igor Mammedov]

Changelog:
   v2:
     * instead of disabling power control on slot and letting ACPI
       PCI hotplug  module in guest to deal with it, set
       PCI_EXP_SLTCTL_PWR_ON on PCIe slot from acpi_pcihp_device_plug_cb()
       when a device plugged into it.

Currently ACPI PCI hotplug is enabled by default for Q35 machine
type and overrides native PCIe hotplug. It works as expected when
a PCIe device is hotplugged into slot, however the device becomes
in-operational after migration. Which is caused by BARs being
disabled on target due to powered off status of the slot.

Proposed fix turns on power on PCIe slot when a device is
hotplugged into it, then that state is migrated and device
stays powred on after migration. 

Tested with (seabios: FC34, Win2012; ovmf: RHEL8)

RHBZ: https://bugzilla.redhat.com/show_bug.cgi?id=2053584
CC: mst@redhat.com
CC: kraxel@redhat.com

Ref to v1:
 https://lore.kernel.org/all/20220225100127.78974d71@redhat.com/T/
Gitlab link:
 https://gitlab.com/imammedo/qemu/-/tree/pcie_poweroff_acpi_regression_rhbz2053584_V2

Igor Mammedov (3):
  pci: expose TYPE_XIO3130_DOWNSTREAM name
  acpi: pcihp: pcie: set power on cap on parent slot
  q35: compat: keep hotplugged PCIe device broken after migration for
    6.2 and older machine types

 include/hw/acpi/pcihp.h                    |  1 +
 include/hw/pci-bridge/xio3130_downstream.h | 15 +++++++++++++++
 include/hw/pci/pcie.h                      |  1 +
 hw/acpi/ich9.c                             | 20 ++++++++++++++++++++
 hw/acpi/pcihp.c                            | 15 ++++++++++++++-
 hw/core/machine.c                          |  4 +++-
 hw/pci-bridge/xio3130_downstream.c         |  3 ++-
 hw/pci/pcie.c                              | 11 +++++++++++
 8 files changed, 67 insertions(+), 3 deletions(-)
 create mode 100644 include/hw/pci-bridge/xio3130_downstream.h

-- 
2.31.1

[PATCH v2 1/3] pci: expose TYPE_XIO3130_DOWNSTREAM name

[Igor Mammedov]

Type name will be used in followup patch for cast check
in pcihp code.

Signed-off-by: Igor Mammedov <imammedo@redhat.com>
---
 include/hw/pci-bridge/xio3130_downstream.h | 15 +++++++++++++++
 hw/pci-bridge/xio3130_downstream.c         |  3 ++-
 2 files changed, 17 insertions(+), 1 deletion(-)
 create mode 100644 include/hw/pci-bridge/xio3130_downstream.h

diff --git a/include/hw/pci-bridge/xio3130_downstream.h b/include/hw/pci-bridge/xio3130_downstream.h
new file mode 100644
index 0000000000..1d10139aea
--- /dev/null
+++ b/include/hw/pci-bridge/xio3130_downstream.h
@@ -0,0 +1,15 @@
+/*
+ * TI X3130 pci express downstream port switch
+ *
+ * Copyright (C) 2022 Igor Mammedov <imammedo@redhat.com>
+ *
+ * SPDX-License-Identifier: GPL-2.0-or-later
+ */
+
+#ifndef HW_PCI_BRIDGE_XIO3130_DOWNSTREAM_H
+#define HW_PCI_BRIDGE_XIO3130_DOWNSTREAM_H
+
+#define TYPE_XIO3130_DOWNSTREAM "xio3130-downstream"
+
+#endif
+
diff --git a/hw/pci-bridge/xio3130_downstream.c b/hw/pci-bridge/xio3130_downstream.c
index 04aae72cd6..b17cafd359 100644
--- a/hw/pci-bridge/xio3130_downstream.c
+++ b/hw/pci-bridge/xio3130_downstream.c
@@ -28,6 +28,7 @@
 #include "migration/vmstate.h"
 #include "qapi/error.h"
 #include "qemu/module.h"
+#include "hw/pci-bridge/xio3130_downstream.h"
 
 #define PCI_DEVICE_ID_TI_XIO3130D       0x8233  /* downstream port */
 #define XIO3130_REVISION                0x1
@@ -173,7 +174,7 @@ static void xio3130_downstream_class_init(ObjectClass *klass, void *data)
 }
 
 static const TypeInfo xio3130_downstream_info = {
-    .name          = "xio3130-downstream",
+    .name          = TYPE_XIO3130_DOWNSTREAM,
     .parent        = TYPE_PCIE_SLOT,
     .class_init    = xio3130_downstream_class_init,
     .interfaces = (InterfaceInfo[]) {
-- 
2.31.1

[PATCH v2 2/3] acpi: pcihp: pcie: set power on cap on parent slot

[Igor Mammedov]

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]
commit d5daff7d312 (pcie: implement slot power control for pcie root ports)

   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 23786d13441 (pci: implement power state)
which disables BARs if power is off.

Fix it by setting PCI_EXP_SLTCTL_PCC to PCI_EXP_SLTCTL_PWR_ON
on slot (root port/downstream port) at the time a device
hotplugged into it. As result PCI_EXP_SLTCTL_PWR_ON is migrated
to target and above call chain keeps device plugged into it
powered on.

Fixes: d5daff7d312 ("pcie: implement slot power control for pcie root ports")
Fixes: 23786d13441 ("pci: implement power state")
Fixes: https://bugzilla.redhat.com/show_bug.cgi?id=2053584
Suggested-by: "Michael S. Tsirkin" <mst@redhat.com>
Signed-off-by: Igor Mammedov <imammedo@redhat.com>
---
 include/hw/pci/pcie.h |  1 +
 hw/acpi/pcihp.c       | 12 +++++++++++-
 hw/pci/pcie.c         | 11 +++++++++++
 3 files changed, 23 insertions(+), 1 deletion(-)

diff --git a/include/hw/pci/pcie.h b/include/hw/pci/pcie.h
index 6063bee0ec..c27368d077 100644
--- a/include/hw/pci/pcie.h
+++ b/include/hw/pci/pcie.h
@@ -112,6 +112,7 @@ void pcie_cap_slot_write_config(PCIDevice *dev,
                                 uint32_t addr, uint32_t val, int len);
 int pcie_cap_slot_post_load(void *opaque, int version_id);
 void pcie_cap_slot_push_attention_button(PCIDevice *dev);
+void pcie_cap_slot_enable_power(PCIDevice *dev);
 
 void pcie_cap_root_init(PCIDevice *dev);
 void pcie_cap_root_reset(PCIDevice *dev);
diff --git a/hw/acpi/pcihp.c b/hw/acpi/pcihp.c
index 6befd23e16..6351bd3424 100644
--- a/hw/acpi/pcihp.c
+++ b/hw/acpi/pcihp.c
@@ -32,6 +32,7 @@
 #include "hw/pci/pci_bridge.h"
 #include "hw/pci/pci_host.h"
 #include "hw/pci/pcie_port.h"
+#include "hw/pci-bridge/xio3130_downstream.h"
 #include "hw/i386/acpi-build.h"
 #include "hw/acpi/acpi.h"
 #include "hw/pci/pci_bus.h"
@@ -336,6 +337,8 @@ void acpi_pcihp_device_plug_cb(HotplugHandler *hotplug_dev, AcpiPciHpState *s,
 {
     PCIDevice *pdev = PCI_DEVICE(dev);
     int slot = PCI_SLOT(pdev->devfn);
+    PCIDevice *bridge;
+    PCIBus *bus;
     int bsel;
 
     /* Don't send event when device is enabled during qemu machine creation:
@@ -365,7 +368,14 @@ void acpi_pcihp_device_plug_cb(HotplugHandler *hotplug_dev, AcpiPciHpState *s,
         return;
     }
 
-    bsel = acpi_pcihp_get_bsel(pci_get_bus(pdev));
+    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);
+    }
+
+    bsel = acpi_pcihp_get_bsel(bus);
     g_assert(bsel >= 0);
     s->acpi_pcihp_pci_status[bsel].up |= (1U << slot);
     acpi_send_event(DEVICE(hotplug_dev), ACPI_PCI_HOTPLUG_STATUS);
diff --git a/hw/pci/pcie.c b/hw/pci/pcie.c
index d7d73a31e4..996f0e24fe 100644
--- a/hw/pci/pcie.c
+++ b/hw/pci/pcie.c
@@ -366,6 +366,17 @@ static void hotplug_event_clear(PCIDevice *dev)
     }
 }
 
+void pcie_cap_slot_enable_power(PCIDevice *dev)
+{
+    uint8_t *exp_cap = dev->config + 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);
+    }
+}
+
 static void pcie_set_power_device(PCIBus *bus, PCIDevice *dev, void *opaque)
 {
     bool *power = opaque;
-- 
2.31.1

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

[Igor Mammedov]

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>
---
 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

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

Re: [PATCH v2 0/3] Fix broken PCIe device after migration

[Michael S. Tsirkin]

On Tue, Mar 01, 2022 at 10:11:57AM -0500, Igor Mammedov wrote:
> Changelog:
>    v2:
>      * instead of disabling power control on slot and letting ACPI
>        PCI hotplug  module in guest to deal with it, set
>        PCI_EXP_SLTCTL_PWR_ON on PCIe slot from acpi_pcihp_device_plug_cb()
>        when a device plugged into it.
> 
> Currently ACPI PCI hotplug is enabled by default for Q35 machine
> type and overrides native PCIe hotplug. It works as expected when
> a PCIe device is hotplugged into slot, however the device becomes
> in-operational after migration. Which is caused by BARs being
> disabled on target due to powered off status of the slot.
> 
> Proposed fix turns on power on PCIe slot when a device is
> hotplugged into it, then that state is migrated and device
> stays powred on after migration. 


Looks good for 1-2. Some questions on 3.
Thanks!

> Tested with (seabios: FC34, Win2012; ovmf: RHEL8)
> 
> RHBZ: https://bugzilla.redhat.com/show_bug.cgi?id=2053584
> CC: mst@redhat.com
> CC: kraxel@redhat.com
> 
> Ref to v1:
>  https://lore.kernel.org/all/20220225100127.78974d71@redhat.com/T/
> Gitlab link:
>  https://gitlab.com/imammedo/qemu/-/tree/pcie_poweroff_acpi_regression_rhbz2053584_V2
> 
> Igor Mammedov (3):
>   pci: expose TYPE_XIO3130_DOWNSTREAM name
>   acpi: pcihp: pcie: set power on cap on parent slot
>   q35: compat: keep hotplugged PCIe device broken after migration for
>     6.2 and older machine types
> 
>  include/hw/acpi/pcihp.h                    |  1 +
>  include/hw/pci-bridge/xio3130_downstream.h | 15 +++++++++++++++
>  include/hw/pci/pcie.h                      |  1 +
>  hw/acpi/ich9.c                             | 20 ++++++++++++++++++++
>  hw/acpi/pcihp.c                            | 15 ++++++++++++++-
>  hw/core/machine.c                          |  4 +++-
>  hw/pci-bridge/xio3130_downstream.c         |  3 ++-
>  hw/pci/pcie.c                              | 11 +++++++++++
>  8 files changed, 67 insertions(+), 3 deletions(-)
>  create mode 100644 include/hw/pci-bridge/xio3130_downstream.h
> 
> -- 
> 2.31.1

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

[Igor Mammedov]

On Tue, 1 Mar 2022 10:20:06 -0500
"Michael S. Tsirkin" <mst@redhat.com> wrote:

> 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?

Probably none (but I won't bet on it) beside user confusion where device
sometimes works and sometimes don't for 6.1-6.2 machine types depending
on whether source runs qemu-6.2 or not.
Guest also will observe PCI_EXP_SLTCTL_PCC flip depending on source QEMU
version, but it shouldn't use it anyways since slot is under control of
ACPI CPI hotplug module.

Feel free to drop this patch if you think it's overkill.

> 
> 
> > ---
> >  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  
>