[RESEND][PATCH 0/2] acpi/nvdimm: support NVDIMM _LS{I,R,W} methods

[RESEND][PATCH 0/2] acpi/nvdimm: support NVDIMM _LS{I,R,W} methods

[Robert Hoo]

The original NVDIMM _DSM functions (index 4~6) for label operations have
been deprecated by new ACPI methods _LS{I,R,W}[1][2].

Patch 1 implements the new _LS{I,R,W} methods, on top of old _DSM
implementation.

Patch 2 fixes some typo of logical and/or with bitwise and/or, though
functionally they haven't causing trouble.

[1] https://uefi.org/htmlspecs/ACPI_Spec_6_4_html/index.html, 6.5.10 NVDIMM Label Methods
[2] https://pmem.io/documents/IntelOptanePMem_DSM_Interface-V2.0.pdf, 3.10 Deprecated Functions

---
Resend for previous failed delivery to "qemu-devel@nongnu.org" due to
550-'Message headers fail syntax check'. 

Robert Hoo (2):
  acpi/nvdimm: Create _LS{I,R,W} method for NVDIMM device
  acpi/nvdimm: Fix aml_or() and aml_and() in if clause

 hw/acpi/nvdimm.c | 60 +++++++++++++++++++++++++++++++++++++++++++-----
 1 file changed, 54 insertions(+), 6 deletions(-)


base-commit: 95a3fcc7487e5bef262e1f937ed8636986764c4e
-- 
2.31.1

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

[Robert Hoo]

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
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. */
+    if (in->revision != 0x1 && in->revision != 0x2) {
+        nvdimm_debug("Revision 0x%x is not supported, expect 0x1 or 0x2.\n",
+                     in->revision);
         nvdimm_dsm_no_payload(NVDIMM_DSM_RET_STATUS_UNSUPPORT, dsm_mem_addr);
         goto exit;
     }
@@ -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));
 
     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 */
+        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);
+
+        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"));
+        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_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))));
+        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))));
+        aml_append(nvdimm_dev, method);
+
         nvdimm_build_device_dsm(nvdimm_dev, handle);
         aml_append(root_dev, nvdimm_dev);
     }
-- 
2.31.1

[PATCH 2/2] acpi/nvdimm: Fix aml_or() and aml_and() in if clause

[Robert Hoo]

It should be some typo originally, where in If condition, using bitwise
and/or, rather than logical and/or.

The resulting change in AML code:

If (((Local6 == Zero) | (Arg0 != Local0)))
==>
If (((Local6 == Zero) || (Arg0 != Local0)))

If (((ObjectType (Arg3) == 0x04) & (SizeOf (Arg3) == One)))
==>
If (((ObjectType (Arg3) == 0x04) && (SizeOf (Arg3) == One)))

Fixes: 90623ebf603 ("nvdimm acpi: check UUID")
Fixes: 4568c948066 ("nvdimm acpi: save arg3 of _DSM method")
Signed-off-by: Robert Hoo <robert.hu@linux.intel.com>
Reviewed-by: Jingqi Liu <jingqi.liu@intel.com>
---
 hw/acpi/nvdimm.c | 7 +++----
 1 file changed, 3 insertions(+), 4 deletions(-)

