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

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

[Michael S. Tsirkin]

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 do much to fix past releases which IIUC
shipped this code since 2.6.0 about a year ago.

Lightly tested with Linux only.

 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

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

[Schmauss, Erik]

> -----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 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. I think nvdimm_build_common_dsm() needs some refining.

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

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

[Michael S. Tsirkin]

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


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

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

[Igor Mammedov]

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  

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

[Michael S. Tsirkin]

On Tue, Apr 24, 2018 at 09:57:22AM +0200, Igor Mammedov wrote:
> 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,

might be worth checking what happens with actual OSPMs.
If it does happen to work, we can try tweaking the ACPI
spec to allow this.

> we could have played games with
> references and Type2Opcode but that didn't work nice with Windows
> ACPI parser. Hence local object.

A comment in code wouldn't hurt to explain this.

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

I agree to that.

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

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

[Schmauss, Erik]

> -----Original Message-----
> From: Michael S. Tsirkin [mailto:mst@redhat.com]
> Sent: Tuesday, April 24, 2018 10:43 AM
> To: Igor Mammedov <imammedo@redhat.com>
> Cc: Schmauss, Erik <erik.schmauss@intel.com>; qemu-devel@nongnu.org; Xiao
> Guangrong <xiaoguangrong.eric@gmail.com>; Williams, Dan J
> <dan.j.williams@intel.com>
> Subject: Re: [PATCH] acpi/nvdimm: remove forward name references
> 
> On Tue, Apr 24, 2018 at 09:57:22AM +0200, Igor Mammedov wrote:
> > 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,
> 
> might be worth checking what happens with actual OSPMs.
> If it does happen to work, we can try tweaking the ACPI spec to allow this.

I'm looking at the ACPI6.2a spec on page 840 and it says
TermArg := Type2Opcode | DataObject | Argterm |LocalTerm |NameString |SymbolicExpression

In our case, MEMA is a namestring so we should be able to avoid the local6 and insert MEMA instead. Just as long as the Named object appears in the table before the object is used, the windows parser should work fine.

After applying this patch, the only change in ASL/AML that I was expecting was for MEMA to be moved to the top of the definition block and leave everything else alone.
 
We usually do not recommend the creation of objects inside of control methods due to the overhead of creating/destroying these objects after each method call. So the one problem that I can see with this is that performance could bog down if this method is called a lot. If it is not called very frequently, then it's probably not a big deal. 

> 
> > we could have played games with
> > references and Type2Opcode but that didn't work nice with Windows ACPI
> > parser. Hence local object.
> 
> A comment in code wouldn't hurt to explain this.
> 
> > 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.
> 
> I agree to that.
> 
> > >
> > >
> > > 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

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

[Michael S. Tsirkin]

On Tue, Apr 24, 2018 at 05:47:58PM +0000, Schmauss, Erik wrote:
> 
> 
> > -----Original Message-----
> > From: Michael S. Tsirkin [mailto:mst@redhat.com]
> > Sent: Tuesday, April 24, 2018 10:43 AM
> > To: Igor Mammedov <imammedo@redhat.com>
> > Cc: Schmauss, Erik <erik.schmauss@intel.com>; qemu-devel@nongnu.org; Xiao
> > Guangrong <xiaoguangrong.eric@gmail.com>; Williams, Dan J
> > <dan.j.williams@intel.com>
> > Subject: Re: [PATCH] acpi/nvdimm: remove forward name references
> > 
> > On Tue, Apr 24, 2018 at 09:57:22AM +0200, Igor Mammedov wrote:
> > > 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,
> > 
> > might be worth checking what happens with actual OSPMs.
> > If it does happen to work, we can try tweaking the ACPI spec to allow this.
> 
> I'm looking at the ACPI6.2a spec on page 840 and it says
> TermArg := Type2Opcode | DataObject | Argterm |LocalTerm |NameString |SymbolicExpression

