[Qemu-devel] [PATCH 0/8] nvdimm acpi: bug fix and cleanup

[Qemu-devel] [PATCH 0/8] nvdimm acpi: bug fix and cleanup

[Xiao Guangrong]

Thanks to Igor's suggestion, this patchset fixes some bugs in NVDIMM ACPI,
also it refines the nvdimm code slightly 

Xiao Guangrong (8):
  acpi nvdimm: fix wrong buffer size returned by DSM method
  acpi nvdimm: fix device physical address base
  acpi nvdimm: fix OperationRegion definition
  acpi nvdimm: fix ARG3 conflict
  acpi nvdimm: fix Arg6 usage
  nvdimm acpi: compile nvdimm acpi code arch-independently
  acpi nvdimm: rename result_size to dsm_out_buf_siz
  nvdimm acpi: use common macros instead of magic names

 hw/acpi/Makefile.objs |   2 +-
 hw/acpi/nvdimm.c      | 180 ++++++++++++++++++++++++++++----------------------
 2 files changed, 101 insertions(+), 81 deletions(-)

-- 
1.8.3.1

[Qemu-devel] [PATCH 1/8] acpi nvdimm: fix wrong buffer size returned by DSM method

[Xiao Guangrong]

Currently, 'RLEN' is the totally buffer size written by QEMU and it is
ACPI internally used only. The buffer size returned to guest should
not include 'RLEN' itself

Signed-off-by: Xiao Guangrong <guangrong.xiao@linux.intel.com>
---
 hw/acpi/nvdimm.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/hw/acpi/nvdimm.c b/hw/acpi/nvdimm.c
index e486128..24a2b3b 100644
--- a/hw/acpi/nvdimm.c
+++ b/hw/acpi/nvdimm.c
@@ -862,7 +862,8 @@ static void nvdimm_build_common_dsm(Aml *dev)
     aml_append(method, aml_store(dsm_mem, aml_name("NTFI")));
 
     result_size = aml_local(1);
