[Qemu-devel] [PATCH 00/15] NVDIMM: introduce nvdimm label support

[Qemu-devel] [PATCH 00/15] NVDIMM: introduce nvdimm label support

[Xiao Guangrong]

This patchset is against commit d4207b223eef3 (fw-cfg: support writeable
blobs) on pci branch of Michael's git tree and can be found at:
      https://github.com/xiaogr/qemu.git nvdimm-label-v1

This is the last part of vNVDIMM implementation which introduces nvdimm
label support

Currently Linux NVDIMM driver does not support namespace operation on this
kind of PMEM, apply below changes to support dynamical namespace:

@@ -798,7 +823,8 @@ static int acpi_nfit_register_dimms(struct acpi_nfit_desc *a
                        continue;
                }
 
-               if (nfit_mem->bdw && nfit_mem->memdev_pmem)
+               //if (nfit_mem->bdw && nfit_mem->memdev_pmem)
+               if (nfit_mem->memdev_pmem)
                        flags |= NDD_ALIASING;

You can append a NVDIMM device in guest and do:                       
# cd /sys/bus/nd/devices/
# cd namespace0.0/
# echo `uuidgen` > uuid
# echo `expr 1024 \* 1024 \* 128` > size
then reload nd.pmem.ko

You can see /dev/pmem0 appears

Xiao Guangrong (15):
  pc-dimm: get memory region from ->get_memory_region()
  pc-dimm: introduce realize callback
  pc-dimm: keep the state of the whole backend memory
  nvdimm: support nvdimm label
  acpi: add aml_object_type
  acpi: add aml_call5
  nvdimm acpi: set HDLE properly
  nvdimm acpi: save arg3 of _DSM method
  nvdimm acpi: check UUID
  nvdimm acpi: abstract the operations for root device and nvdimm
    devices
  nvdimm acpi: check revision
  nvdimm acpi: support Get Namespace Label Size function
  nvdimm acpi: support Get Namespace Label Data function
  nvdimm acpi: support Set Namespace Label Data function
  docs: add NVDIMM ACPI documentation

 docs/specs/acpi_nvdimm.txt  | 132 +++++++++++++++
 hw/acpi/aml-build.c         |  22 +++
 hw/acpi/nvdimm.c            | 392 ++++++++++++++++++++++++++++++++++++++++----
 hw/mem/nvdimm.c             |  95 +++++++++++
 hw/mem/pc-dimm.c            |  21 ++-
 include/hw/acpi/aml-build.h |   3 +
 include/hw/mem/nvdimm.h     |  61 ++++++-
 include/hw/mem/pc-dimm.h    |   6 +-
 8 files changed, 694 insertions(+), 38 deletions(-)
 create mode 100644 docs/specs/acpi_nvdimm.txt

-- 
1.8.3.1

[Qemu-devel] [PATCH 01/15] pc-dimm: get memory region from ->get_memory_region()

[Xiao Guangrong]

Curretly, the memory region of backed memory is all directly
mapped to guest's address space, however, it will be not true
for nvdimm device if we introduce nvdimm label which only can
be indirectly accessed by ACPI DSM method

Also it improves the comments a bit to reflect this fact

Signed-off-by: Xiao Guangrong <guangrong.xiao@linux.intel.com>
---
 hw/mem/pc-dimm.c         | 3 ++-
 include/hw/mem/pc-dimm.h | 3 ++-
 2 files changed, 4 insertions(+), 2 deletions(-)

diff --git a/hw/mem/pc-dimm.c b/hw/mem/pc-dimm.c
index 973bf20..0ba17f0 100644
--- a/hw/mem/pc-dimm.c
+++ b/hw/mem/pc-dimm.c
@@ -353,8 +353,9 @@ static void pc_dimm_get_size(Object *obj, Visitor *v, const char *name,
     int64_t value;
     MemoryRegion *mr;
     PCDIMMDevice *dimm = PC_DIMM(obj);
+    PCDIMMDeviceClass *ddc = PC_DIMM_GET_CLASS(obj);
 
-    mr = host_memory_backend_get_memory(dimm->hostmem, errp);
+    mr = ddc->get_memory_region(dimm);
     value = memory_region_size(mr);
 
     visit_type_int(v, name, &value, errp);
diff --git a/include/hw/mem/pc-dimm.h b/include/hw/mem/pc-dimm.h
index 218dfb0..827f1bc 100644
--- a/include/hw/mem/pc-dimm.h
+++ b/include/hw/mem/pc-dimm.h
@@ -60,7 +60,8 @@ typedef struct PCDIMMDevice {
 
 /**
  * PCDIMMDeviceClass:
- * @get_memory_region: returns #MemoryRegion associated with @dimm
+ * @get_memory_region: returns #MemoryRegion associated with @dimm which
+ * is directly mapped into the physical address space of guest
  */
 typedef struct PCDIMMDeviceClass {
     /* private */
-- 
1.8.3.1

[Qemu-devel] [PATCH 02/15] pc-dimm: introduce realize callback

[Xiao Guangrong]

nvdimm needs to  check if the backend memory is large enough to contain
label data and init its memory region when the device is realized, so
introduce realize callback which is called after common dimm has been
realize

Signed-off-by: Xiao Guangrong <guangrong.xiao@linux.intel.com>
---
 hw/mem/pc-dimm.c         | 5 +++++
 include/hw/mem/pc-dimm.h | 3 +++
 2 files changed, 8 insertions(+)

diff --git a/hw/mem/pc-dimm.c b/hw/mem/pc-dimm.c
index 0ba17f0..47ae823 100644
--- a/hw/mem/pc-dimm.c
+++ b/hw/mem/pc-dimm.c
@@ -399,6 +399,7 @@ static void pc_dimm_init(Object *obj)
 static void pc_dimm_realize(DeviceState *dev, Error **errp)
 {
     PCDIMMDevice *dimm = PC_DIMM(dev);
+    PCDIMMDeviceClass *ddc = PC_DIMM_GET_CLASS(dimm);
 
     if (!dimm->hostmem) {
         error_setg(errp, "'" PC_DIMM_MEMDEV_PROP "' property is not set");
@@ -411,6 +412,10 @@ static void pc_dimm_realize(DeviceState *dev, Error **errp)
                    dimm->node, nb_numa_nodes ? nb_numa_nodes : 1);
         return;
     }
+
+    if (ddc->realize) {
+        ddc->realize(dimm, errp);
+    }
 }
 
 static MemoryRegion *pc_dimm_get_memory_region(PCDIMMDevice *dimm)
diff --git a/include/hw/mem/pc-dimm.h b/include/hw/mem/pc-dimm.h
index 827f1bc..e7b7e5a 100644
--- a/include/hw/mem/pc-dimm.h
+++ b/include/hw/mem/pc-dimm.h
@@ -60,6 +60,8 @@ typedef struct PCDIMMDevice {
 
 /**
  * PCDIMMDeviceClass:
+ * @realize: called after common dimm is realized so that the dimm based
+ * devices get the chance to do specified operations.
  * @get_memory_region: returns #MemoryRegion associated with @dimm which
  * is directly mapped into the physical address space of guest
  */
@@ -68,6 +70,7 @@ typedef struct PCDIMMDeviceClass {
     DeviceClass parent_class;
 
     /* public */
+    void (*realize)(PCDIMMDevice *dimm, Error **errp);
     MemoryRegion *(*get_memory_region)(PCDIMMDevice *dimm);
 } PCDIMMDeviceClass;
 
-- 
1.8.3.1

[Qemu-devel] [PATCH 03/15] pc-dimm: keep the state of the whole backend memory

[Xiao Guangrong]

QEMU keeps the state of memory of dimm device during live migration,
however, it is not enough for nvdimm device as its memory does not
contain its label data, so that we should protect the whole backend
memory instead

Signed-off-by: Xiao Guangrong <guangrong.xiao@linux.intel.com>
---
 hw/mem/pc-dimm.c | 13 +++++++++++--
 1 file changed, 11 insertions(+), 2 deletions(-)

diff --git a/hw/mem/pc-dimm.c b/hw/mem/pc-dimm.c
index 47ae823..1b33e6e 100644
--- a/hw/mem/pc-dimm.c
+++ b/hw/mem/pc-dimm.c
@@ -104,9 +104,16 @@ void pc_dimm_memory_plug(DeviceState *dev, MemoryHotplugState *hpms,
     }
 
     memory_region_add_subregion(&hpms->mr, addr - hpms->base, mr);
-    vmstate_register_ram(mr, dev);
     numa_set_mem_node_id(addr, memory_region_size(mr), dimm->node);
 
+    /*
+     * save the state only for @mr is not enough as it does not contain
+     * the label data of NVDIMM device, so that we keep the state of
+     * whole hostmem instead.
+     */
+    vmstate_register_ram(host_memory_backend_get_memory(dimm->hostmem, errp),
+                         dev);
+
 out:
     error_propagate(errp, local_err);
 }
@@ -115,10 +122,12 @@ void pc_dimm_memory_unplug(DeviceState *dev, MemoryHotplugState *hpms,
                            MemoryRegion *mr)
 {
     PCDIMMDevice *dimm = PC_DIMM(dev);
+    MemoryRegion *hostmem;
 
+    hostmem = host_memory_backend_get_memory(dimm->hostmem, &error_abort);
     numa_unset_mem_node_id(dimm->addr, memory_region_size(mr), dimm->node);
     memory_region_del_subregion(&hpms->mr, mr);
-    vmstate_unregister_ram(mr, dev);
+    vmstate_unregister_ram(hostmem, dev);
 }
 
 static int pc_existing_dimms_capacity_internal(Object *obj, void *opaque)
-- 
1.8.3.1

[Qemu-devel] [PATCH 04/15] nvdimm: support nvdimm label

[Xiao Guangrong]

Introduce a parameter, 'reserve-label', which is false on default. If
it is set, we will reserve 128K memory which is the minimum namespace
label size required by NVDIMM Namespace Spec at the end of backend
memory as NVDIMM label area

Two callbacks, read_label_data() and write_label_data(), are used to
operate the label area

Signed-off-by: Xiao Guangrong <guangrong.xiao@linux.intel.com>
---
 hw/mem/nvdimm.c         | 95 +++++++++++++++++++++++++++++++++++++++++++++++++
 include/hw/mem/nvdimm.h | 61 ++++++++++++++++++++++++++++++-
 2 files changed, 155 insertions(+), 1 deletion(-)

diff --git a/hw/mem/nvdimm.c b/hw/mem/nvdimm.c
index 0a602f2..921e6a1 100644
--- a/hw/mem/nvdimm.c
+++ b/hw/mem/nvdimm.c
@@ -25,18 +25,113 @@
 #include "qemu/osdep.h"
 #include "hw/mem/nvdimm.h"
 
+static bool nvdimm_get_reserve_label(Object *obj, Error **errp)
+{
+    NVDIMMDevice *nvdimm = NVDIMM(obj);
+
+    return nvdimm->reserve_label;
+}
+
+static void nvdimm_set_reserve_label(Object *obj, bool value, Error **errp)
+{
+    NVDIMMDevice *nvdimm = NVDIMM(obj);
+
+    nvdimm->reserve_label = value;
+}
+
+static void nvdimm_init(Object *obj)
+{
+    object_property_add_bool(obj, "reserve-label", nvdimm_get_reserve_label,
+                             nvdimm_set_reserve_label, NULL);
+}
+
+static MemoryRegion *nvdimm_get_memory_region(PCDIMMDevice *dimm)
+{
+    NVDIMMDevice *nvdimm = NVDIMM(dimm);
+
+    return &nvdimm->nvdimm_mr;
+}
+
+static void nvdimm_realize(PCDIMMDevice *dimm, Error **errp)
+{
+    MemoryRegion *mr = host_memory_backend_get_memory(dimm->hostmem, errp);
+    NVDIMMDevice *nvdimm = NVDIMM(dimm);
+    uint64_t size = memory_region_size(mr);
+
+    nvdimm->label_size = nvdimm->reserve_label ? MIN_NAMESPACE_LABEL_SIZE : 0;
+
+    if (size <= nvdimm->label_size) {
+        HostMemoryBackend *hostmem = dimm->hostmem;
+        char *path = object_get_canonical_path_component(OBJECT(hostmem));
+
+        error_setg(errp, "the size of memdev %s (0x%" PRIx64 ") is too"
+                   "small to contain nvdimm label (0x%" PRIx64 ")",
+                   path, memory_region_size(mr), nvdimm->label_size);
+        return;
+    }
+
+    size -= nvdimm->label_size;
+    memory_region_init_alias(&nvdimm->nvdimm_mr, OBJECT(dimm),
+                             "nvdimm-memory", mr, 0, size);
+    nvdimm->nvdimm_mr.align = memory_region_get_alignment(mr);
+
+    nvdimm->label_data = memory_region_get_ram_ptr(mr) + size;
+}
+
+static void nvdimm_assert_rw_label_data(NVDIMMDevice *nvdimm, uint64_t size,
+                                        uint64_t offset)
+{
+    assert(nvdimm->reserve_label &&
+           (nvdimm->label_size >= size + offset) && (offset + size > offset));
+}
+
+static void nvdimm_read_label_data(NVDIMMDevice *nvdimm, void *buf,
+                                   uint64_t size, uint64_t offset)
+{
+    nvdimm_assert_rw_label_data(nvdimm, size, offset);
+
+    memcpy(buf, nvdimm->label_data + offset, size);
+}
+
+static void nvdimm_write_label_data(NVDIMMDevice *nvdimm, const void *buf,
+                                    uint64_t size, uint64_t offset)
+{
+    MemoryRegion *mr;
+    PCDIMMDevice *dimm = PC_DIMM(nvdimm);
+    uint64_t backend_offset;
+
+    nvdimm_assert_rw_label_data(nvdimm, size, offset);
+
+    memcpy(nvdimm->label_data + offset, buf, size);
+
+    mr = host_memory_backend_get_memory(dimm->hostmem, &error_abort);
+    backend_offset = memory_region_size(mr) - nvdimm->label_size + offset;
+    memory_region_set_dirty(mr, backend_offset, size);
+}
+
 static void nvdimm_class_init(ObjectClass *oc, void *data)
 {
     DeviceClass *dc = DEVICE_CLASS(oc);
+    PCDIMMDeviceClass *ddc = PC_DIMM_CLASS(oc);
+    NVDIMMClass *nvc = NVDIMM_CLASS(oc);
 
     /* nvdimm hotplug has not been supported yet. */
     dc->hotpluggable = false;
+
+    ddc->realize = nvdimm_realize;
+    ddc->get_memory_region = nvdimm_get_memory_region;
+
+    nvc->read_label_data = nvdimm_read_label_data;
+    nvc->write_label_data = nvdimm_write_label_data;
 }
 
 static TypeInfo nvdimm_info = {
     .name          = TYPE_NVDIMM,
     .parent        = TYPE_PC_DIMM,
+    .class_size    = sizeof(NVDIMMClass),
     .class_init    = nvdimm_class_init,
+    .instance_size = sizeof(NVDIMMDevice),
+    .instance_init = nvdimm_init,
 };
 
 static void nvdimm_register_types(void)
diff --git a/include/hw/mem/nvdimm.h b/include/hw/mem/nvdimm.h
index 517de9c..bb5c74a 100644
--- a/include/hw/mem/nvdimm.h
+++ b/include/hw/mem/nvdimm.h
@@ -33,7 +33,66 @@
         }                                                     \
     } while (0)
 
