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

[Michael S. Tsirkin]

On Wed, Apr 25, 2018 at 03:49:38PM +0200, Igor Mammedov wrote:
> On Tue, 24 Apr 2018 21:06:37 +0300
> "Michael S. Tsirkin" <address@hidden> wrote:
> 
> > 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.
> Strange, I've rechecked AML definition of TermArg in 6.0 and 6.2a
> and it still says only
> 
>   TermArg := Type2Opcode | DataObject | ArgObj | LocalObj
> 
> For 6.2a, I'm looking at chapter "20.2.5 Term Objects Encoding"
> where exactly do you guys see that longer variant?

Oh interesting. That's in
19.2.3 ASL Root and Secondary Terms

How come it's not the same?

> 
> > 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.
> I think some windows versions weren't happy with it when
> we were trying to use dynamic operation regions with offset
> as namestring.


Good to know, thanks.

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