From patchwork Tue Mar 1 08:42:43 2016 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Xiao Guangrong X-Patchwork-Id: 8463111 Return-Path: X-Original-To: patchwork-kvm@patchwork.kernel.org Delivered-To: patchwork-parsemail@patchwork2.web.kernel.org Received: from mail.kernel.org (mail.kernel.org [198.145.29.136]) by patchwork2.web.kernel.org (Postfix) with ESMTP id 0782AC0553 for ; Tue, 1 Mar 2016 08:43:27 +0000 (UTC) Received: from mail.kernel.org (localhost [127.0.0.1]) by mail.kernel.org (Postfix) with ESMTP id 113052027D for ; Tue, 1 Mar 2016 08:43:26 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [209.132.180.67]) by mail.kernel.org (Postfix) with ESMTP id EA65520265 for ; Tue, 1 Mar 2016 08:43:24 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752587AbcCAInF (ORCPT ); Tue, 1 Mar 2016 03:43:05 -0500 Received: from mga11.intel.com ([192.55.52.93]:39649 "EHLO mga11.intel.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751983AbcCAInA (ORCPT ); Tue, 1 Mar 2016 03:43:00 -0500 Received: from fmsmga002.fm.intel.com ([10.253.24.26]) by fmsmga102.fm.intel.com with ESMTP; 01 Mar 2016 00:42:59 -0800 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.22,522,1449561600"; d="scan'208,223";a="926905654" Received: from xiao.sh.intel.com ([10.239.159.134]) by fmsmga002.fm.intel.com with ESMTP; 01 Mar 2016 00:42:54 -0800 Subject: Re: [PATCH v3 5/8] nvdimm acpi: introduce patched dsm memory To: "Michael S. Tsirkin" References: <1455439865-75784-1-git-send-email-guangrong.xiao@linux.intel.com> <1455439865-75784-6-git-send-email-guangrong.xiao@linux.intel.com> <20160229112956-mutt-send-email-mst@redhat.com> Cc: pbonzini@redhat.com, imammedo@redhat.com, gleb@kernel.org, mtosatti@redhat.com, stefanha@redhat.com, rth@twiddle.net, ehabkost@redhat.com, dan.j.williams@intel.com, kvm@vger.kernel.org, qemu-devel@nongnu.org From: Xiao Guangrong Message-ID: <56D55603.2020403@linux.intel.com> Date: Tue, 1 Mar 2016 16:42:43 +0800 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:38.0) Gecko/20100101 Thunderbird/38.5.1 MIME-Version: 1.0 In-Reply-To: <20160229112956-mutt-send-email-mst@redhat.com> Sender: kvm-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: kvm@vger.kernel.org X-Spam-Status: No, score=-6.9 required=5.0 tests=BAYES_00, RCVD_IN_DNSWL_HI, RP_MATCHES_RCVD, T_TVD_MIME_EPI, UNPARSEABLE_RELAY autolearn=unavailable version=3.3.1 X-Spam-Checker-Version: SpamAssassin 3.3.1 (2010-03-16) on mail.kernel.org X-Virus-Scanned: ClamAV using ClamSMTP On 02/29/2016 05:38 PM, Michael S. Tsirkin wrote: > On Sun, Feb 14, 2016 at 04:51:02PM +0800, Xiao Guangrong wrote: >> The dsm memory is used to save the input parameters and store >> the dsm result which is filled by QEMU. >> >> The address of dsm memory is decided by bios and patched into >> int32 object named "MEMA" >> >> Signed-off-by: Xiao Guangrong > > > This is a bit too hacky for my taste. First, I would prefer an explicit API > to add a DWORD. Second, I would like to avoid offset math hacks > and make API returning offsets. > > Pls see below for a suggestion. > > >> --- >> hw/acpi/nvdimm.c | 34 +++++++++++++++++++++++++++++++++- >> 1 file changed, 33 insertions(+), 1 deletion(-) >> >> diff --git a/hw/acpi/nvdimm.c b/hw/acpi/nvdimm.c >> index 8568b20..bca36ae 100644 >> --- a/hw/acpi/nvdimm.c >> +++ b/hw/acpi/nvdimm.c >> @@ -29,6 +29,7 @@ >> #include "qemu/osdep.h" >> #include "hw/acpi/acpi.h" >> #include "hw/acpi/aml-build.h" >> +#include "hw/acpi/bios-linker-loader.h" >> #include "hw/nvram/fw_cfg.h" >> #include "hw/mem/nvdimm.h" >> >> @@ -406,6 +407,7 @@ void nvdimm_init_acpi_state(AcpiNVDIMMState *state, MemoryRegion *io, >> } >> >> #define NVDIMM_COMMON_DSM "NCAL" >> +#define NVDIMM_ACPI_MEM_ADDR "MEMA" >> >> static void nvdimm_build_common_dsm(Aml *dev) >> { >> @@ -470,7 +472,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; >> + Aml *ssdt, *sb_scope, *dev, *mem_addr; >> + uint32_t zero_offset = 0; >> + int offset; >> >> acpi_add_table(table_offsets, table_data); >> >> @@ -501,9 +505,37 @@ static void nvdimm_build_ssdt(GSList *device_list, GArray *table_offsets, >> >> aml_append(sb_scope, dev); >> >> + /* >> + * 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)); >> + >> + 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); >> + >> + bios_linker_loader_alloc(linker, NVDIMM_DSM_MEM_FILE, TARGET_PAGE_SIZE, >> + false /* high memory */); >> + bios_linker_loader_add_pointer(linker, ACPI_BUILD_TABLE_FILE, >> + NVDIMM_DSM_MEM_FILE, table_data, >> + 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"); >> -- >> 1.8.3.1 > > > > ---> > > 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. > > Signed-off-by: Michael S. Tsirkin > > --- > > diff --git a/include/hw/acpi/aml-build.h b/include/hw/acpi/aml-build.h > index 1b632dc..f8998ea 100644 > --- a/include/hw/acpi/aml-build.h > +++ b/include/hw/acpi/aml-build.h > @@ -286,4 +286,7 @@ void acpi_build_tables_cleanup(AcpiBuildTables *tables, bool mfre); > void > build_rsdt(GArray *table_data, GArray *linker, GArray *table_offsets); > > +int > +build_append_named_dword(GArray *array, const char *name_format, ...); > + > #endif > diff --git a/hw/acpi/aml-build.c b/hw/acpi/aml-build.c > index 0d4b324..7f9fa65 100644 > --- a/hw/acpi/aml-build.c > +++ b/hw/acpi/aml-build.c > @@ -262,6 +262,32 @@ static void build_append_int(GArray *table, uint64_t value) > } > } > > +/* 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? From 29a6803d244bbec807bd1df08aff4483ea776c9b Mon Sep 17 00:00:00 2001 From: Michael S. Tsirkin 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 Signed-off-by: Xiao Guangrong --- 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