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