[PATCH v4 0/7] AML Housekeeping

[PATCH v4 0/7] AML Housekeeping

[Bernhard Beschow]

This series factors out AcpiCpuAmlIfClass::madt_cpu from AcpiDeviceIfClass.
By letting the (x86) CPUs implement the new interface, AML generation is
delegated to the CPUs, freeing the ACPI controllers from worrying about x86 CPU
specifics. The delegation to the CPUs is especially interesting for the PIIX4 PM
since it is also used in MIPS only contexts where no ACPI bios is available.

Furthermore, the series introduces qbus_build_aml() which replaces
isa_build_aml() and resolves some open coding.

v4:
- Squash qbus_build_aml() patches into one (Igor)
- Don't use a bare function pointer for AcpiDeviceIfClass::madt_cpu (Igor)

Testing done:
* `make check`
* `qemu-system-x86_64 -M pc -m 2G -cdrom manjaro-kde-21.2.6-220416-linux515.iso`
* `qemu-system-x86_64 -M q35 -m 2G -cdrom \
   manjaro-kde-21.2.6-220416-linux515.iso`

v3:
- Clean up includes in AcpiDeviceIfClass::madt_cpu sub series last (Markus)
- Restructure qbus_build_aml() sub series (Phil, me)

v2:
- Don't inline qbus_build_aml() (Phil)
- Add 'hw/acpi/piix4: No need to #include "hw/southbridge/piix.h"'

Bernhard Beschow (7):
  hw/i386/acpi-build: Remove unused attributes
  hw/isa/isa-bus: Turn isa_build_aml() into qbus_build_aml()
  hw/acpi/piix4: No need to #include "hw/southbridge/piix.h"
  hw/acpi/acpi_dev_interface: Remove unused parameter from
    AcpiDeviceIfClass::madt_cpu
  hw/acpi/acpi_dev_interface: Factor out TYPE_ACPI_CPU_AML_IF
  hw/acpi/acpi_dev_interface: Remove now unused #include "hw/boards.h"
  hw/i386/pc: Unexport pc_madt_cpu_entry()

 hw/acpi/hmat.h                       |  3 +-
 hw/i386/acpi-common.h                |  3 +-
 include/hw/acpi/acpi_aml_interface.h |  3 ++
 include/hw/acpi/acpi_cpu_interface.h | 26 ++++++++++++++++
 include/hw/acpi/acpi_dev_interface.h |  4 ---
 include/hw/i386/pc.h                 |  6 ----
 include/hw/isa/isa.h                 |  1 -
 hw/acpi/acpi-x86-stub.c              |  7 -----
 hw/acpi/acpi_interface.c             | 18 ++++++++++-
 hw/acpi/cpu.c                        | 13 ++++----
 hw/acpi/hmat.c                       |  1 +
 hw/acpi/memory_hotplug.c             |  1 +
 hw/acpi/piix4.c                      |  3 --
 hw/i2c/smbus_ich9.c                  |  5 +--
 hw/i386/acpi-build.c                 |  5 +--
 hw/i386/acpi-common.c                | 42 +++----------------------
 hw/i386/acpi-microvm.c               |  6 ++--
 hw/i386/generic_event_device_x86.c   |  9 ------
 hw/isa/isa-bus.c                     | 10 ------
 hw/isa/lpc_ich9.c                    |  6 +---
 hw/isa/piix3.c                       |  5 +--
 monitor/qmp-cmds.c                   |  1 +
 target/i386/cpu.c                    | 46 ++++++++++++++++++++++++++++
 23 files changed, 117 insertions(+), 107 deletions(-)
 create mode 100644 include/hw/acpi/acpi_cpu_interface.h

-- 
2.39.1

[PATCH v4 1/7] hw/i386/acpi-build: Remove unused attributes

[Bernhard Beschow]

Ammends commit 3db119da7915 'pc: acpi: switch to AML API composed DSDT'.

Signed-off-by: Bernhard Beschow <shentey@gmail.com>
Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>
Reviewed-by: Igor Mammedov <imammedo@redhat.com>
---
 hw/i386/acpi-build.c | 2 --
 1 file changed, 2 deletions(-)

diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
index 127c4e2d50..8c333973f9 100644
--- a/hw/i386/acpi-build.c
+++ b/hw/i386/acpi-build.c
@@ -117,8 +117,6 @@ typedef struct AcpiMiscInfo {
 #ifdef CONFIG_TPM
     TPMVersion tpm_version;
 #endif
-    const unsigned char *dsdt_code;
-    unsigned dsdt_size;
 } AcpiMiscInfo;
 
 typedef struct FwCfgTPMConfig {
-- 
2.39.1

[PATCH v4 2/7] hw/isa/isa-bus: Turn isa_build_aml() into qbus_build_aml()

[Bernhard Beschow]

Frees isa-bus.c from implicit ACPI dependency.

While at it, resolve open coding of qbus_build_aml() in piix3 and ich9.

Signed-off-by: Bernhard Beschow <shentey@gmail.com>
Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>
Reviewed-by: Igor Mammedov <imammedo@redhat.com>
---
 include/hw/acpi/acpi_aml_interface.h |  3 +++
 include/hw/isa/isa.h                 |  1 -
 hw/acpi/acpi_interface.c             | 10 ++++++++++
 hw/i2c/smbus_ich9.c                  |  5 +----
 hw/i386/acpi-microvm.c               |  3 ++-
 hw/isa/isa-bus.c                     | 10 ----------
 hw/isa/lpc_ich9.c                    |  5 +----
 hw/isa/piix3.c                       |  5 +----
 8 files changed, 18 insertions(+), 24 deletions(-)

diff --git a/include/hw/acpi/acpi_aml_interface.h b/include/hw/acpi/acpi_aml_interface.h
index 436da069d6..11748a8866 100644
--- a/include/hw/acpi/acpi_aml_interface.h
+++ b/include/hw/acpi/acpi_aml_interface.h
@@ -3,6 +3,7 @@
 
 #include "qom/object.h"
 #include "hw/acpi/aml-build.h"
+#include "hw/qdev-core.h"
 
 #define TYPE_ACPI_DEV_AML_IF "acpi-dev-aml-interface"
 typedef struct AcpiDevAmlIfClass AcpiDevAmlIfClass;
@@ -46,4 +47,6 @@ static inline void call_dev_aml_func(DeviceState *dev, Aml *scope)
     }
 }
 
+void qbus_build_aml(BusState *bus, Aml *scope);
+
 #endif
diff --git a/include/hw/isa/isa.h b/include/hw/isa/isa.h
index 6c8a8a92cb..25acd5c34c 100644
--- a/include/hw/isa/isa.h
+++ b/include/hw/isa/isa.h
@@ -86,7 +86,6 @@ bool isa_realize_and_unref(ISADevice *dev, ISABus *bus, Error **errp);
 ISADevice *isa_create_simple(ISABus *bus, const char *name);
 
 ISADevice *isa_vga_init(ISABus *bus);