diff --git a/hw/acpi/nvdimm.c b/hw/acpi/nvdimm.c
index 7cc419401b..2cd26bb9e9 100644
--- a/hw/acpi/nvdimm.c
+++ b/hw/acpi/nvdimm.c
@@ -1040,7 +1040,7 @@ static void nvdimm_build_common_dsm(Aml *dev,
 
     uuid_invalid = aml_lnot(aml_equal(uuid, expected_uuid));
 
-    unsupport = aml_if(aml_or(unpatched, uuid_invalid, NULL));
+    unsupport = aml_if(aml_lor(unpatched, uuid_invalid));
 
     /*
      * function 0 is called to inquire what functions are supported by
@@ -1072,10 +1072,9 @@ static void nvdimm_build_common_dsm(Aml *dev,
      * in the DSM Spec.
      */
     pckg = aml_arg(3);
-    ifctx = aml_if(aml_and(aml_equal(aml_object_type(pckg),
+    ifctx = aml_if(aml_land(aml_equal(aml_object_type(pckg),
                    aml_int(4 /* Package */)) /* It is a Package? */,
-                   aml_equal(aml_sizeof(pckg), aml_int(1)) /* 1 element? */,
-                   NULL));
+                   aml_equal(aml_sizeof(pckg), aml_int(1)) /* 1 element? */));
 
     pckg_index = aml_local(2);
     pckg_buf = aml_local(3);
-- 
2.31.1

Re: [RESEND][PATCH 0/2] acpi/nvdimm: support NVDIMM _LS{I,R,W} methods

[Robert Hoo]

Ping...

On Tue, 2022-04-12 at 14:57 +0800, Robert Hoo wrote:
> The original NVDIMM _DSM functions (index 4~6) for label operations
> have
> been deprecated by new ACPI methods _LS{I,R,W}[1][2].
> 
> Patch 1 implements the new _LS{I,R,W} methods, on top of old _DSM
> implementation.
> 
> Patch 2 fixes some typo of logical and/or with bitwise and/or, though
> functionally they haven't causing trouble.
> 
> [1] https://uefi.org/htmlspecs/ACPI_Spec_6_4_html/index.html, 6.5.10
> NVDIMM Label Methods
> [2] https://pmem.io/documents/IntelOptanePMem_DSM_Interface-V2.0.pdf,
> 3.10 Deprecated Functions
> 
> ---
> Resend for previous failed delivery to "qemu-devel@nongnu.org" due to
> 550-'Message headers fail syntax check'. 
> 
> Robert Hoo (2):
>   acpi/nvdimm: Create _LS{I,R,W} method for NVDIMM device
>   acpi/nvdimm: Fix aml_or() and aml_and() in if clause
> 
>  hw/acpi/nvdimm.c | 60 +++++++++++++++++++++++++++++++++++++++++++---
> --
>  1 file changed, 54 insertions(+), 6 deletions(-)
> 
> 
> base-commit: 95a3fcc7487e5bef262e1f937ed8636986764c4e

Re: [PATCH 2/2] acpi/nvdimm: Fix aml_or() and aml_and() in if clause

[Igor Mammedov]

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

> It should be some typo originally, where in If condition, using bitwise
> and/or, rather than logical and/or.
> 
> The resulting change in AML code:
> 
> If (((Local6 == Zero) | (Arg0 != Local0)))
> ==>  
> If (((Local6 == Zero) || (Arg0 != Local0)))
> 
> If (((ObjectType (Arg3) == 0x04) & (SizeOf (Arg3) == One)))
> ==>  
> If (((ObjectType (Arg3) == 0x04) && (SizeOf (Arg3) == One)))
> 
> Fixes: 90623ebf603 ("nvdimm acpi: check UUID")
> Fixes: 4568c948066 ("nvdimm acpi: save arg3 of _DSM method")
> Signed-off-by: Robert Hoo <robert.hu@linux.intel.com>
> Reviewed-by: Jingqi Liu <jingqi.liu@intel.com>

Reviewed-by: Igor Mammedov <imammedo@redhat.com>

> ---
>  hw/acpi/nvdimm.c | 7 +++----
>  1 file changed, 3 insertions(+), 4 deletions(-)
> 
> diff --git a/hw/acpi/nvdimm.c b/hw/acpi/nvdimm.c
> index 7cc419401b..2cd26bb9e9 100644
> --- a/hw/acpi/nvdimm.c
> +++ b/hw/acpi/nvdimm.c
> @@ -1040,7 +1040,7 @@ static void nvdimm_build_common_dsm(Aml *dev,
>  
>      uuid_invalid = aml_lnot(aml_equal(uuid, expected_uuid));
>  
> -    unsupport = aml_if(aml_or(unpatched, uuid_invalid, NULL));
> +    unsupport = aml_if(aml_lor(unpatched, uuid_invalid));
>  
>      /*
>       * function 0 is called to inquire what functions are supported by
> @@ -1072,10 +1072,9 @@ static void nvdimm_build_common_dsm(Aml *dev,
>       * in the DSM Spec.
>       */
>      pckg = aml_arg(3);
> -    ifctx = aml_if(aml_and(aml_equal(aml_object_type(pckg),
> +    ifctx = aml_if(aml_land(aml_equal(aml_object_type(pckg),
>                     aml_int(4 /* Package */)) /* It is a Package? */,
> -                   aml_equal(aml_sizeof(pckg), aml_int(1)) /* 1 element? */,
> -                   NULL));
> +                   aml_equal(aml_sizeof(pckg), aml_int(1)) /* 1 element? */));
>  
>      pckg_index = aml_local(2);
>      pckg_buf = aml_local(3);

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

Re: [RESEND][PATCH 0/2] acpi/nvdimm: support NVDIMM _LS{I,R,W} methods

[Igor Mammedov]

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

> The original NVDIMM _DSM functions (index 4~6) for label operations have
> been deprecated by new ACPI methods _LS{I,R,W}[1][2].
> 
> Patch 1 implements the new _LS{I,R,W} methods, on top of old _DSM
> implementation.
> 
> Patch 2 fixes some typo of logical and/or with bitwise and/or, though
> functionally they haven't causing trouble.

generic requirement for ACPI patches,
the should pass bios-tables-test (part of 'make check')

for that you need to update testcase expected data,
see tests/qtest/bios-tables-test.c for the process
also see https://www.mail-archive.com/qemu-devel@nongnu.org/msg875304.html for example

> 
> [1] https://uefi.org/htmlspecs/ACPI_Spec_6_4_html/index.html, 6.5.10 NVDIMM Label Methods
> [2] https://pmem.io/documents/IntelOptanePMem_DSM_Interface-V2.0.pdf, 3.10 Deprecated Functions
> 
> ---
> Resend for previous failed delivery to "qemu-devel@nongnu.org" due to
> 550-'Message headers fail syntax check'. 
> 
> Robert Hoo (2):
>   acpi/nvdimm: Create _LS{I,R,W} method for NVDIMM device
>   acpi/nvdimm: Fix aml_or() and aml_and() in if clause
> 
>  hw/acpi/nvdimm.c | 60 +++++++++++++++++++++++++++++++++++++++++++-----
>  1 file changed, 54 insertions(+), 6 deletions(-)
> 
> 
> base-commit: 95a3fcc7487e5bef262e1f937ed8636986764c4e

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

[Robert Hoo]

On Wed, 2022-04-27 at 16:34 +0200, Igor Mammedov wrote:
> 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.

OK. https://pmem.io/documents/IntelOptanePMem_DSM_Interface-V2.0.pdf,
3.10 Deprecated Functions.
I put it in cover letter. Will also mention it here.
> 
...
> > 
> > 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.

https://pmem.io/documents/IntelOptanePMem_DSM_Interface-V2.0.pdf,
Section 3 "_DSM Interface for the NVDIMM Device", table 3-A and 3-B.

I'll add this in comments in next version.
> 
> > +    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_').

OK I'll have a try.
> 
> >          nvdimm_dsm_no_payload(NVDIMM_DSM_RET_STATUS_UNSUPPORT,
> > dsm_mem_addr);
> >          goto exit;
> >      }
> 
> 
> this whole hunk should be a separate patch, properly documented
> 
OK
> 
> also I wonder if DSM

It's not in SDM, but above-mentioned _DSM Interface spec by Intel.
> 
> > @@ -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?

Local in root_dev but global to its sub devices? I think it is doable.

But given your below comments on return param _LS{I,R,W}, I now think,
in v2, I'm not going to reuse existing "NCAL" method, but implement
_LS{I,R,W} their own, stringently follow interface spec. Then, no buff
required at all. How do you like this?
> 
> >  
> >      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

OK
> 
> > +        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

Right. See above.
> 
> > +        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

Thanks Igor. I'm new to asl grammar, I'll take your advice.

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

No problem.
> 
> 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

Right, Going to re-implement these methods rather than wrapper NCAL.
> 
> > +        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)

I observed guest kernel ACPI component complaining this; just warning,
no harm. I'll re-implement it.
> 
> 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);
> >      }
> 
> 

