[RFC PATCH 0/5] Use ACPI PCI hot-plug for q35

[RFC PATCH 0/5] Use ACPI PCI hot-plug for q35

[Julia Suvorova]

PCIe native hot-plug has numerous problems with racing events and unpredictable
guest behaviour (Windows). Switching to ACPI hot-plug for now.

Tested on RHEL 8 and Windows 2019.

pxb-pcie is not yet supported.

Julia Suvorova (5):
  hw/acpi/pcihp: Introduce find_host()
  hw/acpi/ich9: Trace ich9_gpe_readb()/writeb()
  hw/i386/acpi-build: Add ACPI PCI hot-plug methods to q35
  hw/i386/acpi-build: Turn off support of PCIe native hot-plug and SHPC
    in _OSC
  hw/acpi/ich9: Enable ACPI PCI hot-plug

 hw/i386/acpi-build.h   |  2 ++
 include/hw/acpi/ich9.h |  3 +++
 hw/acpi/ich9.c         | 52 +++++++++++++++++++++++++++++++++++++++++-
 hw/acpi/pcihp.c        | 16 ++++++++++++-
 hw/i386/acpi-build.c   | 34 +++++++++++++++++----------
 hw/i386/pc.c           |  4 +++-
 hw/acpi/trace-events   |  4 ++++
 7 files changed, 100 insertions(+), 15 deletions(-)

-- 
2.25.4

[RFC PATCH 1/5] hw/acpi/pcihp: Introduce find_host()

[Julia Suvorova]

Returns the current host bus with ACPI PCI hot-plug support: q35 or i440fx.

Signed-off-by: Julia Suvorova <jusual@redhat.com>
---
 hw/i386/acpi-build.h |  2 ++
 hw/acpi/pcihp.c      | 13 +++++++++++++
 hw/i386/acpi-build.c |  2 +-
 3 files changed, 16 insertions(+), 1 deletion(-)

diff --git a/hw/i386/acpi-build.h b/hw/i386/acpi-build.h
index 74df5fc612..0696b4e48d 100644
--- a/hw/i386/acpi-build.h
+++ b/hw/i386/acpi-build.h
@@ -7,4 +7,6 @@ extern const struct AcpiGenericAddress x86_nvdimm_acpi_dsmio;
 
 void acpi_setup(void);
 
+Object *acpi_get_i386_pci_host(void);
+
 #endif
diff --git a/hw/acpi/pcihp.c b/hw/acpi/pcihp.c
index d42906ea19..3d4ee3af72 100644
--- a/hw/acpi/pcihp.c
+++ b/hw/acpi/pcihp.c
@@ -33,10 +33,12 @@
 #include "hw/acpi/acpi.h"
 #include "exec/address-spaces.h"
 #include "hw/pci/pci_bus.h"
+#include "hw/pci/pci_host.h"
 #include "migration/vmstate.h"
 #include "qapi/error.h"
 #include "qom/qom-qobject.h"
 #include "trace.h"
+#include "hw/i386/acpi-build.h"
 
 #define ACPI_PCIHP_ADDR 0xae00
 #define ACPI_PCIHP_SIZE 0x0014
@@ -86,6 +88,17 @@ static void *acpi_set_bsel(PCIBus *bus, void *opaque)
     return bsel_alloc;
 }
 
