Re: [Qemu-devel] [PATCH] acpi/nvdimm: remove forward name references

[Schmauss, Erik]

> -----Original Message-----
> From: Michael S. Tsirkin [mailto:address@hidden
> Sent: Tuesday, April 24, 2018 10:43 AM
> To: Igor Mammedov <address@hidden>
> Cc: Schmauss, Erik <address@hidden>; address@hidden; Xiao
> Guangrong <address@hidden>; Williams, Dan J
> <address@hidden>
> Subject: Re: [PATCH] acpi/nvdimm: remove forward name references
> 
> On Tue, Apr 24, 2018 at 09:57:22AM +0200, Igor Mammedov wrote:
> > On Tue, 24 Apr 2018 04:02:40 +0300
> > "Michael S. Tsirkin" <address@hidden> wrote:
> >
> > > On Tue, Apr 24, 2018 at 12:41:29AM +0000, Schmauss, Erik wrote:
> > > >
> > > >
> > > > > -----Original Message-----
> > > > > From: Michael S. Tsirkin [mailto:address@hidden
> > > > > Sent: Monday, April 23, 2018 4:03 PM
> > > > > To: address@hidden
> > > > > Cc: Schmauss, Erik <address@hidden>; Igor Mammedov
> > > > > <address@hidden>; Xiao Guangrong
> > > > > <address@hidden>
> > > > > Subject: [PATCH] acpi/nvdimm: remove forward name references
> > > > >
> > > > > NVDIMM SSDT table references a name ("MEMA") before it is
> > > > > defined. This is reported to no longer be supported since Linux 
> > > > > 4.17-rc1.
> > > > >
> > > > > While arguably Linux needs to keep working on old hypervisors,
> > > > > and other OSes seem fine with our behaviour, it seems cleaner to
> > > > > have the definition appear in the SSDT before use.
> > > > >
> > > > > Suggested-by: "Schmauss, Erik" <address@hidden>
> > > > > Cc: address@hidden
> > > > > Signed-off-by: Michael S. Tsirkin <address@hidden>
> > > > > ---
> > > > >
> > > > > Hi Erik,
> > > > > could you pls test the issue and report whether it addresses
> > > > > your concern? I can't
> > > > Hi Michael,
> > > >
> > > > > do much to fix past releases which IIUC shipped this code since
> > > > > 2.6.0 about a year ago.
> > > > >
> > > > > Lightly tested with Linux only.
> > > >
> > > > I'm looking at the ASL tables generated by make check-qtest-x86_64.
> > > > This line
> > >
> > > which line?
> > >
> > > > ends up generating a strange ACPI table where the Operation region
> > > > and field declarations are stuck inside the NCAL method which is
> > > > called from _DSM. If we create the operation region and methods
> > > > inside methods, they disappear after the NCAL method returns.
> > What's wrong with it?
> > Method is complete so temporary objects it has used went out of scope,
> > all within spec rules and worked fine with linux and windows guests.
> >
> > > > I think nvdimm_build_common_dsm() needs some refining.
> > >
> > > What exactly do you refer to?
> > >
> > > DefinitionBlock ("", "SSDT", 1, "BOCHS ", "NVDIMM", 0x00000001) {
> > >     Name (MEMA, 0x07FFE000)
> > >     Scope (\_SB)
> > >     {
> > >         Device (NVDR)
> > >         {
> > >             Name (_HID, "ACPI0012" /* NVDIMM Root Device */)  // _HID:
> Hardware ID
> > >             Method (NCAL, 5, Serialized)
> > >             {
> > >                 Local6 = MEMA /* \MEMA */
> > >                 OperationRegion (NPIO, SystemIO, 0x0A18, 0x04)
> > >                 OperationRegion (NRAM, SystemMemory, Local6, 0x1000)
> > >
> > >           ^^^ this?
> > >
> > > I agree the NPIO could be moved out.
> > > Don't really understand why is Local6 needed - can't MEMA be used
> > > directly? Assuming it isn't NRAM could be moved out too.
> > From spec
> >  DefOpRegion := OpRegionOp NameString RegionSpace RegionOffset
> > RegionLen  RegionOffset := TermArg => Integer  TermArg := Type2Opcode
> > | DataObject | ArgObj | LocalObj
> >
> > So named object is not accepted,
> 
> might be worth checking what happens with actual OSPMs.
> If it does happen to work, we can try tweaking the ACPI spec to allow this.

I'm looking at the ACPI6.2a spec on page 840 and it says
TermArg := Type2Opcode | DataObject | Argterm |LocalTerm |NameString 
|SymbolicExpression

In our case, MEMA is a namestring so we should be able to avoid the local6 and 
insert MEMA instead. Just as long as the Named object appears in the table 
before the object is used, the windows parser should work fine.

After applying this patch, the only change in ASL/AML that I was expecting was 
for MEMA to be moved to the top of the definition block and leave everything 
else alone.
 
We usually do not recommend the creation of objects inside of control methods 
due to the overhead of creating/destroying these objects after each method 
call. So the one problem that I can see with this is that performance could bog 
down if this method is called a lot. If it is not called very frequently, then 
it's probably not a big deal. 

> 
> > we could have played games with
> > references and Type2Opcode but that didn't work nice with Windows ACPI
> > parser. Hence local object.
> 
> A comment in code wouldn't hurt to explain this.
> 
> > The reason why OperationRegion is dynamic, is that if we put it
> > outside of method it will become static, and we would have to use
> > Integer constant there (no named objects are allowed) and then patch
> > it dynamically in bios loader.
> > I'd prefer to keep it as is, without introducing another hack like
> > build_append_named_operation_region() to create it with know offset so
> > firmware could patch it.
> 
> I agree to that.
> 
> > >
> > >
> > > It all seems suboptimal but given the method is serialized, I don't
> > > see anything wrong with it as such.
> > >
> > >
> > > >
> > > > >
> > > > >  hw/acpi/nvdimm.c | 6 ++++--
> > > > >  1 file changed, 4 insertions(+), 2 deletions(-)
> > > > >
> > > > > diff --git a/hw/acpi/nvdimm.c b/hw/acpi/nvdimm.c index
> > > > > 59d6e42..fadebbd
> > > > > 100644
> > > > > --- a/hw/acpi/nvdimm.c
> > > > > +++ b/hw/acpi/nvdimm.c
> > > > > @@ -1234,6 +1234,10 @@ static void nvdimm_build_ssdt(GArray
> > > > > *table_offsets, GArray *table_data,
> > > > >      ssdt = init_aml_allocator();
> > > > >      acpi_data_push(ssdt->buf, sizeof(AcpiTableHeader));
> > > > >
> > > > > +    /* Storage for the memory address */
> > > > > +    mem_addr_offset = table_data->len +
> > > > > +        build_append_named_dword(ssdt->buf,
> > > > > + NVDIMM_ACPI_MEM_ADDR);
> > > > > +
> > > > >      sb_scope = aml_scope("\\_SB");
> > > > >
> > > > >      dev = aml_device("NVDR");
> > > > > @@ -1266,8 +1270,6 @@ static void nvdimm_build_ssdt(GArray
> > > > > *table_offsets, GArray *table_data,
> > > > >
> > > > >      /* copy AML table into ACPI tables blob and patch header there */
> > > > >      g_array_append_vals(table_data, ssdt->buf->data, ssdt->buf->len);
> > > > > -    mem_addr_offset = build_append_named_dword(table_data,
> > > > > -                                               NVDIMM_ACPI_MEM_ADDR);
> > > > >
> > > > >      bios_linker_loader_alloc(linker,
> > > > >                               NVDIMM_DSM_MEM_FILE,
> > > > > dsm_dma_arrea,
> > > > > --
> > > > > MST