[Qemu-devel] [RFC PATCH 0/9] hw/acpi: make build_madt arch agnostic

[Qemu-devel] [RFC PATCH 0/9] hw/acpi: make build_madt arch agnostic

[Wei Yang]

Now MADT is highly depend in architecture and machine type and leaves
duplicated code in different architecture. The series here tries to generalize
it.

MADT contains one main table and several sub tables. These sub tables are
highly related to architecture. Here we introduce one method to make it
architecture agnostic.

  * each architecture define its sub-table implementation function in madt_sub
  * introduces struct madt_input to collect sub table information and pass to
    build_madt

By doing so, each architecture could prepare its own sub-table implementation
and madt_input. And keep build_madt architecture agnostic.

Wei Yang (9):
  hw/acpi: expand pc_madt_cpu_entry in place
  hw/acpi: implement madt_sub[ACPI_APIC_PROCESSOR]
  hw/acpi: implement madt_sub[ACPI_APIC_LOCAL_X2APIC]
  hw/acpi: implement madt_sub[ACPI_APIC_IO]
  hw/acpi: implement madt_sub[ACPI_APIC_XRUPT_OVERRIDE]
  hw/acpi: implement madt_sub[ACPI_APIC_LOCAL_X2APIC_NMI]
  hw/acpi: implement madt_sub[ACPI_APIC_LOCAL_NMI]
  hw/acpi: factor build_madt with madt_input
  hw/acpi: implement madt_main to manipulate main madt table

 hw/acpi/cpu.c                        |  14 +-
 hw/acpi/piix4.c                      |   3 +-
 hw/i386/acpi-build.c                 | 265 +++++++++++++++++----------
 hw/isa/lpc_ich9.c                    |   3 +-
 include/hw/acpi/acpi_dev_interface.h |  12 +-
 include/hw/i386/pc.h                 |   2 +
 6 files changed, 194 insertions(+), 105 deletions(-)

-- 
2.19.1

[Qemu-devel] [RFC PATCH 1/9] hw/acpi: expand pc_madt_cpu_entry in place

[Wei Yang]

This is a preparation for MADT refactor.

Signed-off-by: Wei Yang <richardw.yang@linux.intel.com>
---
 hw/acpi/cpu.c                        | 33 +++++++++++--
 hw/acpi/piix4.c                      |  1 -
 hw/i386/acpi-build.c                 | 71 ++++++++++++----------------
 hw/isa/lpc_ich9.c                    |  1 -
 include/hw/acpi/acpi_dev_interface.h |  6 ---
 5 files changed, 59 insertions(+), 53 deletions(-)