Re: [RESEND][PATCH 0/2] acpi/nvdimm: support NVDIMM _LS{I,R,W} methods

[Robert Hoo]

On Wed, 2022-04-27 at 16:39 +0200, Igor Mammedov wrote:
> On Tue, 12 Apr 2022 14:57:51 +0800
> Robert Hoo <robert.hu@linux.intel.com> wrote:
> 
> > The original NVDIMM _DSM functions (index 4~6) for label operations
> > have
> > been deprecated by new ACPI methods _LS{I,R,W}[1][2].
> > 
> > Patch 1 implements the new _LS{I,R,W} methods, on top of old _DSM
> > implementation.
> > 
> > Patch 2 fixes some typo of logical and/or with bitwise and/or,
> > though
> > functionally they haven't causing trouble.
> 
> generic requirement for ACPI patches,
> the should pass bios-tables-test (part of 'make check')
> 
> for that you need to update testcase expected data,
> see tests/qtest/bios-tables-test.c for the process
> also see 
> https://www.mail-archive.com/qemu-devel@nongnu.org/msg875304.html for
> example
> 

Got it. Thanks Igor.
> > 
> > [1] https://uefi.org/htmlspecs/ACPI_Spec_6_4_html/index.html,
> > 6.5.10 NVDIMM Label Methods
> > [2] 
> > https://pmem.io/documents/IntelOptanePMem_DSM_Interface-V2.0.pdf,
> > 3.10 Deprecated Functions
> > 
> > ---
> > Resend for previous failed delivery to "qemu-devel@nongnu.org" due
> > to
> > 550-'Message headers fail syntax check'. 
> > 
> > Robert Hoo (2):
> >   acpi/nvdimm: Create _LS{I,R,W} method for NVDIMM device
> >   acpi/nvdimm: Fix aml_or() and aml_and() in if clause
> > 
> >  hw/acpi/nvdimm.c | 60 +++++++++++++++++++++++++++++++++++++++++++-
> > ----
> >  1 file changed, 54 insertions(+), 6 deletions(-)
> > 
> > 
> > base-commit: 95a3fcc7487e5bef262e1f937ed8636986764c4e
> 
> 

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

