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 <mst@xxxxxxxxxx>
> 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@xxxxxxxxxx>
> Signed-off-by: Xiao Guangrong <guangrong.xiao@xxxxxxxxxxxxxxx>
> ---
> 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
>
--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
