From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:38623) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1ZvgB1-000882-1A for qemu-devel@nongnu.org; Mon, 09 Nov 2015 01:43:40 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1ZvgAx-0003ZA-NJ for qemu-devel@nongnu.org; Mon, 09 Nov 2015 01:43:38 -0500 Received: from mga14.intel.com ([192.55.52.115]:29728) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1ZvgAx-0003Z5-Ah for qemu-devel@nongnu.org; Mon, 09 Nov 2015 01:43:35 -0500 References: <1446184587-142784-1-git-send-email-guangrong.xiao@linux.intel.com> <1446184587-142784-33-git-send-email-guangrong.xiao@linux.intel.com> <20151108175031.GA26082@redhat.com> From: Xiao Guangrong Message-ID: <56403F1A.1000305@linux.intel.com> Date: Mon, 9 Nov 2015 14:37:14 +0800 MIME-Version: 1.0 In-Reply-To: <20151108175031.GA26082@redhat.com> Content-Type: text/plain; charset=utf-8; format=flowed Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH v6 32/33] nvdimm acpi: support _FIT method List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: "Michael S. Tsirkin" Cc: ehabkost@redhat.com, kvm@vger.kernel.org, gleb@kernel.org, mtosatti@redhat.com, qemu-devel@nongnu.org, stefanha@redhat.com, imammedo@redhat.com, pbonzini@redhat.com, dan.j.williams@intel.com, rth@twiddle.net On 11/09/2015 01:50 AM, Michael S. Tsirkin wrote: > On Fri, Oct 30, 2015 at 01:56:26PM +0800, Xiao Guangrong wrote: >> FIT buffer is not completely mapped into guest address space, so a new >> function, Read FIT, function index 0xFFFFFFFF, is reserved by QEMU to >> read the piece of FIT buffer. The buffer is concatenated before _FIT >> return >> >> Refer to docs/specs/acpi-nvdimm.txt for detailed design >> >> Signed-off-by: Xiao Guangrong >> --- >> hw/acpi/nvdimm.c | 168 +++++++++++++++++++++++++++++++++++++++++++++++++++++-- >> 1 file changed, 164 insertions(+), 4 deletions(-) >> >> diff --git a/hw/acpi/nvdimm.c b/hw/acpi/nvdimm.c >> index f8d7d19..3f35220 100644 >> --- a/hw/acpi/nvdimm.c >> +++ b/hw/acpi/nvdimm.c >> @@ -384,6 +384,18 @@ static void nvdimm_build_nfit(GSList *device_list, GArray *table_offsets, >> g_array_free(structures, true); >> } >> >> +/* >> + * define UUID for NVDIMM Root Device according to Chapter 3 DSM Interface >> + * for NVDIMM Root Device - Example in DSM Spec Rev1. >> + */ >> +#define NVDIMM_DSM_ROOT_UUID "2F10E7A4-9E91-11E4-89D3-123B93F75CBA" >> + >> +/* >> + * Read FIT Function, which is a QEMU internal use only function, more detail >> + * refer to docs/specs/acpi_nvdimm.txt >> + */ >> +#define NVDIMM_DSM_FUNC_READ_FIT 0xFFFFFFFF >> + >> /* define NVDIMM DSM return status codes according to DSM Spec Rev1. */ >> enum { >> /* Common return status codes. */ >> @@ -420,6 +432,11 @@ struct NvdimmFuncInSetLabelData { >> } QEMU_PACKED; >> typedef struct NvdimmFuncInSetLabelData NvdimmFuncInSetLabelData; >> >> +struct NvdimmFuncInReadFit { >> + uint32_t offset; /* fit offset */ >> +} QEMU_PACKED; >> +typedef struct NvdimmFuncInReadFit NvdimmFuncInReadFit; >> + >> struct NvdimmDsmIn { >> uint32_t handle; >> uint32_t revision; >> @@ -429,6 +446,7 @@ struct NvdimmDsmIn { >> uint8_t arg3[0]; >> NvdimmFuncInSetLabelData func_set_label_data; >> NvdimmFuncInGetLabelData func_get_label_data; >> + NvdimmFuncInReadFit func_read_fit; >> }; >> } QEMU_PACKED; >> typedef struct NvdimmDsmIn NvdimmDsmIn; >> @@ -450,13 +468,71 @@ struct NvdimmFuncOutGetLabelData { >> } QEMU_PACKED; >> typedef struct NvdimmFuncOutGetLabelData NvdimmFuncOutGetLabelData; >> >> +struct NvdimmFuncOutReadFit { >> + uint32_t status; /* return status code. */ >> + uint32_t length; /* the length of fit data we read. */ >> + uint8_t fit_data[0]; /* fit data. */ >> +} QEMU_PACKED; >> +typedef struct NvdimmFuncOutReadFit NvdimmFuncOutReadFit; >> + >> static void nvdimm_dsm_write_status(GArray *out, uint32_t status) >> { >> status = cpu_to_le32(status); >> build_append_int_noprefix(out, status, sizeof(status)); >> } >> >> -static void nvdimm_dsm_root(NvdimmDsmIn *in, GArray *out) >> +/* Build fit memory which is presented to guest via _FIT method. */ >> +static void nvdimm_build_fit(AcpiNVDIMMState *state) >> +{ >> + if (!state->fit) { >> + GSList *device_list = nvdimm_get_plugged_device_list(); >> + >> + nvdimm_debug("Rebuild FIT...\n"); >> + state->fit = nvdimm_build_device_structure(device_list); >> + g_slist_free(device_list); >> + } >> +} >> + >> +/* Read FIT data, defined in docs/specs/acpi_nvdimm.txt. */ >> +static void nvdimm_dsm_func_read_fit(AcpiNVDIMMState *state, >> + NvdimmDsmIn *in, GArray *out) >> +{ >> + NvdimmFuncInReadFit *read_fit = &in->func_read_fit; >> + NvdimmFuncOutReadFit fit_out; >> + uint32_t read_length = TARGET_PAGE_SIZE - sizeof(NvdimmFuncOutReadFit); >> + uint32_t status = NVDIMM_DSM_ROOT_DEV_STATUS_INVALID_PARAS; >> + >> + nvdimm_build_fit(state); >> + >> + le32_to_cpus(&read_fit->offset); >> + >> + nvdimm_debug("Read FIT offset %#x.\n", read_fit->offset); >> + >> + if (read_fit->offset > state->fit->len) { >> + nvdimm_debug("offset %#x is beyond fit size (%#x).\n", >> + read_fit->offset, state->fit->len); >> + goto exit; >> + } >> + >> + read_length = MIN(read_length, state->fit->len - read_fit->offset); >> + nvdimm_debug("read length %#x.\n", read_length); >> + >> + fit_out.status = cpu_to_le32(NVDIMM_DSM_STATUS_SUCCESS); >> + fit_out.length = cpu_to_le32(read_length); > > Is array always empty at this point? > If yes, better assert this here to make sure guest can not > use unlimited memory. It's unnecessary. At the end of dsm handler, we have asserted it that the memory size can not beyond the size of dsm memory region: static uint64_t nvdimm_dsm_read(void *opaque, hwaddr addr, unsigned size) { ...... exit: assert(out->len <= memory_region_size(dsm_ram_mr)); /* Write output result to dsm memory. */ memcpy(dsm_ram_addr, out->data, out->len); memory_region_set_dirty(dsm_ram_mr, 0, out->len); buf_size = cpu_to_le32(out->len); ...... } > >> + g_array_append_vals(out, &fit_out, sizeof(fit_out)); >> + >> + if (read_length) { >> + g_array_append_vals(out, state->fit->data + read_fit->offset, >> + read_length); >> + } >> + return; >> + >> +exit: >> + nvdimm_dsm_write_status(out, status); >> +} >> + >> +static void nvdimm_dsm_root(AcpiNVDIMMState *state, NvdimmDsmIn *in, >> + GArray *out) >> { >> uint32_t status = NVDIMM_DSM_STATUS_NOT_SUPPORTED; >> >> @@ -475,6 +551,10 @@ static void nvdimm_dsm_root(NvdimmDsmIn *in, GArray *out) >> return; >> } >> >> + if (in->function == NVDIMM_DSM_FUNC_READ_FIT /* FIT Read */) { >> + return nvdimm_dsm_func_read_fit(state, in, out); >> + } >> + >> nvdimm_debug("Return status %#x.\n", status); >> nvdimm_dsm_write_status(out, status); >> } >> @@ -710,7 +790,7 @@ nvdimm_dsm_read(void *opaque, hwaddr addr, unsigned size) >> >> /* Handle 0 is reserved for NVDIMM Root Device. */ >> if (!in->handle) { >> - nvdimm_dsm_root(in, out); >> + nvdimm_dsm_root(state, in, out); >> goto exit; >> } >> >> @@ -925,8 +1005,88 @@ static void nvdimm_build_acpi_devices(GSList *device_list, Aml *sb_scope) >> */ >> BUILD_DSM_METHOD(dev, method, >> 0 /* 0 is reserved for NVDIMM Root Device*/, >> - "2F10E7A4-9E91-11E4-89D3-123B93F75CBA" >> - /* UUID for NVDIMM Root Devices. */); >> + NVDIMM_DSM_ROOT_UUID /* UUID for NVDIMM Root Devices. */); >> + >> + method = aml_method("RFIT", 1); >> + { >> + Aml *ret, *pckg, *ifcond, *ifctx, *dsm_return = aml_local(0); >> + >> + aml_append(method, aml_create_dword_field(aml_buffer(4, NULL), >> + aml_int(0), "OFST")); > > Creating new names within method is problematic: will > lead to crashes if method is called multiple times. I did the test that let _FIT read 8 bytes per time then this method will be called multiple times, it worked well. The case can cause the problem is the while loop: | Creation of a named object more than once in a given scope is not allowed. As such, | unconditionally creating named objects within a While loop must be avoided. A fatal error will be | generated on the second iteration of the loop, during the attempt to create the same named object | a second time. [ 19.6.143 While (Conditional Loop) in ACPI 6.0 ] That is why i made it as a separate method and split it from the loop in _FIT method. > > > >> + >> + /* prepare NvdimmFuncInReadFit.offset */ >> + aml_append(method, aml_store(aml_arg(0), aml_name("OFST"))); >> + pckg = aml_package(1); >> + aml_append(pckg, aml_name("OFST")); >> + >> + ret = aml_call4("_DSM", >> + aml_touuid(NVDIMM_DSM_ROOT_UUID) /* Root Device UUID */, >> + aml_int(1) /* Revision 1 */, >> + aml_int(NVDIMM_DSM_FUNC_READ_FIT) /* Read FIT >> + Function Index */, >> + pckg); >> + aml_append(method, aml_store(ret, dsm_return)); >> + >> + aml_append(method, aml_create_dword_field(dsm_return, >> + aml_int(0) /* offset at byte 0 */, >> + "STAU")); >> + /* if something is wrong during _DSM. */ >> + ifcond = aml_equal(aml_int(NVDIMM_DSM_STATUS_SUCCESS), >> + aml_name("STAU")); >> + ifctx = aml_if(aml_lnot(ifcond)); >> + { >> + aml_append(ifctx, aml_return(aml_buffer(0, NULL))); >> + } >> + aml_append(method, ifctx); >> + >> + aml_append(method, aml_create_dword_field(dsm_return, >> + aml_int(4) /* offset at byte 4. */, >> + "BFSZ")); >> + /* if we read the end of fit. */ >> + ifctx = aml_if(aml_equal(aml_name("BFSZ"), aml_int(0))); >> + { >> + aml_append(ifctx, aml_return(aml_buffer(0, NULL))); >> + } >> + aml_append(method, ifctx); >> + >> + aml_append(method, aml_store(aml_shiftleft(aml_name("BFSZ"), >> + aml_int(3)), aml_local(6))); >> + aml_append(method, aml_create_field(dsm_return, >> + aml_int(8 * BITS_PER_BYTE), /* offset at byte 8.*/ >> + aml_local(6), "BUFF")); > > Same here. > > Additionally, using named constants for things like BFSZ etc > will help name sure they match. Okay, will change it to: aml_append(method, aml_store(aml_shiftleft(aml_name("BFSZ"), aml_int(3)), aml_name("BFSZ"))); aml_append(method, aml_create_field(dsm_return, aml_int(8 * BITS_PER_BYTE), /* offset at byte 8.*/ aml_name("BFSZ"), "BUFF")); By the way, Michael, what's your idea on this thread: http://marc.info/?l=kvm&m=144671750722962&w=2 Thanks!