On Tue, Jan 17, 2023 at 1:30 AM Bernhard Beschow <shen...@gmail.com> wrote:
>
>
> Am 16. Januar 2023 16:29:30 UTC schrieb Igor Mammedov <imamm...@redhat.com
> >:
> >On Mon, 16 Jan 2023 16:29:03 +0100
> >Bernhard Beschow <shen...@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 <shen...@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;
> >> }
> >>
> >
>