diff --git a/hw/acpi/cpu.c b/hw/acpi/cpu.c
index 7a90c8f82d..d786df1a2c 100644
--- a/hw/acpi/cpu.c
+++ b/hw/acpi/cpu.c
@@ -329,7 +329,7 @@ void build_cpus_aml(Aml *table, MachineState *machine, CPUHotplugFeatures opts,
     char *cphp_res_path = g_strdup_printf("%s." CPUHP_RES_DEVICE, res_root);
     Object *obj = object_resolve_path_type("", TYPE_ACPI_DEVICE_IF, NULL);
     AcpiDeviceIfClass *adevc = ACPI_DEVICE_IF_GET_CLASS(obj);
-    AcpiDeviceIf *adev = ACPI_DEVICE_IF(obj);
+    uint32_t apic_id;
 
     cpu_ctrl_dev = aml_device("%s", cphp_res_path);
     {
@@ -522,8 +522,35 @@ void build_cpus_aml(Aml *table, MachineState *machine, CPUHotplugFeatures opts,
             aml_append(dev, method);
 
             /* build _MAT object */
-            assert(adevc && adevc->madt_cpu);
-            adevc->madt_cpu(adev, i, arch_ids, madt_buf);
+            assert(adevc);
+            apic_id = arch_ids->cpus[i].arch_id;
+            if (apic_id < 255) {
+                AcpiMadtProcessorApic *apic = acpi_data_push(madt_buf,
+                                                             sizeof *apic);
+
+                apic->type = ACPI_APIC_PROCESSOR;
+                apic->length = sizeof(*apic);
+                apic->processor_id = i;
+                apic->local_apic_id = apic_id;
+                if (arch_ids->cpus[i].cpu != NULL) {
+                    apic->flags = cpu_to_le32(1);
+                } else {
+                    apic->flags = cpu_to_le32(0);
+                }
+            } else {
+                AcpiMadtProcessorX2Apic *apic = acpi_data_push(madt_buf,
+                                                               sizeof *apic);
+
+                apic->type = ACPI_APIC_LOCAL_X2APIC;
+                apic->length = sizeof(*apic);
+                apic->uid = cpu_to_le32(i);
+                apic->x2apic_id = cpu_to_le32(apic_id);
+                if (arch_ids->cpus[i].cpu != NULL) {
+                    apic->flags = cpu_to_le32(1);
+                } else {
+                    apic->flags = cpu_to_le32(0);
+                }
+            }
             switch (madt_buf->data[0]) {
             case ACPI_APIC_PROCESSOR: {
                 AcpiMadtProcessorApic *apic = (void *)madt_buf->data;
diff --git a/hw/acpi/piix4.c b/hw/acpi/piix4.c
index 9c079d6834..76fde125a3 100644
--- a/hw/acpi/piix4.c
+++ b/hw/acpi/piix4.c
@@ -722,7 +722,6 @@ static void piix4_pm_class_init(ObjectClass *klass, void *data)
     hc->unplug = piix4_device_unplug_cb;
     adevc->ospm_status = piix4_ospm_status;
     adevc->send_event = piix4_send_gpe;
-    adevc->madt_cpu = pc_madt_cpu_entry;
 }
 
 static const TypeInfo piix4_pm_info = {
diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
index 29980bb3f4..54b70e62ac 100644
--- a/hw/i386/acpi-build.c
+++ b/hw/i386/acpi-build.c
@@ -301,52 +301,12 @@ build_facs(GArray *table_data)
     facs->length = cpu_to_le32(sizeof(*facs));
 }
 
-void pc_madt_cpu_entry(AcpiDeviceIf *adev, int uid,
-                       const CPUArchIdList *apic_ids, GArray *entry)
-{
-    uint32_t apic_id = apic_ids->cpus[uid].arch_id;
-
-    /* ACPI spec says that LAPIC entry for non present
-     * CPU may be omitted from MADT or it must be marked
-     * as disabled. However omitting non present CPU from
-     * MADT breaks hotplug on linux. So possible CPUs
-     * should be put in MADT but kept disabled.
-     */
-    if (apic_id < 255) {
-        AcpiMadtProcessorApic *apic = acpi_data_push(entry, sizeof *apic);
-
-        apic->type = ACPI_APIC_PROCESSOR;
-        apic->length = sizeof(*apic);
-        apic->processor_id = uid;
-        apic->local_apic_id = apic_id;
-        if (apic_ids->cpus[uid].cpu != NULL) {
-            apic->flags = cpu_to_le32(1);
-        } else {
-            apic->flags = cpu_to_le32(0);
-        }
-    } else {
-        AcpiMadtProcessorX2Apic *apic = acpi_data_push(entry, sizeof *apic);
-
-        apic->type = ACPI_APIC_LOCAL_X2APIC;
-        apic->length = sizeof(*apic);
-        apic->uid = cpu_to_le32(uid);
-        apic->x2apic_id = cpu_to_le32(apic_id);
-        if (apic_ids->cpus[uid].cpu != NULL) {
-            apic->flags = cpu_to_le32(1);
-        } else {
-            apic->flags = cpu_to_le32(0);
-        }
-    }
-}
-
 static void
 build_madt(GArray *table_data, BIOSLinker *linker, PCMachineState *pcms)
 {
     MachineClass *mc = MACHINE_GET_CLASS(pcms);
     const CPUArchIdList *apic_ids = mc->possible_cpu_arch_ids(MACHINE(pcms));
     int madt_start = table_data->len;
-    AcpiDeviceIfClass *adevc = ACPI_DEVICE_IF_GET_CLASS(pcms->acpi_dev);
-    AcpiDeviceIf *adev = ACPI_DEVICE_IF(pcms->acpi_dev);
     bool x2apic_mode = false;
 
     AcpiMultipleApicTable *madt;
@@ -359,8 +319,35 @@ build_madt(GArray *table_data, BIOSLinker *linker, PCMachineState *pcms)
     madt->flags = cpu_to_le32(1);
 
     for (i = 0; i < apic_ids->len; i++) {
-        adevc->madt_cpu(adev, i, apic_ids, table_data);
-        if (apic_ids->cpus[i].arch_id > 254) {
+        uint32_t apic_id = apic_ids->cpus[i].arch_id;
+        if (apic_id < 255) {
+            AcpiMadtProcessorApic *apic = acpi_data_push(table_data,
+                                                         sizeof *apic);
+
+            apic->type = ACPI_APIC_PROCESSOR;
+            apic->length = sizeof(*apic);
+            apic->processor_id = i;
+            apic->local_apic_id = apic_id;
+            if (apic_ids->cpus[i].cpu != NULL) {
+                apic->flags = cpu_to_le32(1);
+            } else {
+                apic->flags = cpu_to_le32(0);
+            }
+        } else {
+            AcpiMadtProcessorX2Apic *apic = acpi_data_push(table_data,
+                                                           sizeof *apic);
+
+            apic->type = ACPI_APIC_LOCAL_X2APIC;
+            apic->length = sizeof(*apic);
+            apic->uid = cpu_to_le32(i);
+            apic->x2apic_id = cpu_to_le32(apic_id);
+            if (apic_ids->cpus[i].cpu != NULL) {
+                apic->flags = cpu_to_le32(1);
+            } else {
+                apic->flags = cpu_to_le32(0);
+            }
+        }
+        if (apic_id > 254) {
             x2apic_mode = true;
         }
     }
diff --git a/hw/isa/lpc_ich9.c b/hw/isa/lpc_ich9.c
index ac44aa53be..8e00504cd9 100644
--- a/hw/isa/lpc_ich9.c
+++ b/hw/isa/lpc_ich9.c
@@ -811,7 +811,6 @@ static void ich9_lpc_class_init(ObjectClass *klass, void *data)
     hc->unplug = ich9_pm_device_unplug_cb;
     adevc->ospm_status = ich9_pm_ospm_status;
     adevc->send_event = ich9_send_gpe;
-    adevc->madt_cpu = pc_madt_cpu_entry;
 }
 
 static const TypeInfo ich9_lpc_info = {
diff --git a/include/hw/acpi/acpi_dev_interface.h b/include/hw/acpi/acpi_dev_interface.h
index 43ff119179..160b785045 100644
--- a/include/hw/acpi/acpi_dev_interface.h
+++ b/include/hw/acpi/acpi_dev_interface.h
@@ -35,10 +35,6 @@ void acpi_send_event(DeviceState *dev, AcpiEventStatusBits event);
  * ospm_status: returns status of ACPI device objects, reported
  *              via _OST method if device supports it.
  * send_event: inject a specified event into guest
- * madt_cpu: fills @entry with Interrupt Controller Structure
- *           for CPU indexed by @uid in @apic_ids array,
- *           returned structure types are:
- *           0 - Local APIC, 9 - Local x2APIC, 0xB - GICC
  *
  * Interface is designed for providing unified interface
  * to generic ACPI functionality that could be used without
@@ -52,7 +48,5 @@ typedef struct AcpiDeviceIfClass {
     /* <public> */
     void (*ospm_status)(AcpiDeviceIf *adev, ACPIOSTInfoList ***list);
     void (*send_event)(AcpiDeviceIf *adev, AcpiEventStatusBits ev);
-    void (*madt_cpu)(AcpiDeviceIf *adev, int uid,
-                     const CPUArchIdList *apic_ids, GArray *entry);
 } AcpiDeviceIfClass;
 #endif
-- 
2.19.1

[Qemu-devel] [RFC PATCH 2/9] hw/acpi: implement madt_sub[ACPI_APIC_PROCESSOR]

[Wei Yang]

Signed-off-by: Wei Yang <richardw.yang@linux.intel.com>
---
 hw/acpi/cpu.c                        | 15 +++--------
 hw/acpi/piix4.c                      |  1 +
 hw/i386/acpi-build.c                 | 39 +++++++++++++++++++---------
 hw/isa/lpc_ich9.c                    |  1 +
 include/hw/acpi/acpi_dev_interface.h |  5 ++++
 include/hw/i386/pc.h                 |  1 +
 6 files changed, 38 insertions(+), 24 deletions(-)

diff --git a/hw/acpi/cpu.c b/hw/acpi/cpu.c
index d786df1a2c..35e57f9824 100644
--- a/hw/acpi/cpu.c
+++ b/hw/acpi/cpu.c
@@ -508,6 +508,7 @@ void build_cpus_aml(Aml *table, MachineState *machine, CPUHotplugFeatures opts,
             Aml *uid = aml_int(i);
             GArray *madt_buf = g_array_new(0, 1, 1);
             int arch_id = arch_ids->cpus[i].arch_id;
+            int processor_id = i;
 
             if (opts.acpi_1_compatible && arch_id < 255) {
                 dev = aml_processor(i, 0, 0, CPU_NAME_FMT, i);
@@ -525,18 +526,8 @@ void build_cpus_aml(Aml *table, MachineState *machine, CPUHotplugFeatures opts,
             assert(adevc);
             apic_id = arch_ids->cpus[i].arch_id;
             if (apic_id < 255) {
-                AcpiMadtProcessorApic *apic = acpi_data_push(madt_buf,
-                                                             sizeof *apic);
-
-                apic->type = ACPI_APIC_PROCESSOR;
-                apic->length = sizeof(*apic);
-                apic->processor_id = i;
-                apic->local_apic_id = apic_id;
-                if (arch_ids->cpus[i].cpu != NULL) {
-                    apic->flags = cpu_to_le32(1);
-                } else {
-                    apic->flags = cpu_to_le32(0);
-                }
+                assert(adevc->madt_sub[ACPI_APIC_PROCESSOR]);
+                adevc->madt_sub[ACPI_APIC_PROCESSOR](madt_buf, &processor_id);
             } else {
                 AcpiMadtProcessorX2Apic *apic = acpi_data_push(madt_buf,
                                                                sizeof *apic);
diff --git a/hw/acpi/piix4.c b/hw/acpi/piix4.c
index 76fde125a3..f4336b9238 100644
--- a/hw/acpi/piix4.c
+++ b/hw/acpi/piix4.c
@@ -722,6 +722,7 @@ static void piix4_pm_class_init(ObjectClass *klass, void *data)
     hc->unplug = piix4_device_unplug_cb;
     adevc->ospm_status = piix4_ospm_status;
     adevc->send_event = piix4_send_gpe;
+    adevc->madt_sub = i386_madt_sub;
 }
 
 static const TypeInfo piix4_pm_info = {
diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
index 54b70e62ac..dfcdcaeaf7 100644
--- a/hw/i386/acpi-build.c
+++ b/hw/i386/acpi-build.c
@@ -301,12 +301,37 @@ build_facs(GArray *table_data)
     facs->length = cpu_to_le32(sizeof(*facs));
 }
 
+static void pc_madt_apic_entry(GArray *entry, void *opaque)
+{
+    MachineState *machine = MACHINE(qdev_get_machine());
+    MachineClass *mc = MACHINE_GET_CLASS(machine);
+    const CPUArchIdList *apic_ids = mc->possible_cpu_arch_ids(machine);
+    int *processor_id = opaque;
+    AcpiMadtProcessorApic *apic = acpi_data_push(entry, sizeof *apic);
+
+    apic->type = ACPI_APIC_PROCESSOR;
+    apic->length = sizeof(*apic);
+    apic->processor_id = *processor_id;
+    apic->local_apic_id = apic_ids->cpus[*processor_id].arch_id;
+    if (apic_ids->cpus[*processor_id].cpu != NULL) {
+        apic->flags = cpu_to_le32(1);
+    } else {
+        apic->flags = cpu_to_le32(0);
+    }
+}
+
+madt_operations i386_madt_sub = {
+    [ACPI_APIC_PROCESSOR] = pc_madt_apic_entry,
+};
+
 static void
 build_madt(GArray *table_data, BIOSLinker *linker, PCMachineState *pcms)
 {
     MachineClass *mc = MACHINE_GET_CLASS(pcms);
     const CPUArchIdList *apic_ids = mc->possible_cpu_arch_ids(MACHINE(pcms));
     int madt_start = table_data->len;
+    AcpiDeviceIfClass *adevc = ACPI_DEVICE_IF_GET_CLASS(pcms->acpi_dev);
+
     bool x2apic_mode = false;
 
     AcpiMultipleApicTable *madt;
@@ -320,19 +345,9 @@ build_madt(GArray *table_data, BIOSLinker *linker, PCMachineState *pcms)
 
     for (i = 0; i < apic_ids->len; i++) {
         uint32_t apic_id = apic_ids->cpus[i].arch_id;
+        int processor_id = i;
         if (apic_id < 255) {
-            AcpiMadtProcessorApic *apic = acpi_data_push(table_data,
-                                                         sizeof *apic);
-
-            apic->type = ACPI_APIC_PROCESSOR;
-            apic->length = sizeof(*apic);
-            apic->processor_id = i;
-            apic->local_apic_id = apic_id;
-            if (apic_ids->cpus[i].cpu != NULL) {
-                apic->flags = cpu_to_le32(1);
-            } else {
-                apic->flags = cpu_to_le32(0);
-            }
+            adevc->madt_sub[ACPI_APIC_PROCESSOR](table_data, &processor_id);
         } else {
             AcpiMadtProcessorX2Apic *apic = acpi_data_push(table_data,
                                                            sizeof *apic);
diff --git a/hw/isa/lpc_ich9.c b/hw/isa/lpc_ich9.c
index 8e00504cd9..efb0fd8e94 100644
--- a/hw/isa/lpc_ich9.c
+++ b/hw/isa/lpc_ich9.c
@@ -811,6 +811,7 @@ static void ich9_lpc_class_init(ObjectClass *klass, void *data)
     hc->unplug = ich9_pm_device_unplug_cb;
     adevc->ospm_status = ich9_pm_ospm_status;
     adevc->send_event = ich9_send_gpe;
+    adevc->madt_sub = i386_madt_sub;
 }
 
 static const TypeInfo ich9_lpc_info = {
diff --git a/include/hw/acpi/acpi_dev_interface.h b/include/hw/acpi/acpi_dev_interface.h
index 160b785045..3a3a12d543 100644
--- a/include/hw/acpi/acpi_dev_interface.h
+++ b/include/hw/acpi/acpi_dev_interface.h
@@ -3,6 +3,7 @@
 
 #include "qom/object.h"
 #include "hw/boards.h"
+#include "hw/acpi/acpi-defs.h"
 
 /* These values are part of guest ABI, and can not be changed */
 typedef enum {
@@ -29,6 +30,9 @@ typedef struct AcpiDeviceIf AcpiDeviceIf;
 
 void acpi_send_event(DeviceState *dev, AcpiEventStatusBits event);
 
+typedef void (*madt_operation)(GArray *entry, void *opaque);
+typedef madt_operation madt_operations[ACPI_APIC_RESERVED];
+
 /**
  * AcpiDeviceIfClass:
  *
@@ -48,5 +52,6 @@ typedef struct AcpiDeviceIfClass {
     /* <public> */
     void (*ospm_status)(AcpiDeviceIf *adev, ACPIOSTInfoList ***list);
     void (*send_event)(AcpiDeviceIf *adev, AcpiEventStatusBits ev);
+    madt_operation *madt_sub;
 } AcpiDeviceIfClass;
 #endif
diff --git a/include/hw/i386/pc.h b/include/hw/i386/pc.h
index ca65ef18af..db4ec693d3 100644
--- a/include/hw/i386/pc.h
+++ b/include/hw/i386/pc.h
@@ -281,6 +281,7 @@ void pc_system_firmware_init(PCMachineState *pcms, MemoryRegion *rom_memory);
 /* acpi-build.c */
 void pc_madt_cpu_entry(AcpiDeviceIf *adev, int uid,
                        const CPUArchIdList *apic_ids, GArray *entry);
+extern madt_operations i386_madt_sub;
 
 /* e820 types */
 #define E820_RAM        1
-- 
2.19.1

[Qemu-devel] [RFC PATCH 3/9] hw/acpi: implement madt_sub[ACPI_APIC_LOCAL_X2APIC]

[Wei Yang]

Signed-off-by: Wei Yang <richardw.yang@linux.intel.com>
---
 hw/acpi/cpu.c        | 14 ++------------
 hw/i386/acpi-build.c | 33 +++++++++++++++++++++------------
 2 files changed, 23 insertions(+), 24 deletions(-)

diff --git a/hw/acpi/cpu.c b/hw/acpi/cpu.c
index 35e57f9824..cb5970d659 100644
--- a/hw/acpi/cpu.c
+++ b/hw/acpi/cpu.c
@@ -529,18 +529,8 @@ void build_cpus_aml(Aml *table, MachineState *machine, CPUHotplugFeatures opts,
                 assert(adevc->madt_sub[ACPI_APIC_PROCESSOR]);
                 adevc->madt_sub[ACPI_APIC_PROCESSOR](madt_buf, &processor_id);
             } else {
-                AcpiMadtProcessorX2Apic *apic = acpi_data_push(madt_buf,
-                                                               sizeof *apic);
-
-                apic->type = ACPI_APIC_LOCAL_X2APIC;
-                apic->length = sizeof(*apic);
-                apic->uid = cpu_to_le32(i);
-                apic->x2apic_id = cpu_to_le32(apic_id);
-                if (arch_ids->cpus[i].cpu != NULL) {
-                    apic->flags = cpu_to_le32(1);
-                } else {
-                    apic->flags = cpu_to_le32(0);
-                }
+                adevc->madt_sub[ACPI_APIC_LOCAL_X2APIC](madt_buf,
+                                                        &processor_id);
             }
             switch (madt_buf->data[0]) {
             case ACPI_APIC_PROCESSOR: {
diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
index dfcdcaeaf7..4b480efff9 100644
--- a/hw/i386/acpi-build.c
+++ b/hw/i386/acpi-build.c
@@ -320,8 +320,28 @@ static void pc_madt_apic_entry(GArray *entry, void *opaque)
     }
 }
 
+static void pc_madt_x2apic_entry(GArray *entry, void *opaque)
+{
+    MachineState *machine = MACHINE(qdev_get_machine());
+    MachineClass *mc = MACHINE_GET_CLASS(machine);
+    const CPUArchIdList *apic_ids = mc->possible_cpu_arch_ids(machine);
+    int *processor_id = opaque;
+    AcpiMadtProcessorX2Apic *apic = acpi_data_push(entry, sizeof *apic);
+
+    apic->type = ACPI_APIC_LOCAL_X2APIC;
+    apic->length = sizeof(*apic);
+    apic->uid = cpu_to_le32(*processor_id);
+    apic->x2apic_id = cpu_to_le32(apic_ids->cpus[*processor_id].arch_id);
+    if (apic_ids->cpus[*processor_id].cpu != NULL) {
+        apic->flags = cpu_to_le32(1);
+    } else {
+        apic->flags = cpu_to_le32(0);
+    }
+}
+
 madt_operations i386_madt_sub = {
     [ACPI_APIC_PROCESSOR] = pc_madt_apic_entry,
+    [ACPI_APIC_LOCAL_X2APIC] = pc_madt_x2apic_entry,
 };
 
 static void
@@ -349,18 +369,7 @@ build_madt(GArray *table_data, BIOSLinker *linker, PCMachineState *pcms)
         if (apic_id < 255) {
             adevc->madt_sub[ACPI_APIC_PROCESSOR](table_data, &processor_id);
         } else {
-            AcpiMadtProcessorX2Apic *apic = acpi_data_push(table_data,
-                                                           sizeof *apic);
-
-            apic->type = ACPI_APIC_LOCAL_X2APIC;
-            apic->length = sizeof(*apic);
-            apic->uid = cpu_to_le32(i);
-            apic->x2apic_id = cpu_to_le32(apic_id);
-            if (apic_ids->cpus[i].cpu != NULL) {
-                apic->flags = cpu_to_le32(1);
-            } else {
-                apic->flags = cpu_to_le32(0);
-            }
+            adevc->madt_sub[ACPI_APIC_LOCAL_X2APIC](table_data, &processor_id);
         }
         if (apic_id > 254) {
             x2apic_mode = true;
-- 
2.19.1

[Qemu-devel] [RFC PATCH 4/9] hw/acpi: implement madt_sub[ACPI_APIC_IO]

[Wei Yang]

Signed-off-by: Wei Yang <richardw.yang@linux.intel.com>
---
 hw/i386/acpi-build.c | 21 ++++++++++++++-------
 1 file changed, 14 insertions(+), 7 deletions(-)

diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
index 4b480efff9..a661fff51d 100644
--- a/hw/i386/acpi-build.c
+++ b/hw/i386/acpi-build.c
@@ -339,9 +339,22 @@ static void pc_madt_x2apic_entry(GArray *entry, void *opaque)
     }
 }
 
+static void pc_madt_io_entry(GArray *entry, void *opaque)
+{
+    AcpiMadtIoApic *io_apic;
+
+    io_apic = acpi_data_push(entry, sizeof *io_apic);
+    io_apic->type = ACPI_APIC_IO;
+    io_apic->length = sizeof(*io_apic);
+    io_apic->io_apic_id = ACPI_BUILD_IOAPIC_ID;
+    io_apic->address = cpu_to_le32(IO_APIC_DEFAULT_ADDRESS);
+    io_apic->interrupt = cpu_to_le32(0);
+}
+
 madt_operations i386_madt_sub = {
     [ACPI_APIC_PROCESSOR] = pc_madt_apic_entry,
     [ACPI_APIC_LOCAL_X2APIC] = pc_madt_x2apic_entry,
+    [ACPI_APIC_IO] = pc_madt_io_entry,
 };
 
 static void
@@ -355,7 +368,6 @@ build_madt(GArray *table_data, BIOSLinker *linker, PCMachineState *pcms)
     bool x2apic_mode = false;
 
     AcpiMultipleApicTable *madt;
-    AcpiMadtIoApic *io_apic;
     AcpiMadtIntsrcovr *intsrcovr;
     int i;
 
@@ -376,12 +388,7 @@ build_madt(GArray *table_data, BIOSLinker *linker, PCMachineState *pcms)
         }
     }
 
-    io_apic = acpi_data_push(table_data, sizeof *io_apic);
-    io_apic->type = ACPI_APIC_IO;
-    io_apic->length = sizeof(*io_apic);
-    io_apic->io_apic_id = ACPI_BUILD_IOAPIC_ID;
-    io_apic->address = cpu_to_le32(IO_APIC_DEFAULT_ADDRESS);
-    io_apic->interrupt = cpu_to_le32(0);
+    adevc->madt_sub[ACPI_APIC_IO](table_data, NULL);
 
     if (pcms->apic_xrupt_override) {
         intsrcovr = acpi_data_push(table_data, sizeof *intsrcovr);
-- 
2.19.1

[Qemu-devel] [RFC PATCH 5/9] hw/acpi: implement madt_sub[ACPI_APIC_XRUPT_OVERRIDE]

[Wei Yang]

Signed-off-by: Wei Yang <richardw.yang@linux.intel.com>
---
 hw/i386/acpi-build.c | 37 ++++++++++++++++++++++++-------------
 1 file changed, 24 insertions(+), 13 deletions(-)

diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
index a661fff51d..f48cc5b292 100644
--- a/hw/i386/acpi-build.c
+++ b/hw/i386/acpi-build.c
@@ -351,10 +351,31 @@ static void pc_madt_io_entry(GArray *entry, void *opaque)
     io_apic->interrupt = cpu_to_le32(0);
 }
 
+static void pc_madt_xrupt_override_entry(GArray *entry, void *opaque)
+{
+    AcpiMadtIntsrcovr *intsrcovr;
+    int *idx = opaque;
+
+    intsrcovr = acpi_data_push(entry, sizeof *intsrcovr);
+    intsrcovr->type   = ACPI_APIC_XRUPT_OVERRIDE;
+    intsrcovr->length = sizeof(*intsrcovr);
+    intsrcovr->source = *idx;
+    if (*idx == 0) {
+        intsrcovr->gsi    = cpu_to_le32(2);
+        /* conforms to bus specifications */
+        intsrcovr->flags = cpu_to_le16(0);
+    } else {
+        intsrcovr->gsi    = cpu_to_le32(*idx);
+        /* active high, level triggered */
+        intsrcovr->flags = cpu_to_le16(0xd);
+    }
+}
+
 madt_operations i386_madt_sub = {
     [ACPI_APIC_PROCESSOR] = pc_madt_apic_entry,
     [ACPI_APIC_LOCAL_X2APIC] = pc_madt_x2apic_entry,
     [ACPI_APIC_IO] = pc_madt_io_entry,
+    [ACPI_APIC_XRUPT_OVERRIDE] = pc_madt_xrupt_override_entry,
 };
 
 static void
@@ -368,7 +389,6 @@ build_madt(GArray *table_data, BIOSLinker *linker, PCMachineState *pcms)
     bool x2apic_mode = false;
 
     AcpiMultipleApicTable *madt;
-    AcpiMadtIntsrcovr *intsrcovr;
     int i;
 
     madt = acpi_data_push(table_data, sizeof *madt);
@@ -391,12 +411,8 @@ build_madt(GArray *table_data, BIOSLinker *linker, PCMachineState *pcms)
     adevc->madt_sub[ACPI_APIC_IO](table_data, NULL);
 
     if (pcms->apic_xrupt_override) {
-        intsrcovr = acpi_data_push(table_data, sizeof *intsrcovr);
-        intsrcovr->type   = ACPI_APIC_XRUPT_OVERRIDE;
-        intsrcovr->length = sizeof(*intsrcovr);
-        intsrcovr->source = 0;
-        intsrcovr->gsi    = cpu_to_le32(2);
-        intsrcovr->flags  = cpu_to_le16(0); /* conforms to bus specifications */
+        i = 0;
+        adevc->madt_sub[ACPI_APIC_XRUPT_OVERRIDE](table_data, &i);
     }
     for (i = 1; i < 16; i++) {
 #define ACPI_BUILD_PCI_IRQS ((1<<5) | (1<<9) | (1<<10) | (1<<11))
@@ -404,12 +420,7 @@ build_madt(GArray *table_data, BIOSLinker *linker, PCMachineState *pcms)
             /* No need for a INT source override structure. */
             continue;
         }
-        intsrcovr = acpi_data_push(table_data, sizeof *intsrcovr);
-        intsrcovr->type   = ACPI_APIC_XRUPT_OVERRIDE;
-        intsrcovr->length = sizeof(*intsrcovr);
-        intsrcovr->source = i;
-        intsrcovr->gsi    = cpu_to_le32(i);
-        intsrcovr->flags  = cpu_to_le16(0xd); /* active high, level triggered */
+        adevc->madt_sub[ACPI_APIC_XRUPT_OVERRIDE](table_data, &i);
     }
 
     if (x2apic_mode) {
-- 
2.19.1

[Qemu-devel] [RFC PATCH 6/9] hw/acpi: implement madt_sub[ACPI_APIC_LOCAL_X2APIC_NMI]

[Wei Yang]

Signed-off-by: Wei Yang <richardw.yang@linux.intel.com>
---
 hw/i386/acpi-build.c | 22 ++++++++++++++--------
 1 file changed, 14 insertions(+), 8 deletions(-)

diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
index f48cc5b292..bec0bed53e 100644
--- a/hw/i386/acpi-build.c
+++ b/hw/i386/acpi-build.c
@@ -371,11 +371,24 @@ static void pc_madt_xrupt_override_entry(GArray *entry, void *opaque)
     }
 }
 
+static void pc_madt_x2apic_nmi_entry(GArray *entry, void *opaque)
+{
+    AcpiMadtLocalX2ApicNmi *local_nmi;
+
+    local_nmi = acpi_data_push(entry, sizeof *local_nmi);
+    local_nmi->type   = ACPI_APIC_LOCAL_X2APIC_NMI;
+    local_nmi->length = sizeof(*local_nmi);
+    local_nmi->uid    = 0xFFFFFFFF; /* all processors */
+    local_nmi->flags  = cpu_to_le16(0);
+    local_nmi->lint   = 1; /* ACPI_LINT1 */
+}
+
 madt_operations i386_madt_sub = {
     [ACPI_APIC_PROCESSOR] = pc_madt_apic_entry,
     [ACPI_APIC_LOCAL_X2APIC] = pc_madt_x2apic_entry,
     [ACPI_APIC_IO] = pc_madt_io_entry,
     [ACPI_APIC_XRUPT_OVERRIDE] = pc_madt_xrupt_override_entry,
+    [ACPI_APIC_LOCAL_X2APIC_NMI] = pc_madt_x2apic_nmi_entry,
 };
 
 static void
@@ -424,14 +437,7 @@ build_madt(GArray *table_data, BIOSLinker *linker, PCMachineState *pcms)
     }
 
     if (x2apic_mode) {
-        AcpiMadtLocalX2ApicNmi *local_nmi;
-
-        local_nmi = acpi_data_push(table_data, sizeof *local_nmi);
-        local_nmi->type   = ACPI_APIC_LOCAL_X2APIC_NMI;
-        local_nmi->length = sizeof(*local_nmi);
-        local_nmi->uid    = 0xFFFFFFFF; /* all processors */
-        local_nmi->flags  = cpu_to_le16(0);
-        local_nmi->lint   = 1; /* ACPI_LINT1 */
+        adevc->madt_sub[ACPI_APIC_LOCAL_X2APIC_NMI](table_data, NULL);
     } else {
         AcpiMadtLocalNmi *local_nmi;
 
-- 
2.19.1

[Qemu-devel] [RFC PATCH 7/9] hw/acpi: implement madt_sub[ACPI_APIC_LOCAL_NMI]

[Wei Yang]

Signed-off-by: Wei Yang <richardw.yang@linux.intel.com>
---
 hw/i386/acpi-build.c | 22 ++++++++++++++--------
 1 file changed, 14 insertions(+), 8 deletions(-)

diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
index bec0bed53e..a7aeb215fc 100644
--- a/hw/i386/acpi-build.c
+++ b/hw/i386/acpi-build.c
@@ -383,12 +383,25 @@ static void pc_madt_x2apic_nmi_entry(GArray *entry, void *opaque)
     local_nmi->lint   = 1; /* ACPI_LINT1 */
 }
 
+static void pc_madt_nmi_entry(GArray *entry, void *opaque)
+{
+    AcpiMadtLocalNmi *local_nmi;
+
+    local_nmi = acpi_data_push(entry, sizeof *local_nmi);
+    local_nmi->type         = ACPI_APIC_LOCAL_NMI;
+    local_nmi->length       = sizeof(*local_nmi);
+    local_nmi->processor_id = 0xff; /* all processors */
+    local_nmi->flags        = cpu_to_le16(0);
+    local_nmi->lint         = 1; /* ACPI_LINT1 */
+}
+
 madt_operations i386_madt_sub = {
     [ACPI_APIC_PROCESSOR] = pc_madt_apic_entry,
     [ACPI_APIC_LOCAL_X2APIC] = pc_madt_x2apic_entry,
     [ACPI_APIC_IO] = pc_madt_io_entry,
     [ACPI_APIC_XRUPT_OVERRIDE] = pc_madt_xrupt_override_entry,
     [ACPI_APIC_LOCAL_X2APIC_NMI] = pc_madt_x2apic_nmi_entry,
+    [ACPI_APIC_LOCAL_NMI] = pc_madt_nmi_entry,
 };
 
 static void
@@ -439,14 +452,7 @@ build_madt(GArray *table_data, BIOSLinker *linker, PCMachineState *pcms)
     if (x2apic_mode) {
         adevc->madt_sub[ACPI_APIC_LOCAL_X2APIC_NMI](table_data, NULL);
     } else {
-        AcpiMadtLocalNmi *local_nmi;
-
-        local_nmi = acpi_data_push(table_data, sizeof *local_nmi);
-        local_nmi->type         = ACPI_APIC_LOCAL_NMI;
-        local_nmi->length       = sizeof(*local_nmi);
-        local_nmi->processor_id = 0xff; /* all processors */
-        local_nmi->flags        = cpu_to_le16(0);
-        local_nmi->lint         = 1; /* ACPI_LINT1 */
+        adevc->madt_sub[ACPI_APIC_LOCAL_NMI](table_data, NULL);
     }
 
     build_header(linker, table_data,
-- 
2.19.1

[Qemu-devel] [RFC PATCH 8/9] hw/acpi: factor build_madt with madt_input

[Wei Yang]

struct madt_input is introduced to represent one sub madt table.

With help of madt_sub[] for related sub madt table, build_madt could
be agnostic.

Signed-off-by: Wei Yang <richardw.yang@linux.intel.com>
---
 hw/i386/acpi-build.c | 103 +++++++++++++++++++++++++++----------------
 1 file changed, 65 insertions(+), 38 deletions(-)

diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
index a7aeb215fc..74a34e297e 100644
--- a/hw/i386/acpi-build.c
+++ b/hw/i386/acpi-build.c
@@ -284,6 +284,54 @@ static void acpi_get_pci_holes(Range *hole, Range *hole64)
                                                NULL));
 }
 
+struct madt_input {
+    int sub_id;
+    void *opaque;
+};
+
+int xrupt_override_idx[] = {0, 5, 9, 10, 11};
+static struct madt_input *
+acpi_get_madt_input(PCMachineState *pcms, int *processor_id)
+{
+    MachineClass *mc = MACHINE_GET_CLASS(pcms);
+    const CPUArchIdList *apic_ids = mc->possible_cpu_arch_ids(MACHINE(pcms));
+    int i, sub_heads = 0;
+    uint32_t apic_id;
+    struct madt_input *input = NULL;
+
+    sub_heads = apic_ids->len                      /* PROCESSOR/X2APIC */
+                + 1                                /* APIC_IO */
+                + ARRAY_SIZE(xrupt_override_idx)   /* XRUPT_OVERRIDE */
+                + 1                                /* NMI/X2APIC_NMI */
+                + 1;                               /* END MARK */
+    input = g_new0(struct madt_input, sub_heads);
+    for (i = 0, sub_heads = 0; i < apic_ids->len; i++, sub_heads++) {
+        apic_id = apic_ids->cpus[i].arch_id;
+        if (apic_id < 255) {
+            input[sub_heads].sub_id = ACPI_APIC_PROCESSOR;
+        } else {
+            input[sub_heads].sub_id = ACPI_APIC_LOCAL_X2APIC;
+        }
+        input[sub_heads].opaque = processor_id;
+    }
+    input[sub_heads++].sub_id = ACPI_APIC_IO;
+    for (i = 0; i < ARRAY_SIZE(xrupt_override_idx); i++, sub_heads++) {
+        if (i == 0 && !pcms->apic_xrupt_override) {
+            continue;
+        }
+        input[sub_heads].sub_id = ACPI_APIC_XRUPT_OVERRIDE;
+        input[sub_heads].opaque = &xrupt_override_idx[i];
+    }
+    if (apic_id > 254) {
+        input[sub_heads++].sub_id = ACPI_APIC_LOCAL_X2APIC_NMI;
+    } else {
+        input[sub_heads++].sub_id = ACPI_APIC_LOCAL_NMI;
+    }
+    input[sub_heads].sub_id = ACPI_APIC_RESERVED;
+
+    return input;
+}
+
 static void acpi_align_size(GArray *blob, unsigned align)
 {
     /* Align size to multiple of given size. This reduces the chance
@@ -318,6 +366,7 @@ static void pc_madt_apic_entry(GArray *entry, void *opaque)
     } else {
         apic->flags = cpu_to_le32(0);
     }
+    (*processor_id)++;
 }
 
 static void pc_madt_x2apic_entry(GArray *entry, void *opaque)
@@ -337,6 +386,7 @@ static void pc_madt_x2apic_entry(GArray *entry, void *opaque)
     } else {
         apic->flags = cpu_to_le32(0);
     }
+    (*processor_id)++;
 }
 
 static void pc_madt_io_entry(GArray *entry, void *opaque)
@@ -405,54 +455,27 @@ madt_operations i386_madt_sub = {
 };
 
 static void
-build_madt(GArray *table_data, BIOSLinker *linker, PCMachineState *pcms)
+build_madt(GArray *table_data, BIOSLinker *linker, PCMachineState *pcms,
+           struct madt_input *input)
 {
-    MachineClass *mc = MACHINE_GET_CLASS(pcms);
-    const CPUArchIdList *apic_ids = mc->possible_cpu_arch_ids(MACHINE(pcms));
     int madt_start = table_data->len;
     AcpiDeviceIfClass *adevc = ACPI_DEVICE_IF_GET_CLASS(pcms->acpi_dev);
 
-    bool x2apic_mode = false;
-
     AcpiMultipleApicTable *madt;
-    int i;
+    int i, sub_id;
+    void *opaque;
 
     madt = acpi_data_push(table_data, sizeof *madt);
     madt->local_apic_address = cpu_to_le32(APIC_DEFAULT_ADDRESS);
     madt->flags = cpu_to_le32(1);
 
-    for (i = 0; i < apic_ids->len; i++) {
-        uint32_t apic_id = apic_ids->cpus[i].arch_id;
-        int processor_id = i;
-        if (apic_id < 255) {
-            adevc->madt_sub[ACPI_APIC_PROCESSOR](table_data, &processor_id);
-        } else {
-            adevc->madt_sub[ACPI_APIC_LOCAL_X2APIC](table_data, &processor_id);
+    for (i = 0; ; i++) {
+        sub_id = input[i].sub_id;
+        if (sub_id == ACPI_APIC_RESERVED) {
+            break;
         }
-        if (apic_id > 254) {
-            x2apic_mode = true;
-        }
-    }
-
-    adevc->madt_sub[ACPI_APIC_IO](table_data, NULL);
-
-    if (pcms->apic_xrupt_override) {
-        i = 0;
-        adevc->madt_sub[ACPI_APIC_XRUPT_OVERRIDE](table_data, &i);
-    }
-    for (i = 1; i < 16; i++) {
-#define ACPI_BUILD_PCI_IRQS ((1<<5) | (1<<9) | (1<<10) | (1<<11))
-        if (!(ACPI_BUILD_PCI_IRQS & (1 << i))) {
-            /* No need for a INT source override structure. */
-            continue;
-        }
-        adevc->madt_sub[ACPI_APIC_XRUPT_OVERRIDE](table_data, &i);
-    }
-
-    if (x2apic_mode) {
-        adevc->madt_sub[ACPI_APIC_LOCAL_X2APIC_NMI](table_data, NULL);
-    } else {
-        adevc->madt_sub[ACPI_APIC_LOCAL_NMI](table_data, NULL);
+        opaque = input[i].opaque;
+        adevc->madt_sub[sub_id](table_data, opaque);
     }
 
     build_header(linker, table_data,
@@ -2627,6 +2650,8 @@ void acpi_build(AcpiBuildTables *tables, MachineState *machine)
     GArray *tables_blob = tables->table_data;
     AcpiSlicOem slic_oem = { .id = NULL, .table_id = NULL };
     Object *vmgenid_dev;
+    struct madt_input *input = NULL;
+    int processor_id = 0;
 
     acpi_get_pm_info(&pm);
     acpi_get_misc_info(&misc);
@@ -2671,7 +2696,9 @@ void acpi_build(AcpiBuildTables *tables, MachineState *machine)
     aml_len += tables_blob->len - fadt;
 
     acpi_add_table(table_offsets, tables_blob);
-    build_madt(tables_blob, tables->linker, pcms);
+    input = acpi_get_madt_input(pcms, &processor_id);
+    build_madt(tables_blob, tables->linker, pcms, input);
+    g_free(input);
 
     vmgenid_dev = find_vmgenid_dev();
     if (vmgenid_dev) {
-- 
2.19.1

[Qemu-devel] [RFC PATCH 9/9] hw/acpi: implement madt_main to manipulate main madt table

[Wei Yang]

Different arch would handle the main madt table differently. Add a
helper function to achieve this goal.

Signed-off-by: Wei Yang <richardw.yang@linux.intel.com>
---
 hw/acpi/piix4.c                      |  1 +
 hw/i386/acpi-build.c                 | 13 +++++++++++--
 hw/isa/lpc_ich9.c                    |  1 +
 include/hw/acpi/acpi_dev_interface.h |  1 +
 include/hw/i386/pc.h                 |  1 +
 5 files changed, 15 insertions(+), 2 deletions(-)

diff --git a/hw/acpi/piix4.c b/hw/acpi/piix4.c
index f4336b9238..d7d097daee 100644
--- a/hw/acpi/piix4.c
+++ b/hw/acpi/piix4.c
@@ -723,6 +723,7 @@ static void piix4_pm_class_init(ObjectClass *klass, void *data)
     adevc->ospm_status = piix4_ospm_status;
     adevc->send_event = piix4_send_gpe;
     adevc->madt_sub = i386_madt_sub;
+    adevc->madt_main = i386_madt_main;
 }
 
 static const TypeInfo piix4_pm_info = {
diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
index 74a34e297e..e73e9fddee 100644
--- a/hw/i386/acpi-build.c
+++ b/hw/i386/acpi-build.c
@@ -454,6 +454,14 @@ madt_operations i386_madt_sub = {
     [ACPI_APIC_LOCAL_NMI] = pc_madt_nmi_entry,
 };
 
+void i386_madt_main(GArray *entry, void *opaque)
+{
+    AcpiMultipleApicTable *madt = opaque;
+
+    madt->local_apic_address = cpu_to_le32(APIC_DEFAULT_ADDRESS);
+    madt->flags = cpu_to_le32(1);
+}
+
 static void
 build_madt(GArray *table_data, BIOSLinker *linker, PCMachineState *pcms,
            struct madt_input *input)
@@ -466,8 +474,9 @@ build_madt(GArray *table_data, BIOSLinker *linker, PCMachineState *pcms,
     void *opaque;
 
     madt = acpi_data_push(table_data, sizeof *madt);
-    madt->local_apic_address = cpu_to_le32(APIC_DEFAULT_ADDRESS);
-    madt->flags = cpu_to_le32(1);
+    if (adevc->madt_main) {
+        adevc->madt_main(table_data, madt);
+    }
 
     for (i = 0; ; i++) {
         sub_id = input[i].sub_id;
diff --git a/hw/isa/lpc_ich9.c b/hw/isa/lpc_ich9.c
index efb0fd8e94..21afd1a12d 100644
--- a/hw/isa/lpc_ich9.c
+++ b/hw/isa/lpc_ich9.c
@@ -812,6 +812,7 @@ static void ich9_lpc_class_init(ObjectClass *klass, void *data)
     adevc->ospm_status = ich9_pm_ospm_status;
     adevc->send_event = ich9_send_gpe;
     adevc->madt_sub = i386_madt_sub;
+    adevc->madt_main = i386_madt_main;
 }
 
 static const TypeInfo ich9_lpc_info = {
diff --git a/include/hw/acpi/acpi_dev_interface.h b/include/hw/acpi/acpi_dev_interface.h
index 3a3a12d543..d3b216544d 100644
--- a/include/hw/acpi/acpi_dev_interface.h
+++ b/include/hw/acpi/acpi_dev_interface.h
@@ -52,6 +52,7 @@ typedef struct AcpiDeviceIfClass {
     /* <public> */
     void (*ospm_status)(AcpiDeviceIf *adev, ACPIOSTInfoList ***list);
     void (*send_event)(AcpiDeviceIf *adev, AcpiEventStatusBits ev);
+    madt_operation madt_main;
     madt_operation *madt_sub;
 } AcpiDeviceIfClass;
 #endif
diff --git a/include/hw/i386/pc.h b/include/hw/i386/pc.h
index db4ec693d3..6b7dd060ac 100644
--- a/include/hw/i386/pc.h
+++ b/include/hw/i386/pc.h
@@ -282,6 +282,7 @@ void pc_system_firmware_init(PCMachineState *pcms, MemoryRegion *rom_memory);
 void pc_madt_cpu_entry(AcpiDeviceIf *adev, int uid,
                        const CPUArchIdList *apic_ids, GArray *entry);
 extern madt_operations i386_madt_sub;
+void i386_madt_main(GArray *entry, void *opaque);
 
 /* e820 types */
 #define E820_RAM        1
-- 
2.19.1

Re: [Qemu-devel] [RFC PATCH 0/9] hw/acpi: make build_madt arch agnostic

[Wei Yang]

Igor,

Do you have some spare time to take a look the general idea?

On Mon, May 13, 2019 at 02:19:04PM +0800, Wei Yang wrote:
>Now MADT is highly depend in architecture and machine type and leaves
>duplicated code in different architecture. The series here tries to generalize
>it.
>
>MADT contains one main table and several sub tables. These sub tables are
>highly related to architecture. Here we introduce one method to make it
>architecture agnostic.
>
>  * each architecture define its sub-table implementation function in madt_sub
>  * introduces struct madt_input to collect sub table information and pass to
>    build_madt
>
>By doing so, each architecture could prepare its own sub-table implementation
>and madt_input. And keep build_madt architecture agnostic.
>
>Wei Yang (9):
>  hw/acpi: expand pc_madt_cpu_entry in place
>  hw/acpi: implement madt_sub[ACPI_APIC_PROCESSOR]
>  hw/acpi: implement madt_sub[ACPI_APIC_LOCAL_X2APIC]
>  hw/acpi: implement madt_sub[ACPI_APIC_IO]
>  hw/acpi: implement madt_sub[ACPI_APIC_XRUPT_OVERRIDE]
>  hw/acpi: implement madt_sub[ACPI_APIC_LOCAL_X2APIC_NMI]
>  hw/acpi: implement madt_sub[ACPI_APIC_LOCAL_NMI]
>  hw/acpi: factor build_madt with madt_input
>  hw/acpi: implement madt_main to manipulate main madt table
>
> hw/acpi/cpu.c                        |  14 +-
> hw/acpi/piix4.c                      |   3 +-
> hw/i386/acpi-build.c                 | 265 +++++++++++++++++----------
> hw/isa/lpc_ich9.c                    |   3 +-
> include/hw/acpi/acpi_dev_interface.h |  12 +-
> include/hw/i386/pc.h                 |   2 +
> 6 files changed, 194 insertions(+), 105 deletions(-)
>
>-- 
>2.19.1

-- 
Wei Yang
Help you, Help me

Re: [Qemu-devel] [RFC PATCH 0/9] hw/acpi: make build_madt arch agnostic

[Igor Mammedov]

On Mon, 13 May 2019 14:19:04 +0800
Wei Yang <richardw.yang@linux.intel.com> wrote:

> Now MADT is highly depend in architecture and machine type and leaves
> duplicated code in different architecture. The series here tries to generalize
> it.
> 
> MADT contains one main table and several sub tables. These sub tables are
> highly related to architecture. Here we introduce one method to make it
> architecture agnostic.
> 
>   * each architecture define its sub-table implementation function in madt_sub
>   * introduces struct madt_input to collect sub table information and pass to
>     build_madt
> 
> By doing so, each architecture could prepare its own sub-table implementation
> and madt_input. And keep build_madt architecture agnostic.

I've skimmed over patches, and to me it looks mostly as code movement
without apparent benefits and probably a bit more complex than what we have now
(it might be ok cost if it simplifies MADT support for other boards).

Before I do line by line review could you demonstrate what effect new way
to build MADT would have on arm/virt and i386/virt (from NEMU). So it would be
possible to estimate net benefits from new approach?
(PS: it doesn't have to be patches ready for merging, just a dirty hack
that would demonstrate adding MADT for new board using mad_sub[])

> 
> Wei Yang (9):
>   hw/acpi: expand pc_madt_cpu_entry in place
>   hw/acpi: implement madt_sub[ACPI_APIC_PROCESSOR]
>   hw/acpi: implement madt_sub[ACPI_APIC_LOCAL_X2APIC]
>   hw/acpi: implement madt_sub[ACPI_APIC_IO]
>   hw/acpi: implement madt_sub[ACPI_APIC_XRUPT_OVERRIDE]
>   hw/acpi: implement madt_sub[ACPI_APIC_LOCAL_X2APIC_NMI]
>   hw/acpi: implement madt_sub[ACPI_APIC_LOCAL_NMI]
>   hw/acpi: factor build_madt with madt_input
>   hw/acpi: implement madt_main to manipulate main madt table
> 
>  hw/acpi/cpu.c                        |  14 +-
>  hw/acpi/piix4.c                      |   3 +-
>  hw/i386/acpi-build.c                 | 265 +++++++++++++++++----------
>  hw/isa/lpc_ich9.c                    |   3 +-
>  include/hw/acpi/acpi_dev_interface.h |  12 +-
>  include/hw/i386/pc.h                 |   2 +
>  6 files changed, 194 insertions(+), 105 deletions(-)
> 

Re: [Qemu-devel] [RFC PATCH 0/9] hw/acpi: make build_madt arch agnostic

[Wei Yang]

On Tue, Jun 18, 2019 at 05:59:56PM +0200, Igor Mammedov wrote:
>
>On Mon, 13 May 2019 14:19:04 +0800
>Wei Yang <richardw.yang@linux.intel.com> wrote:
>
>> Now MADT is highly depend in architecture and machine type and leaves
>> duplicated code in different architecture. The series here tries to generalize
>> it.
>> 
>> MADT contains one main table and several sub tables. These sub tables are
>> highly related to architecture. Here we introduce one method to make it
>> architecture agnostic.
>> 
>>   * each architecture define its sub-table implementation function in madt_sub
>>   * introduces struct madt_input to collect sub table information and pass to
>>     build_madt
>> 
>> By doing so, each architecture could prepare its own sub-table implementation
>> and madt_input. And keep build_madt architecture agnostic.
>
>I've skimmed over patches, and to me it looks mostly as code movement
>without apparent benefits and probably a bit more complex than what we have now
>(it might be ok cost if it simplifies MADT support for other boards).
>
>Before I do line by line review could you demonstrate what effect new way
>to build MADT would have on arm/virt and i386/virt (from NEMU). So it would be
>possible to estimate net benefits from new approach?
>(PS: it doesn't have to be patches ready for merging, just a dirty hack
>that would demonstrate adding MADT for new board using mad_sub[])
>

Per APIC spec 5.2.12, MADT contains a *main* table and several *sub* tables
(Interrupt Controllere), so the idea is give a callback hook in
AcpiDeviceIfClass for each table, including *main* and *sub* table.

Current AcpiDeviceIfClass has one callback pc_madt_cpu_entry for some *sub*
tables, after replacing the AcpiDeviceIfClass will look like this:

typedef struct AcpiDeviceIfClass {
    /* <private> */
    InterfaceClass parent_class;

    /* <public> */
    void (*ospm_status)(AcpiDeviceIf *adev, ACPIOSTInfoList ***list);
    void (*send_event)(AcpiDeviceIf *adev, AcpiEventStatusBits ev);
-   void (*madt_cpu)(AcpiDeviceIf *adev, int uid,
-                    const CPUArchIdList *apic_ids, GArray *entry);
+   madt_operation madt_main;
+   madt_operation *madt_sub;
} AcpiDeviceIfClass;

By doing so, each arch could have its own implementation for MADT.

After this refactoring, build_madt could be simplified to:

build_madt(GArray *table_data, BIOSLinker *linker, PCMachineState *pcms,
           struct madt_input *input)
{
    ...

    if (adevc->madt_main) {
        adevc->madt_main(table_data, madt);
    }

    for (i = 0; ; i++) {
        sub_id = input[i].sub_id;
        if (sub_id == ACPI_APIC_RESERVED) {
            break;
        }
        opaque = input[i].opaque;
        adevc->madt_sub[sub_id](table_data, opaque);
    }

    ...
}

input is a list of data necessary to build *sub* table. Its details is also
arch dependent.

For following new arch, what it need to do is prepare the input array and
implement necessary *main*/*sub* table callbacks.

-- 
Wei Yang
Help you, Help me

Re: [Qemu-devel] [RFC PATCH 0/9] hw/acpi: make build_madt arch agnostic

[Igor Mammedov]

On Wed, 19 Jun 2019 14:20:50 +0800
Wei Yang <richardw.yang@linux.intel.com> wrote:

> On Tue, Jun 18, 2019 at 05:59:56PM +0200, Igor Mammedov wrote:
> >
> >On Mon, 13 May 2019 14:19:04 +0800
> >Wei Yang <richardw.yang@linux.intel.com> wrote:
> >  
> >> Now MADT is highly depend in architecture and machine type and leaves
> >> duplicated code in different architecture. The series here tries to generalize
> >> it.
> >> 
> >> MADT contains one main table and several sub tables. These sub tables are
> >> highly related to architecture. Here we introduce one method to make it
> >> architecture agnostic.
> >> 
> >>   * each architecture define its sub-table implementation function in madt_sub
> >>   * introduces struct madt_input to collect sub table information and pass to
> >>     build_madt
> >> 
> >> By doing so, each architecture could prepare its own sub-table implementation
> >> and madt_input. And keep build_madt architecture agnostic.  
> >
> >I've skimmed over patches, and to me it looks mostly as code movement
> >without apparent benefits and probably a bit more complex than what we have now
> >(it might be ok cost if it simplifies MADT support for other boards).
> >
> >Before I do line by line review could you demonstrate what effect new way
> >to build MADT would have on arm/virt and i386/virt (from NEMU). So it would be
> >possible to estimate net benefits from new approach?
> >(PS: it doesn't have to be patches ready for merging, just a dirty hack
> >that would demonstrate adding MADT for new board using mad_sub[])
> >  
> 
> Per APIC spec 5.2.12, MADT contains a *main* table and several *sub* tables
> (Interrupt Controllere), so the idea is give a callback hook in
> AcpiDeviceIfClass for each table, including *main* and *sub* table.
> 
> Current AcpiDeviceIfClass has one callback pc_madt_cpu_entry for some *sub*
> tables, after replacing the AcpiDeviceIfClass will look like this:
> 
> typedef struct AcpiDeviceIfClass {
>     /* <private> */
>     InterfaceClass parent_class;
> 
>     /* <public> */
>     void (*ospm_status)(AcpiDeviceIf *adev, ACPIOSTInfoList ***list);
>     void (*send_event)(AcpiDeviceIf *adev, AcpiEventStatusBits ev);
> -   void (*madt_cpu)(AcpiDeviceIf *adev, int uid,
> -                    const CPUArchIdList *apic_ids, GArray *entry);
> +   madt_operation madt_main;
> +   madt_operation *madt_sub;
> } AcpiDeviceIfClass;
> 
> By doing so, each arch could have its own implementation for MADT.
> 
> After this refactoring, build_madt could be simplified to:
> 
> build_madt(GArray *table_data, BIOSLinker *linker, PCMachineState *pcms,
>            struct madt_input *input)
> {
>     ...
> 
>     if (adevc->madt_main) {
>         adevc->madt_main(table_data, madt);
>     }
> 
>     for (i = 0; ; i++) {
>         sub_id = input[i].sub_id;
>         if (sub_id == ACPI_APIC_RESERVED) {
>             break;
>         }
>         opaque = input[i].opaque;
>         adevc->madt_sub[sub_id](table_data, opaque);
>     }
> 
>     ...
> }
> 
> input is a list of data necessary to build *sub* table. Its details is also
> arch dependent.
I've got general idea reading patches in this series.
As I've mentioned before it's hard to generalize MADT since it
mostly contains entries unique for target/board.
Goal here isn't generalizing at any cost, but rather find out
if there is enough common code to justify generalization
and if it allows us to reduce code duplication and simplify.

> For following new arch, what it need to do is prepare the input array and
> implement necessary *main*/*sub* table callbacks.
What I'd like to see is the actual patch that does this,
to see if it has any merit and to compare to the current
approach.

Re: [Qemu-devel] [RFC PATCH 0/9] hw/acpi: make build_madt arch agnostic

[Wei Yang]

On Wed, Jun 19, 2019 at 11:04:40AM +0200, Igor Mammedov wrote:
>On Wed, 19 Jun 2019 14:20:50 +0800
>Wei Yang <richardw.yang@linux.intel.com> wrote:
>
>> On Tue, Jun 18, 2019 at 05:59:56PM +0200, Igor Mammedov wrote:
>> >
>> >On Mon, 13 May 2019 14:19:04 +0800
>> >Wei Yang <richardw.yang@linux.intel.com> wrote:
>> >  
>> >> Now MADT is highly depend in architecture and machine type and leaves
>> >> duplicated code in different architecture. The series here tries to generalize
>> >> it.
>> >> 
>> >> MADT contains one main table and several sub tables. These sub tables are
>> >> highly related to architecture. Here we introduce one method to make it
>> >> architecture agnostic.
>> >> 
>> >>   * each architecture define its sub-table implementation function in madt_sub
>> >>   * introduces struct madt_input to collect sub table information and pass to
>> >>     build_madt
>> >> 
>> >> By doing so, each architecture could prepare its own sub-table implementation
>> >> and madt_input. And keep build_madt architecture agnostic.  
>> >
>> >I've skimmed over patches, and to me it looks mostly as code movement
>> >without apparent benefits and probably a bit more complex than what we have now
>> >(it might be ok cost if it simplifies MADT support for other boards).
>> >
>> >Before I do line by line review could you demonstrate what effect new way
>> >to build MADT would have on arm/virt and i386/virt (from NEMU). So it would be
>> >possible to estimate net benefits from new approach?
>> >(PS: it doesn't have to be patches ready for merging, just a dirty hack
>> >that would demonstrate adding MADT for new board using mad_sub[])
>> >  
>> 
>> Per APIC spec 5.2.12, MADT contains a *main* table and several *sub* tables
>> (Interrupt Controllere), so the idea is give a callback hook in
>> AcpiDeviceIfClass for each table, including *main* and *sub* table.
>> 
>> Current AcpiDeviceIfClass has one callback pc_madt_cpu_entry for some *sub*
>> tables, after replacing the AcpiDeviceIfClass will look like this:
>> 
>> typedef struct AcpiDeviceIfClass {
>>     /* <private> */
>>     InterfaceClass parent_class;
>> 
>>     /* <public> */
>>     void (*ospm_status)(AcpiDeviceIf *adev, ACPIOSTInfoList ***list);
>>     void (*send_event)(AcpiDeviceIf *adev, AcpiEventStatusBits ev);
>> -   void (*madt_cpu)(AcpiDeviceIf *adev, int uid,
>> -                    const CPUArchIdList *apic_ids, GArray *entry);
>> +   madt_operation madt_main;
>> +   madt_operation *madt_sub;
>> } AcpiDeviceIfClass;
>> 
>> By doing so, each arch could have its own implementation for MADT.
>> 
>> After this refactoring, build_madt could be simplified to:
>> 
>> build_madt(GArray *table_data, BIOSLinker *linker, PCMachineState *pcms,
>>            struct madt_input *input)
>> {
>>     ...
>> 
>>     if (adevc->madt_main) {
>>         adevc->madt_main(table_data, madt);
>>     }
>> 
>>     for (i = 0; ; i++) {
>>         sub_id = input[i].sub_id;
>>         if (sub_id == ACPI_APIC_RESERVED) {
>>             break;
>>         }
>>         opaque = input[i].opaque;
>>         adevc->madt_sub[sub_id](table_data, opaque);
>>     }
>> 
>>     ...
>> }
>> 
>> input is a list of data necessary to build *sub* table. Its details is also
>> arch dependent.
>I've got general idea reading patches in this series.
>As I've mentioned before it's hard to generalize MADT since it
>mostly contains entries unique for target/board.
>Goal here isn't generalizing at any cost, but rather find out
>if there is enough common code to justify generalization
>and if it allows us to reduce code duplication and simplify.
>
>> For following new arch, what it need to do is prepare the input array and
>> implement necessary *main*/*sub* table callbacks.
>What I'd like to see is the actual patch that does this,
>to see if it has any merit and to compare to the current
>approach.

I didn't get some idea about your approach. Would you mind sharing more light?

-- 
Wei Yang
Help you, Help me

Re: [Qemu-devel] [RFC PATCH 0/9] hw/acpi: make build_madt arch agnostic

[Igor Mammedov]

On Thu, 20 Jun 2019 14:18:42 +0000
Wei Yang <richard.weiyang@gmail.com> wrote:

> On Wed, Jun 19, 2019 at 11:04:40AM +0200, Igor Mammedov wrote:
> >On Wed, 19 Jun 2019 14:20:50 +0800
> >Wei Yang <richardw.yang@linux.intel.com> wrote:
> >  
> >> On Tue, Jun 18, 2019 at 05:59:56PM +0200, Igor Mammedov wrote:  
> >> >
> >> >On Mon, 13 May 2019 14:19:04 +0800
> >> >Wei Yang <richardw.yang@linux.intel.com> wrote:
> >> >    
> >> >> Now MADT is highly depend in architecture and machine type and leaves
> >> >> duplicated code in different architecture. The series here tries to generalize
> >> >> it.
> >> >> 
> >> >> MADT contains one main table and several sub tables. These sub tables are
> >> >> highly related to architecture. Here we introduce one method to make it
> >> >> architecture agnostic.
> >> >> 
> >> >>   * each architecture define its sub-table implementation function in madt_sub
> >> >>   * introduces struct madt_input to collect sub table information and pass to
> >> >>     build_madt
> >> >> 
> >> >> By doing so, each architecture could prepare its own sub-table implementation
> >> >> and madt_input. And keep build_madt architecture agnostic.    
> >> >
> >> >I've skimmed over patches, and to me it looks mostly as code movement
> >> >without apparent benefits and probably a bit more complex than what we have now
> >> >(it might be ok cost if it simplifies MADT support for other boards).
> >> >
> >> >Before I do line by line review could you demonstrate what effect new way
> >> >to build MADT would have on arm/virt and i386/virt (from NEMU). So it would be
> >> >possible to estimate net benefits from new approach?
> >> >(PS: it doesn't have to be patches ready for merging, just a dirty hack
> >> >that would demonstrate adding MADT for new board using mad_sub[])
> >> >    
> >> 
> >> Per APIC spec 5.2.12, MADT contains a *main* table and several *sub* tables
> >> (Interrupt Controllere), so the idea is give a callback hook in
> >> AcpiDeviceIfClass for each table, including *main* and *sub* table.
> >> 
> >> Current AcpiDeviceIfClass has one callback pc_madt_cpu_entry for some *sub*
> >> tables, after replacing the AcpiDeviceIfClass will look like this:
> >> 
> >> typedef struct AcpiDeviceIfClass {
> >>     /* <private> */
> >>     InterfaceClass parent_class;
> >> 
> >>     /* <public> */
> >>     void (*ospm_status)(AcpiDeviceIf *adev, ACPIOSTInfoList ***list);
> >>     void (*send_event)(AcpiDeviceIf *adev, AcpiEventStatusBits ev);
> >> -   void (*madt_cpu)(AcpiDeviceIf *adev, int uid,
> >> -                    const CPUArchIdList *apic_ids, GArray *entry);
> >> +   madt_operation madt_main;
> >> +   madt_operation *madt_sub;
> >> } AcpiDeviceIfClass;
> >> 
> >> By doing so, each arch could have its own implementation for MADT.
> >> 
> >> After this refactoring, build_madt could be simplified to:
> >> 
> >> build_madt(GArray *table_data, BIOSLinker *linker, PCMachineState *pcms,
> >>            struct madt_input *input)
> >> {
> >>     ...
> >> 
> >>     if (adevc->madt_main) {
> >>         adevc->madt_main(table_data, madt);
> >>     }
> >> 
> >>     for (i = 0; ; i++) {
> >>         sub_id = input[i].sub_id;
> >>         if (sub_id == ACPI_APIC_RESERVED) {
> >>             break;
> >>         }
> >>         opaque = input[i].opaque;
> >>         adevc->madt_sub[sub_id](table_data, opaque);
> >>     }
> >> 
> >>     ...
> >> }
> >> 
> >> input is a list of data necessary to build *sub* table. Its details is also
> >> arch dependent.  
> >I've got general idea reading patches in this series.
> >As I've mentioned before it's hard to generalize MADT since it
> >mostly contains entries unique for target/board.
> >Goal here isn't generalizing at any cost, but rather find out
> >if there is enough common code to justify generalization
> >and if it allows us to reduce code duplication and simplify.
> >  
> >> For following new arch, what it need to do is prepare the input array and
> >> implement necessary *main*/*sub* table callbacks.  
> >What I'd like to see is the actual patch that does this,
> >to see if it has any merit and to compare to the current
> >approach.  
> 
> I didn't get some idea about your approach. Would you mind sharing more light?
With current approach, 'each board' has its own MADT build routine.
Considering that there is very little to share between different
implementations it might be ok.

This series just add extra data structure for board to populate
and a bunch of callbacks for every record type. Essentially all
the code we have now is still there. It was just moved elsewhere
and made available via callbacks.
This series touches only pc/q35 machines and it's not apparent
to me why it's any better than what we have now.

Re: [Qemu-devel] [RFC PATCH 0/9] hw/acpi: make build_madt arch agnostic

[Wei Yang]

On Thu, Jun 20, 2019 at 05:04:29PM +0200, Igor Mammedov wrote:
>On Thu, 20 Jun 2019 14:18:42 +0000
>Wei Yang <richard.weiyang@gmail.com> wrote:
>
>> On Wed, Jun 19, 2019 at 11:04:40AM +0200, Igor Mammedov wrote:
>> >On Wed, 19 Jun 2019 14:20:50 +0800
>> >Wei Yang <richardw.yang@linux.intel.com> wrote:
>> >  
>> >> On Tue, Jun 18, 2019 at 05:59:56PM +0200, Igor Mammedov wrote:  
>> >> >
>> >> >On Mon, 13 May 2019 14:19:04 +0800
>> >> >Wei Yang <richardw.yang@linux.intel.com> wrote:
>> >> >    
>> >> >> Now MADT is highly depend in architecture and machine type and leaves
>> >> >> duplicated code in different architecture. The series here tries to generalize
>> >> >> it.
>> >> >> 
>> >> >> MADT contains one main table and several sub tables. These sub tables are
>> >> >> highly related to architecture. Here we introduce one method to make it
>> >> >> architecture agnostic.
>> >> >> 
>> >> >>   * each architecture define its sub-table implementation function in madt_sub
>> >> >>   * introduces struct madt_input to collect sub table information and pass to
>> >> >>     build_madt
>> >> >> 
>> >> >> By doing so, each architecture could prepare its own sub-table implementation
>> >> >> and madt_input. And keep build_madt architecture agnostic.    
>> >> >
>> >> >I've skimmed over patches, and to me it looks mostly as code movement
>> >> >without apparent benefits and probably a bit more complex than what we have now
>> >> >(it might be ok cost if it simplifies MADT support for other boards).
>> >> >
>> >> >Before I do line by line review could you demonstrate what effect new way
>> >> >to build MADT would have on arm/virt and i386/virt (from NEMU). So it would be
>> >> >possible to estimate net benefits from new approach?
>> >> >(PS: it doesn't have to be patches ready for merging, just a dirty hack
>> >> >that would demonstrate adding MADT for new board using mad_sub[])
>> >> >    
>> >> 
>> >> Per APIC spec 5.2.12, MADT contains a *main* table and several *sub* tables
>> >> (Interrupt Controllere), so the idea is give a callback hook in
>> >> AcpiDeviceIfClass for each table, including *main* and *sub* table.
>> >> 
>> >> Current AcpiDeviceIfClass has one callback pc_madt_cpu_entry for some *sub*
>> >> tables, after replacing the AcpiDeviceIfClass will look like this:
>> >> 
>> >> typedef struct AcpiDeviceIfClass {
>> >>     /* <private> */
>> >>     InterfaceClass parent_class;
>> >> 
>> >>     /* <public> */
>> >>     void (*ospm_status)(AcpiDeviceIf *adev, ACPIOSTInfoList ***list);
>> >>     void (*send_event)(AcpiDeviceIf *adev, AcpiEventStatusBits ev);
>> >> -   void (*madt_cpu)(AcpiDeviceIf *adev, int uid,
>> >> -                    const CPUArchIdList *apic_ids, GArray *entry);
>> >> +   madt_operation madt_main;
>> >> +   madt_operation *madt_sub;
>> >> } AcpiDeviceIfClass;
>> >> 
>> >> By doing so, each arch could have its own implementation for MADT.
>> >> 
>> >> After this refactoring, build_madt could be simplified to:
>> >> 
>> >> build_madt(GArray *table_data, BIOSLinker *linker, PCMachineState *pcms,
>> >>            struct madt_input *input)
>> >> {
>> >>     ...
>> >> 
>> >>     if (adevc->madt_main) {
>> >>         adevc->madt_main(table_data, madt);
>> >>     }
>> >> 
>> >>     for (i = 0; ; i++) {
>> >>         sub_id = input[i].sub_id;
>> >>         if (sub_id == ACPI_APIC_RESERVED) {
>> >>             break;
>> >>         }
>> >>         opaque = input[i].opaque;
>> >>         adevc->madt_sub[sub_id](table_data, opaque);
>> >>     }
>> >> 
>> >>     ...
>> >> }
>> >> 
>> >> input is a list of data necessary to build *sub* table. Its details is also
>> >> arch dependent.  
>> >I've got general idea reading patches in this series.
>> >As I've mentioned before it's hard to generalize MADT since it
>> >mostly contains entries unique for target/board.
>> >Goal here isn't generalizing at any cost, but rather find out
>> >if there is enough common code to justify generalization
>> >and if it allows us to reduce code duplication and simplify.
>> >  
>> >> For following new arch, what it need to do is prepare the input array and
>> >> implement necessary *main*/*sub* table callbacks.  
>> >What I'd like to see is the actual patch that does this,
>> >to see if it has any merit and to compare to the current
>> >approach.  
>> 
>> I didn't get some idea about your approach. Would you mind sharing more light?
>With current approach, 'each board' has its own MADT build routine.
>Considering that there is very little to share between different
>implementations it might be ok.
>
>This series just add extra data structure for board to populate
>and a bunch of callbacks for every record type. Essentially all
>the code we have now is still there. It was just moved elsewhere
>and made available via callbacks.

Yes, you are right.

>This series touches only pc/q35 machines and it's not apparent
>to me why it's any better than what we have now.

This is the demo for i386. In case you think this approach is reasonable, it
could be applied to arm. And then for new board, we can apply the same
approach.

-- 
Wei Yang
Help you, Help me

Re: [Qemu-devel] [RFC PATCH 0/9] hw/acpi: make build_madt arch agnostic

[Igor Mammedov]

On Fri, 21 Jun 2019 08:56:44 +0800
Wei Yang <richardw.yang@linux.intel.com> wrote:

> On Thu, Jun 20, 2019 at 05:04:29PM +0200, Igor Mammedov wrote:
> >On Thu, 20 Jun 2019 14:18:42 +0000
> >Wei Yang <richard.weiyang@gmail.com> wrote:
> >  
> >> On Wed, Jun 19, 2019 at 11:04:40AM +0200, Igor Mammedov wrote:  
> >> >On Wed, 19 Jun 2019 14:20:50 +0800
> >> >Wei Yang <richardw.yang@linux.intel.com> wrote:
> >> >    
> >> >> On Tue, Jun 18, 2019 at 05:59:56PM +0200, Igor Mammedov wrote:    
> >> >> >
> >> >> >On Mon, 13 May 2019 14:19:04 +0800
> >> >> >Wei Yang <richardw.yang@linux.intel.com> wrote:
> >> >> >      
> >> >> >> Now MADT is highly depend in architecture and machine type and leaves
> >> >> >> duplicated code in different architecture. The series here tries to generalize
> >> >> >> it.
> >> >> >> 
> >> >> >> MADT contains one main table and several sub tables. These sub tables are
> >> >> >> highly related to architecture. Here we introduce one method to make it
> >> >> >> architecture agnostic.
> >> >> >> 
> >> >> >>   * each architecture define its sub-table implementation function in madt_sub
> >> >> >>   * introduces struct madt_input to collect sub table information and pass to
> >> >> >>     build_madt
> >> >> >> 
> >> >> >> By doing so, each architecture could prepare its own sub-table implementation
> >> >> >> and madt_input. And keep build_madt architecture agnostic.      
> >> >> >
> >> >> >I've skimmed over patches, and to me it looks mostly as code movement
> >> >> >without apparent benefits and probably a bit more complex than what we have now
> >> >> >(it might be ok cost if it simplifies MADT support for other boards).
> >> >> >
> >> >> >Before I do line by line review could you demonstrate what effect new way
> >> >> >to build MADT would have on arm/virt and i386/virt (from NEMU). So it would be
> >> >> >possible to estimate net benefits from new approach?
> >> >> >(PS: it doesn't have to be patches ready for merging, just a dirty hack
> >> >> >that would demonstrate adding MADT for new board using mad_sub[])
> >> >> >      
> >> >> 
> >> >> Per APIC spec 5.2.12, MADT contains a *main* table and several *sub* tables
> >> >> (Interrupt Controllere), so the idea is give a callback hook in
> >> >> AcpiDeviceIfClass for each table, including *main* and *sub* table.
> >> >> 
> >> >> Current AcpiDeviceIfClass has one callback pc_madt_cpu_entry for some *sub*
> >> >> tables, after replacing the AcpiDeviceIfClass will look like this:
> >> >> 
> >> >> typedef struct AcpiDeviceIfClass {
> >> >>     /* <private> */
> >> >>     InterfaceClass parent_class;
> >> >> 
> >> >>     /* <public> */
> >> >>     void (*ospm_status)(AcpiDeviceIf *adev, ACPIOSTInfoList ***list);
> >> >>     void (*send_event)(AcpiDeviceIf *adev, AcpiEventStatusBits ev);
> >> >> -   void (*madt_cpu)(AcpiDeviceIf *adev, int uid,
> >> >> -                    const CPUArchIdList *apic_ids, GArray *entry);
> >> >> +   madt_operation madt_main;
> >> >> +   madt_operation *madt_sub;
> >> >> } AcpiDeviceIfClass;
> >> >> 
> >> >> By doing so, each arch could have its own implementation for MADT.
> >> >> 
> >> >> After this refactoring, build_madt could be simplified to:
> >> >> 
> >> >> build_madt(GArray *table_data, BIOSLinker *linker, PCMachineState *pcms,
> >> >>            struct madt_input *input)
> >> >> {
> >> >>     ...
> >> >> 
> >> >>     if (adevc->madt_main) {
> >> >>         adevc->madt_main(table_data, madt);
> >> >>     }
> >> >> 
> >> >>     for (i = 0; ; i++) {
> >> >>         sub_id = input[i].sub_id;
> >> >>         if (sub_id == ACPI_APIC_RESERVED) {
> >> >>             break;
> >> >>         }
> >> >>         opaque = input[i].opaque;
> >> >>         adevc->madt_sub[sub_id](table_data, opaque);
> >> >>     }
> >> >> 
> >> >>     ...
> >> >> }
> >> >> 
> >> >> input is a list of data necessary to build *sub* table. Its details is also
> >> >> arch dependent.    
> >> >I've got general idea reading patches in this series.
> >> >As I've mentioned before it's hard to generalize MADT since it
> >> >mostly contains entries unique for target/board.
> >> >Goal here isn't generalizing at any cost, but rather find out
> >> >if there is enough common code to justify generalization
> >> >and if it allows us to reduce code duplication and simplify.
> >> >    
> >> >> For following new arch, what it need to do is prepare the input array and
> >> >> implement necessary *main*/*sub* table callbacks.    
> >> >What I'd like to see is the actual patch that does this,
> >> >to see if it has any merit and to compare to the current
> >> >approach.    
> >> 
> >> I didn't get some idea about your approach. Would you mind sharing more light?  
> >With current approach, 'each board' has its own MADT build routine.
> >Considering that there is very little to share between different
> >implementations it might be ok.
> >
> >This series just add extra data structure for board to populate
> >and a bunch of callbacks for every record type. Essentially all
> >the code we have now is still there. It was just moved elsewhere
> >and made available via callbacks.  
> 
> Yes, you are right.
> 
> >This series touches only pc/q35 machines and it's not apparent
> >to me why it's any better than what we have now.  
> 
> This is the demo for i386. In case you think this approach is reasonable, it
> could be applied to arm. And then for new board, we can apply the same
> approach.
well, it's not obvious from i386 demo, how it's any better than what
we have now. It lacks arm/virt patches so we could see if it would make
anything better or not.

If I were to talk about i386 demo alone, then I'd say it just makes
code more complex and I'd leave existing MADT code as it.

Re: [Qemu-devel] [RFC PATCH 0/9] hw/acpi: make build_madt arch agnostic

[Wei Yang]

On Fri, Jun 21, 2019 at 10:11:31AM +0200, Igor Mammedov wrote:
>On Fri, 21 Jun 2019 08:56:44 +0800
>Wei Yang <richardw.yang@linux.intel.com> wrote:
>
>> On Thu, Jun 20, 2019 at 05:04:29PM +0200, Igor Mammedov wrote:
>> >On Thu, 20 Jun 2019 14:18:42 +0000
>> >Wei Yang <richard.weiyang@gmail.com> wrote:
>> >  
>> >> On Wed, Jun 19, 2019 at 11:04:40AM +0200, Igor Mammedov wrote:  
>> >> >On Wed, 19 Jun 2019 14:20:50 +0800
>> >> >Wei Yang <richardw.yang@linux.intel.com> wrote:
>> >> >    
>> >> >> On Tue, Jun 18, 2019 at 05:59:56PM +0200, Igor Mammedov wrote:    
>> >> >> >
>> >> >> >On Mon, 13 May 2019 14:19:04 +0800
>> >> >> >Wei Yang <richardw.yang@linux.intel.com> wrote:
>> >> >> >      
>> >> >> >> Now MADT is highly depend in architecture and machine type and leaves
>> >> >> >> duplicated code in different architecture. The series here tries to generalize
>> >> >> >> it.
>> >> >> >> 
>> >> >> >> MADT contains one main table and several sub tables. These sub tables are
>> >> >> >> highly related to architecture. Here we introduce one method to make it
>> >> >> >> architecture agnostic.
>> >> >> >> 
>> >> >> >>   * each architecture define its sub-table implementation function in madt_sub
>> >> >> >>   * introduces struct madt_input to collect sub table information and pass to
>> >> >> >>     build_madt
>> >> >> >> 
>> >> >> >> By doing so, each architecture could prepare its own sub-table implementation
>> >> >> >> and madt_input. And keep build_madt architecture agnostic.      
>> >> >> >
>> >> >> >I've skimmed over patches, and to me it looks mostly as code movement
>> >> >> >without apparent benefits and probably a bit more complex than what we have now
>> >> >> >(it might be ok cost if it simplifies MADT support for other boards).
>> >> >> >
>> >> >> >Before I do line by line review could you demonstrate what effect new way
>> >> >> >to build MADT would have on arm/virt and i386/virt (from NEMU). So it would be
>> >> >> >possible to estimate net benefits from new approach?
>> >> >> >(PS: it doesn't have to be patches ready for merging, just a dirty hack
>> >> >> >that would demonstrate adding MADT for new board using mad_sub[])
>> >> >> >      
>> >> >> 
>> >> >> Per APIC spec 5.2.12, MADT contains a *main* table and several *sub* tables
>> >> >> (Interrupt Controllere), so the idea is give a callback hook in
>> >> >> AcpiDeviceIfClass for each table, including *main* and *sub* table.
>> >> >> 
>> >> >> Current AcpiDeviceIfClass has one callback pc_madt_cpu_entry for some *sub*
>> >> >> tables, after replacing the AcpiDeviceIfClass will look like this:
>> >> >> 
>> >> >> typedef struct AcpiDeviceIfClass {
>> >> >>     /* <private> */
>> >> >>     InterfaceClass parent_class;
>> >> >> 
>> >> >>     /* <public> */
>> >> >>     void (*ospm_status)(AcpiDeviceIf *adev, ACPIOSTInfoList ***list);
>> >> >>     void (*send_event)(AcpiDeviceIf *adev, AcpiEventStatusBits ev);
>> >> >> -   void (*madt_cpu)(AcpiDeviceIf *adev, int uid,
>> >> >> -                    const CPUArchIdList *apic_ids, GArray *entry);
>> >> >> +   madt_operation madt_main;
>> >> >> +   madt_operation *madt_sub;
>> >> >> } AcpiDeviceIfClass;
>> >> >> 
>> >> >> By doing so, each arch could have its own implementation for MADT.
>> >> >> 
>> >> >> After this refactoring, build_madt could be simplified to:
>> >> >> 
>> >> >> build_madt(GArray *table_data, BIOSLinker *linker, PCMachineState *pcms,
>> >> >>            struct madt_input *input)
>> >> >> {
>> >> >>     ...
>> >> >> 
>> >> >>     if (adevc->madt_main) {
>> >> >>         adevc->madt_main(table_data, madt);
>> >> >>     }
>> >> >> 
>> >> >>     for (i = 0; ; i++) {
>> >> >>         sub_id = input[i].sub_id;
>> >> >>         if (sub_id == ACPI_APIC_RESERVED) {
>> >> >>             break;
>> >> >>         }
>> >> >>         opaque = input[i].opaque;
>> >> >>         adevc->madt_sub[sub_id](table_data, opaque);
>> >> >>     }
>> >> >> 
>> >> >>     ...
>> >> >> }
>> >> >> 
>> >> >> input is a list of data necessary to build *sub* table. Its details is also
>> >> >> arch dependent.    
>> >> >I've got general idea reading patches in this series.
>> >> >As I've mentioned before it's hard to generalize MADT since it
>> >> >mostly contains entries unique for target/board.
>> >> >Goal here isn't generalizing at any cost, but rather find out
>> >> >if there is enough common code to justify generalization
>> >> >and if it allows us to reduce code duplication and simplify.
>> >> >    
>> >> >> For following new arch, what it need to do is prepare the input array and
>> >> >> implement necessary *main*/*sub* table callbacks.    
>> >> >What I'd like to see is the actual patch that does this,
>> >> >to see if it has any merit and to compare to the current
>> >> >approach.    
>> >> 
>> >> I didn't get some idea about your approach. Would you mind sharing more light?  
>> >With current approach, 'each board' has its own MADT build routine.
>> >Considering that there is very little to share between different
>> >implementations it might be ok.
>> >
>> >This series just add extra data structure for board to populate
>> >and a bunch of callbacks for every record type. Essentially all
>> >the code we have now is still there. It was just moved elsewhere
>> >and made available via callbacks.  
>> 
>> Yes, you are right.
>> 
>> >This series touches only pc/q35 machines and it's not apparent
>> >to me why it's any better than what we have now.  
>> 
>> This is the demo for i386. In case you think this approach is reasonable, it
>> could be applied to arm. And then for new board, we can apply the same
>> approach.
>well, it's not obvious from i386 demo, how it's any better than what
>we have now. It lacks arm/virt patches so we could see if it would make
>anything better or not.
>

ok, let me add arm/vrit part.

>If I were to talk about i386 demo alone, then I'd say it just makes
>code more complex and I'd leave existing MADT code as it.

-- 
Wei Yang
Help you, Help me