[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH v2 58/74] pc: acpi: piix4: move PCI0._PRT() into
From: |
Michael S. Tsirkin |
Subject: |
Re: [Qemu-devel] [PATCH v2 58/74] pc: acpi: piix4: move PCI0._PRT() into SSDT |
Date: |
Tue, 22 Dec 2015 16:52:58 +0200 |
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" <address@hidden> 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" <address@hidden> wrote:
> > >
> > > > On Thu, Dec 10, 2015 at 05:17:07PM +0100, Igor Mammedov wrote:
> > > > > Signed-off-by: Igor Mammedov <address@hidden>
> > > > > ---
> > > > > v2:
> > > > > - adapt build_prt() for using for PCI0._PRT(), reduces code
> > > > > duplication,
> > > > > Suggested-by: Marcel Apfelbaum <address@hidden>
> > > > >
> > > > > pc: acpi: piix4: adapt build_prt() for using for PCI0._PRT()
> > > > >
> > > > > Signed-off-by: Igor Mammedov <address@hidden>
> > > > > ---
> > > > > 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
> > > > >
- [Qemu-devel] [PATCH 58/74] pc: acpi: piix4: move PCI0._PRT() into SSDT, (continued)
- [Qemu-devel] [PATCH 58/74] pc: acpi: piix4: move PCI0._PRT() into SSDT, Igor Mammedov, 2015/12/09
- Re: [Qemu-devel] [PATCH 58/74] pc: acpi: piix4: move PCI0._PRT() into SSDT, Marcel Apfelbaum, 2015/12/10
- Re: [Qemu-devel] [PATCH v2 58/74] pc: acpi: piix4: move PCI0._PRT() into SSDT, Michael S. Tsirkin, 2015/12/19
- Re: [Qemu-devel] [PATCH v2 58/74] pc: acpi: piix4: move PCI0._PRT() into SSDT, Igor Mammedov, 2015/12/21
- Re: [Qemu-devel] [PATCH v2 58/74] pc: acpi: piix4: move PCI0._PRT() into SSDT, Michael S. Tsirkin, 2015/12/22
- Re: [Qemu-devel] [PATCH v2 58/74] pc: acpi: piix4: move PCI0._PRT() into SSDT, Igor Mammedov, 2015/12/22
- Re: [Qemu-devel] [PATCH v2 58/74] pc: acpi: piix4: move PCI0._PRT() into SSDT,
Michael S. Tsirkin <=
[Qemu-devel] [PATCH 69/74] pc: acpi: q35: move _PIC() method into SSDT, Igor Mammedov, 2015/12/09
[Qemu-devel] [PATCH 73/74] pc: acpi: switch to AML API composed DSDT, Igor Mammedov, 2015/12/09
[Qemu-devel] [PATCH 46/74] pc: acpi: move DBUG() from DSDT to SSDT, Igor Mammedov, 2015/12/09
[Qemu-devel] [PATCH 56/74] pc: acpi: piix4: move IQCR() into SSDT, Igor Mammedov, 2015/12/09
[Qemu-devel] [PATCH 59/74] pc: acpi: piix4: move remaining PCI hotplug bits into SSDT, Igor Mammedov, 2015/12/09
[Qemu-devel] [PATCH 64/74] pc: acpi: q35: move IQST() into SSDT, Igor Mammedov, 2015/12/09
[Qemu-devel] [PATCH 62/74] pc: acpi: q35: move link devices to SSDT, Igor Mammedov, 2015/12/09
[Qemu-devel] [PATCH 68/74] pc: acpi: q35: move PRTP routing table into SSDT, Igor Mammedov, 2015/12/09
[Qemu-devel] [PATCH 45/74] pc: acpi: move HPET from DSDT to SSDT, Igor Mammedov, 2015/12/09