Re: [Qemu-devel] [PATCH v2 58/74] pc: acpi: piix4: move PCI0._PRT() into SSDT

["Michael S. Tsirkin" <mst@redhat.com>]

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" <mst@redhat.com> wrote:
> 
> > On Thu, Dec 10, 2015 at 05:17:07PM +0100, Igor Mammedov wrote:
> > > Signed-off-by: Igor Mammedov <imammedo@redhat.com>
> > > ---
> > > v2:
> > >  - adapt build_prt() for using for PCI0._PRT(), reduces code duplication,
> > >    Suggested-by: Marcel Apfelbaum <marcel.apfelbaum@gmail.com>
> > > 
> > > pc: acpi: piix4: adapt build_prt() for using for PCI0._PRT()
> > > 
> > > Signed-off-by: Igor Mammedov <imammedo@redhat.com>
> > > ---
> > >  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.

> > 
> > > +            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?).


> > 
> > > -            }
> > > -        }
> > > -
> > > -
> > > -        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
> > >