On Tue, 24 Apr 2018 04:02:40 +0300 "Michael S. Tsirkin" <m...@redhat.com> wrote:
> 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. 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