Oh right, that's there since ACPI 6.0. As usual, the issue is that
we can't easily check what does OSPM support.
This is really something worth fixing in the spec IMHO.
We could just try and test a bunch of guest OSPMs and if
they happen to work, switch to that. Lots of work for
uncertain benefit.


> In our case, MEMA is a namestring so we should be able to avoid the local6 and insert MEMA instead. Just as long as the Named object appears in the table before the object is used, the windows parser should work fine.
> 
> After applying this patch, the only change in ASL/AML that I was expecting was for MEMA to be moved to the top of the definition block and leave everything else alone.

Right. This is what I see.

> We usually do not recommend the creation of objects inside of control methods due to the overhead of creating/destroying these objects after each method call. So the one problem that I can see with this is that performance could bog down if this method is called a lot. If it is not called very frequently, then it's probably not a big deal. 
> 
> > 
> > > we could have played games with
> > > references and Type2Opcode but that didn't work nice with Windows ACPI
> > > parser. Hence local object.
> > 
> > A comment in code wouldn't hurt to explain this.
> > 
> > > 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.
> > 
> > I agree to that.
> > 
> > > >
> > > >
> > > > 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

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

[Igor Mammedov]

On Tue, 24 Apr 2018 21:06:37 +0300
"Michael S. Tsirkin" <mst@redhat.com> wrote:

> On Tue, Apr 24, 2018 at 05:47:58PM +0000, Schmauss, Erik wrote:
> > 
> >   
> > > -----Original Message-----
> > > From: Michael S. Tsirkin [mailto:mst@redhat.com]
> > > Sent: Tuesday, April 24, 2018 10:43 AM
> > > To: Igor Mammedov <imammedo@redhat.com>
> > > Cc: Schmauss, Erik <erik.schmauss@intel.com>; qemu-devel@nongnu.org; Xiao
> > > Guangrong <xiaoguangrong.eric@gmail.com>; Williams, Dan J
> > > <dan.j.williams@intel.com>
> > > Subject: Re: [PATCH] acpi/nvdimm: remove forward name references
> > > 
> > > On Tue, Apr 24, 2018 at 09:57:22AM +0200, Igor Mammedov wrote:  
> > > > 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,  
> > > 
> > > might be worth checking what happens with actual OSPMs.
> > > If it does happen to work, we can try tweaking the ACPI spec to allow this.  
> > 
> > I'm looking at the ACPI6.2a spec on page 840 and it says
> > TermArg := Type2Opcode | DataObject | Argterm |LocalTerm |NameString |SymbolicExpression  
> 
> Oh right, that's there since ACPI 6.0.
Strange, I've rechecked AML definition of TermArg in 6.0 and 6.2a
and it still says only

  TermArg := Type2Opcode | DataObject | ArgObj | LocalObj

For 6.2a, I'm looking at chapter "20.2.5 Term Objects Encoding"
where exactly do you guys see that longer variant?


> As usual, the issue is that
> we can't easily check what does OSPM support.
> This is really something worth fixing in the spec IMHO.
> We could just try and test a bunch of guest OSPMs and if
> they happen to work, switch to that. Lots of work for
> uncertain benefit.
I think some windows versions weren't happy with it when
we were trying to use dynamic operation regions with offset
as namestring.


> > In our case, MEMA is a namestring so we should be able to avoid the local6 and insert MEMA instead. Just as long as the Named object appears in the table before the object is used, the windows parser should work fine.
> > 
> > After applying this patch, the only change in ASL/AML that I was expecting was for MEMA to be moved to the top of the definition block and leave everything else alone.  
> 
> Right. This is what I see.
> 
> > We usually do not recommend the creation of objects inside of control methods due to the overhead of creating/destroying these objects after each method call. So the one problem that I can see with this is that performance could bog down if this method is called a lot. If it is not called very frequently, then it's probably not a big deal. 
> >   
> > >   
> > > > we could have played games with
> > > > references and Type2Opcode but that didn't work nice with Windows ACPI
> > > > parser. Hence local object.  
> > > 
> > > A comment in code wouldn't hurt to explain this.
> > >   
> > > > 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.  
> > > 
> > > I agree to that.
> > >   
> > > > >
> > > > >
> > > > > 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  

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

