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

[Igor Mammedov <imammedo@redhat.com>]

On Tue, 24 Apr 2018 04:02:40 +0300
"Michael S. Tsirkin" <mst@redhat.com> wrote:

> On Tue, Apr 24, 2018 at 12:41:29AM +0000, Schmauss, Erik wrote:
> > 
> >   
> > > -----Original Message-----
> > > From: Michael S. Tsirkin [mailto:mst@redhat.com]
> > > Sent: Monday, April 23, 2018 4:03 PM
> > > To: qemu-devel@nongnu.org
> > > Cc: Schmauss, Erik <erik.schmauss@intel.com>; Igor Mammedov
> > > <imammedo@redhat.com>; Xiao Guangrong <xiaoguangrong.eric@gmail.com>
> > > 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" <erik.schmauss@intel.com>
> > > Cc: qemu-stable@nongnu.org
> > > Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> > > ---
> > > 
> > > 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, we could have played games with
references and Type2Opcode but that didn't work nice with Windows
ACPI parser. Hence local object.

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.

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