+static PCIBus *find_host(void)
+{
+    Object *obj = acpi_get_i386_pci_host();
+
+    if (obj) {
+        return PCI_HOST_BRIDGE(obj)->bus;
+    }
+
+    return NULL;
+}
+
 static void acpi_set_pci_info(void)
 {
     static bool bsel_is_set;
diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
index 900f786d08..11c598f955 100644
--- a/hw/i386/acpi-build.c
+++ b/hw/i386/acpi-build.c
@@ -270,7 +270,7 @@ static void acpi_get_misc_info(AcpiMiscInfo *info)
  * Because of the PXB hosts we cannot simply query TYPE_PCI_HOST_BRIDGE.
  * On i386 arch we only have two pci hosts, so we can look only for them.
  */
-static Object *acpi_get_i386_pci_host(void)
+Object *acpi_get_i386_pci_host(void)
 {
     PCIHostState *host;
 
-- 
2.25.4

[RFC PATCH 2/5] hw/acpi/ich9: Trace ich9_gpe_readb()/writeb()

[Julia Suvorova]

Add trace events similar to piix4_gpe_readb() to check gpe status.

Signed-off-by: Julia Suvorova <jusual@redhat.com>
---
 hw/acpi/ich9.c       | 7 ++++++-
 hw/acpi/trace-events | 4 ++++
 2 files changed, 10 insertions(+), 1 deletion(-)

diff --git a/hw/acpi/ich9.c b/hw/acpi/ich9.c
index 2d204babc6..0fdd736da4 100644
--- a/hw/acpi/ich9.c
+++ b/hw/acpi/ich9.c
@@ -36,6 +36,7 @@
 #include "hw/acpi/acpi.h"
 #include "hw/acpi/tco.h"
 #include "exec/address-spaces.h"
+#include "trace.h"
 
 #include "hw/i386/ich9.h"
 #include "hw/mem/pc-dimm.h"
@@ -59,13 +60,17 @@ static void ich9_pm_update_sci_fn(ACPIREGS *regs)
 static uint64_t ich9_gpe_readb(void *opaque, hwaddr addr, unsigned width)
 {
     ICH9LPCPMRegs *pm = opaque;
-    return acpi_gpe_ioport_readb(&pm->acpi_regs, addr);
+    uint64_t val = acpi_gpe_ioport_readb(&pm->acpi_regs, addr);
+
+    trace_ich9_gpe_readb(addr, width, val);
+    return val;
 }
 
 static void ich9_gpe_writeb(void *opaque, hwaddr addr, uint64_t val,
                             unsigned width)
 {
     ICH9LPCPMRegs *pm = opaque;
+    trace_ich9_gpe_writeb(addr, width, val);
     acpi_gpe_ioport_writeb(&pm->acpi_regs, addr, val);
     acpi_update_sci(&pm->acpi_regs, pm->irq);
 }
diff --git a/hw/acpi/trace-events b/hw/acpi/trace-events
index afbc77de1c..b9f4827afc 100644
--- a/hw/acpi/trace-events
+++ b/hw/acpi/trace-events
@@ -32,6 +32,10 @@ cpuhp_acpi_ejecting_cpu(uint32_t idx) "0x%"PRIx32
 cpuhp_acpi_write_ost_ev(uint32_t slot, uint32_t ev) "idx[0x%"PRIx32"] OST EVENT: 0x%"PRIx32
 cpuhp_acpi_write_ost_status(uint32_t slot, uint32_t st) "idx[0x%"PRIx32"] OST STATUS: 0x%"PRIx32
 
+# ich9.c
+ich9_gpe_readb(uint64_t addr, unsigned width, uint64_t val) "addr: 0x%" PRIx64 " width: %d ==> 0x%" PRIx64
+ich9_gpe_writeb(uint64_t addr, unsigned width, uint64_t val) "addr: 0x%" PRIx64 " width: %d <== 0x%" PRIx64
+
 # pcihp.c
 acpi_pci_eject_slot(unsigned bsel, unsigned slot) "bsel: %u slot: %u"
 acpi_pci_unplug(int bsel, int slot) "bsel: %d slot: %d"
-- 
2.25.4

[RFC PATCH 3/5] hw/i386/acpi-build: Add ACPI PCI hot-plug methods to q35

[Julia Suvorova]

Implement notifications and gpe to support q35 ACPI PCI hot-plug.
The addresses specified in [1] remain the same to make fewer changes.

[1] docs/spec/acpi_pci_hotplug.txt

Signed-off-by: Julia Suvorova <jusual@redhat.com>
---
 hw/i386/acpi-build.c | 20 +++++++++++++-------
 1 file changed, 13 insertions(+), 7 deletions(-)

diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
index 11c598f955..5c5ad88ad6 100644
--- a/hw/i386/acpi-build.c
+++ b/hw/i386/acpi-build.c
@@ -201,10 +201,6 @@ static void acpi_get_pm_info(MachineState *machine, AcpiPmInfo *pm)
         /* w2k requires FADT(rev1) or it won't boot, keep PC compatible */
         pm->fadt.rev = 1;
         pm->cpu_hp_io_base = PIIX4_CPU_HOTPLUG_IO_BASE;
-        pm->pcihp_io_base =
-            object_property_get_uint(obj, ACPI_PCIHP_IO_BASE_PROP, NULL);
-        pm->pcihp_io_len =
-            object_property_get_uint(obj, ACPI_PCIHP_IO_LEN_PROP, NULL);
     }
     if (lpc) {
         struct AcpiGenericAddress r = { .space_id = AML_AS_SYSTEM_IO,
@@ -214,6 +210,10 @@ static void acpi_get_pm_info(MachineState *machine, AcpiPmInfo *pm)
         pm->fadt.flags |= 1 << ACPI_FADT_F_RESET_REG_SUP;
         pm->cpu_hp_io_base = ICH9_CPU_HOTPLUG_IO_BASE;
     }
+    pm->pcihp_io_base =
+        object_property_get_uint(obj, ACPI_PCIHP_IO_BASE_PROP, NULL);
+    pm->pcihp_io_len =
+        object_property_get_uint(obj, ACPI_PCIHP_IO_LEN_PROP, NULL);
 
     /* The above need not be conditional on machine type because the reset port
      * happens to be the same on PIIX (pc) and ICH9 (q35). */
@@ -472,7 +472,7 @@ static void build_append_pci_bus_devices(Aml *parent_scope, PCIBus *bus,
         QLIST_FOREACH(sec, &bus->child, sibling) {
             int32_t devfn = sec->parent_dev->devfn;
 
-            if (pci_bus_is_root(sec) || pci_bus_is_express(sec)) {
+            if (pci_bus_is_root(sec)) {
                 continue;
             }
 
@@ -1586,7 +1586,12 @@ static void build_piix4_pci_hotplug(Aml *table)
     aml_append(table, scope);
 }
 
-static Aml *build_q35_osc_method(void)
+static void build_q35_pci_hotplug(Aml *table)
+{
+    build_piix4_pci_hotplug(table);
+}
+
+static Aml *build_q35_osc_method(AcpiPmInfo *pm)
 {
     Aml *if_ctx;
     Aml *if_ctx2;
@@ -1698,6 +1703,7 @@ build_dsdt(GArray *table_data, BIOSLinker *linker,
         build_hpet_aml(dsdt);
         build_q35_isa_bridge(dsdt);
         build_isa_devices_aml(dsdt);
+        build_q35_pci_hotplug(dsdt);
         build_q35_pci0_int(dsdt);
         if (pcms->smbus && !pcmc->do_not_add_smb_acpi) {
             build_smb0(dsdt, pcms->smbus, ICH9_SMB_DEV, ICH9_SMB_FUNC);
@@ -1724,7 +1730,7 @@ build_dsdt(GArray *table_data, BIOSLinker *linker,
     {
         aml_append(scope, aml_name_decl("_HID", aml_string("ACPI0006")));
 
-        if (misc->is_piix4) {
+        if (misc->is_piix4 || pm->pcihp_bridge_en) {
             method = aml_method("_E01", 0, AML_NOTSERIALIZED);
             aml_append(method,
                 aml_acquire(aml_name("\\_SB.PCI0.BLCK"), 0xFFFF));
-- 
2.25.4

[RFC PATCH 4/5] hw/i386/acpi-build: Turn off support of PCIe native hot-plug and SHPC in _OSC

[Julia Suvorova]

Other methods may be used if the system is capable of this and the _OSC bit
is set. Disable them explicitly to force ACPI PCI hot-plug use. The older
versions will still use PCIe native.

Signed-off-by: Julia Suvorova <jusual@redhat.com>
---
 hw/i386/acpi-build.c | 12 ++++++++----
 1 file changed, 8 insertions(+), 4 deletions(-)

diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
index 5c5ad88ad6..0e2891c3ea 100644
--- a/hw/i386/acpi-build.c
+++ b/hw/i386/acpi-build.c
@@ -1599,6 +1599,7 @@ static Aml *build_q35_osc_method(AcpiPmInfo *pm)
     Aml *method;
     Aml *a_cwd1 = aml_name("CDW1");
     Aml *a_ctrl = aml_local(0);
+    unsigned osc_ctrl;
 
     method = aml_method("_OSC", 4, AML_NOTSERIALIZED);
     aml_append(method, aml_create_dword_field(aml_arg(3), aml_int(0), "CDW1"));
@@ -1612,9 +1613,12 @@ static Aml *build_q35_osc_method(AcpiPmInfo *pm)
 
     /*
      * Always allow native PME, AER (no dependencies)
-     * Allow SHPC (PCI bridges can have SHPC controller)
+     * Need to disable native and SHPC hot-plug to use acpihp
+     *
+     * PCI Firmware Specification, Revision 3.2
      */
-    aml_append(if_ctx, aml_and(a_ctrl, aml_int(0x1F), a_ctrl));
+    osc_ctrl = pm->pcihp_bridge_en ? 0x1C : 0x1F;
+    aml_append(if_ctx, aml_and(a_ctrl, aml_int(osc_ctrl), a_ctrl));
 
     if_ctx2 = aml_if(aml_lnot(aml_equal(aml_arg(1), aml_int(1))));
     /* Unknown revision */
@@ -1696,7 +1700,7 @@ build_dsdt(GArray *table_data, BIOSLinker *linker,
         aml_append(dev, aml_name_decl("_CID", aml_eisaid("PNP0A03")));
         aml_append(dev, aml_name_decl("_ADR", aml_int(0)));
         aml_append(dev, aml_name_decl("_UID", aml_int(1)));
-        aml_append(dev, build_q35_osc_method());
+        aml_append(dev, build_q35_osc_method(pm));
         aml_append(sb_scope, dev);
         aml_append(dsdt, sb_scope);
 
@@ -1771,7 +1775,7 @@ build_dsdt(GArray *table_data, BIOSLinker *linker,
             if (pci_bus_is_express(bus)) {
                 aml_append(dev, aml_name_decl("_HID", aml_eisaid("PNP0A08")));
                 aml_append(dev, aml_name_decl("_CID", aml_eisaid("PNP0A03")));
-                aml_append(dev, build_q35_osc_method());
+                aml_append(dev, build_q35_osc_method(pm));
             } else {
                 aml_append(dev, aml_name_decl("_HID", aml_eisaid("PNP0A03")));
             }
-- 
2.25.4

[RFC PATCH 5/5] hw/acpi/ich9: Enable ACPI PCI hot-plug

[Julia Suvorova]

Add acpi_pcihp to ich9_pm and use ACPI PCI hot-plug by default.

Signed-off-by: Julia Suvorova <jusual@redhat.com>
---
 include/hw/acpi/ich9.h |  3 +++
 hw/acpi/ich9.c         | 45 ++++++++++++++++++++++++++++++++++++++++++
 hw/acpi/pcihp.c        |  3 ++-
 hw/i386/pc.c           |  4 +++-
 4 files changed, 53 insertions(+), 2 deletions(-)

diff --git a/include/hw/acpi/ich9.h b/include/hw/acpi/ich9.h
index 28a53181cb..d345da6b74 100644
--- a/include/hw/acpi/ich9.h
+++ b/include/hw/acpi/ich9.h
@@ -24,6 +24,7 @@
 #include "hw/acpi/acpi.h"
 #include "hw/acpi/cpu_hotplug.h"
 #include "hw/acpi/cpu.h"
+#include "hw/acpi/pcihp.h"
 #include "hw/acpi/memory_hotplug.h"
 #include "hw/acpi/acpi_dev_interface.h"
 #include "hw/acpi/tco.h"
@@ -53,6 +54,8 @@ typedef struct ICH9LPCPMRegs {
     AcpiCpuHotplug gpe_cpu;
     CPUHotplugState cpuhp_state;
 
+    bool use_acpi_pci_hotplug;
+    AcpiPciHpState acpi_pci_hotplug;
     MemHotplugState acpi_memory_hotplug;
 
     uint8_t disable_s3;
diff --git a/hw/acpi/ich9.c b/hw/acpi/ich9.c
index 0fdd736da4..e0291373f2 100644
--- a/hw/acpi/ich9.c
+++ b/hw/acpi/ich9.c
@@ -265,6 +265,7 @@ static void pm_reset(void *opaque)
     }
     pm->smi_en_wmask = ~0;
 
+    acpi_pcihp_reset(&pm->acpi_pci_hotplug);
     acpi_update_sci(&pm->acpi_regs, pm->irq);
 }
 
@@ -303,6 +304,17 @@ void ich9_pm_init(PCIDevice *lpc_pci, ICH9LPCPMRegs *pm,
     pm->enable_tco = true;
     acpi_pm_tco_init(&pm->tco_regs, &pm->io);
 
+    if (pm->use_acpi_pci_hotplug) {
+        acpi_pcihp_init(OBJECT(lpc_pci),
+                        &pm->acpi_pci_hotplug,
+                        pci_get_bus(lpc_pci),
+                        pci_address_space_io(lpc_pci),
+                        true);
+        qbus_set_hotplug_handler(BUS(pci_get_bus(lpc_pci)),
+                                 OBJECT(lpc_pci),
+                                 &error_abort);
+    }
+
     pm->irq = sci_irq;
     qemu_register_reset(pm_reset, pm);
     pm->powerdown_notifier.notify = pm_powerdown_req;
@@ -374,6 +386,20 @@ static void ich9_pm_set_enable_tco(Object *obj, bool value, Error **errp)
     s->pm.enable_tco = value;
 }
 
+static bool ich9_pm_get_acpi_pci_hotplug(Object *obj, Error **errp)
+{
+    ICH9LPCState *s = ICH9_LPC_DEVICE(obj);
+
+    return s->pm.use_acpi_pci_hotplug;
+}
+
+static void ich9_pm_set_acpi_pci_hotplug(Object *obj, bool value,
+                                               Error **errp)
+{
+    ICH9LPCState *s = ICH9_LPC_DEVICE(obj);
+
+    s->pm.use_acpi_pci_hotplug = value;
+}
 void ich9_pm_add_properties(Object *obj, ICH9LPCPMRegs *pm)
 {
     static const uint32_t gpe0_len = ICH9_PMIO_GPE0_LEN;
@@ -382,6 +408,7 @@ void ich9_pm_add_properties(Object *obj, ICH9LPCPMRegs *pm)
     pm->disable_s3 = 0;
     pm->disable_s4 = 0;
     pm->s4_val = 2;
+    pm->use_acpi_pci_hotplug = true;
 
     object_property_add_uint32_ptr(obj, ACPI_PM_PROP_PM_IO_BASE,
                                    &pm->pm_io_base, OBJ_PROP_FLAG_READ);
@@ -405,6 +432,9 @@ void ich9_pm_add_properties(Object *obj, ICH9LPCPMRegs *pm)
     object_property_add_bool(obj, ACPI_PM_PROP_TCO_ENABLED,
                              ich9_pm_get_enable_tco,
                              ich9_pm_set_enable_tco);
+    object_property_add_bool(obj, "acpi-pci-hotplug-with-bridge-support",
+                             ich9_pm_get_acpi_pci_hotplug,
+                             ich9_pm_set_acpi_pci_hotplug);
 }
 
 void ich9_pm_device_pre_plug_cb(HotplugHandler *hotplug_dev, DeviceState *dev,
@@ -412,6 +442,11 @@ 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);
+        return;
+    }
+
     if (object_dynamic_cast(OBJECT(dev), TYPE_PC_DIMM) &&
         !lpc->pm.acpi_memory_hotplug.is_enabled)
         error_setg(errp,
@@ -437,6 +472,9 @@ void ich9_pm_device_plug_cb(HotplugHandler *hotplug_dev, DeviceState *dev,
         } else {
             acpi_cpu_plug_cb(hotplug_dev, &lpc->pm.cpuhp_state, dev, errp);
         }
+    } else if (object_dynamic_cast(OBJECT(dev), TYPE_PCI_DEVICE)) {
+        acpi_pcihp_device_plug_cb(hotplug_dev, &lpc->pm.acpi_pci_hotplug,
+                                  dev, errp);
     } else {
         error_setg(errp, "acpi: device plug request for not supported device"
                    " type: %s", object_get_typename(OBJECT(dev)));
@@ -457,6 +495,10 @@ void ich9_pm_device_unplug_request_cb(HotplugHandler *hotplug_dev,
                !lpc->pm.cpu_hotplug_legacy) {
         acpi_cpu_unplug_request_cb(hotplug_dev, &lpc->pm.cpuhp_state,
                                    dev, errp);
+    } else if (object_dynamic_cast(OBJECT(dev), TYPE_PCI_DEVICE)) {
+        acpi_pcihp_device_unplug_request_cb(hotplug_dev,
+                                            &lpc->pm.acpi_pci_hotplug,
+                                            dev, errp);
     } else {
         error_setg(errp, "acpi: device unplug request for not supported device"
                    " type: %s", object_get_typename(OBJECT(dev)));
@@ -474,6 +516,9 @@ void ich9_pm_device_unplug_cb(HotplugHandler *hotplug_dev, DeviceState *dev,
     } else if (object_dynamic_cast(OBJECT(dev), TYPE_CPU) &&
                !lpc->pm.cpu_hotplug_legacy) {
         acpi_cpu_unplug_cb(&lpc->pm.cpuhp_state, dev, errp);
+    } else if (object_dynamic_cast(OBJECT(dev), TYPE_PCI_DEVICE)) {
+        acpi_pcihp_device_unplug_cb(hotplug_dev, &lpc->pm.acpi_pci_hotplug,
+                                    dev, errp);
     } else {
         error_setg(errp, "acpi: device unplug for not supported device"
                    " type: %s", object_get_typename(OBJECT(dev)));
diff --git a/hw/acpi/pcihp.c b/hw/acpi/pcihp.c
index 3d4ee3af72..d905d1b8f2 100644
--- a/hw/acpi/pcihp.c
+++ b/hw/acpi/pcihp.c
@@ -110,7 +110,8 @@ static void acpi_set_pci_info(void)
     }
     bsel_is_set = true;
 
-    bus = find_i440fx(); /* TODO: Q35 support */
+    bus = find_host();
+
     if (bus) {
         /* Scan all PCI buses. Set property to enable acpi based hotplug. */
         pci_for_each_bus_depth_first(bus, acpi_set_bsel, NULL, &bsel_alloc);
diff --git a/hw/i386/pc.c b/hw/i386/pc.c
index 143ac1c354..21c6eb779e 100644
--- a/hw/i386/pc.c
+++ b/hw/i386/pc.c
@@ -96,7 +96,9 @@
 #include "fw_cfg.h"
 #include "trace.h"
 
-GlobalProperty pc_compat_5_0[] = {};
+GlobalProperty pc_compat_5_0[] = {
+    { "ICH9-LPC", "acpi-pci-hotplug-with-bridge-support", "off" },
+};
 const size_t pc_compat_5_0_len = G_N_ELEMENTS(pc_compat_5_0);
 
 GlobalProperty pc_compat_4_2[] = {
-- 
2.25.4

Re: [RFC PATCH 0/5] Use ACPI PCI hot-plug for q35

[no-reply]

Patchew URL: https://patchew.org/QEMU/20200708224615.114077-1-jusual@redhat.com/



Hi,

This series failed the docker-quick@centos7 build test. Please find the testing commands and
their output below. If you have Docker installed, you can probably reproduce it
locally.

=== TEST SCRIPT BEGIN ===
#!/bin/bash
make docker-image-centos7 V=1 NETWORK=1
time make docker-test-quick@centos7 SHOW_ENV=1 J=14 NETWORK=1
=== TEST SCRIPT END ===

  CC      hw/acpi/tpm.o
  CC      hw/acpi/ipmi.o
/tmp/qemu-test/src/hw/acpi/ich9.c: In function 'ich9_pm_init':
/tmp/qemu-test/src/hw/acpi/ich9.c:315:34: error: too many arguments to function 'qbus_set_hotplug_handler'
                                  &error_abort);
                                  ^
In file included from /tmp/qemu-test/src/include/hw/isa/isa.h:8:0,
---
 void qbus_set_hotplug_handler(BusState *bus, Object *handler);
      ^
  CC      hw/acpi/acpi-stub.o
make: *** [hw/acpi/ich9.o] Error 1
make: *** Waiting for unfinished jobs....
  CC      hw/acpi/aml-build-stub.o
Traceback (most recent call last):
---
    raise CalledProcessError(retcode, cmd)
subprocess.CalledProcessError: Command '['sudo', '-n', 'docker', 'run', '--label', 'com.qemu.instance.uuid=e8dad3af07f046e6acc07d5aab1854b9', '-u', '1001', '--security-opt', 'seccomp=unconfined', '--rm', '-e', 'TARGET_LIST=', '-e', 'EXTRA_CONFIGURE_OPTS=', '-e', 'V=', '-e', 'J=14', '-e', 'DEBUG=', '-e', 'SHOW_ENV=1', '-e', 'CCACHE_DIR=/var/tmp/ccache', '-v', '/home/patchew/.cache/qemu-docker-ccache:/var/tmp/ccache:z', '-v', '/var/tmp/patchew-tester-tmp-_j3k8sfi/src/docker-src.2020-07-08-19.27.02.11252:/var/tmp/qemu:z,ro', 'qemu:centos7', '/var/tmp/qemu/run', 'test-quick']' returned non-zero exit status 2.
filter=--filter=label=com.qemu.instance.uuid=e8dad3af07f046e6acc07d5aab1854b9
make[1]: *** [docker-run] Error 1
make[1]: Leaving directory `/var/tmp/patchew-tester-tmp-_j3k8sfi/src'
make: *** [docker-run-test-quick@centos7] Error 2

real    2m25.350s
user    0m8.686s


The full log is available at
http://patchew.org/logs/20200708224615.114077-1-jusual@redhat.com/testing.docker-quick@centos7/?type=message.
---
Email generated automatically by Patchew [https://patchew.org/].
Please send your feedback to patchew-devel@redhat.com

Re: [RFC PATCH 0/5] Use ACPI PCI hot-plug for q35

[no-reply]

Patchew URL: https://patchew.org/QEMU/20200708224615.114077-1-jusual@redhat.com/



Hi,

This series failed the docker-mingw@fedora build test. Please find the testing commands and
their output below. If you have Docker installed, you can probably reproduce it
locally.

=== TEST SCRIPT BEGIN ===
#! /bin/bash
export ARCH=x86_64
make docker-image-fedora V=1 NETWORK=1
time make docker-test-mingw@fedora J=14 NETWORK=1
=== TEST SCRIPT END ===

  CC      hw/acpi/acpi_interface.o
  CC      hw/acpi/bios-linker-loader.o
/tmp/qemu-test/src/hw/acpi/ich9.c: In function 'ich9_pm_init':
/tmp/qemu-test/src/hw/acpi/ich9.c:313:9: error: too many arguments to function 'qbus_set_hotplug_handler'
  313 |         qbus_set_hotplug_handler(BUS(pci_get_bus(lpc_pci)),
      |         ^~~~~~~~~~~~~~~~~~~~~~~~
In file included from /tmp/qemu-test/src/include/hw/isa/isa.h:8,
---
/tmp/qemu-test/src/include/hw/qdev-core.h:538:6: note: declared here
  538 | void qbus_set_hotplug_handler(BusState *bus, Object *handler);
      |      ^~~~~~~~~~~~~~~~~~~~~~~~
make: *** [/tmp/qemu-test/src/rules.mak:69: hw/acpi/ich9.o] Error 1
make: *** Waiting for unfinished jobs....
  CC      hw/acpi/aml-build.o
Traceback (most recent call last):
---
    raise CalledProcessError(retcode, cmd)
subprocess.CalledProcessError: Command '['sudo', '-n', 'docker', 'run', '--label', 'com.qemu.instance.uuid=f40198b991eb4bf4a15cccbf81eccbf5', '-u', '1003', '--security-opt', 'seccomp=unconfined', '--rm', '-e', 'TARGET_LIST=', '-e', 'EXTRA_CONFIGURE_OPTS=', '-e', 'V=', '-e', 'J=14', '-e', 'DEBUG=', '-e', 'SHOW_ENV=', '-e', 'CCACHE_DIR=/var/tmp/ccache', '-v', '/home/patchew2/.cache/qemu-docker-ccache:/var/tmp/ccache:z', '-v', '/var/tmp/patchew-tester-tmp-tvkth6at/src/docker-src.2020-07-08-19.30.00.18172:/var/tmp/qemu:z,ro', 'qemu:fedora', '/var/tmp/qemu/run', 'test-mingw']' returned non-zero exit status 2.
filter=--filter=label=com.qemu.instance.uuid=f40198b991eb4bf4a15cccbf81eccbf5
make[1]: *** [docker-run] Error 1
make[1]: Leaving directory `/var/tmp/patchew-tester-tmp-tvkth6at/src'
make: *** [docker-run-test-mingw@fedora] Error 2

real    3m32.203s
user    0m8.424s


The full log is available at
http://patchew.org/logs/20200708224615.114077-1-jusual@redhat.com/testing.docker-mingw@fedora/?type=message.
---
Email generated automatically by Patchew [https://patchew.org/].
Please send your feedback to patchew-devel@redhat.com

Re: [RFC PATCH 1/5] hw/acpi/pcihp: Introduce find_host()

[Igor Mammedov]

On Thu,  9 Jul 2020 00:46:11 +0200
Julia Suvorova <jusual@redhat.com> wrote:

> Returns the current host bus with ACPI PCI hot-plug support: q35 or i440fx.
> 
> Signed-off-by: Julia Suvorova <jusual@redhat.com>
> ---
>  hw/i386/acpi-build.h |  2 ++
>  hw/acpi/pcihp.c      | 13 +++++++++++++
>  hw/i386/acpi-build.c |  2 +-
>  3 files changed, 16 insertions(+), 1 deletion(-)
> 
> diff --git a/hw/i386/acpi-build.h b/hw/i386/acpi-build.h
> index 74df5fc612..0696b4e48d 100644
> --- a/hw/i386/acpi-build.h
> +++ b/hw/i386/acpi-build.h
> @@ -7,4 +7,6 @@ extern const struct AcpiGenericAddress x86_nvdimm_acpi_dsmio;
>  
>  void acpi_setup(void);
>  
> +Object *acpi_get_i386_pci_host(void);
> +
>  #endif
> diff --git a/hw/acpi/pcihp.c b/hw/acpi/pcihp.c
> index d42906ea19..3d4ee3af72 100644
> --- a/hw/acpi/pcihp.c
> +++ b/hw/acpi/pcihp.c
> @@ -33,10 +33,12 @@
>  #include "hw/acpi/acpi.h"
>  #include "exec/address-spaces.h"
>  #include "hw/pci/pci_bus.h"
> +#include "hw/pci/pci_host.h"
>  #include "migration/vmstate.h"
>  #include "qapi/error.h"
>  #include "qom/qom-qobject.h"
>  #include "trace.h"
> +#include "hw/i386/acpi-build.h"
>  
>  #define ACPI_PCIHP_ADDR 0xae00
>  #define ACPI_PCIHP_SIZE 0x0014
> @@ -86,6 +88,17 @@ static void *acpi_set_bsel(PCIBus *bus, void *opaque)
>      return bsel_alloc;
>  }
>  
> +static PCIBus *find_host(void)
> +{
> +    Object *obj = acpi_get_i386_pci_host();
> +
> +    if (obj) {
> +        return PCI_HOST_BRIDGE(obj)->bus;
> +    }
> +
> +    return NULL;
> +}

My guess you are adding it for 5/5, with a function name a bit off
compared to what you are doing (probably you've tried to reuse find_i440fx() idea)

I'd just make acpi_get_i386_pci_host() public, drop find_host and use

 host = acpi_get_i386_pci_host()
 bus = PCI_HOST_BRIDGE(pci_host)->bus

like it's done elsewhere

>  static void acpi_set_pci_info(void)
>  {
>      static bool bsel_is_set;
> diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
> index 900f786d08..11c598f955 100644
> --- a/hw/i386/acpi-build.c
> +++ b/hw/i386/acpi-build.c
> @@ -270,7 +270,7 @@ static void acpi_get_misc_info(AcpiMiscInfo *info)
>   * Because of the PXB hosts we cannot simply query TYPE_PCI_HOST_BRIDGE.
>   * On i386 arch we only have two pci hosts, so we can look only for them.
>   */
> -static Object *acpi_get_i386_pci_host(void)
> +Object *acpi_get_i386_pci_host(void)
>  {
>      PCIHostState *host;
>  

Re: [RFC PATCH 2/5] hw/acpi/ich9: Trace ich9_gpe_readb()/writeb()

[Igor Mammedov]

On Thu,  9 Jul 2020 00:46:12 +0200
Julia Suvorova <jusual@redhat.com> wrote:

> Add trace events similar to piix4_gpe_readb() to check gpe status.
> 
> Signed-off-by: Julia Suvorova <jusual@redhat.com>

Reviewed-by: Igor Mammedov <imammedo@redhat.com>

> ---
>  hw/acpi/ich9.c       | 7 ++++++-
>  hw/acpi/trace-events | 4 ++++
>  2 files changed, 10 insertions(+), 1 deletion(-)
> 
> diff --git a/hw/acpi/ich9.c b/hw/acpi/ich9.c
> index 2d204babc6..0fdd736da4 100644
> --- a/hw/acpi/ich9.c
> +++ b/hw/acpi/ich9.c
> @@ -36,6 +36,7 @@
>  #include "hw/acpi/acpi.h"
>  #include "hw/acpi/tco.h"
>  #include "exec/address-spaces.h"
> +#include "trace.h"
>  
>  #include "hw/i386/ich9.h"
>  #include "hw/mem/pc-dimm.h"
> @@ -59,13 +60,17 @@ static void ich9_pm_update_sci_fn(ACPIREGS *regs)
>  static uint64_t ich9_gpe_readb(void *opaque, hwaddr addr, unsigned width)
>  {
>      ICH9LPCPMRegs *pm = opaque;
> -    return acpi_gpe_ioport_readb(&pm->acpi_regs, addr);
> +    uint64_t val = acpi_gpe_ioport_readb(&pm->acpi_regs, addr);
> +
> +    trace_ich9_gpe_readb(addr, width, val);
> +    return val;
>  }
>  
>  static void ich9_gpe_writeb(void *opaque, hwaddr addr, uint64_t val,
>                              unsigned width)
>  {
>      ICH9LPCPMRegs *pm = opaque;
> +    trace_ich9_gpe_writeb(addr, width, val);
>      acpi_gpe_ioport_writeb(&pm->acpi_regs, addr, val);
>      acpi_update_sci(&pm->acpi_regs, pm->irq);
>  }
> diff --git a/hw/acpi/trace-events b/hw/acpi/trace-events
> index afbc77de1c..b9f4827afc 100644
> --- a/hw/acpi/trace-events
> +++ b/hw/acpi/trace-events
> @@ -32,6 +32,10 @@ cpuhp_acpi_ejecting_cpu(uint32_t idx) "0x%"PRIx32
>  cpuhp_acpi_write_ost_ev(uint32_t slot, uint32_t ev) "idx[0x%"PRIx32"] OST EVENT: 0x%"PRIx32
>  cpuhp_acpi_write_ost_status(uint32_t slot, uint32_t st) "idx[0x%"PRIx32"] OST STATUS: 0x%"PRIx32
>  
> +# ich9.c
> +ich9_gpe_readb(uint64_t addr, unsigned width, uint64_t val) "addr: 0x%" PRIx64 " width: %d ==> 0x%" PRIx64
> +ich9_gpe_writeb(uint64_t addr, unsigned width, uint64_t val) "addr: 0x%" PRIx64 " width: %d <== 0x%" PRIx64
> +
>  # pcihp.c
>  acpi_pci_eject_slot(unsigned bsel, unsigned slot) "bsel: %u slot: %u"
>  acpi_pci_unplug(int bsel, int slot) "bsel: %d slot: %d"

Re: [RFC PATCH 3/5] hw/i386/acpi-build: Add ACPI PCI hot-plug methods to q35

[Igor Mammedov]

On Thu,  9 Jul 2020 00:46:13 +0200
Julia Suvorova <jusual@redhat.com> wrote:

> Implement notifications and gpe to support q35 ACPI PCI hot-plug.
> The addresses specified in [1] remain the same to make fewer changes.
> 
> [1] docs/spec/acpi_pci_hotplug.txt

CCing Gerd his opinion on reusing piix4 IO port range for q35

 
> Signed-off-by: Julia Suvorova <jusual@redhat.com>
> ---
>  hw/i386/acpi-build.c | 20 +++++++++++++-------
>  1 file changed, 13 insertions(+), 7 deletions(-)
> 
> diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
> index 11c598f955..5c5ad88ad6 100644
> --- a/hw/i386/acpi-build.c
> +++ b/hw/i386/acpi-build.c
> @@ -201,10 +201,6 @@ static void acpi_get_pm_info(MachineState *machine, AcpiPmInfo *pm)
>          /* w2k requires FADT(rev1) or it won't boot, keep PC compatible */
>          pm->fadt.rev = 1;
>          pm->cpu_hp_io_base = PIIX4_CPU_HOTPLUG_IO_BASE;
> -        pm->pcihp_io_base =
> -            object_property_get_uint(obj, ACPI_PCIHP_IO_BASE_PROP, NULL);
> -        pm->pcihp_io_len =
> -            object_property_get_uint(obj, ACPI_PCIHP_IO_LEN_PROP, NULL);
>      }
>      if (lpc) {
>          struct AcpiGenericAddress r = { .space_id = AML_AS_SYSTEM_IO,
> @@ -214,6 +210,10 @@ static void acpi_get_pm_info(MachineState *machine, AcpiPmInfo *pm)
>          pm->fadt.flags |= 1 << ACPI_FADT_F_RESET_REG_SUP;
>          pm->cpu_hp_io_base = ICH9_CPU_HOTPLUG_IO_BASE;
>      }
> +    pm->pcihp_io_base =
> +        object_property_get_uint(obj, ACPI_PCIHP_IO_BASE_PROP, NULL);
> +    pm->pcihp_io_len =
> +        object_property_get_uint(obj, ACPI_PCIHP_IO_LEN_PROP, NULL);
>  
>      /* The above need not be conditional on machine type because the reset port
>       * happens to be the same on PIIX (pc) and ICH9 (q35). */
> @@ -472,7 +472,7 @@ static void build_append_pci_bus_devices(Aml *parent_scope, PCIBus *bus,
>          QLIST_FOREACH(sec, &bus->child, sibling) {
>              int32_t devfn = sec->parent_dev->devfn;
>  
> -            if (pci_bus_is_root(sec) || pci_bus_is_express(sec)) {
> +            if (pci_bus_is_root(sec)) {
>                  continue;
>              }
>  
> @@ -1586,7 +1586,12 @@ static void build_piix4_pci_hotplug(Aml *table)
>      aml_append(table, scope);
>  }
>  
> -static Aml *build_q35_osc_method(void)
> +static void build_q35_pci_hotplug(Aml *table)
> +{
> +    build_piix4_pci_hotplug(table);
> +}

s/build_piix4_pci_hotplug/build_i386_acpi_pci_hotplug/

and reuse it in both cases, instead of adding wrapper?

> +
> +static Aml *build_q35_osc_method(AcpiPmInfo *pm)
>  {
>      Aml *if_ctx;
>      Aml *if_ctx2;
> @@ -1698,6 +1703,7 @@ build_dsdt(GArray *table_data, BIOSLinker *linker,
>          build_hpet_aml(dsdt);
>          build_q35_isa_bridge(dsdt);
>          build_isa_devices_aml(dsdt);
> +        build_q35_pci_hotplug(dsdt);
>          build_q35_pci0_int(dsdt);
>          if (pcms->smbus && !pcmc->do_not_add_smb_acpi) {
>              build_smb0(dsdt, pcms->smbus, ICH9_SMB_DEV, ICH9_SMB_FUNC);
> @@ -1724,7 +1730,7 @@ build_dsdt(GArray *table_data, BIOSLinker *linker,
>      {
>          aml_append(scope, aml_name_decl("_HID", aml_string("ACPI0006")));
>  
> -        if (misc->is_piix4) {
> +        if (misc->is_piix4 || pm->pcihp_bridge_en) {
>              method = aml_method("_E01", 0, AML_NOTSERIALIZED);
>              aml_append(method,
>                  aml_acquire(aml_name("\\_SB.PCI0.BLCK"), 0xFFFF));

Re: [RFC PATCH 4/5] hw/i386/acpi-build: Turn off support of PCIe native hot-plug and SHPC in _OSC

[Igor Mammedov]

On Thu,  9 Jul 2020 00:46:14 +0200
Julia Suvorova <jusual@redhat.com> wrote:

> Other methods may be used if the system is capable of this and the _OSC bit
> is set. Disable them explicitly to force ACPI PCI hot-plug use. The older
> versions will still use PCIe native.
> 
> Signed-off-by: Julia Suvorova <jusual@redhat.com>
> ---
>  hw/i386/acpi-build.c | 12 ++++++++----
>  1 file changed, 8 insertions(+), 4 deletions(-)
> 
> diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
> index 5c5ad88ad6..0e2891c3ea 100644
> --- a/hw/i386/acpi-build.c
> +++ b/hw/i386/acpi-build.c
> @@ -1599,6 +1599,7 @@ static Aml *build_q35_osc_method(AcpiPmInfo *pm)
>      Aml *method;
>      Aml *a_cwd1 = aml_name("CDW1");
>      Aml *a_ctrl = aml_local(0);
> +    unsigned osc_ctrl;
>  
>      method = aml_method("_OSC", 4, AML_NOTSERIALIZED);
>      aml_append(method, aml_create_dword_field(aml_arg(3), aml_int(0), "CDW1"));
> @@ -1612,9 +1613,12 @@ static Aml *build_q35_osc_method(AcpiPmInfo *pm)
>  
>      /*
>       * Always allow native PME, AER (no dependencies)
> -     * Allow SHPC (PCI bridges can have SHPC controller)
> +     * Need to disable native and SHPC hot-plug to use acpihp
> +     *
> +     * PCI Firmware Specification, Revision 3.2
seems incomplete, were you going to point to a concrete chapter that requires this change?

>       */
> -    aml_append(if_ctx, aml_and(a_ctrl, aml_int(0x1F), a_ctrl));
> +    osc_ctrl = pm->pcihp_bridge_en ? 0x1C : 0x1F;
Since you are touching this, how about converting this magic number to
something more readable?
i.e.
            set_bit(ACPI_OSC_SHPC_EN,  osc_ctrl)
          or
            osc_ctrl |= BIT(SOME_FEATURE)

> +    aml_append(if_ctx, aml_and(a_ctrl, aml_int(osc_ctrl), a_ctrl));
>  
>      if_ctx2 = aml_if(aml_lnot(aml_equal(aml_arg(1), aml_int(1))));
>      /* Unknown revision */
> @@ -1696,7 +1700,7 @@ build_dsdt(GArray *table_data, BIOSLinker *linker,
>          aml_append(dev, aml_name_decl("_CID", aml_eisaid("PNP0A03")));
>          aml_append(dev, aml_name_decl("_ADR", aml_int(0)));
>          aml_append(dev, aml_name_decl("_UID", aml_int(1)));
> -        aml_append(dev, build_q35_osc_method());
> +        aml_append(dev, build_q35_osc_method(pm));
>          aml_append(sb_scope, dev);
>          aml_append(dsdt, sb_scope);
>  
> @@ -1771,7 +1775,7 @@ build_dsdt(GArray *table_data, BIOSLinker *linker,
>              if (pci_bus_is_express(bus)) {
>                  aml_append(dev, aml_name_decl("_HID", aml_eisaid("PNP0A08")));
>                  aml_append(dev, aml_name_decl("_CID", aml_eisaid("PNP0A03")));
> -                aml_append(dev, build_q35_osc_method());
> +                aml_append(dev, build_q35_osc_method(pm));
>              } else {
>                  aml_append(dev, aml_name_decl("_HID", aml_eisaid("PNP0A03")));
>              }

Re: [RFC PATCH 5/5] hw/acpi/ich9: Enable ACPI PCI hot-plug

[Igor Mammedov]

On Thu,  9 Jul 2020 00:46:15 +0200
Julia Suvorova <jusual@redhat.com> wrote:

> Add acpi_pcihp to ich9_pm and use ACPI PCI hot-plug by default.
> 
> Signed-off-by: Julia Suvorova <jusual@redhat.com>
> ---
>  include/hw/acpi/ich9.h |  3 +++
>  hw/acpi/ich9.c         | 45 ++++++++++++++++++++++++++++++++++++++++++
>  hw/acpi/pcihp.c        |  3 ++-
>  hw/i386/pc.c           |  4 +++-
>  4 files changed, 53 insertions(+), 2 deletions(-)
> 
> diff --git a/include/hw/acpi/ich9.h b/include/hw/acpi/ich9.h
> index 28a53181cb..d345da6b74 100644
> --- a/include/hw/acpi/ich9.h
> +++ b/include/hw/acpi/ich9.h
> @@ -24,6 +24,7 @@
>  #include "hw/acpi/acpi.h"
>  #include "hw/acpi/cpu_hotplug.h"
>  #include "hw/acpi/cpu.h"
> +#include "hw/acpi/pcihp.h"
>  #include "hw/acpi/memory_hotplug.h"
>  #include "hw/acpi/acpi_dev_interface.h"
>  #include "hw/acpi/tco.h"
> @@ -53,6 +54,8 @@ typedef struct ICH9LPCPMRegs {
>      AcpiCpuHotplug gpe_cpu;
>      CPUHotplugState cpuhp_state;
>  
> +    bool use_acpi_pci_hotplug;
a bit outdated naming,
see 0affda043675c7619248a924a89bfd3781759f63, and rename it to match piix4

> +    AcpiPciHpState acpi_pci_hotplug;
>      MemHotplugState acpi_memory_hotplug;
>  
>      uint8_t disable_s3;
> diff --git a/hw/acpi/ich9.c b/hw/acpi/ich9.c
> index 0fdd736da4..e0291373f2 100644
> --- a/hw/acpi/ich9.c
> +++ b/hw/acpi/ich9.c
> @@ -265,6 +265,7 @@ static void pm_reset(void *opaque)
>      }
>      pm->smi_en_wmask = ~0;
>  
> +    acpi_pcihp_reset(&pm->acpi_pci_hotplug);
>      acpi_update_sci(&pm->acpi_regs, pm->irq);
>  }
>  
> @@ -303,6 +304,17 @@ void ich9_pm_init(PCIDevice *lpc_pci, ICH9LPCPMRegs *pm,
>      pm->enable_tco = true;
>      acpi_pm_tco_init(&pm->tco_regs, &pm->io);
>  
> +    if (pm->use_acpi_pci_hotplug) {
> +        acpi_pcihp_init(OBJECT(lpc_pci),
> +                        &pm->acpi_pci_hotplug,
> +                        pci_get_bus(lpc_pci),
> +                        pci_address_space_io(lpc_pci),
> +                        true);
> +        qbus_set_hotplug_handler(BUS(pci_get_bus(lpc_pci)),
> +                                 OBJECT(lpc_pci),
> +                                 &error_abort);
> +    }
> +
>      pm->irq = sci_irq;
>      qemu_register_reset(pm_reset, pm);
>      pm->powerdown_notifier.notify = pm_powerdown_req;
> @@ -374,6 +386,20 @@ static void ich9_pm_set_enable_tco(Object *obj, bool value, Error **errp)
>      s->pm.enable_tco = value;
>  }
>  
> +static bool ich9_pm_get_acpi_pci_hotplug(Object *obj, Error **errp)
> +{
> +    ICH9LPCState *s = ICH9_LPC_DEVICE(obj);
> +
> +    return s->pm.use_acpi_pci_hotplug;
> +}
> +
> +static void ich9_pm_set_acpi_pci_hotplug(Object *obj, bool value,
> +                                               Error **errp)
> +{
> +    ICH9LPCState *s = ICH9_LPC_DEVICE(obj);
> +
> +    s->pm.use_acpi_pci_hotplug = value;
> +}
>  void ich9_pm_add_properties(Object *obj, ICH9LPCPMRegs *pm)
>  {
>      static const uint32_t gpe0_len = ICH9_PMIO_GPE0_LEN;
> @@ -382,6 +408,7 @@ void ich9_pm_add_properties(Object *obj, ICH9LPCPMRegs *pm)
>      pm->disable_s3 = 0;
>      pm->disable_s4 = 0;
>      pm->s4_val = 2;
> +    pm->use_acpi_pci_hotplug = true;

I'm not sure about making crutch on by default though.

Does linux guests work fine with native hotplug or
does it have the same issues as Windows?

Also you had an idea how to workaround native hotplug issues
on QEMU side.
Can you dump here a quick summary why it didn't work out
in the end?
 

>  
>      object_property_add_uint32_ptr(obj, ACPI_PM_PROP_PM_IO_BASE,
>                                     &pm->pm_io_base, OBJ_PROP_FLAG_READ);
> @@ -405,6 +432,9 @@ void ich9_pm_add_properties(Object *obj, ICH9LPCPMRegs *pm)
>      object_property_add_bool(obj, ACPI_PM_PROP_TCO_ENABLED,
>                               ich9_pm_get_enable_tco,
>                               ich9_pm_set_enable_tco);
> +    object_property_add_bool(obj, "acpi-pci-hotplug-with-bridge-support",
> +                             ich9_pm_get_acpi_pci_hotplug,
> +                             ich9_pm_set_acpi_pci_hotplug);
>  }
>  
>  void ich9_pm_device_pre_plug_cb(HotplugHandler *hotplug_dev, DeviceState *dev,
> @@ -412,6 +442,11 @@ 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);
> +        return;
> +    }
> +
>      if (object_dynamic_cast(OBJECT(dev), TYPE_PC_DIMM) &&
>          !lpc->pm.acpi_memory_hotplug.is_enabled)
>          error_setg(errp,
> @@ -437,6 +472,9 @@ void ich9_pm_device_plug_cb(HotplugHandler *hotplug_dev, DeviceState *dev,
>          } else {
>              acpi_cpu_plug_cb(hotplug_dev, &lpc->pm.cpuhp_state, dev, errp);
>          }
> +    } else if (object_dynamic_cast(OBJECT(dev), TYPE_PCI_DEVICE)) {
> +        acpi_pcihp_device_plug_cb(hotplug_dev, &lpc->pm.acpi_pci_hotplug,
> +                                  dev, errp);
>      } else {
>          error_setg(errp, "acpi: device plug request for not supported device"
>                     " type: %s", object_get_typename(OBJECT(dev)));
> @@ -457,6 +495,10 @@ void ich9_pm_device_unplug_request_cb(HotplugHandler *hotplug_dev,
>                 !lpc->pm.cpu_hotplug_legacy) {
>          acpi_cpu_unplug_request_cb(hotplug_dev, &lpc->pm.cpuhp_state,
>                                     dev, errp);
> +    } else if (object_dynamic_cast(OBJECT(dev), TYPE_PCI_DEVICE)) {
> +        acpi_pcihp_device_unplug_request_cb(hotplug_dev,
> +                                            &lpc->pm.acpi_pci_hotplug,
> +                                            dev, errp);
>      } else {
>          error_setg(errp, "acpi: device unplug request for not supported device"
>                     " type: %s", object_get_typename(OBJECT(dev)));
> @@ -474,6 +516,9 @@ void ich9_pm_device_unplug_cb(HotplugHandler *hotplug_dev, DeviceState *dev,
>      } else if (object_dynamic_cast(OBJECT(dev), TYPE_CPU) &&
>                 !lpc->pm.cpu_hotplug_legacy) {
>          acpi_cpu_unplug_cb(&lpc->pm.cpuhp_state, dev, errp);
> +    } else if (object_dynamic_cast(OBJECT(dev), TYPE_PCI_DEVICE)) {
> +        acpi_pcihp_device_unplug_cb(hotplug_dev, &lpc->pm.acpi_pci_hotplug,
> +                                    dev, errp);
>      } else {
>          error_setg(errp, "acpi: device unplug for not supported device"
>                     " type: %s", object_get_typename(OBJECT(dev)));
> diff --git a/hw/acpi/pcihp.c b/hw/acpi/pcihp.c
> index 3d4ee3af72..d905d1b8f2 100644
> --- a/hw/acpi/pcihp.c
> +++ b/hw/acpi/pcihp.c
> @@ -110,7 +110,8 @@ static void acpi_set_pci_info(void)
>      }
>      bsel_is_set = true;
>  
> -    bus = find_i440fx(); /* TODO: Q35 support */
> +    bus = find_host();
> +
>      if (bus) {
>          /* Scan all PCI buses. Set property to enable acpi based hotplug. */
>          pci_for_each_bus_depth_first(bus, acpi_set_bsel, NULL, &bsel_alloc);
> diff --git a/hw/i386/pc.c b/hw/i386/pc.c
> index 143ac1c354..21c6eb779e 100644
> --- a/hw/i386/pc.c
> +++ b/hw/i386/pc.c
> @@ -96,7 +96,9 @@
>  #include "fw_cfg.h"
>  #include "trace.h"
>  
> -GlobalProperty pc_compat_5_0[] = {};
> +GlobalProperty pc_compat_5_0[] = {
> +    { "ICH9-LPC", "acpi-pci-hotplug-with-bridge-support", "off" },
> +};
>  const size_t pc_compat_5_0_len = G_N_ELEMENTS(pc_compat_5_0);
>  
>  GlobalProperty pc_compat_4_2[] = {

Re: [RFC PATCH 4/5] hw/i386/acpi-build: Turn off support of PCIe native hot-plug and SHPC in _OSC

[Michael S. Tsirkin]

On Mon, Jul 13, 2020 at 04:56:54PM +0200, Igor Mammedov wrote:
> On Thu,  9 Jul 2020 00:46:14 +0200
> Julia Suvorova <jusual@redhat.com> wrote:
> 
> > Other methods may be used if the system is capable of this and the _OSC bit
> > is set. Disable them explicitly to force ACPI PCI hot-plug use. The older
> > versions will still use PCIe native.

Do we need that later part btw?

> > 
> > Signed-off-by: Julia Suvorova <jusual@redhat.com>
> > ---
> >  hw/i386/acpi-build.c | 12 ++++++++----
> >  1 file changed, 8 insertions(+), 4 deletions(-)
> > 
> > diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
> > index 5c5ad88ad6..0e2891c3ea 100644
> > --- a/hw/i386/acpi-build.c
> > +++ b/hw/i386/acpi-build.c
> > @@ -1599,6 +1599,7 @@ static Aml *build_q35_osc_method(AcpiPmInfo *pm)
> >      Aml *method;
> >      Aml *a_cwd1 = aml_name("CDW1");
> >      Aml *a_ctrl = aml_local(0);
> > +    unsigned osc_ctrl;
> >  
> >      method = aml_method("_OSC", 4, AML_NOTSERIALIZED);
> >      aml_append(method, aml_create_dword_field(aml_arg(3), aml_int(0), "CDW1"));
> > @@ -1612,9 +1613,12 @@ static Aml *build_q35_osc_method(AcpiPmInfo *pm)
> >  
> >      /*
> >       * Always allow native PME, AER (no dependencies)
> > -     * Allow SHPC (PCI bridges can have SHPC controller)
> > +     * Need to disable native and SHPC hot-plug to use acpihp
> > +     *
> > +     * PCI Firmware Specification, Revision 3.2

I don't think you have to add a reference as part of this patchset.
The spec in question documents _OSC so it's not a bad idea to
add it, but it's a bit more work, e.g. we generally try to list
the earliest spec that documents the given feature, since
So I suspect this is 3.0 actually.


> seems incomplete, were you going to point to a concrete chapter that requires this change?


It doesn't as such. A better description would be:

/ * Guests seem to generally prefer native hotplug control. As we want them to
  * use ACPI, don't enable it.
  */



> >       */
> > -    aml_append(if_ctx, aml_and(a_ctrl, aml_int(0x1F), a_ctrl));
> > +    osc_ctrl = pm->pcihp_bridge_en ? 0x1C : 0x1F;
> Since you are touching this, how about converting this magic number to
> something more readable?
> i.e.
>             set_bit(ACPI_OSC_SHPC_EN,  osc_ctrl)
>           or
>             osc_ctrl |= BIT(SOME_FEATURE)
> 

... if there is such a macro. If not I suspect it's better as a comment:

	0x10 /* PCI Express Capability Structure control */ |
	0x80 /* PCI Express Advanced Error Reporting control */ |
	0x40 /* PCI Express Native Power Management Events control */



> > +    aml_append(if_ctx, aml_and(a_ctrl, aml_int(osc_ctrl), a_ctrl));
> >  
> >      if_ctx2 = aml_if(aml_lnot(aml_equal(aml_arg(1), aml_int(1))));
> >      /* Unknown revision */
> > @@ -1696,7 +1700,7 @@ build_dsdt(GArray *table_data, BIOSLinker *linker,
> >          aml_append(dev, aml_name_decl("_CID", aml_eisaid("PNP0A03")));
> >          aml_append(dev, aml_name_decl("_ADR", aml_int(0)));
> >          aml_append(dev, aml_name_decl("_UID", aml_int(1)));
> > -        aml_append(dev, build_q35_osc_method());
> > +        aml_append(dev, build_q35_osc_method(pm));
> >          aml_append(sb_scope, dev);
> >          aml_append(dsdt, sb_scope);
> >  
> > @@ -1771,7 +1775,7 @@ build_dsdt(GArray *table_data, BIOSLinker *linker,
> >              if (pci_bus_is_express(bus)) {
> >                  aml_append(dev, aml_name_decl("_HID", aml_eisaid("PNP0A08")));
> >                  aml_append(dev, aml_name_decl("_CID", aml_eisaid("PNP0A03")));
> > -                aml_append(dev, build_q35_osc_method());
> > +                aml_append(dev, build_q35_osc_method(pm));
> >              } else {
> >                  aml_append(dev, aml_name_decl("_HID", aml_eisaid("PNP0A03")));
> >              }

Re: [RFC PATCH 5/5] hw/acpi/ich9: Enable ACPI PCI hot-plug

[Michael S. Tsirkin]

On Mon, Jul 13, 2020 at 05:17:18PM +0200, Igor Mammedov wrote:
> On Thu,  9 Jul 2020 00:46:15 +0200
> Julia Suvorova <jusual@redhat.com> wrote:
> 
> > Add acpi_pcihp to ich9_pm and use ACPI PCI hot-plug by default.
> > 
> > Signed-off-by: Julia Suvorova <jusual@redhat.com>
> > ---
> >  include/hw/acpi/ich9.h |  3 +++
> >  hw/acpi/ich9.c         | 45 ++++++++++++++++++++++++++++++++++++++++++
> >  hw/acpi/pcihp.c        |  3 ++-
> >  hw/i386/pc.c           |  4 +++-
> >  4 files changed, 53 insertions(+), 2 deletions(-)
> > 
> > diff --git a/include/hw/acpi/ich9.h b/include/hw/acpi/ich9.h
> > index 28a53181cb..d345da6b74 100644
> > --- a/include/hw/acpi/ich9.h
> > +++ b/include/hw/acpi/ich9.h
> > @@ -24,6 +24,7 @@
> >  #include "hw/acpi/acpi.h"
> >  #include "hw/acpi/cpu_hotplug.h"
> >  #include "hw/acpi/cpu.h"
> > +#include "hw/acpi/pcihp.h"
> >  #include "hw/acpi/memory_hotplug.h"
> >  #include "hw/acpi/acpi_dev_interface.h"
> >  #include "hw/acpi/tco.h"
> > @@ -53,6 +54,8 @@ typedef struct ICH9LPCPMRegs {
> >      AcpiCpuHotplug gpe_cpu;
> >      CPUHotplugState cpuhp_state;
> >  
> > +    bool use_acpi_pci_hotplug;
> a bit outdated naming,
> see 0affda043675c7619248a924a89bfd3781759f63, and rename it to match piix4
> 
> > +    AcpiPciHpState acpi_pci_hotplug;
> >      MemHotplugState acpi_memory_hotplug;
> >  
> >      uint8_t disable_s3;
> > diff --git a/hw/acpi/ich9.c b/hw/acpi/ich9.c
> > index 0fdd736da4..e0291373f2 100644
> > --- a/hw/acpi/ich9.c
> > +++ b/hw/acpi/ich9.c
> > @@ -265,6 +265,7 @@ static void pm_reset(void *opaque)
> >      }
> >      pm->smi_en_wmask = ~0;
> >  
> > +    acpi_pcihp_reset(&pm->acpi_pci_hotplug);
> >      acpi_update_sci(&pm->acpi_regs, pm->irq);
> >  }
> >  
> > @@ -303,6 +304,17 @@ void ich9_pm_init(PCIDevice *lpc_pci, ICH9LPCPMRegs *pm,
> >      pm->enable_tco = true;
> >      acpi_pm_tco_init(&pm->tco_regs, &pm->io);
> >  
> > +    if (pm->use_acpi_pci_hotplug) {
> > +        acpi_pcihp_init(OBJECT(lpc_pci),
> > +                        &pm->acpi_pci_hotplug,
> > +                        pci_get_bus(lpc_pci),
> > +                        pci_address_space_io(lpc_pci),
> > +                        true);
> > +        qbus_set_hotplug_handler(BUS(pci_get_bus(lpc_pci)),
> > +                                 OBJECT(lpc_pci),
> > +                                 &error_abort);
> > +    }
> > +
> >      pm->irq = sci_irq;
> >      qemu_register_reset(pm_reset, pm);
> >      pm->powerdown_notifier.notify = pm_powerdown_req;
> > @@ -374,6 +386,20 @@ static void ich9_pm_set_enable_tco(Object *obj, bool value, Error **errp)
> >      s->pm.enable_tco = value;
> >  }
> >  
> > +static bool ich9_pm_get_acpi_pci_hotplug(Object *obj, Error **errp)
> > +{
> > +    ICH9LPCState *s = ICH9_LPC_DEVICE(obj);
> > +
> > +    return s->pm.use_acpi_pci_hotplug;
> > +}
> > +
> > +static void ich9_pm_set_acpi_pci_hotplug(Object *obj, bool value,
> > +                                               Error **errp)
> > +{
> > +    ICH9LPCState *s = ICH9_LPC_DEVICE(obj);
> > +
> > +    s->pm.use_acpi_pci_hotplug = value;
> > +}
> >  void ich9_pm_add_properties(Object *obj, ICH9LPCPMRegs *pm)
> >  {
> >      static const uint32_t gpe0_len = ICH9_PMIO_GPE0_LEN;
> > @@ -382,6 +408,7 @@ void ich9_pm_add_properties(Object *obj, ICH9LPCPMRegs *pm)
> >      pm->disable_s3 = 0;
> >      pm->disable_s4 = 0;
> >      pm->s4_val = 2;
> > +    pm->use_acpi_pci_hotplug = true;
> 
> I'm not sure about making crutch on by default though.
> 
> Does linux guests work fine with native hotplug or
> does it have the same issues as Windows?
> 
> Also you had an idea how to workaround native hotplug issues
> on QEMU side.
> Can you dump here a quick summary why it didn't work out
> in the end?

To help out with that a bit:

I think native hotplug just does not match what management needs from
QEMU. QEMU wants what ACPI calls a "VCR-style" ejection where
eject happens automatically after software allows it.

Native hotplug is designed with a human in the loop.

Things like multi-second delay after an eject request,
inability to differentiate between power down and eject,
ability to power device back up after power down
are all architectural in hative hotplug.

Interface is also designed such that there are subtle races making it
hard not to loose hotplug events are also harmless on bare-metal since a
human can just press the eject button again, but with causes no end of
issues for us.

I agree it's a good idea to include the motivation here or in the commit
log, though.

> 
> >  
> >      object_property_add_uint32_ptr(obj, ACPI_PM_PROP_PM_IO_BASE,
> >                                     &pm->pm_io_base, OBJ_PROP_FLAG_READ);
> > @@ -405,6 +432,9 @@ void ich9_pm_add_properties(Object *obj, ICH9LPCPMRegs *pm)
> >      object_property_add_bool(obj, ACPI_PM_PROP_TCO_ENABLED,
> >                               ich9_pm_get_enable_tco,
> >                               ich9_pm_set_enable_tco);
> > +    object_property_add_bool(obj, "acpi-pci-hotplug-with-bridge-support",
> > +                             ich9_pm_get_acpi_pci_hotplug,
> > +                             ich9_pm_set_acpi_pci_hotplug);
> >  }
> >  
> >  void ich9_pm_device_pre_plug_cb(HotplugHandler *hotplug_dev, DeviceState *dev,
> > @@ -412,6 +442,11 @@ 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);
> > +        return;
> > +    }
> > +
> >      if (object_dynamic_cast(OBJECT(dev), TYPE_PC_DIMM) &&
> >          !lpc->pm.acpi_memory_hotplug.is_enabled)
> >          error_setg(errp,
> > @@ -437,6 +472,9 @@ void ich9_pm_device_plug_cb(HotplugHandler *hotplug_dev, DeviceState *dev,
> >          } else {
> >              acpi_cpu_plug_cb(hotplug_dev, &lpc->pm.cpuhp_state, dev, errp);
> >          }
> > +    } else if (object_dynamic_cast(OBJECT(dev), TYPE_PCI_DEVICE)) {
> > +        acpi_pcihp_device_plug_cb(hotplug_dev, &lpc->pm.acpi_pci_hotplug,
> > +                                  dev, errp);
> >      } else {
> >          error_setg(errp, "acpi: device plug request for not supported device"
> >                     " type: %s", object_get_typename(OBJECT(dev)));
> > @@ -457,6 +495,10 @@ void ich9_pm_device_unplug_request_cb(HotplugHandler *hotplug_dev,
> >                 !lpc->pm.cpu_hotplug_legacy) {
> >          acpi_cpu_unplug_request_cb(hotplug_dev, &lpc->pm.cpuhp_state,
> >                                     dev, errp);
> > +    } else if (object_dynamic_cast(OBJECT(dev), TYPE_PCI_DEVICE)) {
> > +        acpi_pcihp_device_unplug_request_cb(hotplug_dev,
> > +                                            &lpc->pm.acpi_pci_hotplug,
> > +                                            dev, errp);
> >      } else {
> >          error_setg(errp, "acpi: device unplug request for not supported device"
> >                     " type: %s", object_get_typename(OBJECT(dev)));
> > @@ -474,6 +516,9 @@ void ich9_pm_device_unplug_cb(HotplugHandler *hotplug_dev, DeviceState *dev,
> >      } else if (object_dynamic_cast(OBJECT(dev), TYPE_CPU) &&
> >                 !lpc->pm.cpu_hotplug_legacy) {
> >          acpi_cpu_unplug_cb(&lpc->pm.cpuhp_state, dev, errp);
> > +    } else if (object_dynamic_cast(OBJECT(dev), TYPE_PCI_DEVICE)) {
> > +        acpi_pcihp_device_unplug_cb(hotplug_dev, &lpc->pm.acpi_pci_hotplug,
> > +                                    dev, errp);
> >      } else {
> >          error_setg(errp, "acpi: device unplug for not supported device"
> >                     " type: %s", object_get_typename(OBJECT(dev)));
> > diff --git a/hw/acpi/pcihp.c b/hw/acpi/pcihp.c
> > index 3d4ee3af72..d905d1b8f2 100644
> > --- a/hw/acpi/pcihp.c
> > +++ b/hw/acpi/pcihp.c
> > @@ -110,7 +110,8 @@ static void acpi_set_pci_info(void)
> >      }
> >      bsel_is_set = true;
> >  
> > -    bus = find_i440fx(); /* TODO: Q35 support */
> > +    bus = find_host();
> > +
> >      if (bus) {
> >          /* Scan all PCI buses. Set property to enable acpi based hotplug. */
> >          pci_for_each_bus_depth_first(bus, acpi_set_bsel, NULL, &bsel_alloc);
> > diff --git a/hw/i386/pc.c b/hw/i386/pc.c
> > index 143ac1c354..21c6eb779e 100644
> > --- a/hw/i386/pc.c
> > +++ b/hw/i386/pc.c
> > @@ -96,7 +96,9 @@
> >  #include "fw_cfg.h"
> >  #include "trace.h"
> >  
> > -GlobalProperty pc_compat_5_0[] = {};
> > +GlobalProperty pc_compat_5_0[] = {
> > +    { "ICH9-LPC", "acpi-pci-hotplug-with-bridge-support", "off" },
> > +};
> >  const size_t pc_compat_5_0_len = G_N_ELEMENTS(pc_compat_5_0);
> >  
> >  GlobalProperty pc_compat_4_2[] = {

Re: [RFC PATCH 3/5] hw/i386/acpi-build: Add ACPI PCI hot-plug methods to q35

[Michael S. Tsirkin]

On Mon, Jul 13, 2020 at 04:39:54PM +0200, Igor Mammedov wrote:
> On Thu,  9 Jul 2020 00:46:13 +0200
> Julia Suvorova <jusual@redhat.com> wrote:
> 
> > Implement notifications and gpe to support q35 ACPI PCI hot-plug.
> > The addresses specified in [1] remain the same to make fewer changes.
> > 
> > [1] docs/spec/acpi_pci_hotplug.txt
> 
> CCing Gerd his opinion on reusing piix4 IO port range for q35
> 
>  
> > Signed-off-by: Julia Suvorova <jusual@redhat.com>
> > ---
> >  hw/i386/acpi-build.c | 20 +++++++++++++-------
> >  1 file changed, 13 insertions(+), 7 deletions(-)
> > 
> > diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
> > index 11c598f955..5c5ad88ad6 100644
> > --- a/hw/i386/acpi-build.c
> > +++ b/hw/i386/acpi-build.c
> > @@ -201,10 +201,6 @@ static void acpi_get_pm_info(MachineState *machine, AcpiPmInfo *pm)
> >          /* w2k requires FADT(rev1) or it won't boot, keep PC compatible */
> >          pm->fadt.rev = 1;
> >          pm->cpu_hp_io_base = PIIX4_CPU_HOTPLUG_IO_BASE;
> > -        pm->pcihp_io_base =
> > -            object_property_get_uint(obj, ACPI_PCIHP_IO_BASE_PROP, NULL);
> > -        pm->pcihp_io_len =
> > -            object_property_get_uint(obj, ACPI_PCIHP_IO_LEN_PROP, NULL);
> >      }
> >      if (lpc) {
> >          struct AcpiGenericAddress r = { .space_id = AML_AS_SYSTEM_IO,
> > @@ -214,6 +210,10 @@ static void acpi_get_pm_info(MachineState *machine, AcpiPmInfo *pm)
> >          pm->fadt.flags |= 1 << ACPI_FADT_F_RESET_REG_SUP;
> >          pm->cpu_hp_io_base = ICH9_CPU_HOTPLUG_IO_BASE;
> >      }
> > +    pm->pcihp_io_base =
> > +        object_property_get_uint(obj, ACPI_PCIHP_IO_BASE_PROP, NULL);
> > +    pm->pcihp_io_len =
> > +        object_property_get_uint(obj, ACPI_PCIHP_IO_LEN_PROP, NULL);
> >  
> >      /* The above need not be conditional on machine type because the reset port
> >       * happens to be the same on PIIX (pc) and ICH9 (q35). */
> > @@ -472,7 +472,7 @@ static void build_append_pci_bus_devices(Aml *parent_scope, PCIBus *bus,
> >          QLIST_FOREACH(sec, &bus->child, sibling) {
> >              int32_t devfn = sec->parent_dev->devfn;
> >  
> > -            if (pci_bus_is_root(sec) || pci_bus_is_express(sec)) {
> > +            if (pci_bus_is_root(sec)) {
> >                  continue;
> >              }
> >  
> > @@ -1586,7 +1586,12 @@ static void build_piix4_pci_hotplug(Aml *table)
> >      aml_append(table, scope);
> >  }
> >  
> > -static Aml *build_q35_osc_method(void)
> > +static void build_q35_pci_hotplug(Aml *table)
> > +{
> > +    build_piix4_pci_hotplug(table);
> > +}
> 
> s/build_piix4_pci_hotplug/build_i386_acpi_pci_hotplug/
> 
> and reuse it in both cases, instead of adding wrapper?

I'm not sure about that - we have microvm too ...

> > +
> > +static Aml *build_q35_osc_method(AcpiPmInfo *pm)
> >  {
> >      Aml *if_ctx;
> >      Aml *if_ctx2;
> > @@ -1698,6 +1703,7 @@ build_dsdt(GArray *table_data, BIOSLinker *linker,
> >          build_hpet_aml(dsdt);
> >          build_q35_isa_bridge(dsdt);
> >          build_isa_devices_aml(dsdt);
> > +        build_q35_pci_hotplug(dsdt);
> >          build_q35_pci0_int(dsdt);
> >          if (pcms->smbus && !pcmc->do_not_add_smb_acpi) {
> >              build_smb0(dsdt, pcms->smbus, ICH9_SMB_DEV, ICH9_SMB_FUNC);
> > @@ -1724,7 +1730,7 @@ build_dsdt(GArray *table_data, BIOSLinker *linker,
> >      {
> >          aml_append(scope, aml_name_decl("_HID", aml_string("ACPI0006")));
> >  
> > -        if (misc->is_piix4) {
> > +        if (misc->is_piix4 || pm->pcihp_bridge_en) {
> >              method = aml_method("_E01", 0, AML_NOTSERIALIZED);
> >              aml_append(method,
> >                  aml_acquire(aml_name("\\_SB.PCI0.BLCK"), 0xFFFF));

Re: [RFC PATCH 3/5] hw/i386/acpi-build: Add ACPI PCI hot-plug methods to q35

[Igor Mammedov]

On Tue, 14 Jul 2020 05:22:20 -0400
"Michael S. Tsirkin" <mst@redhat.com> wrote:

> On Mon, Jul 13, 2020 at 04:39:54PM +0200, Igor Mammedov wrote:
> > On Thu,  9 Jul 2020 00:46:13 +0200
> > Julia Suvorova <jusual@redhat.com> wrote:
> >   
> > > Implement notifications and gpe to support q35 ACPI PCI hot-plug.
> > > The addresses specified in [1] remain the same to make fewer changes.
> > > 
> > > [1] docs/spec/acpi_pci_hotplug.txt  
> > 
> > CCing Gerd his opinion on reusing piix4 IO port range for q35
> > 
> >    
> > > Signed-off-by: Julia Suvorova <jusual@redhat.com>
> > > ---
> > >  hw/i386/acpi-build.c | 20 +++++++++++++-------
> > >  1 file changed, 13 insertions(+), 7 deletions(-)
> > > 
> > > diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
> > > index 11c598f955..5c5ad88ad6 100644
> > > --- a/hw/i386/acpi-build.c
> > > +++ b/hw/i386/acpi-build.c
> > > @@ -201,10 +201,6 @@ static void acpi_get_pm_info(MachineState *machine, AcpiPmInfo *pm)
> > >          /* w2k requires FADT(rev1) or it won't boot, keep PC compatible */
> > >          pm->fadt.rev = 1;
> > >          pm->cpu_hp_io_base = PIIX4_CPU_HOTPLUG_IO_BASE;
> > > -        pm->pcihp_io_base =
> > > -            object_property_get_uint(obj, ACPI_PCIHP_IO_BASE_PROP, NULL);
> > > -        pm->pcihp_io_len =
> > > -            object_property_get_uint(obj, ACPI_PCIHP_IO_LEN_PROP, NULL);
> > >      }
> > >      if (lpc) {
> > >          struct AcpiGenericAddress r = { .space_id = AML_AS_SYSTEM_IO,
> > > @@ -214,6 +210,10 @@ static void acpi_get_pm_info(MachineState *machine, AcpiPmInfo *pm)
> > >          pm->fadt.flags |= 1 << ACPI_FADT_F_RESET_REG_SUP;
> > >          pm->cpu_hp_io_base = ICH9_CPU_HOTPLUG_IO_BASE;
> > >      }
> > > +    pm->pcihp_io_base =
> > > +        object_property_get_uint(obj, ACPI_PCIHP_IO_BASE_PROP, NULL);
> > > +    pm->pcihp_io_len =
> > > +        object_property_get_uint(obj, ACPI_PCIHP_IO_LEN_PROP, NULL);
> > >  
> > >      /* The above need not be conditional on machine type because the reset port
> > >       * happens to be the same on PIIX (pc) and ICH9 (q35). */
> > > @@ -472,7 +472,7 @@ static void build_append_pci_bus_devices(Aml *parent_scope, PCIBus *bus,
> > >          QLIST_FOREACH(sec, &bus->child, sibling) {
> > >              int32_t devfn = sec->parent_dev->devfn;
> > >  
> > > -            if (pci_bus_is_root(sec) || pci_bus_is_express(sec)) {
> > > +            if (pci_bus_is_root(sec)) {
> > >                  continue;
> > >              }
> > >  
> > > @@ -1586,7 +1586,12 @@ static void build_piix4_pci_hotplug(Aml *table)
> > >      aml_append(table, scope);
> > >  }
> > >  
> > > -static Aml *build_q35_osc_method(void)
> > > +static void build_q35_pci_hotplug(Aml *table)
> > > +{
> > > +    build_piix4_pci_hotplug(table);
> > > +}  
> > 
> > s/build_piix4_pci_hotplug/build_i386_acpi_pci_hotplug/
> > 
> > and reuse it in both cases, instead of adding wrapper?  
> 
> I'm not sure about that - we have microvm too ...
it doesn't have pci if I'm not mistaken,
and when/if it gains one someday, it may or maynot use hotplug
it will be up to its specific DSDT to include this call.

What doesn't make sense is using board specific naming
here for the same code. I don't insist on the variant I've suggested,
only on consolidating it instead of adding dummy wrapper


> > > +
> > > +static Aml *build_q35_osc_method(AcpiPmInfo *pm)
> > >  {
> > >      Aml *if_ctx;
> > >      Aml *if_ctx2;
> > > @@ -1698,6 +1703,7 @@ build_dsdt(GArray *table_data, BIOSLinker *linker,
> > >          build_hpet_aml(dsdt);
> > >          build_q35_isa_bridge(dsdt);
> > >          build_isa_devices_aml(dsdt);
> > > +        build_q35_pci_hotplug(dsdt);
> > >          build_q35_pci0_int(dsdt);
> > >          if (pcms->smbus && !pcmc->do_not_add_smb_acpi) {
> > >              build_smb0(dsdt, pcms->smbus, ICH9_SMB_DEV, ICH9_SMB_FUNC);
> > > @@ -1724,7 +1730,7 @@ build_dsdt(GArray *table_data, BIOSLinker *linker,
> > >      {
> > >          aml_append(scope, aml_name_decl("_HID", aml_string("ACPI0006")));
> > >  
> > > -        if (misc->is_piix4) {
> > > +        if (misc->is_piix4 || pm->pcihp_bridge_en) {
> > >              method = aml_method("_E01", 0, AML_NOTSERIALIZED);
> > >              aml_append(method,
> > >                  aml_acquire(aml_name("\\_SB.PCI0.BLCK"), 0xFFFF));  
> 

Re: [RFC PATCH 3/5] hw/i386/acpi-build: Add ACPI PCI hot-plug methods to q35

[Gerd Hoffmann]

On Mon, Jul 13, 2020 at 04:39:54PM +0200, Igor Mammedov wrote:
> On Thu,  9 Jul 2020 00:46:13 +0200
> Julia Suvorova <jusual@redhat.com> wrote:
> 
> > Implement notifications and gpe to support q35 ACPI PCI hot-plug.
> > The addresses specified in [1] remain the same to make fewer changes.
> > 
> > [1] docs/spec/acpi_pci_hotplug.txt
> 
> CCing Gerd his opinion on reusing piix4 IO port range for q35

Oh no, please don't.  Grabbing random io ports is asking for trouble,
especially on q35 where you only need enough pcie devices to have the
port range 0xae00 -> 0xae0f actually overlap with a pci bridge window.


Ideally we'll find some unused spot in the hidden pci bar of the ich9
pm device.  At least the qemu implementation has some room:

    0000000000000600-000000000000067f (prio 0, i/o): ich9-pm
      0000000000000600-0000000000000603 (prio 0, i/o): acpi-evt
      0000000000000604-0000000000000605 (prio 0, i/o): acpi-cnt
      0000000000000608-000000000000060b (prio 0, i/o): acpi-tmr
      0000000000000620-000000000000062f (prio 0, i/o): acpi-gpe0
      0000000000000630-0000000000000637 (prio 0, i/o): acpi-smi
      0000000000000660-000000000000067f (prio 0, i/o): sm-tco

Offset 0x40 seems to be unused for example (but better check the chipset
specs to see if that is really unused or whenever the qemu emulation is
incomplete).


If that doesn't work out pick an io range which is unlikely to conflict
with something.  That excludes anything above > 0x1000 (pci bridge
windows) and anything below 0x3ff (legacy isa).  From the remaining
range 0x400 -> 0xfff the 0xc00 -> 0xcff block is the best candidate
I think.  Because of the pci config registers being there it is unlikely
that the firmware places something in that range.

Oh, I see the cpu hotplug registers are already there:

    [ ... ]
    0000000000000700-000000000000073f (prio 1, i/o): pm-smbus
    0000000000000cd8-0000000000000ce3 (prio 0, i/o): acpi-cpu-hotplug
    0000000000000cf8-0000000000000cfb (prio 0, i/o): pci-conf-idx
    0000000000000cf9-0000000000000cf9 (prio 1, i/o): lpc-reset-control
    0000000000000cfc-0000000000000cff (prio 0, i/o): pci-conf-data
    [ ... ]

So placing the pci hotplug registers next to them (say at 0xcc8) looks
like a good pick to me.


While being at it:  Shouldn't we reserve these port ranges somehow?
Using an acpi device for example, simliar to fw_cfg?  The guest OS
should better know there is something at those ports ...

take care,
  Gerd

Re: [RFC PATCH 3/5] hw/i386/acpi-build: Add ACPI PCI hot-plug methods to q35

[Igor Mammedov]

On Wed, 15 Jul 2020 08:57:51 +0200
Gerd Hoffmann <kraxel@redhat.com> wrote:

> On Mon, Jul 13, 2020 at 04:39:54PM +0200, Igor Mammedov wrote:
> > On Thu,  9 Jul 2020 00:46:13 +0200
> > Julia Suvorova <jusual@redhat.com> wrote:
> >   
> > > Implement notifications and gpe to support q35 ACPI PCI hot-plug.
> > > The addresses specified in [1] remain the same to make fewer changes.
> > > 
> > > [1] docs/spec/acpi_pci_hotplug.txt  
> > 
> > CCing Gerd his opinion on reusing piix4 IO port range for q35  

[...]

> While being at it:  Shouldn't we reserve these port ranges somehow?
> Using an acpi device for example, simliar to fw_cfg?  The guest OS
> should better know there is something at those ports ...

we do it at ACPI level in DSDT, look for comment
/* reserve PCIHP resources */

It should make Windows trip over in case of another range overlap with
reserved ports. (linux kernel is more tolerant and may silently ignore
or print a warning) 

> take care,
>   Gerd
> 

Re: [RFC PATCH 4/5] hw/i386/acpi-build: Turn off support of PCIe native hot-plug and SHPC in _OSC

[Igor Mammedov]

On Tue, 14 Jul 2020 04:39:53 -0400
"Michael S. Tsirkin" <mst@redhat.com> wrote:

> On Mon, Jul 13, 2020 at 04:56:54PM +0200, Igor Mammedov wrote:
> > On Thu,  9 Jul 2020 00:46:14 +0200
> > Julia Suvorova <jusual@redhat.com> wrote:
> >   
> > > Other methods may be used if the system is capable of this and the _OSC bit
> > > is set. Disable them explicitly to force ACPI PCI hot-plug use. The older
> > > versions will still use PCIe native.  
> 
> Do we need that later part btw?
> 
> > > 
> > > Signed-off-by: Julia Suvorova <jusual@redhat.com>
> > > ---
> > >  hw/i386/acpi-build.c | 12 ++++++++----
> > >  1 file changed, 8 insertions(+), 4 deletions(-)
> > > 
> > > diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
> > > index 5c5ad88ad6..0e2891c3ea 100644
> > > --- a/hw/i386/acpi-build.c
> > > +++ b/hw/i386/acpi-build.c
> > > @@ -1599,6 +1599,7 @@ static Aml *build_q35_osc_method(AcpiPmInfo *pm)
> > >      Aml *method;
> > >      Aml *a_cwd1 = aml_name("CDW1");
> > >      Aml *a_ctrl = aml_local(0);
> > > +    unsigned osc_ctrl;
> > >  
> > >      method = aml_method("_OSC", 4, AML_NOTSERIALIZED);
> > >      aml_append(method, aml_create_dword_field(aml_arg(3), aml_int(0), "CDW1"));
> > > @@ -1612,9 +1613,12 @@ static Aml *build_q35_osc_method(AcpiPmInfo *pm)
> > >  
> > >      /*
> > >       * Always allow native PME, AER (no dependencies)
> > > -     * Allow SHPC (PCI bridges can have SHPC controller)
> > > +     * Need to disable native and SHPC hot-plug to use acpihp
> > > +     *
> > > +     * PCI Firmware Specification, Revision 3.2  
> 
> I don't think you have to add a reference as part of this patchset.
> The spec in question documents _OSC so it's not a bad idea to
> add it, but it's a bit more work, e.g. we generally try to list
> the earliest spec that documents the given feature, since
> So I suspect this is 3.0 actually.
> 
> 
> > seems incomplete, were you going to point to a concrete chapter that requires this change?  
> 
> 
> It doesn't as such. A better description would be:
> 
> / * Guests seem to generally prefer native hotplug control. As we want them to
>   * use ACPI, don't enable it.
>   */
> 
> 
> 
> > >       */
> > > -    aml_append(if_ctx, aml_and(a_ctrl, aml_int(0x1F), a_ctrl));
> > > +    osc_ctrl = pm->pcihp_bridge_en ? 0x1C : 0x1F;  
> > Since you are touching this, how about converting this magic number to
> > something more readable?
> > i.e.
> >             set_bit(ACPI_OSC_SHPC_EN,  osc_ctrl)
> >           or
> >             osc_ctrl |= BIT(SOME_FEATURE)
> >   
> 
> ... if there is such a macro. If not I suspect it's better as a comment:
> 
> 	0x10 /* PCI Express Capability Structure control */ |
> 	0x80 /* PCI Express Advanced Error Reporting control */ |
> 	0x40 /* PCI Express Native Power Management Events control */

if that is a spec quoted nearby than, I like comments idea as it
follows the same style which we use with ACPI numbers.
We just need split single magic number onto separate features bits
so it would be readable.

> 
> 
> 
> > > +    aml_append(if_ctx, aml_and(a_ctrl, aml_int(osc_ctrl), a_ctrl));
> > >  
> > >      if_ctx2 = aml_if(aml_lnot(aml_equal(aml_arg(1), aml_int(1))));
> > >      /* Unknown revision */
> > > @@ -1696,7 +1700,7 @@ build_dsdt(GArray *table_data, BIOSLinker *linker,
> > >          aml_append(dev, aml_name_decl("_CID", aml_eisaid("PNP0A03")));
> > >          aml_append(dev, aml_name_decl("_ADR", aml_int(0)));
> > >          aml_append(dev, aml_name_decl("_UID", aml_int(1)));
> > > -        aml_append(dev, build_q35_osc_method());
> > > +        aml_append(dev, build_q35_osc_method(pm));
> > >          aml_append(sb_scope, dev);
> > >          aml_append(dsdt, sb_scope);
> > >  
> > > @@ -1771,7 +1775,7 @@ build_dsdt(GArray *table_data, BIOSLinker *linker,
> > >              if (pci_bus_is_express(bus)) {
> > >                  aml_append(dev, aml_name_decl("_HID", aml_eisaid("PNP0A08")));
> > >                  aml_append(dev, aml_name_decl("_CID", aml_eisaid("PNP0A03")));
> > > -                aml_append(dev, build_q35_osc_method());
> > > +                aml_append(dev, build_q35_osc_method(pm));
> > >              } else {
> > >                  aml_append(dev, aml_name_decl("_HID", aml_eisaid("PNP0A03")));
> > >              }  
> 
> 

Re: [RFC PATCH 5/5] hw/acpi/ich9: Enable ACPI PCI hot-plug

[Igor Mammedov]

On Tue, 14 Jul 2020 04:54:02 -0400
"Michael S. Tsirkin" <mst@redhat.com> wrote:

> On Mon, Jul 13, 2020 at 05:17:18PM +0200, Igor Mammedov wrote:
> > On Thu,  9 Jul 2020 00:46:15 +0200
> > Julia Suvorova <jusual@redhat.com> wrote:
> >   
> > > Add acpi_pcihp to ich9_pm and use ACPI PCI hot-plug by default.
> > > 
> > > Signed-off-by: Julia Suvorova <jusual@redhat.com>
> > > ---
> > >  include/hw/acpi/ich9.h |  3 +++
> > >  hw/acpi/ich9.c         | 45 ++++++++++++++++++++++++++++++++++++++++++
> > >  hw/acpi/pcihp.c        |  3 ++-
> > >  hw/i386/pc.c           |  4 +++-
> > >  4 files changed, 53 insertions(+), 2 deletions(-)
> > > 
> > > diff --git a/include/hw/acpi/ich9.h b/include/hw/acpi/ich9.h
> > > index 28a53181cb..d345da6b74 100644
> > > --- a/include/hw/acpi/ich9.h
> > > +++ b/include/hw/acpi/ich9.h
> > > @@ -24,6 +24,7 @@
> > >  #include "hw/acpi/acpi.h"
> > >  #include "hw/acpi/cpu_hotplug.h"
> > >  #include "hw/acpi/cpu.h"
> > > +#include "hw/acpi/pcihp.h"
> > >  #include "hw/acpi/memory_hotplug.h"
> > >  #include "hw/acpi/acpi_dev_interface.h"
> > >  #include "hw/acpi/tco.h"
> > > @@ -53,6 +54,8 @@ typedef struct ICH9LPCPMRegs {
> > >      AcpiCpuHotplug gpe_cpu;
> > >      CPUHotplugState cpuhp_state;
> > >  
> > > +    bool use_acpi_pci_hotplug;  
> > a bit outdated naming,
> > see 0affda043675c7619248a924a89bfd3781759f63, and rename it to match piix4
> >   
> > > +    AcpiPciHpState acpi_pci_hotplug;
> > >      MemHotplugState acpi_memory_hotplug;
> > >  
> > >      uint8_t disable_s3;
> > > diff --git a/hw/acpi/ich9.c b/hw/acpi/ich9.c
> > > index 0fdd736da4..e0291373f2 100644
> > > --- a/hw/acpi/ich9.c
> > > +++ b/hw/acpi/ich9.c
> > > @@ -265,6 +265,7 @@ static void pm_reset(void *opaque)
> > >      }
> > >      pm->smi_en_wmask = ~0;
> > >  
> > > +    acpi_pcihp_reset(&pm->acpi_pci_hotplug);
> > >      acpi_update_sci(&pm->acpi_regs, pm->irq);
> > >  }
> > >  
> > > @@ -303,6 +304,17 @@ void ich9_pm_init(PCIDevice *lpc_pci, ICH9LPCPMRegs *pm,
> > >      pm->enable_tco = true;
> > >      acpi_pm_tco_init(&pm->tco_regs, &pm->io);
> > >  
> > > +    if (pm->use_acpi_pci_hotplug) {
> > > +        acpi_pcihp_init(OBJECT(lpc_pci),
> > > +                        &pm->acpi_pci_hotplug,
> > > +                        pci_get_bus(lpc_pci),
> > > +                        pci_address_space_io(lpc_pci),
> > > +                        true);
> > > +        qbus_set_hotplug_handler(BUS(pci_get_bus(lpc_pci)),
> > > +                                 OBJECT(lpc_pci),
> > > +                                 &error_abort);
> > > +    }
> > > +
> > >      pm->irq = sci_irq;
> > >      qemu_register_reset(pm_reset, pm);
> > >      pm->powerdown_notifier.notify = pm_powerdown_req;
> > > @@ -374,6 +386,20 @@ static void ich9_pm_set_enable_tco(Object *obj, bool value, Error **errp)
> > >      s->pm.enable_tco = value;
> > >  }
> > >  
> > > +static bool ich9_pm_get_acpi_pci_hotplug(Object *obj, Error **errp)
> > > +{
> > > +    ICH9LPCState *s = ICH9_LPC_DEVICE(obj);
> > > +
> > > +    return s->pm.use_acpi_pci_hotplug;
> > > +}
> > > +
> > > +static void ich9_pm_set_acpi_pci_hotplug(Object *obj, bool value,
> > > +                                               Error **errp)
> > > +{
> > > +    ICH9LPCState *s = ICH9_LPC_DEVICE(obj);
> > > +
> > > +    s->pm.use_acpi_pci_hotplug = value;
> > > +}
> > >  void ich9_pm_add_properties(Object *obj, ICH9LPCPMRegs *pm)
> > >  {
> > >      static const uint32_t gpe0_len = ICH9_PMIO_GPE0_LEN;
> > > @@ -382,6 +408,7 @@ void ich9_pm_add_properties(Object *obj, ICH9LPCPMRegs *pm)
> > >      pm->disable_s3 = 0;
> > >      pm->disable_s4 = 0;
> > >      pm->s4_val = 2;
> > > +    pm->use_acpi_pci_hotplug = true;  
> > 
> > I'm not sure about making crutch on by default though.
> > 
> > Does linux guests work fine with native hotplug or
> > does it have the same issues as Windows?
> > 
> > Also you had an idea how to workaround native hotplug issues
> > on QEMU side.
> > Can you dump here a quick summary why it didn't work out
> > in the end?  
> 
> To help out with that a bit:
> 
> I think native hotplug just does not match what management needs from
> QEMU. QEMU wants what ACPI calls a "VCR-style" ejection where
> eject happens automatically after software allows it.
> 
> Native hotplug is designed with a human in the loop.
> 
> Things like multi-second delay after an eject request,
> inability to differentiate between power down and eject,
> ability to power device back up after power down
> are all architectural in hative hotplug.
> 
> Interface is also designed such that there are subtle races making it
> hard not to loose hotplug events are also harmless on bare-metal since a
> human can just press the eject button again, but with causes no end of
> issues for us.

that is a generic excuse for adding ACPI hotplug to q35,
but what I've asked for, is to document (for history reasons) alternative
ideas that were explored to make native unplug work despite spec issues.
That would be also a good justification for ACPI approach showing that
we've exhausted any other approach before resorting to ACPI hotplug

Jilia had an idea about 'caching' unplug req and depending on cached value
behave differently on power off event. Which looked to me as a thing that
might solve unplug issue.

> I agree it's a good idea to include the motivation here or in the commit
> log, though.
> 
> >   
> > >  
> > >      object_property_add_uint32_ptr(obj, ACPI_PM_PROP_PM_IO_BASE,
> > >                                     &pm->pm_io_base, OBJ_PROP_FLAG_READ);
> > > @@ -405,6 +432,9 @@ void ich9_pm_add_properties(Object *obj, ICH9LPCPMRegs *pm)
> > >      object_property_add_bool(obj, ACPI_PM_PROP_TCO_ENABLED,
> > >                               ich9_pm_get_enable_tco,
> > >                               ich9_pm_set_enable_tco);
> > > +    object_property_add_bool(obj, "acpi-pci-hotplug-with-bridge-support",
> > > +                             ich9_pm_get_acpi_pci_hotplug,
> > > +                             ich9_pm_set_acpi_pci_hotplug);
> > >  }
> > >  
> > >  void ich9_pm_device_pre_plug_cb(HotplugHandler *hotplug_dev, DeviceState *dev,
> > > @@ -412,6 +442,11 @@ 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);
> > > +        return;
> > > +    }
> > > +
> > >      if (object_dynamic_cast(OBJECT(dev), TYPE_PC_DIMM) &&
> > >          !lpc->pm.acpi_memory_hotplug.is_enabled)
> > >          error_setg(errp,
> > > @@ -437,6 +472,9 @@ void ich9_pm_device_plug_cb(HotplugHandler *hotplug_dev, DeviceState *dev,
> > >          } else {
> > >              acpi_cpu_plug_cb(hotplug_dev, &lpc->pm.cpuhp_state, dev, errp);
> > >          }
> > > +    } else if (object_dynamic_cast(OBJECT(dev), TYPE_PCI_DEVICE)) {
> > > +        acpi_pcihp_device_plug_cb(hotplug_dev, &lpc->pm.acpi_pci_hotplug,
> > > +                                  dev, errp);
> > >      } else {
> > >          error_setg(errp, "acpi: device plug request for not supported device"
> > >                     " type: %s", object_get_typename(OBJECT(dev)));
> > > @@ -457,6 +495,10 @@ void ich9_pm_device_unplug_request_cb(HotplugHandler *hotplug_dev,
> > >                 !lpc->pm.cpu_hotplug_legacy) {
> > >          acpi_cpu_unplug_request_cb(hotplug_dev, &lpc->pm.cpuhp_state,
> > >                                     dev, errp);
> > > +    } else if (object_dynamic_cast(OBJECT(dev), TYPE_PCI_DEVICE)) {
> > > +        acpi_pcihp_device_unplug_request_cb(hotplug_dev,
> > > +                                            &lpc->pm.acpi_pci_hotplug,
> > > +                                            dev, errp);
> > >      } else {
> > >          error_setg(errp, "acpi: device unplug request for not supported device"
> > >                     " type: %s", object_get_typename(OBJECT(dev)));
> > > @@ -474,6 +516,9 @@ void ich9_pm_device_unplug_cb(HotplugHandler *hotplug_dev, DeviceState *dev,
> > >      } else if (object_dynamic_cast(OBJECT(dev), TYPE_CPU) &&
> > >                 !lpc->pm.cpu_hotplug_legacy) {
> > >          acpi_cpu_unplug_cb(&lpc->pm.cpuhp_state, dev, errp);
> > > +    } else if (object_dynamic_cast(OBJECT(dev), TYPE_PCI_DEVICE)) {
> > > +        acpi_pcihp_device_unplug_cb(hotplug_dev, &lpc->pm.acpi_pci_hotplug,
> > > +                                    dev, errp);
> > >      } else {
> > >          error_setg(errp, "acpi: device unplug for not supported device"
> > >                     " type: %s", object_get_typename(OBJECT(dev)));
> > > diff --git a/hw/acpi/pcihp.c b/hw/acpi/pcihp.c
> > > index 3d4ee3af72..d905d1b8f2 100644
> > > --- a/hw/acpi/pcihp.c
> > > +++ b/hw/acpi/pcihp.c
> > > @@ -110,7 +110,8 @@ static void acpi_set_pci_info(void)
> > >      }
> > >      bsel_is_set = true;
> > >  
> > > -    bus = find_i440fx(); /* TODO: Q35 support */
> > > +    bus = find_host();
> > > +
> > >      if (bus) {
> > >          /* Scan all PCI buses. Set property to enable acpi based hotplug. */
> > >          pci_for_each_bus_depth_first(bus, acpi_set_bsel, NULL, &bsel_alloc);
> > > diff --git a/hw/i386/pc.c b/hw/i386/pc.c
> > > index 143ac1c354..21c6eb779e 100644
> > > --- a/hw/i386/pc.c
> > > +++ b/hw/i386/pc.c
> > > @@ -96,7 +96,9 @@
> > >  #include "fw_cfg.h"
> > >  #include "trace.h"
> > >  
> > > -GlobalProperty pc_compat_5_0[] = {};
> > > +GlobalProperty pc_compat_5_0[] = {
> > > +    { "ICH9-LPC", "acpi-pci-hotplug-with-bridge-support", "off" },
> > > +};
> > >  const size_t pc_compat_5_0_len = G_N_ELEMENTS(pc_compat_5_0);
> > >  
> > >  GlobalProperty pc_compat_4_2[] = {  
> 

Re: [RFC PATCH 3/5] hw/i386/acpi-build: Add ACPI PCI hot-plug methods to q35

[Gerd Hoffmann]

> > While being at it:  Shouldn't we reserve these port ranges somehow?
> > Using an acpi device for example, simliar to fw_cfg?  The guest OS
> > should better know there is something at those ports ...
> 
> we do it at ACPI level in DSDT, look for comment
> /* reserve PCIHP resources */

Ah, good.

> It should make Windows trip over in case of another range overlap with
> reserved ports. (linux kernel is more tolerant and may silently ignore
> or print a warning) 

Hmm, linux doesn't list the device, can't see it neither in the kernel
log nor in /proc/ioports ...

take care,
  Gerd