[Igor Mammedov]

On Wed, 25 Apr 2018 15:49:38 +0200
Igor Mammedov <imammedo@redhat.com> wrote:

> On Tue, 24 Apr 2018 21:06:37 +0300
> "Michael S. Tsirkin" <mst@redhat.com> wrote:
> 
> > On Tue, Apr 24, 2018 at 05:47:58PM +0000, Schmauss, Erik wrote:  
> > > 
> > >     
> > > > -----Original Message-----
> > > > From: Michael S. Tsirkin [mailto:mst@redhat.com]
> > > > Sent: Tuesday, April 24, 2018 10:43 AM
> > > > To: Igor Mammedov <imammedo@redhat.com>
> > > > Cc: Schmauss, Erik <erik.schmauss@intel.com>; qemu-devel@nongnu.org; Xiao
> > > > Guangrong <xiaoguangrong.eric@gmail.com>; Williams, Dan J
> > > > <dan.j.williams@intel.com>
> > > > Subject: Re: [PATCH] acpi/nvdimm: remove forward name references
> > > > 
> > > > On Tue, Apr 24, 2018 at 09:57:22AM +0200, Igor Mammedov wrote:    
> > > > > 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,    
> > > > 
> > > > might be worth checking what happens with actual OSPMs.
> > > > If it does happen to work, we can try tweaking the ACPI spec to allow this.    
> > > 
> > > I'm looking at the ACPI6.2a spec on page 840 and it says
> > > TermArg := Type2Opcode | DataObject | Argterm |LocalTerm |NameString |SymbolicExpression    
> > 
> > Oh right, that's there since ACPI 6.0.  
> Strange, I've rechecked AML definition of TermArg in 6.0 and 6.2a
> and it still says only
> 
>   TermArg := Type2Opcode | DataObject | ArgObj | LocalObj
> 
> For 6.2a, I'm looking at chapter "20.2.5 Term Objects Encoding"
> where exactly do you guys see that longer variant?
Found it in "19.2.3 ASL Root and Secondary Terms",
so it looks like we have a bug in spec since definition
of TermArg differs.

  
> > As usual, the issue is that
> > we can't easily check what does OSPM support.
> > This is really something worth fixing in the spec IMHO.
> > We could just try and test a bunch of guest OSPMs and if
> > they happen to work, switch to that. Lots of work for
> > uncertain benefit.  
> I think some windows versions weren't happy with it when
> we were trying to use dynamic operation regions with offset
> as namestring.
> 
> 
> > > In our case, MEMA is a namestring so we should be able to avoid the local6 and insert MEMA instead. Just as long as the Named object appears in the table before the object is used, the windows parser should work fine.
> > > 
> > > After applying this patch, the only change in ASL/AML that I was expecting was for MEMA to be moved to the top of the definition block and leave everything else alone.    
> > 
> > Right. This is what I see.
> >   
> > > We usually do not recommend the creation of objects inside of control methods due to the overhead of creating/destroying these objects after each method call. So the one problem that I can see with this is that performance could bog down if this method is called a lot. If it is not called very frequently, then it's probably not a big deal. 
> > >     
> > > >     
> > > > > we could have played games with
> > > > > references and Type2Opcode but that didn't work nice with Windows ACPI
> > > > > parser. Hence local object.    
> > > > 
> > > > A comment in code wouldn't hurt to explain this.
> > > >     
> > > > > 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.    
> > > > 
> > > > I agree to that.
> > > >     
> > > > > >
> > > > > >
> > > > > > 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    
> 
> 

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

[Michael S. Tsirkin]

