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