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.y...@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.
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
