[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH v6 32/33] nvdimm acpi: support _FIT method
|
From: |
Xiao Guangrong |
|
Subject: |
Re: [Qemu-devel] [PATCH v6 32/33] nvdimm acpi: support _FIT method |
|
Date: |
Mon, 9 Nov 2015 14:37:14 +0800 |
|
User-agent: |
Mozilla/5.0 (X11; Linux x86_64; rv:38.0) Gecko/20100101 Thunderbird/38.3.0 |
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 <address@hidden>
---
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!