* [PATCH v3 1/7] hw/acpi/acpi_dev_interface: Remove unused parameter from AcpiDeviceIfClass::madt_cpu
2023-01-16 15:29 [PATCH v3 0/7] AML Housekeeping Bernhard Beschow
@ 2023-01-16 15:29 ` Bernhard Beschow
2023-01-16 16:37 ` Igor Mammedov
2023-01-16 15:29 ` [PATCH v3 2/7] hw/acpi/acpi_dev_interface: Resolve AcpiDeviceIfClass::madt_cpu Bernhard Beschow
` (5 subsequent siblings)
6 siblings, 1 reply; 20+ messages in thread
From: Bernhard Beschow @ 2023-01-16 15:29 UTC (permalink / raw)
To: qemu-devel
Cc: qemu-trivial, Richard Henderson, Paolo Bonzini,
Philippe Mathieu-Daudé, Markus Armbruster, Eduardo Habkost,
Michael S. Tsirkin, Ani Sinha, Marcel Apfelbaum, Igor Mammedov,
Aurelien Jarno, 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>
---
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 991f905f5d..a0647165d1 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.0
^ permalink raw reply related [flat|nested] 20+ messages in thread* Re: [PATCH v3 1/7] hw/acpi/acpi_dev_interface: Remove unused parameter from AcpiDeviceIfClass::madt_cpu
2023-01-16 15:29 ` [PATCH v3 1/7] hw/acpi/acpi_dev_interface: Remove unused parameter from AcpiDeviceIfClass::madt_cpu Bernhard Beschow
@ 2023-01-16 16:37 ` Igor Mammedov
0 siblings, 0 replies; 20+ messages in thread
From: Igor Mammedov @ 2023-01-16 16:37 UTC (permalink / raw)
To: Bernhard Beschow
Cc: qemu-devel, qemu-trivial, Richard Henderson, Paolo Bonzini,
Philippe Mathieu-Daudé, Markus Armbruster, Eduardo Habkost,
Michael S. Tsirkin, Ani Sinha, Marcel Apfelbaum, Aurelien Jarno
On Mon, 16 Jan 2023 16:29:02 +0100
Bernhard Beschow <shentey@gmail.com> wrote:
> 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 991f905f5d..a0647165d1 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;
> }
^ permalink raw reply [flat|nested] 20+ messages in thread
* [PATCH v3 2/7] hw/acpi/acpi_dev_interface: Resolve AcpiDeviceIfClass::madt_cpu
2023-01-16 15:29 [PATCH v3 0/7] AML Housekeeping Bernhard Beschow
2023-01-16 15:29 ` [PATCH v3 1/7] hw/acpi/acpi_dev_interface: Remove unused parameter from AcpiDeviceIfClass::madt_cpu Bernhard Beschow
@ 2023-01-16 15:29 ` Bernhard Beschow
2023-01-16 16:29 ` Igor Mammedov
2023-01-16 15:29 ` [PATCH v3 3/7] hw/acpi/acpi_dev_interface: Remove now unused #include "hw/boards.h" Bernhard Beschow
` (4 subsequent siblings)
6 siblings, 1 reply; 20+ messages in thread
From: Bernhard Beschow @ 2023-01-16 15:29 UTC (permalink / raw)
To: qemu-devel
Cc: qemu-trivial, Richard Henderson, Paolo Bonzini,
Philippe Mathieu-Daudé, Markus Armbruster, Eduardo Habkost,
Michael S. Tsirkin, Ani Sinha, Marcel Apfelbaum, Igor Mammedov,
Aurelien Jarno, 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>
---
hw/i386/acpi-common.h | 7 +++++--
include/hw/acpi/acpi_dev_interface.h | 2 --
include/hw/acpi/cpu.h | 6 +++++-
include/hw/i386/pc.h | 4 ----
hw/acpi/acpi-x86-stub.c | 6 ------
hw/acpi/cpu.c | 10 ++++------
hw/acpi/piix4.c | 2 --
hw/i386/acpi-build.c | 5 ++---
hw/i386/acpi-common.c | 5 ++---
hw/i386/acpi-microvm.c | 3 +--
hw/i386/generic_event_device_x86.c | 9 ---------
hw/isa/lpc_ich9.c | 1 -
12 files changed, 19 insertions(+), 41 deletions(-)
diff --git a/hw/i386/acpi-common.h b/hw/i386/acpi-common.h
index a68825acf5..968d625d88 100644
--- a/hw/i386/acpi-common.h
+++ b/hw/i386/acpi-common.h
@@ -1,15 +1,18 @@
#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"
+#include "hw/boards.h"
/* Default IOAPIC ID */
#define ACPI_BUILD_IOAPIC_ID 0x0
+void pc_madt_cpu_entry(int uid, const CPUArchIdList *apic_ids, GArray *entry,
+ bool force_enabled);
+
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_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/include/hw/acpi/cpu.h b/include/hw/acpi/cpu.h
index 999caaf510..25b25bb594 100644
--- a/include/hw/acpi/cpu.h
+++ b/include/hw/acpi/cpu.h
@@ -15,6 +15,7 @@
#include "hw/qdev-core.h"
#include "hw/acpi/acpi.h"
#include "hw/acpi/aml-build.h"
+#include "hw/boards.h"
#include "hw/hotplug.h"
typedef struct AcpiCpuStatus {
@@ -55,8 +56,11 @@ typedef struct CPUHotplugFeatures {
const char *smi_path;
} CPUHotplugFeatures;
+typedef void (*madt_cpu_fn)(int uid, const CPUArchIdList *apic_ids,
+ GArray *entry, bool force_enabled);
+
void build_cpus_aml(Aml *table, MachineState *machine, CPUHotplugFeatures opts,
- hwaddr io_base,
+ hwaddr io_base, madt_cpu_fn madt_cpu,
const char *res_root,
const char *event_handler_method);
diff --git a/include/hw/i386/pc.h b/include/hw/i386/pc.h
index a0647165d1..a5cce88653 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/acpi/cpu.c b/hw/acpi/cpu.c
index 19c154d78f..db15f9278d 100644
--- a/hw/acpi/cpu.c
+++ b/hw/acpi/cpu.c
@@ -338,7 +338,7 @@ const VMStateDescription vmstate_cpu_hotplug = {
#define CPU_FW_EJECT_EVENT "CEJF"
void build_cpus_aml(Aml *table, MachineState *machine, CPUHotplugFeatures opts,
- hwaddr io_base,
+ hwaddr io_base, madt_cpu_fn madt_cpu,
const char *res_root,
const char *event_handler_method)
{
@@ -353,8 +353,8 @@ 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);
+
+ assert(madt_cpu);
cpu_ctrl_dev = aml_device("%s", cphp_res_path);
{
@@ -664,9 +664,7 @@ 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 */
+ 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 0a81f1ad93..4d0d4fdeeb 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/southbridge/piix.h"
#include "hw/irq.h"
#include "hw/isa/apm.h"
@@ -643,7 +642,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 127c4e2d50..0be3960a37 100644
--- a/hw/i386/acpi-build.c
+++ b/hw/i386/acpi-build.c
@@ -1440,7 +1440,7 @@ build_dsdt(GArray *table_data, BIOSLinker *linker,
.fw_unplugs_cpu = pm->smi_on_cpu_unplug,
};
build_cpus_aml(dsdt, machine, opts, pm->cpu_hp_io_base,
- "\\_SB.PCI0", "\\_GPE._E02");
+ pc_madt_cpu_entry, "\\_SB.PCI0", "\\_GPE._E02");
}
if (pcms->memhp_io_base && nr_mem) {
@@ -2424,8 +2424,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..aabf78092e 100644
--- a/hw/i386/acpi-common.c
+++ b/hw/i386/acpi-common.c
@@ -94,14 +94,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 +110,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(i, apic_ids, table_data, false);
+ pc_madt_cpu_entry(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 fb09185cbd..d8a444d06c 100644
--- a/hw/i386/acpi-microvm.c
+++ b/hw/i386/acpi-microvm.c
@@ -213,8 +213,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 8d541e2b54..0ab0a341be 100644
--- a/hw/isa/lpc_ich9.c
+++ b/hw/isa/lpc_ich9.c
@@ -870,7 +870,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;
}
--
2.39.0
^ permalink raw reply related [flat|nested] 20+ messages in thread* Re: [PATCH v3 2/7] hw/acpi/acpi_dev_interface: Resolve AcpiDeviceIfClass::madt_cpu
2023-01-16 15:29 ` [PATCH v3 2/7] hw/acpi/acpi_dev_interface: Resolve AcpiDeviceIfClass::madt_cpu Bernhard Beschow
@ 2023-01-16 16:29 ` Igor Mammedov
2023-01-17 0:30 ` Bernhard Beschow
0 siblings, 1 reply; 20+ messages in thread
From: Igor Mammedov @ 2023-01-16 16:29 UTC (permalink / raw)
To: Bernhard Beschow
Cc: qemu-devel, qemu-trivial, Richard Henderson, Paolo Bonzini,
Philippe Mathieu-Daudé, Markus Armbruster, Eduardo Habkost,
Michael S. Tsirkin, Ani Sinha, Marcel Apfelbaum, Aurelien Jarno
On Mon, 16 Jan 2023 16:29:03 +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.
>
> 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.
I'm not sure about this approach,
the callback is intend to be used not only by x86 but also in
the end by ARM (it's just that arm/virt CPU hotplug patches are
still work in progress and haven't been merged).
So I'd prefer to keep AcpiDeviceIfClass::madt_cpu.
What's the end goal you are trying to achieve by getting
rid of this callback?
> Signed-off-by: Bernhard Beschow <shentey@gmail.com>
> ---
> hw/i386/acpi-common.h | 7 +++++--
> include/hw/acpi/acpi_dev_interface.h | 2 --
> include/hw/acpi/cpu.h | 6 +++++-
> include/hw/i386/pc.h | 4 ----
> hw/acpi/acpi-x86-stub.c | 6 ------
> hw/acpi/cpu.c | 10 ++++------
> hw/acpi/piix4.c | 2 --
> hw/i386/acpi-build.c | 5 ++---
> hw/i386/acpi-common.c | 5 ++---
> hw/i386/acpi-microvm.c | 3 +--
> hw/i386/generic_event_device_x86.c | 9 ---------
> hw/isa/lpc_ich9.c | 1 -
> 12 files changed, 19 insertions(+), 41 deletions(-)
>
> diff --git a/hw/i386/acpi-common.h b/hw/i386/acpi-common.h
> index a68825acf5..968d625d88 100644
> --- a/hw/i386/acpi-common.h
> +++ b/hw/i386/acpi-common.h
> @@ -1,15 +1,18 @@
> #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"
> +#include "hw/boards.h"
>
> /* Default IOAPIC ID */
> #define ACPI_BUILD_IOAPIC_ID 0x0
>
> +void pc_madt_cpu_entry(int uid, const CPUArchIdList *apic_ids, GArray *entry,
> + bool force_enabled);
> +
> 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_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/include/hw/acpi/cpu.h b/include/hw/acpi/cpu.h
> index 999caaf510..25b25bb594 100644
> --- a/include/hw/acpi/cpu.h
> +++ b/include/hw/acpi/cpu.h
> @@ -15,6 +15,7 @@
> #include "hw/qdev-core.h"
> #include "hw/acpi/acpi.h"
> #include "hw/acpi/aml-build.h"
> +#include "hw/boards.h"
> #include "hw/hotplug.h"
>
> typedef struct AcpiCpuStatus {
> @@ -55,8 +56,11 @@ typedef struct CPUHotplugFeatures {
> const char *smi_path;
> } CPUHotplugFeatures;
>
> +typedef void (*madt_cpu_fn)(int uid, const CPUArchIdList *apic_ids,
> + GArray *entry, bool force_enabled);
> +
> void build_cpus_aml(Aml *table, MachineState *machine, CPUHotplugFeatures opts,
> - hwaddr io_base,
> + hwaddr io_base, madt_cpu_fn madt_cpu,
> const char *res_root,
> const char *event_handler_method);
>
> diff --git a/include/hw/i386/pc.h b/include/hw/i386/pc.h
> index a0647165d1..a5cce88653 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/acpi/cpu.c b/hw/acpi/cpu.c
> index 19c154d78f..db15f9278d 100644
> --- a/hw/acpi/cpu.c
> +++ b/hw/acpi/cpu.c
> @@ -338,7 +338,7 @@ const VMStateDescription vmstate_cpu_hotplug = {
> #define CPU_FW_EJECT_EVENT "CEJF"
>
> void build_cpus_aml(Aml *table, MachineState *machine, CPUHotplugFeatures opts,
> - hwaddr io_base,
> + hwaddr io_base, madt_cpu_fn madt_cpu,
> const char *res_root,
> const char *event_handler_method)
> {
> @@ -353,8 +353,8 @@ 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);
> +
> + assert(madt_cpu);
>
> cpu_ctrl_dev = aml_device("%s", cphp_res_path);
> {
> @@ -664,9 +664,7 @@ 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 */
> + 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 0a81f1ad93..4d0d4fdeeb 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/southbridge/piix.h"
> #include "hw/irq.h"
> #include "hw/isa/apm.h"
> @@ -643,7 +642,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 127c4e2d50..0be3960a37 100644
> --- a/hw/i386/acpi-build.c
> +++ b/hw/i386/acpi-build.c
> @@ -1440,7 +1440,7 @@ build_dsdt(GArray *table_data, BIOSLinker *linker,
> .fw_unplugs_cpu = pm->smi_on_cpu_unplug,
> };
> build_cpus_aml(dsdt, machine, opts, pm->cpu_hp_io_base,
> - "\\_SB.PCI0", "\\_GPE._E02");
> + pc_madt_cpu_entry, "\\_SB.PCI0", "\\_GPE._E02");
> }
>
> if (pcms->memhp_io_base && nr_mem) {
> @@ -2424,8 +2424,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..aabf78092e 100644
> --- a/hw/i386/acpi-common.c
> +++ b/hw/i386/acpi-common.c
> @@ -94,14 +94,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 +110,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(i, apic_ids, table_data, false);
> + pc_madt_cpu_entry(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 fb09185cbd..d8a444d06c 100644
> --- a/hw/i386/acpi-microvm.c
> +++ b/hw/i386/acpi-microvm.c
> @@ -213,8 +213,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 8d541e2b54..0ab0a341be 100644
> --- a/hw/isa/lpc_ich9.c
> +++ b/hw/isa/lpc_ich9.c
> @@ -870,7 +870,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;
> }
>
^ permalink raw reply [flat|nested] 20+ messages in thread* Re: [PATCH v3 2/7] hw/acpi/acpi_dev_interface: Resolve AcpiDeviceIfClass::madt_cpu
2023-01-16 16:29 ` Igor Mammedov
@ 2023-01-17 0:30 ` Bernhard Beschow
2023-01-17 14:49 ` Bernhard Beschow
2023-01-18 14:59 ` Igor Mammedov
0 siblings, 2 replies; 20+ messages in thread
From: Bernhard Beschow @ 2023-01-17 0:30 UTC (permalink / raw)
To: Igor Mammedov
Cc: qemu-devel, qemu-trivial, Richard Henderson, Paolo Bonzini,
Philippe Mathieu-Daudé, Markus Armbruster, Eduardo Habkost,
Michael S. Tsirkin, Ani Sinha, Marcel Apfelbaum, Aurelien Jarno
Am 16. Januar 2023 16:29:30 UTC schrieb Igor Mammedov <imammedo@redhat.com>:
>On Mon, 16 Jan 2023 16:29:03 +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.
>>
>> 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.
>
>I'm not sure about this approach,
>the callback is intend to be used not only by x86 but also in
>the end by ARM (it's just that arm/virt CPU hotplug patches are
>still work in progress and haven't been merged).
IIUC one would call the target-independent build_cpus_aml() from the ARM-specific build_madt(). There, one could pass a function pointer to an ARM-specific madt_cpu_fn. Shouldn't that get us covered?
>
>So I'd prefer to keep AcpiDeviceIfClass::madt_cpu.
>
>What's the end goal you are trying to achieve by getting
>rid of this callback?
ACPI controllers are in principle not x86-specific, yet our PIIX4 and ICH9 device models always assign an x86-specific function to AcpiDeviceIfClass::madt_cpu. That doesn't seem right because the device models make assumptions about their usage contexts which they ideally shouldn't.
In addition, it seems to me that ACPI controllers and AML generation should not be mixed: An ACPI controller deals with (hardware) events while AML generation is usually a BIOS task to inject code into an OS. Therefore, ACPI controllers don't seem to be the right place to assign AML-generating functions because that doesn't match how reality works.
To solve both issues, one could factor out e.g. an AmlDeviceIf from AcpiDeviceIf, so one would't force the device models to provide an madt_cpu. Then one could assign a target-specific AmlDeviceIfClass::madt_cpu e.g. in board code. At the moment I don't see a need for a new interface, however, since the function pointer works just fine: It frees the device models from having to provide it and it is used in code which already deals with AML generation.
My end goal is to make the VT82xx south bridges compatible with x86 and to bring them to feature parity with PIIX. For this, I need to implement the VIA ACPI controller proper, and since these south bridges are currently only used in MIPS and PowerPC contexts I'm feeling a bit uncomfortable having to drag in x86 assumptions into these devices.
Best regards,
Bernhard
>
>> Signed-off-by: Bernhard Beschow <shentey@gmail.com>
>> ---
>> hw/i386/acpi-common.h | 7 +++++--
>> include/hw/acpi/acpi_dev_interface.h | 2 --
>> include/hw/acpi/cpu.h | 6 +++++-
>> include/hw/i386/pc.h | 4 ----
>> hw/acpi/acpi-x86-stub.c | 6 ------
>> hw/acpi/cpu.c | 10 ++++------
>> hw/acpi/piix4.c | 2 --
>> hw/i386/acpi-build.c | 5 ++---
>> hw/i386/acpi-common.c | 5 ++---
>> hw/i386/acpi-microvm.c | 3 +--
>> hw/i386/generic_event_device_x86.c | 9 ---------
>> hw/isa/lpc_ich9.c | 1 -
>> 12 files changed, 19 insertions(+), 41 deletions(-)
>>
>> diff --git a/hw/i386/acpi-common.h b/hw/i386/acpi-common.h
>> index a68825acf5..968d625d88 100644
>> --- a/hw/i386/acpi-common.h
>> +++ b/hw/i386/acpi-common.h
>> @@ -1,15 +1,18 @@
>> #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"
>> +#include "hw/boards.h"
>>
>> /* Default IOAPIC ID */
>> #define ACPI_BUILD_IOAPIC_ID 0x0
>>
>> +void pc_madt_cpu_entry(int uid, const CPUArchIdList *apic_ids, GArray *entry,
>> + bool force_enabled);
>> +
>> 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_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/include/hw/acpi/cpu.h b/include/hw/acpi/cpu.h
>> index 999caaf510..25b25bb594 100644
>> --- a/include/hw/acpi/cpu.h
>> +++ b/include/hw/acpi/cpu.h
>> @@ -15,6 +15,7 @@
>> #include "hw/qdev-core.h"
>> #include "hw/acpi/acpi.h"
>> #include "hw/acpi/aml-build.h"
>> +#include "hw/boards.h"
>> #include "hw/hotplug.h"
>>
>> typedef struct AcpiCpuStatus {
>> @@ -55,8 +56,11 @@ typedef struct CPUHotplugFeatures {
>> const char *smi_path;
>> } CPUHotplugFeatures;
>>
>> +typedef void (*madt_cpu_fn)(int uid, const CPUArchIdList *apic_ids,
>> + GArray *entry, bool force_enabled);
>> +
>> void build_cpus_aml(Aml *table, MachineState *machine, CPUHotplugFeatures opts,
>> - hwaddr io_base,
>> + hwaddr io_base, madt_cpu_fn madt_cpu,
>> const char *res_root,
>> const char *event_handler_method);
>>
>> diff --git a/include/hw/i386/pc.h b/include/hw/i386/pc.h
>> index a0647165d1..a5cce88653 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/acpi/cpu.c b/hw/acpi/cpu.c
>> index 19c154d78f..db15f9278d 100644
>> --- a/hw/acpi/cpu.c
>> +++ b/hw/acpi/cpu.c
>> @@ -338,7 +338,7 @@ const VMStateDescription vmstate_cpu_hotplug = {
>> #define CPU_FW_EJECT_EVENT "CEJF"
>>
>> void build_cpus_aml(Aml *table, MachineState *machine, CPUHotplugFeatures opts,
>> - hwaddr io_base,
>> + hwaddr io_base, madt_cpu_fn madt_cpu,
>> const char *res_root,
>> const char *event_handler_method)
>> {
>> @@ -353,8 +353,8 @@ 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);
>> +
>> + assert(madt_cpu);
>>
>> cpu_ctrl_dev = aml_device("%s", cphp_res_path);
>> {
>> @@ -664,9 +664,7 @@ 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 */
>> + 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 0a81f1ad93..4d0d4fdeeb 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/southbridge/piix.h"
>> #include "hw/irq.h"
>> #include "hw/isa/apm.h"
>> @@ -643,7 +642,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 127c4e2d50..0be3960a37 100644
>> --- a/hw/i386/acpi-build.c
>> +++ b/hw/i386/acpi-build.c
>> @@ -1440,7 +1440,7 @@ build_dsdt(GArray *table_data, BIOSLinker *linker,
>> .fw_unplugs_cpu = pm->smi_on_cpu_unplug,
>> };
>> build_cpus_aml(dsdt, machine, opts, pm->cpu_hp_io_base,
>> - "\\_SB.PCI0", "\\_GPE._E02");
>> + pc_madt_cpu_entry, "\\_SB.PCI0", "\\_GPE._E02");
>> }
>>
>> if (pcms->memhp_io_base && nr_mem) {
>> @@ -2424,8 +2424,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..aabf78092e 100644
>> --- a/hw/i386/acpi-common.c
>> +++ b/hw/i386/acpi-common.c
>> @@ -94,14 +94,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 +110,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(i, apic_ids, table_data, false);
>> + pc_madt_cpu_entry(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 fb09185cbd..d8a444d06c 100644
>> --- a/hw/i386/acpi-microvm.c
>> +++ b/hw/i386/acpi-microvm.c
>> @@ -213,8 +213,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 8d541e2b54..0ab0a341be 100644
>> --- a/hw/isa/lpc_ich9.c
>> +++ b/hw/isa/lpc_ich9.c
>> @@ -870,7 +870,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;
>> }
>>
>
^ permalink raw reply [flat|nested] 20+ messages in thread* Re: [PATCH v3 2/7] hw/acpi/acpi_dev_interface: Resolve AcpiDeviceIfClass::madt_cpu
2023-01-17 0:30 ` Bernhard Beschow
@ 2023-01-17 14:49 ` Bernhard Beschow
2023-01-18 14:59 ` Igor Mammedov
1 sibling, 0 replies; 20+ messages in thread
From: Bernhard Beschow @ 2023-01-17 14:49 UTC (permalink / raw)
To: Igor Mammedov
Cc: qemu-devel, qemu-trivial, Richard Henderson, Paolo Bonzini,
Philippe Mathieu-Daudé, Markus Armbruster, Eduardo Habkost,
Michael S. Tsirkin, Ani Sinha, Marcel Apfelbaum, Aurelien Jarno
[-- Attachment #1: Type: text/plain, Size: 14966 bytes --]
On Tue, Jan 17, 2023 at 1:30 AM Bernhard Beschow <shentey@gmail.com> wrote:
>
>
> Am 16. Januar 2023 16:29:30 UTC schrieb Igor Mammedov <imammedo@redhat.com
> >:
> >On Mon, 16 Jan 2023 16:29:03 +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.
> >>
> >> 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.
> >
> >I'm not sure about this approach,
> >the callback is intend to be used not only by x86 but also in
> >the end by ARM (it's just that arm/virt CPU hotplug patches are
> >still work in progress and haven't been merged).
>
> IIUC one would call the target-independent build_cpus_aml() from the
> ARM-specific build_madt(). There, one could pass a function pointer to an
> ARM-specific madt_cpu_fn. Shouldn't that get us covered?
>
> >
> >So I'd prefer to keep AcpiDeviceIfClass::madt_cpu.
> >
> >What's the end goal you are trying to achieve by getting
> >rid of this callback?
>
> ACPI controllers are in principle not x86-specific, yet our PIIX4 and ICH9
> device models always assign an x86-specific function to
> AcpiDeviceIfClass::madt_cpu. That doesn't seem right because the device
> models make assumptions about their usage contexts which they ideally
> shouldn't.
>
> In addition, it seems to me that ACPI controllers and AML generation
> should not be mixed: An ACPI controller deals with (hardware) events while
> AML generation is usually a BIOS task to inject code into an OS. Therefore,
> ACPI controllers don't seem to be the right place to assign AML-generating
> functions because that doesn't match how reality works.
>
> To solve both issues, one could factor out e.g. an AmlDeviceIf from
> AcpiDeviceIf, so one would't force the device models to provide an
> madt_cpu. Then one could assign a target-specific
> AmlDeviceIfClass::madt_cpu e.g. in board code. At the moment I don't see a
> need for a new interface, however, since the function pointer works just
> fine: It frees the device models from having to provide it and it is used
> in code which already deals with AML generation.
>
Well, yeah, this interface already exists as AcpiDevAmlIf which I even
touched in the last part of this series... CPUs could possibly implement it
though this needs a lot more thought. But, as mentioned above, I don't see
a big urge to do this at the moment.
Best regards,
Bernhard
>
> My end goal is to make the VT82xx south bridges compatible with x86 and to
> bring them to feature parity with PIIX. For this, I need to implement the
> VIA ACPI controller proper, and since these south bridges are currently
> only used in MIPS and PowerPC contexts I'm feeling a bit uncomfortable
> having to drag in x86 assumptions into these devices.
>
> Best regards,
> Bernhard
>
> >
> >> Signed-off-by: Bernhard Beschow <shentey@gmail.com>
> >> ---
> >> hw/i386/acpi-common.h | 7 +++++--
> >> include/hw/acpi/acpi_dev_interface.h | 2 --
> >> include/hw/acpi/cpu.h | 6 +++++-
> >> include/hw/i386/pc.h | 4 ----
> >> hw/acpi/acpi-x86-stub.c | 6 ------
> >> hw/acpi/cpu.c | 10 ++++------
> >> hw/acpi/piix4.c | 2 --
> >> hw/i386/acpi-build.c | 5 ++---
> >> hw/i386/acpi-common.c | 5 ++---
> >> hw/i386/acpi-microvm.c | 3 +--
> >> hw/i386/generic_event_device_x86.c | 9 ---------
> >> hw/isa/lpc_ich9.c | 1 -
> >> 12 files changed, 19 insertions(+), 41 deletions(-)
> >>
> >> diff --git a/hw/i386/acpi-common.h b/hw/i386/acpi-common.h
> >> index a68825acf5..968d625d88 100644
> >> --- a/hw/i386/acpi-common.h
> >> +++ b/hw/i386/acpi-common.h
> >> @@ -1,15 +1,18 @@
> >> #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"
> >> +#include "hw/boards.h"
> >>
> >> /* Default IOAPIC ID */
> >> #define ACPI_BUILD_IOAPIC_ID 0x0
> >>
> >> +void pc_madt_cpu_entry(int uid, const CPUArchIdList *apic_ids, GArray
> *entry,
> >> + bool force_enabled);
> >> +
> >> 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_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/include/hw/acpi/cpu.h b/include/hw/acpi/cpu.h
> >> index 999caaf510..25b25bb594 100644
> >> --- a/include/hw/acpi/cpu.h
> >> +++ b/include/hw/acpi/cpu.h
> >> @@ -15,6 +15,7 @@
> >> #include "hw/qdev-core.h"
> >> #include "hw/acpi/acpi.h"
> >> #include "hw/acpi/aml-build.h"
> >> +#include "hw/boards.h"
> >> #include "hw/hotplug.h"
> >>
> >> typedef struct AcpiCpuStatus {
> >> @@ -55,8 +56,11 @@ typedef struct CPUHotplugFeatures {
> >> const char *smi_path;
> >> } CPUHotplugFeatures;
> >>
> >> +typedef void (*madt_cpu_fn)(int uid, const CPUArchIdList *apic_ids,
> >> + GArray *entry, bool force_enabled);
> >> +
> >> void build_cpus_aml(Aml *table, MachineState *machine,
> CPUHotplugFeatures opts,
> >> - hwaddr io_base,
> >> + hwaddr io_base, madt_cpu_fn madt_cpu,
> >> const char *res_root,
> >> const char *event_handler_method);
> >>
> >> diff --git a/include/hw/i386/pc.h b/include/hw/i386/pc.h
> >> index a0647165d1..a5cce88653 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/acpi/cpu.c b/hw/acpi/cpu.c
> >> index 19c154d78f..db15f9278d 100644
> >> --- a/hw/acpi/cpu.c
> >> +++ b/hw/acpi/cpu.c
> >> @@ -338,7 +338,7 @@ const VMStateDescription vmstate_cpu_hotplug = {
> >> #define CPU_FW_EJECT_EVENT "CEJF"
> >>
> >> void build_cpus_aml(Aml *table, MachineState *machine,
> CPUHotplugFeatures opts,
> >> - hwaddr io_base,
> >> + hwaddr io_base, madt_cpu_fn madt_cpu,
> >> const char *res_root,
> >> const char *event_handler_method)
> >> {
> >> @@ -353,8 +353,8 @@ 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);
> >> +
> >> + assert(madt_cpu);
> >>
> >> cpu_ctrl_dev = aml_device("%s", cphp_res_path);
> >> {
> >> @@ -664,9 +664,7 @@ 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 */
> >> + 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 0a81f1ad93..4d0d4fdeeb 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/southbridge/piix.h"
> >> #include "hw/irq.h"
> >> #include "hw/isa/apm.h"
> >> @@ -643,7 +642,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 127c4e2d50..0be3960a37 100644
> >> --- a/hw/i386/acpi-build.c
> >> +++ b/hw/i386/acpi-build.c
> >> @@ -1440,7 +1440,7 @@ build_dsdt(GArray *table_data, BIOSLinker *linker,
> >> .fw_unplugs_cpu = pm->smi_on_cpu_unplug,
> >> };
> >> build_cpus_aml(dsdt, machine, opts, pm->cpu_hp_io_base,
> >> - "\\_SB.PCI0", "\\_GPE._E02");
> >> + pc_madt_cpu_entry, "\\_SB.PCI0", "\\_GPE._E02");
> >> }
> >>
> >> if (pcms->memhp_io_base && nr_mem) {
> >> @@ -2424,8 +2424,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..aabf78092e 100644
> >> --- a/hw/i386/acpi-common.c
> >> +++ b/hw/i386/acpi-common.c
> >> @@ -94,14 +94,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 +110,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(i, apic_ids, table_data, false);
> >> + pc_madt_cpu_entry(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 fb09185cbd..d8a444d06c 100644
> >> --- a/hw/i386/acpi-microvm.c
> >> +++ b/hw/i386/acpi-microvm.c
> >> @@ -213,8 +213,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 8d541e2b54..0ab0a341be 100644
> >> --- a/hw/isa/lpc_ich9.c
> >> +++ b/hw/isa/lpc_ich9.c
> >> @@ -870,7 +870,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;
> >> }
> >>
> >
>
[-- Attachment #2: Type: text/html, Size: 19504 bytes --]
^ permalink raw reply [flat|nested] 20+ messages in thread* Re: [PATCH v3 2/7] hw/acpi/acpi_dev_interface: Resolve AcpiDeviceIfClass::madt_cpu
2023-01-17 0:30 ` Bernhard Beschow
2023-01-17 14:49 ` Bernhard Beschow
@ 2023-01-18 14:59 ` Igor Mammedov
2023-01-19 14:47 ` Bernhard Beschow
1 sibling, 1 reply; 20+ messages in thread
From: Igor Mammedov @ 2023-01-18 14:59 UTC (permalink / raw)
To: Bernhard Beschow
Cc: qemu-devel, qemu-trivial, Richard Henderson, Paolo Bonzini,
Philippe Mathieu-Daudé, Markus Armbruster, Eduardo Habkost,
Michael S. Tsirkin, Ani Sinha, Marcel Apfelbaum, Aurelien Jarno
On Tue, 17 Jan 2023 00:30:23 +0000
Bernhard Beschow <shentey@gmail.com> wrote:
> Am 16. Januar 2023 16:29:30 UTC schrieb Igor Mammedov <imammedo@redhat.com>:
> >On Mon, 16 Jan 2023 16:29:03 +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.
> >>
> >> 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.
> >
> >I'm not sure about this approach,
> >the callback is intend to be used not only by x86 but also in
> >the end by ARM (it's just that arm/virt CPU hotplug patches are
> >still work in progress and haven't been merged).
>
> IIUC one would call the target-independent build_cpus_aml() from the ARM-specific build_madt(). There, one could pass a function pointer to an ARM-specific madt_cpu_fn. Shouldn't that get us covered?
it will work in this case, but I don't like going back to random
function pointer argument approach instead of using QOM and
interfaces which is much better in describing expectations/contract
for interfaces adn objects it's attached to.
Howver that is not the reason why I'm against this approach, see bellow.
> >So I'd prefer to keep AcpiDeviceIfClass::madt_cpu.
> >
> >What's the end goal you are trying to achieve by getting
> >rid of this callback?
>
> ACPI controllers are in principle not x86-specific, yet our PIIX4 and ICH9 device models always assign an x86-specific function to AcpiDeviceIfClass::madt_cpu. That doesn't seem right because the device models make assumptions about their usage contexts which they ideally shouldn't.
>
> In addition, it seems to me that ACPI controllers and AML generation should not be mixed: An ACPI controller deals with (hardware) events while AML generation is usually a BIOS task to inject code into an OS. Therefore, ACPI controllers don't seem to be the right place to assign AML-generating functions because that doesn't match how reality works.
It would be nice to have pristine hardware-only device models
and firmware composing ACPI tables like baremetal systems do
(they have a luxury of fixed hardware which makes it way simpler),
however it was not practical nor sustainable in mainstream virt case.
That's the reason why ACPI tables (firmware) were moved into QEMU (hardware).
> To solve both issues, one could factor out e.g. an AmlDeviceIf from AcpiDeviceIf, so one would't force the device models to provide an madt_cpu.
> Then one could assign a target-specific AmlDeviceIfClass::madt_cpu e.g. in board code.
ACPI tables in QEMU started this way, It stopped being maintainable
at certain point, see below for more.
>At the moment I don't see a need for a new interface, however, since the function pointer works just fine: It frees the device models from having to provide it and it is used in code which already deals with AML generation.
When the move happened, the bulk of ACPI code was as you suggest made
as machine specific and it's still the case for the most of it.
Then, with ease of adding new related features, it exploded into
I would say hard to maintain mess. Hence before adding more and
making it worse, I introduced call_dev_aml_func()/AcpiDevAmlIf
and started refactoring PCI AML code. That simplified main PCI bus
enumeration quite a bit.
The new interface however does exactly opposite of what you are doing,
i.e. it pushes device specific AML generation into corresponding device
model. It's not ideal as we have to provide stubs for targets where it's
not needed. But stubs for target independent code is typical and proven
approach we use in QEMU for such cases and is relatively easy to maintain.
So I'd call it a reasonable compromise.
Recently posted series (that untangles generic PCI slots AML from
ACPI PCI hotplug frankenstein) continues with AcpiDevAmlIf approach
and pushes more AML into target in-depended device models.
And there is more coming on top of it (with the goal to make most
of what we accumulated in PC/Q35 PCI ACPI code become generic enough
to replace most of PCI related AML elsewhere (microvm, arm/virt,
whatever else that is interested in ACPI tables support).
My ambition with this refactoring stops at making qdev device tree
self-describable at PCI boundary but AcpiDevAmlIf can be applied
to other devices described in DSDT, that hang off the board.
> My end goal is to make the VT82xx south bridges compatible with x86 and to bring them to feature parity with PIIX. For this, I need to implement the VIA ACPI controller proper, and since these south bridges are currently only used in MIPS and PowerPC contexts I'm feeling a bit uncomfortable having to drag in x86 assumptions into these devices.
basic ACPI hardware (GPE/PM registers) code is relatively isolated,
so you probably can implement that without touching piix much.
(with little to no stubs).
If you want to cleanup piix and free other targets from
unfortunate ACPI/x86 specific dependencies, it would probably
need another approach than presented here.
ex:
Isolating core(pristine) piix code in piix-basic class
for PPC/MIPS use, and then branching/inheriting of it
current piix class with all extra x86 'features' might
work without breaking migration. (/me assuming cross
version migration should work in this arrangement,
though I won't bet on it)
> Best regards,
> Bernhard
>
> >
> >> Signed-off-by: Bernhard Beschow <shentey@gmail.com>
> >> ---
> >> hw/i386/acpi-common.h | 7 +++++--
> >> include/hw/acpi/acpi_dev_interface.h | 2 --
> >> include/hw/acpi/cpu.h | 6 +++++-
> >> include/hw/i386/pc.h | 4 ----
> >> hw/acpi/acpi-x86-stub.c | 6 ------
> >> hw/acpi/cpu.c | 10 ++++------
> >> hw/acpi/piix4.c | 2 --
> >> hw/i386/acpi-build.c | 5 ++---
> >> hw/i386/acpi-common.c | 5 ++---
> >> hw/i386/acpi-microvm.c | 3 +--
> >> hw/i386/generic_event_device_x86.c | 9 ---------
> >> hw/isa/lpc_ich9.c | 1 -
> >> 12 files changed, 19 insertions(+), 41 deletions(-)
> >>
> >> diff --git a/hw/i386/acpi-common.h b/hw/i386/acpi-common.h
> >> index a68825acf5..968d625d88 100644
> >> --- a/hw/i386/acpi-common.h
> >> +++ b/hw/i386/acpi-common.h
> >> @@ -1,15 +1,18 @@
> >> #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"
> >> +#include "hw/boards.h"
> >>
> >> /* Default IOAPIC ID */
> >> #define ACPI_BUILD_IOAPIC_ID 0x0
> >>
> >> +void pc_madt_cpu_entry(int uid, const CPUArchIdList *apic_ids, GArray *entry,
> >> + bool force_enabled);
> >> +
> >> 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_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/include/hw/acpi/cpu.h b/include/hw/acpi/cpu.h
> >> index 999caaf510..25b25bb594 100644
> >> --- a/include/hw/acpi/cpu.h
> >> +++ b/include/hw/acpi/cpu.h
> >> @@ -15,6 +15,7 @@
> >> #include "hw/qdev-core.h"
> >> #include "hw/acpi/acpi.h"
> >> #include "hw/acpi/aml-build.h"
> >> +#include "hw/boards.h"
> >> #include "hw/hotplug.h"
> >>
> >> typedef struct AcpiCpuStatus {
> >> @@ -55,8 +56,11 @@ typedef struct CPUHotplugFeatures {
> >> const char *smi_path;
> >> } CPUHotplugFeatures;
> >>
> >> +typedef void (*madt_cpu_fn)(int uid, const CPUArchIdList *apic_ids,
> >> + GArray *entry, bool force_enabled);
> >> +
> >> void build_cpus_aml(Aml *table, MachineState *machine, CPUHotplugFeatures opts,
> >> - hwaddr io_base,
> >> + hwaddr io_base, madt_cpu_fn madt_cpu,
> >> const char *res_root,
> >> const char *event_handler_method);
> >>
> >> diff --git a/include/hw/i386/pc.h b/include/hw/i386/pc.h
> >> index a0647165d1..a5cce88653 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/acpi/cpu.c b/hw/acpi/cpu.c
> >> index 19c154d78f..db15f9278d 100644
> >> --- a/hw/acpi/cpu.c
> >> +++ b/hw/acpi/cpu.c
> >> @@ -338,7 +338,7 @@ const VMStateDescription vmstate_cpu_hotplug = {
> >> #define CPU_FW_EJECT_EVENT "CEJF"
> >>
> >> void build_cpus_aml(Aml *table, MachineState *machine, CPUHotplugFeatures opts,
> >> - hwaddr io_base,
> >> + hwaddr io_base, madt_cpu_fn madt_cpu,
> >> const char *res_root,
> >> const char *event_handler_method)
> >> {
> >> @@ -353,8 +353,8 @@ 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);
> >> +
> >> + assert(madt_cpu);
> >>
> >> cpu_ctrl_dev = aml_device("%s", cphp_res_path);
> >> {
> >> @@ -664,9 +664,7 @@ 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 */
> >> + 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 0a81f1ad93..4d0d4fdeeb 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/southbridge/piix.h"
> >> #include "hw/irq.h"
> >> #include "hw/isa/apm.h"
> >> @@ -643,7 +642,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 127c4e2d50..0be3960a37 100644
> >> --- a/hw/i386/acpi-build.c
> >> +++ b/hw/i386/acpi-build.c
> >> @@ -1440,7 +1440,7 @@ build_dsdt(GArray *table_data, BIOSLinker *linker,
> >> .fw_unplugs_cpu = pm->smi_on_cpu_unplug,
> >> };
> >> build_cpus_aml(dsdt, machine, opts, pm->cpu_hp_io_base,
> >> - "\\_SB.PCI0", "\\_GPE._E02");
> >> + pc_madt_cpu_entry, "\\_SB.PCI0", "\\_GPE._E02");
> >> }
> >>
> >> if (pcms->memhp_io_base && nr_mem) {
> >> @@ -2424,8 +2424,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..aabf78092e 100644
> >> --- a/hw/i386/acpi-common.c
> >> +++ b/hw/i386/acpi-common.c
> >> @@ -94,14 +94,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 +110,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(i, apic_ids, table_data, false);
> >> + pc_madt_cpu_entry(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 fb09185cbd..d8a444d06c 100644
> >> --- a/hw/i386/acpi-microvm.c
> >> +++ b/hw/i386/acpi-microvm.c
> >> @@ -213,8 +213,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 8d541e2b54..0ab0a341be 100644
> >> --- a/hw/isa/lpc_ich9.c
> >> +++ b/hw/isa/lpc_ich9.c
> >> @@ -870,7 +870,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;
> >> }
> >>
> >
>
^ permalink raw reply [flat|nested] 20+ messages in thread* Re: [PATCH v3 2/7] hw/acpi/acpi_dev_interface: Resolve AcpiDeviceIfClass::madt_cpu
2023-01-18 14:59 ` Igor Mammedov
@ 2023-01-19 14:47 ` Bernhard Beschow
2023-01-20 16:34 ` Igor Mammedov
0 siblings, 1 reply; 20+ messages in thread
From: Bernhard Beschow @ 2023-01-19 14:47 UTC (permalink / raw)
To: Igor Mammedov
Cc: qemu-devel, qemu-trivial, Richard Henderson, Paolo Bonzini,
Philippe Mathieu-Daudé, Markus Armbruster, Eduardo Habkost,
Michael S. Tsirkin, Ani Sinha, Marcel Apfelbaum, Aurelien Jarno
Am 18. Januar 2023 14:59:05 UTC schrieb Igor Mammedov <imammedo@redhat.com>:
>On Tue, 17 Jan 2023 00:30:23 +0000
>Bernhard Beschow <shentey@gmail.com> wrote:
>
>> Am 16. Januar 2023 16:29:30 UTC schrieb Igor Mammedov <imammedo@redhat.com>:
>> >On Mon, 16 Jan 2023 16:29:03 +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.
>> >>
>> >> 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.
>> >
>> >I'm not sure about this approach,
>> >the callback is intend to be used not only by x86 but also in
>> >the end by ARM (it's just that arm/virt CPU hotplug patches are
>> >still work in progress and haven't been merged).
>>
>> IIUC one would call the target-independent build_cpus_aml() from the ARM-specific build_madt(). There, one could pass a function pointer to an ARM-specific madt_cpu_fn. Shouldn't that get us covered?
>
>it will work in this case, but I don't like going back to random
>function pointer argument approach instead of using QOM and
>interfaces which is much better in describing expectations/contract
>for interfaces adn objects it's attached to.
>
>Howver that is not the reason why I'm against this approach, see bellow.
>
>> >So I'd prefer to keep AcpiDeviceIfClass::madt_cpu.
>> >
>> >What's the end goal you are trying to achieve by getting
>> >rid of this callback?
>>
>> ACPI controllers are in principle not x86-specific, yet our PIIX4 and ICH9 device models always assign an x86-specific function to AcpiDeviceIfClass::madt_cpu. That doesn't seem right because the device models make assumptions about their usage contexts which they ideally shouldn't.
>>
>> In addition, it seems to me that ACPI controllers and AML generation should not be mixed: An ACPI controller deals with (hardware) events while AML generation is usually a BIOS task to inject code into an OS. Therefore, ACPI controllers don't seem to be the right place to assign AML-generating functions because that doesn't match how reality works.
>
>It would be nice to have pristine hardware-only device models
>and firmware composing ACPI tables like baremetal systems do
>(they have a luxury of fixed hardware which makes it way simpler),
>however it was not practical nor sustainable in mainstream virt case.
>That's the reason why ACPI tables (firmware) were moved into QEMU (hardware).
>
>> To solve both issues, one could factor out e.g. an AmlDeviceIf from AcpiDeviceIf, so one would't force the device models to provide an madt_cpu.
>> Then one could assign a target-specific AmlDeviceIfClass::madt_cpu e.g. in board code.
>ACPI tables in QEMU started this way, It stopped being maintainable
>at certain point, see below for more.
>
>>At the moment I don't see a need for a new interface, however, since the function pointer works just fine: It frees the device models from having to provide it and it is used in code which already deals with AML generation.
>When the move happened, the bulk of ACPI code was as you suggest made
>as machine specific and it's still the case for the most of it.
>Then, with ease of adding new related features, it exploded into
>I would say hard to maintain mess. Hence before adding more and
>making it worse, I introduced call_dev_aml_func()/AcpiDevAmlIf
>and started refactoring PCI AML code. That simplified main PCI bus
>enumeration quite a bit.
>
>The new interface however does exactly opposite of what you are doing,
>i.e. it pushes device specific AML generation into corresponding device
>model. It's not ideal as we have to provide stubs for targets where it's
>not needed. But stubs for target independent code is typical and proven
>approach we use in QEMU for such cases and is relatively easy to maintain.
>So I'd call it a reasonable compromise.
>
>Recently posted series (that untangles generic PCI slots AML from
>ACPI PCI hotplug frankenstein) continues with AcpiDevAmlIf approach
>and pushes more AML into target in-depended device models.
I fully agree with the introduction of AcpiDevAmlIf. In fact I was following your work closely since it helped me making the VT82xx south bridges work with the PC machine. In order to make your approach even more elegant and efficient I'm following up with qbus_build_aml().
This patch doesn't actually question the AcpiDevAmlIf. It rather questions the mixing of CPU-related AML concerns into the ACPI controllers which are a priori CPU-agnostic. The only reason for dragging x86 concerns into these device models is that the AcpiDeviceIfClass requires madt_cpu to be assigned. Here we can apply the "interface segregation principle" which is why I proposed factoring out a dedicated interface from the AcpiDeviceIfClass (and possibly have CPUs implement it). I think that this would match your approach with AcpiDevAmlIf.
>And there is more coming on top of it (with the goal to make most
>of what we accumulated in PC/Q35 PCI ACPI code become generic enough
>to replace most of PCI related AML elsewhere (microvm, arm/virt,
>whatever else that is interested in ACPI tables support).
>My ambition with this refactoring stops at making qdev device tree
>self-describable at PCI boundary but AcpiDevAmlIf can be applied
>to other devices described in DSDT, that hang off the board.
Sounds good!
>> My end goal is to make the VT82xx south bridges compatible with x86 and to bring them to feature parity with PIIX. For this, I need to implement the VIA ACPI controller proper, and since these south bridges are currently only used in MIPS and PowerPC contexts I'm feeling a bit uncomfortable having to drag in x86 assumptions into these devices.
>
>basic ACPI hardware (GPE/PM registers) code is relatively isolated,
>so you probably can implement that without touching piix much.
>(with little to no stubs).
>
>If you want to cleanup piix and free other targets from
>unfortunate ACPI/x86 specific dependencies, it would probably
>need another approach than presented here.
>
>ex:
>Isolating core(pristine) piix code in piix-basic class
>for PPC/MIPS use, and then branching/inheriting of it
>current piix class with all extra x86 'features' might
>work without breaking migration. (/me assuming cross
>version migration should work in this arrangement,
>though I won't bet on it)
It seems to me that extending the PIIX4 PM class hierarchy with piix-basic is just a workaround for the root problem I mentioned above, which is the mixing of unrelated concerns into one interface. So let my try to factor out an AcpiCpuAmlIf from AcpiDeviceIfClass and see how it goes.
Best regards,
Bernhard
>
>> Best regards,
>> Bernhard
>>
>> >
>> >> Signed-off-by: Bernhard Beschow <shentey@gmail.com>
>> >> ---
>> >> hw/i386/acpi-common.h | 7 +++++--
>> >> include/hw/acpi/acpi_dev_interface.h | 2 --
>> >> include/hw/acpi/cpu.h | 6 +++++-
>> >> include/hw/i386/pc.h | 4 ----
>> >> hw/acpi/acpi-x86-stub.c | 6 ------
>> >> hw/acpi/cpu.c | 10 ++++------
>> >> hw/acpi/piix4.c | 2 --
>> >> hw/i386/acpi-build.c | 5 ++---
>> >> hw/i386/acpi-common.c | 5 ++---
>> >> hw/i386/acpi-microvm.c | 3 +--
>> >> hw/i386/generic_event_device_x86.c | 9 ---------
>> >> hw/isa/lpc_ich9.c | 1 -
>> >> 12 files changed, 19 insertions(+), 41 deletions(-)
>> >>
>> >> diff --git a/hw/i386/acpi-common.h b/hw/i386/acpi-common.h
>> >> index a68825acf5..968d625d88 100644
>> >> --- a/hw/i386/acpi-common.h
>> >> +++ b/hw/i386/acpi-common.h
>> >> @@ -1,15 +1,18 @@
>> >> #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"
>> >> +#include "hw/boards.h"
>> >>
>> >> /* Default IOAPIC ID */
>> >> #define ACPI_BUILD_IOAPIC_ID 0x0
>> >>
>> >> +void pc_madt_cpu_entry(int uid, const CPUArchIdList *apic_ids, GArray *entry,
>> >> + bool force_enabled);
>> >> +
>> >> 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_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/include/hw/acpi/cpu.h b/include/hw/acpi/cpu.h
>> >> index 999caaf510..25b25bb594 100644
>> >> --- a/include/hw/acpi/cpu.h
>> >> +++ b/include/hw/acpi/cpu.h
>> >> @@ -15,6 +15,7 @@
>> >> #include "hw/qdev-core.h"
>> >> #include "hw/acpi/acpi.h"
>> >> #include "hw/acpi/aml-build.h"
>> >> +#include "hw/boards.h"
>> >> #include "hw/hotplug.h"
>> >>
>> >> typedef struct AcpiCpuStatus {
>> >> @@ -55,8 +56,11 @@ typedef struct CPUHotplugFeatures {
>> >> const char *smi_path;
>> >> } CPUHotplugFeatures;
>> >>
>> >> +typedef void (*madt_cpu_fn)(int uid, const CPUArchIdList *apic_ids,
>> >> + GArray *entry, bool force_enabled);
>> >> +
>> >> void build_cpus_aml(Aml *table, MachineState *machine, CPUHotplugFeatures opts,
>> >> - hwaddr io_base,
>> >> + hwaddr io_base, madt_cpu_fn madt_cpu,
>> >> const char *res_root,
>> >> const char *event_handler_method);
>> >>
>> >> diff --git a/include/hw/i386/pc.h b/include/hw/i386/pc.h
>> >> index a0647165d1..a5cce88653 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/acpi/cpu.c b/hw/acpi/cpu.c
>> >> index 19c154d78f..db15f9278d 100644
>> >> --- a/hw/acpi/cpu.c
>> >> +++ b/hw/acpi/cpu.c
>> >> @@ -338,7 +338,7 @@ const VMStateDescription vmstate_cpu_hotplug = {
>> >> #define CPU_FW_EJECT_EVENT "CEJF"
>> >>
>> >> void build_cpus_aml(Aml *table, MachineState *machine, CPUHotplugFeatures opts,
>> >> - hwaddr io_base,
>> >> + hwaddr io_base, madt_cpu_fn madt_cpu,
>> >> const char *res_root,
>> >> const char *event_handler_method)
>> >> {
>> >> @@ -353,8 +353,8 @@ 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);
>> >> +
>> >> + assert(madt_cpu);
>> >>
>> >> cpu_ctrl_dev = aml_device("%s", cphp_res_path);
>> >> {
>> >> @@ -664,9 +664,7 @@ 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 */
>> >> + 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 0a81f1ad93..4d0d4fdeeb 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/southbridge/piix.h"
>> >> #include "hw/irq.h"
>> >> #include "hw/isa/apm.h"
>> >> @@ -643,7 +642,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 127c4e2d50..0be3960a37 100644
>> >> --- a/hw/i386/acpi-build.c
>> >> +++ b/hw/i386/acpi-build.c
>> >> @@ -1440,7 +1440,7 @@ build_dsdt(GArray *table_data, BIOSLinker *linker,
>> >> .fw_unplugs_cpu = pm->smi_on_cpu_unplug,
>> >> };
>> >> build_cpus_aml(dsdt, machine, opts, pm->cpu_hp_io_base,
>> >> - "\\_SB.PCI0", "\\_GPE._E02");
>> >> + pc_madt_cpu_entry, "\\_SB.PCI0", "\\_GPE._E02");
>> >> }
>> >>
>> >> if (pcms->memhp_io_base && nr_mem) {
>> >> @@ -2424,8 +2424,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..aabf78092e 100644
>> >> --- a/hw/i386/acpi-common.c
>> >> +++ b/hw/i386/acpi-common.c
>> >> @@ -94,14 +94,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 +110,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(i, apic_ids, table_data, false);
>> >> + pc_madt_cpu_entry(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 fb09185cbd..d8a444d06c 100644
>> >> --- a/hw/i386/acpi-microvm.c
>> >> +++ b/hw/i386/acpi-microvm.c
>> >> @@ -213,8 +213,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 8d541e2b54..0ab0a341be 100644
>> >> --- a/hw/isa/lpc_ich9.c
>> >> +++ b/hw/isa/lpc_ich9.c
>> >> @@ -870,7 +870,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;
>> >> }
>> >>
>> >
>>
>
^ permalink raw reply [flat|nested] 20+ messages in thread* Re: [PATCH v3 2/7] hw/acpi/acpi_dev_interface: Resolve AcpiDeviceIfClass::madt_cpu
2023-01-19 14:47 ` Bernhard Beschow
@ 2023-01-20 16:34 ` Igor Mammedov
2023-01-21 15:37 ` Bernhard Beschow
0 siblings, 1 reply; 20+ messages in thread
From: Igor Mammedov @ 2023-01-20 16:34 UTC (permalink / raw)
To: Bernhard Beschow
Cc: qemu-devel, qemu-trivial, Richard Henderson, Paolo Bonzini,
Philippe Mathieu-Daudé, Markus Armbruster, Eduardo Habkost,
Michael S. Tsirkin, Ani Sinha, Marcel Apfelbaum, Aurelien Jarno
On Thu, 19 Jan 2023 14:47:41 +0000
Bernhard Beschow <shentey@gmail.com> wrote:
> Am 18. Januar 2023 14:59:05 UTC schrieb Igor Mammedov <imammedo@redhat.com>:
> >On Tue, 17 Jan 2023 00:30:23 +0000
> >Bernhard Beschow <shentey@gmail.com> wrote:
> >
> >> Am 16. Januar 2023 16:29:30 UTC schrieb Igor Mammedov <imammedo@redhat.com>:
> >> >On Mon, 16 Jan 2023 16:29:03 +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.
> >> >>
> >> >> 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.
> >> >
> >> >I'm not sure about this approach,
> >> >the callback is intend to be used not only by x86 but also in
> >> >the end by ARM (it's just that arm/virt CPU hotplug patches are
> >> >still work in progress and haven't been merged).
> >>
> >> IIUC one would call the target-independent build_cpus_aml() from the ARM-specific build_madt(). There, one could pass a function pointer to an ARM-specific madt_cpu_fn. Shouldn't that get us covered?
> >
> >it will work in this case, but I don't like going back to random
> >function pointer argument approach instead of using QOM and
> >interfaces which is much better in describing expectations/contract
> >for interfaces adn objects it's attached to.
> >
> >Howver that is not the reason why I'm against this approach, see bellow.
> >
> >> >So I'd prefer to keep AcpiDeviceIfClass::madt_cpu.
> >> >
> >> >What's the end goal you are trying to achieve by getting
> >> >rid of this callback?
> >>
> >> ACPI controllers are in principle not x86-specific, yet our PIIX4 and ICH9 device models always assign an x86-specific function to AcpiDeviceIfClass::madt_cpu. That doesn't seem right because the device models make assumptions about their usage contexts which they ideally shouldn't.
> >>
> >> In addition, it seems to me that ACPI controllers and AML generation should not be mixed: An ACPI controller deals with (hardware) events while AML generation is usually a BIOS task to inject code into an OS. Therefore, ACPI controllers don't seem to be the right place to assign AML-generating functions because that doesn't match how reality works.
> >
> >It would be nice to have pristine hardware-only device models
> >and firmware composing ACPI tables like baremetal systems do
> >(they have a luxury of fixed hardware which makes it way simpler),
> >however it was not practical nor sustainable in mainstream virt case.
> >That's the reason why ACPI tables (firmware) were moved into QEMU (hardware).
> >
> >> To solve both issues, one could factor out e.g. an AmlDeviceIf from AcpiDeviceIf, so one would't force the device models to provide an madt_cpu.
> >> Then one could assign a target-specific AmlDeviceIfClass::madt_cpu e.g. in board code.
> >ACPI tables in QEMU started this way, It stopped being maintainable
> >at certain point, see below for more.
> >
> >>At the moment I don't see a need for a new interface, however, since the function pointer works just fine: It frees the device models from having to provide it and it is used in code which already deals with AML generation.
> >When the move happened, the bulk of ACPI code was as you suggest made
> >as machine specific and it's still the case for the most of it.
> >Then, with ease of adding new related features, it exploded into
> >I would say hard to maintain mess. Hence before adding more and
> >making it worse, I introduced call_dev_aml_func()/AcpiDevAmlIf
> >and started refactoring PCI AML code. That simplified main PCI bus
> >enumeration quite a bit.
> >
> >The new interface however does exactly opposite of what you are doing,
> >i.e. it pushes device specific AML generation into corresponding device
> >model. It's not ideal as we have to provide stubs for targets where it's
> >not needed. But stubs for target independent code is typical and proven
> >approach we use in QEMU for such cases and is relatively easy to maintain.
> >So I'd call it a reasonable compromise.
> >
> >Recently posted series (that untangles generic PCI slots AML from
> >ACPI PCI hotplug frankenstein) continues with AcpiDevAmlIf approach
> >and pushes more AML into target in-depended device models.
>
> I fully agree with the introduction of AcpiDevAmlIf. In fact I was following your work closely since it helped me making the VT82xx south bridges work with the PC machine. In order to make your approach even more elegant and efficient I'm following up with qbus_build_aml().
Reviewed/acked it, thanks for nice cleanuup.
> This patch doesn't actually question the AcpiDevAmlIf. It rather questions the mixing of CPU-related AML concerns into the ACPI controllers which are a priori CPU-agnostic. The only reason for dragging x86 concerns into these device models is that the AcpiDeviceIfClass requires madt_cpu to be assigned. Here we can apply the "interface segregation principle" which is why I proposed factoring out a dedicated interface from the AcpiDeviceIfClass (and possibly have CPUs implement it). I think that this would match your approach with AcpiDevAmlIf.
There was and still is a reason for madt_cpu being part of AcpiDeviceIfClass and x86-ed piix impl.
(It might have been a poor choice to use acpi controller as hotplug controller as well
but that boat has sailed and can't be fixed without breaking ABI/migration.)
But as it is now, hotplug controller is an owner of CPU hotplug data
on which ::madt_cpu relies.
I'd say the same about ::send_event hook which is only used in hotplug case
(GPE events do not have predefined meaning)
That only leaves AcpiDeviceIfClass::ospm_status, which in turn implicitly
depends on hotplug controller to provide status data.
So question is why do you need AcpiDeviceIfClass for VIA, how much of it
and how current impl. gets in the way of your goal?
> >And there is more coming on top of it (with the goal to make most
> >of what we accumulated in PC/Q35 PCI ACPI code become generic enough
> >to replace most of PCI related AML elsewhere (microvm, arm/virt,
> >whatever else that is interested in ACPI tables support).
> >My ambition with this refactoring stops at making qdev device tree
> >self-describable at PCI boundary but AcpiDevAmlIf can be applied
> >to other devices described in DSDT, that hang off the board.
>
> Sounds good!
>
> >> My end goal is to make the VT82xx south bridges compatible with x86 and to bring them to feature parity with PIIX. For this, I need to implement the VIA ACPI controller proper, and since these south bridges are currently only used in MIPS and PowerPC contexts I'm feeling a bit uncomfortable having to drag in x86 assumptions into these devices.
> >
> >basic ACPI hardware (GPE/PM registers) code is relatively isolated,
> >so you probably can implement that without touching piix much.
> >(with little to no stubs).
> >
> >If you want to cleanup piix and free other targets from
> >unfortunate ACPI/x86 specific dependencies, it would probably
> >need another approach than presented here.
> >
> >ex:
> >Isolating core(pristine) piix code in piix-basic class
> >for PPC/MIPS use, and then branching/inheriting of it
> >current piix class with all extra x86 'features' might
> >work without breaking migration. (/me assuming cross
> >version migration should work in this arrangement,
> >though I won't bet on it)
>
> It seems to me that extending the PIIX4 PM class hierarchy with piix-basic is just a workaround for the root problem I mentioned above, which is the mixing of unrelated concerns into one interface. So let my try to factor out an AcpiCpuAmlIf from AcpiDeviceIfClass and see how it goes.
See above, lets discuss it there.
> Best regards,
> Bernhard
>
> >
> >> Best regards,
> >> Bernhard
> >>
> >> >
> >> >> Signed-off-by: Bernhard Beschow <shentey@gmail.com>
> >> >> ---
> >> >> hw/i386/acpi-common.h | 7 +++++--
> >> >> include/hw/acpi/acpi_dev_interface.h | 2 --
> >> >> include/hw/acpi/cpu.h | 6 +++++-
> >> >> include/hw/i386/pc.h | 4 ----
> >> >> hw/acpi/acpi-x86-stub.c | 6 ------
> >> >> hw/acpi/cpu.c | 10 ++++------
> >> >> hw/acpi/piix4.c | 2 --
> >> >> hw/i386/acpi-build.c | 5 ++---
> >> >> hw/i386/acpi-common.c | 5 ++---
> >> >> hw/i386/acpi-microvm.c | 3 +--
> >> >> hw/i386/generic_event_device_x86.c | 9 ---------
> >> >> hw/isa/lpc_ich9.c | 1 -
> >> >> 12 files changed, 19 insertions(+), 41 deletions(-)
> >> >>
> >> >> diff --git a/hw/i386/acpi-common.h b/hw/i386/acpi-common.h
> >> >> index a68825acf5..968d625d88 100644
> >> >> --- a/hw/i386/acpi-common.h
> >> >> +++ b/hw/i386/acpi-common.h
> >> >> @@ -1,15 +1,18 @@
> >> >> #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"
> >> >> +#include "hw/boards.h"
> >> >>
> >> >> /* Default IOAPIC ID */
> >> >> #define ACPI_BUILD_IOAPIC_ID 0x0
> >> >>
> >> >> +void pc_madt_cpu_entry(int uid, const CPUArchIdList *apic_ids, GArray *entry,
> >> >> + bool force_enabled);
> >> >> +
> >> >> 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_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/include/hw/acpi/cpu.h b/include/hw/acpi/cpu.h
> >> >> index 999caaf510..25b25bb594 100644
> >> >> --- a/include/hw/acpi/cpu.h
> >> >> +++ b/include/hw/acpi/cpu.h
> >> >> @@ -15,6 +15,7 @@
> >> >> #include "hw/qdev-core.h"
> >> >> #include "hw/acpi/acpi.h"
> >> >> #include "hw/acpi/aml-build.h"
> >> >> +#include "hw/boards.h"
> >> >> #include "hw/hotplug.h"
> >> >>
> >> >> typedef struct AcpiCpuStatus {
> >> >> @@ -55,8 +56,11 @@ typedef struct CPUHotplugFeatures {
> >> >> const char *smi_path;
> >> >> } CPUHotplugFeatures;
> >> >>
> >> >> +typedef void (*madt_cpu_fn)(int uid, const CPUArchIdList *apic_ids,
> >> >> + GArray *entry, bool force_enabled);
> >> >> +
> >> >> void build_cpus_aml(Aml *table, MachineState *machine, CPUHotplugFeatures opts,
> >> >> - hwaddr io_base,
> >> >> + hwaddr io_base, madt_cpu_fn madt_cpu,
> >> >> const char *res_root,
> >> >> const char *event_handler_method);
> >> >>
> >> >> diff --git a/include/hw/i386/pc.h b/include/hw/i386/pc.h
> >> >> index a0647165d1..a5cce88653 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/acpi/cpu.c b/hw/acpi/cpu.c
> >> >> index 19c154d78f..db15f9278d 100644
> >> >> --- a/hw/acpi/cpu.c
> >> >> +++ b/hw/acpi/cpu.c
> >> >> @@ -338,7 +338,7 @@ const VMStateDescription vmstate_cpu_hotplug = {
> >> >> #define CPU_FW_EJECT_EVENT "CEJF"
> >> >>
> >> >> void build_cpus_aml(Aml *table, MachineState *machine, CPUHotplugFeatures opts,
> >> >> - hwaddr io_base,
> >> >> + hwaddr io_base, madt_cpu_fn madt_cpu,
> >> >> const char *res_root,
> >> >> const char *event_handler_method)
> >> >> {
> >> >> @@ -353,8 +353,8 @@ 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);
> >> >> +
> >> >> + assert(madt_cpu);
> >> >>
> >> >> cpu_ctrl_dev = aml_device("%s", cphp_res_path);
> >> >> {
> >> >> @@ -664,9 +664,7 @@ 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 */
> >> >> + 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 0a81f1ad93..4d0d4fdeeb 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/southbridge/piix.h"
> >> >> #include "hw/irq.h"
> >> >> #include "hw/isa/apm.h"
> >> >> @@ -643,7 +642,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 127c4e2d50..0be3960a37 100644
> >> >> --- a/hw/i386/acpi-build.c
> >> >> +++ b/hw/i386/acpi-build.c
> >> >> @@ -1440,7 +1440,7 @@ build_dsdt(GArray *table_data, BIOSLinker *linker,
> >> >> .fw_unplugs_cpu = pm->smi_on_cpu_unplug,
> >> >> };
> >> >> build_cpus_aml(dsdt, machine, opts, pm->cpu_hp_io_base,
> >> >> - "\\_SB.PCI0", "\\_GPE._E02");
> >> >> + pc_madt_cpu_entry, "\\_SB.PCI0", "\\_GPE._E02");
> >> >> }
> >> >>
> >> >> if (pcms->memhp_io_base && nr_mem) {
> >> >> @@ -2424,8 +2424,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..aabf78092e 100644
> >> >> --- a/hw/i386/acpi-common.c
> >> >> +++ b/hw/i386/acpi-common.c
> >> >> @@ -94,14 +94,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 +110,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(i, apic_ids, table_data, false);
> >> >> + pc_madt_cpu_entry(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 fb09185cbd..d8a444d06c 100644
> >> >> --- a/hw/i386/acpi-microvm.c
> >> >> +++ b/hw/i386/acpi-microvm.c
> >> >> @@ -213,8 +213,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 8d541e2b54..0ab0a341be 100644
> >> >> --- a/hw/isa/lpc_ich9.c
> >> >> +++ b/hw/isa/lpc_ich9.c
> >> >> @@ -870,7 +870,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;
> >> >> }
> >> >>
> >> >
> >>
> >
>
^ permalink raw reply [flat|nested] 20+ messages in thread* Re: [PATCH v3 2/7] hw/acpi/acpi_dev_interface: Resolve AcpiDeviceIfClass::madt_cpu
2023-01-20 16:34 ` Igor Mammedov
@ 2023-01-21 15:37 ` Bernhard Beschow
2023-01-24 12:13 ` Igor Mammedov
0 siblings, 1 reply; 20+ messages in thread
From: Bernhard Beschow @ 2023-01-21 15:37 UTC (permalink / raw)
To: Igor Mammedov
Cc: qemu-devel, qemu-trivial, Richard Henderson, Paolo Bonzini,
Philippe Mathieu-Daudé, Markus Armbruster, Eduardo Habkost,
Michael S. Tsirkin, Ani Sinha, Marcel Apfelbaum, Aurelien Jarno
[-- Attachment #1: Type: text/plain, Size: 22444 bytes --]
On Fri, Jan 20, 2023 at 5:34 PM Igor Mammedov <imammedo@redhat.com> wrote:
> On Thu, 19 Jan 2023 14:47:41 +0000
> Bernhard Beschow <shentey@gmail.com> wrote:
>
> > Am 18. Januar 2023 14:59:05 UTC schrieb Igor Mammedov <
> imammedo@redhat.com>:
> > >On Tue, 17 Jan 2023 00:30:23 +0000
> > >Bernhard Beschow <shentey@gmail.com> wrote:
> > >
> > >> Am 16. Januar 2023 16:29:30 UTC schrieb Igor Mammedov <
> imammedo@redhat.com>:
> > >> >On Mon, 16 Jan 2023 16:29:03 +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.
> > >> >>
> > >> >> 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.
> > >> >
> > >> >I'm not sure about this approach,
> > >> >the callback is intend to be used not only by x86 but also in
> > >> >the end by ARM (it's just that arm/virt CPU hotplug patches are
> > >> >still work in progress and haven't been merged).
> > >>
> > >> IIUC one would call the target-independent build_cpus_aml() from the
> ARM-specific build_madt(). There, one could pass a function pointer to an
> ARM-specific madt_cpu_fn. Shouldn't that get us covered?
> > >
> > >it will work in this case, but I don't like going back to random
> > >function pointer argument approach instead of using QOM and
> > >interfaces which is much better in describing expectations/contract
> > >for interfaces adn objects it's attached to.
> > >
> > >Howver that is not the reason why I'm against this approach, see bellow.
> > >
> > >> >So I'd prefer to keep AcpiDeviceIfClass::madt_cpu.
> > >> >
> > >> >What's the end goal you are trying to achieve by getting
> > >> >rid of this callback?
> > >>
> > >> ACPI controllers are in principle not x86-specific, yet our PIIX4 and
> ICH9 device models always assign an x86-specific function to
> AcpiDeviceIfClass::madt_cpu. That doesn't seem right because the device
> models make assumptions about their usage contexts which they ideally
> shouldn't.
> > >>
> > >> In addition, it seems to me that ACPI controllers and AML generation
> should not be mixed: An ACPI controller deals with (hardware) events while
> AML generation is usually a BIOS task to inject code into an OS. Therefore,
> ACPI controllers don't seem to be the right place to assign AML-generating
> functions because that doesn't match how reality works.
> > >
> > >It would be nice to have pristine hardware-only device models
> > >and firmware composing ACPI tables like baremetal systems do
> > >(they have a luxury of fixed hardware which makes it way simpler),
> > >however it was not practical nor sustainable in mainstream virt case.
> > >That's the reason why ACPI tables (firmware) were moved into QEMU
> (hardware).
> > >
> > >> To solve both issues, one could factor out e.g. an AmlDeviceIf from
> AcpiDeviceIf, so one would't force the device models to provide an
> madt_cpu.
> > >> Then one could assign a target-specific AmlDeviceIfClass::madt_cpu
> e.g. in board code.
> > >ACPI tables in QEMU started this way, It stopped being maintainable
> > >at certain point, see below for more.
> > >
> > >>At the moment I don't see a need for a new interface, however, since
> the function pointer works just fine: It frees the device models from
> having to provide it and it is used in code which already deals with AML
> generation.
> > >When the move happened, the bulk of ACPI code was as you suggest made
> > >as machine specific and it's still the case for the most of it.
> > >Then, with ease of adding new related features, it exploded into
> > >I would say hard to maintain mess. Hence before adding more and
> > >making it worse, I introduced call_dev_aml_func()/AcpiDevAmlIf
> > >and started refactoring PCI AML code. That simplified main PCI bus
> > >enumeration quite a bit.
> > >
> > >The new interface however does exactly opposite of what you are doing,
> > >i.e. it pushes device specific AML generation into corresponding device
> > >model. It's not ideal as we have to provide stubs for targets where it's
> > >not needed. But stubs for target independent code is typical and proven
> > >approach we use in QEMU for such cases and is relatively easy to
> maintain.
> > >So I'd call it a reasonable compromise.
> > >
> > >Recently posted series (that untangles generic PCI slots AML from
> > >ACPI PCI hotplug frankenstein) continues with AcpiDevAmlIf approach
> > >and pushes more AML into target in-depended device models.
> >
> > I fully agree with the introduction of AcpiDevAmlIf. In fact I was
> following your work closely since it helped me making the VT82xx south
> bridges work with the PC machine. In order to make your approach even more
> elegant and efficient I'm following up with qbus_build_aml().
>
> Reviewed/acked it, thanks for nice cleanuup.
>
> > This patch doesn't actually question the AcpiDevAmlIf. It rather
> questions the mixing of CPU-related AML concerns into the ACPI controllers
> which are a priori CPU-agnostic. The only reason for dragging x86 concerns
> into these device models is that the AcpiDeviceIfClass requires madt_cpu to
> be assigned. Here we can apply the "interface segregation principle" which
> is why I proposed factoring out a dedicated interface from the
> AcpiDeviceIfClass (and possibly have CPUs implement it). I think that this
> would match your approach with AcpiDevAmlIf.
>
> There was and still is a reason for madt_cpu being part of
> AcpiDeviceIfClass and x86-ed piix impl.
>
> (It might have been a poor choice to use acpi controller as hotplug
> controller as well
> but that boat has sailed and can't be fixed without breaking
> ABI/migration.)
>
Could you please elaborate a bit on how the ABI is affected? Migration
seems clear to me.
>
> But as it is now, hotplug controller is an owner of CPU hotplug data
> on which ::madt_cpu relies.
>
Could you please also elaborate a bit here on how hotplug and ::madt_cpu
are coupled? When generating the AML, one could check if both
AcpiDeviceIfClass and ::madt_cpu are available. There is code instantiating
TYPE_ACPI_GED in ARM and Loongarch which does not set ::madt_cpu. In order
to see what I mean, I've prepared v4 of this series:
https://lore.kernel.org/qemu-devel/20230121151941.24120-1-shentey@gmail.com
I'd say the same about ::send_event hook which is only used in hotplug case
> (GPE events do not have predefined meaning)
> That only leaves AcpiDeviceIfClass::ospm_status, which in turn implicitly
> depends on hotplug controller to provide status data.
> So question is why do you need AcpiDeviceIfClass for VIA, how much of it
> and how current impl. gets in the way of your goal?
>
I would just like to get feature parity with PIIX. Do you suggest taking a
different approach which avoids the hotplug state in the ACPI controller?
Thanks,
Bernhard
>
> > >And there is more coming on top of it (with the goal to make most
> > >of what we accumulated in PC/Q35 PCI ACPI code become generic enough
> > >to replace most of PCI related AML elsewhere (microvm, arm/virt,
> > >whatever else that is interested in ACPI tables support).
> > >My ambition with this refactoring stops at making qdev device tree
> > >self-describable at PCI boundary but AcpiDevAmlIf can be applied
> > >to other devices described in DSDT, that hang off the board.
> >
> > Sounds good!
> >
> > >> My end goal is to make the VT82xx south bridges compatible with x86
> and to bring them to feature parity with PIIX. For this, I need to
> implement the VIA ACPI controller proper, and since these south bridges are
> currently only used in MIPS and PowerPC contexts I'm feeling a bit
> uncomfortable having to drag in x86 assumptions into these devices.
> > >
> > >basic ACPI hardware (GPE/PM registers) code is relatively isolated,
> > >so you probably can implement that without touching piix much.
> > >(with little to no stubs).
> > >
> > >If you want to cleanup piix and free other targets from
> > >unfortunate ACPI/x86 specific dependencies, it would probably
> > >need another approach than presented here.
> > >
> > >ex:
> > >Isolating core(pristine) piix code in piix-basic class
> > >for PPC/MIPS use, and then branching/inheriting of it
> > >current piix class with all extra x86 'features' might
> > >work without breaking migration. (/me assuming cross
> > >version migration should work in this arrangement,
> > >though I won't bet on it)
> >
> > It seems to me that extending the PIIX4 PM class hierarchy with
> piix-basic is just a workaround for the root problem I mentioned above,
> which is the mixing of unrelated concerns into one interface. So let my try
> to factor out an AcpiCpuAmlIf from AcpiDeviceIfClass and see how it goes.
> See above, lets discuss it there.
>
> > Best regards,
> > Bernhard
> >
> > >
> > >> Best regards,
> > >> Bernhard
> > >>
> > >> >
> > >> >> Signed-off-by: Bernhard Beschow <shentey@gmail.com>
> > >> >> ---
> > >> >> hw/i386/acpi-common.h | 7 +++++--
> > >> >> include/hw/acpi/acpi_dev_interface.h | 2 --
> > >> >> include/hw/acpi/cpu.h | 6 +++++-
> > >> >> include/hw/i386/pc.h | 4 ----
> > >> >> hw/acpi/acpi-x86-stub.c | 6 ------
> > >> >> hw/acpi/cpu.c | 10 ++++------
> > >> >> hw/acpi/piix4.c | 2 --
> > >> >> hw/i386/acpi-build.c | 5 ++---
> > >> >> hw/i386/acpi-common.c | 5 ++---
> > >> >> hw/i386/acpi-microvm.c | 3 +--
> > >> >> hw/i386/generic_event_device_x86.c | 9 ---------
> > >> >> hw/isa/lpc_ich9.c | 1 -
> > >> >> 12 files changed, 19 insertions(+), 41 deletions(-)
> > >> >>
> > >> >> diff --git a/hw/i386/acpi-common.h b/hw/i386/acpi-common.h
> > >> >> index a68825acf5..968d625d88 100644
> > >> >> --- a/hw/i386/acpi-common.h
> > >> >> +++ b/hw/i386/acpi-common.h
> > >> >> @@ -1,15 +1,18 @@
> > >> >> #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"
> > >> >> +#include "hw/boards.h"
> > >> >>
> > >> >> /* Default IOAPIC ID */
> > >> >> #define ACPI_BUILD_IOAPIC_ID 0x0
> > >> >>
> > >> >> +void pc_madt_cpu_entry(int uid, const CPUArchIdList *apic_ids,
> GArray *entry,
> > >> >> + bool force_enabled);
> > >> >> +
> > >> >> 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_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/include/hw/acpi/cpu.h b/include/hw/acpi/cpu.h
> > >> >> index 999caaf510..25b25bb594 100644
> > >> >> --- a/include/hw/acpi/cpu.h
> > >> >> +++ b/include/hw/acpi/cpu.h
> > >> >> @@ -15,6 +15,7 @@
> > >> >> #include "hw/qdev-core.h"
> > >> >> #include "hw/acpi/acpi.h"
> > >> >> #include "hw/acpi/aml-build.h"
> > >> >> +#include "hw/boards.h"
> > >> >> #include "hw/hotplug.h"
> > >> >>
> > >> >> typedef struct AcpiCpuStatus {
> > >> >> @@ -55,8 +56,11 @@ typedef struct CPUHotplugFeatures {
> > >> >> const char *smi_path;
> > >> >> } CPUHotplugFeatures;
> > >> >>
> > >> >> +typedef void (*madt_cpu_fn)(int uid, const CPUArchIdList
> *apic_ids,
> > >> >> + GArray *entry, bool force_enabled);
> > >> >> +
> > >> >> void build_cpus_aml(Aml *table, MachineState *machine,
> CPUHotplugFeatures opts,
> > >> >> - hwaddr io_base,
> > >> >> + hwaddr io_base, madt_cpu_fn madt_cpu,
> > >> >> const char *res_root,
> > >> >> const char *event_handler_method);
> > >> >>
> > >> >> diff --git a/include/hw/i386/pc.h b/include/hw/i386/pc.h
> > >> >> index a0647165d1..a5cce88653 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/acpi/cpu.c b/hw/acpi/cpu.c
> > >> >> index 19c154d78f..db15f9278d 100644
> > >> >> --- a/hw/acpi/cpu.c
> > >> >> +++ b/hw/acpi/cpu.c
> > >> >> @@ -338,7 +338,7 @@ const VMStateDescription vmstate_cpu_hotplug =
> {
> > >> >> #define CPU_FW_EJECT_EVENT "CEJF"
> > >> >>
> > >> >> void build_cpus_aml(Aml *table, MachineState *machine,
> CPUHotplugFeatures opts,
> > >> >> - hwaddr io_base,
> > >> >> + hwaddr io_base, madt_cpu_fn madt_cpu,
> > >> >> const char *res_root,
> > >> >> const char *event_handler_method)
> > >> >> {
> > >> >> @@ -353,8 +353,8 @@ 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);
> > >> >> +
> > >> >> + assert(madt_cpu);
> > >> >>
> > >> >> cpu_ctrl_dev = aml_device("%s", cphp_res_path);
> > >> >> {
> > >> >> @@ -664,9 +664,7 @@ 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 */
> > >> >> + 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 0a81f1ad93..4d0d4fdeeb 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/southbridge/piix.h"
> > >> >> #include "hw/irq.h"
> > >> >> #include "hw/isa/apm.h"
> > >> >> @@ -643,7 +642,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 127c4e2d50..0be3960a37 100644
> > >> >> --- a/hw/i386/acpi-build.c
> > >> >> +++ b/hw/i386/acpi-build.c
> > >> >> @@ -1440,7 +1440,7 @@ build_dsdt(GArray *table_data, BIOSLinker
> *linker,
> > >> >> .fw_unplugs_cpu = pm->smi_on_cpu_unplug,
> > >> >> };
> > >> >> build_cpus_aml(dsdt, machine, opts, pm->cpu_hp_io_base,
> > >> >> - "\\_SB.PCI0", "\\_GPE._E02");
> > >> >> + pc_madt_cpu_entry, "\\_SB.PCI0",
> "\\_GPE._E02");
> > >> >> }
> > >> >>
> > >> >> if (pcms->memhp_io_base && nr_mem) {
> > >> >> @@ -2424,8 +2424,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..aabf78092e 100644
> > >> >> --- a/hw/i386/acpi-common.c
> > >> >> +++ b/hw/i386/acpi-common.c
> > >> >> @@ -94,14 +94,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 +110,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(i, apic_ids, table_data, false);
> > >> >> + pc_madt_cpu_entry(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 fb09185cbd..d8a444d06c 100644
> > >> >> --- a/hw/i386/acpi-microvm.c
> > >> >> +++ b/hw/i386/acpi-microvm.c
> > >> >> @@ -213,8 +213,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 8d541e2b54..0ab0a341be 100644
> > >> >> --- a/hw/isa/lpc_ich9.c
> > >> >> +++ b/hw/isa/lpc_ich9.c
> > >> >> @@ -870,7 +870,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;
> > >> >> }
> > >> >>
> > >> >
> > >>
> > >
> >
>
>
[-- Attachment #2: Type: text/html, Size: 31247 bytes --]
^ permalink raw reply [flat|nested] 20+ messages in thread* Re: [PATCH v3 2/7] hw/acpi/acpi_dev_interface: Resolve AcpiDeviceIfClass::madt_cpu
2023-01-21 15:37 ` Bernhard Beschow
@ 2023-01-24 12:13 ` Igor Mammedov
0 siblings, 0 replies; 20+ messages in thread
From: Igor Mammedov @ 2023-01-24 12:13 UTC (permalink / raw)
To: Bernhard Beschow
Cc: qemu-devel, qemu-trivial, Richard Henderson, Paolo Bonzini,
Philippe Mathieu-Daudé, Markus Armbruster, Eduardo Habkost,
Michael S. Tsirkin, Ani Sinha, Marcel Apfelbaum, Aurelien Jarno
On Sat, 21 Jan 2023 16:37:26 +0100
Bernhard Beschow <shentey@gmail.com> wrote:
> On Fri, Jan 20, 2023 at 5:34 PM Igor Mammedov <imammedo@redhat.com> wrote:
>
> > On Thu, 19 Jan 2023 14:47:41 +0000
> > Bernhard Beschow <shentey@gmail.com> wrote:
> >
> > > Am 18. Januar 2023 14:59:05 UTC schrieb Igor Mammedov <
> > imammedo@redhat.com>:
> > > >On Tue, 17 Jan 2023 00:30:23 +0000
> > > >Bernhard Beschow <shentey@gmail.com> wrote:
> > > >
> > > >> Am 16. Januar 2023 16:29:30 UTC schrieb Igor Mammedov <
> > imammedo@redhat.com>:
> > > >> >On Mon, 16 Jan 2023 16:29:03 +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.
> > > >> >>
> > > >> >> 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.
> > > >> >
> > > >> >I'm not sure about this approach,
> > > >> >the callback is intend to be used not only by x86 but also in
> > > >> >the end by ARM (it's just that arm/virt CPU hotplug patches are
> > > >> >still work in progress and haven't been merged).
> > > >>
> > > >> IIUC one would call the target-independent build_cpus_aml() from the
> > ARM-specific build_madt(). There, one could pass a function pointer to an
> > ARM-specific madt_cpu_fn. Shouldn't that get us covered?
> > > >
> > > >it will work in this case, but I don't like going back to random
> > > >function pointer argument approach instead of using QOM and
> > > >interfaces which is much better in describing expectations/contract
> > > >for interfaces adn objects it's attached to.
> > > >
> > > >Howver that is not the reason why I'm against this approach, see bellow.
> > > >
> > > >> >So I'd prefer to keep AcpiDeviceIfClass::madt_cpu.
> > > >> >
> > > >> >What's the end goal you are trying to achieve by getting
> > > >> >rid of this callback?
> > > >>
> > > >> ACPI controllers are in principle not x86-specific, yet our PIIX4 and
> > ICH9 device models always assign an x86-specific function to
> > AcpiDeviceIfClass::madt_cpu. That doesn't seem right because the device
> > models make assumptions about their usage contexts which they ideally
> > shouldn't.
> > > >>
> > > >> In addition, it seems to me that ACPI controllers and AML generation
> > should not be mixed: An ACPI controller deals with (hardware) events while
> > AML generation is usually a BIOS task to inject code into an OS. Therefore,
> > ACPI controllers don't seem to be the right place to assign AML-generating
> > functions because that doesn't match how reality works.
> > > >
> > > >It would be nice to have pristine hardware-only device models
> > > >and firmware composing ACPI tables like baremetal systems do
> > > >(they have a luxury of fixed hardware which makes it way simpler),
> > > >however it was not practical nor sustainable in mainstream virt case.
> > > >That's the reason why ACPI tables (firmware) were moved into QEMU
> > (hardware).
> > > >
> > > >> To solve both issues, one could factor out e.g. an AmlDeviceIf from
> > AcpiDeviceIf, so one would't force the device models to provide an
> > madt_cpu.
> > > >> Then one could assign a target-specific AmlDeviceIfClass::madt_cpu
> > e.g. in board code.
> > > >ACPI tables in QEMU started this way, It stopped being maintainable
> > > >at certain point, see below for more.
> > > >
> > > >>At the moment I don't see a need for a new interface, however, since
> > the function pointer works just fine: It frees the device models from
> > having to provide it and it is used in code which already deals with AML
> > generation.
> > > >When the move happened, the bulk of ACPI code was as you suggest made
> > > >as machine specific and it's still the case for the most of it.
> > > >Then, with ease of adding new related features, it exploded into
> > > >I would say hard to maintain mess. Hence before adding more and
> > > >making it worse, I introduced call_dev_aml_func()/AcpiDevAmlIf
> > > >and started refactoring PCI AML code. That simplified main PCI bus
> > > >enumeration quite a bit.
> > > >
> > > >The new interface however does exactly opposite of what you are doing,
> > > >i.e. it pushes device specific AML generation into corresponding device
> > > >model. It's not ideal as we have to provide stubs for targets where it's
> > > >not needed. But stubs for target independent code is typical and proven
> > > >approach we use in QEMU for such cases and is relatively easy to
> > maintain.
> > > >So I'd call it a reasonable compromise.
> > > >
> > > >Recently posted series (that untangles generic PCI slots AML from
> > > >ACPI PCI hotplug frankenstein) continues with AcpiDevAmlIf approach
> > > >and pushes more AML into target in-depended device models.
> > >
> > > I fully agree with the introduction of AcpiDevAmlIf. In fact I was
> > following your work closely since it helped me making the VT82xx south
> > bridges work with the PC machine. In order to make your approach even more
> > elegant and efficient I'm following up with qbus_build_aml().
> >
> > Reviewed/acked it, thanks for nice cleanuup.
> >
> > > This patch doesn't actually question the AcpiDevAmlIf. It rather
> > questions the mixing of CPU-related AML concerns into the ACPI controllers
> > which are a priori CPU-agnostic. The only reason for dragging x86 concerns
> > into these device models is that the AcpiDeviceIfClass requires madt_cpu to
> > be assigned. Here we can apply the "interface segregation principle" which
> > is why I proposed factoring out a dedicated interface from the
> > AcpiDeviceIfClass (and possibly have CPUs implement it). I think that this
> > would match your approach with AcpiDevAmlIf.
> >
> > There was and still is a reason for madt_cpu being part of
> > AcpiDeviceIfClass and x86-ed piix impl.
> >
> > (It might have been a poor choice to use acpi controller as hotplug
> > controller as well
> > but that boat has sailed and can't be fixed without breaking
> > ABI/migration.)
> >
>
> Could you please elaborate a bit on how the ABI is affected? Migration
> seems clear to me.
What I'm saying wrt piix refactoring is that ACPI part of piix exposes
registers (including made up ones) to guest and then migrates various
hotplug state.
That behavior should not change when doing cross version migration.
(upstream policy is: version-ed machine type from older QEMU
should be migratable to new QEMU with the same machine type and
keep exposed old QEMU ABI behavior the same, and nice to have
would be backward also working)
I don't think it's possible to isolate/remove hotplug controller
from piix without breaking ABI, hence was a suggestion to have
piix-basic and leave qemu extensions in current piix which
should be inherited from piix-basic. This way PPC target would
pull only piix-basic and drop ACPI dependencies.
> > But as it is now, hotplug controller is an owner of CPU hotplug data
> > on which ::madt_cpu relies.
> >
>
> Could you please also elaborate a bit here on how hotplug and ::madt_cpu
> are coupled? When generating the AML, one could check if both
> AcpiDeviceIfClass and ::madt_cpu are available. There is code instantiating
> TYPE_ACPI_GED in ARM and Loongarch which does not set ::madt_cpu. In order
> to see what I mean, I've prepared v4 of this series:
> https://lore.kernel.org/qemu-devel/20230121151941.24120-1-shentey@gmail.com
>
> I'd say the same about ::send_event hook which is only used in hotplug case
> > (GPE events do not have predefined meaning)
> > That only leaves AcpiDeviceIfClass::ospm_status, which in turn implicitly
> > depends on hotplug controller to provide status data.
> > So question is why do you need AcpiDeviceIfClass for VIA, how much of it
> > and how current impl. gets in the way of your goal?
> >
>
> I would just like to get feature parity with PIIX. Do you suggest taking a
> different approach which avoids the hotplug state in the ACPI controller?
I'd avoid pulling QEMU specific ACPI hotplug into VIA
(we already have trouble as it is with piix pulling in
unrelevant ACPI dependencies into other targets)
If one really needs ACPI hotplug on machine that uses VIA,
I'd rather inherit via-qemu from basic VIA (as per spec)
or better reuse generic hotplug controller (GED_DEVICE)
that we already have (used in virt-arm/microvm). Though
the later would limit guest OS to something modern (since
it uses other means then legacy GPE registers block).
>
> Thanks,
> Bernhard
>
> >
> > > >And there is more coming on top of it (with the goal to make most
> > > >of what we accumulated in PC/Q35 PCI ACPI code become generic enough
> > > >to replace most of PCI related AML elsewhere (microvm, arm/virt,
> > > >whatever else that is interested in ACPI tables support).
> > > >My ambition with this refactoring stops at making qdev device tree
> > > >self-describable at PCI boundary but AcpiDevAmlIf can be applied
> > > >to other devices described in DSDT, that hang off the board.
> > >
> > > Sounds good!
> > >
> > > >> My end goal is to make the VT82xx south bridges compatible with x86
> > and to bring them to feature parity with PIIX. For this, I need to
> > implement the VIA ACPI controller proper, and since these south bridges are
> > currently only used in MIPS and PowerPC contexts I'm feeling a bit
> > uncomfortable having to drag in x86 assumptions into these devices.
> > > >
> > > >basic ACPI hardware (GPE/PM registers) code is relatively isolated,
> > > >so you probably can implement that without touching piix much.
> > > >(with little to no stubs).
> > > >
> > > >If you want to cleanup piix and free other targets from
> > > >unfortunate ACPI/x86 specific dependencies, it would probably
> > > >need another approach than presented here.
> > > >
> > > >ex:
> > > >Isolating core(pristine) piix code in piix-basic class
> > > >for PPC/MIPS use, and then branching/inheriting of it
> > > >current piix class with all extra x86 'features' might
> > > >work without breaking migration. (/me assuming cross
> > > >version migration should work in this arrangement,
> > > >though I won't bet on it)
> > >
> > > It seems to me that extending the PIIX4 PM class hierarchy with
> > piix-basic is just a workaround for the root problem I mentioned above,
> > which is the mixing of unrelated concerns into one interface. So let my try
> > to factor out an AcpiCpuAmlIf from AcpiDeviceIfClass and see how it goes.
> > See above, lets discuss it there.
> >
> > > Best regards,
> > > Bernhard
> > >
> > > >
> > > >> Best regards,
> > > >> Bernhard
> > > >>
> > > >> >
> > > >> >> Signed-off-by: Bernhard Beschow <shentey@gmail.com>
> > > >> >> ---
> > > >> >> hw/i386/acpi-common.h | 7 +++++--
> > > >> >> include/hw/acpi/acpi_dev_interface.h | 2 --
> > > >> >> include/hw/acpi/cpu.h | 6 +++++-
> > > >> >> include/hw/i386/pc.h | 4 ----
> > > >> >> hw/acpi/acpi-x86-stub.c | 6 ------
> > > >> >> hw/acpi/cpu.c | 10 ++++------
> > > >> >> hw/acpi/piix4.c | 2 --
> > > >> >> hw/i386/acpi-build.c | 5 ++---
> > > >> >> hw/i386/acpi-common.c | 5 ++---
> > > >> >> hw/i386/acpi-microvm.c | 3 +--
> > > >> >> hw/i386/generic_event_device_x86.c | 9 ---------
> > > >> >> hw/isa/lpc_ich9.c | 1 -
> > > >> >> 12 files changed, 19 insertions(+), 41 deletions(-)
> > > >> >>
> > > >> >> diff --git a/hw/i386/acpi-common.h b/hw/i386/acpi-common.h
> > > >> >> index a68825acf5..968d625d88 100644
> > > >> >> --- a/hw/i386/acpi-common.h
> > > >> >> +++ b/hw/i386/acpi-common.h
> > > >> >> @@ -1,15 +1,18 @@
> > > >> >> #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"
> > > >> >> +#include "hw/boards.h"
> > > >> >>
> > > >> >> /* Default IOAPIC ID */
> > > >> >> #define ACPI_BUILD_IOAPIC_ID 0x0
> > > >> >>
> > > >> >> +void pc_madt_cpu_entry(int uid, const CPUArchIdList *apic_ids,
> > GArray *entry,
> > > >> >> + bool force_enabled);
> > > >> >> +
> > > >> >> 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_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/include/hw/acpi/cpu.h b/include/hw/acpi/cpu.h
> > > >> >> index 999caaf510..25b25bb594 100644
> > > >> >> --- a/include/hw/acpi/cpu.h
> > > >> >> +++ b/include/hw/acpi/cpu.h
> > > >> >> @@ -15,6 +15,7 @@
> > > >> >> #include "hw/qdev-core.h"
> > > >> >> #include "hw/acpi/acpi.h"
> > > >> >> #include "hw/acpi/aml-build.h"
> > > >> >> +#include "hw/boards.h"
> > > >> >> #include "hw/hotplug.h"
> > > >> >>
> > > >> >> typedef struct AcpiCpuStatus {
> > > >> >> @@ -55,8 +56,11 @@ typedef struct CPUHotplugFeatures {
> > > >> >> const char *smi_path;
> > > >> >> } CPUHotplugFeatures;
> > > >> >>
> > > >> >> +typedef void (*madt_cpu_fn)(int uid, const CPUArchIdList
> > *apic_ids,
> > > >> >> + GArray *entry, bool force_enabled);
> > > >> >> +
> > > >> >> void build_cpus_aml(Aml *table, MachineState *machine,
> > CPUHotplugFeatures opts,
> > > >> >> - hwaddr io_base,
> > > >> >> + hwaddr io_base, madt_cpu_fn madt_cpu,
> > > >> >> const char *res_root,
> > > >> >> const char *event_handler_method);
> > > >> >>
> > > >> >> diff --git a/include/hw/i386/pc.h b/include/hw/i386/pc.h
> > > >> >> index a0647165d1..a5cce88653 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/acpi/cpu.c b/hw/acpi/cpu.c
> > > >> >> index 19c154d78f..db15f9278d 100644
> > > >> >> --- a/hw/acpi/cpu.c
> > > >> >> +++ b/hw/acpi/cpu.c
> > > >> >> @@ -338,7 +338,7 @@ const VMStateDescription vmstate_cpu_hotplug =
> > {
> > > >> >> #define CPU_FW_EJECT_EVENT "CEJF"
> > > >> >>
> > > >> >> void build_cpus_aml(Aml *table, MachineState *machine,
> > CPUHotplugFeatures opts,
> > > >> >> - hwaddr io_base,
> > > >> >> + hwaddr io_base, madt_cpu_fn madt_cpu,
> > > >> >> const char *res_root,
> > > >> >> const char *event_handler_method)
> > > >> >> {
> > > >> >> @@ -353,8 +353,8 @@ 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);
> > > >> >> +
> > > >> >> + assert(madt_cpu);
> > > >> >>
> > > >> >> cpu_ctrl_dev = aml_device("%s", cphp_res_path);
> > > >> >> {
> > > >> >> @@ -664,9 +664,7 @@ 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 */
> > > >> >> + 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 0a81f1ad93..4d0d4fdeeb 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/southbridge/piix.h"
> > > >> >> #include "hw/irq.h"
> > > >> >> #include "hw/isa/apm.h"
> > > >> >> @@ -643,7 +642,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 127c4e2d50..0be3960a37 100644
> > > >> >> --- a/hw/i386/acpi-build.c
> > > >> >> +++ b/hw/i386/acpi-build.c
> > > >> >> @@ -1440,7 +1440,7 @@ build_dsdt(GArray *table_data, BIOSLinker
> > *linker,
> > > >> >> .fw_unplugs_cpu = pm->smi_on_cpu_unplug,
> > > >> >> };
> > > >> >> build_cpus_aml(dsdt, machine, opts, pm->cpu_hp_io_base,
> > > >> >> - "\\_SB.PCI0", "\\_GPE._E02");
> > > >> >> + pc_madt_cpu_entry, "\\_SB.PCI0",
> > "\\_GPE._E02");
> > > >> >> }
> > > >> >>
> > > >> >> if (pcms->memhp_io_base && nr_mem) {
> > > >> >> @@ -2424,8 +2424,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..aabf78092e 100644
> > > >> >> --- a/hw/i386/acpi-common.c
> > > >> >> +++ b/hw/i386/acpi-common.c
> > > >> >> @@ -94,14 +94,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 +110,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(i, apic_ids, table_data, false);
> > > >> >> + pc_madt_cpu_entry(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 fb09185cbd..d8a444d06c 100644
> > > >> >> --- a/hw/i386/acpi-microvm.c
> > > >> >> +++ b/hw/i386/acpi-microvm.c
> > > >> >> @@ -213,8 +213,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 8d541e2b54..0ab0a341be 100644
> > > >> >> --- a/hw/isa/lpc_ich9.c
> > > >> >> +++ b/hw/isa/lpc_ich9.c
> > > >> >> @@ -870,7 +870,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;
> > > >> >> }
> > > >> >>
> > > >> >
> > > >>
> > > >
> > >
> >
> >
^ permalink raw reply [flat|nested] 20+ messages in thread
* [PATCH v3 3/7] hw/acpi/acpi_dev_interface: Remove now unused #include "hw/boards.h"
2023-01-16 15:29 [PATCH v3 0/7] AML Housekeeping Bernhard Beschow
2023-01-16 15:29 ` [PATCH v3 1/7] hw/acpi/acpi_dev_interface: Remove unused parameter from AcpiDeviceIfClass::madt_cpu Bernhard Beschow
2023-01-16 15:29 ` [PATCH v3 2/7] hw/acpi/acpi_dev_interface: Resolve AcpiDeviceIfClass::madt_cpu Bernhard Beschow
@ 2023-01-16 15:29 ` Bernhard Beschow
2023-01-16 15:29 ` [PATCH v3 4/7] hw/acpi/piix4: No need to #include "hw/southbridge/piix.h" Bernhard Beschow
` (3 subsequent siblings)
6 siblings, 0 replies; 20+ messages in thread
From: Bernhard Beschow @ 2023-01-16 15:29 UTC (permalink / raw)
To: qemu-devel
Cc: qemu-trivial, Richard Henderson, Paolo Bonzini,
Philippe Mathieu-Daudé, Markus Armbruster, Eduardo Habkost,
Michael S. Tsirkin, Ani Sinha, Marcel Apfelbaum, Igor Mammedov,
Aurelien Jarno, 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 db15f9278d..bc77c00d66 100644
--- a/hw/acpi/cpu.c
+++ b/hw/acpi/cpu.c
@@ -1,6 +1,7 @@
#include "qemu/osdep.h"
#include "migration/vmstate.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 2932b3f3a5..45b0f2905d 100644
--- a/monitor/qmp-cmds.c
+++ b/monitor/qmp-cmds.c
@@ -44,6 +44,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.0
^ permalink raw reply related [flat|nested] 20+ messages in thread* [PATCH v3 4/7] hw/acpi/piix4: No need to #include "hw/southbridge/piix.h"
2023-01-16 15:29 [PATCH v3 0/7] AML Housekeeping Bernhard Beschow
` (2 preceding siblings ...)
2023-01-16 15:29 ` [PATCH v3 3/7] hw/acpi/acpi_dev_interface: Remove now unused #include "hw/boards.h" Bernhard Beschow
@ 2023-01-16 15:29 ` Bernhard Beschow
2023-01-16 15:29 ` [PATCH v3 5/7] hw/i386/acpi-build: Remove unused attributes Bernhard Beschow
` (2 subsequent siblings)
6 siblings, 0 replies; 20+ messages in thread
From: Bernhard Beschow @ 2023-01-16 15:29 UTC (permalink / raw)
To: qemu-devel
Cc: qemu-trivial, Richard Henderson, Paolo Bonzini,
Philippe Mathieu-Daudé, Markus Armbruster, Eduardo Habkost,
Michael S. Tsirkin, Ani Sinha, Marcel Apfelbaum, Igor Mammedov,
Aurelien Jarno, 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 4d0d4fdeeb..2e19a55526 100644
--- a/hw/acpi/piix4.c
+++ b/hw/acpi/piix4.c
@@ -20,7 +20,6 @@
*/
#include "qemu/osdep.h"
-#include "hw/southbridge/piix.h"
#include "hw/irq.h"
#include "hw/isa/apm.h"
#include "hw/i2c/pm_smbus.h"
--
2.39.0
^ permalink raw reply related [flat|nested] 20+ messages in thread* [PATCH v3 5/7] hw/i386/acpi-build: Remove unused attributes
2023-01-16 15:29 [PATCH v3 0/7] AML Housekeeping Bernhard Beschow
` (3 preceding siblings ...)
2023-01-16 15:29 ` [PATCH v3 4/7] hw/acpi/piix4: No need to #include "hw/southbridge/piix.h" Bernhard Beschow
@ 2023-01-16 15:29 ` Bernhard Beschow
2023-01-16 16:45 ` Igor Mammedov
2023-01-16 15:29 ` [PATCH v3 6/7] hw/isa/isa-bus: Turn isa_build_aml() into qbus_build_aml() Bernhard Beschow
2023-01-16 15:29 ` [PATCH v3 7/7] piix3, ich9: Reuse qbus_build_aml() Bernhard Beschow
6 siblings, 1 reply; 20+ messages in thread
From: Bernhard Beschow @ 2023-01-16 15:29 UTC (permalink / raw)
To: qemu-devel
Cc: qemu-trivial, Richard Henderson, Paolo Bonzini,
Philippe Mathieu-Daudé, Markus Armbruster, Eduardo Habkost,
Michael S. Tsirkin, Ani Sinha, Marcel Apfelbaum, Igor Mammedov,
Aurelien Jarno, 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>
---
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 0be3960a37..428328dc2d 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.0
^ permalink raw reply related [flat|nested] 20+ messages in thread* Re: [PATCH v3 5/7] hw/i386/acpi-build: Remove unused attributes
2023-01-16 15:29 ` [PATCH v3 5/7] hw/i386/acpi-build: Remove unused attributes Bernhard Beschow
@ 2023-01-16 16:45 ` Igor Mammedov
0 siblings, 0 replies; 20+ messages in thread
From: Igor Mammedov @ 2023-01-16 16:45 UTC (permalink / raw)
To: Bernhard Beschow
Cc: qemu-devel, qemu-trivial, Richard Henderson, Paolo Bonzini,
Philippe Mathieu-Daudé, Markus Armbruster, Eduardo Habkost,
Michael S. Tsirkin, Ani Sinha, Marcel Apfelbaum, Aurelien Jarno
On Mon, 16 Jan 2023 16:29:06 +0100
Bernhard Beschow <shentey@gmail.com> wrote:
> 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 0be3960a37..428328dc2d 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 {
^ permalink raw reply [flat|nested] 20+ messages in thread
* [PATCH v3 6/7] hw/isa/isa-bus: Turn isa_build_aml() into qbus_build_aml()
2023-01-16 15:29 [PATCH v3 0/7] AML Housekeeping Bernhard Beschow
` (4 preceding siblings ...)
2023-01-16 15:29 ` [PATCH v3 5/7] hw/i386/acpi-build: Remove unused attributes Bernhard Beschow
@ 2023-01-16 15:29 ` Bernhard Beschow
2023-01-18 11:34 ` Igor Mammedov
2023-01-16 15:29 ` [PATCH v3 7/7] piix3, ich9: Reuse qbus_build_aml() Bernhard Beschow
6 siblings, 1 reply; 20+ messages in thread
From: Bernhard Beschow @ 2023-01-16 15:29 UTC (permalink / raw)
To: qemu-devel
Cc: qemu-trivial, Richard Henderson, Paolo Bonzini,
Philippe Mathieu-Daudé, Markus Armbruster, Eduardo Habkost,
Michael S. Tsirkin, Ani Sinha, Marcel Apfelbaum, Igor Mammedov,
Aurelien Jarno, Bernhard Beschow
Frees isa-bus.c from implicit ACPI dependency.
Signed-off-by: Bernhard Beschow <shentey@gmail.com>
Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>
---
include/hw/acpi/acpi_aml_interface.h | 3 +++
include/hw/isa/isa.h | 1 -
hw/acpi/acpi_interface.c | 10 ++++++++++
hw/i386/acpi-microvm.c | 3 ++-
hw/isa/isa-bus.c | 10 ----------
5 files changed, 15 insertions(+), 12 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/i386/acpi-microvm.c b/hw/i386/acpi-microvm.c
index d8a444d06c..fec22d85c1 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);
--
2.39.0
^ permalink raw reply related [flat|nested] 20+ messages in thread* Re: [PATCH v3 6/7] hw/isa/isa-bus: Turn isa_build_aml() into qbus_build_aml()
2023-01-16 15:29 ` [PATCH v3 6/7] hw/isa/isa-bus: Turn isa_build_aml() into qbus_build_aml() Bernhard Beschow
@ 2023-01-18 11:34 ` Igor Mammedov
0 siblings, 0 replies; 20+ messages in thread
From: Igor Mammedov @ 2023-01-18 11:34 UTC (permalink / raw)
To: Bernhard Beschow
Cc: qemu-devel, qemu-trivial, Richard Henderson, Paolo Bonzini,
Philippe Mathieu-Daudé, Markus Armbruster, Eduardo Habkost,
Michael S. Tsirkin, Ani Sinha, Marcel Apfelbaum, Aurelien Jarno
On Mon, 16 Jan 2023 16:29:07 +0100
Bernhard Beschow <shentey@gmail.com> wrote:
> Frees isa-bus.c from implicit ACPI dependency.
>
> 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/i386/acpi-microvm.c | 3 ++-
> hw/isa/isa-bus.c | 10 ----------
> 5 files changed, 15 insertions(+), 12 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/i386/acpi-microvm.c b/hw/i386/acpi-microvm.c
> index d8a444d06c..fec22d85c1 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);
^ permalink raw reply [flat|nested] 20+ messages in thread
* [PATCH v3 7/7] piix3, ich9: Reuse qbus_build_aml()
2023-01-16 15:29 [PATCH v3 0/7] AML Housekeeping Bernhard Beschow
` (5 preceding siblings ...)
2023-01-16 15:29 ` [PATCH v3 6/7] hw/isa/isa-bus: Turn isa_build_aml() into qbus_build_aml() Bernhard Beschow
@ 2023-01-16 15:29 ` Bernhard Beschow
2023-01-18 11:39 ` Igor Mammedov
6 siblings, 1 reply; 20+ messages in thread
From: Bernhard Beschow @ 2023-01-16 15:29 UTC (permalink / raw)
To: qemu-devel
Cc: qemu-trivial, Richard Henderson, Paolo Bonzini,
Philippe Mathieu-Daudé, Markus Armbruster, Eduardo Habkost,
Michael S. Tsirkin, Ani Sinha, Marcel Apfelbaum, Igor Mammedov,
Aurelien Jarno, Bernhard Beschow
Signed-off-by: Bernhard Beschow <shentey@gmail.com>
Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>
---
hw/i2c/smbus_ich9.c | 5 +----
hw/isa/lpc_ich9.c | 5 +----
hw/isa/piix3.c | 5 +----
3 files changed, 3 insertions(+), 12 deletions(-)
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/isa/lpc_ich9.c b/hw/isa/lpc_ich9.c
index 0ab0a341be..d5d4b0f177 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.0
^ permalink raw reply related [flat|nested] 20+ messages in thread* Re: [PATCH v3 7/7] piix3, ich9: Reuse qbus_build_aml()
2023-01-16 15:29 ` [PATCH v3 7/7] piix3, ich9: Reuse qbus_build_aml() Bernhard Beschow
@ 2023-01-18 11:39 ` Igor Mammedov
0 siblings, 0 replies; 20+ messages in thread
From: Igor Mammedov @ 2023-01-18 11:39 UTC (permalink / raw)
To: Bernhard Beschow
Cc: qemu-devel, qemu-trivial, Richard Henderson, Paolo Bonzini,
Philippe Mathieu-Daudé, Markus Armbruster, Eduardo Habkost,
Michael S. Tsirkin, Ani Sinha, Marcel Apfelbaum, Aurelien Jarno
On Mon, 16 Jan 2023 16:29:08 +0100
Bernhard Beschow <shentey@gmail.com> wrote:
> Signed-off-by: Bernhard Beschow <shentey@gmail.com>
> Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>
Reviewed-by: Igor Mammedov <imammedo@redhat.com>
I'd squash it in previous patch, and rename that to something
'remove no longer needed isa_build_aml()
isa_build_aml() doesn't do anything except
calling call_dev_aml_func() on bus children
along with other places that do the same.
Move that into ... and cleanup those places
as well.
'
> ---
> hw/i2c/smbus_ich9.c | 5 +----
> hw/isa/lpc_ich9.c | 5 +----
> hw/isa/piix3.c | 5 +----
> 3 files changed, 3 insertions(+), 12 deletions(-)
>
> 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/isa/lpc_ich9.c b/hw/isa/lpc_ich9.c
> index 0ab0a341be..d5d4b0f177 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)
^ permalink raw reply [flat|nested] 20+ messages in thread