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

[Igor Mammedov]

On Thu, 20 Jun 2019 14:18:42 +0000
Wei Yang <address@hidden> 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 <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.  
> 
> 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.
This series touches only pc/q35 machines and it's not apparent
to me why it's any better than what we have now.