Re: [PATCH 1/2] acpi/nvdimm: Create _LS{I,R,W} method for NVDIMM device

[Igor Mammedov]

On Tue, 12 Apr 2022 14:57:52 +0800
Robert Hoo <robert.hu@linux.intel.com> wrote:

> Since ACPI 6.2, previous NVDIMM/_DSM funcions "Get Namespace Label Data
> Size (function index 4)", "Get Namespace Label Data (function index 5)",
> "Set Namespace Label Data (function index 6)" has been deprecated by ACPI

where it's said that old way was deprecated, should be mentioned here including
pointer to spec where it came into effect.

> standard method _LSI, _LSR, _LSW respectively. Functions semantics are
> almost identical, so my implementation is to reuse existing _DSMs, just
> create _LS{I,R,W} interfaces and constructs parameters and call _DSMs.
> 
> Only child NVDIMM devices has these methods, rather Root device.
> 
> By this patch, new NVDIMM sub device in ACPI namespace will be like this:
> 
> Device (NV00)
> {
>       Name (_ADR, One)  // _ADR: Address
>         Method (_LSI, 0, NotSerialized)  // _LSI: Label Storage Information
>         {
>              Return (NCAL (ToUUID ("4309ac30-0d11-11e4-9191-0800200c9a66"), 
> 0x02, 0x04, Zero, One))
>         }
> 
>         Method (_LSR, 2, Serialized)  // _LSR: Label Storage Read
>         {
>               CreateDWordField (BUFF, Zero, DWD0)
>                 CreateDWordField (BUFF, 0x04, DWD1)
>                 Name (PKG1, Package (0x01)
>                 {
>                     BUFF
>                 })
>                 DWD0 = Arg0
>                 DWD1 = Arg1
>                 Return (NCAL (ToUUID 
> ("4309ac30-0d11-11e4-9191-0800200c9a66"), 0x02, 0x05, PKG1, One))
>         }
> 
>         Method (_LSW, 3, Serialized)  // _LSW: Label Storage Write
>         {
>                 CreateDWordField (BUFF, Zero, DWD0)
>                 CreateDWordField (BUFF, 0x04, DWD1)
>                 CreateField (BUFF, 0x40, 0x7FA0, FILD)
>                 Name (PKG1, Package (0x01)
>                 {
>                     BUFF
>                 })
>                 DWD0 = Arg0
>                 DWD1 = Arg1
>                 FILD = Arg2
>                 Return (NCAL (ToUUID 
> ("4309ac30-0d11-11e4-9191-0800200c9a66"), 0x02, 0x06, PKG1, One))
>          }
> 
>          Method (_DSM, 4, NotSerialized)  // _DSM: Device-Specific Method
>          {
>                 Return (NCAL (Arg0, Arg1, Arg2, Arg3, One))
>          }
> }
> 
> Signed-off-by: Robert Hoo <robert.hu@linux.intel.com>
> Reviewed-by: Jingqi Liu<jingqi.liu@intel.com>
> ---
>  hw/acpi/nvdimm.c | 56 ++++++++++++++++++++++++++++++++++++++++++++----
>  1 file changed, 52 insertions(+), 4 deletions(-)
> 
> diff --git a/hw/acpi/nvdimm.c b/hw/acpi/nvdimm.c
> index 0d43da19ea..7cc419401b 100644
> --- a/hw/acpi/nvdimm.c
> +++ b/hw/acpi/nvdimm.c
> @@ -848,10 +848,10 @@ nvdimm_dsm_write(void *opaque, hwaddr addr, uint64_t 
> val, unsigned size)
>  
>      nvdimm_debug("Revision 0x%x Handler 0x%x Function 0x%x.\n", in->revision,
>                   in->handle, in->function);
> -
> -    if (in->revision != 0x1 /* Currently we only support DSM Spec Rev1. */) {
> -        nvdimm_debug("Revision 0x%x is not supported, expect 0x%x.\n",
> -                     in->revision, 0x1);
> +    /* Currently we only support DSM Spec Rev1 and Rev2. */

where does revision 2 come from? It would be better to add a pointer to 
relevant spec.

> +    if (in->revision != 0x1 && in->revision != 0x2) {
> +        nvdimm_debug("Revision 0x%x is not supported, expect 0x1 or 0x2.\n",
> +                     in->revision);

since you are touching nvdimm_debug(), please replace it with tracing,
see docs/devel/tracing.rst and any commit that adds tracing calls
(functions starting with 'trace_').

>          nvdimm_dsm_no_payload(NVDIMM_DSM_RET_STATUS_UNSUPPORT, dsm_mem_addr);
>          goto exit;
>      }


this whole hunk should be a separate patch, properly documented


also I wonder if DSM

> @@ -1247,6 +1247,11 @@ 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, *buff;
> +
> +    /* Build common shared buffer for params pass in/out */
> +    buff = aml_buffer(4096, NULL);
> +    aml_append(root_dev, aml_name_decl("BUFF", buff));

is there a reason to use global variable instead of LocalX?

>  
>      for (slot = 0; slot < ram_slots; slot++) {
>          uint32_t handle = nvdimm_slot_to_handle(slot);
> @@ -1264,6 +1269,49 @@ static void nvdimm_build_nvdimm_devices(Aml *root_dev, 
> uint32_t ram_slots)
>           */
>          aml_append(nvdimm_dev, aml_name_decl("_ADR", aml_int(handle)));
>  
> +        /* Build _LSI, _LSR, _LSW */

should be 1 comment per method with spec/ver and chapter where it's defined

> +        method = aml_method("_LSI", 0, AML_NOTSERIALIZED);
> +        aml_append(method, aml_return(aml_call5(NVDIMM_COMMON_DSM,
> +                            
> aml_touuid("4309AC30-0D11-11E4-9191-0800200C9A66"),
> +                            aml_int(2), aml_int(4), aml_int(0),
> +                            aml_int(handle))));
> +        aml_append(nvdimm_dev, method);

_LSI should return Package

> +        method = aml_method("_LSR", 2, AML_SERIALIZED);
> +        aml_append(method,
> +            aml_create_dword_field(aml_name("BUFF"), aml_int(0), "DWD0"));
> +        aml_append(method,
> +            aml_create_dword_field(aml_name("BUFF"), aml_int(4), "DWD1"));
theoretically aml_create_dword_field() takes TermArg as source buffer,
so it doesn't have to be a global named buffer.
Try keep buffer in LocalX variable and check if it works in earliest
Windows version that supports NVDIMMs. If it does then drop BUFF and use
Local variable, if not then that fact should be mentioned in commit 
message/patch

> +        pkg = aml_package(1);
> +        aml_append(pkg, aml_name("BUFF"));
> +        aml_append(method, aml_name_decl("PKG1", pkg));
> +        aml_append(method, aml_store(aml_arg(0), aml_name("DWD0")));
> +        aml_append(method, aml_store(aml_arg(1), aml_name("DWD1")));
perhaps use less magical names for fields, something like:
  DOFF
  TLEN
add appropriate comments

Also I'd prepare/fill in buffer first and only then declare initialize
Package + don't use named object for Package if it can be done with help
of Local variables.

> +        aml_append(method, aml_return(aml_call5(NVDIMM_COMMON_DSM,
> +                            
> aml_touuid("4309AC30-0D11-11E4-9191-0800200C9A66"),
> +                            aml_int(2), aml_int(5), aml_name("PKG1"),
> +                            aml_int(handle))));

this shall return Package not a Buffer

> +        aml_append(nvdimm_dev, method);
> +
> +        method = aml_method("_LSW", 3, AML_SERIALIZED);
> +        aml_append(method,
> +            aml_create_dword_field(aml_name("BUFF"), aml_int(0), "DWD0"));
> +        aml_append(method,
> +            aml_create_dword_field(aml_name("BUFF"), aml_int(4), "DWD1"));
> +        aml_append(method,
> +            aml_create_field(aml_name("BUFF"), aml_int(64), aml_int(32672), 
> "FILD"));
> +        pkg = aml_package(1);
> +        aml_append(pkg, aml_name("BUFF"));
> +        aml_append(method, aml_name_decl("PKG1", pkg));
> +        aml_append(method, aml_store(aml_arg(0), aml_name("DWD0")));
> +        aml_append(method, aml_store(aml_arg(1), aml_name("DWD1")));
> +        aml_append(method, aml_store(aml_arg(2), aml_name("FILD")));
> +        aml_append(method, aml_return(aml_call5(NVDIMM_COMMON_DSM,
> +                            
> aml_touuid("4309AC30-0D11-11E4-9191-0800200C9A66"),
> +                            aml_int(2), aml_int(6), aml_name("PKG1"),
> +                            aml_int(handle))));

should return Integer not Buffer, it looks like implicit conversion will take 
care of it,
but it would be better to explicitly convert it to Integer even if it's only 
for the sake
of documenting expected return value (or add a comment)

Also returned value in case of error NVDIMM_DSM_RET_STATUS_INVALID,
in NVDIMM and ACPI spec differ. So fix the spec or remap returned value.


> +        aml_append(nvdimm_dev, method);
> +
>          nvdimm_build_device_dsm(nvdimm_dev, handle);
>          aml_append(root_dev, nvdimm_dev);
>      }