-    aml_append(method, aml_store(aml_name("RLEN"), result_size));
+    /* RLEN is not included in the payload returned to guest. */
+    aml_append(method, aml_subtract(aml_name("RLEN"), aml_int(4), result_size));
     aml_append(method, aml_store(aml_shiftleft(result_size, aml_int(3)),
                                  result_size));
     aml_append(method, aml_create_field(aml_name("ODAT"), aml_int(0),
-- 
1.8.3.1

[Qemu-devel] [PATCH 2/8] acpi nvdimm: fix device physical address base

[Xiao Guangrong]

According to ACPI 6.0  spec, "Memory Device Physical Address
Region Base" in memdev is defined as "This field provides the
Device Physical Address base of the region". This field should
be zero in our case

Signed-off-by: Xiao Guangrong <guangrong.xiao@linux.intel.com>
---
 hw/acpi/nvdimm.c | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/hw/acpi/nvdimm.c b/hw/acpi/nvdimm.c
index 24a2b3b..05fdf9c 100644
--- a/hw/acpi/nvdimm.c
+++ b/hw/acpi/nvdimm.c
@@ -289,8 +289,6 @@ static void
 nvdimm_build_structure_memdev(GArray *structures, DeviceState *dev)
 {
     NvdimmNfitMemDev *nfit_memdev;
-    uint64_t addr = object_property_get_int(OBJECT(dev), PC_DIMM_ADDR_PROP,
-                                            NULL);
     uint64_t size = object_property_get_int(OBJECT(dev), PC_DIMM_SIZE_PROP,
                                             NULL);
     int slot = object_property_get_int(OBJECT(dev), PC_DIMM_SLOT_PROP,
@@ -314,7 +312,8 @@ nvdimm_build_structure_memdev(GArray *structures, DeviceState *dev)
 
     /* The memory region on the device. */
     nfit_memdev->region_len = cpu_to_le64(size);
-    nfit_memdev->region_dpa = cpu_to_le64(addr);
+    /* The device address starts from 0. */
+    nfit_memdev->region_dpa = cpu_to_le64(0);
 
     /* Only one interleave for PMEM. */
     nfit_memdev->interleave_ways = cpu_to_le16(1);
-- 
1.8.3.1

[Qemu-devel] [PATCH 3/8] acpi nvdimm: fix OperationRegion definition

[Xiao Guangrong]

Based on ACPI spec:
 RegionOffset := TermArg => Integer

However, Named object is not a TermArg.

This patch moves OperationRegion to NCAL() and uses localX as
its RegionOffset

Suggested-by: Igor Mammedov <imammedo@redhat.com>
Signed-off-by: Xiao Guangrong <guangrong.xiao@linux.intel.com>
---
 hw/acpi/nvdimm.c | 122 ++++++++++++++++++++++++++++---------------------------
 1 file changed, 62 insertions(+), 60 deletions(-)

diff --git a/hw/acpi/nvdimm.c b/hw/acpi/nvdimm.c
index 05fdf9c..c2f5caa 100644
--- a/hw/acpi/nvdimm.c
+++ b/hw/acpi/nvdimm.c
@@ -780,14 +780,73 @@ static void nvdimm_build_common_dsm(Aml *dev)
 {
     Aml *method, *ifctx, *function, *handle, *uuid, *dsm_mem, *result_size;
     Aml *elsectx, *unsupport, *unpatched, *expected_uuid, *uuid_invalid;
-    Aml *pckg, *pckg_index, *pckg_buf;
+    Aml *pckg, *pckg_index, *pckg_buf, *field;
     uint8_t byte_list[1];
 
     method = aml_method(NVDIMM_COMMON_DSM, 5, AML_SERIALIZED);
     uuid = aml_arg(0);
     function = aml_arg(2);
     handle = aml_arg(4);
-    dsm_mem = aml_name(NVDIMM_ACPI_MEM_ADDR);
+    dsm_mem = aml_local(6);
+
+    aml_append(method, aml_store(aml_name(NVDIMM_ACPI_MEM_ADDR), dsm_mem));
+
+    /* map DSM memory and IO into ACPI namespace. */
+    aml_append(method, aml_operation_region("NPIO", AML_SYSTEM_IO,
+               aml_int(NVDIMM_ACPI_IO_BASE), NVDIMM_ACPI_IO_LEN));
+    aml_append(method, aml_operation_region("NRAM", AML_SYSTEM_MEMORY,
+               dsm_mem, sizeof(NvdimmDsmIn)));
+
+    /*
+     * DSM notifier:
+     * NTFI: write the address of DSM memory and notify QEMU to emulate
+     *       the access.
+     *
+     * It is the IO port so that accessing them will cause VM-exit, the
+     * control will be transferred to QEMU.
+     */
+    field = aml_field("NPIO", AML_DWORD_ACC, AML_NOLOCK, AML_PRESERVE);
+    aml_append(field, aml_named_field("NTFI",
+               sizeof(uint32_t) * BITS_PER_BYTE));
+    aml_append(method, field);
+
+    /*
+     * DSM input:
+     * HDLE: store device's handle, it's zero if the _DSM call happens
+     *       on NVDIMM Root Device.
+     * REVS: store the Arg1 of _DSM call.
+     * FUNC: store the Arg2 of _DSM call.
+     * ARG3: store the Arg3 of _DSM call.
+     *
+     * They are RAM mapping on host so that these accesses never cause
+     * VM-EXIT.
+     */
+    field = aml_field("NRAM", AML_DWORD_ACC, AML_NOLOCK, AML_PRESERVE);
+    aml_append(field, aml_named_field("HDLE",
+               sizeof(typeof_field(NvdimmDsmIn, handle)) * BITS_PER_BYTE));
+    aml_append(field, aml_named_field("REVS",
+               sizeof(typeof_field(NvdimmDsmIn, revision)) * BITS_PER_BYTE));
+    aml_append(field, aml_named_field("FUNC",
+               sizeof(typeof_field(NvdimmDsmIn, function)) * BITS_PER_BYTE));
+    aml_append(field, aml_named_field("ARG3",
+         (sizeof(NvdimmDsmIn) - offsetof(NvdimmDsmIn, arg3)) * BITS_PER_BYTE));
+    aml_append(method, field);
+
+    /*
+     * DSM output:
+     * RLEN: the size of the buffer filled by QEMU.
+     * ODAT: the buffer QEMU uses to store the result.
+     *
+     * Since the page is reused by both input and out, the input data
+     * will be lost after storing new result into ODAT so we should fetch
+     * all the input data before writing the result.
+     */
+    field = aml_field("NRAM", AML_DWORD_ACC, AML_NOLOCK, AML_PRESERVE);
+    aml_append(field, aml_named_field("RLEN",
+               sizeof(typeof_field(NvdimmDsmOut, len)) * BITS_PER_BYTE));
+    aml_append(field, aml_named_field("ODAT",
+       (sizeof(NvdimmDsmOut) - offsetof(NvdimmDsmOut, data)) * BITS_PER_BYTE));
+    aml_append(method, field);
 
     /*
      * do not support any method if DSM memory address has not been
@@ -914,7 +973,7 @@ static void nvdimm_build_ssdt(GSList *device_list, GArray *table_offsets,
                               GArray *table_data, BIOSLinker *linker,
                               GArray *dsm_dma_arrea)
 {
-    Aml *ssdt, *sb_scope, *dev, *field;
+    Aml *ssdt, *sb_scope, *dev;
     int mem_addr_offset, nvdimm_ssdt;
 
     acpi_add_table(table_offsets, table_data);
@@ -939,63 +998,6 @@ static void nvdimm_build_ssdt(GSList *device_list, GArray *table_offsets,
      */
     aml_append(dev, aml_name_decl("_HID", aml_string("ACPI0012")));
 
-    /* map DSM memory and IO into ACPI namespace. */
-    aml_append(dev, aml_operation_region("NPIO", AML_SYSTEM_IO,
-               aml_int(NVDIMM_ACPI_IO_BASE), NVDIMM_ACPI_IO_LEN));
-    aml_append(dev, aml_operation_region("NRAM", AML_SYSTEM_MEMORY,
-               aml_name(NVDIMM_ACPI_MEM_ADDR), sizeof(NvdimmDsmIn)));
-
-    /*
-     * DSM notifier:
-     * NTFI: write the address of DSM memory and notify QEMU to emulate
-     *       the access.
-     *
-     * It is the IO port so that accessing them will cause VM-exit, the
-     * control will be transferred to QEMU.
-     */
-    field = aml_field("NPIO", AML_DWORD_ACC, AML_NOLOCK, AML_PRESERVE);
-    aml_append(field, aml_named_field("NTFI",
-               sizeof(uint32_t) * BITS_PER_BYTE));
-    aml_append(dev, field);
-
-    /*
-     * DSM input:
-     * HDLE: store device's handle, it's zero if the _DSM call happens
-     *       on NVDIMM Root Device.
-     * REVS: store the Arg1 of _DSM call.
-     * FUNC: store the Arg2 of _DSM call.
-     * ARG3: store the Arg3 of _DSM call.
-     *
-     * They are RAM mapping on host so that these accesses never cause
-     * VM-EXIT.
-     */
-    field = aml_field("NRAM", AML_DWORD_ACC, AML_NOLOCK, AML_PRESERVE);
-    aml_append(field, aml_named_field("HDLE",
-               sizeof(typeof_field(NvdimmDsmIn, handle)) * BITS_PER_BYTE));
-    aml_append(field, aml_named_field("REVS",
-               sizeof(typeof_field(NvdimmDsmIn, revision)) * BITS_PER_BYTE));
-    aml_append(field, aml_named_field("FUNC",
-               sizeof(typeof_field(NvdimmDsmIn, function)) * BITS_PER_BYTE));
-    aml_append(field, aml_named_field("ARG3",
-               (sizeof(NvdimmDsmIn) - offsetof(NvdimmDsmIn, arg3)) * BITS_PER_BYTE));
-    aml_append(dev, field);
-
-    /*
-     * DSM output:
-     * RLEN: the size of the buffer filled by QEMU.
-     * ODAT: the buffer QEMU uses to store the result.
-     *
-     * Since the page is reused by both input and out, the input data
-     * will be lost after storing new result into ODAT so we should fetch
-     * all the input data before writing the result.
-     */
-    field = aml_field("NRAM", AML_DWORD_ACC, AML_NOLOCK, AML_PRESERVE);
-    aml_append(field, aml_named_field("RLEN",
-               sizeof(typeof_field(NvdimmDsmOut, len)) * BITS_PER_BYTE));
-    aml_append(field, aml_named_field("ODAT",
-               (sizeof(NvdimmDsmOut) - offsetof(NvdimmDsmOut, data)) * BITS_PER_BYTE));
-    aml_append(dev, field);
-
     nvdimm_build_common_dsm(dev);
 
     /* 0 is reserved for root device. */
-- 
1.8.3.1

[Qemu-devel] [PATCH 4/8] acpi nvdimm: fix ARG3 conflict

[Xiao Guangrong]

As ARG3 is a reserved name, we rename it to FARG

Suggested-by: Igor Mammedov <imammedo@redhat.com>
Signed-off-by: Xiao Guangrong <guangrong.xiao@linux.intel.com>
---
 hw/acpi/nvdimm.c | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

diff --git a/hw/acpi/nvdimm.c b/hw/acpi/nvdimm.c
index c2f5caa..0b8c275 100644
--- a/hw/acpi/nvdimm.c
+++ b/hw/acpi/nvdimm.c
@@ -816,7 +816,8 @@ static void nvdimm_build_common_dsm(Aml *dev)
      *       on NVDIMM Root Device.
      * REVS: store the Arg1 of _DSM call.
      * FUNC: store the Arg2 of _DSM call.
-     * ARG3: store the Arg3 of _DSM call.
+     * FARG: store the Arg3 of _DSM call which is a Package containing
+     *       function-specific arguments.
      *
      * They are RAM mapping on host so that these accesses never cause
      * VM-EXIT.
@@ -828,7 +829,7 @@ static void nvdimm_build_common_dsm(Aml *dev)
                sizeof(typeof_field(NvdimmDsmIn, revision)) * BITS_PER_BYTE));
     aml_append(field, aml_named_field("FUNC",
                sizeof(typeof_field(NvdimmDsmIn, function)) * BITS_PER_BYTE));
-    aml_append(field, aml_named_field("ARG3",
+    aml_append(field, aml_named_field("FARG",
          (sizeof(NvdimmDsmIn) - offsetof(NvdimmDsmIn, arg3)) * BITS_PER_BYTE));
     aml_append(method, field);
 
@@ -910,7 +911,7 @@ static void nvdimm_build_common_dsm(Aml *dev)
     pckg_buf = aml_local(3);
     aml_append(ifctx, aml_store(aml_index(pckg, aml_int(0)), pckg_index));
     aml_append(ifctx, aml_store(aml_derefof(pckg_index), pckg_buf));
-    aml_append(ifctx, aml_store(pckg_buf, aml_name("ARG3")));
+    aml_append(ifctx, aml_store(pckg_buf, aml_name("FARG")));
     aml_append(method, ifctx);
 
     /*
-- 
1.8.3.1

[Qemu-devel] [PATCH 5/8] acpi nvdimm: fix Arg6 usage

[Xiao Guangrong]

As the function only has 5 args, we use local7 instead of it

Suggested-by: Igor Mammedov <imammedo@redhat.com>
Signed-off-by: Xiao Guangrong <guangrong.xiao@linux.intel.com>
---
 hw/acpi/nvdimm.c | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

diff --git a/hw/acpi/nvdimm.c b/hw/acpi/nvdimm.c
index 0b8c275..cc958a4 100644
--- a/hw/acpi/nvdimm.c
+++ b/hw/acpi/nvdimm.c
@@ -780,7 +780,7 @@ static void nvdimm_build_common_dsm(Aml *dev)
 {
     Aml *method, *ifctx, *function, *handle, *uuid, *dsm_mem, *result_size;
     Aml *elsectx, *unsupport, *unpatched, *expected_uuid, *uuid_invalid;
-    Aml *pckg, *pckg_index, *pckg_buf, *field;
+    Aml *pckg, *pckg_index, *pckg_buf, *field, *dsm_out_buf;
     uint8_t byte_list[1];
 
     method = aml_method(NVDIMM_COMMON_DSM, 5, AML_SERIALIZED);
@@ -788,6 +788,7 @@ static void nvdimm_build_common_dsm(Aml *dev)
     function = aml_arg(2);
     handle = aml_arg(4);
     dsm_mem = aml_local(6);
+    dsm_out_buf = aml_local(7);
 
     aml_append(method, aml_store(aml_name(NVDIMM_ACPI_MEM_ADDR), dsm_mem));
 
@@ -928,8 +929,8 @@ static void nvdimm_build_common_dsm(Aml *dev)
     aml_append(method, aml_create_field(aml_name("ODAT"), aml_int(0),
                                         result_size, "OBUF"));
     aml_append(method, aml_concatenate(aml_buffer(0, NULL), aml_name("OBUF"),
-                                       aml_arg(6)));
-    aml_append(method, aml_return(aml_arg(6)));
+                                       dsm_out_buf));
+    aml_append(method, aml_return(dsm_out_buf));
     aml_append(dev, method);
 }
 
-- 
1.8.3.1

[Qemu-devel] [PATCH 6/8] nvdimm acpi: compile nvdimm acpi code arch-independently

[Xiao Guangrong]

As the arch dependent info, TARGET_PAGE_SIZE, has been dropped
from nvdimm acpi code, it can be compiled arch-independently

Signed-off-by: Xiao Guangrong <guangrong.xiao@linux.intel.com>
---
 hw/acpi/Makefile.objs | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/hw/acpi/Makefile.objs b/hw/acpi/Makefile.objs
index 4b7da66..489e63b 100644
--- a/hw/acpi/Makefile.objs
+++ b/hw/acpi/Makefile.objs
@@ -3,7 +3,7 @@ common-obj-$(CONFIG_ACPI_X86_ICH) += ich9.o tco.o
 common-obj-$(CONFIG_ACPI_CPU_HOTPLUG) += cpu_hotplug.o
 common-obj-$(CONFIG_ACPI_MEMORY_HOTPLUG) += memory_hotplug.o memory_hotplug_acpi_table.o
 common-obj-$(CONFIG_ACPI_CPU_HOTPLUG) += cpu.o
-obj-$(CONFIG_ACPI_NVDIMM) += nvdimm.o
+common-obj-$(CONFIG_ACPI_NVDIMM) += nvdimm.o
 common-obj-$(CONFIG_ACPI) += acpi_interface.o
 common-obj-$(CONFIG_ACPI) += bios-linker-loader.o
 common-obj-$(CONFIG_ACPI) += aml-build.o
-- 
1.8.3.1

[Qemu-devel] [PATCH 7/8] acpi nvdimm: rename result_size to dsm_out_buf_siz

[Xiao Guangrong]

Rename it as dsm_out_buf_siz is more descriptive

Suggested-by: Igor Mammedov <imammedo@redhat.com>
Signed-off-by: Xiao Guangrong <guangrong.xiao@linux.intel.com>
---
 hw/acpi/nvdimm.c | 15 ++++++++-------
 1 file changed, 8 insertions(+), 7 deletions(-)

diff --git a/hw/acpi/nvdimm.c b/hw/acpi/nvdimm.c
index cc958a4..12126d1 100644
--- a/hw/acpi/nvdimm.c
+++ b/hw/acpi/nvdimm.c
@@ -778,9 +778,9 @@ void nvdimm_init_acpi_state(AcpiNVDIMMState *state, MemoryRegion *io,
 
 static void nvdimm_build_common_dsm(Aml *dev)
 {
-    Aml *method, *ifctx, *function, *handle, *uuid, *dsm_mem, *result_size;
+    Aml *method, *ifctx, *function, *handle, *uuid, *dsm_mem;
     Aml *elsectx, *unsupport, *unpatched, *expected_uuid, *uuid_invalid;
-    Aml *pckg, *pckg_index, *pckg_buf, *field, *dsm_out_buf;
+    Aml *pckg, *pckg_index, *pckg_buf, *field, *dsm_out_buf, *dsm_out_buf_size;
     uint8_t byte_list[1];
 
     method = aml_method(NVDIMM_COMMON_DSM, 5, AML_SERIALIZED);
@@ -921,13 +921,14 @@ static void nvdimm_build_common_dsm(Aml *dev)
      */
     aml_append(method, aml_store(dsm_mem, aml_name("NTFI")));
 
-    result_size = aml_local(1);
+    dsm_out_buf_size = aml_local(1);
     /* RLEN is not included in the payload returned to guest. */
-    aml_append(method, aml_subtract(aml_name("RLEN"), aml_int(4), result_size));
-    aml_append(method, aml_store(aml_shiftleft(result_size, aml_int(3)),
-                                 result_size));
+    aml_append(method, aml_subtract(aml_name("RLEN"), aml_int(4),
+                                    dsm_out_buf_size));
+    aml_append(method, aml_store(aml_shiftleft(dsm_out_buf_size, aml_int(3)),
+                                 dsm_out_buf_size));
     aml_append(method, aml_create_field(aml_name("ODAT"), aml_int(0),
-                                        result_size, "OBUF"));
+                                        dsm_out_buf_size, "OBUF"));
     aml_append(method, aml_concatenate(aml_buffer(0, NULL), aml_name("OBUF"),
                                        dsm_out_buf));
     aml_append(method, aml_return(dsm_out_buf));
-- 
1.8.3.1

[Qemu-devel] [PATCH 8/8] nvdimm acpi: use common macros instead of magic names

[Xiao Guangrong]

There are some names repeatedly used in acpi code, define them
as macros to refine the code

Suggested-by: Igor Mammedov <imammedo@redhat.com>
Signed-off-by: Xiao Guangrong <guangrong.xiao@linux.intel.com>
---
 hw/acpi/nvdimm.c | 83 +++++++++++++++++++++++++++++++++-----------------------
 1 file changed, 49 insertions(+), 34 deletions(-)

diff --git a/hw/acpi/nvdimm.c b/hw/acpi/nvdimm.c
index 12126d1..bb896c9 100644
--- a/hw/acpi/nvdimm.c
+++ b/hw/acpi/nvdimm.c
@@ -773,8 +773,20 @@ void nvdimm_init_acpi_state(AcpiNVDIMMState *state, MemoryRegion *io,
                     state->dsm_mem->len);
 }
 
-#define NVDIMM_COMMON_DSM      "NCAL"
-#define NVDIMM_ACPI_MEM_ADDR   "MEMA"
+#define NVDIMM_COMMON_DSM       "NCAL"
+#define NVDIMM_ACPI_MEM_ADDR    "MEMA"
+
+#define NVDIMM_DSM_MEMORY       "NRAM"
+#define NVDIMM_DSM_IOPORT       "NPIO"
+
+#define NVDIMM_DSM_NOTIFY       "NTFI"
+#define NVDIMM_DSM_HANDLE       "HDLE"
+#define NVDIMM_DSM_REVISION     "REVS"
+#define NVDIMM_DSM_FUNCTION     "FUNC"
+#define NVDIMM_DSM_ARG3         "FARG"
+
+#define NVDIMM_DSM_OUT_BUF_SIZE "RLEN"
+#define NVDIMM_DSM_OUT_BUF      "ODAT"
 
 static void nvdimm_build_common_dsm(Aml *dev)
 {
@@ -793,60 +805,63 @@ static void nvdimm_build_common_dsm(Aml *dev)
     aml_append(method, aml_store(aml_name(NVDIMM_ACPI_MEM_ADDR), dsm_mem));
 
     /* map DSM memory and IO into ACPI namespace. */
-    aml_append(method, aml_operation_region("NPIO", AML_SYSTEM_IO,
+    aml_append(method, aml_operation_region(NVDIMM_DSM_IOPORT, AML_SYSTEM_IO,
                aml_int(NVDIMM_ACPI_IO_BASE), NVDIMM_ACPI_IO_LEN));
-    aml_append(method, aml_operation_region("NRAM", AML_SYSTEM_MEMORY,
-               dsm_mem, sizeof(NvdimmDsmIn)));
+    aml_append(method, aml_operation_region(NVDIMM_DSM_MEMORY,
+               AML_SYSTEM_MEMORY, dsm_mem, sizeof(NvdimmDsmIn)));
 
     /*
      * DSM notifier:
-     * NTFI: write the address of DSM memory and notify QEMU to emulate
-     *       the access.
+     * NVDIMM_DSM_NOTIFY: write the address of DSM memory and notify QEMU to
+     *                    emulate the access.
      *
      * It is the IO port so that accessing them will cause VM-exit, the
      * control will be transferred to QEMU.
      */
-    field = aml_field("NPIO", AML_DWORD_ACC, AML_NOLOCK, AML_PRESERVE);
-    aml_append(field, aml_named_field("NTFI",
+    field = aml_field(NVDIMM_DSM_IOPORT, AML_DWORD_ACC, AML_NOLOCK,
+                      AML_PRESERVE);
+    aml_append(field, aml_named_field(NVDIMM_DSM_NOTIFY,
                sizeof(uint32_t) * BITS_PER_BYTE));
     aml_append(method, field);
 
     /*
      * DSM input:
-     * HDLE: store device's handle, it's zero if the _DSM call happens
-     *       on NVDIMM Root Device.
-     * REVS: store the Arg1 of _DSM call.
-     * FUNC: store the Arg2 of _DSM call.
-     * FARG: store the Arg3 of _DSM call which is a Package containing
-     *       function-specific arguments.
+     * NVDIMM_DSM_HANDLE: store device's handle, it's zero if the _DSM call
+     *                    happens on NVDIMM Root Device.
+     * NVDIMM_DSM_REVISION: store the Arg1 of _DSM call.
+     * NVDIMM_DSM_FUNCTION: store the Arg2 of _DSM call.
+     * NVDIMM_DSM_ARG3: store the Arg3 of _DSM call which is a Package
+     *                  containing function-specific arguments.
      *
      * They are RAM mapping on host so that these accesses never cause
      * VM-EXIT.
      */
-    field = aml_field("NRAM", AML_DWORD_ACC, AML_NOLOCK, AML_PRESERVE);
-    aml_append(field, aml_named_field("HDLE",
+    field = aml_field(NVDIMM_DSM_MEMORY, AML_DWORD_ACC, AML_NOLOCK,
+                      AML_PRESERVE);
+    aml_append(field, aml_named_field(NVDIMM_DSM_HANDLE,
                sizeof(typeof_field(NvdimmDsmIn, handle)) * BITS_PER_BYTE));
-    aml_append(field, aml_named_field("REVS",
+    aml_append(field, aml_named_field(NVDIMM_DSM_REVISION,
                sizeof(typeof_field(NvdimmDsmIn, revision)) * BITS_PER_BYTE));
-    aml_append(field, aml_named_field("FUNC",
+    aml_append(field, aml_named_field(NVDIMM_DSM_FUNCTION,
                sizeof(typeof_field(NvdimmDsmIn, function)) * BITS_PER_BYTE));
-    aml_append(field, aml_named_field("FARG",
+    aml_append(field, aml_named_field(NVDIMM_DSM_ARG3,
          (sizeof(NvdimmDsmIn) - offsetof(NvdimmDsmIn, arg3)) * BITS_PER_BYTE));
     aml_append(method, field);
 
     /*
      * DSM output:
-     * RLEN: the size of the buffer filled by QEMU.
-     * ODAT: the buffer QEMU uses to store the result.
+     * NVDIMM_DSM_OUT_BUF_SIZE: the size of the buffer filled by QEMU.
+     * NVDIMM_DSM_OUT_BUF: the buffer QEMU uses to store the result.
      *
      * Since the page is reused by both input and out, the input data
      * will be lost after storing new result into ODAT so we should fetch
      * all the input data before writing the result.
      */
-    field = aml_field("NRAM", AML_DWORD_ACC, AML_NOLOCK, AML_PRESERVE);
-    aml_append(field, aml_named_field("RLEN",
+    field = aml_field(NVDIMM_DSM_MEMORY, AML_DWORD_ACC, AML_NOLOCK,
+                      AML_PRESERVE);
+    aml_append(field, aml_named_field(NVDIMM_DSM_OUT_BUF_SIZE,
                sizeof(typeof_field(NvdimmDsmOut, len)) * BITS_PER_BYTE));
-    aml_append(field, aml_named_field("ODAT",
+    aml_append(field, aml_named_field(NVDIMM_DSM_OUT_BUF,
        (sizeof(NvdimmDsmOut) - offsetof(NvdimmDsmOut, data)) * BITS_PER_BYTE));
     aml_append(method, field);
 
@@ -892,9 +907,9 @@ static void nvdimm_build_common_dsm(Aml *dev)
      * it reserves 0 for root device and is the handle for NVDIMM devices.
      * See the comments in nvdimm_slot_to_handle().
      */
-    aml_append(method, aml_store(handle, aml_name("HDLE")));
-    aml_append(method, aml_store(aml_arg(1), aml_name("REVS")));
-    aml_append(method, aml_store(aml_arg(2), aml_name("FUNC")));
+    aml_append(method, aml_store(handle, aml_name(NVDIMM_DSM_HANDLE)));
+    aml_append(method, aml_store(aml_arg(1), aml_name(NVDIMM_DSM_REVISION)));
+    aml_append(method, aml_store(aml_arg(2), aml_name(NVDIMM_DSM_FUNCTION)));
 
     /*
      * The fourth parameter (Arg3) of _DSM is a package which contains
@@ -912,23 +927,23 @@ static void nvdimm_build_common_dsm(Aml *dev)
     pckg_buf = aml_local(3);
     aml_append(ifctx, aml_store(aml_index(pckg, aml_int(0)), pckg_index));
     aml_append(ifctx, aml_store(aml_derefof(pckg_index), pckg_buf));
-    aml_append(ifctx, aml_store(pckg_buf, aml_name("FARG")));
+    aml_append(ifctx, aml_store(pckg_buf, aml_name(NVDIMM_DSM_ARG3)));
     aml_append(method, ifctx);
 
     /*
      * tell QEMU about the real address of DSM memory, then QEMU
      * gets the control and fills the result in DSM memory.
      */
-    aml_append(method, aml_store(dsm_mem, aml_name("NTFI")));
+    aml_append(method, aml_store(dsm_mem, aml_name(NVDIMM_DSM_NOTIFY)));
 
     dsm_out_buf_size = aml_local(1);
     /* RLEN is not included in the payload returned to guest. */
-    aml_append(method, aml_subtract(aml_name("RLEN"), aml_int(4),
-                                    dsm_out_buf_size));
+    aml_append(method, aml_subtract(aml_name(NVDIMM_DSM_OUT_BUF_SIZE),
+               aml_int(4), dsm_out_buf_size));
     aml_append(method, aml_store(aml_shiftleft(dsm_out_buf_size, aml_int(3)),
                                  dsm_out_buf_size));
-    aml_append(method, aml_create_field(aml_name("ODAT"), aml_int(0),
-                                        dsm_out_buf_size, "OBUF"));
+    aml_append(method, aml_create_field(aml_name(NVDIMM_DSM_OUT_BUF),
+               aml_int(0), dsm_out_buf_size, "OBUF"));
     aml_append(method, aml_concatenate(aml_buffer(0, NULL), aml_name("OBUF"),
                                        dsm_out_buf));
     aml_append(method, aml_return(dsm_out_buf));
-- 
1.8.3.1

Re: [Qemu-devel] [PATCH 8/8] nvdimm acpi: use common macros instead of magic names

[Stefan Hajnoczi]

[-- Attachment #1: Type: text/plain, Size: 1589 bytes --]

On Sat, Oct 29, 2016 at 12:11:56AM +0800, Xiao Guangrong wrote:
>      /*
>       * DSM notifier:
> -     * NTFI: write the address of DSM memory and notify QEMU to emulate
> -     *       the access.
> +     * NVDIMM_DSM_NOTIFY: write the address of DSM memory and notify QEMU to
> +     *                    emulate the access.

Did you mean to search-replace this instance of "NTFI"?

>      /*
>       * DSM input:
> -     * HDLE: store device's handle, it's zero if the _DSM call happens
> -     *       on NVDIMM Root Device.
> -     * REVS: store the Arg1 of _DSM call.
> -     * FUNC: store the Arg2 of _DSM call.
> -     * FARG: store the Arg3 of _DSM call which is a Package containing
> -     *       function-specific arguments.
> +     * NVDIMM_DSM_HANDLE: store device's handle, it's zero if the _DSM call
> +     *                    happens on NVDIMM Root Device.
> +     * NVDIMM_DSM_REVISION: store the Arg1 of _DSM call.
> +     * NVDIMM_DSM_FUNCTION: store the Arg2 of _DSM call.
> +     * NVDIMM_DSM_ARG3: store the Arg3 of _DSM call which is a Package
> +     *                  containing function-specific arguments.

Did you mean to search-replace these names?  I think it makes sense to
use the literal ACPI names instead of the macro names in documentation.

>      /*
>       * DSM output:
> -     * RLEN: the size of the buffer filled by QEMU.
> -     * ODAT: the buffer QEMU uses to store the result.
> +     * NVDIMM_DSM_OUT_BUF_SIZE: the size of the buffer filled by QEMU.
> +     * NVDIMM_DSM_OUT_BUF: the buffer QEMU uses to store the result.

Same here.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 455 bytes --]

Re: [Qemu-devel] [PATCH 0/8] nvdimm acpi: bug fix and cleanup

[Stefan Hajnoczi]

[-- Attachment #1: Type: text/plain, Size: 898 bytes --]

On Sat, Oct 29, 2016 at 12:11:48AM +0800, Xiao Guangrong wrote:
> Thanks to Igor's suggestion, this patchset fixes some bugs in NVDIMM ACPI,
> also it refines the nvdimm code slightly 
> 
> Xiao Guangrong (8):
>   acpi nvdimm: fix wrong buffer size returned by DSM method
>   acpi nvdimm: fix device physical address base
>   acpi nvdimm: fix OperationRegion definition
>   acpi nvdimm: fix ARG3 conflict
>   acpi nvdimm: fix Arg6 usage
>   nvdimm acpi: compile nvdimm acpi code arch-independently
>   acpi nvdimm: rename result_size to dsm_out_buf_siz
>   nvdimm acpi: use common macros instead of magic names
> 
>  hw/acpi/Makefile.objs |   2 +-
>  hw/acpi/nvdimm.c      | 180 ++++++++++++++++++++++++++++----------------------
>  2 files changed, 101 insertions(+), 81 deletions(-)

Besides my comments on the last patch:

Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 455 bytes --]