-void isa_build_aml(ISABus *bus, Aml *scope);
 
 /**
  * isa_register_ioport: Install an I/O port region on the ISA bus.
diff --git a/hw/acpi/acpi_interface.c b/hw/acpi/acpi_interface.c
index c668d361f6..8637ff18fc 100644
--- a/hw/acpi/acpi_interface.c
+++ b/hw/acpi/acpi_interface.c
@@ -2,6 +2,7 @@
 #include "hw/acpi/acpi_dev_interface.h"
 #include "hw/acpi/acpi_aml_interface.h"
 #include "qemu/module.h"
+#include "qemu/queue.h"
 
 void acpi_send_event(DeviceState *dev, AcpiEventStatusBits event)
 {
@@ -12,6 +13,15 @@ void acpi_send_event(DeviceState *dev, AcpiEventStatusBits event)
     }
 }
 
+void qbus_build_aml(BusState *bus, Aml *scope)
+{
+    BusChild *kid;
+
+    QTAILQ_FOREACH(kid, &bus->children, sibling) {
+        call_dev_aml_func(DEVICE(kid->child), scope);
+    }
+}
+
 static void register_types(void)
 {
     static const TypeInfo acpi_dev_if_info = {
diff --git a/hw/i2c/smbus_ich9.c b/hw/i2c/smbus_ich9.c
index ee50ba1f2c..52ba77f3fc 100644
--- a/hw/i2c/smbus_ich9.c
+++ b/hw/i2c/smbus_ich9.c
@@ -97,13 +97,10 @@ static void ich9_smbus_realize(PCIDevice *d, Error **errp)
 
 static void build_ich9_smb_aml(AcpiDevAmlIf *adev, Aml *scope)
 {
-    BusChild *kid;
     ICH9SMBState *s = ICH9_SMB_DEVICE(adev);
     BusState *bus = BUS(s->smb.smbus);
 
-    QTAILQ_FOREACH(kid, &bus->children, sibling) {
-            call_dev_aml_func(DEVICE(kid->child), scope);
-    }
+    qbus_build_aml(bus, scope);
 }
 
 static void ich9_smb_class_init(ObjectClass *klass, void *data)
diff --git a/hw/i386/acpi-microvm.c b/hw/i386/acpi-microvm.c
index fb09185cbd..a075360d85 100644
--- a/hw/i386/acpi-microvm.c
+++ b/hw/i386/acpi-microvm.c
@@ -26,6 +26,7 @@
 
 #include "exec/memory.h"
 #include "hw/acpi/acpi.h"
+#include "hw/acpi/acpi_aml_interface.h"
 #include "hw/acpi/aml-build.h"
 #include "hw/acpi/bios-linker-loader.h"
 #include "hw/acpi/generic_event_device.h"
@@ -129,7 +130,7 @@ build_dsdt_microvm(GArray *table_data, BIOSLinker *linker,
 
     sb_scope = aml_scope("_SB");
     fw_cfg_add_acpi_dsdt(sb_scope, x86ms->fw_cfg);
-    isa_build_aml(ISA_BUS(isabus), sb_scope);
+    qbus_build_aml(BUS(isabus), sb_scope);
     build_ged_aml(sb_scope, GED_DEVICE, x86ms->acpi_dev,
                   GED_MMIO_IRQ, AML_SYSTEM_MEMORY, GED_MMIO_BASE);
     acpi_dsdt_add_power_button(sb_scope);
diff --git a/hw/isa/isa-bus.c b/hw/isa/isa-bus.c
index 1bee1a47f1..f155b80010 100644
--- a/hw/isa/isa-bus.c
+++ b/hw/isa/isa-bus.c
@@ -24,7 +24,6 @@
 #include "hw/sysbus.h"
 #include "sysemu/sysemu.h"
 #include "hw/isa/isa.h"
-#include "hw/acpi/acpi_aml_interface.h"
 
 static ISABus *isabus;
 
@@ -188,15 +187,6 @@ ISADevice *isa_vga_init(ISABus *bus)
     }
 }
 
-void isa_build_aml(ISABus *bus, Aml *scope)
-{
-    BusChild *kid;
-
-    QTAILQ_FOREACH(kid, &bus->parent_obj.children, sibling) {
-        call_dev_aml_func(DEVICE(kid->child), scope);
-    }
-}
-
 static void isabus_bridge_class_init(ObjectClass *klass, void *data)
 {
     DeviceClass *dc = DEVICE_CLASS(klass);
diff --git a/hw/isa/lpc_ich9.c b/hw/isa/lpc_ich9.c
index 8d541e2b54..1fba3c210c 100644
--- a/hw/isa/lpc_ich9.c
+++ b/hw/isa/lpc_ich9.c
@@ -813,7 +813,6 @@ static void ich9_send_gpe(AcpiDeviceIf *adev, AcpiEventStatusBits ev)
 static void build_ich9_isa_aml(AcpiDevAmlIf *adev, Aml *scope)
 {
     Aml *field;
-    BusChild *kid;
     ICH9LPCState *s = ICH9_LPC_DEVICE(adev);
     BusState *bus = BUS(s->isa_bus);
     Aml *sb_scope = aml_scope("\\_SB");
@@ -835,9 +834,7 @@ static void build_ich9_isa_aml(AcpiDevAmlIf *adev, Aml *scope)
     aml_append(sb_scope, field);
     aml_append(scope, sb_scope);
 
-    QTAILQ_FOREACH(kid, &bus->children, sibling) {
-            call_dev_aml_func(DEVICE(kid->child), scope);
-    }
+    qbus_build_aml(bus, scope);
 }
 
 static void ich9_lpc_class_init(ObjectClass *klass, void *data)
diff --git a/hw/isa/piix3.c b/hw/isa/piix3.c
index 283b971ec4..a9cb39bf21 100644
--- a/hw/isa/piix3.c
+++ b/hw/isa/piix3.c
@@ -306,7 +306,6 @@ static void pci_piix3_realize(PCIDevice *dev, Error **errp)
 static void build_pci_isa_aml(AcpiDevAmlIf *adev, Aml *scope)
 {
     Aml *field;
-    BusChild *kid;
     Aml *sb_scope = aml_scope("\\_SB");
     BusState *bus = qdev_get_child_bus(DEVICE(adev), "isa.0");
 
@@ -322,9 +321,7 @@ static void build_pci_isa_aml(AcpiDevAmlIf *adev, Aml *scope)
     aml_append(sb_scope, field);
     aml_append(scope, sb_scope);
 
-    QTAILQ_FOREACH(kid, &bus->children, sibling) {
-        call_dev_aml_func(DEVICE(kid->child), scope);
-    }
+    qbus_build_aml(bus, scope);
 }
 
 static void pci_piix3_class_init(ObjectClass *klass, void *data)
-- 
2.39.1

[PATCH v4 3/7] hw/acpi/piix4: No need to #include "hw/southbridge/piix.h"

[Bernhard Beschow]

hw/acpi/piix4 has its own header with its structure definition etc.

Ammends commit 2bfd0845f0 'hw/acpi/piix4: move PIIX4PMState into
separate piix4.h header'.

Signed-off-by: Bernhard Beschow <shentey@gmail.com>
Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>
---
 hw/acpi/piix4.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/hw/acpi/piix4.c b/hw/acpi/piix4.c
index 0a81f1ad93..2ab4930f11 100644
--- a/hw/acpi/piix4.c
+++ b/hw/acpi/piix4.c
@@ -21,7 +21,6 @@
 
 #include "qemu/osdep.h"
 #include "hw/i386/pc.h"
-#include "hw/southbridge/piix.h"
 #include "hw/irq.h"
 #include "hw/isa/apm.h"
 #include "hw/i2c/pm_smbus.h"
-- 
2.39.1

[PATCH v4 4/7] hw/acpi/acpi_dev_interface: Remove unused parameter from AcpiDeviceIfClass::madt_cpu

[Bernhard Beschow]

The only function ever assigned to AcpiDeviceIfClass::madt_cpu is
pc_madt_cpu_entry() which doesn't use the AcpiDeviceIf parameter.

Signed-off-by: Bernhard Beschow <shentey@gmail.com>
Reviewed-by: Igor Mammedov <imammedo@redhat.com>
---
 include/hw/acpi/acpi_dev_interface.h | 3 +--
 include/hw/i386/pc.h                 | 6 ++----
 hw/acpi/acpi-x86-stub.c              | 5 ++---
 hw/acpi/cpu.c                        | 3 +--
 hw/i386/acpi-common.c                | 7 +++----
 5 files changed, 9 insertions(+), 15 deletions(-)

diff --git a/include/hw/acpi/acpi_dev_interface.h b/include/hw/acpi/acpi_dev_interface.h
index ea6056ab92..a1648220ff 100644
--- a/include/hw/acpi/acpi_dev_interface.h
+++ b/include/hw/acpi/acpi_dev_interface.h
@@ -52,8 +52,7 @@ 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,
+    void (*madt_cpu)(int uid, const CPUArchIdList *apic_ids, GArray *entry,
                      bool force_enabled);
 };
 #endif
diff --git a/include/hw/i386/pc.h b/include/hw/i386/pc.h
index 88a120bc23..66e3d059ef 100644
--- a/include/hw/i386/pc.h
+++ b/include/hw/i386/pc.h
@@ -9,7 +9,6 @@
 #include "hw/block/flash.h"
 #include "hw/i386/x86.h"
 
-#include "hw/acpi/acpi_dev_interface.h"
 #include "hw/hotplug.h"
 #include "qom/object.h"
 #include "hw/i386/sgx-epc.h"
@@ -193,9 +192,8 @@ bool pc_system_ovmf_table_find(const char *entry, uint8_t **data,
 void pc_system_parse_ovmf_flash(uint8_t *flash_ptr, size_t flash_size);
 
 /* hw/i386/acpi-common.c */
-void pc_madt_cpu_entry(AcpiDeviceIf *adev, int uid,
-                       const CPUArchIdList *apic_ids, GArray *entry,
-                       bool force_enabled);
+void pc_madt_cpu_entry(int uid, const CPUArchIdList *apic_ids,
+                       GArray *entry, bool force_enabled);
 
 /* sgx.c */
 void pc_machine_init_sgx_epc(PCMachineState *pcms);
diff --git a/hw/acpi/acpi-x86-stub.c b/hw/acpi/acpi-x86-stub.c
index 3df1e090f4..d0d399d26b 100644
--- a/hw/acpi/acpi-x86-stub.c
+++ b/hw/acpi/acpi-x86-stub.c
@@ -2,9 +2,8 @@
 #include "hw/i386/pc.h"
 #include "hw/i386/acpi-build.h"
 
-void pc_madt_cpu_entry(AcpiDeviceIf *adev, int uid,
-                       const CPUArchIdList *apic_ids, GArray *entry,
-                       bool force_enabled)
+void pc_madt_cpu_entry(int uid, const CPUArchIdList *apic_ids,
+                       GArray *entry, bool force_enabled)
 {
 }
 