On Wed, Apr 25, 2018 at 03:49:38PM +0200, Igor Mammedov wrote:
> On Tue, 24 Apr 2018 21:06:37 +0300
> "Michael S. Tsirkin" <mst@redhat.com> wrote:
> 
> > On Tue, Apr 24, 2018 at 05:47:58PM +0000, Schmauss, Erik wrote:
> > > 
> > >   
> > > > -----Original Message-----
> > > > From: Michael S. Tsirkin [mailto:mst@redhat.com]
> > > > Sent: Tuesday, April 24, 2018 10:43 AM
> > > > To: Igor Mammedov <imammedo@redhat.com>
> > > > Cc: Schmauss, Erik <erik.schmauss@intel.com>; qemu-devel@nongnu.org; Xiao
> > > > Guangrong <xiaoguangrong.eric@gmail.com>; Williams, Dan J
> > > > <dan.j.williams@intel.com>
> > > > Subject: Re: [PATCH] acpi/nvdimm: remove forward name references
> > > > 
> > > > On Tue, Apr 24, 2018 at 09:57:22AM +0200, Igor Mammedov wrote:  
> > > > > 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,  
> > > > 
> > > > might be worth checking what happens with actual OSPMs.
> > > > If it does happen to work, we can try tweaking the ACPI spec to allow this.  
> > > 
> > > I'm looking at the ACPI6.2a spec on page 840 and it says
> > > TermArg := Type2Opcode | DataObject | Argterm |LocalTerm |NameString |SymbolicExpression  
> > 
> > Oh right, that's there since ACPI 6.0.
> Strange, I've rechecked AML definition of TermArg in 6.0 and 6.2a
> and it still says only
> 
>   TermArg := Type2Opcode | DataObject | ArgObj | LocalObj
> 
> For 6.2a, I'm looking at chapter "20.2.5 Term Objects Encoding"
> where exactly do you guys see that longer variant?

Oh interesting. That's in
19.2.3 ASL Root and Secondary Terms

How come it's not the same?

> 
> > As usual, the issue is that
> > we can't easily check what does OSPM support.
> > This is really something worth fixing in the spec IMHO.
> > We could just try and test a bunch of guest OSPMs and if
> > they happen to work, switch to that. Lots of work for
> > uncertain benefit.
> I think some windows versions weren't happy with it when
> we were trying to use dynamic operation regions with offset
> as namestring.


Good to know, thanks.

> 
> > > In our case, MEMA is a namestring so we should be able to avoid the local6 and insert MEMA instead. Just as long as the Named object appears in the table before the object is used, the windows parser should work fine.
> > > 
> > > After applying this patch, the only change in ASL/AML that I was expecting was for MEMA to be moved to the top of the definition block and leave everything else alone.  
> > 
> > Right. This is what I see.
> > 
> > > We usually do not recommend the creation of objects inside of control methods due to the overhead of creating/destroying these objects after each method call. So the one problem that I can see with this is that performance could bog down if this method is called a lot. If it is not called very frequently, then it's probably not a big deal. 
> > >   
> > > >   
> > > > > we could have played games with
> > > > > references and Type2Opcode but that didn't work nice with Windows ACPI
> > > > > parser. Hence local object.  
> > > > 
> > > > A comment in code wouldn't hurt to explain this.
> > > >   
> > > > > 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.  
> > > > 
> > > > I agree to that.
> > > >   
> > > > > >
> > > > > >
> > > > > > 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  

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

[Igor Mammedov]

On Wed, 25 Apr 2018 17:17:04 +0300
"Michael S. Tsirkin" <mst@redhat.com> wrote:

> On Wed, Apr 25, 2018 at 03:49:38PM +0200, Igor Mammedov wrote:
> > On Tue, 24 Apr 2018 21:06:37 +0300
> > "Michael S. Tsirkin" <mst@redhat.com> wrote:
> >   
> > > On Tue, Apr 24, 2018 at 05:47:58PM +0000, Schmauss, Erik wrote:  
> > > > 
> > > >     
> > > > > -----Original Message-----
> > > > > From: Michael S. Tsirkin [mailto:mst@redhat.com]
> > > > > Sent: Tuesday, April 24, 2018 10:43 AM
> > > > > To: Igor Mammedov <imammedo@redhat.com>
> > > > > Cc: Schmauss, Erik <erik.schmauss@intel.com>; qemu-devel@nongnu.org; Xiao
> > > > > Guangrong <xiaoguangrong.eric@gmail.com>; Williams, Dan J
> > > > > <dan.j.williams@intel.com>
> > > > > Subject: Re: [PATCH] acpi/nvdimm: remove forward name references
> > > > > 
> > > > > On Tue, Apr 24, 2018 at 09:57:22AM +0200, Igor Mammedov wrote:    
> > > > > > On Tue, 24 Apr 2018 04:02:40 +0300
> > > > > > "Michael S. Tsirkin" <mst@redhat.com> wrote:
> > > > > > 
[...]
> > > > > > From spec
> > > > > >  DefOpRegion := OpRegionOp NameString RegionSpace RegionOffset
> > > > > > RegionLen  RegionOffset := TermArg => Integer  TermArg := Type2Opcode
> > > > > > | DataObject | ArgObj | LocalObj
> > > > > >
> > > > > > So named object is not accepted,    
> > > > > 
> > > > > might be worth checking what happens with actual OSPMs.
> > > > > If it does happen to work, we can try tweaking the ACPI spec to allow this.    
> > > > 
> > > > I'm looking at the ACPI6.2a spec on page 840 and it says
> > > > TermArg := Type2Opcode | DataObject | Argterm |LocalTerm |NameString |SymbolicExpression    
> > > 
> > > Oh right, that's there since ACPI 6.0.  
> > Strange, I've rechecked AML definition of TermArg in 6.0 and 6.2a
> > and it still says only
> > 
> >   TermArg := Type2Opcode | DataObject | ArgObj | LocalObj
> > 
> > For 6.2a, I'm looking at chapter "20.2.5 Term Objects Encoding"
> > where exactly do you guys see that longer variant?  
> 
> Oh interesting. That's in
> 19.2.3 ASL Root and Secondary Terms
> 
> How come it's not the same?
bugs are everywhere, see "20.2.5 Term Objects Encoding"

> 
> >   
> > > As usual, the issue is that
> > > we can't easily check what does OSPM support.
> > > This is really something worth fixing in the spec IMHO.
> > > We could just try and test a bunch of guest OSPMs and if
> > > they happen to work, switch to that. Lots of work for
> > > uncertain benefit.  
> > I think some windows versions weren't happy with it when
> > we were trying to use dynamic operation regions with offset
> > as namestring.  
> 
> 
> Good to know, thanks.
I'll try to retest MS VMs I have here, to see which one breaks.

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

[Schmauss, Erik]

