* [PATCH v2 1/3] pci: expose TYPE_XIO3130_DOWNSTREAM name
2022-03-01 15:11 [PATCH v2 0/3] Fix broken PCIe device after migration Igor Mammedov
@ 2022-03-01 15:11 ` Igor Mammedov
2022-03-01 15:11 ` [PATCH v2 2/3] acpi: pcihp: pcie: set power on cap on parent slot Igor Mammedov
` (2 subsequent siblings)
3 siblings, 0 replies; 7+ messages in thread
From: Igor Mammedov @ 2022-03-01 15:11 UTC (permalink / raw)
To: qemu-devel; +Cc: kraxel, mst
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
^ permalink raw reply related [flat|nested] 7+ messages in thread* [PATCH v2 2/3] acpi: pcihp: pcie: set power on cap on parent slot
2022-03-01 15:11 [PATCH v2 0/3] Fix broken PCIe device after migration Igor Mammedov
2022-03-01 15:11 ` [PATCH v2 1/3] pci: expose TYPE_XIO3130_DOWNSTREAM name Igor Mammedov
@ 2022-03-01 15:11 ` Igor Mammedov
2022-03-01 15:12 ` [PATCH v2 3/3] q35: compat: keep hotplugged PCIe device broken after migration for 6.2 and older machine types Igor Mammedov
2022-03-01 15:20 ` [PATCH v2 0/3] Fix broken PCIe device after migration Michael S. Tsirkin
3 siblings, 0 replies; 7+ messages in thread
From: Igor Mammedov @ 2022-03-01 15:11 UTC (permalink / raw)
To: qemu-devel; +Cc: kraxel, mst
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
^ permalink raw reply related [flat|nested] 7+ messages in thread* [PATCH v2 3/3] q35: compat: keep hotplugged PCIe device broken after migration for 6.2 and older machine types
2022-03-01 15:11 [PATCH v2 0/3] Fix broken PCIe device after migration Igor Mammedov
2022-03-01 15:11 ` [PATCH v2 1/3] pci: expose TYPE_XIO3130_DOWNSTREAM name Igor Mammedov
2022-03-01 15:11 ` [PATCH v2 2/3] acpi: pcihp: pcie: set power on cap on parent slot Igor Mammedov
@ 2022-03-01 15:12 ` Igor Mammedov
2022-03-01 15:20 ` Michael S. Tsirkin
2022-03-01 15:20 ` [PATCH v2 0/3] Fix broken PCIe device after migration Michael S. Tsirkin
3 siblings, 1 reply; 7+ messages in thread
From: Igor Mammedov @ 2022-03-01 15:12 UTC (permalink / raw)
To: qemu-devel; +Cc: kraxel, mst
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
^ permalink raw reply related [flat|nested] 7+ messages in thread* Re: [PATCH v2 3/3] q35: compat: keep hotplugged PCIe device broken after migration for 6.2 and older machine types
2022-03-01 15:12 ` [PATCH v2 3/3] q35: compat: keep hotplugged PCIe device broken after migration for 6.2 and older machine types Igor Mammedov
@ 2022-03-01 15:20 ` Michael S. Tsirkin
2022-03-02 7:51 ` Igor Mammedov
0 siblings, 1 reply; 7+ messages in thread
From: Michael S. Tsirkin @ 2022-03-01 15:20 UTC (permalink / raw)
To: Igor Mammedov; +Cc: qemu-devel, kraxel
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
^ permalink raw reply [flat|nested] 7+ messages in thread* Re: [PATCH v2 3/3] q35: compat: keep hotplugged PCIe device broken after migration for 6.2 and older machine types
2022-03-01 15:20 ` Michael S. Tsirkin
@ 2022-03-02 7:51 ` Igor Mammedov
0 siblings, 0 replies; 7+ messages in thread
From: Igor Mammedov @ 2022-03-02 7:51 UTC (permalink / raw)
To: Michael S. Tsirkin; +Cc: qemu-devel, kraxel
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
>
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH v2 0/3] Fix broken PCIe device after migration
2022-03-01 15:11 [PATCH v2 0/3] Fix broken PCIe device after migration Igor Mammedov
` (2 preceding siblings ...)
2022-03-01 15:12 ` [PATCH v2 3/3] q35: compat: keep hotplugged PCIe device broken after migration for 6.2 and older machine types Igor Mammedov
@ 2022-03-01 15:20 ` Michael S. Tsirkin
3 siblings, 0 replies; 7+ messages in thread
From: Michael S. Tsirkin @ 2022-03-01 15:20 UTC (permalink / raw)
To: Igor Mammedov; +Cc: qemu-devel, kraxel
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
^ permalink raw reply [flat|nested] 7+ messages in thread