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

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

[Schmauss, Erik]

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

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


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 */);

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

[Michael S. Tsirkin]

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

Interesting. What is the result if such a table is encountered?
Will this break on old hypervisors that already
shipped with this set of tables?

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

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

> 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

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

[Dan Williams]

On Mon, Apr 23, 2018 at 1:35 PM, Schmauss, Erik <erik.schmauss@intel.com> 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).
>
> 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.
> 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... 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...
>
>
> 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 */);

I gave this a shot and it appears to breaking some assumption of where
this device is mapped relative to System RAM:

 ioremap on RAM at 0x00000000bffe0000 - 0x000000019ffe1fff
 WARNING: CPU: 0 PID: 0 at arch/x86/mm/ioremap.c:166
__ioremap_caller+0x28b/0x300
 Modules linked in:
 CPU: 0 PID: 0 Comm: swapper/0 Not tainted 4.17.0-rc1+ #1734
 Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS
rel-1.11.1-0-g0551a4be2c-prebuilt.qemu-project.org 04/01/2014
 RIP: 0010:__ioremap_caller+0x28b/0x300
 RSP: 0000:ffffffff82603db0 EFLAGS: 00010286
 RAX: 0000000000000000 RBX: 00000000bffe0000 RCX: 0000000000000006
 RDX: 0000000000000168 RSI: ffffffff82618f90 RDI: 0000000000000246
 RBP: 00000000e0002000 R08: 0000000000000000 R09: 0000000000000000
 R10: ffff88043e7d8000 R11: 0000000000000000 R12: 0000000000000000
 R13: 00000000bffe0000 R14: ffffffff81a731e9 R15: ffffffff82603ee4
 FS:  0000000000000000(0000) GS:ffff880431400000(0000) knlGS:0000000000000000
 CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
 CR2: 00000000ffffffff CR3: 0000000002610000 CR4: 00000000000406b0
 Call Trace:
  ? acpi_os_map_iomem+0x7b/0x1c0
  acpi_os_map_iomem+0x189/0x1c0
  acpi_tb_acquire_table+0x39/0x64
  acpi_tb_validate_table+0x21/0x33
  acpi_tb_verify_temp_table+0x37/0x213
  acpi_reallocate_root_table+0xe1/0x112
  acpi_early_init+0x4b/0x102
  start_kernel+0x419/0x4ee
  secondary_startup_64+0xa5/0xb0

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

[Michael S. Tsirkin]

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).
> 
> 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. 
> 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... 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...
> 
> 
> 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");

Hmm I suspect this won't work because you are appending to
table_data, but the ssdt header isn't there yet - it's
in ssdt at this point.

Look at hw/acpi/vmgenid.c for how to do it right:

    vgia_offset = table_data->len +
        build_append_named_dword(ssdt->buf, "VGIA");

ugly, but works.


>  
>      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 */);

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

[Michael S. Tsirkin]

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).
> 
> Interesting. What is the result if such a table is encountered?
> Will this break on old hypervisors that already
> shipped with this set of tables?
> 
> > 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 ...
> 
> > 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.

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

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

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

[Schmauss, Erik]

> -----Original Message-----
> From: Michael S. Tsirkin [mailto:mst@redhat.com]
> Sent: Monday, April 23, 2018 4:05 PM
> To: Schmauss, Erik <erik.schmauss@intel.com>
> Cc: imammedo@redhat.com; qemu-devel@nongnu.org; Williams, Dan J
> <dan.j.williams@intel.com>; He, Junyan <junyan.he@intel.com>; Moore, Robert
> <robert.moore@intel.com>
> 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