On 10/19/2015 02:05 AM, Michael S. Tsirkin wrote:
On Mon, Oct 19, 2015 at 08:54:14AM +0800, Xiao Guangrong wrote:
__DSM is defined in ACPI 6.0: 9.14.1 _DSM (Device Specific Method)
Function 0 is a query function. We do not support any function on root
device and only 3 functions are support for NVDIMM device,
DSM_DEV_FUN_NAMESPACE_LABEL_SIZE, DSM_DEV_FUN_GET_NAMESPACE_LABEL_DATA and
DSM_DEV_FUN_SET_NAMESPACE_LABEL_DATA, that means we currently only allow to
access device's Label Namespace
Signed-off-by: Xiao Guangrong <guangrong.xiao@xxxxxxxxxxxxxxx>
---
hw/acpi/nvdimm.c | 184 ++++++++++++++++++++++++++++++++++++++++++++++++++++++-
1 file changed, 182 insertions(+), 2 deletions(-)
diff --git a/hw/acpi/nvdimm.c b/hw/acpi/nvdimm.c
index b211b8b..37fea1c 100644
--- a/hw/acpi/nvdimm.c
+++ b/hw/acpi/nvdimm.c
@@ -260,6 +260,22 @@ static uint32_t nvdimm_slot_to_dcr_index(int slot)
return nvdimm_slot_to_spa_index(slot) + 1;
}
+static NVDIMMDevice
+*nvdimm_get_device_by_handle(GSList *list, uint32_t handle)
+{
+ for (; list; list = list->next) {
+ NVDIMMDevice *nvdimm = list->data;
+ int slot = object_property_get_int(OBJECT(nvdimm), DIMM_SLOT_PROP,
+ NULL);
+
+ if (nvdimm_slot_to_handle(slot) == handle) {
+ return nvdimm;
+ }
+ }
+
+ return NULL;
+}
+
/*
* Please refer to ACPI 6.0: 5.2.25.1 System Physical Address Range
* Structure
@@ -411,6 +427,60 @@ static void nvdimm_build_nfit(GArray *structures, GArray *table_offsets,
/* detailed _DSM design please refer to docs/specs/acpi_nvdimm.txt */
#define NOTIFY_VALUE 0x99
Again, please prefix everything consistently.
Okay, will do. Sorry for i missed it.
+enum {
+ DSM_FUN_IMPLEMENTED = 0,
+
+ /* NVDIMM Root Device Functions */
+ DSM_ROOT_DEV_FUN_ARS_CAP = 1,
+ DSM_ROOT_DEV_FUN_ARS_START = 2,
+ DSM_ROOT_DEV_FUN_ARS_QUERY = 3,
+
+ /* NVDIMM Device (non-root) Functions */
+ DSM_DEV_FUN_SMART = 1,
+ DSM_DEV_FUN_SMART_THRESHOLD = 2,
+ DSM_DEV_FUN_BLOCK_NVDIMM_FLAGS = 3,
+ DSM_DEV_FUN_NAMESPACE_LABEL_SIZE = 4,
+ DSM_DEV_FUN_GET_NAMESPACE_LABEL_DATA = 5,
+ DSM_DEV_FUN_SET_NAMESPACE_LABEL_DATA = 6,
+ DSM_DEV_FUN_VENDOR_EFFECT_LOG_SIZE = 7,
+ DSM_DEV_FUN_GET_VENDOR_EFFECT_LOG = 8,
+ DSM_DEV_FUN_VENDOR_SPECIFIC = 9,
+};
Does FUN stand for "function"? FUNC or FN is probably better.
Yes.
Please list exact names as they appear in spec so
they can be searched for.
The spec reference was at where this _FUN_ is used, eg:
/*
* Please refer to DSM specification 4.4.1 Get Namespace Label Size
* (Function Index 4).
*
* It gets the size of Namespace Label data area and the max data size
* that Get/Set Namespace Label Data functions can transfer.
*/
static void nvdimm_dsm_func_label_size(NVDIMMDevice *nvdimm, GArray *out)
I will follow your ‘single use’ comments below, these definitions will
be dropped, the code will be like this:
switch (function) {
case 4 /* DSM Spec 4.4.1 Get Namespace Label Size Get Namespace Label Size. */:
nvdimm_dsm_func_label_size();
case ...
...
};
+
+enum {
+ /* Common return status codes. */
+ DSM_STATUS_SUCCESS = 0, /* Success */
+ DSM_STATUS_NOT_SUPPORTED = 1, /* Not Supported */
+
+ /* NVDIMM Root Device _DSM function return status codes*/
+ DSM_ROOT_DEV_STATUS_INVALID_PARAS = 2, /* Invalid Input Parameters */
+ DSM_ROOT_DEV_STATUS_FUNCTION_SPECIFIC_ERROR = 3, /* Function-Specific
+ Error */
+
+ /* NVDIMM Device (non-root) _DSM function return status codes*/
+ DSM_DEV_STATUS_NON_EXISTING_MEM_DEV = 2, /* Non-Existing Memory Device */
+ DSM_DEV_STATUS_INVALID_PARAS = 3, /* Invalid Input Parameters */
+ DSM_DEV_STATUS_VENDOR_SPECIFIC_ERROR = 4, /* Vendor Specific Error */
+};
+
+/* Current revision supported by DSM specification is 1. */
+#define DSM_REVISION (1)
+
+/*
+ * please refer to ACPI 6.0: 9.14.1 _DSM (Device Specific Method): Return
+ * Value Information:
Drop "please refer to".
Okay.
+ * if set to zero, no functions are supported (other than function zero)
+ * for the specified UUID and Revision ID. If set to one, at least one
+ * additional function is supported.
+ */
+
+/* do not support any function on root. */
+#define ROOT_SUPPORT_FUN (0ULL)
Needs a name that implies the comment somehow.
+#define DIMM_SUPPORT_FUN ((1 << DSM_FUN_IMPLEMENTED) \
+ | (1 << DSM_DEV_FUN_NAMESPACE_LABEL_SIZE) \
+ | (1 << DSM_DEV_FUN_GET_NAMESPACE_LABEL_DATA) \
+ | (1 << DSM_DEV_FUN_SET_NAMESPACE_LABEL_DATA))
+
I think it's best to just drop these macros.
There's a single point of use - just add a comment there
explaining what does it mean.
Okay. Good to me.
You will be able to drop all _FUN_ macros too.
Yes, it's good for code reduction.
struct dsm_in {
uint32_t handle;
uint32_t revision;
@@ -420,6 +490,11 @@ struct dsm_in {
} QEMU_PACKED;
typedef struct dsm_in dsm_in;
+struct cmd_out_implemented {
+ uint64_t cmd_list;
+};
+typedef struct cmd_out_implemented cmd_out_implemented;
+
struct dsm_out {
/* the size of buffer filled by QEMU. */
uint32_t len;
@@ -434,12 +509,115 @@ nvdimm_dsm_read(void *opaque, hwaddr addr, unsigned size)
return 0;
}
+static void nvdimm_dsm_write_status(GArray *out, uint32_t status)
+{
+ /* status locates in the first 4 bytes in the dsm memory. */
located?
Yes...
+ assert(!out->len);
But dsm itself can be part of a bigger table.
So don't do it.
Okay, will drop it.
+
+ status = cpu_to_le32(status);
+ g_array_append_vals(out, &status, sizeof(status));
I think this should just use the (unfortunately named)
build_append_int_noprefix. Same applied everywhere
where you add single values.
Okay, will use it instead.
+}
+
+static void nvdimm_dsm_write_root(dsm_in *in, GArray *out)
+{
+ uint32_t status = DSM_STATUS_NOT_SUPPORTED;
+
+ /* please refer to ACPI 6.0: 9.14.1 _DSM (Device Specific Method) */
+ if (in->function == DSM_FUN_IMPLEMENTED) {
+ uint64_t cmd_list = cpu_to_le64(ROOT_SUPPORT_FUN);
see about about single use values.
Yes, it is good to me, will follow it.
+
+ g_array_append_vals(out, &cmd_list, sizeof(cmd_list));
+ return;
+ }
+
+ nvdimm_debug("Return status %#x.\n", status);
+ nvdimm_dsm_write_status(out, status);
+}
+
+static void nvdimm_dsm_write_nvdimm(dsm_in *in, GArray *out)
+{
+ GSList *list = nvdimm_get_plugged_device_list();
+ NVDIMMDevice *nvdimm = nvdimm_get_device_by_handle(list, in->handle);
+ uint32_t status = DSM_DEV_STATUS_NON_EXISTING_MEM_DEV;
+ uint64_t cmd_list;
+
+ if (!nvdimm) {
+ goto set_status_free;
+ }
+
+ switch (in->function) {
+ /* please refer to ACPI 6.0: 9.14.1 _DSM (Device Specific Method) */
+ case DSM_FUN_IMPLEMENTED:
+ cmd_list = cpu_to_le64(DIMM_SUPPORT_FUN);
+ g_array_append_vals(out, &cmd_list, sizeof(cmd_list));
+ goto free;
+ default:
+ status = DSM_STATUS_NOT_SUPPORTED;
+ };
+
+set_status_free:
+ nvdimm_debug("Return status %#x.\n", status);
+ nvdimm_dsm_write_status(out, status);
+free:
+ g_slist_free(list);
+}
+
static void
nvdimm_dsm_write(void *opaque, hwaddr addr, uint64_t val, unsigned size)
{
+ NVDIMMState *state = opaque;
+ MemoryRegion *dsm_ram_mr;
+ dsm_in *in;
+ GArray *out;
+ void *dsm_ram_addr;
Why don't you give this the correct type? Will avoid need for casts.
If it's defined as "dsm_out *", that will make "copy(in, dsm_ram_addr..)"
little strange.
I will do it as your suggestion, and make a comment for the copy operation:
/*
* The DSM memory is used for both OSPM saves its input parameter and QEMU
* saves its output result.
*/
+
if (val != NOTIFY_VALUE) {
fprintf(stderr, "BUG: unexepected notify value 0x%" PRIx64, val);
}
+
+ dsm_ram_mr = memory_region_find(&state->mr, getpagesize(),
+ getpagesize()).mr;
+ dsm_ram_addr = memory_region_get_ram_ptr(dsm_ram_mr);
This needs a validity check for size.
Okay, will add this:
assert(memory_region_size(dsm_ram_mr) == getpagesize());
+
+ /*
+ * copy all input data to our local memory to avoid potential issue
+ * as the dsm memory is visible to guest.
this comment doesn't help.
pls replace "potential issue" with an explanation.
Okay, will change the comment to:
/* As DSM memory is mapped to guest address space so that evil guest can change
* its content while we are doing DSM emulation. Avoid it by copying DSM memory
* to QEMU local memory
*/
+ */
+ in = g_malloc(memory_region_size(dsm_ram_mr));
+ memcpy(in, dsm_ram_addr, memory_region_size(dsm_ram_mr));
+
+ le32_to_cpus(&in->revision);
+ le32_to_cpus(&in->function);
+ le32_to_cpus(&in->handle);
+
+ nvdimm_debug("Revision %#x Handler %#x Function %#x.\n", in->revision,
+ in->handle, in->function);
+
+ out = g_array_new(false, true /* clear */, 1);
+
+ if (in->revision != DSM_REVISION) {
+ nvdimm_debug("Revision %#x is not supported, expect %#x.\n",
+ in->revision, DSM_REVISION);
+ nvdimm_dsm_write_status(out, DSM_STATUS_NOT_SUPPORTED);
+ goto exit;
+ }
+
+ /* Handle 0 is reserved for NVDIMM Root Device. */
+ if (!in->handle) {
+ nvdimm_dsm_write_root(in, out);
+ goto exit;
+ }
+
+ nvdimm_dsm_write_nvdimm(in, out);
+
+exit:
+ /* Write our output result to dsm memory. */
+ ((dsm_out *)dsm_ram_addr)->len = out->len;
+ memcpy(((dsm_out *)dsm_ram_addr)->data, out->data, out->len);
This breaks migration as memory is not dirtied.
address_space_write is generally preferable to change memory.
Good point, will fix.
--
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
