On Tue, Apr 24, 2018 at 12:41:29AM +0000, Schmauss, Erik wrote: > > > > -----Original Message----- > > From: Michael S. Tsirkin [mailto:m...@redhat.com] > > Sent: Monday, April 23, 2018 4:03 PM > > To: qemu-devel@nongnu.org > > Cc: Schmauss, Erik <erik.schma...@intel.com>; Igor Mammedov > > <imamm...@redhat.com>; Xiao Guangrong <xiaoguangrong.e...@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.schma...@intel.com> > > Cc: qemu-sta...@nongnu.org > > Signed-off-by: Michael S. Tsirkin <m...@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