| Message ID | 56D55883.3090901@linux.intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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@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; 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@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 03/01/2016 05:08 PM, Michael S. Tsirkin wrote: > 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. Okay... will do. > Rename it something that implies what it does, not it's value. Offset of > what is it? > > For example > nvdimm_ssdt = table_data->len; Yep, good to me. > > > >> >> - 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 = ....; ? Yup, it is better. -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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