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

[Igor Mammedov <imammedo@redhat.com>]

On Thu, 24 Feb 2022 13:11:53 -0500
"Michael S. Tsirkin" <mst@redhat.com> wrote:

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

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

no need for it,
this code path is called only when acpi hotplug is enabled.
In case it's disabled these callbacks are not wired to affected PCI buses,
and PCI[e] uses default native hotplug callbacks.


> > +         */
> > +        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  
>