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

[Schmauss, Erik]

> -----Original Message-----
> From: Michael S. Tsirkin [mailto:address@hidden
> Sent: Monday, April 23, 2018 4:03 PM
> To: address@hidden
> Cc: Schmauss, Erik <address@hidden>; Igor Mammedov
> <address@hidden>; Xiao Guangrong <address@hidden>
> 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" <address@hidden>
> Cc: address@hidden
> Signed-off-by: Michael S. Tsirkin <address@hidden>
> ---
> 
> 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