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

2018-04-23 Thread Schmauss, Erik 

> -Original Message-
> From: Michael S. Tsirkin [mailto:m...@redhat.com]
> Sent: Monday, April 23, 2018 4:05 PM
> To: Schmauss, Erik 
> Cc: imamm...@redhat.com; qemu-devel@nongnu.org; Williams, Dan J
> ; He, Junyan ; Moore, Robert
> 
> 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 +, 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

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

2018-04-23 Thread 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 +, 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

2018-04-23 Thread Michael S. Tsirkin 
On Mon, Apr 23, 2018 at 08:35:45PM +, 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

2018-04-23 Thread Dan Williams 
On Mon, Apr 23, 2018 at 1:35 PM, 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");
>
>  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 0xbffe - 0x00019ffe1fff
 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: :82603db0 EFLAGS: 00010286
 RAX:  RBX: bffe RCX: 0006
 RDX: 0168 RSI: 82618f90 RDI: 0246
 RBP: e0002000 R08:  R09: 
 R10: 88043e7d8000 R11:  R12: 
 R13: bffe R14: 81a731e9 R15: 82603ee4
 FS:  () GS:88043140() knlGS:
 CS:  0010 DS:  ES:  CR0: 80050033
 CR2:  CR3: 0261 CR4: 000406b0
 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

2018-04-23 Thread Michael S. Tsirkin 
On Mon, Apr 23, 2018 at 08:35:45PM +, 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