> -----Original Message-----
> From: Igor Mammedov [mailto:imammedo@redhat.com]
> Sent: Wednesday, April 25, 2018 8:32 AM
> To: Michael S. Tsirkin <mst@redhat.com>
> Cc: Schmauss, Erik <erik.schmauss@intel.com>; qemu-devel@nongnu.org; Xiao
> Guangrong <xiaoguangrong.eric@gmail.com>; Williams, Dan J
> <dan.j.williams@intel.com>
> Subject: Re: [PATCH] acpi/nvdimm: remove forward name references
> 
> On Wed, 25 Apr 2018 17:17:04 +0300
> "Michael S. Tsirkin" <mst@redhat.com> wrote:
> 
> > On Wed, Apr 25, 2018 at 03:49:38PM +0200, Igor Mammedov wrote:
> > > On Tue, 24 Apr 2018 21:06:37 +0300
> > > "Michael S. Tsirkin" <mst@redhat.com> wrote:
> > >
> > > > On Tue, Apr 24, 2018 at 05:47:58PM +0000, Schmauss, Erik wrote:
> > > > >
> > > > >
> > > > > > -----Original Message-----
> > > > > > From: Michael S. Tsirkin [mailto:mst@redhat.com]
> > > > > > Sent: Tuesday, April 24, 2018 10:43 AM
> > > > > > To: Igor Mammedov <imammedo@redhat.com>
> > > > > > Cc: Schmauss, Erik <erik.schmauss@intel.com>;
> > > > > > qemu-devel@nongnu.org; Xiao Guangrong
> > > > > > <xiaoguangrong.eric@gmail.com>; Williams, Dan J
> > > > > > <dan.j.williams@intel.com>
> > > > > > Subject: Re: [PATCH] acpi/nvdimm: remove forward name
> > > > > > references
> > > > > >
> > > > > > On Tue, Apr 24, 2018 at 09:57:22AM +0200, Igor Mammedov wrote:
> > > > > > > On Tue, 24 Apr 2018 04:02:40 +0300 "Michael S. Tsirkin"
> > > > > > > <mst@redhat.com> wrote:
> > > > > > >
> [...]
> > > > > > > From spec
> > > > > > >  DefOpRegion := OpRegionOp NameString RegionSpace
> > > > > > > RegionOffset RegionLen  RegionOffset := TermArg => Integer
> > > > > > > TermArg := Type2Opcode
> > > > > > > | DataObject | ArgObj | LocalObj
> > > > > > >
> > > > > > > So named object is not accepted,
> > > > > >
> > > > > > might be worth checking what happens with actual OSPMs.
> > > > > > If it does happen to work, we can try tweaking the ACPI spec to allow
> this.
> > > > >
> > > > > I'm looking at the ACPI6.2a spec on page 840 and it says
> > > > > TermArg := Type2Opcode | DataObject | Argterm |LocalTerm
> |NameString |SymbolicExpression
> > > >
> > > > Oh right, that's there since ACPI 6.0.
> > > Strange, I've rechecked AML definition of TermArg in 6.0 and 6.2a
> > > and it still says only
> > >
> > >   TermArg := Type2Opcode | DataObject | ArgObj | LocalObj
> > >
> > > For 6.2a, I'm looking at chapter "20.2.5 Term Objects Encoding"
> > > where exactly do you guys see that longer variant?
> >
> > Oh interesting. That's in
> > 19.2.3 ASL Root and Secondary Terms
> >
> > How come it's not the same?
> bugs are everywhere, see "20.2.5 Term Objects Encoding"
> 
> >
> > >
> > > > As usual, the issue is that
> > > > we can't easily check what does OSPM support.
> > > > This is really something worth fixing in the spec IMHO.
> > > > We could just try and test a bunch of guest OSPMs and if they
> > > > happen to work, switch to that. Lots of work for uncertain
> > > > benefit.
> > > I think some windows versions weren't happy with it when we were
> > > trying to use dynamic operation regions with offset as namestring.
> >
> >
> > Good to know, thanks.

I have made a big mistake. I was comparing the resulting SSDT of this patch to an SSDT
Generated by a much older version of kvm/qemu. The latest kvm/qemu upstream builds
the SSDT with the operation region contained inside of methods. This should resolve
MEMA of the operation region when the method is invoked. The older SSDT that I was
looking at declared the memory region outside of methods and had a forward reference
to MEMA which we no longer support.

So the latest qemu/kvm works just fine and this patch is not needed, Dan has confirmed that
the latest qemu/kvm works for his nvdimm setup.

Thanks for all of the help and I'm sorry for any confusion that I might have caused!

> I'll try to retest MS VMs I have here, to see which one breaks.
> 

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

[Dan Williams]

>> > Good to know, thanks.
>
> I have made a big mistake. I was comparing the resulting SSDT of this patch to an SSDT
> Generated by a much older version of kvm/qemu. The latest kvm/qemu upstream builds
> the SSDT with the operation region contained inside of methods. This should resolve
> MEMA of the operation region when the method is invoked. The older SSDT that I was
> looking at declared the memory region outside of methods and had a forward reference
> to MEMA which we no longer support.
>
> So the latest qemu/kvm works just fine and this patch is not needed, Dan has confirmed that
> the latest qemu/kvm works for his nvdimm setup.
>
> Thanks for all of the help and I'm sorry for any confusion that I might have caused!

Yes, my bad as well. I saw the regression on v2.8.0, but updating to
latest qemu-mainline also fixes the problem for me.

Thanks for the debug.