-#define TYPE_NVDIMM             "nvdimm"
+/*
+ * The minimum label data size is required by NVDIMM Namespace
+ * specification, see the chapter 2 Namespaces:
+ *   "NVDIMMs following the NVDIMM Block Mode Specification use an area
+ *    at least 128KB in size, which holds around 1000 labels."
+ */
+#define MIN_NAMESPACE_LABEL_SIZE      (128UL << 10)
+
+#define TYPE_NVDIMM      "nvdimm"
+#define NVDIMM(obj)      OBJECT_CHECK(NVDIMMDevice, (obj), TYPE_NVDIMM)
+#define NVDIMM_CLASS(oc) OBJECT_CLASS_CHECK(NVDIMMClass, (oc), TYPE_NVDIMM)
+#define NVDIMM_GET_CLASS(obj) OBJECT_GET_CLASS(NVDIMMClass, (obj), \
+                                               TYPE_NVDIMM)
+struct NVDIMMDevice {
+    /* private */
+    PCDIMMDevice parent_obj;
+
+    /* public */
+
+    /*
+     * if we need to reserve memory region for NVDIMM label at the
+     * end of backend memory?
+     */
+    bool reserve_label;
+
+    /*
+     * the size of label data in NVDIMM device which is presented to
+     * guest via __DSM "Get Namespace Label Size" function.
+     */
+    uint64_t label_size;
+
+    /*
+     * the address of label data which is read by __DSM "Get Namespace
+     * Label Data" function and written by __DSM "Set Namespace Label
+     * Data" function.
+     */
+    void *label_data;
+
+    /*
+     * it's the PMEM region in NVDIMM device, which is presented to
+     * guest via ACPI NFIT and _FIT method if NVDIMM hotplug is supported.
+     */
+    MemoryRegion nvdimm_mr;
+};
+typedef struct NVDIMMDevice NVDIMMDevice;
+
+struct NVDIMMClass {
+    /* private */
+    PCDIMMDeviceClass parent_class;
+
+    /* public */
+
+    /* read @size bytes from NVDIMM label data at @offset into @buf. */
+    void (*read_label_data)(NVDIMMDevice *nvdimm, void *buf,
+                            uint64_t size, uint64_t offset);
+    /* write @size bytes from @buf to NVDIMM label data at @offset. */
+    void (*write_label_data)(NVDIMMDevice *nvdimm, const void *buf,
+                             uint64_t size, uint64_t offset);
+};
+typedef struct NVDIMMClass NVDIMMClass;
 
 #define NVDIMM_DSM_MEM_FILE     "etc/acpi/nvdimm-mem"
 
-- 
1.8.3.1

[Qemu-devel] [PATCH 05/15] acpi: add aml_object_type

[Xiao Guangrong]

Implement ObjectType which is used by NVDIMM _DSM method in
later patch

Signed-off-by: Xiao Guangrong <guangrong.xiao@linux.intel.com>
---
 hw/acpi/aml-build.c         | 8 ++++++++
 include/hw/acpi/aml-build.h | 1 +
 2 files changed, 9 insertions(+)

diff --git a/hw/acpi/aml-build.c b/hw/acpi/aml-build.c
index ab89ca6..85c3ef7 100644
--- a/hw/acpi/aml-build.c
+++ b/hw/acpi/aml-build.c
@@ -1472,6 +1472,14 @@ Aml *aml_concatenate(Aml *source1, Aml *source2, Aml *target)
                                  target);
 }
 