diff --git a/hw/acpi/cpu.c b/hw/acpi/cpu.c
index 4e580959a2..19c154d78f 100644
--- a/hw/acpi/cpu.c
+++ b/hw/acpi/cpu.c
@@ -355,7 +355,6 @@ 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);
 
     cpu_ctrl_dev = aml_device("%s", cphp_res_path);
     {
@@ -666,7 +665,7 @@ void build_cpus_aml(Aml *table, MachineState *machine, CPUHotplugFeatures opts,
 
             /* build _MAT object */
             assert(adevc && adevc->madt_cpu);
-            adevc->madt_cpu(adev, i, arch_ids, madt_buf,
+            adevc->madt_cpu(i, arch_ids, madt_buf,
                             true); /* set enabled flag */
             aml_append(dev, aml_name_decl("_MAT",
                 aml_buffer(madt_buf->len, (uint8_t *)madt_buf->data)));
diff --git a/hw/i386/acpi-common.c b/hw/i386/acpi-common.c
index 4aaafbdd7b..52e5c1439a 100644
--- a/hw/i386/acpi-common.c
+++ b/hw/i386/acpi-common.c
@@ -33,9 +33,8 @@
 #include "acpi-build.h"
 #include "acpi-common.h"
 
-void pc_madt_cpu_entry(AcpiDeviceIf *adev, int uid,
-                       const CPUArchIdList *apic_ids, GArray *entry,
-                       bool force_enabled)
+void pc_madt_cpu_entry(int uid, const CPUArchIdList *apic_ids,
+                       GArray *entry, bool force_enabled)
 {
     uint32_t apic_id = apic_ids->cpus[uid].arch_id;
     /* Flags – Local APIC Flags */
@@ -112,7 +111,7 @@ void acpi_build_madt(GArray *table_data, BIOSLinker *linker,
     build_append_int_noprefix(table_data, 1 /* PCAT_COMPAT */, 4); /* Flags */
 
     for (i = 0; i < apic_ids->len; i++) {
-        adevc->madt_cpu(adev, i, apic_ids, table_data, false);
+        adevc->madt_cpu(i, apic_ids, table_data, false);
         if (apic_ids->cpus[i].arch_id > 254) {
             x2apic_mode = true;
         }
-- 
2.39.1

[PATCH v4 5/7] hw/acpi/acpi_dev_interface: Factor out TYPE_ACPI_CPU_AML_IF

[Bernhard Beschow]

This class attribute was always set to pc_madt_cpu_entry().
pc_madt_cpu_entry() is architecture dependent and was assigned to the
attribute even in architecture agnostic code such as in hw/acpi/piix4.c
and hw/isa/lpc_ich9. Not having to set madt_cpu there resolves the
assumption that these device models are only ever used with ACPI on x86
targets.

The only target independent location where madt_cpu was called was hw/
acpi/cpu.c. Here a function pointer can be passed via an argument
instead. The other locations where it was called was in x86-specific code
where pc_madt_cpu_entry() can be used directly.

While at it, move pc_madt_cpu_entry() from the public include/hw/i386/
pc.h to the private hw/i386/acpi-common where it is also implemented.

Signed-off-by: Bernhard Beschow <shentey@gmail.com>

TYPE_ACPI_CPU_IF
---
 hw/i386/acpi-common.h                |  3 +--
 include/hw/acpi/acpi_cpu_interface.h | 26 ++++++++++++++++++++++++++
 include/hw/acpi/acpi_dev_interface.h |  2 --
 hw/acpi/acpi_interface.c             |  8 +++++++-
 hw/acpi/cpu.c                        | 11 ++++++-----
 hw/acpi/piix4.c                      |  2 --
 hw/i386/acpi-build.c                 |  3 +--
 hw/i386/acpi-common.c                |  8 +++++---
 hw/i386/acpi-microvm.c               |  3 +--
 hw/i386/generic_event_device_x86.c   |  9 ---------
 hw/isa/lpc_ich9.c                    |  1 -
 target/i386/cpu.c                    | 13 +++++++++++++
 12 files changed, 60 insertions(+), 29 deletions(-)
 create mode 100644 include/hw/acpi/acpi_cpu_interface.h

diff --git a/hw/i386/acpi-common.h b/hw/i386/acpi-common.h
index a68825acf5..b3c56ee014 100644
--- a/hw/i386/acpi-common.h
+++ b/hw/i386/acpi-common.h
@@ -1,7 +1,6 @@
 #ifndef HW_I386_ACPI_COMMON_H
 #define HW_I386_ACPI_COMMON_H
 
-#include "hw/acpi/acpi_dev_interface.h"
 #include "hw/acpi/bios-linker-loader.h"
 #include "hw/i386/x86.h"
 
@@ -9,7 +8,7 @@
 #define ACPI_BUILD_IOAPIC_ID 0x0
 
 void acpi_build_madt(GArray *table_data, BIOSLinker *linker,
-                     X86MachineState *x86ms, AcpiDeviceIf *adev,
+                     X86MachineState *x86ms,
                      const char *oem_id, const char *oem_table_id);
 
 #endif
diff --git a/include/hw/acpi/acpi_cpu_interface.h b/include/hw/acpi/acpi_cpu_interface.h
new file mode 100644
index 0000000000..600f0b68cd
--- /dev/null
+++ b/include/hw/acpi/acpi_cpu_interface.h
@@ -0,0 +1,26 @@
+#ifndef ACPI_CPU_INTERFACE_H
+#define ACPI_CPU_INTERFACE_H
+
+#include "qom/object.h"
+#include "hw/boards.h"
+#include "hw/qdev-core.h"
+
+#define TYPE_ACPI_CPU_AML_IF "acpi-cpu-aml-interface"
+
+typedef struct AcpiCpuAmlIfClass AcpiCpuAmlIfClass;
+DECLARE_CLASS_CHECKERS(AcpiCpuAmlIfClass, ACPI_CPU_AML_IF,
+                       TYPE_ACPI_CPU_AML_IF)
+#define ACPI_CPU_AML_IF(obj) \
+    INTERFACE_CHECK(AcpiCpuAmlIf, (obj), TYPE_ACPI_CPU_AML_IF)
+
+typedef struct AcpiCpuAmlIf AcpiCpuAmlIf;
+
+struct AcpiCpuAmlIfClass {
+    /* <private> */
+    InterfaceClass parent_class;
+
+    /* <public> */
+    void (*madt_cpu)(int uid, const CPUArchIdList *apic_ids, GArray *entry,
+                     bool force_enabled);
+};
+#endif
diff --git a/include/hw/acpi/acpi_dev_interface.h b/include/hw/acpi/acpi_dev_interface.h
index a1648220ff..ca92928124 100644
--- a/include/hw/acpi/acpi_dev_interface.h
+++ b/include/hw/acpi/acpi_dev_interface.h
@@ -52,7 +52,5 @@ struct AcpiDeviceIfClass {
     /* <public> */
     void (*ospm_status)(AcpiDeviceIf *adev, ACPIOSTInfoList ***list);
     void (*send_event)(AcpiDeviceIf *adev, AcpiEventStatusBits ev);
-    void (*madt_cpu)(int uid, const CPUArchIdList *apic_ids, GArray *entry,
-                     bool force_enabled);
 };
 #endif
diff --git a/hw/acpi/acpi_interface.c b/hw/acpi/acpi_interface.c
index 8637ff18fc..11a57e2154 100644
--- a/hw/acpi/acpi_interface.c
+++ b/hw/acpi/acpi_interface.c
@@ -1,4 +1,5 @@
 #include "qemu/osdep.h"
+#include "hw/acpi/acpi_cpu_interface.h"
 #include "hw/acpi/acpi_dev_interface.h"
 #include "hw/acpi/acpi_aml_interface.h"
 #include "qemu/module.h"
@@ -34,10 +35,15 @@ static void register_types(void)
         .parent        = TYPE_INTERFACE,
         .class_size = sizeof(AcpiDevAmlIfClass),
     };
-
+    static const TypeInfo acpi_cpu_aml_if_info = {
+        .name          = TYPE_ACPI_CPU_AML_IF,
+        .parent        = TYPE_INTERFACE,
+        .class_size = sizeof(AcpiCpuAmlIfClass),
+    };
 
     type_register_static(&acpi_dev_if_info);
     type_register_static(&acpi_dev_aml_if_info);
+    type_register_static(&acpi_cpu_aml_if_info);
 }
 
 type_init(register_types)
diff --git a/hw/acpi/cpu.c b/hw/acpi/cpu.c
index 19c154d78f..f6647e99b1 100644
--- a/hw/acpi/cpu.c
+++ b/hw/acpi/cpu.c
@@ -1,5 +1,6 @@
 #include "qemu/osdep.h"
 #include "migration/vmstate.h"
+#include "hw/acpi/acpi_cpu_interface.h"
 #include "hw/acpi/cpu.h"
 #include "qapi/error.h"
 #include "qapi/qapi-events-acpi.h"
@@ -353,8 +354,6 @@ void build_cpus_aml(Aml *table, MachineState *machine, CPUHotplugFeatures opts,
     MachineClass *mc = MACHINE_GET_CLASS(machine);
     const CPUArchIdList *arch_ids = mc->possible_cpu_arch_ids(machine);
     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);
 
     cpu_ctrl_dev = aml_device("%s", cphp_res_path);
     {
@@ -648,6 +647,8 @@ void build_cpus_aml(Aml *table, MachineState *machine, CPUHotplugFeatures opts,
         for (i = 0; i < arch_ids->len; i++) {
             Aml *dev;
             Aml *uid = aml_int(i);
+            ObjectClass *oc = object_class_by_name(arch_ids->cpus[i].type);
+            AcpiCpuAmlIfClass *acpuac = ACPI_CPU_AML_IF_CLASS(oc);
             GArray *madt_buf = g_array_new(0, 1, 1);
             int arch_id = arch_ids->cpus[i].arch_id;
 
@@ -664,9 +665,9 @@ 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(i, arch_ids, madt_buf,
-                            true); /* set enabled flag */
+            assert(acpuac && acpuac->madt_cpu);
+            acpuac->madt_cpu(i, arch_ids, madt_buf,
+                             true); /* set enabled flag */
             aml_append(dev, aml_name_decl("_MAT",
                 aml_buffer(madt_buf->len, (uint8_t *)madt_buf->data)));
             g_array_free(madt_buf, true);
diff --git a/hw/acpi/piix4.c b/hw/acpi/piix4.c
index 2ab4930f11..2e19a55526 100644
--- a/hw/acpi/piix4.c
+++ b/hw/acpi/piix4.c
@@ -20,7 +20,6 @@
  */
 
 #include "qemu/osdep.h"
-#include "hw/i386/pc.h"
 #include "hw/irq.h"
 #include "hw/isa/apm.h"
 #include "hw/i2c/pm_smbus.h"
@@ -642,7 +641,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 8c333973f9..b12a843447 100644
--- a/hw/i386/acpi-build.c
+++ b/hw/i386/acpi-build.c
@@ -2422,8 +2422,7 @@ void acpi_build(AcpiBuildTables *tables, MachineState *machine)
 
     acpi_add_table(table_offsets, tables_blob);
     acpi_build_madt(tables_blob, tables->linker, x86ms,
-                    ACPI_DEVICE_IF(x86ms->acpi_dev), x86ms->oem_id,
-                    x86ms->oem_table_id);
+                    x86ms->oem_id, x86ms->oem_table_id);
 
 #ifdef CONFIG_ACPI_ERST
     {
diff --git a/hw/i386/acpi-common.c b/hw/i386/acpi-common.c
index 52e5c1439a..0d1a2bb8aa 100644
--- a/hw/i386/acpi-common.c
+++ b/hw/i386/acpi-common.c
@@ -25,6 +25,7 @@
 
 #include "exec/memory.h"
 #include "hw/acpi/acpi.h"
+#include "hw/acpi/acpi_cpu_interface.h"
 #include "hw/acpi/aml-build.h"
 #include "hw/acpi/utils.h"
 #include "hw/i386/pc.h"
@@ -94,14 +95,13 @@ build_xrupt_override(GArray *entry, uint8_t src, uint32_t gsi, uint16_t flags)
  * 5.2.8 Multiple APIC Description Table
  */
 void acpi_build_madt(GArray *table_data, BIOSLinker *linker,
-                     X86MachineState *x86ms, AcpiDeviceIf *adev,
+                     X86MachineState *x86ms,
                      const char *oem_id, const char *oem_table_id)
 {
     int i;
     bool x2apic_mode = false;
     MachineClass *mc = MACHINE_GET_CLASS(x86ms);
     const CPUArchIdList *apic_ids = mc->possible_cpu_arch_ids(MACHINE(x86ms));
-    AcpiDeviceIfClass *adevc = ACPI_DEVICE_IF_GET_CLASS(adev);
     AcpiTable table = { .sig = "APIC", .rev = 1, .oem_id = oem_id,
                         .oem_table_id = oem_table_id };
 
@@ -111,7 +111,9 @@ void acpi_build_madt(GArray *table_data, BIOSLinker *linker,
     build_append_int_noprefix(table_data, 1 /* PCAT_COMPAT */, 4); /* Flags */
 
     for (i = 0; i < apic_ids->len; i++) {
-        adevc->madt_cpu(i, apic_ids, table_data, false);
+        ObjectClass *oc = object_class_by_name(apic_ids->cpus[i].type);
+        AcpiCpuAmlIfClass *acpuac = ACPI_CPU_AML_IF_CLASS(oc);
+        acpuac->madt_cpu(i, apic_ids, table_data, false);
         if (apic_ids->cpus[i].arch_id > 254) {
             x2apic_mode = true;
         }
diff --git a/hw/i386/acpi-microvm.c b/hw/i386/acpi-microvm.c
index a075360d85..fec22d85c1 100644
--- a/hw/i386/acpi-microvm.c
+++ b/hw/i386/acpi-microvm.c
@@ -214,8 +214,7 @@ static void acpi_build_microvm(AcpiBuildTables *tables,
 
     acpi_add_table(table_offsets, tables_blob);
     acpi_build_madt(tables_blob, tables->linker, X86_MACHINE(machine),
-                    ACPI_DEVICE_IF(x86ms->acpi_dev), x86ms->oem_id,
-                    x86ms->oem_table_id);
+                    x86ms->oem_id, x86ms->oem_table_id);
 
 #ifdef CONFIG_ACPI_ERST
     {
diff --git a/hw/i386/generic_event_device_x86.c b/hw/i386/generic_event_device_x86.c
index e26fb02a2e..8fc233e1f1 100644
--- a/hw/i386/generic_event_device_x86.c
+++ b/hw/i386/generic_event_device_x86.c
@@ -8,19 +8,10 @@
 
 #include "qemu/osdep.h"
 #include "hw/acpi/generic_event_device.h"
-#include "hw/i386/pc.h"
-
-static void acpi_ged_x86_class_init(ObjectClass *class, void *data)
-{
-    AcpiDeviceIfClass *adevc = ACPI_DEVICE_IF_CLASS(class);
-
-    adevc->madt_cpu = pc_madt_cpu_entry;
-}
 
 static const TypeInfo acpi_ged_x86_info = {
     .name          = TYPE_ACPI_GED_X86,
     .parent        = TYPE_ACPI_GED,
-    .class_init    = acpi_ged_x86_class_init,
     .interfaces = (InterfaceInfo[]) {
         { TYPE_HOTPLUG_HANDLER },
         { TYPE_ACPI_DEVICE_IF },
diff --git a/hw/isa/lpc_ich9.c b/hw/isa/lpc_ich9.c
index 1fba3c210c..d5d4b0f177 100644
--- a/hw/isa/lpc_ich9.c
+++ b/hw/isa/lpc_ich9.c
@@ -867,7 +867,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;
     amldevc->build_dev_aml = build_ich9_isa_aml;
 }
 
diff --git a/target/i386/cpu.c b/target/i386/cpu.c
index 4d2b8d0444..6ac50506a7 100644
--- a/target/i386/cpu.c
+++ b/target/i386/cpu.c
@@ -37,7 +37,9 @@
 #include "hw/i386/topology.h"
 #ifndef CONFIG_USER_ONLY
 #include "exec/address-spaces.h"
+#include "hw/acpi/acpi_cpu_interface.h"
 #include "hw/boards.h"
+#include "hw/i386/pc.h"
 #include "hw/i386/sgx-epc.h"
 #endif
 
@@ -7114,6 +7116,9 @@ static void x86_cpu_common_class_init(ObjectClass *oc, void *data)
     CPUClass *cc = CPU_CLASS(oc);
     DeviceClass *dc = DEVICE_CLASS(oc);
     ResettableClass *rc = RESETTABLE_CLASS(oc);
+#ifndef CONFIG_USER_ONLY
+    AcpiCpuAmlIfClass *acpuac = ACPI_CPU_AML_IF_CLASS(oc);
+#endif
     FeatureWord w;
 
     device_class_set_parent_realize(dc, x86_cpu_realizefn,
@@ -7138,6 +7143,7 @@ static void x86_cpu_common_class_init(ObjectClass *oc, void *data)
 
 #ifndef CONFIG_USER_ONLY
     cc->sysemu_ops = &i386_sysemu_ops;
+    acpuac->madt_cpu = pc_madt_cpu_entry;
 #endif /* !CONFIG_USER_ONLY */
 
     cc->gdb_arch_name = x86_gdb_arch_name;
@@ -7203,6 +7209,13 @@ static const TypeInfo x86_cpu_type_info = {
     .abstract = true,
     .class_size = sizeof(X86CPUClass),
     .class_init = x86_cpu_common_class_init,
+
+#ifndef CONFIG_USER_ONLY
+    .interfaces = (InterfaceInfo[]) {
+        { TYPE_ACPI_CPU_AML_IF },
+        { }
+    }
+#endif
 };
 
 /* "base" CPU model, used by query-cpu-model-expansion */
-- 
2.39.1

[PATCH v4 6/7] hw/acpi/acpi_dev_interface: Remove now unused #include "hw/boards.h"

[Bernhard Beschow]

Removing the "hw/boards.h" include from hw/acpi/acpi_dev_interface.h
requires include fixes in various unrelated files to keep the code
compiling.

Signed-off-by: Bernhard Beschow <shentey@gmail.com>
---
 hw/acpi/hmat.h                       | 3 ++-
 include/hw/acpi/acpi_dev_interface.h | 1 -
 hw/acpi/cpu.c                        | 1 +
 hw/acpi/hmat.c                       | 1 +
 hw/acpi/memory_hotplug.c             | 1 +
 monitor/qmp-cmds.c                   | 1 +
 6 files changed, 6 insertions(+), 2 deletions(-)

diff --git a/hw/acpi/hmat.h b/hw/acpi/hmat.h
index b57f0e7e80..fd989cb661 100644
--- a/hw/acpi/hmat.h
+++ b/hw/acpi/hmat.h
@@ -27,7 +27,8 @@
 #ifndef HMAT_H
 #define HMAT_H
 
-#include "hw/acpi/aml-build.h"
+#include "hw/acpi/bios-linker-loader.h"
+#include "sysemu/numa.h"
 
 /*
  * ACPI 6.3: 5.2.27.3 Memory Proximity Domain Attributes Structure,
diff --git a/include/hw/acpi/acpi_dev_interface.h b/include/hw/acpi/acpi_dev_interface.h
index ca92928124..68d9d15f50 100644
--- a/include/hw/acpi/acpi_dev_interface.h
+++ b/include/hw/acpi/acpi_dev_interface.h
@@ -3,7 +3,6 @@
 
 #include "qapi/qapi-types-acpi.h"
 #include "qom/object.h"
-#include "hw/boards.h"
 #include "hw/qdev-core.h"
 
 /* These values are part of guest ABI, and can not be changed */
diff --git a/hw/acpi/cpu.c b/hw/acpi/cpu.c
index f6647e99b1..1a7eb54c98 100644
--- a/hw/acpi/cpu.c
+++ b/hw/acpi/cpu.c
@@ -2,6 +2,7 @@
 #include "migration/vmstate.h"
 #include "hw/acpi/acpi_cpu_interface.h"
 #include "hw/acpi/cpu.h"
+#include "hw/core/cpu.h"
 #include "qapi/error.h"
 #include "qapi/qapi-events-acpi.h"
 #include "trace.h"
diff --git a/hw/acpi/hmat.c b/hw/acpi/hmat.c
index 3a6d51282a..d9de0daf89 100644
--- a/hw/acpi/hmat.c
+++ b/hw/acpi/hmat.c
@@ -27,6 +27,7 @@
 #include "qemu/osdep.h"
 #include "qemu/units.h"
 #include "sysemu/numa.h"
+#include "hw/acpi/aml-build.h"
 #include "hw/acpi/hmat.h"
 
 /*
diff --git a/hw/acpi/memory_hotplug.c b/hw/acpi/memory_hotplug.c
index d926f4f77d..0b883df813 100644
--- a/hw/acpi/memory_hotplug.c
+++ b/hw/acpi/memory_hotplug.c
@@ -1,6 +1,7 @@
 #include "qemu/osdep.h"
 #include "hw/acpi/memory_hotplug.h"
 #include "hw/mem/pc-dimm.h"
+#include "hw/boards.h"
 #include "hw/qdev-core.h"
 #include "migration/vmstate.h"
 #include "trace.h"
diff --git a/monitor/qmp-cmds.c b/monitor/qmp-cmds.c
index bf22a8c5a6..051b825986 100644
--- a/monitor/qmp-cmds.c
+++ b/monitor/qmp-cmds.c
@@ -41,6 +41,7 @@
 #include "hw/acpi/acpi_dev_interface.h"
 #include "hw/intc/intc.h"
 #include "hw/rdma/rdma.h"
+#include "hw/boards.h"
 #include "monitor/stats.h"
 
 NameInfo *qmp_query_name(Error **errp)
-- 
2.39.1

[PATCH v4 7/7] hw/i386/pc: Unexport pc_madt_cpu_entry()

[Bernhard Beschow]

pc_madt_cpu_entry() is now only used in target/i386/cpu, so move it
there, and unexport and rename it.

Signed-off-by: Bernhard Beschow <shentey@gmail.com>
---
 include/hw/i386/pc.h    |  4 ----
 hw/acpi/acpi-x86-stub.c |  6 ------
 hw/i386/acpi-common.c   | 33 ---------------------------------
 target/i386/cpu.c       | 37 +++++++++++++++++++++++++++++++++++--
 4 files changed, 35 insertions(+), 45 deletions(-)

diff --git a/include/hw/i386/pc.h b/include/hw/i386/pc.h
index 66e3d059ef..9ab1818812 100644
--- a/include/hw/i386/pc.h
+++ b/include/hw/i386/pc.h
@@ -191,10 +191,6 @@ bool pc_system_ovmf_table_find(const char *entry, uint8_t **data,
                                int *data_len);
 void pc_system_parse_ovmf_flash(uint8_t *flash_ptr, size_t flash_size);
 
-/* hw/i386/acpi-common.c */
-void pc_madt_cpu_entry(int uid, const CPUArchIdList *apic_ids,
-                       GArray *entry, bool force_enabled);
-
 /* sgx.c */
 void pc_machine_init_sgx_epc(PCMachineState *pcms);
 
diff --git a/hw/acpi/acpi-x86-stub.c b/hw/acpi/acpi-x86-stub.c
index d0d399d26b..9662a594ad 100644
--- a/hw/acpi/acpi-x86-stub.c
+++ b/hw/acpi/acpi-x86-stub.c
@@ -1,12 +1,6 @@
 #include "qemu/osdep.h"
-#include "hw/i386/pc.h"
 #include "hw/i386/acpi-build.h"
 
-void pc_madt_cpu_entry(int uid, const CPUArchIdList *apic_ids,
-                       GArray *entry, bool force_enabled)
-{
-}
-
 Object *acpi_get_i386_pci_host(void)
 {
        return NULL;
diff --git a/hw/i386/acpi-common.c b/hw/i386/acpi-common.c
index 0d1a2bb8aa..0041623aeb 100644
--- a/hw/i386/acpi-common.c
+++ b/hw/i386/acpi-common.c
@@ -28,44 +28,11 @@
 #include "hw/acpi/acpi_cpu_interface.h"
 #include "hw/acpi/aml-build.h"
 #include "hw/acpi/utils.h"
-#include "hw/i386/pc.h"
 #include "target/i386/cpu.h"
 
 #include "acpi-build.h"
 #include "acpi-common.h"
 
-void pc_madt_cpu_entry(int uid, const CPUArchIdList *apic_ids,
-                       GArray *entry, bool force_enabled)
-{
-    uint32_t apic_id = apic_ids->cpus[uid].arch_id;
-    /* Flags – Local APIC Flags */
-    uint32_t flags = apic_ids->cpus[uid].cpu != NULL || force_enabled ?
-                     1 /* Enabled */ : 0;
-
-    /* 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) {
-        /* Rev 1.0b, Table 5-13 Processor Local APIC Structure */
-        build_append_int_noprefix(entry, 0, 1);       /* Type */
-        build_append_int_noprefix(entry, 8, 1);       /* Length */
-        build_append_int_noprefix(entry, uid, 1);     /* ACPI Processor ID */
-        build_append_int_noprefix(entry, apic_id, 1); /* APIC ID */
-        build_append_int_noprefix(entry, flags, 4); /* Flags */
-    } else {
-        /* Rev 4.0, 5.2.12.12 Processor Local x2APIC Structure */
-        build_append_int_noprefix(entry, 9, 1);       /* Type */
-        build_append_int_noprefix(entry, 16, 1);      /* Length */
-        build_append_int_noprefix(entry, 0, 2);       /* Reserved */
-        build_append_int_noprefix(entry, apic_id, 4); /* X2APIC ID */
-        build_append_int_noprefix(entry, flags, 4);   /* Flags */
-        build_append_int_noprefix(entry, uid, 4);     /* ACPI Processor UID */
-    }
-}
-
 static void build_ioapic(GArray *entry, uint8_t id, uint32_t addr, uint32_t irq)
 {
     /* Rev 1.0b, 5.2.8.2 IO APIC */
diff --git a/target/i386/cpu.c b/target/i386/cpu.c
index 6ac50506a7..b05062bc57 100644
--- a/target/i386/cpu.c
+++ b/target/i386/cpu.c
@@ -38,8 +38,8 @@
 #ifndef CONFIG_USER_ONLY
 #include "exec/address-spaces.h"
 #include "hw/acpi/acpi_cpu_interface.h"
+#include "hw/acpi/aml-build.h"
 #include "hw/boards.h"
-#include "hw/i386/pc.h"
 #include "hw/i386/sgx-epc.h"
 #endif
 
@@ -7108,6 +7108,39 @@ static const struct SysemuCPUOps i386_sysemu_ops = {
     .write_elf64_qemunote = x86_cpu_write_elf64_qemunote,
     .legacy_vmsd = &vmstate_x86_cpu,
 };
+
+static void x86_madt_cpu_entry(int uid, const CPUArchIdList *apic_ids,
+                               GArray *entry, bool force_enabled)
+{
+    uint32_t apic_id = apic_ids->cpus[uid].arch_id;
+    /* Flags – Local APIC Flags */
+    uint32_t flags = apic_ids->cpus[uid].cpu != NULL || force_enabled ?
+                     1 /* Enabled */ : 0;
+
+    /*
+     * 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) {
+        /* Rev 1.0b, Table 5-13 Processor Local APIC Structure */
+        build_append_int_noprefix(entry, 0, 1);       /* Type */
+        build_append_int_noprefix(entry, 8, 1);       /* Length */
+        build_append_int_noprefix(entry, uid, 1);     /* ACPI Processor ID */
+        build_append_int_noprefix(entry, apic_id, 1); /* APIC ID */
+        build_append_int_noprefix(entry, flags, 4); /* Flags */
+    } else {
+        /* Rev 4.0, 5.2.12.12 Processor Local x2APIC Structure */
+        build_append_int_noprefix(entry, 9, 1);       /* Type */
+        build_append_int_noprefix(entry, 16, 1);      /* Length */
+        build_append_int_noprefix(entry, 0, 2);       /* Reserved */
+        build_append_int_noprefix(entry, apic_id, 4); /* X2APIC ID */
+        build_append_int_noprefix(entry, flags, 4);   /* Flags */
+        build_append_int_noprefix(entry, uid, 4);     /* ACPI Processor UID */
+    }
+}
 #endif
 
 static void x86_cpu_common_class_init(ObjectClass *oc, void *data)
@@ -7143,7 +7176,7 @@ static void x86_cpu_common_class_init(ObjectClass *oc, void *data)
 
 #ifndef CONFIG_USER_ONLY
     cc->sysemu_ops = &i386_sysemu_ops;
-    acpuac->madt_cpu = pc_madt_cpu_entry;
+    acpuac->madt_cpu = x86_madt_cpu_entry;
 #endif /* !CONFIG_USER_ONLY */
 
     cc->gdb_arch_name = x86_gdb_arch_name;
-- 
2.39.1

Re: [PATCH v4 5/7] hw/acpi/acpi_dev_interface: Factor out TYPE_ACPI_CPU_AML_IF

[Igor Mammedov]

On Sat, 21 Jan 2023 16:19:39 +0100
Bernhard Beschow <shentey@gmail.com> wrote:

> This class attribute was always set to pc_madt_cpu_entry().
> pc_madt_cpu_entry() is architecture dependent and was assigned to the
> attribute even in architecture agnostic code such as in hw/acpi/piix4.c
> and hw/isa/lpc_ich9. Not having to set madt_cpu there resolves the
> assumption that these device models are only ever used with ACPI on x86
> targets.

While it would work, I don't see a compelling reason to drop
AcpiDeviceIf::madt_cpu and cause all this churn and extra code.

Simple pc_madt_cpu_entry() stub works just fine for targets
that don't need madt_cpu (it's not elegant but, it's the way
such issues a typically dealt with).

If goal is to clean up piix4 from *all* x86 extensions and have
pure pixx4 and make sure other targets do not pull in not needed
ACPI dependencies, then this patch should be a part of a series
that does all of that to demonstrate that achievable without
breaking migration.

So far I'm not convinced that this patch has merit on its own.
I suggest to drop it for now.

> The only target independent location where madt_cpu was called was hw/
> acpi/cpu.c. Here a function pointer can be passed via an argument
> instead. The other locations where it was called was in x86-specific code
> where pc_madt_cpu_entry() can be used directly.
this part diverged from what this patch does.

> While at it, move pc_madt_cpu_entry() from the public include/hw/i386/
> pc.h to the private hw/i386/acpi-common where it is also implemented.

PS:
  usage of 'ifdef's is not welcome in new code,
  unless *there is no other way* around that.
 

> Signed-off-by: Bernhard Beschow <shentey@gmail.com>
> 
> TYPE_ACPI_CPU_IF
> ---
>  hw/i386/acpi-common.h                |  3 +--
>  include/hw/acpi/acpi_cpu_interface.h | 26 ++++++++++++++++++++++++++
>  include/hw/acpi/acpi_dev_interface.h |  2 --
>  hw/acpi/acpi_interface.c             |  8 +++++++-
>  hw/acpi/cpu.c                        | 11 ++++++-----
>  hw/acpi/piix4.c                      |  2 --
>  hw/i386/acpi-build.c                 |  3 +--
>  hw/i386/acpi-common.c                |  8 +++++---
>  hw/i386/acpi-microvm.c               |  3 +--
>  hw/i386/generic_event_device_x86.c   |  9 ---------
>  hw/isa/lpc_ich9.c                    |  1 -
>  target/i386/cpu.c                    | 13 +++++++++++++
>  12 files changed, 60 insertions(+), 29 deletions(-)
>  create mode 100644 include/hw/acpi/acpi_cpu_interface.h
> 
> diff --git a/hw/i386/acpi-common.h b/hw/i386/acpi-common.h
> index a68825acf5..b3c56ee014 100644
> --- a/hw/i386/acpi-common.h
> +++ b/hw/i386/acpi-common.h
> @@ -1,7 +1,6 @@
>  #ifndef HW_I386_ACPI_COMMON_H
>  #define HW_I386_ACPI_COMMON_H
>  
> -#include "hw/acpi/acpi_dev_interface.h"
>  #include "hw/acpi/bios-linker-loader.h"
>  #include "hw/i386/x86.h"
>  
> @@ -9,7 +8,7 @@
>  #define ACPI_BUILD_IOAPIC_ID 0x0
>  
>  void acpi_build_madt(GArray *table_data, BIOSLinker *linker,
> -                     X86MachineState *x86ms, AcpiDeviceIf *adev,
> +                     X86MachineState *x86ms,
>                       const char *oem_id, const char *oem_table_id);
>  
>  #endif
> diff --git a/include/hw/acpi/acpi_cpu_interface.h b/include/hw/acpi/acpi_cpu_interface.h
> new file mode 100644
> index 0000000000..600f0b68cd
> --- /dev/null
> +++ b/include/hw/acpi/acpi_cpu_interface.h
> @@ -0,0 +1,26 @@
> +#ifndef ACPI_CPU_INTERFACE_H
> +#define ACPI_CPU_INTERFACE_H
> +
> +#include "qom/object.h"
> +#include "hw/boards.h"
> +#include "hw/qdev-core.h"
> +
> +#define TYPE_ACPI_CPU_AML_IF "acpi-cpu-aml-interface"
> +
> +typedef struct AcpiCpuAmlIfClass AcpiCpuAmlIfClass;
> +DECLARE_CLASS_CHECKERS(AcpiCpuAmlIfClass, ACPI_CPU_AML_IF,
> +                       TYPE_ACPI_CPU_AML_IF)
> +#define ACPI_CPU_AML_IF(obj) \
> +    INTERFACE_CHECK(AcpiCpuAmlIf, (obj), TYPE_ACPI_CPU_AML_IF)
> +
> +typedef struct AcpiCpuAmlIf AcpiCpuAmlIf;
> +
> +struct AcpiCpuAmlIfClass {
> +    /* <private> */
> +    InterfaceClass parent_class;
> +
> +    /* <public> */
> +    void (*madt_cpu)(int uid, const CPUArchIdList *apic_ids, GArray *entry,
> +                     bool force_enabled);
> +};
> +#endif
> diff --git a/include/hw/acpi/acpi_dev_interface.h b/include/hw/acpi/acpi_dev_interface.h
> index a1648220ff..ca92928124 100644
> --- a/include/hw/acpi/acpi_dev_interface.h
> +++ b/include/hw/acpi/acpi_dev_interface.h
> @@ -52,7 +52,5 @@ struct AcpiDeviceIfClass {
>      /* <public> */
>      void (*ospm_status)(AcpiDeviceIf *adev, ACPIOSTInfoList ***list);
>      void (*send_event)(AcpiDeviceIf *adev, AcpiEventStatusBits ev);
> -    void (*madt_cpu)(int uid, const CPUArchIdList *apic_ids, GArray *entry,
> -                     bool force_enabled);
>  };
>  #endif
> diff --git a/hw/acpi/acpi_interface.c b/hw/acpi/acpi_interface.c
> index 8637ff18fc..11a57e2154 100644
> --- a/hw/acpi/acpi_interface.c
> +++ b/hw/acpi/acpi_interface.c
> @@ -1,4 +1,5 @@
>  #include "qemu/osdep.h"
> +#include "hw/acpi/acpi_cpu_interface.h"
>  #include "hw/acpi/acpi_dev_interface.h"
>  #include "hw/acpi/acpi_aml_interface.h"
>  #include "qemu/module.h"
> @@ -34,10 +35,15 @@ static void register_types(void)
>          .parent        = TYPE_INTERFACE,
>          .class_size = sizeof(AcpiDevAmlIfClass),
>      };
> -
> +    static const TypeInfo acpi_cpu_aml_if_info = {
> +        .name          = TYPE_ACPI_CPU_AML_IF,
> +        .parent        = TYPE_INTERFACE,
> +        .class_size = sizeof(AcpiCpuAmlIfClass),
> +    };
>  
>      type_register_static(&acpi_dev_if_info);
>      type_register_static(&acpi_dev_aml_if_info);
> +    type_register_static(&acpi_cpu_aml_if_info);
>  }
>  
>  type_init(register_types)
> diff --git a/hw/acpi/cpu.c b/hw/acpi/cpu.c
> index 19c154d78f..f6647e99b1 100644
> --- a/hw/acpi/cpu.c
> +++ b/hw/acpi/cpu.c
> @@ -1,5 +1,6 @@
>  #include "qemu/osdep.h"
>  #include "migration/vmstate.h"
> +#include "hw/acpi/acpi_cpu_interface.h"
>  #include "hw/acpi/cpu.h"
>  #include "qapi/error.h"
>  #include "qapi/qapi-events-acpi.h"
> @@ -353,8 +354,6 @@ void build_cpus_aml(Aml *table, MachineState *machine, CPUHotplugFeatures opts,
>      MachineClass *mc = MACHINE_GET_CLASS(machine);
>      const CPUArchIdList *arch_ids = mc->possible_cpu_arch_ids(machine);
>      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);
>  
>      cpu_ctrl_dev = aml_device("%s", cphp_res_path);
>      {
> @@ -648,6 +647,8 @@ void build_cpus_aml(Aml *table, MachineState *machine, CPUHotplugFeatures opts,
>          for (i = 0; i < arch_ids->len; i++) {
>              Aml *dev;
>              Aml *uid = aml_int(i);
> +            ObjectClass *oc = object_class_by_name(arch_ids->cpus[i].type);
> +            AcpiCpuAmlIfClass *acpuac = ACPI_CPU_AML_IF_CLASS(oc);
>              GArray *madt_buf = g_array_new(0, 1, 1);
>              int arch_id = arch_ids->cpus[i].arch_id;
>  
> @@ -664,9 +665,9 @@ 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(i, arch_ids, madt_buf,
> -                            true); /* set enabled flag */
> +            assert(acpuac && acpuac->madt_cpu);
> +            acpuac->madt_cpu(i, arch_ids, madt_buf,
> +                             true); /* set enabled flag */
>              aml_append(dev, aml_name_decl("_MAT",
>                  aml_buffer(madt_buf->len, (uint8_t *)madt_buf->data)));
>              g_array_free(madt_buf, true);
> diff --git a/hw/acpi/piix4.c b/hw/acpi/piix4.c
> index 2ab4930f11..2e19a55526 100644
> --- a/hw/acpi/piix4.c
> +++ b/hw/acpi/piix4.c
> @@ -20,7 +20,6 @@
>   */
>  
>  #include "qemu/osdep.h"
> -#include "hw/i386/pc.h"
>  #include "hw/irq.h"
>  #include "hw/isa/apm.h"
>  #include "hw/i2c/pm_smbus.h"
> @@ -642,7 +641,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 8c333973f9..b12a843447 100644
> --- a/hw/i386/acpi-build.c
> +++ b/hw/i386/acpi-build.c
> @@ -2422,8 +2422,7 @@ void acpi_build(AcpiBuildTables *tables, MachineState *machine)
>  
>      acpi_add_table(table_offsets, tables_blob);
>      acpi_build_madt(tables_blob, tables->linker, x86ms,
> -                    ACPI_DEVICE_IF(x86ms->acpi_dev), x86ms->oem_id,
> -                    x86ms->oem_table_id);
> +                    x86ms->oem_id, x86ms->oem_table_id);
>  
>  #ifdef CONFIG_ACPI_ERST
>      {
> diff --git a/hw/i386/acpi-common.c b/hw/i386/acpi-common.c
> index 52e5c1439a..0d1a2bb8aa 100644
> --- a/hw/i386/acpi-common.c
> +++ b/hw/i386/acpi-common.c
> @@ -25,6 +25,7 @@
>  
>  #include "exec/memory.h"
>  #include "hw/acpi/acpi.h"
> +#include "hw/acpi/acpi_cpu_interface.h"
>  #include "hw/acpi/aml-build.h"
>  #include "hw/acpi/utils.h"
>  #include "hw/i386/pc.h"
> @@ -94,14 +95,13 @@ build_xrupt_override(GArray *entry, uint8_t src, uint32_t gsi, uint16_t flags)
>   * 5.2.8 Multiple APIC Description Table
>   */
>  void acpi_build_madt(GArray *table_data, BIOSLinker *linker,
> -                     X86MachineState *x86ms, AcpiDeviceIf *adev,
> +                     X86MachineState *x86ms,
>                       const char *oem_id, const char *oem_table_id)
>  {
>      int i;
>      bool x2apic_mode = false;
>      MachineClass *mc = MACHINE_GET_CLASS(x86ms);
>      const CPUArchIdList *apic_ids = mc->possible_cpu_arch_ids(MACHINE(x86ms));
> -    AcpiDeviceIfClass *adevc = ACPI_DEVICE_IF_GET_CLASS(adev);
>      AcpiTable table = { .sig = "APIC", .rev = 1, .oem_id = oem_id,
>                          .oem_table_id = oem_table_id };
>  
> @@ -111,7 +111,9 @@ void acpi_build_madt(GArray *table_data, BIOSLinker *linker,
>      build_append_int_noprefix(table_data, 1 /* PCAT_COMPAT */, 4); /* Flags */
>  
>      for (i = 0; i < apic_ids->len; i++) {
> -        adevc->madt_cpu(i, apic_ids, table_data, false);
> +        ObjectClass *oc = object_class_by_name(apic_ids->cpus[i].type);
> +        AcpiCpuAmlIfClass *acpuac = ACPI_CPU_AML_IF_CLASS(oc);
> +        acpuac->madt_cpu(i, apic_ids, table_data, false);
>          if (apic_ids->cpus[i].arch_id > 254) {
>              x2apic_mode = true;
>          }
> diff --git a/hw/i386/acpi-microvm.c b/hw/i386/acpi-microvm.c
> index a075360d85..fec22d85c1 100644
> --- a/hw/i386/acpi-microvm.c
> +++ b/hw/i386/acpi-microvm.c
> @@ -214,8 +214,7 @@ static void acpi_build_microvm(AcpiBuildTables *tables,
>  
>      acpi_add_table(table_offsets, tables_blob);
>      acpi_build_madt(tables_blob, tables->linker, X86_MACHINE(machine),
> -                    ACPI_DEVICE_IF(x86ms->acpi_dev), x86ms->oem_id,
> -                    x86ms->oem_table_id);
> +                    x86ms->oem_id, x86ms->oem_table_id);
>  
>  #ifdef CONFIG_ACPI_ERST
>      {
> diff --git a/hw/i386/generic_event_device_x86.c b/hw/i386/generic_event_device_x86.c
> index e26fb02a2e..8fc233e1f1 100644
> --- a/hw/i386/generic_event_device_x86.c
> +++ b/hw/i386/generic_event_device_x86.c
> @@ -8,19 +8,10 @@
>  
>  #include "qemu/osdep.h"
>  #include "hw/acpi/generic_event_device.h"
> -#include "hw/i386/pc.h"
> -
> -static void acpi_ged_x86_class_init(ObjectClass *class, void *data)
> -{
> -    AcpiDeviceIfClass *adevc = ACPI_DEVICE_IF_CLASS(class);
> -
> -    adevc->madt_cpu = pc_madt_cpu_entry;
> -}
>  
>  static const TypeInfo acpi_ged_x86_info = {
>      .name          = TYPE_ACPI_GED_X86,
>      .parent        = TYPE_ACPI_GED,
> -    .class_init    = acpi_ged_x86_class_init,
>      .interfaces = (InterfaceInfo[]) {
>          { TYPE_HOTPLUG_HANDLER },
>          { TYPE_ACPI_DEVICE_IF },
> diff --git a/hw/isa/lpc_ich9.c b/hw/isa/lpc_ich9.c
> index 1fba3c210c..d5d4b0f177 100644
> --- a/hw/isa/lpc_ich9.c
> +++ b/hw/isa/lpc_ich9.c
> @@ -867,7 +867,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;
>      amldevc->build_dev_aml = build_ich9_isa_aml;
>  }
>  
> diff --git a/target/i386/cpu.c b/target/i386/cpu.c
> index 4d2b8d0444..6ac50506a7 100644
> --- a/target/i386/cpu.c
> +++ b/target/i386/cpu.c
> @@ -37,7 +37,9 @@
>  #include "hw/i386/topology.h"
>  #ifndef CONFIG_USER_ONLY
>  #include "exec/address-spaces.h"
> +#include "hw/acpi/acpi_cpu_interface.h"
>  #include "hw/boards.h"
> +#include "hw/i386/pc.h"
>  #include "hw/i386/sgx-epc.h"
>  #endif
>  
> @@ -7114,6 +7116,9 @@ static void x86_cpu_common_class_init(ObjectClass *oc, void *data)
>      CPUClass *cc = CPU_CLASS(oc);
>      DeviceClass *dc = DEVICE_CLASS(oc);
>      ResettableClass *rc = RESETTABLE_CLASS(oc);
> +#ifndef CONFIG_USER_ONLY
> +    AcpiCpuAmlIfClass *acpuac = ACPI_CPU_AML_IF_CLASS(oc);
> +#endif
>      FeatureWord w;
>  
>      device_class_set_parent_realize(dc, x86_cpu_realizefn,
> @@ -7138,6 +7143,7 @@ static void x86_cpu_common_class_init(ObjectClass *oc, void *data)
>  
>  #ifndef CONFIG_USER_ONLY
>      cc->sysemu_ops = &i386_sysemu_ops;
> +    acpuac->madt_cpu = pc_madt_cpu_entry;
>  #endif /* !CONFIG_USER_ONLY */
>  
>      cc->gdb_arch_name = x86_gdb_arch_name;
> @@ -7203,6 +7209,13 @@ static const TypeInfo x86_cpu_type_info = {
>      .abstract = true,
>      .class_size = sizeof(X86CPUClass),
>      .class_init = x86_cpu_common_class_init,
> +
> +#ifndef CONFIG_USER_ONLY
> +    .interfaces = (InterfaceInfo[]) {
> +        { TYPE_ACPI_CPU_AML_IF },
> +        { }
> +    }
> +#endif
>  };
>  
>  /* "base" CPU model, used by query-cpu-model-expansion */

Re: [PATCH v4 0/7] AML Housekeeping

[Igor Mammedov]

On Sat, 21 Jan 2023 16:19:34 +0100
Bernhard Beschow <shentey@gmail.com> wrote:

> This series factors out AcpiCpuAmlIfClass::madt_cpu from AcpiDeviceIfClass.
> By letting the (x86) CPUs implement the new interface, AML generation is
> delegated to the CPUs, freeing the ACPI controllers from worrying about x86 CPU
> specifics. The delegation to the CPUs is especially interesting for the PIIX4 PM
> since it is also used in MIPS only contexts where no ACPI bios is available.
> 
> Furthermore, the series introduces qbus_build_aml() which replaces
> isa_build_aml() and resolves some open coding.

I'm done with this series review
(skipped 6-7/7, since they depend on 5/7 which seems unnecessary to me)

> 
> v4:
> - Squash qbus_build_aml() patches into one (Igor)
> - Don't use a bare function pointer for AcpiDeviceIfClass::madt_cpu (Igor)
> 
> Testing done:
> * `make check`
> * `qemu-system-x86_64 -M pc -m 2G -cdrom manjaro-kde-21.2.6-220416-linux515.iso`
> * `qemu-system-x86_64 -M q35 -m 2G -cdrom \
>    manjaro-kde-21.2.6-220416-linux515.iso`
> 
> v3:
> - Clean up includes in AcpiDeviceIfClass::madt_cpu sub series last (Markus)
> - Restructure qbus_build_aml() sub series (Phil, me)
> 
> v2:
> - Don't inline qbus_build_aml() (Phil)
> - Add 'hw/acpi/piix4: No need to #include "hw/southbridge/piix.h"'
> 
> Bernhard Beschow (7):
>   hw/i386/acpi-build: Remove unused attributes
>   hw/isa/isa-bus: Turn isa_build_aml() into qbus_build_aml()
>   hw/acpi/piix4: No need to #include "hw/southbridge/piix.h"
>   hw/acpi/acpi_dev_interface: Remove unused parameter from
>     AcpiDeviceIfClass::madt_cpu
>   hw/acpi/acpi_dev_interface: Factor out TYPE_ACPI_CPU_AML_IF
>   hw/acpi/acpi_dev_interface: Remove now unused #include "hw/boards.h"
>   hw/i386/pc: Unexport pc_madt_cpu_entry()
> 
>  hw/acpi/hmat.h                       |  3 +-
>  hw/i386/acpi-common.h                |  3 +-
>  include/hw/acpi/acpi_aml_interface.h |  3 ++
>  include/hw/acpi/acpi_cpu_interface.h | 26 ++++++++++++++++
>  include/hw/acpi/acpi_dev_interface.h |  4 ---
>  include/hw/i386/pc.h                 |  6 ----
>  include/hw/isa/isa.h                 |  1 -
>  hw/acpi/acpi-x86-stub.c              |  7 -----
>  hw/acpi/acpi_interface.c             | 18 ++++++++++-
>  hw/acpi/cpu.c                        | 13 ++++----
>  hw/acpi/hmat.c                       |  1 +
>  hw/acpi/memory_hotplug.c             |  1 +
>  hw/acpi/piix4.c                      |  3 --
>  hw/i2c/smbus_ich9.c                  |  5 +--
>  hw/i386/acpi-build.c                 |  5 +--
>  hw/i386/acpi-common.c                | 42 +++----------------------
>  hw/i386/acpi-microvm.c               |  6 ++--
>  hw/i386/generic_event_device_x86.c   |  9 ------
>  hw/isa/isa-bus.c                     | 10 ------
>  hw/isa/lpc_ich9.c                    |  6 +---
>  hw/isa/piix3.c                       |  5 +--
>  monitor/qmp-cmds.c                   |  1 +
>  target/i386/cpu.c                    | 46 ++++++++++++++++++++++++++++
>  23 files changed, 117 insertions(+), 107 deletions(-)
>  create mode 100644 include/hw/acpi/acpi_cpu_interface.h
> 

Re: [PATCH v4 0/7] AML Housekeeping

[Bernhard Beschow]

Am 25. Januar 2023 16:52:34 UTC schrieb Igor Mammedov <imammedo@redhat.com>:
>On Sat, 21 Jan 2023 16:19:34 +0100
>Bernhard Beschow <shentey@gmail.com> wrote:
>
>> This series factors out AcpiCpuAmlIfClass::madt_cpu from AcpiDeviceIfClass.
>> By letting the (x86) CPUs implement the new interface, AML generation is
>> delegated to the CPUs, freeing the ACPI controllers from worrying about x86 CPU
>> specifics. The delegation to the CPUs is especially interesting for the PIIX4 PM
>> since it is also used in MIPS only contexts where no ACPI bios is available.
>> 
>> Furthermore, the series introduces qbus_build_aml() which replaces
>> isa_build_aml() and resolves some open coding.
>
>I'm done with this series review
>(skipped 6-7/7, since they depend on 5/7 which seems unnecessary to me)

Thanks!

Okay, let's omit patches 5-7 for now. It makes sense to include them in a dedicated x86 cleanup series.

Michael, shall I respin a v5 with only the reviewed patches?

Best regards,
Bernhard
>
>> 
>> v4:
>> - Squash qbus_build_aml() patches into one (Igor)
>> - Don't use a bare function pointer for AcpiDeviceIfClass::madt_cpu (Igor)
>> 
>> Testing done:
>> * `make check`
>> * `qemu-system-x86_64 -M pc -m 2G -cdrom manjaro-kde-21.2.6-220416-linux515.iso`
>> * `qemu-system-x86_64 -M q35 -m 2G -cdrom \
>>    manjaro-kde-21.2.6-220416-linux515.iso`
>> 
>> v3:
>> - Clean up includes in AcpiDeviceIfClass::madt_cpu sub series last (Markus)
>> - Restructure qbus_build_aml() sub series (Phil, me)
>> 
>> v2:
>> - Don't inline qbus_build_aml() (Phil)
>> - Add 'hw/acpi/piix4: No need to #include "hw/southbridge/piix.h"'
>> 
>> Bernhard Beschow (7):
>>   hw/i386/acpi-build: Remove unused attributes
>>   hw/isa/isa-bus: Turn isa_build_aml() into qbus_build_aml()
>>   hw/acpi/piix4: No need to #include "hw/southbridge/piix.h"
>>   hw/acpi/acpi_dev_interface: Remove unused parameter from
>>     AcpiDeviceIfClass::madt_cpu
>>   hw/acpi/acpi_dev_interface: Factor out TYPE_ACPI_CPU_AML_IF
>>   hw/acpi/acpi_dev_interface: Remove now unused #include "hw/boards.h"
>>   hw/i386/pc: Unexport pc_madt_cpu_entry()
>> 
>>  hw/acpi/hmat.h                       |  3 +-
>>  hw/i386/acpi-common.h                |  3 +-
>>  include/hw/acpi/acpi_aml_interface.h |  3 ++
>>  include/hw/acpi/acpi_cpu_interface.h | 26 ++++++++++++++++
>>  include/hw/acpi/acpi_dev_interface.h |  4 ---
>>  include/hw/i386/pc.h                 |  6 ----
>>  include/hw/isa/isa.h                 |  1 -
>>  hw/acpi/acpi-x86-stub.c              |  7 -----
>>  hw/acpi/acpi_interface.c             | 18 ++++++++++-
>>  hw/acpi/cpu.c                        | 13 ++++----
>>  hw/acpi/hmat.c                       |  1 +
>>  hw/acpi/memory_hotplug.c             |  1 +
>>  hw/acpi/piix4.c                      |  3 --
>>  hw/i2c/smbus_ich9.c                  |  5 +--
>>  hw/i386/acpi-build.c                 |  5 +--
>>  hw/i386/acpi-common.c                | 42 +++----------------------
>>  hw/i386/acpi-microvm.c               |  6 ++--
>>  hw/i386/generic_event_device_x86.c   |  9 ------
>>  hw/isa/isa-bus.c                     | 10 ------
>>  hw/isa/lpc_ich9.c                    |  6 +---
>>  hw/isa/piix3.c                       |  5 +--
>>  monitor/qmp-cmds.c                   |  1 +
>>  target/i386/cpu.c                    | 46 ++++++++++++++++++++++++++++
>>  23 files changed, 117 insertions(+), 107 deletions(-)
>>  create mode 100644 include/hw/acpi/acpi_cpu_interface.h
>> 
>

Re: [PATCH v4 0/7] AML Housekeeping

[Michael S. Tsirkin]

On Thu, Jan 26, 2023 at 10:42:31AM +0000, Bernhard Beschow wrote:
> 
> 
> Am 25. Januar 2023 16:52:34 UTC schrieb Igor Mammedov <imammedo@redhat.com>:
> >On Sat, 21 Jan 2023 16:19:34 +0100
> >Bernhard Beschow <shentey@gmail.com> wrote:
> >
> >> This series factors out AcpiCpuAmlIfClass::madt_cpu from AcpiDeviceIfClass.
> >> By letting the (x86) CPUs implement the new interface, AML generation is
> >> delegated to the CPUs, freeing the ACPI controllers from worrying about x86 CPU
> >> specifics. The delegation to the CPUs is especially interesting for the PIIX4 PM
> >> since it is also used in MIPS only contexts where no ACPI bios is available.
> >> 
> >> Furthermore, the series introduces qbus_build_aml() which replaces
> >> isa_build_aml() and resolves some open coding.
> >
> >I'm done with this series review
> >(skipped 6-7/7, since they depend on 5/7 which seems unnecessary to me)
> 
> Thanks!
> 
> Okay, let's omit patches 5-7 for now. It makes sense to include them in a dedicated x86 cleanup series.
> 
> Michael, shall I respin a v5 with only the reviewed patches?
> 
> Best regards,
> Bernhard

No need.

> >
> >> 
> >> v4:
> >> - Squash qbus_build_aml() patches into one (Igor)
> >> - Don't use a bare function pointer for AcpiDeviceIfClass::madt_cpu (Igor)
> >> 
> >> Testing done:
> >> * `make check`
> >> * `qemu-system-x86_64 -M pc -m 2G -cdrom manjaro-kde-21.2.6-220416-linux515.iso`
> >> * `qemu-system-x86_64 -M q35 -m 2G -cdrom \
> >>    manjaro-kde-21.2.6-220416-linux515.iso`
> >> 
> >> v3:
> >> - Clean up includes in AcpiDeviceIfClass::madt_cpu sub series last (Markus)
> >> - Restructure qbus_build_aml() sub series (Phil, me)
> >> 
> >> v2:
> >> - Don't inline qbus_build_aml() (Phil)
> >> - Add 'hw/acpi/piix4: No need to #include "hw/southbridge/piix.h"'
> >> 
> >> Bernhard Beschow (7):
> >>   hw/i386/acpi-build: Remove unused attributes
> >>   hw/isa/isa-bus: Turn isa_build_aml() into qbus_build_aml()
> >>   hw/acpi/piix4: No need to #include "hw/southbridge/piix.h"
> >>   hw/acpi/acpi_dev_interface: Remove unused parameter from
> >>     AcpiDeviceIfClass::madt_cpu
> >>   hw/acpi/acpi_dev_interface: Factor out TYPE_ACPI_CPU_AML_IF
> >>   hw/acpi/acpi_dev_interface: Remove now unused #include "hw/boards.h"
> >>   hw/i386/pc: Unexport pc_madt_cpu_entry()
> >> 
> >>  hw/acpi/hmat.h                       |  3 +-
> >>  hw/i386/acpi-common.h                |  3 +-
> >>  include/hw/acpi/acpi_aml_interface.h |  3 ++
> >>  include/hw/acpi/acpi_cpu_interface.h | 26 ++++++++++++++++
> >>  include/hw/acpi/acpi_dev_interface.h |  4 ---
> >>  include/hw/i386/pc.h                 |  6 ----
> >>  include/hw/isa/isa.h                 |  1 -
> >>  hw/acpi/acpi-x86-stub.c              |  7 -----
> >>  hw/acpi/acpi_interface.c             | 18 ++++++++++-
> >>  hw/acpi/cpu.c                        | 13 ++++----
> >>  hw/acpi/hmat.c                       |  1 +
> >>  hw/acpi/memory_hotplug.c             |  1 +
> >>  hw/acpi/piix4.c                      |  3 --
> >>  hw/i2c/smbus_ich9.c                  |  5 +--
> >>  hw/i386/acpi-build.c                 |  5 +--
> >>  hw/i386/acpi-common.c                | 42 +++----------------------
> >>  hw/i386/acpi-microvm.c               |  6 ++--
> >>  hw/i386/generic_event_device_x86.c   |  9 ------
> >>  hw/isa/isa-bus.c                     | 10 ------
> >>  hw/isa/lpc_ich9.c                    |  6 +---
> >>  hw/isa/piix3.c                       |  5 +--
> >>  monitor/qmp-cmds.c                   |  1 +
> >>  target/i386/cpu.c                    | 46 ++++++++++++++++++++++++++++
> >>  23 files changed, 117 insertions(+), 107 deletions(-)
> >>  create mode 100644 include/hw/acpi/acpi_cpu_interface.h
> >> 
> >