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