Re: [PATCH v3 4/5] acpi/nvdimm: Implement ACPI NVDIMM Label Methods

[Igor Mammedov <imammedo@redhat.com>]

On Fri, 09 Sep 2022 22:02:34 +0800
Robert Hoo <robert.hu@linux.intel.com> wrote:

> On Fri, 2022-09-09 at 15:39 +0200, Igor Mammedov wrote:
> > On Thu,  1 Sep 2022 11:27:20 +0800
> > Robert Hoo <robert.hu@linux.intel.com> wrote:
> >   
> > > Recent ACPI spec [1] has defined NVDIMM Label Methods _LS{I,R,W},
> > > which
> > > deprecates corresponding _DSM Functions defined by PMEM _DSM
> > > Interface spec
> > > [2].
> > > 
> > > Since the semantics of the new Label Methods are same as old _DSM
> > > methods, the implementations here simply wrapper old ones.
> > > 
> > > ASL form diff can be found in next patch of updating golden master
> > > binaries.
> > > 
> > > [1] ACPI Spec v6.4, 6.5.10 NVDIMM Label Methods
> > > https://uefi.org/sites/default/files/resources/ACPI_Spec_6_4_Jan22.pdf
> > > [2] Intel PMEM _DSM Interface Spec v2.0, 3.10 Deprecated Functions
> > > https://pmem.io/documents/IntelOptanePMem_DSM_Interface-V2.0.pdf
> > > 
> > > Signed-off-by: Robert Hoo <robert.hu@linux.intel.com>
> > > Reviewed-by: Jingqi Liu <jingqi.liu@intel.com>  
> > 
> > looks more or less fine except of excessive use of named variables
> > which creates global scope variables.
> > 
> > I'd suggest to store temporary buffers/packages in LocalX variales,
> > you should be able to do that for everything modulo
> > aml_create_dword_field().
> > 
> > see an example below
> >   
> > > ---
> > >  hw/acpi/nvdimm.c | 91
> > > ++++++++++++++++++++++++++++++++++++++++++++++++
> > >  1 file changed, 91 insertions(+)
> > > 
> > > diff --git a/hw/acpi/nvdimm.c b/hw/acpi/nvdimm.c
> > > index afff911c1e..516acfe53b 100644
> > > --- a/hw/acpi/nvdimm.c
> > > +++ b/hw/acpi/nvdimm.c
> > > @@ -1243,6 +1243,7 @@ static void nvdimm_build_fit(Aml *dev)
> > >  static void nvdimm_build_nvdimm_devices(Aml *root_dev, uint32_t
> > > ram_slots)
> > >  {
> > >      uint32_t slot;
> > > +    Aml *method, *pkg, *field, *com_call;
> > >  
> > >      for (slot = 0; slot < ram_slots; slot++) {
> > >          uint32_t handle = nvdimm_slot_to_handle(slot);
> > > @@ -1260,6 +1261,96 @@ static void nvdimm_build_nvdimm_devices(Aml
> > > *root_dev, uint32_t ram_slots)
> > >           */
> > >          aml_append(nvdimm_dev, aml_name_decl("_ADR",
> > > aml_int(handle)));
> > >  
> > > +        /*
> > > +         * ACPI v6.4: Section 6.5.10 NVDIMM Label Methods
> > > +         */
> > > +        /* _LSI */
> > > +        method = aml_method("_LSI", 0, AML_SERIALIZED);
> > > +        com_call = aml_call5(NVDIMM_COMMON_DSM,
> > > +                            aml_touuid(NVDIMM_DEVICE_DSM_UUID),
> > > +                            aml_int(1), aml_int(4), aml_int(0),
> > > +                            aml_int(handle));
> > > +        aml_append(method, aml_store(com_call, aml_local(0)));
> > > +
> > > +        aml_append(method, aml_create_dword_field(aml_local(0),
> > > +                                                  aml_int(0),
> > > "STTS"));
> > > +        aml_append(method, aml_create_dword_field(aml_local(0),
> > > aml_int(4),
> > > +                                                  "SLSA"));
> > > +        aml_append(method, aml_create_dword_field(aml_local(0),
> > > aml_int(8),
> > > +                                                  "MAXT"));
> > > +
> > > +        pkg = aml_package(3);
> > > +        aml_append(pkg, aml_name("STTS"));
> > > +        aml_append(pkg, aml_name("SLSA"));
> > > +        aml_append(pkg, aml_name("MAXT"));
> > > +        aml_append(method, aml_name_decl("RET", pkg));  
> > 
> > ex: put it in local instead of named variable and return that
> > the same applies to other named temporary named variables.  
> 
> Could you help provide the example in form of ASL?

see hw/i386/acpi-build.c: build_prt() or aml_pci_device_dsm()

> Thanks.
> >   
> > > +        aml_append(method, aml_return(aml_name("RET")));
> > > +
> > > +        aml_append(nvdimm_dev, method);
> > > +
> > > +        /* _LSR */
> > > +        method = aml_method("_LSR", 2, AML_SERIALIZED);
> > > +        aml_append(method, aml_name_decl("INPT", aml_buffer(8,
> > > NULL)));
> > > +
> > > +        aml_append(method,
> > > aml_create_dword_field(aml_name("INPT"),
> > > +                                                  aml_int(0),
> > > "OFST"));
> > > +        aml_append(method,
> > > aml_create_dword_field(aml_name("INPT"),
> > > +                                                  aml_int(4),
> > > "LEN"));
> > > +        aml_append(method, aml_store(aml_arg(0),
> > > aml_name("OFST")));
> > > +        aml_append(method, aml_store(aml_arg(1),
> > > aml_name("LEN")));
> > > +
> > > +        pkg = aml_package(1);
> > > +        aml_append(pkg, aml_name("INPT"));
> > > +        aml_append(method, aml_name_decl("PKG1", pkg));
> > > +
> > > +        com_call = aml_call5(NVDIMM_COMMON_DSM,
> > > +                            aml_touuid(NVDIMM_DEVICE_DSM_UUID),
> > > +                            aml_int(1), aml_int(5),
> > > aml_name("PKG1"),
> > > +                            aml_int(handle));
> > > +        aml_append(method, aml_store(com_call, aml_local(3)));
> > > +        field = aml_create_dword_field(aml_local(3), aml_int(0),
> > > "STTS");
> > > +        aml_append(method, field);
> > > +        field = aml_create_field(aml_local(3), aml_int(32),
> > > +                                 aml_shiftleft(aml_name("LEN"),
> > > aml_int(3)),
> > > +                                 "LDAT");
> > > +        aml_append(method, field);
> > > +        aml_append(method, aml_name_decl("LSA", aml_buffer(0,
> > > NULL)));
> > > +        aml_append(method, aml_to_buffer(aml_name("LDAT"),
> > > aml_name("LSA")));
> > > +        pkg = aml_package(2);
> > > +        aml_append(pkg, aml_name("STTS"));
> > > +        aml_append(pkg, aml_name("LSA"));
> > > +        aml_append(method, aml_name_decl("RET", pkg));
> > > +        aml_append(method, aml_return(aml_name("RET")));
> > > +        aml_append(nvdimm_dev, method);
> > > +
> > > +        /* _LSW */
> > > +        method = aml_method("_LSW", 3, AML_SERIALIZED);
> > > +        aml_append(method, aml_store(aml_arg(2), aml_local(2)));
> > > +        aml_append(method, aml_name_decl("INPT", aml_buffer(8,
> > > NULL)));
> > > +        field = aml_create_dword_field(aml_name("INPT"),
> > > +                                                  aml_int(0),
> > > "OFST");
> > > +        aml_append(method, field);
> > > +        field = aml_create_dword_field(aml_name("INPT"),
> > > +                                                  aml_int(4),
> > > "TLEN");
> > > +        aml_append(method, field);
> > > +        aml_append(method, aml_store(aml_arg(0),
> > > aml_name("OFST")));
> > > +        aml_append(method, aml_store(aml_arg(1),
> > > aml_name("TLEN")));
> > > +
> > > +        aml_append(method, aml_concatenate(aml_name("INPT"),
> > > aml_local(2),
> > > +                                            aml_name("INPT")));
> > > +        pkg = aml_package(1);
> > > +        aml_append(pkg, aml_name("INPT"));
> > > +        aml_append(method, aml_name_decl("PKG1", pkg));
> > > +        com_call = aml_call5(NVDIMM_COMMON_DSM,
> > > +                            aml_touuid(NVDIMM_DEVICE_DSM_UUID),
> > > +                            aml_int(1), aml_int(6),
> > > aml_name("PKG1"),
> > > +                            aml_int(handle));
> > > +        aml_append(method, aml_store(com_call, aml_local(3)));
> > > +        field = aml_create_dword_field(aml_local(3), aml_int(0),
> > > "STTS");
> > > +        aml_append(method, field);
> > > +        aml_append(method,
> > > aml_return(aml_to_integer(aml_name("STTS"))));  
> > 
> > why do you need explicitly convert DWORD field to integer?
> > it should be fine to return STTS directly (implicit conversion should
> > take care of the rest)
> >   
> > > +        aml_append(nvdimm_dev, method);
> > > +
> > >          nvdimm_build_device_dsm(nvdimm_dev, handle);
> > >          aml_append(root_dev, nvdimm_dev);
> > >      }  
> > 
> >   
>