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

[Wei Yang <richard.weiyang@gmail.com>]

On Wed, Jun 19, 2019 at 11:04:40AM +0200, Igor Mammedov wrote:
>On Wed, 19 Jun 2019 14:20:50 +0800
>Wei Yang <richardw.yang@linux.intel.com> 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 <richardw.yang@linux.intel.com> 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.

I didn't get some idea about your approach. Would you mind sharing more light?

-- 
Wei Yang
Help you, Help me