[Qemu-devel] [PATCH 00/13] acpi: Make piix-specific and q35-specific code generic

[Qemu-devel] [PATCH 00/13] acpi: Make piix-specific and q35-specific code generic

[Eduardo Habkost]

This series removes piix-specific and q35-specific code from
acpi-build.c, making it generic and without direct dependencies
to piix and q35 code.

This series needs to be applied after the following:
* [PATCH v3 0/6] pc: Initialization and compat function cleanup
* [PATCH V3 0/3] hw/pcie: Multi-root support for Q35
* [PATCH 00/16] pc: Eliminate struct PcGuestInfo

For reference, there's a git tree containing this series plus all
the dependencies, at:
  git://github.com/ehabkost/qemu-hacks.git work/acpi-decouple

Eduardo Habkost (13):
  pc: Add PCMachineState::pci_host field
  acpi: Remove unnecessary check for NULL pci_host
  acpi: Eliminate acpi_get_i386_pci_host() function
  acpi: Move DSDT info to PCMachineClass
  acpi: Simplify s3/s4 property querying
  acpi: Use &error_abort when getting PCI hotplug properties
  acpi: Use QOM property to get CPU hotplug I/O base
  acpi: Always try to init PCI hotplug I/O base
  acpi: Use PCMachineState::acpi_dev to get ACPI dev
  acpi: Change acpi_pci_hotplug_enabled() argument to PCMachineState
  acpi: Don't use find_i440fx() when setting bsel properties
  intel_iommu.h: Missing sysbus.h include
  acpi: Don't include q35 and piix headers

 hw/acpi/cpu_hotplug.c         |   3 +
 hw/acpi/ich9.c                |   2 +
 hw/acpi/piix4.c               |   2 +
 hw/i386/acpi-build.c          | 161 +++++++++++++-----------------------------
 hw/i386/pc_piix.c             |   4 ++
 hw/i386/pc_q35.c              |   6 ++
 hw/pci-host/piix.c            |   1 +
 include/hw/acpi/cpu_hotplug.h |   1 +
 include/hw/acpi/pc-hotplug.h  |   4 +-
 include/hw/i386/intel_iommu.h |   1 +
 include/hw/i386/pc.h          |   3 +
 11 files changed, 74 insertions(+), 114 deletions(-)

-- 
2.1.0

[Qemu-devel] [PATCH 01/13] pc: Add PCMachineState::pci_host field

[Eduardo Habkost]

This will allow us to avoid direct references to piix and q35 in
acpi-build.c.

Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
---
 hw/i386/pc_q35.c     | 2 ++
 hw/pci-host/piix.c   | 1 +
 include/hw/i386/pc.h | 1 +
 3 files changed, 4 insertions(+)

diff --git a/hw/i386/pc_q35.c b/hw/i386/pc_q35.c
index 0907746..317d36a 100644
--- a/hw/i386/pc_q35.c
+++ b/hw/i386/pc_q35.c
@@ -171,6 +171,8 @@ static void pc_q35_init(MachineState *machine)
     phb = PCI_HOST_BRIDGE(q35_host);
     host_bus = phb->bus;
     pcms->bus = phb->bus;
