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

[Michael S. Tsirkin]

On Tue, Apr 24, 2018 at 05:47:58PM +0000, Schmauss, Erik wrote:
> 
> 
> > -----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

Oh right, that's there since ACPI 6.0. As usual, the issue is that
we can't easily check what does OSPM support.
This is really something worth fixing in the spec IMHO.
We could just try and test a bunch of guest OSPMs and if
they happen to work, switch to that. Lots of work for
uncertain benefit.


> 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.

Right. This is what I see.

> 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