On Mon, Oct 19, 2015 at 12:39:24PM +0800, Xiao Guangrong wrote:
>
>
> 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. */:
If it's the same spec, you don't have to repeat it:
/* Encode function according to DSM Spec rev 1.0 */
> switch (function) {
> case 4 /* 4.4.1 Get Namespace Label Size Get Namespace Label Size. */:
same for chapter etc.
> 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.
> */
Fix up the english here pls:
> * The DSM memory has two uses: OSPM saves its input parameter there, QEMU
> * uses it to save 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());
No, the point is to make sure getpagesize is big enough to hold
the structure.
> >
> >>+
> >>+ /*
> >>+ * 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
s/As/The/
s/that evil/an evil/
> * its content while we are doing DSM emulation. Avoid it by copying DSM memory
s/Avoid it/Avoid this/
> * 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
