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

[Igor Mammedov]

On Wed, 19 Jun 2019 14:20:50 +0800
Wei Yang <address@hidden> wrote:

> On Tue, Jun 18, 2019 at 05:59:56PM +0200, Igor Mammedov wrote:
> >
> >On Mon, 13 May 2019 14:19:04 +0800
> >Wei Yang <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[])
> >  
> 
> Per APIC spec 5.2.12, MADT contains a *main* table and several *sub* tables
> (Interrupt Controllere), so the idea is give a callback hook in
> AcpiDeviceIfClass for each table, including *main* and *sub* table.
> 
> Current AcpiDeviceIfClass has one callback pc_madt_cpu_entry for some *sub*
> tables, after replacing the AcpiDeviceIfClass will look like this:
> 
> typedef struct AcpiDeviceIfClass {
>     /* <private> */
>     InterfaceClass parent_class;
> 
>     /* <public> */
>     void (*ospm_status)(AcpiDeviceIf *adev, ACPIOSTInfoList ***list);
>     void (*send_event)(AcpiDeviceIf *adev, AcpiEventStatusBits ev);
> -   void (*madt_cpu)(AcpiDeviceIf *adev, int uid,
> -                    const CPUArchIdList *apic_ids, GArray *entry);
> +   madt_operation madt_main;
> +   madt_operation *madt_sub;
> } AcpiDeviceIfClass;
> 
> By doing so, each arch could have its own implementation for MADT.
> 
> After this refactoring, build_madt could be simplified to:
> 
> build_madt(GArray *table_data, BIOSLinker *linker, PCMachineState *pcms,
>            struct madt_input *input)
> {
>     ...
> 
>     if (adevc->madt_main) {
>         adevc->madt_main(table_data, madt);
>     }
> 
>     for (i = 0; ; i++) {
>         sub_id = input[i].sub_id;
>         if (sub_id == ACPI_APIC_RESERVED) {
>             break;
>         }
>         opaque = input[i].opaque;
>         adevc->madt_sub[sub_id](table_data, opaque);
>     }
> 
>     ...
> }
> 
> input is a list of data necessary to build *sub* table. Its details is also
> arch dependent.
I've got general idea reading patches in this series.
As I've mentioned before it's hard to generalize MADT since it
mostly contains entries unique for target/board.
Goal here isn't generalizing at any cost, but rather find out
if there is enough common code to justify generalization
and if it allows us to reduce code duplication and simplify.

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