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

[Igor Mammedov]

On Mon, 13 May 2019 14:19:04 +0800
Wei Yang <address@hidden> wrote:

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

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

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

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