[Igor Mammedov]

On Fri, 29 Apr 2022 17:01:47 +0800
Robert Hoo <robert.hu@linux.intel.com> wrote:

> On Wed, 2022-04-27 at 16:34 +0200, Igor Mammedov wrote:
> > 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.  
> 
> OK. https://pmem.io/documents/IntelOptanePMem_DSM_Interface-V2.0.pdf,
> 3.10 Deprecated Functions.
> I put it in cover letter. Will also mention it here.
> >   
> ...
> > > 
> > > 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.  
> 
> https://pmem.io/documents/IntelOptanePMem_DSM_Interface-V2.0.pdf,
> Section 3 "_DSM Interface for the NVDIMM Device", table 3-A and 3-B.
> 
> I'll add this in comments in next version.
> >   
> > > +    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_').  
> 
> OK I'll have a try.

just make conversion a separate patch

> >   
> > >          nvdimm_dsm_no_payload(NVDIMM_DSM_RET_STATUS_UNSUPPORT,
> > > dsm_mem_addr);
> > >          goto exit;
> > >      }  
> > 
> > 
> > this whole hunk should be a separate patch, properly documented
> >   
> OK
> > 
> > also I wonder if DSM  
> 
> It's not in SDM, but above-mentioned _DSM Interface spec by Intel.
> >   
> > > @@ -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?  
> 
> Local in root_dev but global to its sub devices? I think it is doable.
> 
> But given your below comments on return param _LS{I,R,W}, I now think,
> in v2, I'm not going to reuse existing "NCAL" method, but implement
> _LS{I,R,W} their own, stringently follow interface spec. Then, no buff
> required at all. How do you like this?
> >   
> > >  
> > >      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  
> 
> OK
> >   
> > > +        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  
> 
> Right. See above.
> >   
> > > +        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  
> 
> Thanks Igor. I'm new to asl grammar, I'll take your advice.
> 
> >   
> > > +        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  
> 
> No problem.
> > 
> > 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  
> 
> Right, Going to re-implement these methods rather than wrapper NCAL.

wrapper is fine, you just need to repackage content of the Buffer
into a Package

> >   
> > > +        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)  
> 
> I observed guest kernel ACPI component complaining this; just warning,
> no harm. I'll re-implement it.

try to test it with Windows guest (it usually less tolerable to errors
than Linux + it's own quirks that you need to carter to)
Also it would e nice if you test and put results in cover letter
not only for Linux but for Windows as well.

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

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

[Robert Hoo]

On Tue, 2022-05-03 at 10:27 +0200, Igor Mammedov wrote:
> On Fri, 29 Apr 2022 17:01:47 +0800
> Robert Hoo <robert.hu@linux.intel.com> wrote:
> 
> > On Wed, 2022-04-27 at 16:34 +0200, Igor Mammedov wrote:
> > > 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.  
> > 
> > OK. 
> > https://pmem.io/documents/IntelOptanePMem_DSM_Interface-V2.0.pdf,
> > 3.10 Deprecated Functions.
> > I put it in cover letter. Will also mention it here.
> > >   
> > 
> > ...
> > > > 
> > > > 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.  
> > 
> > https://pmem.io/documents/IntelOptanePMem_DSM_Interface-V2.0.pdf,
> > Section 3 "_DSM Interface for the NVDIMM Device", table 3-A and 3-
> > B.
> > 
> > I'll add this in comments in next version.
> > >   
> > > > +    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_').  
> > 
> > OK I'll have a try.
> 
> just make conversion a separate patch

Yeah, I supposed so too.
> 
> > >   
> > > >          nvdimm_dsm_no_payload(NVDIMM_DSM_RET_STATUS_UNSUPPORT,
> > > > dsm_mem_addr);
> > > >          goto exit;
> > > >      }  
> > > 
> > > 
> > > this whole hunk should be a separate patch, properly documented
> > >   
> > 
> > OK
> > > 
> > > also I wonder if DSM  
> > 
> > It's not in SDM, but above-mentioned _DSM Interface spec by Intel.
> > >   
> > > > @@ -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?  
> > 
> > Local in root_dev but global to its sub devices? I think it is
> > doable.
> > 
> > But given your below comments on return param _LS{I,R,W}, I now
> > think,
> > in v2, I'm not going to reuse existing "NCAL" method, but implement
> > _LS{I,R,W} their own, stringently follow interface spec. Then, no
> > buff
> > required at all. How do you like this?
> > >   
> > > >  
> > > >      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  
> > 
> > OK
> > >   
> > > > +        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  
> > 
> > Right. See above.
> > >   
> > > > +        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  
> > 
> > Thanks Igor. I'm new to asl grammar, I'll take your advice.
> > 
> > >   
> > > > +        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  
> > 
> > No problem.
> > > 
> > > 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  
> > 
> > Right, Going to re-implement these methods rather than wrapper
> > NCAL.
> 
> wrapper is fine, you just need to repackage content of the Buffer
> into a Package
> 
I now prefer re-implementation, i.e. make _LS{I,R,W} their own
functions, less NACL's burden and don't make it more complex, it's
already not neat; and more point, I think by this we can save the 4K
Buff at all.
Does this sound all right to you?

