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" <m...@redhat.com> wrote:
>
> > On Tue, Apr 24, 2018 at 05:47:58PM +0000, Schmauss, Erik wrote:
> > >
> > >
> > > > -----Original Message-----
> > > > From: Michael S. Tsirkin [mailto:m...@redhat.com]
> > > > Sent: Tuesday, April 24, 2018 10:43 AM
> > > > To: Igor Mammedov <imamm...@redhat.com>
> > > > Cc: Schmauss, Erik <erik.schma...@intel.com>; qemu-devel@nongnu.org;
> > > > Xiao
> > > > Guangrong <xiaoguangrong.e...@gmail.com>; Williams, Dan J
> > > > <dan.j.willi...@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" <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,
> > > >
> > > > 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
