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  
>