> > >   
> > > > +        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)  
> > 
> > I observed guest kernel ACPI component complaining this; just
> > warning,
> > no harm. I'll re-implement it.
> 
> try to test it with Windows guest (it usually less tolerable to
> errors
> than Linux + it's own quirks that you need to carter to)
> Also it would e nice if you test and put results in cover letter
> not only for Linux but for Windows as well.
> 
I'll try to, but have no Windows resource at hand, I'll ask around if
any test resource to cover that.
> > > 
> > > 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);
> > > >      }  
> > > 
> > >   
> 
> 

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

[Igor Mammedov]

On Thu, 05 May 2022 11:07:53 +0800
Robert Hoo <robert.hu@linux.intel.com> wrote:

> On Tue, 2022-05-03 at 10:27 +0200, Igor Mammedov wrote:
> > On Fri, 29 Apr 2022 17:01:47 +0800
> > Robert Hoo <robert.hu@linux.intel.com> wrote:
> >   
> > > On Wed, 2022-04-27 at 16:34 +0200, Igor Mammedov wrote:  
> > > > 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.    
> > > 
> > > OK. 
> > > https://pmem.io/documents/IntelOptanePMem_DSM_Interface-V2.0.pdf,
> > > 3.10 Deprecated Functions.
> > > I put it in cover letter. Will also mention it here.  
> > > >     
> > > 
> > > ...  
> > > > > 
> > > > > 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.    
> > > 
> > > https://pmem.io/documents/IntelOptanePMem_DSM_Interface-V2.0.pdf,
> > > Section 3 "_DSM Interface for the NVDIMM Device", table 3-A and 3-
> > > B.
> > > 
> > > I'll add this in comments in next version.  
> > > >     
> > > > > +    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_').    
> > > 
> > > OK I'll have a try.  
> > 
> > just make conversion a separate patch  
> 
> Yeah, I supposed so too.
> >   
> > > >     
> > > > >          nvdimm_dsm_no_payload(NVDIMM_DSM_RET_STATUS_UNSUPPORT,
> > > > > dsm_mem_addr);
> > > > >          goto exit;
> > > > >      }    
> > > > 
> > > > 
> > > > this whole hunk should be a separate patch, properly documented
> > > >     
> > > 
> > > OK  
> > > > 
> > > > also I wonder if DSM    
> > > 
> > > It's not in SDM, but above-mentioned _DSM Interface spec by Intel.  
> > > >     
> > > > > @@ -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?    
> > > 
> > > Local in root_dev but global to its sub devices? I think it is
> > > doable.
> > > 
> > > But given your below comments on return param _LS{I,R,W}, I now
> > > think,
> > > in v2, I'm not going to reuse existing "NCAL" method, but implement
> > > _LS{I,R,W} their own, stringently follow interface spec. Then, no
> > > buff
> > > required at all. How do you like this?  
> > > >     
> > > > >  
> > > > >      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    
> > > 
> > > OK  
> > > >     
> > > > > +        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    
> > > 
> > > Right. See above.  
> > > >     
> > > > > +        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    
> > > 
> > > Thanks Igor. I'm new to asl grammar, I'll take your advice.
> > >   
> > > >     
> > > > > +        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    
> > > 
> > > No problem.  
> > > > 
> > > > 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    
> > > 
> > > Right, Going to re-implement these methods rather than wrapper
> > > NCAL.  
> > 
> > wrapper is fine, you just need to repackage content of the Buffer
> > into a Package
> >   
> I now prefer re-implementation, i.e. make _LS{I,R,W} their own
> functions, less NACL's burden and don't make it more complex, it's
> already not neat; and more point, I think by this we can save the 4K
> Buff at all.
> Does this sound all right to you?

On one hand what you propose will be bit simpler (but not mach) than
repacking (and only on AML guest side), however it will add to host
side an extra interface/ABI that we will have to maintain, also it
won't save space as buffer will still be there for legacy interface.

So unless we have to add new host/guest ABI, I'd prefer reusing
existing one and complicate only new _LS{I,R,W} AML without
touching NACL or host side.

