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

["Michael S. Tsirkin" <mst@redhat.com>]

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 <erik.schmauss@intel.com>; Igor Mammedov
> > <imammedo@redhat.com>; Xiao Guangrong <xiaoguangrong.eric@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.schmauss@intel.com>
> > Cc: qemu-stable@nongnu.org
> > Signed-off-by: Michael S. Tsirkin <mst@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