+/* ACPI 1.0b: 16.2.5.4 Type 2 Opcodes Encoding: DefObjectType */
+Aml *aml_object_type(Aml *object)
+{
+    Aml *var = aml_opcode(0x8E /* ObjectTypeOp */);
+    aml_append(var, object);
+    return var;
+}
+
 void
 build_header(GArray *linker, GArray *table_data,
              AcpiTableHeader *h, const char *sig, int len, uint8_t rev,
diff --git a/include/hw/acpi/aml-build.h b/include/hw/acpi/aml-build.h
index 66f48ec..5b135cf 100644
--- a/include/hw/acpi/aml-build.h
+++ b/include/hw/acpi/aml-build.h
@@ -354,6 +354,7 @@ Aml *aml_unicode(const char *str);
 Aml *aml_derefof(Aml *arg);
 Aml *aml_sizeof(Aml *arg);
 Aml *aml_concatenate(Aml *source1, Aml *source2, Aml *target);
+Aml *aml_object_type(Aml *object);
 
 void
 build_header(GArray *linker, GArray *table_data,
-- 
1.8.3.1

[Qemu-devel] [PATCH 06/15] acpi: add aml_call5

[Xiao Guangrong]

It will be used by NVDIMM ACPI

Signed-off-by: Xiao Guangrong <guangrong.xiao@linux.intel.com>
---
 hw/acpi/aml-build.c         | 14 ++++++++++++++
 include/hw/acpi/aml-build.h |  2 ++
 2 files changed, 16 insertions(+)

diff --git a/hw/acpi/aml-build.c b/hw/acpi/aml-build.c
index 85c3ef7..1783216 100644
--- a/hw/acpi/aml-build.c
+++ b/hw/acpi/aml-build.c
@@ -657,6 +657,20 @@ Aml *aml_call4(const char *method, Aml *arg1, Aml *arg2, Aml *arg3, Aml *arg4)
     return var;
 }
 
+/* helper to call method with 5 arguments */
+Aml *aml_call5(const char *method, Aml *arg1, Aml *arg2, Aml *arg3, Aml *arg4,
+               Aml *arg5)
+{
+    Aml *var = aml_alloc();
+    build_append_namestring(var->buf, "%s", method);
+    aml_append(var, arg1);
+    aml_append(var, arg2);
+    aml_append(var, arg3);
+    aml_append(var, arg4);
+    aml_append(var, arg5);
+    return var;
+}
+
 /*
  * ACPI 5.0: 6.4.3.8.1 GPIO Connection Descriptor
  * Type 1, Large Item Name 0xC
diff --git a/include/hw/acpi/aml-build.h b/include/hw/acpi/aml-build.h
index 5b135cf..7b95d95 100644
--- a/include/hw/acpi/aml-build.h
+++ b/include/hw/acpi/aml-build.h
@@ -269,6 +269,8 @@ Aml *aml_call1(const char *method, Aml *arg1);
 Aml *aml_call2(const char *method, Aml *arg1, Aml *arg2);
 Aml *aml_call3(const char *method, Aml *arg1, Aml *arg2, Aml *arg3);
 Aml *aml_call4(const char *method, Aml *arg1, Aml *arg2, Aml *arg3, Aml *arg4);
+Aml *aml_call5(const char *method, Aml *arg1, Aml *arg2, Aml *arg3, Aml *arg4,
+               Aml *arg5);
 Aml *aml_gpio_int(AmlConsumerAndProducer con_and_pro,
                   AmlLevelAndEdge edge_level,
                   AmlActiveHighAndLow active_level, AmlShared shared,
-- 
1.8.3.1

[Qemu-devel] [PATCH 07/15] nvdimm acpi: set HDLE properly

[Xiao Guangrong]

Now we pass HDLE to Qemu properly, use 0 for root device and use the
handle for nvdimm devices

Signed-off-by: Xiao Guangrong <guangrong.xiao@linux.intel.com>
---
 hw/acpi/nvdimm.c | 22 ++++++++++++----------
 1 file changed, 12 insertions(+), 10 deletions(-)

diff --git a/hw/acpi/nvdimm.c b/hw/acpi/nvdimm.c
index 9531340..586c02a 100644
--- a/hw/acpi/nvdimm.c
+++ b/hw/acpi/nvdimm.c
@@ -488,7 +488,7 @@ static void nvdimm_build_common_dsm(Aml *dev)
     Aml *method, *ifctx, *function, *dsm_mem, *unpatched, *result_size;
     uint8_t byte_list[1];
 
-    method = aml_method(NVDIMM_COMMON_DSM, 4, AML_SERIALIZED);
+    method = aml_method(NVDIMM_COMMON_DSM, 5, AML_SERIALIZED);
     function = aml_arg(2);
     dsm_mem = aml_name(NVDIMM_ACPI_MEM_ADDR);
 
@@ -514,11 +514,10 @@ static void nvdimm_build_common_dsm(Aml *dev)
 
     /*
      * The HDLE indicates the DSM function is issued from which device,
-     * it is not used at this time as no function is supported yet.
-     * Currently we make it always be 0 for all the devices and will set
-     * the appropriate value once real function is implemented.
+     * it reserves 0 for root device and is the handle for NVDIMM devices.
+     * See the comments in nvdimm_slot_to_handle().
      */
-    aml_append(method, aml_store(aml_int(0x0), aml_name("HDLE")));
+    aml_append(method, aml_store(aml_arg(4), aml_name("HDLE")));
     aml_append(method, aml_store(aml_arg(1), aml_name("REVS")));
     aml_append(method, aml_store(aml_arg(2), aml_name("FUNC")));
 
@@ -540,13 +539,14 @@ static void nvdimm_build_common_dsm(Aml *dev)
     aml_append(dev, method);
 }
 
-static void nvdimm_build_device_dsm(Aml *dev)
+static void nvdimm_build_device_dsm(Aml *dev, uint32_t handle)
 {
     Aml *method;
 
     method = aml_method("_DSM", 4, AML_NOTSERIALIZED);
-    aml_append(method, aml_return(aml_call4(NVDIMM_COMMON_DSM, aml_arg(0),
-                                  aml_arg(1), aml_arg(2), aml_arg(3))));
+    aml_append(method, aml_return(aml_call5(NVDIMM_COMMON_DSM, aml_arg(0),
+                                  aml_arg(1), aml_arg(2), aml_arg(3),
+                                  aml_int(handle))));
     aml_append(dev, method);
 }
 
@@ -571,7 +571,7 @@ static void nvdimm_build_nvdimm_devices(GSList *device_list, Aml *root_dev)
          */
         aml_append(nvdimm_dev, aml_name_decl("_ADR", aml_int(handle)));
 
-        nvdimm_build_device_dsm(nvdimm_dev);
+        nvdimm_build_device_dsm(nvdimm_dev, handle);
         aml_append(root_dev, nvdimm_dev);
     }
 }