> 
> > > >     
> > > > > +        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)    
> > > 
> > > I observed guest kernel ACPI component complaining this; just
> > > warning,
> > > no harm. I'll re-implement it.  
> > 
> > try to test it with Windows guest (it usually less tolerable to
> > errors
> > than Linux + it's own quirks that you need to carter to)
> > Also it would e nice if you test and put results in cover letter
> > not only for Linux but for Windows as well.
> >   
> I'll try to, but have no Windows resource at hand, I'll ask around if
> any test resource to cover that.
> > > > 
> > > > 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);
> > > > >      }    
> > > > 
> > > >     
> > 
> >   
> 

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

[Robert Hoo]

On Thu, 2022-05-05 at 10:50 +0200, Igor Mammedov wrote:
...
> > > > > > @@ -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?    
> > > > 
> > > > Local in root_dev but global to its sub devices? I think it is
> > > > doable.
> > > > 
> > > > But given your below comments on return param _LS{I,R,W}, I now
> > > > think,
> > > > in v2, I'm not going to reuse existing "NCAL" method, but
> > > > implement
> > > > _LS{I,R,W} their own, stringently follow interface spec. Then,
> > > > no
> > > > buff
> > > > required at all. How do you like this?  
> > > > >     
...
> > > > >     
> > > > > > +        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    
> > > > 
> > > > Right. See above.  
> > > > >     
> > > > > > +        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    
> > > > 
> > > > Thanks Igor. I'm new to asl grammar, I'll take your advice.
> > > >   
> > > > >     
> > > > > > +        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    
> > > > 
> > > > No problem.  
> > > > > 
> > > > > 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    
> > > > 
> > > > Right, Going to re-implement these methods rather than wrapper
> > > > NCAL.  
> > > 
> > > wrapper is fine, you just need to repackage content of the Buffer
> > > into a Package
> > >   
> > 
> > I now prefer re-implementation, i.e. make _LS{I,R,W} their own
> > functions, less NACL's burden and don't make it more complex, it's
> > already not neat; and more point, I think by this we can save the
> > 4K
> > Buff at all.
> > Does this sound all right to you?
> 
> On one hand what you propose will be bit simpler (but not mach) than
> repacking (and only on AML guest side), however it will add to host
> side an extra interface/ABI that we will have to maintain, also it
> won't save space as buffer will still be there for legacy interface.

No, sorry, I didn't explain it clear.
No extra interface/ABI but these 3 must _LS{I,R,W} nvdimm-sub-device
methods. Of course, I'm going to extract 'SystemIO' and 'SystemMemory'
operation regions out of NACL to be globally available.

The buffer (BUFF in above patch) will be gone. It is added by my this
patch, its mere use is to covert param of _LS{I,R,W} into those of
NACL. If I implemented each _LS{I,R,W} on their own, rather than wrap
the multi-purpose NACL, no buffer needed, at least I now assume so.
And, why declare the 4K buffer global to sub-nvdimm? I now recall that
it is because if not each sub-nvdimm device would contain a 4K buff,
which will make this SSDT enormously large.
> 
> So unless we have to add new host/guest ABI, I'd prefer reusing
> existing one and complicate only new _LS{I,R,W} AML without
> touching NACL or host side.

As mentioned above, I assume no new host/guest ABI, just extract
'SystemIO' and 'SystemMemory' operation regions to a higher level
scope.
> 
> > 
> > > > >     
> > > > > > +        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)    
> > > > 
> > > > I observed guest kernel ACPI component complaining this; just
> > > > warning,
> > > > no harm. I'll re-implement it.  
> > > 
> > > try to test it with Windows guest (it usually less tolerable to
> > > errors
> > > than Linux + it's own quirks that you need to carter to)
> > > Also it would e nice if you test and put results in cover letter
> > > not only for Linux but for Windows as well.
> > >   
> > 
> > I'll try to, but have no Windows resource at hand, I'll ask around
> > if
> > any test resource to cover that.
> > > > > 
> > > > > 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);
> > > > > >      }    

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

[Igor Mammedov]

On Thu, 05 May 2022 21:26:59 +0800
Robert Hoo <robert.hu@linux.intel.com> wrote:

