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

[Igor Mammedov <imammedo@redhat.com>]

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