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

[Wei Yang]

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.

For following new arch, what it need to do is prepare the input array and
implement necessary *main*/*sub* table callbacks.

-- 
Wei Yang
Help you, Help me