From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:38299) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1fAmM9-0002p8-4U for qemu-devel@nongnu.org; Mon, 23 Apr 2018 21:02:54 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1fAmM6-0007q5-21 for qemu-devel@nongnu.org; Mon, 23 Apr 2018 21:02:53 -0400 Received: from mx3-rdu2.redhat.com ([66.187.233.73]:35824 helo=mx1.redhat.com) by eggs.gnu.org with esmtps (TLS1.0:DHE_RSA_AES_256_CBC_SHA1:32) (Exim 4.71) (envelope-from ) id 1fAmM5-0007pv-S8 for qemu-devel@nongnu.org; Mon, 23 Apr 2018 21:02:49 -0400 Date: Tue, 24 Apr 2018 04:02:40 +0300 From: "Michael S. Tsirkin" Message-ID: <20180424035504-mutt-send-email-mst@kernel.org> References: <1524524398-41342-1-git-send-email-mst@redhat.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: Subject: Re: [Qemu-devel] [PATCH] acpi/nvdimm: remove forward name references List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: "Schmauss, Erik" Cc: "qemu-devel@nongnu.org" , Igor Mammedov , Xiao Guangrong , "Williams, Dan J" 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 ; Igor Mammedov > > ; Xiao Guangrong > > 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" > > Cc: qemu-stable@nongnu.org > > Signed-off-by: Michael S. Tsirkin > > --- > > > > 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