From: Wei Yang <richard.weiyang@gmail.com>
To: Igor Mammedov <imammedo@redhat.com>
Cc: yang.zhong@intel.com, ehabkost@redhat.com, mst@redhat.com,
qemu-devel@nongnu.org, Wei Yang <richard.weiyang@gmail.com>,
Wei Yang <richardw.yang@linux.intel.com>,
pbonzini@redhat.com, rth@twiddle.net
Subject: Re: [Qemu-devel] [RFC PATCH 0/9] hw/acpi: make build_madt arch agnostic
Date: Fri, 21 Jun 2019 21:33:35 +0000 [thread overview]
Message-ID: <20190621213335.ijukkx26vpkyqvbq@master> (raw)
In-Reply-To: <20190621101131.5cf44250@redhat.com>
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
prev parent reply other threads:[~2019-06-21 21:35 UTC|newest]
Thread overview: 19+ messages / expand[flat|nested] mbox.gz Atom feed top
2019-05-13 6:19 [Qemu-devel] [RFC PATCH 0/9] hw/acpi: make build_madt arch agnostic Wei Yang
2019-05-13 6:19 ` [Qemu-devel] [RFC PATCH 1/9] hw/acpi: expand pc_madt_cpu_entry in place Wei Yang
2019-05-13 6:19 ` [Qemu-devel] [RFC PATCH 2/9] hw/acpi: implement madt_sub[ACPI_APIC_PROCESSOR] Wei Yang
2019-05-13 6:19 ` [Qemu-devel] [RFC PATCH 3/9] hw/acpi: implement madt_sub[ACPI_APIC_LOCAL_X2APIC] Wei Yang
2019-05-13 6:19 ` [Qemu-devel] [RFC PATCH 4/9] hw/acpi: implement madt_sub[ACPI_APIC_IO] Wei Yang
2019-05-13 6:19 ` [Qemu-devel] [RFC PATCH 5/9] hw/acpi: implement madt_sub[ACPI_APIC_XRUPT_OVERRIDE] Wei Yang
2019-05-13 6:19 ` [Qemu-devel] [RFC PATCH 6/9] hw/acpi: implement madt_sub[ACPI_APIC_LOCAL_X2APIC_NMI] Wei Yang
2019-05-13 6:19 ` [Qemu-devel] [RFC PATCH 7/9] hw/acpi: implement madt_sub[ACPI_APIC_LOCAL_NMI] Wei Yang
2019-05-13 6:19 ` [Qemu-devel] [RFC PATCH 8/9] hw/acpi: factor build_madt with madt_input Wei Yang
2019-05-13 6:19 ` [Qemu-devel] [RFC PATCH 9/9] hw/acpi: implement madt_main to manipulate main madt table Wei Yang
2019-06-03 6:22 ` [Qemu-devel] [RFC PATCH 0/9] hw/acpi: make build_madt arch agnostic Wei Yang
2019-06-18 15:59 ` Igor Mammedov
2019-06-19 6:20 ` Wei Yang
2019-06-19 9:04 ` Igor Mammedov
2019-06-20 14:18 ` Wei Yang
2019-06-20 15:04 ` Igor Mammedov
2019-06-21 0:56 ` Wei Yang
2019-06-21 8:11 ` Igor Mammedov
2019-06-21 21:33 ` Wei Yang [this message]
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20190621213335.ijukkx26vpkyqvbq@master \
--to=richard.weiyang@gmail.com \
--cc=ehabkost@redhat.com \
--cc=imammedo@redhat.com \
--cc=mst@redhat.com \
--cc=pbonzini@redhat.com \
--cc=qemu-devel@nongnu.org \
--cc=richardw.yang@linux.intel.com \
--cc=rth@twiddle.net \
--cc=yang.zhong@intel.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).