* Re: [Qemu-devel] RFC: altering the NVDIMM acpi table
2018-04-23 20:35 [Qemu-devel] RFC: altering the NVDIMM acpi table Schmauss, Erik
@ 2018-04-23 20:57 ` Michael S. Tsirkin
2018-04-23 23:05 ` Michael S. Tsirkin
2018-04-23 21:00 ` Dan Williams
2018-04-23 21:11 ` Michael S. Tsirkin
2 siblings, 1 reply; 6+ messages in thread
From: Michael S. Tsirkin @ 2018-04-23 20:57 UTC (permalink / raw)
To: Schmauss, Erik
Cc: imammedo@redhat.com, qemu-devel@nongnu.org, Williams, Dan J,
He, Junyan, Moore, Robert
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
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [Qemu-devel] RFC: altering the NVDIMM acpi table
2018-04-23 20:57 ` Michael S. Tsirkin
@ 2018-04-23 23:05 ` Michael S. Tsirkin
2018-04-24 0:28 ` Schmauss, Erik
0 siblings, 1 reply; 6+ messages in thread
From: Michael S. Tsirkin @ 2018-04-23 23:05 UTC (permalink / raw)
To: Schmauss, Erik
Cc: imammedo@redhat.com, qemu-devel@nongnu.org, Williams, Dan J,
He, Junyan, Moore, Robert
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
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [Qemu-devel] RFC: altering the NVDIMM acpi table
2018-04-23 23:05 ` Michael S. Tsirkin
@ 2018-04-24 0:28 ` Schmauss, Erik
0 siblings, 0 replies; 6+ messages in thread
From: Schmauss, Erik @ 2018-04-24 0:28 UTC (permalink / raw)
To: Michael S. Tsirkin
Cc: imammedo@redhat.com, qemu-devel@nongnu.org, Williams, Dan J,
He, Junyan, Moore, Robert
> -----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
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [Qemu-devel] RFC: altering the NVDIMM acpi table
2018-04-23 20:35 [Qemu-devel] RFC: altering the NVDIMM acpi table Schmauss, Erik
2018-04-23 20:57 ` Michael S. Tsirkin
@ 2018-04-23 21:00 ` Dan Williams
2018-04-23 21:11 ` Michael S. Tsirkin
2 siblings, 0 replies; 6+ messages in thread
From: Dan Williams @ 2018-04-23 21:00 UTC (permalink / raw)
To: Schmauss, Erik
Cc: mst@redhat.com, imammedo@redhat.com, qemu-devel@nongnu.org,
He, Junyan, Moore, Robert
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
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [Qemu-devel] RFC: altering the NVDIMM acpi table
2018-04-23 20:35 [Qemu-devel] RFC: altering the NVDIMM acpi table Schmauss, Erik
2018-04-23 20:57 ` Michael S. Tsirkin
2018-04-23 21:00 ` Dan Williams
@ 2018-04-23 21:11 ` Michael S. Tsirkin
2 siblings, 0 replies; 6+ messages in thread
From: Michael S. Tsirkin @ 2018-04-23 21:11 UTC (permalink / raw)
To: Schmauss, Erik
Cc: imammedo@redhat.com, qemu-devel@nongnu.org, Williams, Dan J,
He, Junyan, Moore, Robert
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 */);
^ permalink raw reply [flat|nested] 6+ messages in thread