On Thu, 1 Sep 2022 11:27:20 +0800
Robert Hoo <robert...@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...@linux.intel.com>
> Reviewed-by: Jingqi Liu <jingqi....@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.
> + 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);
> }