@@ -664,7 +664,9 @@ static void nvdimm_build_ssdt(GSList *device_list, GArray *table_offsets,
     aml_append(dev, field);
 
     nvdimm_build_common_dsm(dev);
-    nvdimm_build_device_dsm(dev);
+
+    /* 0 is reserved for root device. */
+    nvdimm_build_device_dsm(dev, 0);
 
     nvdimm_build_nvdimm_devices(device_list, dev);
 
-- 
1.8.3.1

[Qemu-devel] [PATCH 08/15] nvdimm acpi: save arg3 of _DSM method

[Xiao Guangrong]

Check if the input Arg3 is valid then store it into ARG3 if it is
needed

Signed-off-by: Xiao Guangrong <guangrong.xiao@linux.intel.com>
---
 hw/acpi/nvdimm.c | 20 ++++++++++++++++++++
 1 file changed, 20 insertions(+)

diff --git a/hw/acpi/nvdimm.c b/hw/acpi/nvdimm.c
index 586c02a..38aba3d 100644
--- a/hw/acpi/nvdimm.c
+++ b/hw/acpi/nvdimm.c
@@ -486,6 +486,7 @@ void nvdimm_init_acpi_state(AcpiNVDIMMState *state, MemoryRegion *io,
 static void nvdimm_build_common_dsm(Aml *dev)
 {
     Aml *method, *ifctx, *function, *dsm_mem, *unpatched, *result_size;
+    Aml *pckg, *pckg_index, *pckg_buf;
     uint8_t byte_list[1];
 
     method = aml_method(NVDIMM_COMMON_DSM, 5, AML_SERIALIZED);
@@ -522,6 +523,25 @@ static void nvdimm_build_common_dsm(Aml *dev)
     aml_append(method, aml_store(aml_arg(2), aml_name("FUNC")));
 
     /*
+     * The fourth parameter (Arg3) of _DSM is a package which contains
+     * a buffer, the layout of the buffer is specified by UUID (Arg0),
+     * Revision ID (Arg1) and Function Index (Arg2) which are documented
+     * in the DSM Spec.
+     */
+    pckg = aml_arg(3);
+    ifctx = aml_if(aml_and(aml_equal(aml_object_type(pckg),
+                   aml_int(4 /* Package */)) /* It is a Package? */,
+                   aml_equal(aml_sizeof(pckg), aml_int(1)) /* 1 element? */,
+                   NULL));
+
+    pckg_index = aml_local(2);
+    pckg_buf = aml_local(3);
+    aml_append(ifctx, aml_store(aml_index(pckg, aml_int(0)), pckg_index));
+    aml_append(ifctx, aml_store(aml_derefof(pckg_index), pckg_buf));
+    aml_append(ifctx, aml_store(pckg_buf, aml_name("ARG3")));
+    aml_append(method, ifctx);
+
+    /*
      * tell QEMU about the real address of DSM memory, then QEMU
      * gets the control and fills the result in DSM memory.
      */
-- 
1.8.3.1

[Qemu-devel] [PATCH 09/15] nvdimm acpi: check UUID

[Xiao Guangrong]

Check arg0 which indicates UUID to see if it is valid

Signed-off-by: Xiao Guangrong <guangrong.xiao@linux.intel.com>
---
 hw/acpi/nvdimm.c | 32 ++++++++++++++++++++++++++------
 1 file changed, 26 insertions(+), 6 deletions(-)

diff --git a/hw/acpi/nvdimm.c b/hw/acpi/nvdimm.c
index 38aba3d..4177227 100644
--- a/hw/acpi/nvdimm.c
+++ b/hw/acpi/nvdimm.c
@@ -485,19 +485,39 @@ void nvdimm_init_acpi_state(AcpiNVDIMMState *state, MemoryRegion *io,
 
 static void nvdimm_build_common_dsm(Aml *dev)
 {
-    Aml *method, *ifctx, *function, *dsm_mem, *unpatched, *result_size;
+    Aml *method, *ifctx, *function, *handle, *uuid, *dsm_mem, *result_size;
+    Aml *elsectx, *unsupport, *unpatched, *expected_uuid, *uuid_invalid;
     Aml *pckg, *pckg_index, *pckg_buf;
     uint8_t byte_list[1];
 
     method = aml_method(NVDIMM_COMMON_DSM, 5, AML_SERIALIZED);
+    uuid = aml_arg(0);
     function = aml_arg(2);
+    handle = aml_arg(4);
     dsm_mem = aml_name(NVDIMM_ACPI_MEM_ADDR);
 
     /*
      * do not support any method if DSM memory address has not been
      * patched.
      */
-    unpatched = aml_if(aml_equal(dsm_mem, aml_int(0x0)));
+    unpatched = aml_equal(dsm_mem, aml_int(0x0));
+
+    expected_uuid = aml_local(0);
+
+    ifctx = aml_if(aml_equal(handle, aml_int(0x0)));
+    aml_append(ifctx, aml_store(
+               aml_touuid("2F10E7A4-9E91-11E4-89D3-123B93F75CBA")
+               /* UUID for NVDIMM Root Device */, expected_uuid));
+    aml_append(method, ifctx);
+    elsectx = aml_else();
+    aml_append(elsectx, aml_store(
+               aml_touuid("4309AC30-0D11-11E4-9191-0800200C9A66")
+               /* UUID for NVDIMM Devices */, expected_uuid));
+    aml_append(method, elsectx);
+
+    uuid_invalid = aml_lnot(aml_equal(uuid, expected_uuid));
+
+    unsupport = aml_if(aml_or(unpatched, uuid_invalid, NULL));
 
     /*
      * function 0 is called to inquire what functions are supported by
@@ -506,19 +526,19 @@ static void nvdimm_build_common_dsm(Aml *dev)
     ifctx = aml_if(aml_equal(function, aml_int(0)));
     byte_list[0] = 0 /* No function Supported */;
     aml_append(ifctx, aml_return(aml_buffer(1, byte_list)));
-    aml_append(unpatched, ifctx);
+    aml_append(unsupport, ifctx);
 
     /* No function is supported yet. */
     byte_list[0] = 1 /* Not Supported */;
-    aml_append(unpatched, aml_return(aml_buffer(1, byte_list)));
-    aml_append(method, unpatched);
+    aml_append(unsupport, aml_return(aml_buffer(1, byte_list)));
+    aml_append(method, unsupport);
 
     /*
      * The HDLE indicates the DSM function is issued from which device,
      * it reserves 0 for root device and is the handle for NVDIMM devices.
      * See the comments in nvdimm_slot_to_handle().
      */
-    aml_append(method, aml_store(aml_arg(4), aml_name("HDLE")));
+    aml_append(method, aml_store(handle, aml_name("HDLE")));
     aml_append(method, aml_store(aml_arg(1), aml_name("REVS")));
     aml_append(method, aml_store(aml_arg(2), aml_name("FUNC")));
 
-- 
1.8.3.1

[Qemu-devel] [PATCH 10/15] nvdimm acpi: abstract the operations for root device and nvdimm devices

[Xiao Guangrong]

It separates the operations between root device and nvdimm devices in
order to introducing label functions support for nvdimm device

Signed-off-by: Xiao Guangrong <guangrong.xiao@linux.intel.com>
---
 hw/acpi/nvdimm.c | 72 ++++++++++++++++++++++++++++++++++++++++++--------------
 1 file changed, 54 insertions(+), 18 deletions(-)

diff --git a/hw/acpi/nvdimm.c b/hw/acpi/nvdimm.c
index 4177227..071f66f 100644
--- a/hw/acpi/nvdimm.c
+++ b/hw/acpi/nvdimm.c
@@ -404,6 +404,53 @@ struct NvdimmDsmFuncNoPayloadOut {
 } QEMU_PACKED;
 typedef struct NvdimmDsmFuncNoPayloadOut NvdimmDsmFuncNoPayloadOut;
 
+static void
+nvdimm_dsm_function0(uint32_t supported_func, hwaddr dsm_mem_addr)
+{
+    NvdimmDsmFunc0Out func0 = {
+        .len = cpu_to_le32(sizeof(func0)),
+        .supported_func = cpu_to_le32(supported_func),
+    };
+    cpu_physical_memory_write(dsm_mem_addr, &func0, sizeof(func0));
+}
+
+static void
+nvdimm_dsm_no_payload(uint32_t func_ret_status, hwaddr dsm_mem_addr)
+{
+    NvdimmDsmFuncNoPayloadOut out = {
+        .len = cpu_to_le32(sizeof(out)),
+        .func_ret_status = cpu_to_le32(func_ret_status),
+    };
+    cpu_physical_memory_write(dsm_mem_addr, &out, sizeof(out));
+}
+
+static void nvdimm_dsm_root(NvdimmDsmIn *in, hwaddr dsm_mem_addr)
+{
+    /*
+     * function 0 is called to inquire which functions are supported by
+     * OSPM
+     */
+    if (!in->function) {
+        return nvdimm_dsm_function0(0 /* No function supported other
+                                         than function 0 */, dsm_mem_addr);
+    }
+
+    /* No function except function 0 is supported yet. */
+    nvdimm_dsm_no_payload(1 /* Not Supported */, dsm_mem_addr);
+}
+
+static void nvdimm_dsm_device(NvdimmDsmIn *in, hwaddr dsm_mem_addr)
+{
+    /* See the comments in nvdimm_dsm_root(). */
+    if (!in->function) {
+        return nvdimm_dsm_function0(0 /* No function supported other
+                                         than function 0 */, dsm_mem_addr);
+    }
+
+    /* No function except function 0 is supported yet. */
+    nvdimm_dsm_no_payload(1 /* Not Supported */, dsm_mem_addr);
+}
+
 static uint64_t
 nvdimm_dsm_read(void *opaque, hwaddr addr, unsigned size)
 {
@@ -434,26 +481,15 @@ nvdimm_dsm_write(void *opaque, hwaddr addr, uint64_t val, unsigned size)
     nvdimm_debug("Revision %#x Handler %#x Function %#x.\n", in->revision,
                  in->handle, in->function);
 
-    /*
-     * function 0 is called to inquire which functions are supported by
-     * OSPM
-     */
-    if (in->function == 0) {
-        NvdimmDsmFunc0Out func0 = {
-            .len = cpu_to_le32(sizeof(func0)),
-             /* No function supported other than function 0 */
-            .supported_func = cpu_to_le32(0),
-        };
-        cpu_physical_memory_write(dsm_mem_addr, &func0, sizeof func0);
-    } else {
-        /* No function except function 0 is supported yet. */
-        NvdimmDsmFuncNoPayloadOut out = {
-            .len = cpu_to_le32(sizeof(out)),
-            .func_ret_status = cpu_to_le32(1)  /* Not Supported */,
-        };
-        cpu_physical_memory_write(dsm_mem_addr, &out, sizeof(out));
+     /* Handle 0 is reserved for NVDIMM Root Device. */
+    if (!in->handle) {
+        nvdimm_dsm_root(in, dsm_mem_addr);
+        goto exit;
     }
 
+    nvdimm_dsm_device(in, dsm_mem_addr);
+
+exit:
     g_free(in);
 }
 
-- 
1.8.3.1

[Qemu-devel] [PATCH 11/15] nvdimm acpi: check revision

[Xiao Guangrong]

Cuurently only revision 1 is supported

Signed-off-by: Xiao Guangrong <guangrong.xiao@linux.intel.com>
---
 hw/acpi/nvdimm.c | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/hw/acpi/nvdimm.c b/hw/acpi/nvdimm.c
index 071f66f..83d80f5 100644
--- a/hw/acpi/nvdimm.c
+++ b/hw/acpi/nvdimm.c
@@ -481,6 +481,13 @@ nvdimm_dsm_write(void *opaque, hwaddr addr, uint64_t val, unsigned size)
     nvdimm_debug("Revision %#x Handler %#x Function %#x.\n", in->revision,
                  in->handle, in->function);
 
+    if (in->revision != 0x1 /* Currently we only support DSM Spec Rev1. */) {
+        nvdimm_debug("Revision %#x is not supported, expect %#x.\n",
+                     in->revision, 0x1);
+        nvdimm_dsm_no_payload(1 /* Not Supported */, dsm_mem_addr);
+        goto exit;
+    }
+
      /* Handle 0 is reserved for NVDIMM Root Device. */
     if (!in->handle) {
         nvdimm_dsm_root(in, dsm_mem_addr);
-- 
1.8.3.1

[Qemu-devel] [PATCH 12/15] nvdimm acpi: support Get Namespace Label Size function

[Xiao Guangrong]

Function 4 is used to get Namespace label size

Signed-off-by: Xiao Guangrong <guangrong.xiao@linux.intel.com>
---
 hw/acpi/nvdimm.c | 130 +++++++++++++++++++++++++++++++++++++++++++++++++++++--
 1 file changed, 126 insertions(+), 4 deletions(-)

diff --git a/hw/acpi/nvdimm.c b/hw/acpi/nvdimm.c
index 83d80f5..4b5f672 100644
--- a/hw/acpi/nvdimm.c
+++ b/hw/acpi/nvdimm.c
@@ -216,6 +216,26 @@ 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(uint32_t handle)
+{
+    NVDIMMDevice *nvdimm = NULL;
+    GSList *list, *device_list = nvdimm_get_plugged_device_list();
+
+    for (list = device_list; list; list = list->next) {
+        NVDIMMDevice *nvd = list->data;
+        int slot = object_property_get_int(OBJECT(nvd), PC_DIMM_SLOT_PROP,
+                                           NULL);
+
+        if (nvdimm_slot_to_handle(slot) == handle) {
+            nvdimm = nvd;
+            break;
+        }
+    }
+
+    g_slist_free(device_list);
+    return nvdimm;
+}
+
 /* ACPI 6.0: 5.2.25.1 System Physical Address Range Structure */
 static void
 nvdimm_build_structure_spa(GArray *structures, DeviceState *dev)
@@ -404,6 +424,35 @@ struct NvdimmDsmFuncNoPayloadOut {
 } QEMU_PACKED;
 typedef struct NvdimmDsmFuncNoPayloadOut NvdimmDsmFuncNoPayloadOut;
 
+struct NvdimmFuncGetLabelSizeOut {
+    /* the size of buffer filled by QEMU. */
+    uint32_t len;
+    uint32_t func_ret_status; /* return status code. */
+    uint32_t label_size; /* the size of label data area. */
+    /*
+     * Maximum size of the namespace label data length supported by
+     * the platform in Get/Set Namespace Label Data functions.
+     */
+    uint32_t max_xfer;
+} QEMU_PACKED;
+typedef struct NvdimmFuncGetLabelSizeOut NvdimmFuncGetLabelSizeOut;
+QEMU_BUILD_BUG_ON(sizeof(NvdimmFuncGetLabelSizeOut) > TARGET_PAGE_SIZE);
+
+struct NvdimmFuncGetLabelDataOut {
+    /* the size of buffer filled by QEMU. */
+    uint32_t len;
+    uint32_t func_ret_status; /* return status code. */
+    uint8_t out_buf[0]; /* the data got via Get Namesapce Label function. */
+} QEMU_PACKED;
+typedef struct NvdimmFuncGetLabelDataOut NvdimmFuncGetLabelDataOut;
+
+struct NvdimmFuncSetLabelDataIn {
+    uint32_t offset; /* the offset in the namespace label data area. */
+    uint32_t length; /* the size of data is to be written via the function. */
+    uint8_t in_buf[0]; /* the data written to label data area. */
+} QEMU_PACKED;
+typedef struct NvdimmFuncSetLabelDataIn NvdimmFuncSetLabelDataIn;
+
 static void
 nvdimm_dsm_function0(uint32_t supported_func, hwaddr dsm_mem_addr)
 {
@@ -439,16 +488,89 @@ static void nvdimm_dsm_root(NvdimmDsmIn *in, hwaddr dsm_mem_addr)
     nvdimm_dsm_no_payload(1 /* Not Supported */, dsm_mem_addr);
 }
 
+/*
+ * the max transfer size is the max size transferred by both a
+ * 'Get Namespace Label Data' function and a 'Set Namespace Label Data'
+ * function.
+ */
+static uint32_t nvdimm_get_max_xfer_label_size(void)
+{
+    uint32_t max_get_size, max_set_size, dsm_memory_size = TARGET_PAGE_SIZE;
+
+    /*
+     * the max data ACPI can read one time which is transferred by
+     * the response of 'Get Namespace Label Data' function.
+     */
+    max_get_size = dsm_memory_size - sizeof(NvdimmFuncGetLabelDataOut);
+
+    /*
+     * the max data ACPI can write one time which is transferred by
+     * 'Set Namespace Label Data' function.
+     */
+    max_set_size = dsm_memory_size - sizeof(NvdimmDsmIn) -
+                   sizeof(NvdimmFuncSetLabelDataIn);
+
+    return MIN(max_get_size, max_set_size);
+}
+
+/*
+ * DSM Spec Rev1 4.4 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_label_size(NVDIMMDevice *nvdimm, hwaddr dsm_mem_addr)
+{
+    NvdimmFuncGetLabelSizeOut label_size_out = {
+        .len = cpu_to_le32(sizeof(label_size_out)),
+    };
+    uint32_t label_size, mxfer;
+
+    label_size = nvdimm->label_size;
+    mxfer = nvdimm_get_max_xfer_label_size();
+
+    nvdimm_debug("label_size %#x, max_xfer %#x.\n", label_size, mxfer);
+
+    label_size_out.func_ret_status = cpu_to_le32(0 /* Success */);
+    label_size_out.label_size = cpu_to_le32(label_size);
+    label_size_out.max_xfer = cpu_to_le32(mxfer);
+
+    cpu_physical_memory_write(dsm_mem_addr, &label_size_out,
+                              sizeof(label_size_out));
+}
+
 static void nvdimm_dsm_device(NvdimmDsmIn *in, hwaddr dsm_mem_addr)
 {
+    NVDIMMDevice *nvdimm = nvdimm_get_device_by_handle(in->handle);
+
     /* See the comments in nvdimm_dsm_root(). */
     if (!in->function) {
-        return nvdimm_dsm_function0(0 /* No function supported other
-                                         than function 0 */, dsm_mem_addr);
+        uint32_t supported_func = 0;
+
+        if (nvdimm && nvdimm->reserve_label) {
+            supported_func |= 0x1 /* Bit 0 indicates whether there is
+                                     support for any functions other
+                                     than function 0. */ |
+                              1 << 4 /* Get Namespace Label Size */;
+        }
+        return nvdimm_dsm_function0(supported_func, dsm_mem_addr);
     }
 
-    /* No function except function 0 is supported yet. */
-    nvdimm_dsm_no_payload(1 /* Not Supported */, dsm_mem_addr);
+    if (!nvdimm) {
+        return nvdimm_dsm_no_payload(1 /* Non-Existing Memory Device */,
+                                     dsm_mem_addr);
+    }
+
+    /* Encode DSM function according to DSM Spec Rev1. */
+    switch (in->function) {
+    case 4 /* Get Namespace Label Size */:
+        if (nvdimm->reserve_label) {
+            nvdimm_dsm_label_size(nvdimm, dsm_mem_addr);
+        }
+        break;
+    default:
+        nvdimm_dsm_no_payload(1 /* Not Supported */, dsm_mem_addr);
+    }
 }
 
 static uint64_t
-- 
1.8.3.1

[Qemu-devel] [PATCH 13/15] nvdimm acpi: support Get Namespace Label Data function

[Xiao Guangrong]

Function 5 is used to get Namespace Label Data

Signed-off-by: Xiao Guangrong <guangrong.xiao@linux.intel.com>
---
 hw/acpi/nvdimm.c | 81 +++++++++++++++++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 80 insertions(+), 1 deletion(-)

diff --git a/hw/acpi/nvdimm.c b/hw/acpi/nvdimm.c
index 4b5f672..1240328 100644
--- a/hw/acpi/nvdimm.c
+++ b/hw/acpi/nvdimm.c
@@ -438,6 +438,14 @@ struct NvdimmFuncGetLabelSizeOut {
 typedef struct NvdimmFuncGetLabelSizeOut NvdimmFuncGetLabelSizeOut;
 QEMU_BUILD_BUG_ON(sizeof(NvdimmFuncGetLabelSizeOut) > TARGET_PAGE_SIZE);
 
+struct NvdimmFuncGetLabelDataIn {
+    uint32_t offset; /* the offset in the namespace label data area. */
+    uint32_t length; /* the size of data is to be read via the function. */
+} QEMU_PACKED;
+typedef struct NvdimmFuncGetLabelDataIn NvdimmFuncGetLabelDataIn;
+QEMU_BUILD_BUG_ON(sizeof(NvdimmFuncGetLabelDataIn) + sizeof(NvdimmDsmIn) >
+                  TARGET_PAGE_SIZE);
+
 struct NvdimmFuncGetLabelDataOut {
     /* the size of buffer filled by QEMU. */
     uint32_t len;
@@ -445,6 +453,7 @@ struct NvdimmFuncGetLabelDataOut {
     uint8_t out_buf[0]; /* the data got via Get Namesapce Label function. */
 } QEMU_PACKED;
 typedef struct NvdimmFuncGetLabelDataOut NvdimmFuncGetLabelDataOut;
+QEMU_BUILD_BUG_ON(sizeof(NvdimmFuncGetLabelDataOut) > TARGET_PAGE_SIZE);
 
 struct NvdimmFuncSetLabelDataIn {
     uint32_t offset; /* the offset in the namespace label data area. */
@@ -539,6 +548,70 @@ static void nvdimm_dsm_label_size(NVDIMMDevice *nvdimm, hwaddr dsm_mem_addr)
                               sizeof(label_size_out));
 }
 
+static uint32_t nvdimm_rw_label_data_check(NVDIMMDevice *nvdimm,
+                                           uint32_t offset, uint32_t length)
+{
+    uint32_t ret = 3 /* Invalid Input Parameters */;
+
+    if (offset + length < offset) {
+        nvdimm_debug("offset %#x + length %#x is overflow.\n", offset,
+                     length);
+        return ret;
+    }
+
+    if (nvdimm->label_size < offset + length) {
+        nvdimm_debug("position %#x is beyond label data (len = %#lx).\n",
+                     offset + length, nvdimm->label_size);
+        return ret;
+    }
+
+    if (length > nvdimm_get_max_xfer_label_size()) {
+        nvdimm_debug("length (%#x) is larger than max_xfer (%#x).\n",
+                     length, nvdimm_get_max_xfer_label_size());
+        return ret;
+    }
+
+    return 0 /* Success */;
+}
+
+/*
+ * DSM Spec Rev1 4.5 Get Namespace Label Data (Function Index 5).
+ */
+static void nvdimm_dsm_get_label_data(NVDIMMDevice *nvdimm, NvdimmDsmIn *in,
+                                      hwaddr dsm_mem_addr)
+{
+    NVDIMMClass *nvc = NVDIMM_GET_CLASS(nvdimm);
+    NvdimmFuncGetLabelDataIn *get_label_data;
+    NvdimmFuncGetLabelDataOut *get_label_data_out;
+    uint32_t status;
+    int size;
+
+    get_label_data = (NvdimmFuncGetLabelDataIn *)in->arg3;
+    le32_to_cpus(&get_label_data->offset);
+    le32_to_cpus(&get_label_data->length);
+
+    nvdimm_debug("Read Label Data: offset %#x length %#x.\n",
+                 get_label_data->offset, get_label_data->length);
+
+    status = nvdimm_rw_label_data_check(nvdimm, get_label_data->offset,
+                                        get_label_data->length);
+    if (status != 0 /* Success */) {
+        return nvdimm_dsm_no_payload(status, dsm_mem_addr);
+    }
+
+    size = sizeof(*get_label_data_out) + get_label_data->length;
+    assert(size <= TARGET_PAGE_SIZE);
+    get_label_data_out = g_malloc(size);
+
+    get_label_data_out->len = cpu_to_le32(size);
+    get_label_data_out->func_ret_status = cpu_to_le32(0 /* Success */);
+    nvc->read_label_data(nvdimm, get_label_data_out->out_buf,
+                         get_label_data->length, get_label_data->offset);
+
+    cpu_physical_memory_write(dsm_mem_addr, get_label_data_out, size);
+    g_free(get_label_data_out);
+}
+
 static void nvdimm_dsm_device(NvdimmDsmIn *in, hwaddr dsm_mem_addr)
 {
     NVDIMMDevice *nvdimm = nvdimm_get_device_by_handle(in->handle);
@@ -551,7 +624,8 @@ static void nvdimm_dsm_device(NvdimmDsmIn *in, hwaddr dsm_mem_addr)
             supported_func |= 0x1 /* Bit 0 indicates whether there is
                                      support for any functions other
                                      than function 0. */ |
-                              1 << 4 /* Get Namespace Label Size */;
+                              1 << 4 /* Get Namespace Label Size */ |
+                              1 << 5 /* Get Namespace Label Data */;
         }
         return nvdimm_dsm_function0(supported_func, dsm_mem_addr);
     }
@@ -568,6 +642,11 @@ static void nvdimm_dsm_device(NvdimmDsmIn *in, hwaddr dsm_mem_addr)
             nvdimm_dsm_label_size(nvdimm, dsm_mem_addr);
         }
         break;
+    case 5 /* Get Namespace Label Data */:
+        if (nvdimm->reserve_label) {
+            nvdimm_dsm_get_label_data(nvdimm, in, dsm_mem_addr);
+        }
+        break;
     default:
         nvdimm_dsm_no_payload(1 /* Not Supported */, dsm_mem_addr);
     }
-- 
1.8.3.1

[Qemu-devel] [PATCH 14/15] nvdimm acpi: support Set Namespace Label Data function

[Xiao Guangrong]

Function 6 is used to set Namespace Label Data

Signed-off-by: Xiao Guangrong <guangrong.xiao@linux.intel.com>
---
 hw/acpi/nvdimm.c | 42 +++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 41 insertions(+), 1 deletion(-)

diff --git a/hw/acpi/nvdimm.c b/hw/acpi/nvdimm.c
index 1240328..f17be52 100644
--- a/hw/acpi/nvdimm.c
+++ b/hw/acpi/nvdimm.c
@@ -461,6 +461,8 @@ struct NvdimmFuncSetLabelDataIn {
     uint8_t in_buf[0]; /* the data written to label data area. */
 } QEMU_PACKED;
 typedef struct NvdimmFuncSetLabelDataIn NvdimmFuncSetLabelDataIn;
+QEMU_BUILD_BUG_ON(sizeof(NvdimmFuncSetLabelDataIn) + sizeof(NvdimmDsmIn) >
+                  TARGET_PAGE_SIZE);
 
 static void
 nvdimm_dsm_function0(uint32_t supported_func, hwaddr dsm_mem_addr)
@@ -612,6 +614,38 @@ static void nvdimm_dsm_get_label_data(NVDIMMDevice *nvdimm, NvdimmDsmIn *in,
     g_free(get_label_data_out);
 }
 
+/*
+ * DSM Spec Rev1 4.6 Set Namespace Label Data (Function Index 6).
+ */
+static void nvdimm_dsm_set_label_data(NVDIMMDevice *nvdimm, NvdimmDsmIn *in,
+                                      hwaddr dsm_mem_addr)
+{
+    NVDIMMClass *nvc = NVDIMM_GET_CLASS(nvdimm);
+    NvdimmFuncSetLabelDataIn *set_label_data;
+    uint32_t status;
+
+    set_label_data = (NvdimmFuncSetLabelDataIn *)in->arg3;
+
+    le32_to_cpus(&set_label_data->offset);
+    le32_to_cpus(&set_label_data->length);
+
+    nvdimm_debug("Write Label Data: offset %#x length %#x.\n",
+                 set_label_data->offset, set_label_data->length);
+
+    status = nvdimm_rw_label_data_check(nvdimm, set_label_data->offset,
+                                        set_label_data->length);
+    if (status != 0 /* Success */) {
+        return nvdimm_dsm_no_payload(status, dsm_mem_addr);
+    }
+
+    assert(sizeof(*in) + sizeof(*set_label_data) + set_label_data->length <=
+           TARGET_PAGE_SIZE);
+
+    nvc->write_label_data(nvdimm, set_label_data->in_buf,
+                          set_label_data->length, set_label_data->offset);
+    nvdimm_dsm_no_payload(0 /* Success */, dsm_mem_addr);
+}
+
 static void nvdimm_dsm_device(NvdimmDsmIn *in, hwaddr dsm_mem_addr)
 {
     NVDIMMDevice *nvdimm = nvdimm_get_device_by_handle(in->handle);
@@ -625,7 +659,8 @@ static void nvdimm_dsm_device(NvdimmDsmIn *in, hwaddr dsm_mem_addr)
                                      support for any functions other
                                      than function 0. */ |
                               1 << 4 /* Get Namespace Label Size */ |
-                              1 << 5 /* Get Namespace Label Data */;
+                              1 << 5 /* Get Namespace Label Data */ |
+                              1 << 6 /* Set Namespace Label Data */;
         }
         return nvdimm_dsm_function0(supported_func, dsm_mem_addr);
     }
@@ -647,6 +682,11 @@ static void nvdimm_dsm_device(NvdimmDsmIn *in, hwaddr dsm_mem_addr)
             nvdimm_dsm_get_label_data(nvdimm, in, dsm_mem_addr);
         }
         break;
+    case 0x6 /* Set Namespace Label Data */:
+        if (nvdimm->reserve_label) {
+            nvdimm_dsm_set_label_data(nvdimm, in, dsm_mem_addr);
+        }
+        break;
     default:
         nvdimm_dsm_no_payload(1 /* Not Supported */, dsm_mem_addr);
     }
