Re: [PATCH v3 5/8] nvdimm acpi: introduce patched dsm memory

[Xiao Guangrong <guangrong.xiao@linux.intel.com>]

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



On 02/29/2016 05:38 PM, Michael S. Tsirkin wrote:

> +/* Build NAME(XXXX, 0x00000000) where 0x00000000 is encoded as a dword,
> + * and return the offset to 0x00000000 for runtime patching.
> + *
> + * Warning: runtime patching is best avoided. Only use this as
> + * a replacement for DataTableRegion (for guests that don't
> + * support it).
> + */
> +int
> +build_append_named_dword(GArray *array, const char *name_format, ...)
> +{
> +    int offset;
> +    va_list ap;
> +
> +    va_start(ap, name_format);
> +    build_append_namestringv(array, name_format, ap);
> +    va_end(ap);

The NameOP was missed here...

The idea is great and i fixed and applied it on the top this patchset, the patch
is attached, would it be good to you?


[-- Attachment #2: 0001-acpi-add-build_append_named_dword-returning-an-offse.patch --]
[-- Type: text/x-patch, Size: 3865 bytes --]

>From 29a6803d244bbec807bd1df08aff4483ea776c9b Mon Sep 17 00:00:00 2001
From: Michael S. Tsirkin <mst@redhat.com>
Date: Tue, 1 Mar 2016 16:33:49 +0800
Subject: [PATCH] acpi: add build_append_named_dword, returning an offset in
 buffer

This is a very limited form of support for runtime patching -
similar in functionality to what we can do with ACPI_EXTRACT
macros in python, but implemented in C.

This is to allow ACPI code direct access to data tables -
which is exactly what DataTableRegion is there for, except
no known windows release so far implements DataTableRegion.

[ Xiao: fixed missed NameOp and applied it to NVDIMM ACPI. ]

Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
Signed-off-by: Xiao Guangrong <guangrong.xiao@linux.intel.com>
---
 hw/acpi/aml-build.c |  1 +
 hw/acpi/nvdimm.c    | 33 +++++++--------------------------
 2 files changed, 8 insertions(+), 26 deletions(-)

diff --git a/hw/acpi/aml-build.c b/hw/acpi/aml-build.c
index f40b93e..9d97ce8 100644
--- a/hw/acpi/aml-build.c
+++ b/hw/acpi/aml-build.c
@@ -271,6 +271,7 @@ build_append_named_dword(GArray *array, const char *name_format, ...)
     int offset;
     va_list ap;
 
+    build_append_byte(array, 0x08); /* NameOp */
     va_start(ap, name_format);
     build_append_namestringv(array, name_format, ap);
     va_end(ap);
diff --git a/hw/acpi/nvdimm.c b/hw/acpi/nvdimm.c
index a6fbbee..fbdff76 100644
--- a/hw/acpi/nvdimm.c
+++ b/hw/acpi/nvdimm.c
@@ -565,10 +565,9 @@ static void nvdimm_build_nvdimm_devices(GSList *device_list, Aml *root_dev)
 static void nvdimm_build_ssdt(GSList *device_list, GArray *table_offsets,
                               GArray *table_data, GArray *linker)
 {
-    Aml *ssdt, *sb_scope, *dev, *field, *mem_addr;
+    Aml *ssdt, *sb_scope, *dev, *field;
     Aml *min_addr, *max_addr, *mr32, *method, *crs;
-    uint32_t zero_offset = 0;
-    int offset;
+    int offset, table_len;
 
     acpi_add_table(table_offsets, table_data);
 
@@ -682,31 +681,13 @@ static void nvdimm_build_ssdt(GSList *device_list, GArray *table_offsets,
     nvdimm_build_nvdimm_devices(device_list, dev);
 
     aml_append(sb_scope, dev);
+    aml_append(ssdt, sb_scope);
 
-    /*
-     * leave it at the end of ssdt so that we can conveniently get the
-     * offset of int32 object which will be patched with the real address
-     * of the dsm memory by BIOS.
-     *
-     * 0x32000000 is the magic number to let aml_int() create int32 object.
-     * It will be zeroed later to make bios_linker_loader_add_pointer()
-     * happy.
-     */
-    mem_addr = aml_name_decl(NVDIMM_ACPI_MEM_ADDR, aml_int(0x32000000));
+    table_len = table_data->len;
 
-    aml_append(sb_scope, mem_addr);
-    aml_append(ssdt, sb_scope);
     /* copy AML table into ACPI tables blob and patch header there */
     g_array_append_vals(table_data, ssdt->buf->data, ssdt->buf->len);
-
-    offset = table_data->len - 4;
-
-    /*
-     * zero the last 4 bytes, i.e, it is the offset of
-     * NVDIMM_ACPI_MEM_ADDR object.
-     */
-    g_array_remove_range(table_data, offset, 4);
-    g_array_append_vals(table_data, &zero_offset, 4);
+    offset = build_append_named_dword(table_data, NVDIMM_ACPI_MEM_ADDR);
 
     bios_linker_loader_alloc(linker, NVDIMM_DSM_MEM_FILE, TARGET_PAGE_SIZE,
                              false /* high memory */);
@@ -715,8 +696,8 @@ static void nvdimm_build_ssdt(GSList *device_list, GArray *table_offsets,
                                    table_data->data + offset,
                                    sizeof(uint32_t));
     build_header(linker, table_data,
-        (void *)(table_data->data + table_data->len - ssdt->buf->len),
-        "SSDT", ssdt->buf->len, 1, NULL, "NVDIMM");
+        (void *)(table_data->data + table_len),
+        "SSDT", table_data->len - table_len, 1, NULL, "NVDIMM");
     free_aml_allocator();
 }
 
-- 
1.8.3.1