+    pcms->pci_host = phb;
+
     /* create ISA bus */
     lpc = pci_create_simple_multifunction(host_bus, PCI_DEVFN(ICH9_LPC_DEV,
                                           ICH9_LPC_FUNC), true,
diff --git a/hw/pci-host/piix.c b/hw/pci-host/piix.c
index 715208b..7711e27 100644
--- a/hw/pci-host/piix.c
+++ b/hw/pci-host/piix.c
@@ -335,6 +335,7 @@ PCIBus *i440fx_init(const char *host_type, const char *pci_type,
                     address_space_io, 0, TYPE_PCI_BUS);
     s->bus = b;
     object_property_add_child(qdev_get_machine(), "i440fx", OBJECT(dev), NULL);
+    PC_MACHINE(qdev_get_machine())->pci_host = s;
     qdev_init_nofail(dev);
 
     d = pci_create_simple(b, 0, pci_type);
diff --git a/include/hw/i386/pc.h b/include/hw/i386/pc.h
index 6ff4721..8b184c1 100644
--- a/include/hw/i386/pc.h
+++ b/include/hw/i386/pc.h
@@ -38,6 +38,7 @@ struct PCMachineState {
     OnOffAuto vmport;
     OnOffAuto smm;
     ram_addr_t below_4g_mem_size, above_4g_mem_size;
+    PCIHostState *pci_host;
     PCIBus *bus;
     Notifier machine_done;
     FWCfgState *fw_cfg;
-- 
2.1.0

Re: [Qemu-devel] [PATCH 01/13] pc: Add PCMachineState::pci_host field

[Marcel Apfelbaum]

On 12/03/2015 12:22 AM, Eduardo Habkost wrote:
> This will allow us to avoid direct references to piix and q35 in
> acpi-build.c.
>
> Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
> ---
>   hw/i386/pc_q35.c     | 2 ++
>   hw/pci-host/piix.c   | 1 +
>   include/hw/i386/pc.h | 1 +
>   3 files changed, 4 insertions(+)
>
> diff --git a/hw/i386/pc_q35.c b/hw/i386/pc_q35.c
> index 0907746..317d36a 100644
> --- a/hw/i386/pc_q35.c
> +++ b/hw/i386/pc_q35.c
> @@ -171,6 +171,8 @@ static void pc_q35_init(MachineState *machine)
>       phb = PCI_HOST_BRIDGE(q35_host);
>       host_bus = phb->bus;
>       pcms->bus = phb->bus;
> +    pcms->pci_host = phb;
> +
>       /* create ISA bus */
>       lpc = pci_create_simple_multifunction(host_bus, PCI_DEVFN(ICH9_LPC_DEV,
>                                             ICH9_LPC_FUNC), true,
> diff --git a/hw/pci-host/piix.c b/hw/pci-host/piix.c
> index 715208b..7711e27 100644
> --- a/hw/pci-host/piix.c
> +++ b/hw/pci-host/piix.c
> @@ -335,6 +335,7 @@ PCIBus *i440fx_init(const char *host_type, const char *pci_type,
>                       address_space_io, 0, TYPE_PCI_BUS);
>       s->bus = b;
>       object_property_add_child(qdev_get_machine(), "i440fx", OBJECT(dev), NULL);
> +    PC_MACHINE(qdev_get_machine())->pci_host = s;
>       qdev_init_nofail(dev);
>
>       d = pci_create_simple(b, 0, pci_type);
> diff --git a/include/hw/i386/pc.h b/include/hw/i386/pc.h
> index 6ff4721..8b184c1 100644
> --- a/include/hw/i386/pc.h
> +++ b/include/hw/i386/pc.h
> @@ -38,6 +38,7 @@ struct PCMachineState {
>       OnOffAuto vmport;
>       OnOffAuto smm;
>       ram_addr_t below_4g_mem_size, above_4g_mem_size;
> +    PCIHostState *pci_host;

Having both pci_host and bus as class fields is not needed.
If you have the host, maybe you can get rid of the bus.

Thanks,
Marcel

>       PCIBus *bus;
>       Notifier machine_done;
>       FWCfgState *fw_cfg;
>

Re: [Qemu-devel] [PATCH 01/13] pc: Add PCMachineState::pci_host field

[Eduardo Habkost]

On Thu, Dec 03, 2015 at 05:35:03PM +0200, Marcel Apfelbaum wrote:
> On 12/03/2015 12:22 AM, Eduardo Habkost wrote:
> >This will allow us to avoid direct references to piix and q35 in
> >acpi-build.c.
> >
> >Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
[...]
> >diff --git a/include/hw/i386/pc.h b/include/hw/i386/pc.h
> >index 6ff4721..8b184c1 100644
> >--- a/include/hw/i386/pc.h
> >+++ b/include/hw/i386/pc.h
> >@@ -38,6 +38,7 @@ struct PCMachineState {
> >      OnOffAuto vmport;
> >      OnOffAuto smm;
> >      ram_addr_t below_4g_mem_size, above_4g_mem_size;
> >+    PCIHostState *pci_host;
> 
> Having both pci_host and bus as class fields is not needed.
> If you have the host, maybe you can get rid of the bus.

Yes, I will send that as folllow-up.

-- 
Eduardo

[Qemu-devel] [PATCH 02/13] acpi: Remove unnecessary check for NULL pci_host

[Eduardo Habkost]

If acpi_get_i386_pci_host() returned NULL, acpi_get_pci_info()
would crash before build_ssdt() gets called. Remove unnecessary
check for NULL pci_host.

Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
---
 hw/i386/acpi-build.c | 8 +-------
 1 file changed, 1 insertion(+), 7 deletions(-)

diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
index 3c7af74..20a7066 100644
--- a/hw/i386/acpi-build.c
+++ b/hw/i386/acpi-build.c
@@ -1338,13 +1338,7 @@ build_ssdt(GArray *table_data, GArray *linker,
         aml_append(sb_scope, method);
 
         {
-            Object *pci_host;
-            PCIBus *bus = NULL;
-
-            pci_host = acpi_get_i386_pci_host();
-            if (pci_host) {
-                bus = PCI_HOST_BRIDGE(pci_host)->bus;
-            }
+            PCIBus *bus = PCI_HOST_BRIDGE(acpi_get_i386_pci_host())->bus;
 
             if (bus) {
                 Aml *scope = aml_scope("PCI0");
-- 
2.1.0

[Qemu-devel] [PATCH 03/13] acpi: Eliminate acpi_get_i386_pci_host() function

[Eduardo Habkost]

We can simply use PCMachineState::pci_host directly.

Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
---
 hw/i386/acpi-build.c | 38 +++++++-------------------------------
 1 file changed, 7 insertions(+), 31 deletions(-)

diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
index 20a7066..a595575 100644
--- a/hw/i386/acpi-build.c
+++ b/hw/i386/acpi-build.c
@@ -241,32 +241,9 @@ static void acpi_get_misc_info(AcpiMiscInfo *info)
     info->applesmc_io_base = applesmc_port();
 }
 
-/*
- * 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)
+static void acpi_get_pci_info(PcPciInfo *info, PCMachineState *pcms)
 {
-    PCIHostState *host;
-
-    host = OBJECT_CHECK(PCIHostState,
-                        object_resolve_path("/machine/i440fx", NULL),
-                        TYPE_PCI_HOST_BRIDGE);
-    if (!host) {
-        host = OBJECT_CHECK(PCIHostState,
-                            object_resolve_path("/machine/q35", NULL),
-                            TYPE_PCI_HOST_BRIDGE);
-    }
-
-    return OBJECT(host);
-}
-
-static void acpi_get_pci_info(PcPciInfo *info)
-{
-    Object *pci_host;
-
-
-    pci_host = acpi_get_i386_pci_host();
+    Object *pci_host = OBJECT(pcms->pci_host);
     g_assert(pci_host);
 
     info->w32.begin = object_property_get_int(pci_host,
@@ -1338,7 +1315,7 @@ build_ssdt(GArray *table_data, GArray *linker,
         aml_append(sb_scope, method);
 
         {
-            PCIBus *bus = PCI_HOST_BRIDGE(acpi_get_i386_pci_host())->bus;
+            PCIBus *bus = pcms->pci_host->bus;
 
             if (bus) {
                 Aml *scope = aml_scope("PCI0");
@@ -1643,12 +1620,11 @@ struct AcpiBuildState {
     MemoryRegion *linker_mr;
 } AcpiBuildState;
 
-static bool acpi_get_mcfg(AcpiMcfgInfo *mcfg)
+static bool acpi_get_mcfg(AcpiMcfgInfo *mcfg, PCMachineState *pcms)
 {
-    Object *pci_host;
+    Object *pci_host = OBJECT(pcms->pci_host);
     QObject *o;
 
-    pci_host = acpi_get_i386_pci_host();
     g_assert(pci_host);
 
     o = object_property_get_qobject(pci_host, PCIE_HOST_MCFG_BASE, NULL);
@@ -1694,7 +1670,7 @@ void acpi_build(PCMachineState *pcms, AcpiBuildTables *tables)
     acpi_get_pm_info(&pm);
     acpi_get_dsdt(&misc);
     acpi_get_misc_info(&misc);
-    acpi_get_pci_info(&pci);
+    acpi_get_pci_info(&pci, pcms);
 
     table_offsets = g_array_new(false, true /* clear */,
                                         sizeof(uint32_t));
@@ -1751,7 +1727,7 @@ void acpi_build(PCMachineState *pcms, AcpiBuildTables *tables)
         acpi_add_table(table_offsets, tables_blob);
         build_srat(tables_blob, tables->linker, pcms);
     }
-    if (acpi_get_mcfg(&mcfg)) {
+    if (acpi_get_mcfg(&mcfg, pcms)) {
         acpi_add_table(table_offsets, tables_blob);
         build_mcfg_q35(tables_blob, tables->linker, &mcfg);
     }
-- 
2.1.0

[Qemu-devel] [PATCH 04/13] acpi: Move DSDT info to PCMachineClass

[Eduardo Habkost]

Remove direct dependency on q35-acpi-dsdt.hex and acpi-dsdt.hex
from acpi-build.c.

Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
---
 hw/i386/acpi-build.c | 35 +++++++----------------------------
 hw/i386/pc_piix.c    |  4 ++++
 hw/i386/pc_q35.c     |  4 ++++
 include/hw/i386/pc.h |  2 ++
 4 files changed, 17 insertions(+), 28 deletions(-)

diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
index a595575..4677e97 100644
--- a/hw/i386/acpi-build.c
+++ b/hw/i386/acpi-build.c
@@ -51,9 +51,6 @@
 #include "hw/pci-host/q35.h"
 #include "hw/i386/intel_iommu.h"
 
-#include "hw/i386/q35-acpi-dsdt.hex"
-#include "hw/i386/acpi-dsdt.hex"
-
 #include "hw/acpi/aml-build.h"
 
 #include "qapi/qmp/qint.h"
@@ -108,8 +105,6 @@ typedef struct AcpiPmInfo {
 typedef struct AcpiMiscInfo {
     bool has_hpet;
     TPMVersion tpm_version;
-    const unsigned char *dsdt_code;
-    unsigned dsdt_size;
     uint16_t pvpanic_port;
     uint16_t applesmc_io_base;
 } AcpiMiscInfo;
@@ -121,22 +116,6 @@ typedef struct AcpiBuildPciBusHotplugState {
     bool pcihp_bridge_en;
 } AcpiBuildPciBusHotplugState;
 
-static void acpi_get_dsdt(AcpiMiscInfo *info)
-{
-    Object *piix = piix4_pm_find();
-    Object *lpc = ich9_lpc_find();
-    assert(!!piix != !!lpc);
-
-    if (piix) {
-        info->dsdt_code = AcpiDsdtAmlCode;
-        info->dsdt_size = sizeof AcpiDsdtAmlCode;
-    }
-    if (lpc) {
-        info->dsdt_code = Q35AcpiDsdtAmlCode;
-        info->dsdt_size = sizeof Q35AcpiDsdtAmlCode;
-    }
-}
-
 static
 int acpi_add_cpu_info(Object *o, void *opaque)
 {
@@ -1570,18 +1549,19 @@ build_dmar_q35(GArray *table_data, GArray *linker)
 }
 
 static void
-build_dsdt(GArray *table_data, GArray *linker, AcpiMiscInfo *misc)
+build_dsdt(GArray *table_data, GArray *linker, PCMachineState *pcms)
 {
+    PCMachineClass *pcmc = PC_MACHINE_GET_CLASS(pcms);
     AcpiTableHeader *dsdt;
 
-    assert(misc->dsdt_code && misc->dsdt_size);
+    assert(pcmc->dsdt_code && pcmc->dsdt_size);
 
-    dsdt = acpi_data_push(table_data, misc->dsdt_size);
-    memcpy(dsdt, misc->dsdt_code, misc->dsdt_size);
+    dsdt = acpi_data_push(table_data, pcmc->dsdt_size);
+    memcpy(dsdt, pcmc->dsdt_code, pcmc->dsdt_size);
 
     memset(dsdt, 0, sizeof *dsdt);
     build_header(linker, table_data, dsdt, "DSDT",
-                 misc->dsdt_size, 1);
+                 pcmc->dsdt_size, 1);
 }
 
 static GArray *
@@ -1668,7 +1648,6 @@ void acpi_build(PCMachineState *pcms, AcpiBuildTables *tables)
 
     acpi_get_cpu_info(&cpu);
     acpi_get_pm_info(&pm);
-    acpi_get_dsdt(&misc);
     acpi_get_misc_info(&misc);
     acpi_get_pci_info(&pci, pcms);
 
@@ -1690,7 +1669,7 @@ void acpi_build(PCMachineState *pcms, AcpiBuildTables *tables)
 
     /* DSDT is pointed to by FADT */
     dsdt = tables_blob->len;
-    build_dsdt(tables_blob, tables->linker, &misc);
+    build_dsdt(tables_blob, tables->linker, pcms);
 
     /* Count the size of the DSDT and SSDT, we will need it for legacy
      * sizing of ACPI tables.
diff --git a/hw/i386/pc_piix.c b/hw/i386/pc_piix.c
index f0c2dc8..f4e03b5 100644
--- a/hw/i386/pc_piix.c
+++ b/hw/i386/pc_piix.c
@@ -53,6 +53,7 @@
 #include "hw/xen/xen_pt.h"
 #endif
 #include "migration/migration.h"
+#include "hw/i386/acpi-dsdt.hex"
 
 #define MAX_IDE_BUS 2
 
@@ -402,11 +403,14 @@ static void pc_xen_hvm_init(MachineState *machine)
 
 static void pc_i440fx_machine_options(MachineClass *m)
 {
+    PCMachineClass *pcmc = PC_MACHINE_CLASS(m);
     m->family = "pc_piix";
     m->desc = "Standard PC (i440FX + PIIX, 1996)";
     m->hot_add_cpu = pc_hot_add_cpu;
     m->default_machine_opts = "firmware=bios-256k.bin";
     m->default_display = "std";
+    pcmc->dsdt_code = AcpiDsdtAmlCode;
+    pcmc->dsdt_size = sizeof AcpiDsdtAmlCode;
 }
 
 static void pc_i440fx_2_5_machine_options(MachineClass *m)
diff --git a/hw/i386/pc_q35.c b/hw/i386/pc_q35.c
index 317d36a..37d4e8c 100644
--- a/hw/i386/pc_q35.c
+++ b/hw/i386/pc_q35.c
@@ -45,6 +45,7 @@
 #include "hw/usb.h"
 #include "qemu/error-report.h"
 #include "migration/migration.h"
+#include "hw/i386/q35-acpi-dsdt.hex"
 
 /* ICH9 AHCI has 6 ports */
 #define MAX_SATA_PORTS     6
@@ -328,6 +329,7 @@ static void pc_compat_1_4(MachineState *machine)
 
 static void pc_q35_machine_options(MachineClass *m)
 {
+    PCMachineClass *pcmc = PC_MACHINE_CLASS(m);
     m->family = "pc_q35";
     m->desc = "Standard PC (Q35 + ICH9, 2009)";
     m->hot_add_cpu = pc_hot_add_cpu;
@@ -336,6 +338,8 @@ static void pc_q35_machine_options(MachineClass *m)
     m->default_display = "std";
     m->no_floppy = 1;
     m->no_tco = 0;
+    pcmc->dsdt_code = Q35AcpiDsdtAmlCode;
+    pcmc->dsdt_size = sizeof Q35AcpiDsdtAmlCode;
 }
 
 static void pc_q35_2_5_machine_options(MachineClass *m)
diff --git a/include/hw/i386/pc.h b/include/hw/i386/pc.h
index 8b184c1..e09fb98 100644
--- a/include/hw/i386/pc.h
+++ b/include/hw/i386/pc.h
@@ -89,6 +89,8 @@ struct PCMachineClass {
      */
     unsigned acpi_data_size;
     bool enforce_aligned_dimm;
+    const unsigned char *dsdt_code;
+    unsigned dsdt_size;
 };
 
 #define TYPE_PC_MACHINE "generic-pc-machine"
-- 
2.1.0

[Qemu-devel] [PATCH 05/13] acpi: Simplify s3/s4 property querying

[Eduardo Habkost]

object_property_get_bool() already returns false when the
property is not present, there's no need to call
object_property_get_qobject() manually.

Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
---
 hw/i386/acpi-build.c | 32 +++++++++-----------------------
 1 file changed, 9 insertions(+), 23 deletions(-)

diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
index 4677e97..9a45a59 100644
--- a/hw/i386/acpi-build.c
+++ b/hw/i386/acpi-build.c
@@ -146,7 +146,6 @@ static void acpi_get_pm_info(AcpiPmInfo *pm)
     Object *piix = piix4_pm_find();
     Object *lpc = ich9_lpc_find();
     Object *obj = NULL;
-    QObject *o;
 
     pm->cpu_hp_io_base = 0;
     pm->pcihp_io_base = 0;
@@ -169,28 +168,15 @@ static void acpi_get_pm_info(AcpiPmInfo *pm)
     pm->mem_hp_io_base = ACPI_MEMORY_HOTPLUG_BASE;
     pm->mem_hp_io_len = ACPI_MEMORY_HOTPLUG_IO_LEN;
 
-    /* Fill in optional s3/s4 related properties */
-    o = object_property_get_qobject(obj, ACPI_PM_PROP_S3_DISABLED, NULL);
-    if (o) {
-        pm->s3_disabled = qint_get_int(qobject_to_qint(o));
-    } else {
-        pm->s3_disabled = false;
-    }
-    qobject_decref(o);
-    o = object_property_get_qobject(obj, ACPI_PM_PROP_S4_DISABLED, NULL);
-    if (o) {
-        pm->s4_disabled = qint_get_int(qobject_to_qint(o));
-    } else {
-        pm->s4_disabled = false;
-    }
-    qobject_decref(o);
-    o = object_property_get_qobject(obj, ACPI_PM_PROP_S4_VAL, NULL);
-    if (o) {
-        pm->s4_val = qint_get_int(qobject_to_qint(o));
-    } else {
-        pm->s4_val = false;
-    }
-    qobject_decref(o);
+    /* Fill in optional s3/s4 related properties
+     * (default to false if property is not present)
+     */
+    pm->s3_disabled =
+        object_property_get_bool(obj, ACPI_PM_PROP_S3_DISABLED, NULL);
+    pm->s4_disabled =
+        object_property_get_bool(obj, ACPI_PM_PROP_S4_DISABLED, NULL);
+    pm->s4_val =
+        object_property_get_bool(obj, ACPI_PM_PROP_S4_VAL, NULL);
 
     /* Fill in mandatory properties */
     pm->sci_int = object_property_get_int(obj, ACPI_PM_PROP_SCI_INT, NULL);
-- 
2.1.0

[Qemu-devel] [PATCH 06/13] acpi: Use &error_abort when getting PCI hotplug properties

[Eduardo Habkost]

The properties must be present, so use &error_abort to ensure we
don't ignore errors.

Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
---
 hw/i386/acpi-build.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
index 9a45a59..4f42e28 100644
--- a/hw/i386/acpi-build.c
+++ b/hw/i386/acpi-build.c
@@ -154,9 +154,11 @@ static void acpi_get_pm_info(AcpiPmInfo *pm)
         obj = piix;
         pm->cpu_hp_io_base = PIIX4_CPU_HOTPLUG_IO_BASE;
         pm->pcihp_io_base =
-            object_property_get_int(obj, ACPI_PCIHP_IO_BASE_PROP, NULL);
+            object_property_get_int(obj, ACPI_PCIHP_IO_BASE_PROP,
+                                    &error_abort);
         pm->pcihp_io_len =
-            object_property_get_int(obj, ACPI_PCIHP_IO_LEN_PROP, NULL);
+            object_property_get_int(obj, ACPI_PCIHP_IO_LEN_PROP,
+                                    &error_abort);
     }
     if (lpc) {
         obj = lpc;
-- 
2.1.0

[Qemu-devel] [PATCH 07/13] acpi: Use QOM property to get CPU hotplug I/O base

[Eduardo Habkost]

Instead of hardcoding the ich9 and piix I/O bases, use a QOM
property, the same way we already do for PCI hotplug I/O base.

Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
---
 hw/acpi/cpu_hotplug.c         | 3 +++
 hw/acpi/ich9.c                | 2 ++
 hw/acpi/piix4.c               | 2 ++
 hw/i386/acpi-build.c          | 5 ++---
 include/hw/acpi/cpu_hotplug.h | 1 +
 include/hw/acpi/pc-hotplug.h  | 4 ++--
 6 files changed, 12 insertions(+), 5 deletions(-)

diff --git a/hw/acpi/cpu_hotplug.c b/hw/acpi/cpu_hotplug.c
index f5b9972..6d4d7c1 100644
--- a/hw/acpi/cpu_hotplug.c
+++ b/hw/acpi/cpu_hotplug.c
@@ -73,4 +73,7 @@ void acpi_cpu_hotplug_init(MemoryRegion *parent, Object *owner,
     memory_region_init_io(&gpe_cpu->io, owner, &AcpiCpuHotplug_ops,
                           gpe_cpu, "acpi-cpu-hotplug", ACPI_GPE_PROC_LEN);
     memory_region_add_subregion(parent, base, &gpe_cpu->io);
+    gpe_cpu->base = base;
+    object_property_add_uint16_ptr(owner, ACPI_CPUHP_IO_BASE_PROP,
+                                   &gpe_cpu->base, &error_abort);
 }
diff --git a/hw/acpi/ich9.c b/hw/acpi/ich9.c
index 1c7fcfa..aa30c3e 100644
--- a/hw/acpi/ich9.c
+++ b/hw/acpi/ich9.c
@@ -46,6 +46,8 @@ do { printf("%s "fmt, __func__, ## __VA_ARGS__); } while (0)
 #define ICH9_DEBUG(fmt, ...)    do { } while (0)
 #endif
 
+#define ICH9_CPU_HOTPLUG_IO_BASE 0x0CD8
+
 static void ich9_pm_update_sci_fn(ACPIREGS *regs)
 {
     ICH9LPCPMRegs *pm = container_of(regs, ICH9LPCPMRegs, acpi_regs);
diff --git a/hw/acpi/piix4.c b/hw/acpi/piix4.c
index 2cd2fee..e4393fd 100644
--- a/hw/acpi/piix4.c
+++ b/hw/acpi/piix4.c
@@ -46,6 +46,8 @@
 # define PIIX4_DPRINTF(format, ...)     do { } while (0)
 #endif
 
+#define PIIX4_CPU_HOTPLUG_IO_BASE 0xaf00
+
 #define GPE_BASE 0xafe0
 #define GPE_LEN 4
 
diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
index 4f42e28..6774ced 100644
--- a/hw/i386/acpi-build.c
+++ b/hw/i386/acpi-build.c
@@ -147,12 +147,10 @@ static void acpi_get_pm_info(AcpiPmInfo *pm)
     Object *lpc = ich9_lpc_find();
     Object *obj = NULL;
 
-    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 =
             object_property_get_int(obj, ACPI_PCIHP_IO_BASE_PROP,
                                     &error_abort);
@@ -162,10 +160,11 @@ static void acpi_get_pm_info(AcpiPmInfo *pm)
     }
     if (lpc) {
         obj = lpc;
-        pm->cpu_hp_io_base = ICH9_CPU_HOTPLUG_IO_BASE;
     }
     assert(obj);
 
+    pm->cpu_hp_io_base =
+        object_property_get_int(obj, ACPI_CPUHP_IO_BASE_PROP, &error_abort);
     pm->cpu_hp_io_len = ACPI_GPE_PROC_LEN;
     pm->mem_hp_io_base = ACPI_MEMORY_HOTPLUG_BASE;
     pm->mem_hp_io_len = ACPI_MEMORY_HOTPLUG_IO_LEN;
diff --git a/include/hw/acpi/cpu_hotplug.h b/include/hw/acpi/cpu_hotplug.h
index f6d358d..0e1dd99 100644
--- a/include/hw/acpi/cpu_hotplug.h
+++ b/include/hw/acpi/cpu_hotplug.h
@@ -16,6 +16,7 @@
 #include "hw/acpi/pc-hotplug.h"
 
 typedef struct AcpiCpuHotplug {
+    uint16_t base;
     MemoryRegion io;
     uint8_t sts[ACPI_GPE_PROC_LEN];
 } AcpiCpuHotplug;
diff --git a/include/hw/acpi/pc-hotplug.h b/include/hw/acpi/pc-hotplug.h
index 77b1569..f4b006c 100644
--- a/include/hw/acpi/pc-hotplug.h
+++ b/include/hw/acpi/pc-hotplug.h
@@ -25,8 +25,8 @@
 /* 256 CPU IDs, 8 bits per entry: */
 #define ACPI_GPE_PROC_LEN 32
 
-#define ICH9_CPU_HOTPLUG_IO_BASE 0x0CD8
-#define PIIX4_CPU_HOTPLUG_IO_BASE 0xaf00
+#define ACPI_CPUHP_IO_BASE_PROP "acpi-cpuhp-io-base"
+
 #define CPU_HOTPLUG_RESOURCE_DEVICE PRES
 
 #define ACPI_MEMORY_HOTPLUG_IO_LEN 24
-- 
2.1.0

[Qemu-devel] [PATCH 08/13] acpi: Always try to init PCI hotplug I/O base

[Eduardo Habkost]

Instead of making pcihp_io_base/pcihp_io_len initialization
specific to piix4, just check if ACPI_PCIHP_IO_BASE_PROP property
is present.

No behavior is changed, as only piix4 initializes the
ACPI_PCIHP_IO_BASE_PROP property today.

Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
---
 hw/i386/acpi-build.c | 16 ++++++++++++----
 1 file changed, 12 insertions(+), 4 deletions(-)

diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
index 6774ced..617cd53 100644
--- a/hw/i386/acpi-build.c
+++ b/hw/i386/acpi-build.c
@@ -141,6 +141,11 @@ static void acpi_get_cpu_info(AcpiCpuInfo *cpu)
     object_child_foreach(root, acpi_add_cpu_info, cpu);
 }
 
+static bool acpi_pci_hotplug_enabled(Object *acpi_dev)
+{
+    return object_property_find(acpi_dev, ACPI_PCIHP_IO_BASE_PROP, NULL);
+}
+
 static void acpi_get_pm_info(AcpiPmInfo *pm)
 {
     Object *piix = piix4_pm_find();
@@ -151,6 +156,13 @@ static void acpi_get_pm_info(AcpiPmInfo *pm)
     pm->pcihp_io_len = 0;
     if (piix) {
         obj = piix;
+    }
+    if (lpc) {
+        obj = lpc;
+    }
+    assert(obj);
+
+    if (acpi_pci_hotplug_enabled(obj)) {
         pm->pcihp_io_base =
             object_property_get_int(obj, ACPI_PCIHP_IO_BASE_PROP,
                                     &error_abort);
@@ -158,10 +170,6 @@ static void acpi_get_pm_info(AcpiPmInfo *pm)
             object_property_get_int(obj, ACPI_PCIHP_IO_LEN_PROP,
                                     &error_abort);
     }
-    if (lpc) {
-        obj = lpc;
-    }
-    assert(obj);
 
     pm->cpu_hp_io_base =
         object_property_get_int(obj, ACPI_CPUHP_IO_BASE_PROP, &error_abort);
-- 
2.1.0

[Qemu-devel] [PATCH 09/13] acpi: Use PCMachineState::acpi_dev to get ACPI dev

[Eduardo Habkost]

Instead of calling piix4_pm_find() and ich9_lpc_find(), simply
use PCMachineState::acpi_dev to get the object containing the
ACPI-related properties.

Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
---
 hw/i386/acpi-build.c | 16 ++++------------
 1 file changed, 4 insertions(+), 12 deletions(-)

diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
index 617cd53..440d4e2 100644
--- a/hw/i386/acpi-build.c
+++ b/hw/i386/acpi-build.c
@@ -146,21 +146,13 @@ static bool acpi_pci_hotplug_enabled(Object *acpi_dev)
     return object_property_find(acpi_dev, ACPI_PCIHP_IO_BASE_PROP, NULL);
 }
 
-static void acpi_get_pm_info(AcpiPmInfo *pm)
+static void acpi_get_pm_info(AcpiPmInfo *pm, PCMachineState *pcms)
 {
-    Object *piix = piix4_pm_find();
-    Object *lpc = ich9_lpc_find();
-    Object *obj = NULL;
+    Object *obj = OBJECT(pcms->acpi_dev);
 
+    assert(obj);
     pm->pcihp_io_base = 0;
     pm->pcihp_io_len = 0;
-    if (piix) {
-        obj = piix;
-    }
-    if (lpc) {
-        obj = lpc;
-    }
-    assert(obj);
 
     if (acpi_pci_hotplug_enabled(obj)) {
         pm->pcihp_io_base =
@@ -1642,7 +1634,7 @@ void acpi_build(PCMachineState *pcms, AcpiBuildTables *tables)
     GArray *tables_blob = tables->table_data;
 
     acpi_get_cpu_info(&cpu);
-    acpi_get_pm_info(&pm);
+    acpi_get_pm_info(&pm, pcms);
     acpi_get_misc_info(&misc);
     acpi_get_pci_info(&pci, pcms);
 
-- 
2.1.0

[Qemu-devel] [PATCH 10/13] acpi: Change acpi_pci_hotplug_enabled() argument to PCMachineState

[Eduardo Habkost]

Now that we just use PCMachineState::acpi_dev, we can change the
argument to acpi_pci_hotplug_enabled().

Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
---
 hw/i386/acpi-build.c | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
index 440d4e2..585100f 100644
--- a/hw/i386/acpi-build.c
+++ b/hw/i386/acpi-build.c
@@ -141,9 +141,10 @@ static void acpi_get_cpu_info(AcpiCpuInfo *cpu)
     object_child_foreach(root, acpi_add_cpu_info, cpu);
 }
 
-static bool acpi_pci_hotplug_enabled(Object *acpi_dev)
+static bool acpi_pci_hotplug_enabled(PCMachineState *pcms)
 {
-    return object_property_find(acpi_dev, ACPI_PCIHP_IO_BASE_PROP, NULL);
+    return object_property_find(OBJECT(pcms->acpi_dev),
+                                ACPI_PCIHP_IO_BASE_PROP, NULL);
 }
 
 static void acpi_get_pm_info(AcpiPmInfo *pm, PCMachineState *pcms)
@@ -154,7 +155,7 @@ static void acpi_get_pm_info(AcpiPmInfo *pm, PCMachineState *pcms)
     pm->pcihp_io_base = 0;
     pm->pcihp_io_len = 0;
 
-    if (acpi_pci_hotplug_enabled(obj)) {
+    if (acpi_pci_hotplug_enabled(pcms)) {
         pm->pcihp_io_base =
             object_property_get_int(obj, ACPI_PCIHP_IO_BASE_PROP,
                                     &error_abort);
-- 
2.1.0

[Qemu-devel] [PATCH 11/13] acpi: Don't use find_i440fx() when setting bsel properties

[Eduardo Habkost]

Instead of checking for i440fx, the code can simply check if the
machine has ACPI PCI hotplug enabled.

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

diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
index 585100f..048967e 100644
--- a/hw/i386/acpi-build.c
+++ b/hw/i386/acpi-build.c
@@ -397,12 +397,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(PCMachineState *pcms)
 {
-    PCIBus *bus = find_i440fx(); /* TODO: Q35 support */
+    PCIBus *bus = pcms->bus;
     unsigned bsel_alloc = 0;
 
-    if (bus) {
+    if (acpi_pci_hotplug_enabled(pcms) && 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);
     }
@@ -1855,7 +1855,7 @@ void acpi_setup(PCMachineState *pcms)
 
     build_state->pcms = pcms;
 
-    acpi_set_pci_info();
+    acpi_set_pci_info(pcms);
 
     acpi_build_tables_init(&tables);
     acpi_build(build_state->pcms, &tables);
-- 
2.1.0

[Qemu-devel] [PATCH 12/13] intel_iommu.h: Missing sysbus.h include

[Eduardo Habkost]

intel_iommu.h uses SysBusDevice. This never caused any problems
before because all files including intel_iommu.h were already
including sysbus.h.

Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
---
 include/hw/i386/intel_iommu.h | 1 +
 1 file changed, 1 insertion(+)

diff --git a/include/hw/i386/intel_iommu.h b/include/hw/i386/intel_iommu.h
index 5dbadb7..e175000 100644
--- a/include/hw/i386/intel_iommu.h
+++ b/include/hw/i386/intel_iommu.h
@@ -21,6 +21,7 @@
 
 #ifndef INTEL_IOMMU_H
 #define INTEL_IOMMU_H
+#include "hw/sysbus.h"
 #include "hw/qdev.h"
 #include "sysemu/dma.h"
 
-- 
2.1.0

[Qemu-devel] [PATCH 13/13] acpi: Don't include q35 and piix headers

[Eduardo Habkost]

The ACPI code doesn't need to include q35 and piix headers
anymore, as the code is generic have no piix-specific or
q35-specific parts.

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

diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
index 048967e..d27ea67 100644
--- a/hw/i386/acpi-build.c
+++ b/hw/i386/acpi-build.c
@@ -28,6 +28,9 @@
 #include "qemu/osdep.h"
 #include "qemu/error-report.h"
 #include "hw/pci/pci.h"
+#include "hw/pci/pci_bridge.h"
+#include "hw/pci/pci_host.h"
+#include "hw/pci/pcie_host.h"
 #include "qom/cpu.h"
 #include "hw/i386/pc.h"
 #include "target-i386/cpu.h"
@@ -43,12 +46,9 @@
 #include "hw/acpi/tpm.h"
 #include "sysemu/tpm_backend.h"
 
-/* Supported chipsets: */
-#include "hw/acpi/piix4.h"
+#include "hw/acpi/pc-hotplug.h"
 #include "hw/acpi/pcihp.h"
-#include "hw/i386/ich9.h"
 #include "hw/pci/pci_bus.h"
-#include "hw/pci-host/q35.h"
 #include "hw/i386/intel_iommu.h"
 
 #include "hw/acpi/aml-build.h"
-- 
2.1.0

Re: [Qemu-devel] [PATCH 00/13] acpi: Make piix-specific and q35-specific code generic

[Igor Mammedov]

On Wed,  2 Dec 2015 20:22:45 -0200
Eduardo Habkost <ehabkost@redhat.com> wrote:

> This series removes piix-specific and q35-specific code from
> acpi-build.c, making it generic and without direct dependencies
> to piix and q35 code.
we are conflicting reshuffling acpi-build.c at the same time
could be cleanup done on top of my refactoring
drop_ASL_support_v1 branch:

https://github.com/imammedo/qemu/commits/drop_ASL_support_v1

> This series needs to be applied after the following:
> * [PATCH v3 0/6] pc: Initialization and compat function cleanup
> * [PATCH V3 0/3] hw/pcie: Multi-root support for Q35
> * [PATCH 00/16] pc: Eliminate struct PcGuestInfo
> 
> For reference, there's a git tree containing this series plus all
> the dependencies, at:
>   git://github.com/ehabkost/qemu-hacks.git work/acpi-decouple
> 
> Eduardo Habkost (13):
>   pc: Add PCMachineState::pci_host field
>   acpi: Remove unnecessary check for NULL pci_host
>   acpi: Eliminate acpi_get_i386_pci_host() function
>   acpi: Move DSDT info to PCMachineClass
>   acpi: Simplify s3/s4 property querying
>   acpi: Use &error_abort when getting PCI hotplug properties
>   acpi: Use QOM property to get CPU hotplug I/O base
>   acpi: Always try to init PCI hotplug I/O base
>   acpi: Use PCMachineState::acpi_dev to get ACPI dev
>   acpi: Change acpi_pci_hotplug_enabled() argument to PCMachineState
>   acpi: Don't use find_i440fx() when setting bsel properties
>   intel_iommu.h: Missing sysbus.h include
>   acpi: Don't include q35 and piix headers
> 
>  hw/acpi/cpu_hotplug.c         |   3 +
>  hw/acpi/ich9.c                |   2 +
>  hw/acpi/piix4.c               |   2 +
>  hw/i386/acpi-build.c          | 161 +++++++++++++-----------------------------
>  hw/i386/pc_piix.c             |   4 ++
>  hw/i386/pc_q35.c              |   6 ++
>  hw/pci-host/piix.c            |   1 +
>  include/hw/acpi/cpu_hotplug.h |   1 +
>  include/hw/acpi/pc-hotplug.h  |   4 +-
>  include/hw/i386/intel_iommu.h |   1 +
>  include/hw/i386/pc.h          |   3 +
>  11 files changed, 74 insertions(+), 114 deletions(-)
> 

Re: [Qemu-devel] [PATCH 00/13] acpi: Make piix-specific and q35-specific code generic

[Eduardo Habkost]

On Thu, Dec 03, 2015 at 04:19:19PM +0100, Igor Mammedov wrote:
> On Wed,  2 Dec 2015 20:22:45 -0200
> Eduardo Habkost <ehabkost@redhat.com> wrote:
> 
> > This series removes piix-specific and q35-specific code from
> > acpi-build.c, making it generic and without direct dependencies
> > to piix and q35 code.
> we are conflicting reshuffling acpi-build.c at the same time
> could be cleanup done on top of my refactoring
> drop_ASL_support_v1 branch:
> 
> https://github.com/imammedo/qemu/commits/drop_ASL_support_v1

Do you plan to submit it soon?

It looks like my series and yours go in opposite directions. I am
trying to remove piix4-specific code from acpi-build, and you are
addding a is_piix4 field to AcpiMiscInfo. Your series makes much
more difficult to remove piix4-specific code from acpi-build.c,
but this sounds like a reasonable price for moving the dsdt.dsl
logic to C. Maybe I should change my goal from "removing
piix4-specific code" to just simplifying the code.

I will redo this series on top of yours, taking that into
account.

There are a few conflicts with the other series, but they are
very easy to solve:

> > This series needs to be applied after the following:
> > * [PATCH v3 0/6] pc: Initialization and compat function cleanup

No conflicts.

> > * [PATCH V3 0/3] hw/pcie: Multi-root support for Q35

Trivial conflict at crs_range_merge().

> > * [PATCH 00/16] pc: Eliminate struct PcGuestInfo

Simple conflict at build_ssdt().

-- 
Eduardo

Re: [Qemu-devel] [PATCH 00/13] acpi: Make piix-specific and q35-specific code generic

[Igor Mammedov]

On Thu, 3 Dec 2015 15:16:21 -0200
Eduardo Habkost <ehabkost@redhat.com> wrote:

> On Thu, Dec 03, 2015 at 04:19:19PM +0100, Igor Mammedov wrote:
> > On Wed,  2 Dec 2015 20:22:45 -0200
> > Eduardo Habkost <ehabkost@redhat.com> wrote:
> > 
> > > This series removes piix-specific and q35-specific code from
> > > acpi-build.c, making it generic and without direct dependencies
> > > to piix and q35 code.
> > we are conflicting reshuffling acpi-build.c at the same time
> > could be cleanup done on top of my refactoring
> > drop_ASL_support_v1 branch:
> > 
> > https://github.com/imammedo/qemu/commits/drop_ASL_support_v1
> 
> Do you plan to submit it soon?
I plan to submit it as soon as 2.5 is released since Michael is busy
with 2.5 and possibly the only reviewer.
But if it help I can submit it earlier and add you in CC.

> 
> It looks like my series and yours go in opposite directions. I am
> trying to remove piix4-specific code from acpi-build, and you are
> addding a is_piix4 field to AcpiMiscInfo. Your series makes much
> more difficult to remove piix4-specific code from acpi-build.c,
> but this sounds like a reasonable price for moving the dsdt.dsl
> logic to C. Maybe I should change my goal from "removing
> piix4-specific code" to just simplifying the code.
It should be ok to remove explicit detection of piix4/q35 from
acpi-build.c, what I wouldn't wish though is to pull ACPI deps
into boards codebase. What we have in acpi-build.c is mostly
firmware generation code and it would be better to keep it there.

After merging refactoring, I plan to simplify it by moving out
generic memory/cpu hotplug parts into hw/acpi/,
but the rest of DSDT will probably stay there and we can
merge SSDT into DSDT which also should help to simplify it.

But for now it's dumb refactoring which is byte compatible with
DSDTs we have in ASL to avoid regressions in such huge series.

> 
> I will redo this series on top of yours, taking that into
> account.
Thanks!

> 
> There are a few conflicts with the other series, but they are
> very easy to solve:
> 
> > > This series needs to be applied after the following:
> > > * [PATCH v3 0/6] pc: Initialization and compat function cleanup
> 
> No conflicts.
> 
> > > * [PATCH V3 0/3] hw/pcie: Multi-root support for Q35
> 
> Trivial conflict at crs_range_merge().
> 
> > > * [PATCH 00/16] pc: Eliminate struct PcGuestInfo
> 
> Simple conflict at build_ssdt().
> 

Re: [Qemu-devel] [PATCH 00/13] acpi: Make piix-specific and q35-specific code generic

[Eduardo Habkost]

On Fri, Dec 04, 2015 at 02:24:41PM +0100, Igor Mammedov wrote:
> On Thu, 3 Dec 2015 15:16:21 -0200
> Eduardo Habkost <ehabkost@redhat.com> wrote:
> 
> > On Thu, Dec 03, 2015 at 04:19:19PM +0100, Igor Mammedov wrote:
> > > On Wed,  2 Dec 2015 20:22:45 -0200
> > > Eduardo Habkost <ehabkost@redhat.com> wrote:
> > > 
> > > > This series removes piix-specific and q35-specific code from
> > > > acpi-build.c, making it generic and without direct dependencies
> > > > to piix and q35 code.
> > > we are conflicting reshuffling acpi-build.c at the same time
> > > could be cleanup done on top of my refactoring
> > > drop_ASL_support_v1 branch:
> > > 
> > > https://github.com/imammedo/qemu/commits/drop_ASL_support_v1
> > 
> > Do you plan to submit it soon?
> I plan to submit it as soon as 2.5 is released since Michael is busy
> with 2.5 and possibly the only reviewer.
> But if it help I can submit it earlier and add you in CC.

I have been submitting for-2.6 patches, already. It helps get the
patches reviewed earlier so they have a chance to get applied as
soon as 2.5 is released.

> 
> > 
> > It looks like my series and yours go in opposite directions. I am
> > trying to remove piix4-specific code from acpi-build, and you are
> > addding a is_piix4 field to AcpiMiscInfo. Your series makes much
> > more difficult to remove piix4-specific code from acpi-build.c,
> > but this sounds like a reasonable price for moving the dsdt.dsl
> > logic to C. Maybe I should change my goal from "removing
> > piix4-specific code" to just simplifying the code.
> It should be ok to remove explicit detection of piix4/q35 from
> acpi-build.c, what I wouldn't wish though is to pull ACPI deps
> into boards codebase. What we have in acpi-build.c is mostly
> firmware generation code and it would be better to keep it there.

Agreed. And your refactoring makes the piix4/q35 differences much
more visible.

After making the piix4/q35 differences visible, I wonder if we
could make the reason for each "if (piix4)" line clearer. For
example, I assume some of the "if (piix4)" lines could become
"if (acpi_pci_hotplug_enabled())", but I don't know which ones.

> 
> After merging refactoring, I plan to simplify it by moving out
> generic memory/cpu hotplug parts into hw/acpi/,
> but the rest of DSDT will probably stay there and we can
> merge SSDT into DSDT which also should help to simplify it.
> 
> But for now it's dumb refactoring which is byte compatible with
> DSDTs we have in ASL to avoid regressions in such huge series.

I will let you make the cleanups you are planning, first. After
that is settled, I will check which of the patches in this series
still make sense. My original plan was to only suggest 1 or 2
simple cleanups after a discussion with Marcel. :)

-- 
Eduardo

Re: [Qemu-devel] [PATCH 00/13] acpi: Make piix-specific and q35-specific code generic

[Marcel Apfelbaum]

On 12/04/2015 04:08 PM, Eduardo Habkost wrote:
> On Fri, Dec 04, 2015 at 02:24:41PM +0100, Igor Mammedov wrote:
>> On Thu, 3 Dec 2015 15:16:21 -0200
>> Eduardo Habkost <ehabkost@redhat.com> wrote:

[..]

> I will let you make the cleanups you are planning, first. After
> that is settled, I will check which of the patches in this series
> still make sense.

I was planning to try and review the series, however after reading
this, the smart thing to do now is wait ... :)

Thanks,
Marcel


  My original plan was to only suggest 1 or 2
> simple cleanups after a discussion with Marcel. :)
>