-- 
1.8.3.1

[Qemu-devel] [PATCH 15/15] docs: add NVDIMM ACPI documentation

[Xiao Guangrong]

It describes the basic concepts of NVDIMM ACPI and the interfaces
between QEMU and the ACPI BIOS

Signed-off-by: Xiao Guangrong <guangrong.xiao@linux.intel.com>
---
 docs/specs/acpi_nvdimm.txt | 132 +++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 132 insertions(+)
 create mode 100644 docs/specs/acpi_nvdimm.txt

diff --git a/docs/specs/acpi_nvdimm.txt b/docs/specs/acpi_nvdimm.txt
new file mode 100644
index 0000000..19117ef
--- /dev/null
+++ b/docs/specs/acpi_nvdimm.txt
@@ -0,0 +1,132 @@
+QEMU<->ACPI BIOS NVDIMM interface
+---------------------------------
+
+QEMU supports NVDIMM via ACPI. This document describes the basic concepts of
+NVDIMM ACPI and the interface between QEMU and the ACPI BIOS.
+
+NVDIMM ACPI Background
+----------------------
+NVDIMM is introduced in ACPI 6.0 which defines an NVDIMM root device under
+_SB scope with a _HID of “ACPI0012”. For each NVDIMM present or intended
+to be supported by platform, platform firmware also exposes an ACPI
+Namespace Device under the root device.
+
+The NVDIMM child devices under the NVDIMM root device are defined with _ADR
+corresponding to the NFIT device handle. The NVDIMM root device and the
+NVDIMM devices can have device specific methods (_DSM) to provide additional
+functions specific to a particular NVDIMM implementation.
+
+This is an example from ACPI 6.0, a platform contains one NVDIMM:
+
+Scope (\_SB){
+   Device (NVDR) // Root device
+   {
+      Name (_HID, “ACPI0012”)
+      Method (_STA) {...}
+      Method (_FIT) {...}
+      Method (_DSM, ...) {...}
+      Device (NVD)
+      {
+         Name(_ADR, h) //where h is NFIT Device Handle for this NVDIMM
+         Method (_DSM, ...) {...}
+      }
+   }
+}
+
+Method supported on both NVDIMM root device and NVDIMM device
+_DSM (Device Specific Method)
+   It is a control method that enables devices to provide device specific
+   control functions that are consumed by the device driver.
+   The NVDIMM DSM specification can be found at:
+        http://pmem.io/documents/NVDIMM_DSM_Interface_Example.pdf
+
+   Arguments:
+   Arg0 – A Buffer containing a UUID (16 Bytes)
+   Arg1 – An Integer containing the Revision ID (4 Bytes)
+   Arg2 – An Integer containing the Function Index (4 Bytes)
+   Arg3 – A package containing parameters for the function specified by the
+          UUID, Revision ID, and Function Index
+
+   Return Value:
+   If Function Index = 0, a Buffer containing a function index bitfield.
+   Otherwise, the return value and type depends on the UUID, revision ID
+   and function index which are described in the DSM specification.
+
+Methods on NVDIMM ROOT Device
+_FIT(Firmware Interface Table)
+   It evaluates to a buffer returning data in the format of a series of NFIT
+   Type Structure.
+
+   Arguments: None
+
+   Return Value:
+   A Buffer containing a list of NFIT Type structure entries.
+
+   The detailed definition of the structure can be found at ACPI 6.0: 5.2.25
+   NVDIMM Firmware Interface Table (NFIT).
+
+QEMU NVDIMM Implemention
+========================
+QEMU uses 4 bytes IO Port starting from 0x0a18 and a RAM-based memory page
+for NVDIMM ACPI.
+
+Memory:
+   QEMU uses BIOS Linker/loader feature to ask BIOS to allocate a memory
+   page and dynamically patch its into a int32 object named "MEMA" in ACPI.
+
+   This page is RAM-based and it is used to transfer data between _DSM
+   method and QEMU. If ACPI has control, this pages is owned by ACPI which
+   writes _DSM input data to it, otherwise, it is owned by QEMU which
+   emulates _DSM access and writes the output data to it.
+
+   ACPI writes _DSM Input Data (based on the offset in the page):
+   [0x0 - 0x3]: 4 bytes, NVDIMM Device Handle, 0 is reserved for NVDIMM
+                Root device.
+   [0x4 - 0x7]: 4 bytes, Revision ID, that is the Arg1 of _DSM method.
+   [0x8 - 0xB]: 4 bytes. Function Index, that is the Arg2 of _DSM method.
+   [0xC - 0xFFF]: 4084 bytes, the Arg3 of _DSM method.
+
+   QEMU Writes Output Data (based on the offset in the page):
+   [0x0 - 0x3]: 4 bytes, the length of result
+   [0x4 - 0xFFF]: 4092 bytes, the DSM result filled by QEMU
+
+IO Port 0x0a18 - 0xa1b:
+   ACPI writes the address of the memory page allocated by BIOS to this
+   port then QEMU gets the control and fills the result in the memory page.
+
+   write Access:
+   [0x0a18 - 0xa1b]: 4 bytes, the address of the memory page allocated
+                     by BIOS.
+
+_DSM process diagram:
+---------------------
+"MEMA" indicates the address of memory page allocated by BIOS.
+
+ +----------------------+      +-----------------------+
+ |    1. OSPM           |      |    2. OSPM            |
+ | save _DSM input data |      |  write "MEMA" to      | Exit to QEMU
+ | to the page          +----->|  IO port 0x0a18       +------------+
+ | indicated by "MEMA"  |      |                       |            |
+ +----------------------+      +-----------------------+            |
+                                                                    |
+                                                                    v
+ +-------------   ----+       +-----------+      +------------------+--------+
+ |      5 QEMU        |       | 4 QEMU    |      |        3. QEMU            |
+ | write _DSM result  |       |  emulate  |      | get _DSM input data from  |
+ | to the page        +<------+ _DSM      +<-----+ the page indicated by the |
+ |                    |       |           |      | value from the IO port    |
+ +--------+-----------+       +-----------+      +---------------------------+
+          |
+          | Enter Guest
+          |
+          v
+ +--------------------------+      +--------------+
+ |     6 OSPM               |      |   7 OSPM     |
+ | result size is returned  |      |  _DSM return |
+ | by reading  DSM          +----->+              |
+ | result from the page     |      |              |
+ +--------------------------+      +--------------+
+
+ _FIT implementation
+ -------------------
+ TODO (will fill it when nvdimm hotplug is introduced)
\ No newline at end of file
-- 
1.8.3.1

