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

[Michael S. Tsirkin]

On Tue, Mar 01, 2016 at 04:53:23PM +0800, Xiao Guangrong wrote:
> 
> 
> 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?
> 

OK but I can't review this patch on top of patch.
Please split this in aml-build and nvdimm changes,
then squash the am-build change with my patch and include it
as 5/8, then append yours squashed with the nvdimm.c changes.


> >From 29a6803d244bbec807bd1df08aff4483ea776c9b Mon Sep 17 00:00:00 2001
> From: Michael S. Tsirkin <address@hidden>
> 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 <address@hidden>
> Signed-off-by: Xiao Guangrong <address@hidden>
> ---
>  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;


Rename it something that implies what it does, not it's value. Offset of
what is it?

For example
        nvdimm_ssdt = 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);

Here too, please give it a better name
        mem_addr_offset = ....; ?

>  
>      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
>