Re: [Qemu-devel] RFC: altering the NVDIMM acpi table

[Schmauss, Erik]

> -----Original Message-----
> From: Michael S. Tsirkin [mailto:address@hidden
> Sent: Monday, April 23, 2018 4:05 PM
> To: Schmauss, Erik <address@hidden>
> Cc: address@hidden; address@hidden; Williams, Dan J
> <address@hidden>; He, Junyan <address@hidden>; Moore, Robert
> <address@hidden>
> Subject: Re: RFC: altering the NVDIMM acpi table
> 
> On Mon, Apr 23, 2018 at 11:57:04PM +0300, Michael S. Tsirkin wrote:
> > On Mon, Apr 23, 2018 at 08:35:45PM +0000, Schmauss, Erik wrote:
> > > Hello,
> > >
> > > I work on ACPICA and we have recently made changes to the behavior
> > > of the Linux AML interpreter to match other OS implementations.
> > > After sending the patches to upstream Linux, we have identified that
> > > hw/acpi/nvdimm.c specifies an ACPI table with a forward reference
> > > (MEMA is a forward reference that is no longer supported as of Linux
> > > 4.17-rc1).
> >
Hi Michael,

Thanks for your responses. Comments below:

> > Interesting. What is the result if such a table is encountered?

As of now, what happens is that this results in a runtime error while trying to 
create the NRAM operation region and fail to create the NRAM operation region 
and the NRAM fields. If there are any tables that cause an error during the 
initial table load, the table is not loaded. 

> > Will this break on old hypervisors that already shipped with this set
> > of tables?

Yes

> >
> > > We would like to change this file to move the declaration of Name
> > > (MEMA,...) to appear as the very first declaration in the SSDT. Below is a
> patch outlining the change that I would like to make.
> >
> > I think this will work just fine, but I would like to see a comment
> > explaining what the issue is.
> > Names aren't actually resolved until method actually runs, right?
> > For example, a name could be defined by a dynamically loaded
> > definition block ...

What happens after 4.17-rc1 is that once a acpi_load_table() is called, we will 
load the table in a single pass. Aside from packages elements, we no longer 
support forward references.

> >
> > > However, I am having a hard time getting make check to run  to
> > > completion in a reasonable amount of time. It always seems to fail
> > > on some sort of checksum test...
> >
> > Are you running this on Linux? On bare metal or within a VM?
> > Most people here test it on Linux with KVM.

I am running this on Linux. I'm fairly novice user of QEMU in general and I 
have only used the Makefile commands like make, make check, make check-clean, 
make check-qtest-x86_64

> 
> In addition, isn't https://github.com/acpica/acpica/commit/0c08790c
> trying to fix exactly this configuration?

This is the case for package elements. This is the only forward reference that 
we support. MEMA is a named object and we do not support forward referencing of 
named objects.

> 
> > > It would be great if you could let me know what you think of the
> > > change and what I can do to speed up the execution time of make
> > > check...
> >
> > You could limit to just qtest tests.
> >
> > make check-qtest
> >
> > >
> > > Thanks,
> > >
> > > Erik Schmauss
> > >
> > > diff --git a/hw/acpi/nvdimm.c b/hw/acpi/nvdimm.c index
> > > 59d6e4254c..7c9efd9ac7 100644
> > > --- a/hw/acpi/nvdimm.c
> > > +++ b/hw/acpi/nvdimm.c
> > > @@ -1234,6 +1234,9 @@ static void nvdimm_build_ssdt(GArray
> *table_offsets, GArray *table_data,
> > >      ssdt = init_aml_allocator();
> > >      acpi_data_push(ssdt->buf, sizeof(AcpiTableHeader));
> > >
> > > +    mem_addr_offset = build_append_named_dword(table_data,
> > > +
> > > + NVDIMM_ACPI_MEM_ADDR);
> > > +
> > >      sb_scope = aml_scope("\\_SB");
> > >
> > >      dev = aml_device("NVDR");
> > > @@ -1266,9 +1269,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,
> > >                               sizeof(NvdimmDsmIn), false /* high
> > > memory */);
> >
> >
> > --
> > MST