[Qemu-devel] [PATCH RFC 0/6] q35: add acpi pci hotplug support

[Qemu-devel] [PATCH RFC 0/6] q35: add acpi pci hotplug support

[Aleksandr Bezzubikov]

The series adds hotplug support to legacy PCI buses for Q35 machines.
The ACPI hotplug code is emitted if at least one legacy pci-bridge is present.

This series is mostly based on past Marcel's series 
https://lists.gnu.org/archive/html/qemu-devel/2016-05/msg05681.html,
but rebased on current master with some minor changes according to current codebase.

ACPI code emission approach used in this series can be called "static", 
because it checkswhether a bridge exists only at initial DSDT generation moment. 
The main goal is to enable AML PCI hotplug-related code to be generated dynamically. 
In other words, the bridge plugged in - a new acpi definition block is
loaded (using LoadTable method). 
This is necessary for PCIE-PCI bridge hotplugging feature. 

Aleksandr Bezzubikov (6):
  hw/acpi: remove dead acpi code
  hw/acpi: simplify dsdt building code
  hw/acpi: fix pcihp io initialization
  hw/acpi: prepare pci hotplug IO for ich9
  hw/acpi: extend acpi pci hotplug support for pci express
  hw/ich9: enable acpi pci hotplug

 hw/acpi/ich9.c         |  31 +++++++++++++
 hw/i386/acpi-build.c   | 116 ++++++++++++++++++++++++-------------------------
 hw/isa/lpc_ich9.c      |  12 +++++
 include/hw/acpi/ich9.h |   4 ++
 include/hw/i386/pc.h   |   7 ++-
 5 files changed, 111 insertions(+), 59 deletions(-)

-- 
2.7.4

[Qemu-devel] [PATCH RFC 1/6] hw/acpi: remove dead acpi code

[Aleksandr Bezzubikov]

Signed-off-by: Aleksandr Bezzubikov <zuban32s@gmail.com>
---
 hw/i386/acpi-build.c | 10 ----------
 1 file changed, 10 deletions(-)

diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
index afcadac..6fce967 100644
--- a/hw/i386/acpi-build.c
+++ b/hw/i386/acpi-build.c
@@ -1912,16 +1912,6 @@ build_dsdt(GArray *table_data, BIOSLinker *linker,
         build_piix4_pci0_int(dsdt);
     } else {
         sb_scope = aml_scope("_SB");
-        aml_append(sb_scope,
-            aml_operation_region("PCST", AML_SYSTEM_IO, aml_int(0xae00), 0x0c));
-        aml_append(sb_scope,
-            aml_operation_region("PCSB", AML_SYSTEM_IO, aml_int(0xae0c), 0x01));
-        field = aml_field("PCSB", AML_ANY_ACC, AML_NOLOCK, AML_WRITE_AS_ZEROS);
-        aml_append(field, aml_named_field("PCIB", 8));
-        aml_append(sb_scope, field);
-        aml_append(dsdt, sb_scope);
-
-        sb_scope = aml_scope("_SB");
         dev = aml_device("PCI0");
         aml_append(dev, aml_name_decl("_HID", aml_eisaid("PNP0A08")));
         aml_append(dev, aml_name_decl("_CID", aml_eisaid("PNP0A03")));
-- 
2.7.4

[Qemu-devel] [PATCH RFC 2/6] hw/acpi: simplify dsdt building code

[Aleksandr Bezzubikov]

Signed-off-by: Aleksandr Bezzubikov <zuban32s@gmail.com>
---
 hw/i386/acpi-build.c | 59 +++++++++++++++++++++++-----------------------------
 1 file changed, 26 insertions(+), 33 deletions(-)

diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
index 6fce967..b0dcd34 100644
--- a/hw/i386/acpi-build.c
+++ b/hw/i386/acpi-build.c
@@ -1886,7 +1886,7 @@ build_dsdt(GArray *table_data, BIOSLinker *linker,
     PCMachineClass *pcmc = PC_MACHINE_GET_CLASS(machine);
     uint32_t nr_mem = machine->ram_slots;
     int root_bus_limit = 0xFF;
-    PCIBus *bus = NULL;
+    PCIBus *root_bus = NULL;
     int i;
 
     dsdt = init_aml_allocator();
@@ -1927,6 +1927,9 @@ build_dsdt(GArray *table_data, BIOSLinker *linker,
         build_q35_pci0_int(dsdt);
     }
 
+    root_bus = PC_MACHINE(machine)->bus;
+    assert(root_bus);
+
     if (pcmc->legacy_cpu_hotplug) {
         build_legacy_cpu_hotplug_aml(dsdt, machine, pm->cpu_hp_io_base);
     } else {
@@ -1961,8 +1964,8 @@ build_dsdt(GArray *table_data, BIOSLinker *linker,
     aml_append(dsdt, scope);
 
     crs_range_set_init(&crs_range_set);
-    bus = PC_MACHINE(machine)->bus;
-    if (bus) {
+    {
+        PCIBus *bus = root_bus;
         QLIST_FOREACH(bus, &bus->child, sibling) {
             uint8_t bus_num = pci_bus_num(bus);
             uint8_t numa_node = pci_bus_numa_node(bus);
@@ -2206,38 +2209,28 @@ build_dsdt(GArray *table_data, BIOSLinker *linker,
 
     sb_scope = aml_scope("\\_SB");
     {
-        Object *pci_host;
-        PCIBus *bus = NULL;
-
-        pci_host = acpi_get_i386_pci_host();
-        if (pci_host) {
-            bus = PCI_HOST_BRIDGE(pci_host)->bus;
+        Aml *scope = aml_scope("PCI0");
+        /* Scan all PCI buses. Generate tables to support hotplug. */
+        build_append_pci_bus_devices(scope, root_bus, pm->pcihp_bridge_en);
+
+        if (misc->tpm_version != TPM_VERSION_UNSPEC) {
+            dev = aml_device("ISA.TPM");
+            aml_append(dev, aml_name_decl("_HID", aml_eisaid("PNP0C31")));
+            aml_append(dev, aml_name_decl("_STA", aml_int(0xF)));
+            crs = aml_resource_template();
+            aml_append(crs, aml_memory32_fixed(TPM_TIS_ADDR_BASE,
+                       TPM_TIS_ADDR_SIZE, AML_READ_WRITE));
+            /*
+                FIXME: TPM_TIS_IRQ=5 conflicts with PNP0C0F irqs,
+                Rewrite to take IRQ from TPM device model and
+                fix default IRQ value there to use some unused IRQ
+             */
+            /* aml_append(crs, aml_irq_no_flags(TPM_TIS_IRQ)); */
+            aml_append(dev, aml_name_decl("_CRS", crs));
+            aml_append(scope, dev);
         }
 
-        if (bus) {
-            Aml *scope = aml_scope("PCI0");
-            /* Scan all PCI buses. Generate tables to support hotplug. */
-            build_append_pci_bus_devices(scope, bus, pm->pcihp_bridge_en);
-
-            if (misc->tpm_version != TPM_VERSION_UNSPEC) {
-                dev = aml_device("ISA.TPM");
-                aml_append(dev, aml_name_decl("_HID", aml_eisaid("PNP0C31")));
-                aml_append(dev, aml_name_decl("_STA", aml_int(0xF)));
-                crs = aml_resource_template();
-                aml_append(crs, aml_memory32_fixed(TPM_TIS_ADDR_BASE,
-                           TPM_TIS_ADDR_SIZE, AML_READ_WRITE));
-                /*
-                    FIXME: TPM_TIS_IRQ=5 conflicts with PNP0C0F irqs,
-                    Rewrite to take IRQ from TPM device model and
-                    fix default IRQ value there to use some unused IRQ
-                 */
-                /* aml_append(crs, aml_irq_no_flags(TPM_TIS_IRQ)); */
-                aml_append(dev, aml_name_decl("_CRS", crs));
-                aml_append(scope, dev);
-            }
-
-            aml_append(sb_scope, scope);
-        }
+        aml_append(sb_scope, scope);
     }
     aml_append(dsdt, sb_scope);
 
-- 
2.7.4

[Qemu-devel] [PATCH RFC 3/6] hw/acpi: fix pcihp io initialization

[Aleksandr Bezzubikov]

Signed-off-by: Aleksandr Bezzubikov <zuban32s@gmail.com>
---
 hw/i386/acpi-build.c | 9 +++++----
 1 file changed, 5 insertions(+), 4 deletions(-)

diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
index b0dcd34..c99dbcc 100644
--- a/hw/i386/acpi-build.c
+++ b/hw/i386/acpi-build.c
@@ -129,17 +129,18 @@ static void acpi_get_pm_info(AcpiPmInfo *pm)
     Object *lpc = ich9_lpc_find();
     Object *obj = NULL;
     QObject *o;
+    int pcihp_io_len, pcihp_io_base;
 
     pm->cpu_hp_io_base = 0;
-    pm->pcihp_io_base = 0;
-    pm->pcihp_io_len = 0;
     if (piix) {
         obj = piix;
         pm->cpu_hp_io_base = PIIX4_CPU_HOTPLUG_IO_BASE;
-        pm->pcihp_io_base =
+        pcihp_io_base =
             object_property_get_int(obj, ACPI_PCIHP_IO_BASE_PROP, NULL);
-        pm->pcihp_io_len =
+        pcihp_io_len =
             object_property_get_int(obj, ACPI_PCIHP_IO_LEN_PROP, NULL);
+        pm->pcihp_io_base = (pcihp_io_base == -1) ? 0 : pcihp_io_base;
+        pm->pcihp_io_len = (pcihp_io_len == -1) ? 0 : pcihp_io_len;
     }
     if (lpc) {
         obj = lpc;
-- 
2.7.4

[Qemu-devel] [PATCH RFC 4/6] hw/acpi: prepare pci hotplug IO for ich9

[Aleksandr Bezzubikov]

Signed-off-by: Aleksandr Bezzubikov <zuban32s@gmail.com>
---
 hw/i386/acpi-build.c | 13 +++++++------
 1 file changed, 7 insertions(+), 6 deletions(-)

diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
index c99dbcc..e434efe 100644
--- a/hw/i386/acpi-build.c
+++ b/hw/i386/acpi-build.c
@@ -135,12 +135,6 @@ static void acpi_get_pm_info(AcpiPmInfo *pm)
     if (piix) {
         obj = piix;
         pm->cpu_hp_io_base = PIIX4_CPU_HOTPLUG_IO_BASE;
-        pcihp_io_base =
-            object_property_get_int(obj, ACPI_PCIHP_IO_BASE_PROP, NULL);
-        pcihp_io_len =
-            object_property_get_int(obj, ACPI_PCIHP_IO_LEN_PROP, NULL);
-        pm->pcihp_io_base = (pcihp_io_base == -1) ? 0 : pcihp_io_base;
-        pm->pcihp_io_len = (pcihp_io_len == -1) ? 0 : pcihp_io_len;
     }
     if (lpc) {
         obj = lpc;
@@ -148,6 +142,13 @@ static void acpi_get_pm_info(AcpiPmInfo *pm)
     }
     assert(obj);
 
+    pcihp_io_base =
+        object_property_get_int(obj, ACPI_PCIHP_IO_BASE_PROP, NULL);
+    pcihp_io_len =
+        object_property_get_int(obj, ACPI_PCIHP_IO_LEN_PROP, NULL);
+    pm->pcihp_io_base = (pcihp_io_base == -1) ? 0 : pcihp_io_base;
+    pm->pcihp_io_len = (pcihp_io_len == -1) ? 0 : pcihp_io_len;
+
     /* Fill in optional s3/s4 related properties */
     o = object_property_get_qobject(obj, ACPI_PM_PROP_S3_DISABLED, NULL);
     if (o) {
-- 
2.7.4

[Qemu-devel] [PATCH RFC 5/6] hw/acpi: extend acpi pci hotplug support for pci express

[Aleksandr Bezzubikov]

Signed-off-by: Aleksandr Bezzubikov <zuban32s@gmail.com>
---
 hw/i386/acpi-build.c | 47 +++++++++++++++++++++++++++++++----------------
 1 file changed, 31 insertions(+), 16 deletions(-)

diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
index e434efe..8bbece5 100644
--- a/hw/i386/acpi-build.c
+++ b/hw/i386/acpi-build.c
@@ -254,6 +254,11 @@ static void acpi_get_pci_holes(Range *hole, Range *hole64)
                                               NULL));
 }
 
+static void acpi_test_pci_bus(PCIBus *bus, void *opaque)
+{
+    *(bool *)opaque |= !pci_bus_is_express(bus);
+}
+
 #define ACPI_PORT_SMI_CMD           0x00b2 /* TODO: this is APM_CNT_IOPORT */
 
 static void acpi_align_size(GArray *blob, unsigned align)
@@ -489,7 +494,7 @@ static void *acpi_set_bsel(PCIBus *bus, void *opaque)
     unsigned *bsel_alloc = opaque;
     unsigned *bus_bsel;
 
-    if (qbus_is_hotpluggable(BUS(bus))) {
+    if (qbus_is_hotpluggable(BUS(bus)) && !pci_bus_is_express(bus)) {
         bus_bsel = g_malloc(sizeof *bus_bsel);
 
         *bus_bsel = (*bsel_alloc)++;
@@ -500,15 +505,12 @@ static void *acpi_set_bsel(PCIBus *bus, void *opaque)
     return bsel_alloc;
 }
 
-static void acpi_set_pci_info(void)
+static void acpi_set_pci_info(PCIBus *bus)
 {
-    PCIBus *bus = find_i440fx(); /* TODO: Q35 support */
     unsigned bsel_alloc = ACPI_PCIHP_BSEL_DEFAULT;
 
-    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);
-    }
+    /* Scan all PCI buses. Set property to enable acpi based hotplug. */
+    pci_for_each_bus_depth_first(bus, acpi_set_bsel, NULL, &bsel_alloc);
 }
 
 static void build_append_pcihp_notify_entry(Aml *method, int slot)
@@ -570,7 +572,7 @@ static void build_append_pci_bus_devices(Aml *parent_scope, PCIBus *bus,
          * In this case they aren't themselves hot-pluggable.
          * Hotplugged bridges *are* hot-pluggable.
          */
-        bridge_in_acpi = pc->is_bridge && pcihp_bridge_en &&
+        bridge_in_acpi = pc->is_bridge && !pc->is_express && pcihp_bridge_en &&
             !DEVICE(pdev)->hotplugged;
 
         hotplug_enabled_dev = bsel && dc->hotpluggable && !bridge_in_acpi;
@@ -1787,7 +1789,7 @@ static void build_piix4_isa_bridge(Aml *table)
     aml_append(table, scope);
 }
 
-static void build_piix4_pci_hotplug(Aml *table)
+static void build_acpi_pci_hotplug(Aml *table)
 {
     Aml *scope;
     Aml *field;
@@ -1889,6 +1891,7 @@ build_dsdt(GArray *table_data, BIOSLinker *linker,
     uint32_t nr_mem = machine->ram_slots;
     int root_bus_limit = 0xFF;
     PCIBus *root_bus = NULL;
+    bool has_pci_bus = false;
     int i;
 
     dsdt = init_aml_allocator();
@@ -1910,7 +1913,6 @@ build_dsdt(GArray *table_data, BIOSLinker *linker,
         build_piix4_pm(dsdt);
         build_piix4_isa_bridge(dsdt);
         build_isa_devices_aml(dsdt);
-        build_piix4_pci_hotplug(dsdt);
         build_piix4_pci0_int(dsdt);
     } else {
         sb_scope = aml_scope("_SB");
@@ -1932,6 +1934,11 @@ build_dsdt(GArray *table_data, BIOSLinker *linker,
     root_bus = PC_MACHINE(machine)->bus;
     assert(root_bus);
 
+    pci_for_each_bus(root_bus, acpi_test_pci_bus, &has_pci_bus);
+    if (pm->pcihp_bridge_en && has_pci_bus) {
+        build_acpi_pci_hotplug(dsdt);
+    }
+
     if (pcmc->legacy_cpu_hotplug) {
         build_legacy_cpu_hotplug_aml(dsdt, machine, pm->cpu_hp_io_base);
     } else {
@@ -1947,7 +1954,7 @@ build_dsdt(GArray *table_data, BIOSLinker *linker,
     {
         aml_append(scope, aml_name_decl("_HID", aml_string("ACPI0006")));
 
-        if (misc->is_piix4) {
+        if (pm->pcihp_bridge_en && has_pci_bus) {
             method = aml_method("_E01", 0, AML_NOTSERIALIZED);
             aml_append(method,
                 aml_acquire(aml_name("\\_SB.PCI0.BLCK"), 0xFFFF));
@@ -2080,7 +2087,7 @@ build_dsdt(GArray *table_data, BIOSLinker *linker,
     crs_range_set_free(&crs_range_set);
 
     /* reserve PCIHP resources */
-    if (pm->pcihp_io_len) {
+    if (pm->pcihp_bridge_en && has_pci_bus && pm->pcihp_io_len) {
         dev = aml_device("PHPR");
         aml_append(dev, aml_name_decl("_HID", aml_string("PNP0A06")));
         aml_append(dev,
@@ -2212,8 +2219,13 @@ build_dsdt(GArray *table_data, BIOSLinker *linker,
     sb_scope = aml_scope("\\_SB");
     {
         Aml *scope = aml_scope("PCI0");
-        /* Scan all PCI buses. Generate tables to support hotplug. */
-        build_append_pci_bus_devices(scope, root_bus, pm->pcihp_bridge_en);
+        bool scope_used = false;
+
+        if (pm->pcihp_bridge_en && has_pci_bus) {
+            /* Scan all PCI buses. Generate tables to support hotplug. */
+            build_append_pci_bus_devices(scope, root_bus, pm->pcihp_bridge_en);
+            scope_used = true;
+        }
 
         if (misc->tpm_version != TPM_VERSION_UNSPEC) {
             dev = aml_device("ISA.TPM");
@@ -2230,9 +2242,12 @@ build_dsdt(GArray *table_data, BIOSLinker *linker,
             /* aml_append(crs, aml_irq_no_flags(TPM_TIS_IRQ)); */
             aml_append(dev, aml_name_decl("_CRS", crs));
             aml_append(scope, dev);
+            scope_used = true;
         }
 
-        aml_append(sb_scope, scope);
+        if (scope_used) {
+            aml_append(sb_scope, scope);
+        }
     }
     aml_append(dsdt, sb_scope);
 
@@ -2866,7 +2881,7 @@ void acpi_setup(void)
 
     build_state = g_malloc0(sizeof *build_state);
 
-    acpi_set_pci_info();
+    acpi_set_pci_info(pcms->bus);
 
     acpi_build_tables_init(&tables);
     acpi_build(&tables, MACHINE(pcms));
-- 
2.7.4

[Qemu-devel] [PATCH RFC 6/6] hw/ich9: enable acpi pci hotplug

[Aleksandr Bezzubikov]

Signed-off-by: Aleksandr Bezzubikov <zuban32s@gmail.com>
---
 hw/acpi/ich9.c         | 31 +++++++++++++++++++++++++++++++
 hw/isa/lpc_ich9.c      | 12 ++++++++++++
 include/hw/acpi/ich9.h |  4 ++++
 include/hw/i386/pc.h   |  7 ++++++-
 4 files changed, 53 insertions(+), 1 deletion(-)

diff --git a/hw/acpi/ich9.c b/hw/acpi/ich9.c
index 5c279bb..25339eb 100644
--- a/hw/acpi/ich9.c
+++ b/hw/acpi/ich9.c
@@ -258,6 +258,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);
 }
 
@@ -301,6 +302,10 @@ void ich9_pm_init(PCIDevice *lpc_pci, ICH9LPCPMRegs *pm,
     pm->powerdown_notifier.notify = pm_powerdown_req;
     qemu_register_powerdown_notifier(&pm->powerdown_notifier);
 
+    acpi_pcihp_init(OBJECT(lpc_pci), &pm->acpi_pci_hotplug, lpc_pci->bus,
+                    pci_address_space_io(lpc_pci),
+                    pm->use_acpi_pci_hotplug);
+
     legacy_acpi_cpu_hotplug_init(pci_address_space_io(lpc_pci),
         OBJECT(lpc_pci), &pm->gpe_cpu, ICH9_CPU_HOTPLUG_IO_BASE);
 
@@ -335,6 +340,21 @@ static void ich9_pm_set_memory_hotplug_support(Object *obj, bool value,
     s->pm.acpi_memory_hotplug.is_enabled = value;
 }
 
+static bool ich9_pm_get_acpi_pci_hotplug_support(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_support(Object *obj, bool value,
+                                                 Error **errp)
+{
+    ICH9LPCState *s = ICH9_LPC_DEVICE(obj);
+
+    s->pm.use_acpi_pci_hotplug = value;
+}
+
 static bool ich9_pm_get_cpu_hotplug_legacy(Object *obj, Error **errp)
 {
     ICH9LPCState *s = ICH9_LPC_DEVICE(obj);
@@ -446,6 +466,7 @@ void ich9_pm_add_properties(Object *obj, ICH9LPCPMRegs *pm, Error **errp)
 {
     static const uint32_t gpe0_len = ICH9_PMIO_GPE0_LEN;
     pm->acpi_memory_hotplug.is_enabled = true;
+    pm->use_acpi_pci_hotplug = true;
     pm->cpu_hotplug_legacy = true;
     pm->disable_s3 = 0;
     pm->disable_s4 = 0;
@@ -462,6 +483,10 @@ void ich9_pm_add_properties(Object *obj, ICH9LPCPMRegs *pm, Error **errp)
                              ich9_pm_get_memory_hotplug_support,
                              ich9_pm_set_memory_hotplug_support,
                              NULL);
+    object_property_add_bool(obj, "acpi-pci-hotplug-with-bridge-support",
+                             ich9_pm_get_acpi_pci_hotplug_support,
+                             ich9_pm_set_acpi_pci_hotplug_support,
+                             NULL);
     object_property_add_bool(obj, "cpu-hotplug-legacy",
                              ich9_pm_get_cpu_hotplug_legacy,
                              ich9_pm_set_cpu_hotplug_legacy,
@@ -497,6 +522,9 @@ void ich9_pm_device_plug_cb(HotplugHandler *hotplug_dev, DeviceState *dev,
             acpi_memory_plug_cb(hotplug_dev, &lpc->pm.acpi_memory_hotplug,
                                 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 if (object_dynamic_cast(OBJECT(dev), TYPE_CPU)) {
         if (lpc->pm.cpu_hotplug_legacy) {
             legacy_acpi_cpu_plug_cb(hotplug_dev, &lpc->pm.gpe_cpu, dev, errp);
@@ -519,6 +547,9 @@ void ich9_pm_device_unplug_request_cb(HotplugHandler *hotplug_dev,
         acpi_memory_unplug_request_cb(hotplug_dev,
                                       &lpc->pm.acpi_memory_hotplug, 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 if (object_dynamic_cast(OBJECT(dev), TYPE_CPU) &&
                !lpc->pm.cpu_hotplug_legacy) {
         acpi_cpu_unplug_request_cb(hotplug_dev, &lpc->pm.cpuhp_state,
diff --git a/hw/isa/lpc_ich9.c b/hw/isa/lpc_ich9.c
index e2215dc..13574d0 100644
--- a/hw/isa/lpc_ich9.c
+++ b/hw/isa/lpc_ich9.c
@@ -33,6 +33,7 @@
 #include "hw/hw.h"
 #include "qapi/visitor.h"
 #include "qemu/range.h"
+#include "qapi/error.h"
 #include "hw/isa/isa.h"
 #include "hw/sysbus.h"
 #include "hw/i386/pc.h"
@@ -574,6 +575,15 @@ static const MemoryRegionOps rcrb_mmio_ops = {
     .endianness = DEVICE_LITTLE_ENDIAN,
 };
 
+static void ich9_update_bus_hotplug(PCIBus *pci_bus, void *opaque)
+{
+    ICH9LPCState *s = opaque;
+
+    if (!pci_bus_is_express(pci_bus)) {
+        qbus_set_hotplug_handler(BUS(pci_bus), DEVICE(s), &error_abort);
+    }
+}
+
 static void ich9_lpc_machine_ready(Notifier *n, void *opaque)
 {
     ICH9LPCState *s = container_of(n, ICH9LPCState, machine_ready);
@@ -597,6 +607,8 @@ static void ich9_lpc_machine_ready(Notifier *n, void *opaque)
         /* floppy */
         pci_conf[0x82] |= 0x08;
     }
+
+    pci_for_each_bus(s->d.bus, ich9_update_bus_hotplug, s);
 }
 
 /* reset control */
diff --git a/include/hw/acpi/ich9.h b/include/hw/acpi/ich9.h
index a352c94..e1df363 100644
--- a/include/hw/acpi/ich9.h
+++ b/include/hw/acpi/ich9.h
@@ -22,6 +22,7 @@
 #define HW_ACPI_ICH9_H
 
 #include "hw/acpi/acpi.h"
+#include "hw/acpi/pcihp.h"
 #include "hw/acpi/cpu_hotplug.h"
 #include "hw/acpi/cpu.h"
 #include "hw/acpi/memory_hotplug.h"
@@ -55,6 +56,9 @@ typedef struct ICH9LPCPMRegs {
 
     MemHotplugState acpi_memory_hotplug;
 
+    AcpiPciHpState acpi_pci_hotplug;
+    bool use_acpi_pci_hotplug;
+
     uint8_t disable_s3;
     uint8_t disable_s4;
     uint8_t s4_val;
diff --git a/include/hw/i386/pc.h b/include/hw/i386/pc.h
index e447f5d..0d4e8b2 100644
--- a/include/hw/i386/pc.h
+++ b/include/hw/i386/pc.h
@@ -464,7 +464,12 @@ bool e820_get_entry(int, uint32_t, uint64_t *, uint64_t *);
     },
 
 #define PC_COMPAT_2_5 \
-    HW_COMPAT_2_5
+    HW_COMPAT_2_5 \
+    {\
+        .driver   = "ICH9-LPC",\
+        .property = "acpi-pci-hotplug-with-bridge-support",\
+        .value    = "off",\
+    },
 
 /* Helper for setting model-id for CPU models that changed model-id
  * depending on QEMU versions up to QEMU 2.4.
-- 
2.7.4

Re: [Qemu-devel] [PATCH RFC 0/6] q35: add acpi pci hotplug support

[Michael S. Tsirkin]

On Fri, Jun 30, 2017 at 12:55:56AM +0300, Aleksandr Bezzubikov wrote:
> The series adds hotplug support to legacy PCI buses for Q35 machines.
> The ACPI hotplug code is emitted if at least one legacy pci-bridge is present.
> 
> This series is mostly based on past Marcel's series 
> https://lists.gnu.org/archive/html/qemu-devel/2016-05/msg05681.html,
> but rebased on current master with some minor changes according to current codebase.
> 
> ACPI code emission approach used in this series can be called "static", 
> because it checkswhether a bridge exists only at initial DSDT generation moment. 
> The main goal is to enable AML PCI hotplug-related code to be generated dynamically. 
> In other words, the bridge plugged in - a new acpi definition block is
> loaded (using LoadTable method). 
> This is necessary for PCIE-PCI bridge hotplugging feature. 

I wonder whether we really need ACPI hotplug.
Most modern systems are limited to native PCIE.

I do understand the need to add PCI devices sometimes,
but maybe we can find a way to do this with native hotplug.

For example, how about the following approach


- PCIE downstream port - X - PCIE-to-PCI bridge - PCI device


By hotplugging the bridge+device combination into a downstream
port (point X) we can potentially make devices enumerate
properly.

This might cause some issues with IO port assignment (uses 4K
io port space due to bridge aperture limitations)
but maybe we do not need so many legacy PCI devices -
people who do can simply use piix.


For this to work we need a way to create bridge instance
that is invisible to the guest. There is already a
way to do this for multifunction devices:

create bridge in function != 0
attach device
then create a dummy function 0

Inelegant - it would be cleaner to support this for function 0
as well - but should allow you to test the idea directly.



> Aleksandr Bezzubikov (6):
>   hw/acpi: remove dead acpi code
>   hw/acpi: simplify dsdt building code
>   hw/acpi: fix pcihp io initialization
>   hw/acpi: prepare pci hotplug IO for ich9
>   hw/acpi: extend acpi pci hotplug support for pci express
>   hw/ich9: enable acpi pci hotplug
> 
>  hw/acpi/ich9.c         |  31 +++++++++++++
>  hw/i386/acpi-build.c   | 116 ++++++++++++++++++++++++-------------------------
>  hw/isa/lpc_ich9.c      |  12 +++++
>  include/hw/acpi/ich9.h |   4 ++
>  include/hw/i386/pc.h   |   7 ++-
>  5 files changed, 111 insertions(+), 59 deletions(-)
> 
> -- 
> 2.7.4

Re: [Qemu-devel] [PATCH RFC 3/6] hw/acpi: fix pcihp io initialization

[Michael S. Tsirkin]

On Fri, Jun 30, 2017 at 12:55:59AM +0300, Aleksandr Bezzubikov wrote:
> Signed-off-by: Aleksandr Bezzubikov <zuban32s@gmail.com>

Any data on what is wrong with the current code?
Pls include this in commit log.

> ---
>  hw/i386/acpi-build.c | 9 +++++----
>  1 file changed, 5 insertions(+), 4 deletions(-)
> 
> diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
> index b0dcd34..c99dbcc 100644
> --- a/hw/i386/acpi-build.c
> +++ b/hw/i386/acpi-build.c
> @@ -129,17 +129,18 @@ static void acpi_get_pm_info(AcpiPmInfo *pm)
>      Object *lpc = ich9_lpc_find();
>      Object *obj = NULL;
>      QObject *o;
> +    int pcihp_io_len, pcihp_io_base;
>  
>      pm->cpu_hp_io_base = 0;
> -    pm->pcihp_io_base = 0;
> -    pm->pcihp_io_len = 0;
>      if (piix) {
>          obj = piix;
>          pm->cpu_hp_io_base = PIIX4_CPU_HOTPLUG_IO_BASE;
> -        pm->pcihp_io_base =
> +        pcihp_io_base =
>              object_property_get_int(obj, ACPI_PCIHP_IO_BASE_PROP, NULL);
> -        pm->pcihp_io_len =
> +        pcihp_io_len =
>              object_property_get_int(obj, ACPI_PCIHP_IO_LEN_PROP, NULL);
> +        pm->pcihp_io_base = (pcihp_io_base == -1) ? 0 : pcihp_io_base;
> +        pm->pcihp_io_len = (pcihp_io_len == -1) ? 0 : pcihp_io_len;
>      }
>      if (lpc) {
>          obj = lpc;
> -- 
> 2.7.4

Re: [Qemu-devel] [PATCH RFC 1/6] hw/acpi: remove dead acpi code

[Michael S. Tsirkin]

On Fri, Jun 30, 2017 at 12:55:57AM +0300, Aleksandr Bezzubikov wrote:
> Signed-off-by: Aleksandr Bezzubikov <zuban32s@gmail.com>


I agree it seems unused. I'm not sure why do we have this
code. Igor?

> ---
>  hw/i386/acpi-build.c | 10 ----------
>  1 file changed, 10 deletions(-)
> 
> diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
> index afcadac..6fce967 100644
> --- a/hw/i386/acpi-build.c
> +++ b/hw/i386/acpi-build.c
> @@ -1912,16 +1912,6 @@ build_dsdt(GArray *table_data, BIOSLinker *linker,
>          build_piix4_pci0_int(dsdt);
>      } else {
>          sb_scope = aml_scope("_SB");
> -        aml_append(sb_scope,
> -            aml_operation_region("PCST", AML_SYSTEM_IO, aml_int(0xae00), 0x0c));
> -        aml_append(sb_scope,
> -            aml_operation_region("PCSB", AML_SYSTEM_IO, aml_int(0xae0c), 0x01));
> -        field = aml_field("PCSB", AML_ANY_ACC, AML_NOLOCK, AML_WRITE_AS_ZEROS);
> -        aml_append(field, aml_named_field("PCIB", 8));
> -        aml_append(sb_scope, field);
> -        aml_append(dsdt, sb_scope);
> -
> -        sb_scope = aml_scope("_SB");
>          dev = aml_device("PCI0");
>          aml_append(dev, aml_name_decl("_HID", aml_eisaid("PNP0A08")));
>          aml_append(dev, aml_name_decl("_CID", aml_eisaid("PNP0A03")));
> -- 
> 2.7.4

Re: [Qemu-devel] [PATCH RFC 4/6] hw/acpi: prepare pci hotplug IO for ich9

[Michael S. Tsirkin]

On Fri, Jun 30, 2017 at 12:56:00AM +0300, Aleksandr Bezzubikov wrote:
> Signed-off-by: Aleksandr Bezzubikov <zuban32s@gmail.com>

Add more explanation in commit log please.

> ---
>  hw/i386/acpi-build.c | 13 +++++++------
>  1 file changed, 7 insertions(+), 6 deletions(-)
> 
> diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
> index c99dbcc..e434efe 100644
> --- a/hw/i386/acpi-build.c
> +++ b/hw/i386/acpi-build.c
> @@ -135,12 +135,6 @@ static void acpi_get_pm_info(AcpiPmInfo *pm)
>      if (piix) {
>          obj = piix;
>          pm->cpu_hp_io_base = PIIX4_CPU_HOTPLUG_IO_BASE;
> -        pcihp_io_base =
> -            object_property_get_int(obj, ACPI_PCIHP_IO_BASE_PROP, NULL);
> -        pcihp_io_len =
> -            object_property_get_int(obj, ACPI_PCIHP_IO_LEN_PROP, NULL);
> -        pm->pcihp_io_base = (pcihp_io_base == -1) ? 0 : pcihp_io_base;
> -        pm->pcihp_io_len = (pcihp_io_len == -1) ? 0 : pcihp_io_len;
>      }
>      if (lpc) {
>          obj = lpc;
> @@ -148,6 +142,13 @@ static void acpi_get_pm_info(AcpiPmInfo *pm)
>      }
>      assert(obj);
>  
> +    pcihp_io_base =
> +        object_property_get_int(obj, ACPI_PCIHP_IO_BASE_PROP, NULL);
> +    pcihp_io_len =
> +        object_property_get_int(obj, ACPI_PCIHP_IO_LEN_PROP, NULL);
> +    pm->pcihp_io_base = (pcihp_io_base == -1) ? 0 : pcihp_io_base;
> +    pm->pcihp_io_len = (pcihp_io_len == -1) ? 0 : pcihp_io_len;
> +
>      /* Fill in optional s3/s4 related properties */
>      o = object_property_get_qobject(obj, ACPI_PM_PROP_S3_DISABLED, NULL);
>      if (o) {
> -- 
> 2.7.4

Re: [Qemu-devel] [PATCH RFC 5/6] hw/acpi: extend acpi pci hotplug support for pci express

[Michael S. Tsirkin]

On Fri, Jun 30, 2017 at 12:56:01AM +0300, Aleksandr Bezzubikov wrote:
> Signed-off-by: Aleksandr Bezzubikov <zuban32s@gmail.com>

Which means what exactly?

> ---
>  hw/i386/acpi-build.c | 47 +++++++++++++++++++++++++++++++----------------
>  1 file changed, 31 insertions(+), 16 deletions(-)
> 
> diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
> index e434efe..8bbece5 100644
> --- a/hw/i386/acpi-build.c
> +++ b/hw/i386/acpi-build.c
> @@ -254,6 +254,11 @@ static void acpi_get_pci_holes(Range *hole, Range *hole64)
>                                                NULL));
>  }
>  
> +static void acpi_test_pci_bus(PCIBus *bus, void *opaque)
> +{
> +    *(bool *)opaque |= !pci_bus_is_express(bus);
> +}
> +
>  #define ACPI_PORT_SMI_CMD           0x00b2 /* TODO: this is APM_CNT_IOPORT */
>  
>  static void acpi_align_size(GArray *blob, unsigned align)
> @@ -489,7 +494,7 @@ static void *acpi_set_bsel(PCIBus *bus, void *opaque)
>      unsigned *bsel_alloc = opaque;
>      unsigned *bus_bsel;
>  
> -    if (qbus_is_hotpluggable(BUS(bus))) {
> +    if (qbus_is_hotpluggable(BUS(bus)) && !pci_bus_is_express(bus)) {
>          bus_bsel = g_malloc(sizeof *bus_bsel);
>  
>          *bus_bsel = (*bsel_alloc)++;
> @@ -500,15 +505,12 @@ static void *acpi_set_bsel(PCIBus *bus, void *opaque)
>      return bsel_alloc;
>  }
>  
> -static void acpi_set_pci_info(void)
> +static void acpi_set_pci_info(PCIBus *bus)
>  {
> -    PCIBus *bus = find_i440fx(); /* TODO: Q35 support */
>      unsigned bsel_alloc = ACPI_PCIHP_BSEL_DEFAULT;
>  
> -    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);
> -    }
> +    /* Scan all PCI buses. Set property to enable acpi based hotplug. */
> +    pci_for_each_bus_depth_first(bus, acpi_set_bsel, NULL, &bsel_alloc);
>  }
>  
>  static void build_append_pcihp_notify_entry(Aml *method, int slot)
> @@ -570,7 +572,7 @@ static void build_append_pci_bus_devices(Aml *parent_scope, PCIBus *bus,
>           * In this case they aren't themselves hot-pluggable.
>           * Hotplugged bridges *are* hot-pluggable.
>           */
> -        bridge_in_acpi = pc->is_bridge && pcihp_bridge_en &&
> +        bridge_in_acpi = pc->is_bridge && !pc->is_express && pcihp_bridge_en &&
>              !DEVICE(pdev)->hotplugged;
>  
>          hotplug_enabled_dev = bsel && dc->hotpluggable && !bridge_in_acpi;
> @@ -1787,7 +1789,7 @@ static void build_piix4_isa_bridge(Aml *table)
>      aml_append(table, scope);
>  }
>  
> -static void build_piix4_pci_hotplug(Aml *table)
> +static void build_acpi_pci_hotplug(Aml *table)
>  {
>      Aml *scope;
>      Aml *field;
> @@ -1889,6 +1891,7 @@ build_dsdt(GArray *table_data, BIOSLinker *linker,
>      uint32_t nr_mem = machine->ram_slots;
>      int root_bus_limit = 0xFF;
>      PCIBus *root_bus = NULL;
> +    bool has_pci_bus = false;
>      int i;
>  
>      dsdt = init_aml_allocator();
> @@ -1910,7 +1913,6 @@ build_dsdt(GArray *table_data, BIOSLinker *linker,
>          build_piix4_pm(dsdt);
>          build_piix4_isa_bridge(dsdt);
>          build_isa_devices_aml(dsdt);
> -        build_piix4_pci_hotplug(dsdt);
>          build_piix4_pci0_int(dsdt);
>      } else {
>          sb_scope = aml_scope("_SB");
> @@ -1932,6 +1934,11 @@ build_dsdt(GArray *table_data, BIOSLinker *linker,
>      root_bus = PC_MACHINE(machine)->bus;
>      assert(root_bus);
>  
> +    pci_for_each_bus(root_bus, acpi_test_pci_bus, &has_pci_bus);
> +    if (pm->pcihp_bridge_en && has_pci_bus) {
> +        build_acpi_pci_hotplug(dsdt);
> +    }
> +
>      if (pcmc->legacy_cpu_hotplug) {
>          build_legacy_cpu_hotplug_aml(dsdt, machine, pm->cpu_hp_io_base);
>      } else {
> @@ -1947,7 +1954,7 @@ build_dsdt(GArray *table_data, BIOSLinker *linker,
>      {
>          aml_append(scope, aml_name_decl("_HID", aml_string("ACPI0006")));
>  
> -        if (misc->is_piix4) {
> +        if (pm->pcihp_bridge_en && has_pci_bus) {
>              method = aml_method("_E01", 0, AML_NOTSERIALIZED);
>              aml_append(method,
>                  aml_acquire(aml_name("\\_SB.PCI0.BLCK"), 0xFFFF));
> @@ -2080,7 +2087,7 @@ build_dsdt(GArray *table_data, BIOSLinker *linker,
>      crs_range_set_free(&crs_range_set);
>  
>      /* reserve PCIHP resources */
> -    if (pm->pcihp_io_len) {
> +    if (pm->pcihp_bridge_en && has_pci_bus && pm->pcihp_io_len) {
>          dev = aml_device("PHPR");
>          aml_append(dev, aml_name_decl("_HID", aml_string("PNP0A06")));
>          aml_append(dev,
> @@ -2212,8 +2219,13 @@ build_dsdt(GArray *table_data, BIOSLinker *linker,
>      sb_scope = aml_scope("\\_SB");
>      {
>          Aml *scope = aml_scope("PCI0");
> -        /* Scan all PCI buses. Generate tables to support hotplug. */
> -        build_append_pci_bus_devices(scope, root_bus, pm->pcihp_bridge_en);
> +        bool scope_used = false;
> +
> +        if (pm->pcihp_bridge_en && has_pci_bus) {
> +            /* Scan all PCI buses. Generate tables to support hotplug. */
> +            build_append_pci_bus_devices(scope, root_bus, pm->pcihp_bridge_en);
> +            scope_used = true;
> +        }
>  
>          if (misc->tpm_version != TPM_VERSION_UNSPEC) {
>              dev = aml_device("ISA.TPM");
> @@ -2230,9 +2242,12 @@ build_dsdt(GArray *table_data, BIOSLinker *linker,
>              /* aml_append(crs, aml_irq_no_flags(TPM_TIS_IRQ)); */
>              aml_append(dev, aml_name_decl("_CRS", crs));
>              aml_append(scope, dev);
> +            scope_used = true;
>          }
>  
> -        aml_append(sb_scope, scope);
> +        if (scope_used) {
> +            aml_append(sb_scope, scope);
> +        }
>      }
>      aml_append(dsdt, sb_scope);
>  
> @@ -2866,7 +2881,7 @@ void acpi_setup(void)
>  
>      build_state = g_malloc0(sizeof *build_state);
>  
> -    acpi_set_pci_info();
> +    acpi_set_pci_info(pcms->bus);
>  
>      acpi_build_tables_init(&tables);
>      acpi_build(&tables, MACHINE(pcms));
> -- 
> 2.7.4

Re: [Qemu-devel] [PATCH RFC 6/6] hw/ich9: enable acpi pci hotplug

[Michael S. Tsirkin]

On Fri, Jun 30, 2017 at 12:56:02AM +0300, Aleksandr Bezzubikov wrote:
> Signed-off-by: Aleksandr Bezzubikov <zuban32s@gmail.com>

This commit log is where you describe what works and
what doesn't so people can figure it out from git log.

> ---
>  hw/acpi/ich9.c         | 31 +++++++++++++++++++++++++++++++
>  hw/isa/lpc_ich9.c      | 12 ++++++++++++
>  include/hw/acpi/ich9.h |  4 ++++
>  include/hw/i386/pc.h   |  7 ++++++-
>  4 files changed, 53 insertions(+), 1 deletion(-)
> 
> diff --git a/hw/acpi/ich9.c b/hw/acpi/ich9.c
> index 5c279bb..25339eb 100644
> --- a/hw/acpi/ich9.c
> +++ b/hw/acpi/ich9.c
> @@ -258,6 +258,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);
>  }
>  
> @@ -301,6 +302,10 @@ void ich9_pm_init(PCIDevice *lpc_pci, ICH9LPCPMRegs *pm,
>      pm->powerdown_notifier.notify = pm_powerdown_req;
>      qemu_register_powerdown_notifier(&pm->powerdown_notifier);
>  
> +    acpi_pcihp_init(OBJECT(lpc_pci), &pm->acpi_pci_hotplug, lpc_pci->bus,
> +                    pci_address_space_io(lpc_pci),
> +                    pm->use_acpi_pci_hotplug);
> +
>      legacy_acpi_cpu_hotplug_init(pci_address_space_io(lpc_pci),
>          OBJECT(lpc_pci), &pm->gpe_cpu, ICH9_CPU_HOTPLUG_IO_BASE);
>  
> @@ -335,6 +340,21 @@ static void ich9_pm_set_memory_hotplug_support(Object *obj, bool value,
>      s->pm.acpi_memory_hotplug.is_enabled = value;
>  }
>  
> +static bool ich9_pm_get_acpi_pci_hotplug_support(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_support(Object *obj, bool value,
> +                                                 Error **errp)
> +{
> +    ICH9LPCState *s = ICH9_LPC_DEVICE(obj);
> +
> +    s->pm.use_acpi_pci_hotplug = value;
> +}
> +
>  static bool ich9_pm_get_cpu_hotplug_legacy(Object *obj, Error **errp)
>  {
>      ICH9LPCState *s = ICH9_LPC_DEVICE(obj);
> @@ -446,6 +466,7 @@ void ich9_pm_add_properties(Object *obj, ICH9LPCPMRegs *pm, Error **errp)
>  {
>      static const uint32_t gpe0_len = ICH9_PMIO_GPE0_LEN;
>      pm->acpi_memory_hotplug.is_enabled = true;
> +    pm->use_acpi_pci_hotplug = true;
>      pm->cpu_hotplug_legacy = true;
>      pm->disable_s3 = 0;
>      pm->disable_s4 = 0;
> @@ -462,6 +483,10 @@ void ich9_pm_add_properties(Object *obj, ICH9LPCPMRegs *pm, Error **errp)
>                               ich9_pm_get_memory_hotplug_support,
>                               ich9_pm_set_memory_hotplug_support,
>                               NULL);
> +    object_property_add_bool(obj, "acpi-pci-hotplug-with-bridge-support",
> +                             ich9_pm_get_acpi_pci_hotplug_support,
> +                             ich9_pm_set_acpi_pci_hotplug_support,
> +                             NULL);
>      object_property_add_bool(obj, "cpu-hotplug-legacy",
>                               ich9_pm_get_cpu_hotplug_legacy,
>                               ich9_pm_set_cpu_hotplug_legacy,
> @@ -497,6 +522,9 @@ void ich9_pm_device_plug_cb(HotplugHandler *hotplug_dev, DeviceState *dev,
>              acpi_memory_plug_cb(hotplug_dev, &lpc->pm.acpi_memory_hotplug,
>                                  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 if (object_dynamic_cast(OBJECT(dev), TYPE_CPU)) {
>          if (lpc->pm.cpu_hotplug_legacy) {
>              legacy_acpi_cpu_plug_cb(hotplug_dev, &lpc->pm.gpe_cpu, dev, errp);
> @@ -519,6 +547,9 @@ void ich9_pm_device_unplug_request_cb(HotplugHandler *hotplug_dev,
>          acpi_memory_unplug_request_cb(hotplug_dev,
>                                        &lpc->pm.acpi_memory_hotplug, 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 if (object_dynamic_cast(OBJECT(dev), TYPE_CPU) &&
>                 !lpc->pm.cpu_hotplug_legacy) {
>          acpi_cpu_unplug_request_cb(hotplug_dev, &lpc->pm.cpuhp_state,
> diff --git a/hw/isa/lpc_ich9.c b/hw/isa/lpc_ich9.c
> index e2215dc..13574d0 100644
> --- a/hw/isa/lpc_ich9.c
> +++ b/hw/isa/lpc_ich9.c
> @@ -33,6 +33,7 @@
>  #include "hw/hw.h"
>  #include "qapi/visitor.h"
>  #include "qemu/range.h"
> +#include "qapi/error.h"
>  #include "hw/isa/isa.h"
>  #include "hw/sysbus.h"
>  #include "hw/i386/pc.h"
> @@ -574,6 +575,15 @@ static const MemoryRegionOps rcrb_mmio_ops = {
>      .endianness = DEVICE_LITTLE_ENDIAN,
>  };
>  
> +static void ich9_update_bus_hotplug(PCIBus *pci_bus, void *opaque)
> +{
> +    ICH9LPCState *s = opaque;
> +
> +    if (!pci_bus_is_express(pci_bus)) {
> +        qbus_set_hotplug_handler(BUS(pci_bus), DEVICE(s), &error_abort);
> +    }
> +}
> +
>  static void ich9_lpc_machine_ready(Notifier *n, void *opaque)
>  {
>      ICH9LPCState *s = container_of(n, ICH9LPCState, machine_ready);
> @@ -597,6 +607,8 @@ static void ich9_lpc_machine_ready(Notifier *n, void *opaque)
>          /* floppy */
>          pci_conf[0x82] |= 0x08;
>      }
> +
> +    pci_for_each_bus(s->d.bus, ich9_update_bus_hotplug, s);
>  }
>  
>  /* reset control */
> diff --git a/include/hw/acpi/ich9.h b/include/hw/acpi/ich9.h
> index a352c94..e1df363 100644
> --- a/include/hw/acpi/ich9.h
> +++ b/include/hw/acpi/ich9.h
> @@ -22,6 +22,7 @@
>  #define HW_ACPI_ICH9_H
>  
>  #include "hw/acpi/acpi.h"
> +#include "hw/acpi/pcihp.h"
>  #include "hw/acpi/cpu_hotplug.h"
>  #include "hw/acpi/cpu.h"
>  #include "hw/acpi/memory_hotplug.h"
> @@ -55,6 +56,9 @@ typedef struct ICH9LPCPMRegs {
>  
>      MemHotplugState acpi_memory_hotplug;
>  
> +    AcpiPciHpState acpi_pci_hotplug;
> +    bool use_acpi_pci_hotplug;
> +
>      uint8_t disable_s3;
>      uint8_t disable_s4;
>      uint8_t s4_val;
> diff --git a/include/hw/i386/pc.h b/include/hw/i386/pc.h
> index e447f5d..0d4e8b2 100644
> --- a/include/hw/i386/pc.h
> +++ b/include/hw/i386/pc.h
> @@ -464,7 +464,12 @@ bool e820_get_entry(int, uint32_t, uint64_t *, uint64_t *);
>      },
>  
>  #define PC_COMPAT_2_5 \
> -    HW_COMPAT_2_5
> +    HW_COMPAT_2_5 \
> +    {\
> +        .driver   = "ICH9-LPC",\
> +        .property = "acpi-pci-hotplug-with-bridge-support",\
> +        .value    = "off",\
> +    },
>  
>  /* Helper for setting model-id for CPU models that changed model-id
>   * depending on QEMU versions up to QEMU 2.4.
> -- 
> 2.7.4

Re: [Qemu-devel] [PATCH RFC 0/6] q35: add acpi pci hotplug support

[Marcel Apfelbaum]

On 30/06/2017 2:17, Michael S. Tsirkin wrote:
> On Fri, Jun 30, 2017 at 12:55:56AM +0300, Aleksandr Bezzubikov wrote:
>> The series adds hotplug support to legacy PCI buses for Q35 machines.
>> The ACPI hotplug code is emitted if at least one legacy pci-bridge is present.
>>
>> This series is mostly based on past Marcel's series
>> https://lists.gnu.org/archive/html/qemu-devel/2016-05/msg05681.html,
>> but rebased on current master with some minor changes according to current codebase.
>>
>> ACPI code emission approach used in this series can be called "static",
>> because it checkswhether a bridge exists only at initial DSDT generation moment.
>> The main goal is to enable AML PCI hotplug-related code to be generated dynamically.
>> In other words, the bridge plugged in - a new acpi definition block is
>> loaded (using LoadTable method).
>> This is necessary for PCIE-PCI bridge hotplugging feature.
> 

Hi Michael,
Thank you for looking into this.

> I wonder whether we really need ACPI hotplug.
> Most modern systems are limited to native PCIE.
> 

I was under impression that ACPI hotplug will still remain
for memory/cpu hotplug, but I might be wrong. If this
is the case (ACPI hotplug is used for other subsystems),
I don't see why modern system would disable the PCI APCI hoptplug.
(I am not saying PCIe hotplug is not preferred.)

And if the modern systems are limited to native PCIe
we might have a bigger problem since the QEMU default
x86 machine is PCI based and we don't have PCIe obviously...
maybe is this the right time to switch to Q35 as the default machine?
(I am aware it should be a different discussion)


> I do understand the need to add PCI devices sometimes,
> but maybe we can find a way to do this with native hotplug.
> 
> For example, how about the following approach
> 
> 
> - PCIE downstream port - X - PCIE-to-PCI bridge - PCI device
> 

It can be a solution, yes, but then we would limit (seriously)
the number of PCI devices we can add. It will not be a problem
if we will continue to keep Q35 for "modern systems" only machine
and keep PC around as the default machine. However adding full support
for pci-bridge will allow us to switch to Q35 and use it
as the default machine (sometime sooner) since it would
be easier to map older configurations.

> 
> By hotplugging the bridge+device combination into a downstream
> port (point X) we can potentially make devices enumerate
> properly.
> 

It may work, yes, Alexandr will you be willing to try it?

> This might cause some issues with IO port assignment (uses 4K
> io port space due to bridge aperture limitations)
> but maybe we do not need so many legacy PCI devices -
> people who do can simply use piix.
> 

IO port assignment issue can be solved by using non standard
IO port space, some OSes can actually deal with it (I think),
however it will not solve the limitation of the number of
pci devices we can have, since each device (PCIe or not) will be
under a different bus we will have 256 PCI devices max.
We can use multi-functions, but then the hot-plug granularity
goes away.

> 
> For this to work we need a way to create bridge instance
> that is invisible to the guest. There is already a
> way to do this for multifunction devices:
> 
> create bridge in function != 0
> attach device
> then create a dummy function 0
> 
> Inelegant - it would be cleaner to support this for function 0
> as well - but should allow you to test the idea directly.
> 

The benefit of this project is to  make Q35 a "wider" machine
that would include all prev QEMU devices and will facilitate
the transition from the pc to q35 machine.

So for the modern systems not supporting PCI ACPI hotplug
we don't need pci-bridges anyway, but for the older ones
the ACPI code of the pci-bridge will be loaded into the
ACPI namespace only if a pci-bridge is actually hot-plugged.

That being said, if we decide that q35 will be used for
modern systems only and the pc machine will remain the
default for the next years, we may try to bundle
the pci-bridge with the device/s behind it.

Thanks,
Marcel

> 
> 
>> Aleksandr Bezzubikov (6):
>>    hw/acpi: remove dead acpi code
>>    hw/acpi: simplify dsdt building code
>>    hw/acpi: fix pcihp io initialization
>>    hw/acpi: prepare pci hotplug IO for ich9
>>    hw/acpi: extend acpi pci hotplug support for pci express
>>    hw/ich9: enable acpi pci hotplug
>>
>>   hw/acpi/ich9.c         |  31 +++++++++++++
>>   hw/i386/acpi-build.c   | 116 ++++++++++++++++++++++++-------------------------
>>   hw/isa/lpc_ich9.c      |  12 +++++
>>   include/hw/acpi/ich9.h |   4 ++
>>   include/hw/i386/pc.h   |   7 ++-
>>   5 files changed, 111 insertions(+), 59 deletions(-)
>>
>> -- 
>> 2.7.4

Re: [Qemu-devel] [PATCH RFC 0/6] q35: add acpi pci hotplug support

[Michael S. Tsirkin]

On Fri, Jun 30, 2017 at 10:25:05AM +0300, Marcel Apfelbaum wrote:
> On 30/06/2017 2:17, Michael S. Tsirkin wrote:
> > On Fri, Jun 30, 2017 at 12:55:56AM +0300, Aleksandr Bezzubikov wrote:
> > > The series adds hotplug support to legacy PCI buses for Q35 machines.
> > > The ACPI hotplug code is emitted if at least one legacy pci-bridge is present.
> > > 
> > > This series is mostly based on past Marcel's series
> > > https://lists.gnu.org/archive/html/qemu-devel/2016-05/msg05681.html,
> > > but rebased on current master with some minor changes according to current codebase.
> > > 
> > > ACPI code emission approach used in this series can be called "static",
> > > because it checkswhether a bridge exists only at initial DSDT generation moment.
> > > The main goal is to enable AML PCI hotplug-related code to be generated dynamically.
> > > In other words, the bridge plugged in - a new acpi definition block is
> > > loaded (using LoadTable method).
> > > This is necessary for PCIE-PCI bridge hotplugging feature.
> > 
> 
> Hi Michael,
> Thank you for looking into this.
> 
> > I wonder whether we really need ACPI hotplug.
> > Most modern systems are limited to native PCIE.
> > 
> 
> I was under impression that ACPI hotplug will still remain
> for memory/cpu hotplug, but I might be wrong. If this
> is the case (ACPI hotplug is used for other subsystems),
> I don't see why modern system would disable the PCI APCI hoptplug.
> (I am not saying PCIe hotplug is not preferred.)
> 
> And if the modern systems are limited to native PCIe
> we might have a bigger problem since the QEMU default
> x86 machine is PCI based and we don't have PCIe obviously...
> maybe is this the right time to switch to Q35 as the default machine?
> (I am aware it should be a different discussion)
> 
> 
> > I do understand the need to add PCI devices sometimes,
> > but maybe we can find a way to do this with native hotplug.
> > 
> > For example, how about the following approach
> > 
> > 
> > - PCIE downstream port - X - PCIE-to-PCI bridge - PCI device
> > 
> 
> It can be a solution, yes, but then we would limit (seriously)
> the number of PCI devices we can add. It will not be a problem
> if we will continue to keep Q35 for "modern systems" only machine
> and keep PC around as the default machine. However adding full support
> for pci-bridge will allow us to switch to Q35 and use it
> as the default machine (sometime sooner) since it would
> be easier to map older configurations.

Part of the value from where I stand is dropping support for
a large matrix of configurations, without hurting users.
So we maintain piix but add features to q35 only.

Where are these pci devices coming from that you need such a large
number of them?  Part of the issue is things like sound, but these
aren't even always hotpluggable.

> > 
> > By hotplugging the bridge+device combination into a downstream
> > port (point X) we can potentially make devices enumerate
> > properly.
> > 
> 
> It may work, yes, Alexandr will you be willing to try it?
> 
> > This might cause some issues with IO port assignment (uses 4K
> > io port space due to bridge aperture limitations)
> > but maybe we do not need so many legacy PCI devices -
> > people who do can simply use piix.
> > 
> 
> IO port assignment issue can be solved by using non standard
> IO port space, some OSes can actually deal with it (I think),
> however it will not solve the limitation of the number of
> pci devices we can have, since each device (PCIe or not) will be
> under a different bus we will have 256 PCI devices max.
> We can use multi-functions, but then the hot-plug granularity
> goes away.

That's quite a lot. SRIOV systems sometimes go higher because
people expose each VF as a separate device to the guest,
but these almost always
. are pcie
. have no io space

> > 
> > For this to work we need a way to create bridge instance
> > that is invisible to the guest. There is already a
> > way to do this for multifunction devices:
> > 
> > create bridge in function != 0
> > attach device
> > then create a dummy function 0
> > 
> > Inelegant - it would be cleaner to support this for function 0
> > as well - but should allow you to test the idea directly.
> > 
> 
> The benefit of this project is to  make Q35 a "wider" machine
> that would include all prev QEMU devices and will facilitate
> the transition from the pc to q35 machine.

But it's not a given that it's a win. We carry in a bunch of
crappy half-way supported devices. No way to drop them from piix.

> So for the modern systems not supporting PCI ACPI hotplug
> we don't need pci-bridges anyway, but for the older ones
> the ACPI code of the pci-bridge will be loaded into the
> ACPI namespace only if a pci-bridge is actually hot-plugged.
> 
> That being said, if we decide that q35 will be used for
> modern systems only and the pc machine will remain the
> default for the next years, we may try to bundle
> the pci-bridge with the device/s behind it.
> 
> Thanks,
> Marcel

I'm not sure it matters what the default is, but yea.
Let's look at the big picture, yes we can add a PV interface
and support this, but should we?

> > 
> > 
> > > Aleksandr Bezzubikov (6):
> > >    hw/acpi: remove dead acpi code
> > >    hw/acpi: simplify dsdt building code
> > >    hw/acpi: fix pcihp io initialization
> > >    hw/acpi: prepare pci hotplug IO for ich9
> > >    hw/acpi: extend acpi pci hotplug support for pci express
> > >    hw/ich9: enable acpi pci hotplug
> > > 
> > >   hw/acpi/ich9.c         |  31 +++++++++++++
> > >   hw/i386/acpi-build.c   | 116 ++++++++++++++++++++++++-------------------------
> > >   hw/isa/lpc_ich9.c      |  12 +++++
> > >   include/hw/acpi/ich9.h |   4 ++
> > >   include/hw/i386/pc.h   |   7 ++-
> > >   5 files changed, 111 insertions(+), 59 deletions(-)
> > > 
> > > -- 
> > > 2.7.4

Re: [Qemu-devel] [PATCH RFC 1/6] hw/acpi: remove dead acpi code

[Igor Mammedov]

On Fri, 30 Jun 2017 02:27:40 +0300
"Michael S. Tsirkin" <mst@redhat.com> wrote:

> On Fri, Jun 30, 2017 at 12:55:57AM +0300, Aleksandr Bezzubikov wrote:
> > Signed-off-by: Aleksandr Bezzubikov <zuban32s@gmail.com>  
> 
> 
> I agree it seems unused. I'm not sure why do we have this
> code. Igor?
it's leftovers of original ASL code from SeaBIOS,
my conversion only was 1:1 from ASL to AML to avoid regressions
and almost no cleanups has been done on result.

It should be safe to remove these objects as they are not used by
Q35 (no AML or hw users for these ports)

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

> 
> > ---
> >  hw/i386/acpi-build.c | 10 ----------
> >  1 file changed, 10 deletions(-)
> > 
> > diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
> > index afcadac..6fce967 100644
> > --- a/hw/i386/acpi-build.c
> > +++ b/hw/i386/acpi-build.c
> > @@ -1912,16 +1912,6 @@ build_dsdt(GArray *table_data, BIOSLinker *linker,
> >          build_piix4_pci0_int(dsdt);
> >      } else {
> >          sb_scope = aml_scope("_SB");
> > -        aml_append(sb_scope,
> > -            aml_operation_region("PCST", AML_SYSTEM_IO, aml_int(0xae00), 0x0c));
> > -        aml_append(sb_scope,
> > -            aml_operation_region("PCSB", AML_SYSTEM_IO, aml_int(0xae0c), 0x01));
> > -        field = aml_field("PCSB", AML_ANY_ACC, AML_NOLOCK, AML_WRITE_AS_ZEROS);
> > -        aml_append(field, aml_named_field("PCIB", 8));
> > -        aml_append(sb_scope, field);
> > -        aml_append(dsdt, sb_scope);
> > -
> > -        sb_scope = aml_scope("_SB");
> >          dev = aml_device("PCI0");
> >          aml_append(dev, aml_name_decl("_HID", aml_eisaid("PNP0A08")));
> >          aml_append(dev, aml_name_decl("_CID", aml_eisaid("PNP0A03")));
> > -- 
> > 2.7.4  

Re: [Qemu-devel] [PATCH RFC 0/6] q35: add acpi pci hotplug support

[Igor Mammedov]

On Fri, 30 Jun 2017 10:25:05 +0300
Marcel Apfelbaum <marcel@redhat.com> wrote:

[...]
> 
> So for the modern systems not supporting PCI ACPI hotplug
> we don't need pci-bridges anyway, but for the older ones
> the ACPI code of the pci-bridge will be loaded into the
> ACPI namespace only if a pci-bridge is actually hot-plugged.

just note that the set of 'older' guest OSes is limited to
one that do not support SHPC (i.e. to EOLed WinXP & co)
as for linux and more modern Windows SHPC hotplug should
just work without our ACPI hack (which taxes low memory
to keep acpi tables for bridges).

So I'm in favor of Michael's suggestion to leave ACPI PCI
only in PC machine for old WinXP guests and to keep Q35
clean, where linux or newer Windows guests could just
use standard SHPC.

[...]

Re: [Qemu-devel] [PATCH RFC 0/6] q35: add acpi pci hotplug support

[Alexander Bezzubikov]

Hmm, I've failed to make SHPC (from current master) work on any modern
Linux guests.

2017-07-03 15:27 GMT+03:00 Igor Mammedov <imammedo@redhat.com>:

> On Fri, 30 Jun 2017 10:25:05 +0300
> Marcel Apfelbaum <marcel@redhat.com> wrote:
>
> [...]
> >
> > So for the modern systems not supporting PCI ACPI hotplug
> > we don't need pci-bridges anyway, but for the older ones
> > the ACPI code of the pci-bridge will be loaded into the
> > ACPI namespace only if a pci-bridge is actually hot-plugged.
>
> just note that the set of 'older' guest OSes is limited to
> one that do not support SHPC (i.e. to EOLed WinXP & co)
> as for linux and more modern Windows SHPC hotplug should
> just work without our ACPI hack (which taxes low memory
> to keep acpi tables for bridges).
>
> So I'm in favor of Michael's suggestion to leave ACPI PCI
> only in PC machine for old WinXP guests and to keep Q35
> clean, where linux or newer Windows guests could just
> use standard SHPC.
>
> [...]
>



-- 
Alexander Bezzubikov

Re: [Qemu-devel] [PATCH RFC 0/6] q35: add acpi pci hotplug support

[Alexander Bezzubikov]

Do we have any surely working scenario of pci-bridge's SHPC usage? Because
first of all I've tried to use SHPC to avoi involving any ACPI code, but it
didn't look functioning even for ordinary pci-bridge, that's why after
talking to Marcel I left SHPC idea.

2017-07-03 16:58 GMT+03:00 Alexander Bezzubikov <zuban32s@gmail.com>:

> Hmm, I've failed to make SHPC (from current master) work on any modern
> Linux guests.
>
> 2017-07-03 15:27 GMT+03:00 Igor Mammedov <imammedo@redhat.com>:
>
>> On Fri, 30 Jun 2017 10:25:05 +0300
>> Marcel Apfelbaum <marcel@redhat.com> wrote:
>>
>> [...]
>> >
>> > So for the modern systems not supporting PCI ACPI hotplug
>> > we don't need pci-bridges anyway, but for the older ones
>> > the ACPI code of the pci-bridge will be loaded into the
>> > ACPI namespace only if a pci-bridge is actually hot-plugged.
>>
>> just note that the set of 'older' guest OSes is limited to
>> one that do not support SHPC (i.e. to EOLed WinXP & co)
>> as for linux and more modern Windows SHPC hotplug should
>> just work without our ACPI hack (which taxes low memory
>> to keep acpi tables for bridges).
>>
>> So I'm in favor of Michael's suggestion to leave ACPI PCI
>> only in PC machine for old WinXP guests and to keep Q35
>> clean, where linux or newer Windows guests could just
>> use standard SHPC.
>>
>> [...]
>>
>
>
>
> --
> Alexander Bezzubikov
>



-- 
Alexander Bezzubikov

Re: [Qemu-devel] [PATCH RFC 0/6] q35: add acpi pci hotplug support

[Michael S. Tsirkin]

On Mon, Jul 03, 2017 at 02:27:11PM +0200, Igor Mammedov wrote:
> On Fri, 30 Jun 2017 10:25:05 +0300
> Marcel Apfelbaum <marcel@redhat.com> wrote:
> 
> [...]
> > 
> > So for the modern systems not supporting PCI ACPI hotplug
> > we don't need pci-bridges anyway, but for the older ones
> > the ACPI code of the pci-bridge will be loaded into the
> > ACPI namespace only if a pci-bridge is actually hot-plugged.
> 
> just note that the set of 'older' guest OSes is limited to
> one that do not support SHPC (i.e. to EOLed WinXP & co)
> as for linux and more modern Windows SHPC hotplug should
> just work without our ACPI hack (which taxes low memory
> to keep acpi tables for bridges).
> 
> So I'm in favor of Michael's suggestion to leave ACPI PCI
> only in PC machine for old WinXP guests and to keep Q35
> clean, where linux or newer Windows guests could just
> use standard SHPC.
> 
> [...]

I didn't realize windows actually supports SHPC for PCI.

Do they correctly set _OSC Arg3, bit offset 1?
	SHPC Native Hot Plug control
	The OS sets this bit to 1 to request control over PCI/PCI-X Standard Hot-Plug Controller
	(SHPC) hot plug. If the OS successfully receives control of this feature, it must track and
	update the status of hot plug slots and handle hot plug events as described in the SHPC
	Specification.
I was under impression they only set bit 0.

-- 
MST

Re: [Qemu-devel] [PATCH RFC 0/6] q35: add acpi pci hotplug support

[Michael S. Tsirkin]

SHPC did work for linux. As ACPI is enabled by default, you need
to expose it in _OSC and maybe jump through more hoops
to avoid ACPI from trying to take over.

Source is available so if it doesn't work you should be able
to find out why.



On Mon, Jul 03, 2017 at 05:41:12PM +0300, Alexander Bezzubikov wrote:
> Do we have any surely working scenario of pci-bridge's SHPC usage? Because
> first of all I've tried to use SHPC to avoi involving any ACPI code, but it
> didn't look functioning even for ordinary pci-bridge, that's why after talking
> to Marcel I left SHPC idea.
> 
> 2017-07-03 16:58 GMT+03:00 Alexander Bezzubikov <zuban32s@gmail.com>:
> 
>     Hmm, I've failed to make SHPC (from current master) work on any modern
>     Linux guests.
> 
>     2017-07-03 15:27 GMT+03:00 Igor Mammedov <imammedo@redhat.com>:
> 
>         On Fri, 30 Jun 2017 10:25:05 +0300
>         Marcel Apfelbaum <marcel@redhat.com> wrote:
> 
>         [...]
>         >
>         > So for the modern systems not supporting PCI ACPI hotplug
>         > we don't need pci-bridges anyway, but for the older ones
>         > the ACPI code of the pci-bridge will be loaded into the
>         > ACPI namespace only if a pci-bridge is actually hot-plugged.
> 
>         just note that the set of 'older' guest OSes is limited to
>         one that do not support SHPC (i.e. to EOLed WinXP & co)
>         as for linux and more modern Windows SHPC hotplug should
>         just work without our ACPI hack (which taxes low memory
>         to keep acpi tables for bridges).
> 
>         So I'm in favor of Michael's suggestion to leave ACPI PCI
>         only in PC machine for old WinXP guests and to keep Q35
>         clean, where linux or newer Windows guests could just
>         use standard SHPC.
> 
>         [...]
> 
> 
> 
> 
>     --
>     Alexander Bezzubikov
> 
> 
> 
> 
> --
> Alexander Bezzubikov

Re: [Qemu-devel] [PATCH RFC 0/6] q35: add acpi pci hotplug support

[Marcel Apfelbaum]

On 03/07/2017 19:34, Michael S. Tsirkin wrote:
> On Mon, Jul 03, 2017 at 02:27:11PM +0200, Igor Mammedov wrote:
>> On Fri, 30 Jun 2017 10:25:05 +0300
>> Marcel Apfelbaum <marcel@redhat.com> wrote:
>>
>> [...]
>>>
>>> So for the modern systems not supporting PCI ACPI hotplug
>>> we don't need pci-bridges anyway, but for the older ones
>>> the ACPI code of the pci-bridge will be loaded into the
>>> ACPI namespace only if a pci-bridge is actually hot-plugged.
>>
>> just note that the set of 'older' guest OSes is limited to
>> one that do not support SHPC (i.e. to EOLed WinXP & co)
>> as for linux and more modern Windows SHPC hotplug should
>> just work without our ACPI hack (which taxes low memory
>> to keep acpi tables for bridges).
>>
>> So I'm in favor of Michael's suggestion to leave ACPI PCI
>> only in PC machine for old WinXP guests and to keep Q35
>> clean, where linux or newer Windows guests could just
>> use standard SHPC.
>>
>> [...]
> 
> I didn't realize windows actually supports SHPC for PCI.

Me neither, if Igor is right I am all for shpc hotplug
since Q35 is not supposed to support older guests.

I remember I succeeded to enable shpc hotplug some time
ago, but only for Linux guests.

Igor, do you have some spec/doc on newer Windows OSes that confirm
PCI shpc hotplug support?

> 
> Do they correctly set _OSC Arg3, bit offset 1?
> 	SHPC Native Hot Plug control
> 	The OS sets this bit to 1 to request control over PCI/PCI-X Standard Hot-Plug Controller
> 	(SHPC) hot plug. If the OS successfully receives control of this feature, it must track and
> 	update the status of hot plug slots and handle hot plug events as described in the SHPC
> 	Specification.
> I was under impression they only set bit 0.
> 

Alexandr, if modern Windows OSes do support shpc, it makes our
job easier, can you please try to enable shpc hotplug?

Thanks,
Marcel

Re: [Qemu-devel] [PATCH RFC 0/6] q35: add acpi pci hotplug support

[Michael S. Tsirkin]

On Mon, Jul 03, 2017 at 09:26:33PM +0300, Marcel Apfelbaum wrote:
> On 03/07/2017 19:34, Michael S. Tsirkin wrote:
> > On Mon, Jul 03, 2017 at 02:27:11PM +0200, Igor Mammedov wrote:
> > > On Fri, 30 Jun 2017 10:25:05 +0300
> > > Marcel Apfelbaum <marcel@redhat.com> wrote:
> > > 
> > > [...]
> > > > 
> > > > So for the modern systems not supporting PCI ACPI hotplug
> > > > we don't need pci-bridges anyway, but for the older ones
> > > > the ACPI code of the pci-bridge will be loaded into the
> > > > ACPI namespace only if a pci-bridge is actually hot-plugged.
> > > 
> > > just note that the set of 'older' guest OSes is limited to
> > > one that do not support SHPC (i.e. to EOLed WinXP & co)
> > > as for linux and more modern Windows SHPC hotplug should
> > > just work without our ACPI hack (which taxes low memory
> > > to keep acpi tables for bridges).
> > > 
> > > So I'm in favor of Michael's suggestion to leave ACPI PCI
> > > only in PC machine for old WinXP guests and to keep Q35
> > > clean, where linux or newer Windows guests could just
> > > use standard SHPC.
> > > 
> > > [...]
> > 
> > I didn't realize windows actually supports SHPC for PCI.
> 
> Me neither, if Igor is right I am all for shpc hotplug
> since Q35 is not supposed to support older guests.
> 
> I remember I succeeded to enable shpc hotplug some time
> ago, but only for Linux guests.
> 
> Igor, do you have some spec/doc on newer Windows OSes that confirm
> PCI shpc hotplug support?

Just try it, easier than poking at specs which aren't always up to date.

> > 
> > Do they correctly set _OSC Arg3, bit offset 1?
> > 	SHPC Native Hot Plug control
> > 	The OS sets this bit to 1 to request control over PCI/PCI-X Standard Hot-Plug Controller
> > 	(SHPC) hot plug. If the OS successfully receives control of this feature, it must track and
> > 	update the status of hot plug slots and handle hot plug events as described in the SHPC
> > 	Specification.
> > I was under impression they only set bit 0.
> > 
> 
> Alexandr, if modern Windows OSes do support shpc, it makes our
> job easier, can you please try to enable shpc hotplug?
> 
> Thanks,
> Marcel

No need to enable or even have a bridge for that at all -
set the bit in _OSC, see what does guest enable.

Re: [Qemu-devel] [PATCH RFC 0/6] q35: add acpi pci hotplug support

[Alexander Bezzubikov]

Tried it on Win7 Enterprise SP1 - SHPC works well,  _OSC patches aren't
necessary (since pci-bridge has its own controller, I suppose).
On Linux guests it works when adding device from CLI with -device, but OS
seems to fail detecting the device when I add it with device_add from
monitor.
Also there're some issues with unplugging on Linux (haven't tested
unplugging on WIndows yet). That's the news.

2017-07-03 21:29 GMT+03:00 Michael S. Tsirkin <mst@redhat.com>:

> On Mon, Jul 03, 2017 at 09:26:33PM +0300, Marcel Apfelbaum wrote:
> > On 03/07/2017 19:34, Michael S. Tsirkin wrote:
> > > On Mon, Jul 03, 2017 at 02:27:11PM +0200, Igor Mammedov wrote:
> > > > On Fri, 30 Jun 2017 10:25:05 +0300
> > > > Marcel Apfelbaum <marcel@redhat.com> wrote:
> > > >
> > > > [...]
> > > > >
> > > > > So for the modern systems not supporting PCI ACPI hotplug
> > > > > we don't need pci-bridges anyway, but for the older ones
> > > > > the ACPI code of the pci-bridge will be loaded into the
> > > > > ACPI namespace only if a pci-bridge is actually hot-plugged.
> > > >
> > > > just note that the set of 'older' guest OSes is limited to
> > > > one that do not support SHPC (i.e. to EOLed WinXP & co)
> > > > as for linux and more modern Windows SHPC hotplug should
> > > > just work without our ACPI hack (which taxes low memory
> > > > to keep acpi tables for bridges).
> > > >
> > > > So I'm in favor of Michael's suggestion to leave ACPI PCI
> > > > only in PC machine for old WinXP guests and to keep Q35
> > > > clean, where linux or newer Windows guests could just
> > > > use standard SHPC.
> > > >
> > > > [...]
> > >
> > > I didn't realize windows actually supports SHPC for PCI.
> >
> > Me neither, if Igor is right I am all for shpc hotplug
> > since Q35 is not supposed to support older guests.
> >
> > I remember I succeeded to enable shpc hotplug some time
> > ago, but only for Linux guests.
> >
> > Igor, do you have some spec/doc on newer Windows OSes that confirm
> > PCI shpc hotplug support?
>
> Just try it, easier than poking at specs which aren't always up to date.
>
> > >
> > > Do they correctly set _OSC Arg3, bit offset 1?
> > >     SHPC Native Hot Plug control
> > >     The OS sets this bit to 1 to request control over PCI/PCI-X
> Standard Hot-Plug Controller
> > >     (SHPC) hot plug. If the OS successfully receives control of this
> feature, it must track and
> > >     update the status of hot plug slots and handle hot plug events as
> described in the SHPC
> > >     Specification.
> > > I was under impression they only set bit 0.
> > >
> >
> > Alexandr, if modern Windows OSes do support shpc, it makes our
> > job easier, can you please try to enable shpc hotplug?
> >
> > Thanks,
> > Marcel
>
> No need to enable or even have a bridge for that at all -
> set the bit in _OSC, see what does guest enable.
>



-- 
Alexander Bezzubikov

Re: [Qemu-devel] [PATCH RFC 0/6] q35: add acpi pci hotplug support

[Alexander Bezzubikov]

That is why I think we can consider a possibility of forgetting about ACPI
hot plug in pcie-pci bridge and use only SHPC (with some correcting work).
Especially since q35 is used only for 'modern' Windows guests and there're
no big problems with SHPC on Linux guests.

вт, 4 июля 2017 г. в 1:06, Alexander Bezzubikov <zuban32s@gmail.com>:

> Tried it on Win7 Enterprise SP1 - SHPC works well,  _OSC patches aren't
> necessary (since pci-bridge has its own controller, I suppose).
> On Linux guests it works when adding device from CLI with -device, but OS
> seems to fail detecting the device when I add it with device_add from
> monitor.
> Also there're some issues with unplugging on Linux (haven't tested
> unplugging on WIndows yet). That's the news.
>
> 2017-07-03 21:29 GMT+03:00 Michael S. Tsirkin <mst@redhat.com>:
>
>> On Mon, Jul 03, 2017 at 09:26:33PM +0300, Marcel Apfelbaum wrote:
>> > On 03/07/2017 19:34, Michael S. Tsirkin wrote:
>> > > On Mon, Jul 03, 2017 at 02:27:11PM +0200, Igor Mammedov wrote:
>> > > > On Fri, 30 Jun 2017 10:25:05 +0300
>> > > > Marcel Apfelbaum <marcel@redhat.com> wrote:
>> > > >
>> > > > [...]
>> > > > >
>> > > > > So for the modern systems not supporting PCI ACPI hotplug
>> > > > > we don't need pci-bridges anyway, but for the older ones
>> > > > > the ACPI code of the pci-bridge will be loaded into the
>> > > > > ACPI namespace only if a pci-bridge is actually hot-plugged.
>> > > >
>> > > > just note that the set of 'older' guest OSes is limited to
>> > > > one that do not support SHPC (i.e. to EOLed WinXP & co)
>> > > > as for linux and more modern Windows SHPC hotplug should
>> > > > just work without our ACPI hack (which taxes low memory
>> > > > to keep acpi tables for bridges).
>> > > >
>> > > > So I'm in favor of Michael's suggestion to leave ACPI PCI
>> > > > only in PC machine for old WinXP guests and to keep Q35
>> > > > clean, where linux or newer Windows guests could just
>> > > > use standard SHPC.
>> > > >
>> > > > [...]
>> > >
>> > > I didn't realize windows actually supports SHPC for PCI.
>> >
>> > Me neither, if Igor is right I am all for shpc hotplug
>> > since Q35 is not supposed to support older guests.
>> >
>> > I remember I succeeded to enable shpc hotplug some time
>> > ago, but only for Linux guests.
>> >
>> > Igor, do you have some spec/doc on newer Windows OSes that confirm
>> > PCI shpc hotplug support?
>>
>> Just try it, easier than poking at specs which aren't always up to date.
>>
>> > >
>> > > Do they correctly set _OSC Arg3, bit offset 1?
>> > >     SHPC Native Hot Plug control
>> > >     The OS sets this bit to 1 to request control over PCI/PCI-X
>> Standard Hot-Plug Controller
>> > >     (SHPC) hot plug. If the OS successfully receives control of this
>> feature, it must track and
>> > >     update the status of hot plug slots and handle hot plug events as
>> described in the SHPC
>> > >     Specification.
>> > > I was under impression they only set bit 0.
>> > >
>> >
>> > Alexandr, if modern Windows OSes do support shpc, it makes our
>> > job easier, can you please try to enable shpc hotplug?
>> >
>> > Thanks,
>> > Marcel
>>
>> No need to enable or even have a bridge for that at all -
>> set the bit in _OSC, see what does guest enable.
>>
>
>
>
> --
> Alexander Bezzubikov
>
-- 
Alexander Bezzubikov

Re: [Qemu-devel] [PATCH RFC 0/6] q35: add acpi pci hotplug support

[Marcel Apfelbaum]

On 04/07/2017 4:00, Alexander Bezzubikov wrote:
> That is why I think we can consider a possibility of forgetting about 
> ACPI hot plug in pcie-pci bridge and use only SHPC (with some correcting 
> work). Especially since q35 is used only for 'modern' Windows guests and 
> there're no big problems with SHPC on Linux guests.

Hi Alexandr,

Sure, SHPC is certainly enough, and Windows7+ guests support
is also enough.
Patches for a hot-pluggable pcie-pci bridge supporting SHPC based
hot-plug/hot-unplug are welcome :) .

One last thing, please avoid top-posting...

Thanks,
Marcel

> 
> вт, 4 июля 2017 г. в 1:06, Alexander Bezzubikov <zuban32s@gmail.com 
> <mailto:zuban32s@gmail.com>>:
> 
>     Tried it on Win7 Enterprise SP1 - SHPC works well,  _OSC patches
>     aren't necessary (since pci-bridge has its own controller, I suppose).
>     On Linux guests it works when adding device from CLI with -device,
>     but OS seems to fail detecting the device when I add it with
>     device_add from monitor.
>     Also there're some issues with unplugging on Linux (haven't tested
>     unplugging on WIndows yet). That's the news.
> 
>     2017-07-03 21:29 GMT+03:00 Michael S. Tsirkin <mst@redhat.com
>     <mailto:mst@redhat.com>>:
> 
>         On Mon, Jul 03, 2017 at 09:26:33PM +0300, Marcel Apfelbaum wrote:
>          > On 03/07/2017 19:34, Michael S. Tsirkin wrote:
>          > > On Mon, Jul 03, 2017 at 02:27:11PM +0200, Igor Mammedov wrote:
>          > > > On Fri, 30 Jun 2017 10:25:05 +0300
>          > > > Marcel Apfelbaum <marcel@redhat.com
>         <mailto:marcel@redhat.com>> wrote:
>          > > >
>          > > > [...]
>          > > > >
>          > > > > So for the modern systems not supporting PCI ACPI hotplug
>          > > > > we don't need pci-bridges anyway, but for the older ones
>          > > > > the ACPI code of the pci-bridge will be loaded into the
>          > > > > ACPI namespace only if a pci-bridge is actually
>         hot-plugged.
>          > > >
>          > > > just note that the set of 'older' guest OSes is limited to
>          > > > one that do not support SHPC (i.e. to EOLed WinXP & co)
>          > > > as for linux and more modern Windows SHPC hotplug should
>          > > > just work without our ACPI hack (which taxes low memory
>          > > > to keep acpi tables for bridges).
>          > > >
>          > > > So I'm in favor of Michael's suggestion to leave ACPI PCI
>          > > > only in PC machine for old WinXP guests and to keep Q35
>          > > > clean, where linux or newer Windows guests could just
>          > > > use standard SHPC.
>          > > >
>          > > > [...]
>          > >
>          > > I didn't realize windows actually supports SHPC for PCI.
>          >
>          > Me neither, if Igor is right I am all for shpc hotplug
>          > since Q35 is not supposed to support older guests.
>          >
>          > I remember I succeeded to enable shpc hotplug some time
>          > ago, but only for Linux guests.
>          >
>          > Igor, do you have some spec/doc on newer Windows OSes that
>         confirm
>          > PCI shpc hotplug support?
> 
>         Just try it, easier than poking at specs which aren't always up
>         to date.
> 
>         > >
>         > > Do they correctly set _OSC Arg3, bit offset 1?
>         > >     SHPC Native Hot Plug control
>         > >     The OS sets this bit to 1 to request control over PCI/PCI-X Standard Hot-Plug Controller
>         > >     (SHPC) hot plug. If the OS successfully receives control of this feature, it must track and
>         > >     update the status of hot plug slots and handle hot plug events as described in the SHPC
>         > >     Specification.
>         > > I was under impression they only set bit 0.
>         > >
>         >
>         > Alexandr, if modern Windows OSes do support shpc, it makes our
>         > job easier, can you please try to enable shpc hotplug?
>         >
>         > Thanks,
>         > Marcel
> 
>         No need to enable or even have a bridge for that at all -
>         set the bit in _OSC, see what does guest enable.
> 
> 
> 
> 
>     -- 
>     Alexander Bezzubikov
> 
> -- 
> Alexander Bezzubikov

Re: [Qemu-devel] [PATCH RFC 0/6] q35: add acpi pci hotplug support

[Igor Mammedov]

On Mon, 3 Jul 2017 19:34:07 +0300
"Michael S. Tsirkin" <mst@redhat.com> wrote:

> On Mon, Jul 03, 2017 at 02:27:11PM +0200, Igor Mammedov wrote:
> > On Fri, 30 Jun 2017 10:25:05 +0300
> > Marcel Apfelbaum <marcel@redhat.com> wrote:
> > 
> > [...]  
> > > 
> > > So for the modern systems not supporting PCI ACPI hotplug
> > > we don't need pci-bridges anyway, but for the older ones
> > > the ACPI code of the pci-bridge will be loaded into the
> > > ACPI namespace only if a pci-bridge is actually hot-plugged.  
> > 
> > just note that the set of 'older' guest OSes is limited to
> > one that do not support SHPC (i.e. to EOLed WinXP & co)
> > as for linux and more modern Windows SHPC hotplug should
> > just work without our ACPI hack (which taxes low memory
> > to keep acpi tables for bridges).
> > 
> > So I'm in favor of Michael's suggestion to leave ACPI PCI
> > only in PC machine for old WinXP guests and to keep Q35
> > clean, where linux or newer Windows guests could just
> > use standard SHPC.
> > 
> > [...]  
> 
> I didn't realize windows actually supports SHPC for PCI.
> 
> Do they correctly set _OSC Arg3, bit offset 1?
> 	SHPC Native Hot Plug control
> 	The OS sets this bit to 1 to request control over PCI/PCI-X Standard Hot-Plug Controller
> 	(SHPC) hot plug. If the OS successfully receives control of this feature, it must track and
> 	update the status of hot plug slots and handle hot plug events as described in the SHPC
> 	Specification.
> I was under impression they only set bit 0.

Alexander,

wrt windows it might be worth to keep in mind
  https://msdn.microsoft.com/en-us/library/windows/hardware/dn631753(v=vs.85).aspx

as for QEMU,
one should remove masking SHPC support in QEMU ACPI part
 build_q35_osc_method()

hw switch to SHPC method happens for bridges in shpc_init() see qbus_set_hotplug_handler()