> On Thu, 2022-05-05 at 10:50 +0200, Igor Mammedov wrote:
> ...
> > > > > > > @@ -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?      
> > > > > 
> > > > > Local in root_dev but global to its sub devices? I think it is
> > > > > doable.
> > > > > 
> > > > > But given your below comments on return param _LS{I,R,W}, I now
> > > > > think,
> > > > > in v2, I'm not going to reuse existing "NCAL" method, but
> > > > > implement
> > > > > _LS{I,R,W} their own, stringently follow interface spec. Then,
> > > > > no
> > > > > buff
> > > > > required at all. How do you like this?    
> > > > > >       
> ...
> > > > > >       
> > > > > > > +        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      
> > > > > 
> > > > > Right. See above.    
> > > > > >       
> > > > > > > +        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      
> > > > > 
> > > > > Thanks Igor. I'm new to asl grammar, I'll take your advice.
> > > > >     
> > > > > >       
> > > > > > > +        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      
> > > > > 
> > > > > No problem.    
> > > > > > 
> > > > > > 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      
> > > > > 
> > > > > Right, Going to re-implement these methods rather than wrapper
> > > > > NCAL.    
> > > > 
> > > > wrapper is fine, you just need to repackage content of the Buffer
> > > > into a Package
> > > >     
> > > 
> > > I now prefer re-implementation, i.e. make _LS{I,R,W} their own
> > > functions, less NACL's burden and don't make it more complex, it's
> > > already not neat; and more point, I think by this we can save the
> > > 4K
> > > Buff at all.
> > > Does this sound all right to you?  
> > 
> > On one hand what you propose will be bit simpler (but not mach) than
> > repacking (and only on AML guest side), however it will add to host
> > side an extra interface/ABI that we will have to maintain, also it
> > won't save space as buffer will still be there for legacy interface.  
> 
> No, sorry, I didn't explain it clear.
> No extra interface/ABI but these 3 must _LS{I,R,W} nvdimm-sub-device
> methods. Of course, I'm going to extract 'SystemIO' and 'SystemMemory'
> operation regions out of NACL to be globally available.
> 
> The buffer (BUFF in above patch) will be gone. It is added by my this
> patch, its mere use is to covert param of _LS{I,R,W} into those of
> NACL. If I implemented each _LS{I,R,W} on their own, rather than wrap
> the multi-purpose NACL, no buffer needed, at least I now assume so.
> And, why declare the 4K buffer global to sub-nvdimm? I now recall that
> it is because if not each sub-nvdimm device would contain a 4K buff,
> which will make this SSDT enormously large.

ok, lets see how it will look like when you are done.

> > 
> > So unless we have to add new host/guest ABI, I'd prefer reusing
> > existing one and complicate only new _LS{I,R,W} AML without
> > touching NACL or host side.  
> 
> As mentioned above, I assume no new host/guest ABI, just extract
> 'SystemIO' and 'SystemMemory' operation regions to a higher level
> scope.
> >   
> > >   
> > > > > >       
> > > > > > > +        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)      
> > > > > 
> > > > > I observed guest kernel ACPI component complaining this; just
> > > > > warning,
> > > > > no harm. I'll re-implement it.    
> > > > 
> > > > try to test it with Windows guest (it usually less tolerable to
> > > > errors
> > > > than Linux + it's own quirks that you need to carter to)
> > > > Also it would e nice if you test and put results in cover letter
> > > > not only for Linux but for Windows as well.
> > > >     
> > > 
> > > I'll try to, but have no Windows resource at hand, I'll ask around
> > > if
> > > any test resource to cover that.  
> > > > > > 
> > > > > > 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);
> > > > > > >      }      
> 

Re: [PATCH 2/2] acpi/nvdimm: Fix aml_or() and aml_and() in if clause

[Michael S. Tsirkin]

On Tue, Apr 12, 2022 at 02:57:53PM +0800, Robert Hoo wrote:
> It should be some typo originally, where in If condition, using bitwise
> and/or, rather than logical and/or.
> 
> The resulting change in AML code:
> 
> If (((Local6 == Zero) | (Arg0 != Local0)))
> ==>
> If (((Local6 == Zero) || (Arg0 != Local0)))
> 
> If (((ObjectType (Arg3) == 0x04) & (SizeOf (Arg3) == One)))
> ==>
> If (((ObjectType (Arg3) == 0x04) && (SizeOf (Arg3) == One)))
> 
> Fixes: 90623ebf603 ("nvdimm acpi: check UUID")
> Fixes: 4568c948066 ("nvdimm acpi: save arg3 of _DSM method")
> Signed-off-by: Robert Hoo <robert.hu@linux.intel.com>
> Reviewed-by: Jingqi Liu <jingqi.liu@intel.com>

This changes existing AML, you need to do the dance
with updating bios test tables, see header of ./tests/qtest/bios-tables-test.c

