From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:50900) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1aBOJH-0004Rh-7x for qemu-devel@nongnu.org; Tue, 22 Dec 2015 09:53:08 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1aBOJC-0000zC-2u for qemu-devel@nongnu.org; Tue, 22 Dec 2015 09:53:07 -0500 Received: from mx1.redhat.com ([209.132.183.28]:49710) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1aBOJB-0000z7-PP for qemu-devel@nongnu.org; Tue, 22 Dec 2015 09:53:02 -0500 Date: Tue, 22 Dec 2015 16:52:58 +0200 From: "Michael S. Tsirkin" Message-ID: <20151222164737-mutt-send-email-mst@redhat.com> References: <566970CB.2070804@gmail.com> <1449764227-42899-1-git-send-email-imammedo@redhat.com> <20151219192322.GA25389@redhat.com> <20151221135516.1777e3fb@nial.brq.redhat.com> <20151222111944-mutt-send-email-mst@redhat.com> <20151222151201.7d15082c@nial.brq.redhat.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20151222151201.7d15082c@nial.brq.redhat.com> Subject: Re: [Qemu-devel] [PATCH v2 58/74] pc: acpi: piix4: move PCI0._PRT() into SSDT List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Igor Mammedov Cc: qemu-devel@nongnu.org, marcel.apfelbaum@gmail.com On Tue, Dec 22, 2015 at 03:12:01PM +0100, Igor Mammedov wrote: > On Tue, 22 Dec 2015 11:34:46 +0200 > "Michael S. Tsirkin" wrote: > > > On Mon, Dec 21, 2015 at 01:55:16PM +0100, Igor Mammedov wrote: > > > On Sat, 19 Dec 2015 21:23:22 +0200 > > > "Michael S. Tsirkin" wrote: > > > > > > > On Thu, Dec 10, 2015 at 05:17:07PM +0100, Igor Mammedov wrote: > > > > > Signed-off-by: Igor Mammedov > > > > > --- > > > > > v2: > > > > > - adapt build_prt() for using for PCI0._PRT(), reduces code duplication, > > > > > Suggested-by: Marcel Apfelbaum > > > > > > > > > > pc: acpi: piix4: adapt build_prt() for using for PCI0._PRT() > > > > > > > > > > Signed-off-by: Igor Mammedov > > > > > --- > > > > > hw/i386/acpi-build.c | 41 +++++++++++++++++++++++++++-------- > > > > > hw/i386/acpi-dsdt.dsl | 60 --------------------------------------------------- > > > > > 2 files changed, 32 insertions(+), 69 deletions(-) > > > > > > > > > > diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c > > > > > index cf98037..1b065df 100644 > > > > > --- a/hw/i386/acpi-build.c > > > > > +++ b/hw/i386/acpi-build.c > > > > > @@ -620,6 +620,17 @@ static void build_append_pci_bus_devices(Aml *parent_scope, PCIBus *bus, > > > > > qobject_decref(bsel); > > > > > } > > > > > > > > > > +static Aml *build_prt_entry(const char *link_name) > > > > Pls document what this does. > > > > > > > +{ > > > > > + Aml *a_zero = aml_int(0); > > > > > + Aml *pkg = aml_package(4); > > > > > + aml_append(pkg, a_zero); > > > > > + aml_append(pkg, a_zero); > > > > > + aml_append(pkg, aml_name("%s", link_name)); > > > > > + aml_append(pkg, a_zero); > > > > > + return pkg; > > > > > +} > > > > > + > > > > > /* > > > > > * initialize_route - Initialize the interrupt routing rule > > > > > * through a specific LINK: > > > > > @@ -630,12 +641,8 @@ static Aml *initialize_route(Aml *route, const char *link_name, > > > > > Aml *lnk_idx, int idx) > > > > > { > > > > > Aml *if_ctx = aml_if(aml_equal(lnk_idx, aml_int(idx))); > > > > > - Aml *pkg = aml_package(4); > > > > > + Aml *pkg = build_prt_entry(link_name); > > > > > > > > > > - aml_append(pkg, aml_int(0)); > > > > > - aml_append(pkg, aml_int(0)); > > > > > - aml_append(pkg, aml_name("%s", link_name)); > > > > > - aml_append(pkg, aml_int(0)); > > > > > aml_append(if_ctx, aml_store(pkg, route)); > > > > > > > > > > return if_ctx; > > > > > @@ -651,9 +658,9 @@ static Aml *initialize_route(Aml *route, const char *link_name, > > > > > * The hash function is (slot + pin) & 3 -> "LNK[D|A|B|C]". > > > > > * > > > > > */ > > > > > -static Aml *build_prt(void) > > > > > +static Aml *build_prt(bool is_pci0_prt) > > > > > { > > > > > - Aml *method, *while_ctx, *pin, *res; > > > > > + Aml *method, *while_ctx, *pin, *res, *if_ctx, *if_ctx2, *else_ctx2; > > > > > > > > Move if_ctx2/if_ctx/else_ctx2 where they are used? > > > > Also, can't we come up with better name than if_ctx2? > > > > How about if_lnk_1 and if_pin_4? > > > sure > > > > > > > > > > > > > > > > > method = aml_method("_PRT", 0, AML_NOTSERIALIZED); > > > > > res = aml_local(0); > > > > > @@ -678,7 +685,19 @@ static Aml *build_prt(void) > > > > > > > > > > /* route[2] = "LNK[D|A|B|C]", selection based on pin % 3 */ > > > > > aml_append(while_ctx, initialize_route(route, "LNKD", lnk_idx, 0)); > > > > > - aml_append(while_ctx, initialize_route(route, "LNKA", lnk_idx, 1)); > > > > > + if (is_pci0_prt) { > > > > > + if_ctx = aml_if(aml_equal(lnk_idx, aml_int(1))); > > > > > + /* device 1 is the power-management device, needs SCI */ > > > > > > > > Doesn't this context belong above the previous line? > > > that how it was in ASL, but yep it belongs to a whole if block > > > so I'll move it up > > > > > > > > > > > > + if_ctx2 = aml_if(aml_equal(pin, aml_int(4))); > > > > > + aml_append(if_ctx2, aml_store(build_prt_entry("LNKS"), route)); > > > > > + aml_append(if_ctx, if_ctx2); > > > > > + else_ctx2 = aml_else(); > > > > > + aml_append(else_ctx2, aml_store(build_prt_entry("LNKA"), route)); > > > > > + aml_append(if_ctx, else_ctx2); > > > > > > > > This looks weird. Why isn't else_ctx2 added to if_ctx2? > > > it should be if_ctx2, I'll fix it. > > > > Hmm I'm not sure actually. > > The API for if/else is confusing, but I'm not sure what's > > a better one. > the weird thing is that this creates exactly the same AML > as in original DSDT, > anyway I'll try to make it more clear > on respin Maybe by using {} scopes and declaring variables there. Also name them more explicitly: if_device_1 if_pin_4. > > > > > > > > > > > + aml_append(while_ctx, if_ctx); > > > > > + } else { > > > > > + aml_append(while_ctx, initialize_route(route, "LNKA", lnk_idx, 1)); > > > > > + } > > > > > aml_append(while_ctx, initialize_route(route, "LNKB", lnk_idx, 2)); > > > > > aml_append(while_ctx, initialize_route(route, "LNKC", lnk_idx, 3)); > > > > > > > > > > @@ -1348,6 +1367,10 @@ static void build_piix4_pci0_int(Aml *table) > > > > > Aml *method; > > > > > uint32_t irqs; > > > > > Aml *sb_scope = aml_scope("_SB"); > > > > > + Aml *pci0_scope = aml_scope("PCI0"); > > > > > + > > > > > + aml_append(pci0_scope, build_prt(true)); > > > > > + aml_append(sb_scope, pci0_scope); > > > > > > > > > > field = aml_field("PCI0.ISA.P40C", AML_BYTE_ACC, AML_NOLOCK, AML_PRESERVE); > > > > > aml_append(field, aml_named_field("PRQ0", 8)); > > > > > @@ -1569,7 +1592,7 @@ build_ssdt(GArray *table_data, GArray *linker, > > > > > aml_append(dev, aml_name_decl("_PXM", aml_int(numa_node))); > > > > > } > > > > > > > > > > - aml_append(dev, build_prt()); > > > > > + aml_append(dev, build_prt(false)); > > > > > crs = build_crs(PCI_HOST_BRIDGE(BUS(bus)->parent), > > > > > io_ranges, mem_ranges); > > > > > aml_append(dev, aml_name_decl("_CRS", crs)); > > > > > diff --git a/hw/i386/acpi-dsdt.dsl b/hw/i386/acpi-dsdt.dsl > > > > > index bc6bd45..5d741dd 100644 > > > > > --- a/hw/i386/acpi-dsdt.dsl > > > > > +++ b/hw/i386/acpi-dsdt.dsl > > > > > @@ -78,64 +78,4 @@ DefinitionBlock ( > > > > > /* Hotplug notification method supplied by SSDT */ > > > > > External(\_SB.PCI0.PCNT, MethodObj) > > > > > } > > > > > - > > > > > - > > > > > -/**************************************************************** > > > > > - * PCI IRQs > > > > > - ****************************************************************/ > > > > > - > > > > > - Scope(\_SB) { > > > > > - Scope(PCI0) { > > > > > - Method (_PRT, 0) { > > > > > - Store(Package(128) {}, Local0) > > > > > - Store(Zero, Local1) > > > > > - While(LLess(Local1, 128)) { > > > > > - // slot = pin >> 2 > > > > > - Store(ShiftRight(Local1, 2), Local2) > > > > > - > > > > > - // lnk = (slot + pin) & 3 > > > > > - Store(And(Add(Local1, Local2), 3), Local3) > > > > > - If (LEqual(Local3, 0)) { > > > > > - Store(Package(4) { Zero, Zero, LNKD, Zero }, Local4) > > > > > - } > > > > > - If (LEqual(Local3, 1)) { > > > > > - // device 1 is the power-management device, needs SCI > > > > > - If (LEqual(Local1, 4)) { > > > > > - Store(Package(4) { Zero, Zero, LNKS, Zero }, Local4) > > > > > - } Else { > > > > > - Store(Package(4) { Zero, Zero, LNKA, Zero }, Local4) > > > > > - } > > > > > - } > > > > > - If (LEqual(Local3, 2)) { > > > > > - Store(Package(4) { Zero, Zero, LNKB, Zero }, Local4) > > > > > - } > > > > > - If (LEqual(Local3, 3)) { > > > > > - Store(Package(4) { Zero, Zero, LNKC, Zero }, Local4) > > > > > - } > > > > > - > > > > > - // Complete the interrupt routing entry: > > > > > - // Package(4) { 0x[slot]FFFF, [pin], [link], 0) } > > > > > - > > > > > - Store(Or(ShiftLeft(Local2, 16), 0xFFFF), Index(Local4, 0)) > > > > > - Store(And(Local1, 3), Index(Local4, 1)) > > > > > - Store(Local4, Index(Local0, Local1)) > > > > > - > > > > > - Increment(Local1) > > > > > - } > > > > > - > > > > > - Return(Local0) > > > > > > > > Interesting. And where's this code? Probably in previous or follow-up patches ... > > > it was and still is in original build_prt(), > > > I'm just modifying it to generate missing parts of DSDT AML > > > equivalent which it's replacing. > > > > OK so commit log should explain that PRT for expander > > buses was already generated in C, the only difference for > > root bus is LINKA. > > > > And add comment explaining that for pci0, > > device 1 is connected to SCI (LNKS) > > (maybe rename flag to device_1_is_pm?). > sure > > > > > > > > > > > > > > - } > > > > > - } > > > > > - > > > > > - > > > > > - External(PRQ0, FieldUnitObj) > > > > > - External(PRQ1, FieldUnitObj) > > > > > - External(PRQ2, FieldUnitObj) > > > > > - External(PRQ3, FieldUnitObj) > > > > > - External(LNKA, DeviceObj) > > > > > - External(LNKB, DeviceObj) > > > > > - External(LNKC, DeviceObj) > > > > > - External(LNKD, DeviceObj) > > > > > - External(LNKS, DeviceObj) > > > > > - } > > > > > } > > > > > -- > > > > > 1.8.3.1 > > > > >