hw/acpi/cpu.c | 14 +- hw/acpi/piix4.c | 3 +- hw/i386/acpi-build.c | 265 +++++++++++++++++---------- hw/isa/lpc_ich9.c | 3 +- include/hw/acpi/acpi_dev_interface.h | 12 +- include/hw/i386/pc.h | 2 + 6 files changed, 194 insertions(+), 105 deletions(-)
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.
Wei Yang (9):
hw/acpi: expand pc_madt_cpu_entry in place
hw/acpi: implement madt_sub[ACPI_APIC_PROCESSOR]
hw/acpi: implement madt_sub[ACPI_APIC_LOCAL_X2APIC]
hw/acpi: implement madt_sub[ACPI_APIC_IO]
hw/acpi: implement madt_sub[ACPI_APIC_XRUPT_OVERRIDE]
hw/acpi: implement madt_sub[ACPI_APIC_LOCAL_X2APIC_NMI]
hw/acpi: implement madt_sub[ACPI_APIC_LOCAL_NMI]
hw/acpi: factor build_madt with madt_input
hw/acpi: implement madt_main to manipulate main madt table
hw/acpi/cpu.c | 14 +-
hw/acpi/piix4.c | 3 +-
hw/i386/acpi-build.c | 265 +++++++++++++++++----------
hw/isa/lpc_ich9.c | 3 +-
include/hw/acpi/acpi_dev_interface.h | 12 +-
include/hw/i386/pc.h | 2 +
6 files changed, 194 insertions(+), 105 deletions(-)
--
2.19.1
Igor, Do you have some spare time to take a look the general idea? On Mon, May 13, 2019 at 02:19:04PM +0800, Wei Yang 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. > >Wei Yang (9): > hw/acpi: expand pc_madt_cpu_entry in place > hw/acpi: implement madt_sub[ACPI_APIC_PROCESSOR] > hw/acpi: implement madt_sub[ACPI_APIC_LOCAL_X2APIC] > hw/acpi: implement madt_sub[ACPI_APIC_IO] > hw/acpi: implement madt_sub[ACPI_APIC_XRUPT_OVERRIDE] > hw/acpi: implement madt_sub[ACPI_APIC_LOCAL_X2APIC_NMI] > hw/acpi: implement madt_sub[ACPI_APIC_LOCAL_NMI] > hw/acpi: factor build_madt with madt_input > hw/acpi: implement madt_main to manipulate main madt table > > hw/acpi/cpu.c | 14 +- > hw/acpi/piix4.c | 3 +- > hw/i386/acpi-build.c | 265 +++++++++++++++++---------- > hw/isa/lpc_ich9.c | 3 +- > include/hw/acpi/acpi_dev_interface.h | 12 +- > include/hw/i386/pc.h | 2 + > 6 files changed, 194 insertions(+), 105 deletions(-) > >-- >2.19.1 -- Wei Yang Help you, Help me
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[]) > > Wei Yang (9): > hw/acpi: expand pc_madt_cpu_entry in place > hw/acpi: implement madt_sub[ACPI_APIC_PROCESSOR] > hw/acpi: implement madt_sub[ACPI_APIC_LOCAL_X2APIC] > hw/acpi: implement madt_sub[ACPI_APIC_IO] > hw/acpi: implement madt_sub[ACPI_APIC_XRUPT_OVERRIDE] > hw/acpi: implement madt_sub[ACPI_APIC_LOCAL_X2APIC_NMI] > hw/acpi: implement madt_sub[ACPI_APIC_LOCAL_NMI] > hw/acpi: factor build_madt with madt_input > hw/acpi: implement madt_main to manipulate main madt table > > hw/acpi/cpu.c | 14 +- > hw/acpi/piix4.c | 3 +- > hw/i386/acpi-build.c | 265 +++++++++++++++++---------- > hw/isa/lpc_ich9.c | 3 +- > include/hw/acpi/acpi_dev_interface.h | 12 +- > include/hw/i386/pc.h | 2 + > 6 files changed, 194 insertions(+), 105 deletions(-) >
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.
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
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.
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
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.
This series touches only pc/q35 machines and it's not apparent
to me why it's any better than what we have now.
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.
--
Wei Yang
Help you, Help me
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.
On Fri, Jun 21, 2019 at 10:11:31AM +0200, Igor Mammedov wrote:
>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.
>
ok, let me add arm/vrit part.
>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.
--
Wei Yang
Help you, Help me
© 2016 - 2024 Red Hat, Inc.