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

[Igor Mammedov <imammedo@redhat.com>]

On Fri, 21 Jun 2019 08:56:44 +0800
Wei Yang <richardw.yang@linux.intel.com> wrote:

> On Thu, Jun 20, 2019 at 05:04:29PM +0200, Igor Mammedov wrote:
> >On Thu, 20 Jun 2019 14:18:42 +0000
> >Wei Yang <richard.weiyang@gmail.com> wrote:
> >  
> >> 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?  
> >With current approach, 'each board' has its own MADT build routine.
> >Considering that there is very little to share between different
> >implementations it might be ok.
> >
> >This series just add extra data structure for board to populate
> >and a bunch of callbacks for every record type. Essentially all
> >the code we have now is still there. It was just moved elsewhere
> >and made available via callbacks.  
> 
> Yes, you are right.
> 
> >This series touches only pc/q35 machines and it's not apparent
> >to me why it's any better than what we have now.  
> 
> This is the demo for i386. In case you think this approach is reasonable, it
> could be applied to arm. And then for new board, we can apply the same
> approach.
well, it's not obvious from i386 demo, how it's any better than what
we have now. It lacks arm/virt patches so we could see if it would make
anything better or not.

If I were to talk about i386 demo alone, then I'd say it just makes
code more complex and I'd leave existing MADT code as it.