Re: [Qemu-devel] [PATCH 00/15] NVDIMM: introduce nvdimm label support

[Stefan Hajnoczi]

[-- Attachment #1: Type: text/plain, Size: 1103 bytes --]

On Thu, Mar 17, 2016 at 04:32:46PM +0800, Xiao Guangrong wrote:
> This patchset is against commit d4207b223eef3 (fw-cfg: support writeable
> blobs) on pci branch of Michael's git tree and can be found at:
>       https://github.com/xiaogr/qemu.git nvdimm-label-v1
> 
> This is the last part of vNVDIMM implementation which introduces nvdimm
> label support
> 
> Currently Linux NVDIMM driver does not support namespace operation on this
> kind of PMEM, apply below changes to support dynamical namespace:
> 
> @@ -798,7 +823,8 @@ static int acpi_nfit_register_dimms(struct acpi_nfit_desc *a
>                         continue;
>                 }
>  
> -               if (nfit_mem->bdw && nfit_mem->memdev_pmem)
> +               //if (nfit_mem->bdw && nfit_mem->memdev_pmem)
> +               if (nfit_mem->memdev_pmem)
>                         flags |= NDD_ALIASING;

Not a blocker for this patch series, but why does Linux require Block
Device Window to enable namespace support?

Will this be changed upstream in the Linux driver?

Have you tested Windows guest support?

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

Re: [Qemu-devel] [PATCH 04/15] nvdimm: support nvdimm label

[Stefan Hajnoczi]

[-- Attachment #1: Type: text/plain, Size: 1100 bytes --]

On Thu, Mar 17, 2016 at 04:32:50PM +0800, Xiao Guangrong wrote:
> +static void nvdimm_init(Object *obj)
> +{
> +    object_property_add_bool(obj, "reserve-label", nvdimm_get_reserve_label,
> +                             nvdimm_set_reserve_label, NULL);

In the future users may wish for larger namespace label sizes.  This
bool option will not allow that.

Perhaps the option should be an integer called "label-size"?

> +static void nvdimm_assert_rw_label_data(NVDIMMDevice *nvdimm, uint64_t size,
> +                                        uint64_t offset)
> +{
> +    assert(nvdimm->reserve_label &&
> +           (nvdimm->label_size >= size + offset) && (offset + size > offset));
> +}

It's not clear from this patch alone, but QEMU is not allowed to assert
due to invalid inputs from the guest.  So if input validation is
necessary here because the values may be invalid, please write if
statements and error returns.

This is important so guests cannot cause QEMU to core dump (SIGABRT
default behavior) and so that nested virtualization doesn't allow a
nested guest to DoS its parent guest.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

Re: [Qemu-devel] [PATCH 10/15] nvdimm acpi: abstract the operations for root device and nvdimm devices

[Stefan Hajnoczi]

[-- Attachment #1: Type: text/plain, Size: 1297 bytes --]

On Thu, Mar 17, 2016 at 04:32:56PM +0800, Xiao Guangrong wrote:
> +static void
> +nvdimm_dsm_function0(uint32_t supported_func, hwaddr dsm_mem_addr)
> +{
> +    NvdimmDsmFunc0Out func0 = {
> +        .len = cpu_to_le32(sizeof(func0)),
> +        .supported_func = cpu_to_le32(supported_func),
> +    };
> +    cpu_physical_memory_write(dsm_mem_addr, &func0, sizeof(func0));
> +}
> +
> +static void
> +nvdimm_dsm_no_payload(uint32_t func_ret_status, hwaddr dsm_mem_addr)
> +{
> +    NvdimmDsmFuncNoPayloadOut out = {
> +        .len = cpu_to_le32(sizeof(out)),
> +        .func_ret_status = cpu_to_le32(func_ret_status),
> +    };
> +    cpu_physical_memory_write(dsm_mem_addr, &out, sizeof(out));
> +}
> +
> +static void nvdimm_dsm_root(NvdimmDsmIn *in, hwaddr dsm_mem_addr)
> +{
> +    /*
> +     * function 0 is called to inquire which functions are supported by
> +     * OSPM
> +     */
> +    if (!in->function) {
> +        return nvdimm_dsm_function0(0 /* No function supported other
> +                                         than function 0 */, dsm_mem_addr);

The return type is void so "return foo()" looks strange.  I went back
and double-checked function prototypes because I was surprised by this
line of code.  Please use the conventional "foo(); return;" for void
return instead.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

Re: [Qemu-devel] [PATCH 12/15] nvdimm acpi: support Get Namespace Label Size function

[Stefan Hajnoczi]

[-- Attachment #1: Type: text/plain, Size: 1227 bytes --]

On Thu, Mar 17, 2016 at 04:32:58PM +0800, Xiao Guangrong wrote:
> -    /* No function except function 0 is supported yet. */
> -    nvdimm_dsm_no_payload(1 /* Not Supported */, dsm_mem_addr);
> +    if (!nvdimm) {
> +        return nvdimm_dsm_no_payload(1 /* Non-Existing Memory Device */,

"Non-existing Memory Device" is 2 according to the spec.  1 would be
"Not Supported".

Please define constants for all these magic values instead of putting
literals into the code:

enum {
    NVDIMM_DSM_SUCCESS = 0,
    NVDIMM_DSM_NOT_SUPPORTED = 1,
    NVDIMM_DSM_NON_EXISTING_MEMORY_DEVICE = 2,
    NVDIMM_DSM_INVALID_INPUT_PARAMETERS = 3,
    NVDIMM_DSM_VENDOR_SPECIFIC_ERROR = 4,
};

> +                                     dsm_mem_addr);
> +    }
> +
> +    /* Encode DSM function according to DSM Spec Rev1. */
> +    switch (in->function) {
> +    case 4 /* Get Namespace Label Size */:
> +        if (nvdimm->reserve_label) {
> +            nvdimm_dsm_label_size(nvdimm, dsm_mem_addr);
> +        }
> +        break;

What is the expected return status code if the device has no labels?

This function must write a return status code, otherwise the guest will
read the out buffer and attempt to interpret uninitialized memory.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

Re: [Qemu-devel] [PATCH 00/15] NVDIMM: introduce nvdimm label support

[Michael S. Tsirkin]

On Thu, Mar 17, 2016 at 04:32:46PM +0800, Xiao Guangrong wrote:
> This patchset is against commit d4207b223eef3 (fw-cfg: support writeable
> blobs) on pci branch of Michael's git tree and can be found at:
>       https://github.com/xiaogr/qemu.git nvdimm-label-v1

I see there have been comments, so feel free to iterate on-list, but
please also remember to repost after 2.6 is out.
Thanks!


> This is the last part of vNVDIMM implementation which introduces nvdimm
> label support
> 
> Currently Linux NVDIMM driver does not support namespace operation on this
> kind of PMEM, apply below changes to support dynamical namespace:
> 
> @@ -798,7 +823,8 @@ static int acpi_nfit_register_dimms(struct acpi_nfit_desc *a
>                         continue;
>                 }
>  
> -               if (nfit_mem->bdw && nfit_mem->memdev_pmem)
> +               //if (nfit_mem->bdw && nfit_mem->memdev_pmem)
> +               if (nfit_mem->memdev_pmem)
>                         flags |= NDD_ALIASING;
> 
> You can append a NVDIMM device in guest and do:                       
> # cd /sys/bus/nd/devices/
> # cd namespace0.0/
> # echo `uuidgen` > uuid
> # echo `expr 1024 \* 1024 \* 128` > size
> then reload nd.pmem.ko
> 
> You can see /dev/pmem0 appears
> 
> Xiao Guangrong (15):
>   pc-dimm: get memory region from ->get_memory_region()
>   pc-dimm: introduce realize callback
>   pc-dimm: keep the state of the whole backend memory
>   nvdimm: support nvdimm label
>   acpi: add aml_object_type
>   acpi: add aml_call5
>   nvdimm acpi: set HDLE properly
>   nvdimm acpi: save arg3 of _DSM method
>   nvdimm acpi: check UUID
>   nvdimm acpi: abstract the operations for root device and nvdimm
>     devices
>   nvdimm acpi: check revision
>   nvdimm acpi: support Get Namespace Label Size function
>   nvdimm acpi: support Get Namespace Label Data function
>   nvdimm acpi: support Set Namespace Label Data function
>   docs: add NVDIMM ACPI documentation
> 
>  docs/specs/acpi_nvdimm.txt  | 132 +++++++++++++++
>  hw/acpi/aml-build.c         |  22 +++
>  hw/acpi/nvdimm.c            | 392 ++++++++++++++++++++++++++++++++++++++++----
>  hw/mem/nvdimm.c             |  95 +++++++++++
>  hw/mem/pc-dimm.c            |  21 ++-
>  include/hw/acpi/aml-build.h |   3 +
>  include/hw/mem/nvdimm.h     |  61 ++++++-
>  include/hw/mem/pc-dimm.h    |   6 +-
>  8 files changed, 694 insertions(+), 38 deletions(-)
>  create mode 100644 docs/specs/acpi_nvdimm.txt
> 
> -- 
> 1.8.3.1

Re: [Qemu-devel] [PATCH 00/15] NVDIMM: introduce nvdimm label support

[Dan Williams]

On Thu, Mar 17, 2016 at 3:04 AM, Stefan Hajnoczi <stefanha@gmail.com> wrote:
> On Thu, Mar 17, 2016 at 04:32:46PM +0800, Xiao Guangrong wrote:
>> This patchset is against commit d4207b223eef3 (fw-cfg: support writeable
>> blobs) on pci branch of Michael's git tree and can be found at:
>>       https://github.com/xiaogr/qemu.git nvdimm-label-v1
>>
>> This is the last part of vNVDIMM implementation which introduces nvdimm
>> label support
>>
>> Currently Linux NVDIMM driver does not support namespace operation on this
>> kind of PMEM, apply below changes to support dynamical namespace:
>>
>> @@ -798,7 +823,8 @@ static int acpi_nfit_register_dimms(struct acpi_nfit_desc *a
>>                         continue;
>>                 }
>>
>> -               if (nfit_mem->bdw && nfit_mem->memdev_pmem)
>> +               //if (nfit_mem->bdw && nfit_mem->memdev_pmem)
>> +               if (nfit_mem->memdev_pmem)
>>                         flags |= NDD_ALIASING;
>
> Not a blocker for this patch series, but why does Linux require Block
> Device Window to enable namespace support?

A namespace label delineates aliased capacity between the pmem and
block-window access mechanisms.  If there is no aliased capacity then
the size of the namespace can be directly derived from the nfit range
and a label need not be considered.  Contiguous (dax-capable)
sub-divisions of pmem can be had via partitioning of the resulting
gendisk.

> Will this be changed upstream in the Linux driver?

There are no plans to change this.  Also note that the above change
only allows one allocation which is probably not what we want.

All this said, the hypervisor could simulate aliased block-window
capacity in a similar manner as nfit_test.  This would allow
dis-contiguous allocations of the persistent memory range passed to a
guest via this label mechanism.

Re: [Qemu-devel] [PATCH 00/15] NVDIMM: introduce nvdimm label support

[Stefan Hajnoczi]

[-- Attachment #1: Type: text/plain, Size: 1851 bytes --]

On Tue, Mar 22, 2016 at 08:37:40AM -0700, Dan Williams wrote:
> On Thu, Mar 17, 2016 at 3:04 AM, Stefan Hajnoczi <stefanha@gmail.com> wrote:
> > On Thu, Mar 17, 2016 at 04:32:46PM +0800, Xiao Guangrong wrote:
> >> This patchset is against commit d4207b223eef3 (fw-cfg: support writeable
> >> blobs) on pci branch of Michael's git tree and can be found at:
> >>       https://github.com/xiaogr/qemu.git nvdimm-label-v1
> >>
> >> This is the last part of vNVDIMM implementation which introduces nvdimm
> >> label support
> >>
> >> Currently Linux NVDIMM driver does not support namespace operation on this
> >> kind of PMEM, apply below changes to support dynamical namespace:
> >>
> >> @@ -798,7 +823,8 @@ static int acpi_nfit_register_dimms(struct acpi_nfit_desc *a
> >>                         continue;
> >>                 }
> >>
> >> -               if (nfit_mem->bdw && nfit_mem->memdev_pmem)
> >> +               //if (nfit_mem->bdw && nfit_mem->memdev_pmem)
> >> +               if (nfit_mem->memdev_pmem)
> >>                         flags |= NDD_ALIASING;
> >
> > Not a blocker for this patch series, but why does Linux require Block
> > Device Window to enable namespace support?
> 
> A namespace label delineates aliased capacity between the pmem and
> block-window access mechanisms.  If there is no aliased capacity then
> the size of the namespace can be directly derived from the nfit range
> and a label need not be considered.  Contiguous (dax-capable)
> sub-divisions of pmem can be had via partitioning of the resulting
> gendisk.

Xiao Guangrong: Given this new information, what is the purpose of the
QEMU patches for ACPI DSM and especially the namespace label support?

The QEMU NVDIMM device only supports pmem so now I'm not sure we need
namespace labels or the ACPI DSM at all.

Stefan

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

Re: [Qemu-devel] [PATCH 00/15] NVDIMM: introduce nvdimm label support

[Xiao Guangrong]

Sorry for the delay and i just come back from my vacation.

On 03/23/2016 04:30 AM, Stefan Hajnoczi wrote:
> On Tue, Mar 22, 2016 at 08:37:40AM -0700, Dan Williams wrote:
>> On Thu, Mar 17, 2016 at 3:04 AM, Stefan Hajnoczi <stefanha@gmail.com> wrote:
>>> On Thu, Mar 17, 2016 at 04:32:46PM +0800, Xiao Guangrong wrote:
>>>> This patchset is against commit d4207b223eef3 (fw-cfg: support writeable
>>>> blobs) on pci branch of Michael's git tree and can be found at:
>>>>        https://github.com/xiaogr/qemu.git nvdimm-label-v1
>>>>
>>>> This is the last part of vNVDIMM implementation which introduces nvdimm
>>>> label support
>>>>
>>>> Currently Linux NVDIMM driver does not support namespace operation on this
>>>> kind of PMEM, apply below changes to support dynamical namespace:
>>>>
>>>> @@ -798,7 +823,8 @@ static int acpi_nfit_register_dimms(struct acpi_nfit_desc *a
>>>>                          continue;
>>>>                  }
>>>>
>>>> -               if (nfit_mem->bdw && nfit_mem->memdev_pmem)
>>>> +               //if (nfit_mem->bdw && nfit_mem->memdev_pmem)
>>>> +               if (nfit_mem->memdev_pmem)
>>>>                          flags |= NDD_ALIASING;
>>>
>>> Not a blocker for this patch series, but why does Linux require Block
>>> Device Window to enable namespace support?
>>
>> A namespace label delineates aliased capacity between the pmem and
>> block-window access mechanisms.  If there is no aliased capacity then
>> the size of the namespace can be directly derived from the nfit range
>> and a label need not be considered.  Contiguous (dax-capable)
>> sub-divisions of pmem can be had via partitioning of the resulting
>> gendisk.
>
> Xiao Guangrong: Given this new information, what is the purpose of the
> QEMU patches for ACPI DSM and especially the namespace label support?
>

The initiation goal we introduced vlabel support is:
| Yes, I see Linux driver supports label-less vNVDIMM that is exact current QEMU
| doing. However, label-less is only Linux specific implementation (as it
| completely bypasses namespace), other OS vendors (e.g Microsoft) will use label
| storage to address their own requirements，or they do not follow namespace spec
| at all. Another reason is that label is essential for PBLK support.

I did not get the windows driver so far, so this is not tested on windows yet,
just double checked with windows driver team, they told me that:
"It always assumes there is one there so we will need to change how this works"
So the update of windows side will be made.

> The QEMU NVDIMM device only supports pmem so now I'm not sure we need
> namespace labels or the ACPI DSM at all.
>

We are planing to support PBLK which "simulated BLK it can simply make one big
aperture that happens to overlap the same address range as PMEM", it will "allow
enabling BTT on BLK namespaces to support power-fail-safe sector updates for a
filesystem on PMEM". The idea was contributed by Dan in our internal discussion...

Re: [Qemu-devel] [PATCH 04/15] nvdimm: support nvdimm label

[Xiao Guangrong]

On 03/17/2016 06:28 PM, Stefan Hajnoczi wrote:
> On Thu, Mar 17, 2016 at 04:32:50PM +0800, Xiao Guangrong wrote:
>> +static void nvdimm_init(Object *obj)
>> +{
>> +    object_property_add_bool(obj, "reserve-label", nvdimm_get_reserve_label,
>> +                             nvdimm_set_reserve_label, NULL);
>
> In the future users may wish for larger namespace label sizes.  This
> bool option will not allow that.
>
> Perhaps the option should be an integer called "label-size"?

Yes, good to me.

>
>> +static void nvdimm_assert_rw_label_data(NVDIMMDevice *nvdimm, uint64_t size,
>> +                                        uint64_t offset)
>> +{
>> +    assert(nvdimm->reserve_label &&
>> +           (nvdimm->label_size >= size + offset) && (offset + size > offset));
>> +}
>
> It's not clear from this patch alone, but QEMU is not allowed to assert
> due to invalid inputs from the guest.  So if input validation is
> necessary here because the values may be invalid, please write if
> statements and error returns.

The caller should check it before calling these callbacks, in our case, we did
it in nvdimm_rw_label_data_check() in patch 13.

So if that happen, it is really a QEMU internal BUG.

>
> This is important so guests cannot cause QEMU to core dump (SIGABRT
> default behavior) and so that nested virtualization doesn't allow a
> nested guest to DoS its parent guest.


Yes, i understood it, but it is not the case in this patch as the assert()
can not be triggered by guest.

Maybe i should mention it in the changelog to make this fact more clean.

Re: [Qemu-devel] [PATCH 10/15] nvdimm acpi: abstract the operations for root device and nvdimm devices

[Xiao Guangrong]

On 03/17/2016 06:35 PM, Stefan Hajnoczi wrote:
> On Thu, Mar 17, 2016 at 04:32:56PM +0800, Xiao Guangrong wrote:
>> +static void
>> +nvdimm_dsm_function0(uint32_t supported_func, hwaddr dsm_mem_addr)
>> +{
>> +    NvdimmDsmFunc0Out func0 = {
>> +        .len = cpu_to_le32(sizeof(func0)),
>> +        .supported_func = cpu_to_le32(supported_func),
>> +    };
>> +    cpu_physical_memory_write(dsm_mem_addr, &func0, sizeof(func0));
>> +}
>> +
>> +static void
>> +nvdimm_dsm_no_payload(uint32_t func_ret_status, hwaddr dsm_mem_addr)
>> +{
>> +    NvdimmDsmFuncNoPayloadOut out = {
>> +        .len = cpu_to_le32(sizeof(out)),
>> +        .func_ret_status = cpu_to_le32(func_ret_status),
>> +    };
>> +    cpu_physical_memory_write(dsm_mem_addr, &out, sizeof(out));
>> +}
>> +
>> +static void nvdimm_dsm_root(NvdimmDsmIn *in, hwaddr dsm_mem_addr)
>> +{
>> +    /*
>> +     * function 0 is called to inquire which functions are supported by
>> +     * OSPM
>> +     */
>> +    if (!in->function) {
>> +        return nvdimm_dsm_function0(0 /* No function supported other
>> +                                         than function 0 */, dsm_mem_addr);
>
> The return type is void so "return foo()" looks strange.  I went back
> and double-checked function prototypes because I was surprised by this
> line of code.  Please use the conventional "foo(); return;" for void
> return instead.
>

I am so lazy to omit this single line. :)

Okay, will follow this style in the next version.

Re: [Qemu-devel] [PATCH 12/15] nvdimm acpi: support Get Namespace Label Size function

[Xiao Guangrong]

On 03/17/2016 06:58 PM, Stefan Hajnoczi wrote:
> On Thu, Mar 17, 2016 at 04:32:58PM +0800, Xiao Guangrong wrote:
>> -    /* No function except function 0 is supported yet. */
>> -    nvdimm_dsm_no_payload(1 /* Not Supported */, dsm_mem_addr);
>> +    if (!nvdimm) {
>> +        return nvdimm_dsm_no_payload(1 /* Non-Existing Memory Device */,
>
> "Non-existing Memory Device" is 2 according to the spec.  1 would be
> "Not Supported".

Yes, indeed.

>
> Please define constants for all these magic values instead of putting
> literals into the code:
>
> enum {
>      NVDIMM_DSM_SUCCESS = 0,
>      NVDIMM_DSM_NOT_SUPPORTED = 1,
>      NVDIMM_DSM_NON_EXISTING_MEMORY_DEVICE = 2,
>      NVDIMM_DSM_INVALID_INPUT_PARAMETERS = 3,
>      NVDIMM_DSM_VENDOR_SPECIFIC_ERROR = 4,
> };

Er, it seems Michael much prefers the style which uses raw number which
is followed by a comment. :(

>
>> +                                     dsm_mem_addr);
>> +    }
>> +
>> +    /* Encode DSM function according to DSM Spec Rev1. */
>> +    switch (in->function) {
>> +    case 4 /* Get Namespace Label Size */:
>> +        if (nvdimm->reserve_label) {
>> +            nvdimm_dsm_label_size(nvdimm, dsm_mem_addr);
>> +        }
>> +        break;
>
> What is the expected return status code if the device has no labels?
>
> This function must write a return status code, otherwise the guest will
> read the out buffer and attempt to interpret uninitialized memory.
>

Yes, my fault, will fix it. Thanks you for pointing it out.

Re: [Qemu-devel] [PATCH 00/15] NVDIMM: introduce nvdimm label support

[Xiao Guangrong]

On 03/22/2016 07:17 PM, Michael S. Tsirkin wrote:
> On Thu, Mar 17, 2016 at 04:32:46PM +0800, Xiao Guangrong wrote:
>> This patchset is against commit d4207b223eef3 (fw-cfg: support writeable
>> blobs) on pci branch of Michael's git tree and can be found at:
>>        https://github.com/xiaogr/qemu.git nvdimm-label-v1
>
> I see there have been comments, so feel free to iterate on-list, but
> please also remember to repost after 2.6 is out.

Okay.

Thank you, Michael!

Re: [Qemu-devel] [PATCH 00/15] NVDIMM: introduce nvdimm label support

[Stefan Hajnoczi]

[-- Attachment #1: Type: text/plain, Size: 834 bytes --]

On Wed, Mar 23, 2016 at 10:46:56AM +0800, Xiao Guangrong wrote:
> On 03/23/2016 04:30 AM, Stefan Hajnoczi wrote:
> >On Tue, Mar 22, 2016 at 08:37:40AM -0700, Dan Williams wrote:
> >>On Thu, Mar 17, 2016 at 3:04 AM, Stefan Hajnoczi <stefanha@gmail.com> wrote:
> >>>On Thu, Mar 17, 2016 at 04:32:46PM +0800, Xiao Guangrong wrote:
> >The QEMU NVDIMM device only supports pmem so now I'm not sure we need
> >namespace labels or the ACPI DSM at all.
> >
> 
> We are planing to support PBLK which "simulated BLK it can simply make one big
> aperture that happens to overlap the same address range as PMEM", it will "allow
> enabling BTT on BLK namespaces to support power-fail-safe sector updates for a
> filesystem on PMEM". The idea was contributed by Dan in our internal discussion...

Thanks, that makes sense.

Stefan

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 473 bytes --]