> ---
>  hw/acpi/nvdimm.c | 7 +++----
>  1 file changed, 3 insertions(+), 4 deletions(-)
> 
> diff --git a/hw/acpi/nvdimm.c b/hw/acpi/nvdimm.c
> index 7cc419401b..2cd26bb9e9 100644
> --- a/hw/acpi/nvdimm.c
> +++ b/hw/acpi/nvdimm.c
> @@ -1040,7 +1040,7 @@ static void nvdimm_build_common_dsm(Aml *dev,
>  
>      uuid_invalid = aml_lnot(aml_equal(uuid, expected_uuid));
>  
> -    unsupport = aml_if(aml_or(unpatched, uuid_invalid, NULL));
> +    unsupport = aml_if(aml_lor(unpatched, uuid_invalid));
>  
>      /*
>       * function 0 is called to inquire what functions are supported by
> @@ -1072,10 +1072,9 @@ static void nvdimm_build_common_dsm(Aml *dev,
>       * in the DSM Spec.
>       */
>      pckg = aml_arg(3);
> -    ifctx = aml_if(aml_and(aml_equal(aml_object_type(pckg),
> +    ifctx = aml_if(aml_land(aml_equal(aml_object_type(pckg),
>                     aml_int(4 /* Package */)) /* It is a Package? */,
> -                   aml_equal(aml_sizeof(pckg), aml_int(1)) /* 1 element? */,
> -                   NULL));
> +                   aml_equal(aml_sizeof(pckg), aml_int(1)) /* 1 element? */));
>  
>      pckg_index = aml_local(2);
>      pckg_buf = aml_local(3);
> -- 
> 2.31.1

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

[Robert Hoo]

On Fri, 2022-05-06 at 11:23 +0200, Igor Mammedov wrote:
> 
> > 
> > No, sorry, I didn't explain it clear.
> > No extra interface/ABI but these 3 must _LS{I,R,W} nvdimm-sub-
> > device
> > methods. Of course, I'm going to extract 'SystemIO' and
> > 'SystemMemory'
> > operation regions out of NACL to be globally available.
> > 
> > The buffer (BUFF in above patch) will be gone. It is added by my
> > this
> > patch, its mere use is to covert param of _LS{I,R,W} into those of
> > NACL. If I implemented each _LS{I,R,W} on their own, rather than
> > wrap
> > the multi-purpose NACL, no buffer needed, at least I now assume so.
> > And, why declare the 4K buffer global to sub-nvdimm? I now recall
> > that
> > it is because if not each sub-nvdimm device would contain a 4K
> > buff,
> > which will make this SSDT enormously large.
> 
> ok, lets see how it will look like when you are done.

In ASL, can we define package with Arg in? e.g.

Name (PKG1, Package ()
            {
                Arg0,
                Arg1,
                Arg2
            })
But it cannot pass compilation. Any approach to achieve this? if so, we
can still use simpler wrap scheme like v1 and save the 4K buffer.
> 
> > > 
> > > So unless we have to add new host/guest ABI, I'd prefer reusing
> > > existing one and complicate only new _LS{I,R,W} AML without
> > > touching NACL or host side.  
> > 
> > As mentioned above, I assume no new host/guest ABI, just extract
> > 'SystemIO' and 'SystemMemory' operation regions to a higher level
> > scope.
> > >  

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

[Igor Mammedov]

On Wed, 18 May 2022 08:20:56 +0800
Robert Hoo <robert.hu@linux.intel.com> wrote:

> On Fri, 2022-05-06 at 11:23 +0200, Igor Mammedov wrote:
> >   
> > > 
> > > No, sorry, I didn't explain it clear.
> > > No extra interface/ABI but these 3 must _LS{I,R,W} nvdimm-sub-
> > > device
> > > methods. Of course, I'm going to extract 'SystemIO' and
> > > 'SystemMemory'
> > > operation regions out of NACL to be globally available.
> > > 
> > > The buffer (BUFF in above patch) will be gone. It is added by my
> > > this
> > > patch, its mere use is to covert param of _LS{I,R,W} into those of
> > > NACL. If I implemented each _LS{I,R,W} on their own, rather than
> > > wrap
> > > the multi-purpose NACL, no buffer needed, at least I now assume so.
> > > And, why declare the 4K buffer global to sub-nvdimm? I now recall
> > > that
> > > it is because if not each sub-nvdimm device would contain a 4K
> > > buff,
> > > which will make this SSDT enormously large.  
> > 
> > ok, lets see how it will look like when you are done.  
> 
> In ASL, can we define package with Arg in? e.g.
> 
> Name (PKG1, Package ()
>             {
>                 Arg0,
>                 Arg1,
>                 Arg2
>             })

Looking at the spec it doesn't seem to be a valid construct.
see "DefPackage :=" and "PackageElement :=" definitions.

However you can try to play with RefOf to turn ArgX into
reference (mind 'read' rules fro ArgTerm).

> But it cannot pass compilation. Any approach to achieve this? if so, we
> can still use simpler wrap scheme like v1 and save the 4K buffer.



> >   
> > > > 
> > > > So unless we have to add new host/guest ABI, I'd prefer reusing
> > > > existing one and complicate only new _LS{I,R,W} AML without
> > > > touching NACL or host side.    
> > > 
> > > As mentioned above, I assume no new host/guest ABI, just extract
> > > 'SystemIO' and 'SystemMemory' operation regions to a higher level
> > > scope.  
> > > >    
>