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

[Michael S. Tsirkin]

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.

> 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