[PATCH 0/2] add vNVDIMM support for Xen

[PATCH 0/2] add vNVDIMM support for Xen

[Haozhong Zhang]

This patch series extends current vNVDIMM implementation to provide
vNVDIMM to HVM domains when QEMU is used as the device model of Xen.

This patch series is based on upstream qemu rather of qemu-xen,
because vNVDIMM support has not been in qemu-xen (commit f165e58).

* Following two problem that prevent Xen from directly using current
  implementation are solved by this patch series.

 (1) The current way to allocate memory for pc-dimm based vNVDIMM
     through file-backend device is not compatible with Xen. Patch 2
     adds another pc-nvdimm device to manage vNVDIMM's memory by
     itself.
    
     A pc-nvdimm device can be used only with Xen and is specified by
     parameters like
            -device pc-nvdimm,file=/dev/pmem0,size=MBYTES
	    
 (2) Xen uses its hvmloader rather than QEMU to build guest ACPI
     tables. In order to reuse as much code as possible, patch 2 calls
     the existing QEMU code to build guest ACPI tables for pc-nvdimm
     devices and passes them to hvmloader.

* Test
 (1) A patched Xen is used for test. Xen patch series is sent
     separately with title "[PATCH 0/4] add support for vNVDIMM".
      
 (2) Prepare a memory backend file:
            dd if=/dev/zero of=/tmp/nvm0 bs=1G count=10

 (3) Add the following line to a HVM domain's configuration xl.cfg:
            nvdimm =[ 'file=/tmp/nvm0,size=10240' ]

 (4) Launch a HVM domain from above xl.cfg.

 (5) If guest Linux kernel is 4.2 or newer and kernel modules
     libnvdimm, nfit, nd_btt and nd_pmem are loaded, then you will see
     the whole nvdimm device used as a single namespace and /dev/pmem0
     will appear.



Haozhong Zhang (2):
  pc-nvdimm: implement pc-nvdimm device abstract
  pc-nvdimm acpi: build ACPI tables for pc-nvdimm devices

 hw/acpi/nvdimm.c           |   5 +-
 hw/i386/pc.c               |   6 +-
 hw/mem/Makefile.objs       |   1 +
 hw/mem/pc-nvdimm.c         | 239 +++++++++++++++++++++++++++++++++++++++++++++
 include/hw/mem/pc-nvdimm.h |  49 ++++++++++
 include/hw/xen/xen.h       |   2 +
 xen-hvm.c                  |  73 ++++++++++++++
 7 files changed, 373 insertions(+), 2 deletions(-)
 create mode 100644 hw/mem/pc-nvdimm.c
 create mode 100644 include/hw/mem/pc-nvdimm.h

-- 
2.4.8

[PATCH 1/2] pc-nvdimm: implement pc-nvdimm device abstract

[Haozhong Zhang]

The current implementation of vNVDIMM is based on pc-dimm and uses
memory-backend device to allocate memory, which is not compatible with
Xen ("-mem-path not supported with Xen" in qemu_ram_alloc_file()). This
patch adds another pc-nvdimm device that does not rely on pc-dimm and
allocates memory by itself.

This patch combines several parts of Guangrong's v2 patch series
"implement vNVDIMM" and Xen-specific adjustments.

Signed-off-by: Haozhong Zhang <haozhong.zhang@intel.com>
---
 hw/mem/Makefile.objs       |   1 +
 hw/mem/pc-nvdimm.c         | 239 +++++++++++++++++++++++++++++++++++++++++++++
 include/hw/mem/pc-nvdimm.h |  49 ++++++++++
 xen-hvm.c                  |   2 +
 4 files changed, 291 insertions(+)
 create mode 100644 hw/mem/pc-nvdimm.c
 create mode 100644 include/hw/mem/pc-nvdimm.h

diff --git a/hw/mem/Makefile.objs b/hw/mem/Makefile.objs
index f12f8b9..4257da0 100644
--- a/hw/mem/Makefile.objs
+++ b/hw/mem/Makefile.objs
@@ -1,2 +1,3 @@
 common-obj-$(CONFIG_MEM_HOTPLUG) += pc-dimm.o
 common-obj-$(CONFIG_NVDIMM) += nvdimm.o
+common-obj-$(CONFIG_NVDIMM) += pc-nvdimm.o
diff --git a/hw/mem/pc-nvdimm.c b/hw/mem/pc-nvdimm.c
new file mode 100644
index 0000000..321f734
--- /dev/null
+++ b/hw/mem/pc-nvdimm.c
@@ -0,0 +1,239 @@
+/*
+ * NVDIMM (A Non-Volatile Dual In-line Memory Module) Virtualization Implement
+ *
+ * Copyright(C) 2015 Intel Corporation.
+ *
+ * Author:
+ *  Xiao Guangrong <guangrong.xiao@linux.intel.com>
+ *  Haozhong Zhang <haozhong.zhang@intel.com>
+ *
+ * Currently, it only supports PMEM Virtualization.
+ *
+ * This library is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU Lesser General Public
+ * License as published by the Free Software Foundation; either
+ * version 2 of the License, or (at your option) any later version.
+ *
+ * This library is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
+ * Lesser General Public License for more details.
+ *
+ * You should have received a copy of the GNU Lesser General Public
+ * License along with this library; if not, see <http://www.gnu.org/licenses/>
+ */
+
+#include <sys/stat.h>
+
+#include "qom/object.h"
+#include "qapi/visitor.h"
+#include "qemu/mmap-alloc.h"
+#include "exec/address-spaces.h"
+#include "hw/mem/pc-nvdimm.h"
+#include "hw/xen/xen.h"
+
+#define PC_NVDIMM_ADDR_ALIGN 0x40000000
+
+struct NvdimmsInfo {
+    ram_addr_t current_addr;
+    int device_index;
+};
+
+static struct NvdimmsInfo nvdimms_info;
+
+static ram_addr_t pc_nvdimm_reserved_range_push(uint64_t size)
+{
+    uint64_t current;
+
+    current = ROUND_UP(nvdimms_info.current_addr, PC_NVDIMM_ADDR_ALIGN);
+
+    /* do not have enough space? */
+    if (current + size < current) {
+        return 0;
+    }
+
+    nvdimms_info.current_addr = current + size;
+    return current;
+}
+
+void pc_nvdimm_reserve_range(ram_addr_t offset)
+{
+    nvdimms_info.current_addr = ROUND_UP(offset, PC_NVDIMM_ADDR_ALIGN);
+}
+
+static int pc_nvdimm_new_device_index(void)
+{
+    return nvdimms_info.device_index++;
+}
+
+static void pc_nvdimm_get_addr(Object *obj, Visitor *v, void *opaque,
+                               const char *name, Error **errp)
+{
+    int64_t value;
+    PCNVDIMMDevice *nvdimm = PC_NVDIMM(obj);
+    value = nvdimm->addr;
+    visit_type_int(v, &value, name, errp);
+}
+
+static void pc_nvdimm_get_slot(Object *obj, Visitor *v, void *opaque,
+                               const char *name, Error **errp)
+{
+    int64_t value;
+    PCNVDIMMDevice *nvdimm = PC_NVDIMM(obj);
+    value = nvdimm->dev_idx + 1;
+    visit_type_int(v, &value, name, errp);
+}
+
+static void pc_nvdimm_get_node(Object *obj, Visitor *v, void *opaque,
+                               const char *name, Error **errp)
+{
+    int64_t value;
+    value = 0;
+    visit_type_int(v, &value, name, errp);
+}
+
+static void pc_nvdimm_get_size(Object *obj, Visitor *v, void *opaque,
+                               const char *name, Error **errp)
+{
+    PCNVDIMMDevice *nvdimm = PC_NVDIMM(obj);
+    uint64_t value = nvdimm->size;
+    visit_type_size(v, &value, name, errp);
+}
+
+static void pc_nvdimm_set_size(Object *obj, Visitor *v, void *opaque,
+                               const char *name, Error **errp)
+{
+    PCNVDIMMDevice *nvdimm = PC_NVDIMM(obj);
+    Error *local_err = NULL;
+    uint64_t value;
+
+    if (memory_region_size(&nvdimm->mr)) {
+        error_setg(&local_err, "cannot change property value");
+        goto out;
+    }
+
+    visit_type_size(v, &value, name, &local_err);
+    if (local_err) {
+        goto out;
+    }
+    if (!value) {
+        error_setg(&local_err, "Property '%s.%s' doesn't take value '%"
+                   PRIu64 "'", object_get_typename(obj), name, value);
+        goto out;
+    }
+    nvdimm->size = value << 20;
+ out:
+    error_propagate(errp, local_err);
+}
+
+static char *pc_nvdimm_get_file(Object *obj, Error **errp)
+{
+    PCNVDIMMDevice *nvdimm = PC_NVDIMM(obj);
+    return g_strdup(nvdimm->file);
+}
+
+static void pc_nvdimm_set_file(Object *obj, const char *str, Error **errp)
+{
+    PCNVDIMMDevice *nvdimm = PC_NVDIMM(obj);
+    if (nvdimm->file) {
+        g_free(nvdimm->file);
+    }
+    nvdimm->file = g_strdup(str);
+}
+
+static void pc_nvdimm_init(Object *obj)
+{
+    object_property_add(obj, PC_NVDIMM_ADDR_PROP, "int", pc_nvdimm_get_addr,
+                        NULL, NULL, NULL, &error_abort);
+    object_property_add(obj, PC_NVDIMM_SLOT_PROP, "int", pc_nvdimm_get_slot,
+                        NULL, NULL, NULL, &error_abort);
+    object_property_add(obj, PC_NVDIMM_NODE_PROP, "int", pc_nvdimm_get_node,
+                        NULL, NULL, NULL, &error_abort);
+    object_property_add(obj, PC_NVDIMM_SIZE_PROP, "int", pc_nvdimm_get_size,
+                        pc_nvdimm_set_size, NULL, NULL, &error_abort);
+    object_property_add_str(obj, PC_NVDIMM_FILE_PROP,
+                            pc_nvdimm_get_file, pc_nvdimm_set_file, NULL);
+}
+
+static void pc_nvdimm_realize(DeviceState *dev, Error **errp)
+{
+    PCNVDIMMDevice *nvdimm = PC_NVDIMM(dev);
+    MemoryRegion *nvdimm_mr = &nvdimm->mr;
+    char name[13] = { 0 };
+    void *buf;
+    ram_addr_t addr;
+    uint64_t size = nvdimm->size;
+    int fd;
+
+    if (!xen_enabled()) {
+        error_setg(errp, "xen is not enabled");
+        return;
+    }
+
+    if (!nvdimm->file) {
+        error_setg(errp, "file property is not set");
+        return;
+    }
+    if (!size) {
+        error_setg(errp, "size property is not set");
+        return;
+    }
+
+    fd = open(nvdimm->file, O_RDWR);
+    if (fd < 0) {
+        error_setg(errp, "can not open %s", nvdimm->file);
+        return;
+    }
+
+    buf = qemu_ram_mmap(fd, size, PC_NVDIMM_ADDR_ALIGN, true);
+    if (buf == MAP_FAILED) {
+        error_setg(errp, "can not do mmap on %s", nvdimm->file);
+        goto do_close;
+    }
+
+    addr = pc_nvdimm_reserved_range_push(size);
+    if (!addr) {
+        error_setg(errp, "do not have enough space for size %#lx.\n", size);
+        goto do_unmap;
+    }
+    nvdimm->addr = addr;
+
+    nvdimm->dev_idx = pc_nvdimm_new_device_index();
+    sprintf(name, "xen.nvdimm%02x", nvdimm->dev_idx);
+    memory_region_init_ram_ptr(nvdimm_mr, NULL, name, size, buf);
+    vmstate_register_ram(nvdimm_mr, DEVICE(dev));
+    memory_region_add_subregion(get_system_memory(), addr, nvdimm_mr);
+
+    return;
+
+ do_unmap:
+    qemu_ram_munmap(buf, size);
+ do_close:
+    close(fd);
+}
+
+static void pc_nvdimm_class_init(ObjectClass *oc, void *data)
+{
+    DeviceClass *dc = DEVICE_CLASS(oc);
+
+    /* nvdimm hotplug has not been supported yet. */
+    dc->hotpluggable = false;
+
+    dc->realize = pc_nvdimm_realize;
+    dc->desc = "NVDIMM memory module";
+}
+
+static TypeInfo pc_nvdimm_info = {
+    .name          = TYPE_PC_NVDIMM,
+    .parent        = TYPE_DEVICE,
+    .instance_size = sizeof(PCNVDIMMDevice),
+    .instance_init = pc_nvdimm_init,
+    .class_init    = pc_nvdimm_class_init,
+};
+
+static void pc_nvdimm_register_types(void)
+{
+    type_register_static(&pc_nvdimm_info);
+}
+
+type_init(pc_nvdimm_register_types)
diff --git a/include/hw/mem/pc-nvdimm.h b/include/hw/mem/pc-nvdimm.h
new file mode 100644
index 0000000..797ea02
--- /dev/null
+++ b/include/hw/mem/pc-nvdimm.h
@@ -0,0 +1,49 @@
+/*
+ * NVDIMM (A Non-Volatile Dual In-line Memory Module) Virtualization Implement
+ *
+ * Copyright(C) 2015 Intel Corporation.
+ *
+ * Author:
+ *  Xiao Guangrong <guangrong.xiao@linux.intel.com>
+ *  Haozhong Zhang <haozhong.zhang@intel.com>
+ *
+ * This work is licensed under the terms of the GNU GPL, version 2 or later.
+ * See the COPYING file in the top-level directory.
+ */
+
+#ifndef QEMU_PC_NVDIMM_H
+#define QEMU_PC_NVDIMM_H
+
+#include "hw/qdev.h"
+#include "hw/mem/pc-dimm.h"
+#include "exec/memory.h"
+
+/* Xen is incompatible with memory management of pc-dimm,
+ * so fallback to a standalone device type and manage memory
+ * by itself.
+ */
+
+typedef struct PCNVDIMMDevice {
+    /* private */
+    DeviceState parent_obj;
+
+    char *file;
+    MemoryRegion mr;
+    uint64_t addr;
+    uint64_t size;
+    int dev_idx;
+} PCNVDIMMDevice;
+
+#define TYPE_PC_NVDIMM      "pc-nvdimm"
+#define PC_NVDIMM(obj) \
+    OBJECT_CHECK(PCNVDIMMDevice, (obj), TYPE_PC_NVDIMM)
+
+#define PC_NVDIMM_ADDR_PROP PC_DIMM_ADDR_PROP
+#define PC_NVDIMM_SLOT_PROP PC_DIMM_SLOT_PROP
+#define PC_NVDIMM_NODE_PROP PC_DIMM_NODE_PROP
+#define PC_NVDIMM_SIZE_PROP PC_DIMM_SIZE_PROP
+#define PC_NVDIMM_FILE_PROP "file"
+
+void pc_nvdimm_reserve_range(ram_addr_t offset);
+
+#endif
diff --git a/xen-hvm.c b/xen-hvm.c
index 3d78a0c..6ebf43f 100644
--- a/xen-hvm.c
+++ b/xen-hvm.c
@@ -236,6 +236,8 @@ static void xen_ram_init(PCMachineState *pcms,
                                  pcms->above_4g_mem_size);
         memory_region_add_subregion(sysmem, 0x100000000ULL, &ram_hi);
     }
+
+    pc_nvdimm_reserve_range((1ULL << 32) + pcms->above_4g_mem_size);
 }
 
 void xen_ram_alloc(ram_addr_t ram_addr, ram_addr_t size, MemoryRegion *mr)
-- 
2.4.8

[PATCH 2/2] pc-nvdimm acpi: build ACPI tables for pc-nvdimm devices

[Haozhong Zhang]

Reuse existing NVDIMM ACPI code to build ACPI tables for pc-nvdimm
devices. The resulting tables are then copied into Xen guest domain so
tha they can be later loaded by Xen hvmloader.

Signed-off-by: Haozhong Zhang <haozhong.zhang@intel.com>
---
 hw/acpi/nvdimm.c     |  5 +++-
 hw/i386/pc.c         |  6 ++++-
 include/hw/xen/xen.h |  2 ++
 xen-hvm.c            | 71 ++++++++++++++++++++++++++++++++++++++++++++++++++++
 4 files changed, 82 insertions(+), 2 deletions(-)

diff --git a/hw/acpi/nvdimm.c b/hw/acpi/nvdimm.c
index df1b176..7c4b931 100644
--- a/hw/acpi/nvdimm.c
+++ b/hw/acpi/nvdimm.c
@@ -29,12 +29,15 @@
 #include "hw/acpi/acpi.h"
 #include "hw/acpi/aml-build.h"
 #include "hw/mem/nvdimm.h"
+#include "hw/mem/pc-nvdimm.h"
+#include "hw/xen/xen.h"
 
 static int nvdimm_plugged_device_list(Object *obj, void *opaque)
 {
     GSList **list = opaque;
+    const char *type_name = xen_enabled() ? TYPE_PC_NVDIMM : TYPE_NVDIMM;
 
-    if (object_dynamic_cast(obj, TYPE_NVDIMM)) {
+    if (object_dynamic_cast(obj, type_name)) {
         DeviceState *dev = DEVICE(obj);
 
         if (dev->realized) { /* only realized NVDIMMs matter */
diff --git a/hw/i386/pc.c b/hw/i386/pc.c
index 459260b..fadacf5 100644
--- a/hw/i386/pc.c
+++ b/hw/i386/pc.c
@@ -1186,7 +1186,11 @@ void pc_guest_info_machine_done(Notifier *notifier, void *data)
         }
     }
 
-    acpi_setup(&guest_info_state->info);
+    if (!xen_enabled()) {
+        acpi_setup(&guest_info_state->info);
+    } else if (xen_hvm_acpi_setup(PC_MACHINE(qdev_get_machine()))) {
+        error_report("Warning: failed to initialize Xen HVM ACPI tables");
+    }
 }
 
 PcGuestInfo *pc_guest_info_init(PCMachineState *pcms)
diff --git a/include/hw/xen/xen.h b/include/hw/xen/xen.h
index e90931a..8b705e1 100644
--- a/include/hw/xen/xen.h
+++ b/include/hw/xen/xen.h
@@ -51,4 +51,6 @@ void xen_register_framebuffer(struct MemoryRegion *mr);
 #  define HVM_MAX_VCPUS 32
 #endif
 
+int xen_hvm_acpi_setup(PCMachineState *pcms);
+
 #endif /* QEMU_HW_XEN_H */
diff --git a/xen-hvm.c b/xen-hvm.c
index 6ebf43f..f1f5e77 100644
--- a/xen-hvm.c
+++ b/xen-hvm.c
@@ -26,6 +26,13 @@
 #include <xen/hvm/params.h>
 #include <xen/hvm/e820.h>
 
+#include "qemu/error-report.h"
+#include "hw/acpi/acpi.h"
+#include "hw/acpi/aml-build.h"
+#include "hw/acpi/bios-linker-loader.h"
+#include "hw/mem/nvdimm.h"
+#include "hw/mem/pc-nvdimm.h"
+
 //#define DEBUG_XEN_HVM
 
 #ifdef DEBUG_XEN_HVM
@@ -1330,6 +1337,70 @@ int xen_hvm_init(PCMachineState *pcms,
     return 0;
 }
 
+int xen_hvm_acpi_setup(PCMachineState *pcms)
+{
+    AcpiBuildTables *hvm_acpi_tables;
+    GArray *tables_blob, *table_offsets;
+
+    ram_addr_t acpi_tables_addr, acpi_tables_size;
+    void *host;
+
+    struct xs_handle *xs = NULL;
+    char path[80], value[17];
+
+    if (!pcms->nvdimm) {
+        return 0;
+    }
+
+    hvm_acpi_tables = g_malloc0(sizeof(AcpiBuildTables));
+    if (!hvm_acpi_tables) {
+        return -1;
+    }
+    acpi_build_tables_init(hvm_acpi_tables);
+    tables_blob = hvm_acpi_tables->table_data;
+    table_offsets = g_array_new(false, true, sizeof(uint32_t));
+    bios_linker_loader_alloc(hvm_acpi_tables->linker,
+                             ACPI_BUILD_TABLE_FILE, 64, false);
+
+    /* build NFIT tables */
+    nvdimm_build_acpi(table_offsets, tables_blob, hvm_acpi_tables->linker);
+    g_array_free(table_offsets, true);
+
+    /* copy APCI tables into VM */
+    acpi_tables_size = tables_blob->len;
+    acpi_tables_addr =
+        (pcms->below_4g_mem_size - acpi_tables_size) & XC_PAGE_MASK;
+    host = xc_map_foreign_range(xen_xc, xen_domid,
+                                ROUND_UP(acpi_tables_size, XC_PAGE_SIZE),
+                                PROT_READ | PROT_WRITE,
+                                acpi_tables_addr >> XC_PAGE_SHIFT);
+    memcpy(host, tables_blob->data, acpi_tables_size);
+    munmap(host, ROUND_UP(acpi_tables_size, XC_PAGE_SIZE));
+
+    /* write address and size of ACPI tables to xenstore */
+    xs = xs_open(0);
+    if (xs == NULL) {
+        error_report("could not contact XenStore\n");
+        return -1;
+    }
+    snprintf(path, sizeof(path),
+             "/local/domain/%d/hvmloader/dm-acpi/address", xen_domid);
+    snprintf(value, sizeof(value), "%"PRIu64, (uint64_t) acpi_tables_addr);
+    if (!xs_write(xs, 0, path, value, strlen(value))) {
+        error_report("failed to write NFIT base address to xenstore\n");
+        return -1;
+    }
+    snprintf(path, sizeof(path),
+             "/local/domain/%d/hvmloader/dm-acpi/length", xen_domid);
+    snprintf(value, sizeof(value), "%"PRIu64, (uint64_t) acpi_tables_size);
+    if (!xs_write(xs, 0, path, value, strlen(value))) {
+        error_report("failed to write NFIT size to xenstore\n");
+        return -1;
+    }
+
+    return 0;
+}
+
 void destroy_hvm_domain(bool reboot)
 {
     XenXC xc_handle;
-- 
2.4.8

Re: [PATCH 0/2] add vNVDIMM support for Xen

[Xiao Guangrong]

On 12/29/2015 07:28 PM, Haozhong Zhang wrote:
> This patch series extends current vNVDIMM implementation to provide
> vNVDIMM to HVM domains when QEMU is used as the device model of Xen.
>
> This patch series is based on upstream qemu rather of qemu-xen,
> because vNVDIMM support has not been in qemu-xen (commit f165e58).
>
> * Following two problem that prevent Xen from directly using current
>    implementation are solved by this patch series.
>
>   (1) The current way to allocate memory for pc-dimm based vNVDIMM
>       through file-backend device is not compatible with Xen. Patch 2
>       adds another pc-nvdimm device to manage vNVDIMM's memory by
>       itself.
>

Let's figure a way to reuse current NVDIMM device mode, two modes
completely make the implementation complex and unmaintainable.

Also, the way reserving memory region for NVDIMM ACPI needs to be
adjusted and that is i am currently working on.

Re: [PATCH 0/2] add vNVDIMM support for Xen

[Stefano Stabellini]

On Tue, 29 Dec 2015, Haozhong Zhang wrote:
> This patch series extends current vNVDIMM implementation to provide
> vNVDIMM to HVM domains when QEMU is used as the device model of Xen.
> 
> This patch series is based on upstream qemu rather of qemu-xen,
> because vNVDIMM support has not been in qemu-xen (commit f165e58).
> 
> * Following two problem that prevent Xen from directly using current
>   implementation are solved by this patch series.
> 
>  (1) The current way to allocate memory for pc-dimm based vNVDIMM
>      through file-backend device is not compatible with Xen. Patch 2
>      adds another pc-nvdimm device to manage vNVDIMM's memory by
>      itself.
>     
>      A pc-nvdimm device can be used only with Xen and is specified by
>      parameters like
>             -device pc-nvdimm,file=/dev/pmem0,size=MBYTES
> 	    
>  (2) Xen uses its hvmloader rather than QEMU to build guest ACPI
>      tables. In order to reuse as much code as possible, patch 2 calls
>      the existing QEMU code to build guest ACPI tables for pc-nvdimm
>      devices and passes them to hvmloader.

I don't think that is acceptable: this would introduce a VM build time
dependency between QEMU and hvmloader which I think is undesirable.

Please note that Anthony is working on a way to pass ACPI tables from
the toolstack to hvmloader: 

http://marc.info/?l=xen-devel&m=144587582606159

I would build this work on top of his series.


> * Test
>  (1) A patched Xen is used for test. Xen patch series is sent
>      separately with title "[PATCH 0/4] add support for vNVDIMM".
>       
>  (2) Prepare a memory backend file:
>             dd if=/dev/zero of=/tmp/nvm0 bs=1G count=10
> 
>  (3) Add the following line to a HVM domain's configuration xl.cfg:
>             nvdimm =[ 'file=/tmp/nvm0,size=10240' ]
> 
>  (4) Launch a HVM domain from above xl.cfg.
> 
>  (5) If guest Linux kernel is 4.2 or newer and kernel modules
>      libnvdimm, nfit, nd_btt and nd_pmem are loaded, then you will see
>      the whole nvdimm device used as a single namespace and /dev/pmem0
>      will appear.
> 
> 
> 
> Haozhong Zhang (2):
>   pc-nvdimm: implement pc-nvdimm device abstract
>   pc-nvdimm acpi: build ACPI tables for pc-nvdimm devices
> 
>  hw/acpi/nvdimm.c           |   5 +-
>  hw/i386/pc.c               |   6 +-
>  hw/mem/Makefile.objs       |   1 +
>  hw/mem/pc-nvdimm.c         | 239 +++++++++++++++++++++++++++++++++++++++++++++
>  include/hw/mem/pc-nvdimm.h |  49 ++++++++++
>  include/hw/xen/xen.h       |   2 +
>  xen-hvm.c                  |  73 ++++++++++++++
>  7 files changed, 373 insertions(+), 2 deletions(-)
>  create mode 100644 hw/mem/pc-nvdimm.c
>  create mode 100644 include/hw/mem/pc-nvdimm.h
> 
> -- 
> 2.4.8
> 

Re: [PATCH 0/2] add vNVDIMM support for Xen

[Stefano Stabellini]

On Tue, 29 Dec 2015, Xiao Guangrong wrote:
> On 12/29/2015 07:28 PM, Haozhong Zhang wrote:
> > This patch series extends current vNVDIMM implementation to provide
> > vNVDIMM to HVM domains when QEMU is used as the device model of Xen.
> > 
> > This patch series is based on upstream qemu rather of qemu-xen,
> > because vNVDIMM support has not been in qemu-xen (commit f165e58).
> > 
> > * Following two problem that prevent Xen from directly using current
> >    implementation are solved by this patch series.
> > 
> >   (1) The current way to allocate memory for pc-dimm based vNVDIMM
> >       through file-backend device is not compatible with Xen. Patch 2
> >       adds another pc-nvdimm device to manage vNVDIMM's memory by
> >       itself.
> > 
> 
> Let's figure a way to reuse current NVDIMM device mode, two modes
> completely make the implementation complex and unmaintainable.

I agree


> Also, the way reserving memory region for NVDIMM ACPI needs to be
> adjusted and that is i am currently working on.

Re: [PATCH 2/2] pc-nvdimm acpi: build ACPI tables for pc-nvdimm devices

[Stefano Stabellini]

CC'ing the Xen tools maintainers and Anthony.

On Tue, 29 Dec 2015, Haozhong Zhang wrote:
> Reuse existing NVDIMM ACPI code to build ACPI tables for pc-nvdimm
> devices. The resulting tables are then copied into Xen guest domain so
> tha they can be later loaded by Xen hvmloader.
> 
> Signed-off-by: Haozhong Zhang <haozhong.zhang@intel.com>

How much work would it be to generate the nvdimm acpi tables from the
Xen toolstack?

Getting the tables from QEMU doesn't seem like a good idea to me, unless
we start getting the whole set of ACPI tables that way.


>  hw/acpi/nvdimm.c     |  5 +++-
>  hw/i386/pc.c         |  6 ++++-
>  include/hw/xen/xen.h |  2 ++
>  xen-hvm.c            | 71 ++++++++++++++++++++++++++++++++++++++++++++++++++++
>  4 files changed, 82 insertions(+), 2 deletions(-)
> 
> diff --git a/hw/acpi/nvdimm.c b/hw/acpi/nvdimm.c
> index df1b176..7c4b931 100644
> --- a/hw/acpi/nvdimm.c
> +++ b/hw/acpi/nvdimm.c
> @@ -29,12 +29,15 @@
>  #include "hw/acpi/acpi.h"
>  #include "hw/acpi/aml-build.h"
>  #include "hw/mem/nvdimm.h"
> +#include "hw/mem/pc-nvdimm.h"
> +#include "hw/xen/xen.h"
>  
>  static int nvdimm_plugged_device_list(Object *obj, void *opaque)
>  {
>      GSList **list = opaque;
> +    const char *type_name = xen_enabled() ? TYPE_PC_NVDIMM : TYPE_NVDIMM;
>  
> -    if (object_dynamic_cast(obj, TYPE_NVDIMM)) {
> +    if (object_dynamic_cast(obj, type_name)) {
>          DeviceState *dev = DEVICE(obj);
>  
>          if (dev->realized) { /* only realized NVDIMMs matter */
> diff --git a/hw/i386/pc.c b/hw/i386/pc.c
> index 459260b..fadacf5 100644
> --- a/hw/i386/pc.c
> +++ b/hw/i386/pc.c
> @@ -1186,7 +1186,11 @@ void pc_guest_info_machine_done(Notifier *notifier, void *data)
>          }
>      }
>  
> -    acpi_setup(&guest_info_state->info);
> +    if (!xen_enabled()) {
> +        acpi_setup(&guest_info_state->info);
> +    } else if (xen_hvm_acpi_setup(PC_MACHINE(qdev_get_machine()))) {
> +        error_report("Warning: failed to initialize Xen HVM ACPI tables");
> +    }
>  }
>  
>  PcGuestInfo *pc_guest_info_init(PCMachineState *pcms)
> diff --git a/include/hw/xen/xen.h b/include/hw/xen/xen.h
> index e90931a..8b705e1 100644
> --- a/include/hw/xen/xen.h
> +++ b/include/hw/xen/xen.h
> @@ -51,4 +51,6 @@ void xen_register_framebuffer(struct MemoryRegion *mr);
>  #  define HVM_MAX_VCPUS 32
>  #endif
>  
> +int xen_hvm_acpi_setup(PCMachineState *pcms);
> +
>  #endif /* QEMU_HW_XEN_H */
> diff --git a/xen-hvm.c b/xen-hvm.c
> index 6ebf43f..f1f5e77 100644
> --- a/xen-hvm.c
> +++ b/xen-hvm.c
> @@ -26,6 +26,13 @@
>  #include <xen/hvm/params.h>
>  #include <xen/hvm/e820.h>
>  
> +#include "qemu/error-report.h"
> +#include "hw/acpi/acpi.h"
> +#include "hw/acpi/aml-build.h"
> +#include "hw/acpi/bios-linker-loader.h"
> +#include "hw/mem/nvdimm.h"
> +#include "hw/mem/pc-nvdimm.h"
> +
>  //#define DEBUG_XEN_HVM
>  
>  #ifdef DEBUG_XEN_HVM
> @@ -1330,6 +1337,70 @@ int xen_hvm_init(PCMachineState *pcms,
>      return 0;
>  }
>  
> +int xen_hvm_acpi_setup(PCMachineState *pcms)
> +{
> +    AcpiBuildTables *hvm_acpi_tables;
> +    GArray *tables_blob, *table_offsets;
> +
> +    ram_addr_t acpi_tables_addr, acpi_tables_size;
> +    void *host;
> +
> +    struct xs_handle *xs = NULL;
> +    char path[80], value[17];
> +
> +    if (!pcms->nvdimm) {
> +        return 0;
> +    }
> +
> +    hvm_acpi_tables = g_malloc0(sizeof(AcpiBuildTables));
> +    if (!hvm_acpi_tables) {
> +        return -1;
> +    }
> +    acpi_build_tables_init(hvm_acpi_tables);
> +    tables_blob = hvm_acpi_tables->table_data;
> +    table_offsets = g_array_new(false, true, sizeof(uint32_t));
> +    bios_linker_loader_alloc(hvm_acpi_tables->linker,
> +                             ACPI_BUILD_TABLE_FILE, 64, false);
> +
> +    /* build NFIT tables */
> +    nvdimm_build_acpi(table_offsets, tables_blob, hvm_acpi_tables->linker);
> +    g_array_free(table_offsets, true);
> +
> +    /* copy APCI tables into VM */
> +    acpi_tables_size = tables_blob->len;
> +    acpi_tables_addr =
> +        (pcms->below_4g_mem_size - acpi_tables_size) & XC_PAGE_MASK;
> +    host = xc_map_foreign_range(xen_xc, xen_domid,
> +                                ROUND_UP(acpi_tables_size, XC_PAGE_SIZE),
> +                                PROT_READ | PROT_WRITE,
> +                                acpi_tables_addr >> XC_PAGE_SHIFT);
> +    memcpy(host, tables_blob->data, acpi_tables_size);
> +    munmap(host, ROUND_UP(acpi_tables_size, XC_PAGE_SIZE));
> +
> +    /* write address and size of ACPI tables to xenstore */
> +    xs = xs_open(0);
> +    if (xs == NULL) {
> +        error_report("could not contact XenStore\n");
> +        return -1;
> +    }
> +    snprintf(path, sizeof(path),
> +             "/local/domain/%d/hvmloader/dm-acpi/address", xen_domid);
> +    snprintf(value, sizeof(value), "%"PRIu64, (uint64_t) acpi_tables_addr);
> +    if (!xs_write(xs, 0, path, value, strlen(value))) {
> +        error_report("failed to write NFIT base address to xenstore\n");
> +        return -1;
> +    }
> +    snprintf(path, sizeof(path),
> +             "/local/domain/%d/hvmloader/dm-acpi/length", xen_domid);
> +    snprintf(value, sizeof(value), "%"PRIu64, (uint64_t) acpi_tables_size);
> +    if (!xs_write(xs, 0, path, value, strlen(value))) {
> +        error_report("failed to write NFIT size to xenstore\n");
> +        return -1;
> +    }
> +
> +    return 0;
> +}
> +
>  void destroy_hvm_domain(bool reboot)
>  {
>      XenXC xc_handle;
> -- 
> 2.4.8
> 
> 
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xen.org
> http://lists.xen.org/xen-devel
> 

Re: [PATCH 2/2] pc-nvdimm acpi: build ACPI tables for pc-nvdimm devices

[Konrad Rzeszutek Wilk]

On Mon, Jan 04, 2016 at 04:01:08PM +0000, Stefano Stabellini wrote:
> CC'ing the Xen tools maintainers and Anthony.
> 
> On Tue, 29 Dec 2015, Haozhong Zhang wrote:
> > Reuse existing NVDIMM ACPI code to build ACPI tables for pc-nvdimm
> > devices. The resulting tables are then copied into Xen guest domain so
> > tha they can be later loaded by Xen hvmloader.
> > 
> > Signed-off-by: Haozhong Zhang <haozhong.zhang@intel.com>
> 
> How much work would it be to generate the nvdimm acpi tables from the
> Xen toolstack?

Why duplicate the code? The QEMU generates the NFIT tables and its sub-tables.
> 
> Getting the tables from QEMU doesn't seem like a good idea to me, unless
> we start getting the whole set of ACPI tables that way.

There is also the ACPI DSDT code - which requires an memory region
to be reserved for the AML code to drop the parameters so that QEMU
can scan the NVDIMM for failures. The region (and size) should be
determined by QEMU since it works on this data.


> 
> 
> >  hw/acpi/nvdimm.c     |  5 +++-
> >  hw/i386/pc.c         |  6 ++++-
> >  include/hw/xen/xen.h |  2 ++
> >  xen-hvm.c            | 71 ++++++++++++++++++++++++++++++++++++++++++++++++++++
> >  4 files changed, 82 insertions(+), 2 deletions(-)
> > 
> > diff --git a/hw/acpi/nvdimm.c b/hw/acpi/nvdimm.c
> > index df1b176..7c4b931 100644
> > --- a/hw/acpi/nvdimm.c
> > +++ b/hw/acpi/nvdimm.c
> > @@ -29,12 +29,15 @@
> >  #include "hw/acpi/acpi.h"
> >  #include "hw/acpi/aml-build.h"
> >  #include "hw/mem/nvdimm.h"
> > +#include "hw/mem/pc-nvdimm.h"
> > +#include "hw/xen/xen.h"
> >  
> >  static int nvdimm_plugged_device_list(Object *obj, void *opaque)
> >  {
> >      GSList **list = opaque;
> > +    const char *type_name = xen_enabled() ? TYPE_PC_NVDIMM : TYPE_NVDIMM;
> >  
> > -    if (object_dynamic_cast(obj, TYPE_NVDIMM)) {
> > +    if (object_dynamic_cast(obj, type_name)) {
> >          DeviceState *dev = DEVICE(obj);
> >  
> >          if (dev->realized) { /* only realized NVDIMMs matter */
> > diff --git a/hw/i386/pc.c b/hw/i386/pc.c
> > index 459260b..fadacf5 100644
> > --- a/hw/i386/pc.c
> > +++ b/hw/i386/pc.c
> > @@ -1186,7 +1186,11 @@ void pc_guest_info_machine_done(Notifier *notifier, void *data)
> >          }
> >      }
> >  
> > -    acpi_setup(&guest_info_state->info);
> > +    if (!xen_enabled()) {
> > +        acpi_setup(&guest_info_state->info);
> > +    } else if (xen_hvm_acpi_setup(PC_MACHINE(qdev_get_machine()))) {
> > +        error_report("Warning: failed to initialize Xen HVM ACPI tables");
> > +    }
> >  }
> >  
> >  PcGuestInfo *pc_guest_info_init(PCMachineState *pcms)
> > diff --git a/include/hw/xen/xen.h b/include/hw/xen/xen.h
> > index e90931a..8b705e1 100644
> > --- a/include/hw/xen/xen.h
> > +++ b/include/hw/xen/xen.h
> > @@ -51,4 +51,6 @@ void xen_register_framebuffer(struct MemoryRegion *mr);
> >  #  define HVM_MAX_VCPUS 32
> >  #endif
> >  
> > +int xen_hvm_acpi_setup(PCMachineState *pcms);
> > +
> >  #endif /* QEMU_HW_XEN_H */
> > diff --git a/xen-hvm.c b/xen-hvm.c
> > index 6ebf43f..f1f5e77 100644
> > --- a/xen-hvm.c
> > +++ b/xen-hvm.c
> > @@ -26,6 +26,13 @@
> >  #include <xen/hvm/params.h>
> >  #include <xen/hvm/e820.h>
> >  
> > +#include "qemu/error-report.h"
> > +#include "hw/acpi/acpi.h"
> > +#include "hw/acpi/aml-build.h"
> > +#include "hw/acpi/bios-linker-loader.h"
> > +#include "hw/mem/nvdimm.h"
> > +#include "hw/mem/pc-nvdimm.h"
> > +
> >  //#define DEBUG_XEN_HVM
> >  
> >  #ifdef DEBUG_XEN_HVM
> > @@ -1330,6 +1337,70 @@ int xen_hvm_init(PCMachineState *pcms,
> >      return 0;
> >  }
> >  
> > +int xen_hvm_acpi_setup(PCMachineState *pcms)
> > +{
> > +    AcpiBuildTables *hvm_acpi_tables;
> > +    GArray *tables_blob, *table_offsets;
> > +
> > +    ram_addr_t acpi_tables_addr, acpi_tables_size;
> > +    void *host;
> > +
> > +    struct xs_handle *xs = NULL;
> > +    char path[80], value[17];
> > +
> > +    if (!pcms->nvdimm) {
> > +        return 0;
> > +    }
> > +
> > +    hvm_acpi_tables = g_malloc0(sizeof(AcpiBuildTables));
> > +    if (!hvm_acpi_tables) {
> > +        return -1;
> > +    }
> > +    acpi_build_tables_init(hvm_acpi_tables);
> > +    tables_blob = hvm_acpi_tables->table_data;
> > +    table_offsets = g_array_new(false, true, sizeof(uint32_t));
> > +    bios_linker_loader_alloc(hvm_acpi_tables->linker,
> > +                             ACPI_BUILD_TABLE_FILE, 64, false);
> > +
> > +    /* build NFIT tables */
> > +    nvdimm_build_acpi(table_offsets, tables_blob, hvm_acpi_tables->linker);
> > +    g_array_free(table_offsets, true);
> > +
> > +    /* copy APCI tables into VM */
> > +    acpi_tables_size = tables_blob->len;
> > +    acpi_tables_addr =
> > +        (pcms->below_4g_mem_size - acpi_tables_size) & XC_PAGE_MASK;
> > +    host = xc_map_foreign_range(xen_xc, xen_domid,
> > +                                ROUND_UP(acpi_tables_size, XC_PAGE_SIZE),
> > +                                PROT_READ | PROT_WRITE,
> > +                                acpi_tables_addr >> XC_PAGE_SHIFT);
> > +    memcpy(host, tables_blob->data, acpi_tables_size);
> > +    munmap(host, ROUND_UP(acpi_tables_size, XC_PAGE_SIZE));
> > +
> > +    /* write address and size of ACPI tables to xenstore */
> > +    xs = xs_open(0);
> > +    if (xs == NULL) {
> > +        error_report("could not contact XenStore\n");
> > +        return -1;
> > +    }
> > +    snprintf(path, sizeof(path),
> > +             "/local/domain/%d/hvmloader/dm-acpi/address", xen_domid);
> > +    snprintf(value, sizeof(value), "%"PRIu64, (uint64_t) acpi_tables_addr);
> > +    if (!xs_write(xs, 0, path, value, strlen(value))) {
> > +        error_report("failed to write NFIT base address to xenstore\n");
> > +        return -1;
> > +    }
> > +    snprintf(path, sizeof(path),
> > +             "/local/domain/%d/hvmloader/dm-acpi/length", xen_domid);
> > +    snprintf(value, sizeof(value), "%"PRIu64, (uint64_t) acpi_tables_size);
> > +    if (!xs_write(xs, 0, path, value, strlen(value))) {
> > +        error_report("failed to write NFIT size to xenstore\n");
> > +        return -1;
> > +    }
> > +
> > +    return 0;
> > +}
> > +
> >  void destroy_hvm_domain(bool reboot)
> >  {
> >      XenXC xc_handle;
> > -- 
> > 2.4.8
> > 
> > 
> > _______________________________________________
> > Xen-devel mailing list
> > Xen-devel@lists.xen.org
> > http://lists.xen.org/xen-devel
> > 
> 
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xen.org
> http://lists.xen.org/xen-devel

Re: [PATCH 0/2] add vNVDIMM support for Xen

[Haozhong Zhang]

On 01/04/16 15:57, Stefano Stabellini wrote:
> On Tue, 29 Dec 2015, Xiao Guangrong wrote:
> > On 12/29/2015 07:28 PM, Haozhong Zhang wrote:
> > > This patch series extends current vNVDIMM implementation to provide
> > > vNVDIMM to HVM domains when QEMU is used as the device model of Xen.
> > > 
> > > This patch series is based on upstream qemu rather of qemu-xen,
> > > because vNVDIMM support has not been in qemu-xen (commit f165e58).
> > > 
> > > * Following two problem that prevent Xen from directly using current
> > >    implementation are solved by this patch series.
> > > 
> > >   (1) The current way to allocate memory for pc-dimm based vNVDIMM
> > >       through file-backend device is not compatible with Xen. Patch 2
> > >       adds another pc-nvdimm device to manage vNVDIMM's memory by
> > >       itself.
> > > 
> > 
> > Let's figure a way to reuse current NVDIMM device mode, two modes
> > completely make the implementation complex and unmaintainable.
> 
> I agree
>
Yes, I'm looking at how to make the file-backend device work for Xen.

Haozhong

Re: [PATCH 0/2] add vNVDIMM support for Xen

[Haozhong Zhang]

On 01/04/16 15:56, Stefano Stabellini wrote:
> On Tue, 29 Dec 2015, Haozhong Zhang wrote:
> > This patch series extends current vNVDIMM implementation to provide
> > vNVDIMM to HVM domains when QEMU is used as the device model of Xen.
> > 
> > This patch series is based on upstream qemu rather of qemu-xen,
> > because vNVDIMM support has not been in qemu-xen (commit f165e58).
> > 
> > * Following two problem that prevent Xen from directly using current
> >   implementation are solved by this patch series.
> > 
> >  (1) The current way to allocate memory for pc-dimm based vNVDIMM
> >      through file-backend device is not compatible with Xen. Patch 2
> >      adds another pc-nvdimm device to manage vNVDIMM's memory by
> >      itself.
> >     
> >      A pc-nvdimm device can be used only with Xen and is specified by
> >      parameters like
> >             -device pc-nvdimm,file=/dev/pmem0,size=MBYTES
> > 	    
> >  (2) Xen uses its hvmloader rather than QEMU to build guest ACPI
> >      tables. In order to reuse as much code as possible, patch 2 calls
> >      the existing QEMU code to build guest ACPI tables for pc-nvdimm
> >      devices and passes them to hvmloader.
> 
> I don't think that is acceptable: this would introduce a VM build time
> dependency between QEMU and hvmloader which I think is undesirable.
>
Guess I should not say "calls ... QEMU code". In fact, QEMU copies
some ACPI tables into guest and writes its location and size in
xenstore so that hvmloader can find and load those tables. Because
hvmloader uses certain xenstore keys to find ACPI tables, I think it
does not tightly depend on QEMU (and those keys and ACPI tables could
also be prepared by other device models or Xen tool stack).

> Please note that Anthony is working on a way to pass ACPI tables from
> the toolstack to hvmloader: 
> 
> http://marc.info/?l=xen-devel&m=144587582606159
> 
> I would build this work on top of his series.
>
I'll look at this work.

Thanks,
Haozhong

> 
> > * Test
> >  (1) A patched Xen is used for test. Xen patch series is sent
> >      separately with title "[PATCH 0/4] add support for vNVDIMM".
> >       
> >  (2) Prepare a memory backend file:
> >             dd if=/dev/zero of=/tmp/nvm0 bs=1G count=10
> > 
> >  (3) Add the following line to a HVM domain's configuration xl.cfg:
> >             nvdimm =[ 'file=/tmp/nvm0,size=10240' ]
> > 
> >  (4) Launch a HVM domain from above xl.cfg.
> > 
> >  (5) If guest Linux kernel is 4.2 or newer and kernel modules
> >      libnvdimm, nfit, nd_btt and nd_pmem are loaded, then you will see
> >      the whole nvdimm device used as a single namespace and /dev/pmem0
> >      will appear.
> > 
> > 
> > 
> > Haozhong Zhang (2):
> >   pc-nvdimm: implement pc-nvdimm device abstract
> >   pc-nvdimm acpi: build ACPI tables for pc-nvdimm devices
> > 
> >  hw/acpi/nvdimm.c           |   5 +-
> >  hw/i386/pc.c               |   6 +-
> >  hw/mem/Makefile.objs       |   1 +
> >  hw/mem/pc-nvdimm.c         | 239 +++++++++++++++++++++++++++++++++++++++++++++
> >  include/hw/mem/pc-nvdimm.h |  49 ++++++++++
> >  include/hw/xen/xen.h       |   2 +
> >  xen-hvm.c                  |  73 ++++++++++++++
> >  7 files changed, 373 insertions(+), 2 deletions(-)
> >  create mode 100644 hw/mem/pc-nvdimm.c
> >  create mode 100644 include/hw/mem/pc-nvdimm.h
> > 
> > -- 
> > 2.4.8
> > 

Re: [PATCH 2/2] pc-nvdimm acpi: build ACPI tables for pc-nvdimm devices

[Haozhong Zhang]

On 01/04/16 16:01, Stefano Stabellini wrote:
> CC'ing the Xen tools maintainers and Anthony.
> 
> On Tue, 29 Dec 2015, Haozhong Zhang wrote:
> > Reuse existing NVDIMM ACPI code to build ACPI tables for pc-nvdimm
> > devices. The resulting tables are then copied into Xen guest domain so
> > tha they can be later loaded by Xen hvmloader.
> > 
> > Signed-off-by: Haozhong Zhang <haozhong.zhang@intel.com>
> 
> How much work would it be to generate the nvdimm acpi tables from the
> Xen toolstack?
>
I think at least two parts of work should be done if generating nvdimm
ACPI tables from toolstack:

(1) An AML builder.
    NVDIMM SSDT table contains a part of definition block, so an AML
    builder is required. If guest ACPI tables is going to be generated
    from toolstack, I think we can port QEMU's AML builder to
    toolstack.

(2) Interface to pass information of vNVDIMM devices from QEMU to Xen.
    Currently, QEMU is responsible to decide the slot IDs of vNVDIMM
    devices, the address space where vNVDIMM devices are mapped to and
    other stuffs that need to be written in ACPI tables. In the
    future, there could be more as planed, including information about
    NVDIMM label areas. If generating NVDIMM ACPI tables from Xen, all
    those information should be passed from QEMU.  Maybe we can pass
    them through xenstore.

> Getting the tables from QEMU doesn't seem like a good idea to me, unless
> we start getting the whole set of ACPI tables that way.
>
As most of current and future vNVDIMM implementation in QEMU would be
related to ACPI, I really want to reuse those code as much as possible.

Haozhong

Re: [PATCH 2/2] pc-nvdimm acpi: build ACPI tables for pc-nvdimm devices

[Stefano Stabellini]

On Mon, 4 Jan 2016, Konrad Rzeszutek Wilk wrote:
> On Mon, Jan 04, 2016 at 04:01:08PM +0000, Stefano Stabellini wrote:
> > CC'ing the Xen tools maintainers and Anthony.
> > 
> > On Tue, 29 Dec 2015, Haozhong Zhang wrote:
> > > Reuse existing NVDIMM ACPI code to build ACPI tables for pc-nvdimm
> > > devices. The resulting tables are then copied into Xen guest domain so
> > > tha they can be later loaded by Xen hvmloader.
> > > 
> > > Signed-off-by: Haozhong Zhang <haozhong.zhang@intel.com>
> > 
> > How much work would it be to generate the nvdimm acpi tables from the
> > Xen toolstack?
> 
> Why duplicate the code? The QEMU generates the NFIT tables and its sub-tables.
>
>
> > Getting the tables from QEMU doesn't seem like a good idea to me, unless
> > we start getting the whole set of ACPI tables that way.
> 
> There is also the ACPI DSDT code - which requires an memory region
> to be reserved for the AML code to drop the parameters so that QEMU
> can scan the NVDIMM for failures. The region (and size) should be
> determined by QEMU since it works on this data.

QEMU can generate the whole set of ACPI tables. Why should we take only
the nvdimm tables and not the others?

I don't think it is wise to have two components which both think are in
control of generating ACPI tables, hvmloader (soon to be the toolstack
with Anthony's work) and QEMU. From an architectural perspective, it
doesn't look robust to me.

Could we take this opportunity to switch to QEMU generating the whole
set of ACPI tables?
 
 
> > >  hw/acpi/nvdimm.c     |  5 +++-
> > >  hw/i386/pc.c         |  6 ++++-
> > >  include/hw/xen/xen.h |  2 ++
> > >  xen-hvm.c            | 71 ++++++++++++++++++++++++++++++++++++++++++++++++++++
> > >  4 files changed, 82 insertions(+), 2 deletions(-)
> > > 
> > > diff --git a/hw/acpi/nvdimm.c b/hw/acpi/nvdimm.c
> > > index df1b176..7c4b931 100644
> > > --- a/hw/acpi/nvdimm.c
> > > +++ b/hw/acpi/nvdimm.c
> > > @@ -29,12 +29,15 @@
> > >  #include "hw/acpi/acpi.h"
> > >  #include "hw/acpi/aml-build.h"
> > >  #include "hw/mem/nvdimm.h"
> > > +#include "hw/mem/pc-nvdimm.h"
> > > +#include "hw/xen/xen.h"
> > >  
> > >  static int nvdimm_plugged_device_list(Object *obj, void *opaque)
> > >  {
> > >      GSList **list = opaque;
> > > +    const char *type_name = xen_enabled() ? TYPE_PC_NVDIMM : TYPE_NVDIMM;
> > >  
> > > -    if (object_dynamic_cast(obj, TYPE_NVDIMM)) {
> > > +    if (object_dynamic_cast(obj, type_name)) {
> > >          DeviceState *dev = DEVICE(obj);
> > >  
> > >          if (dev->realized) { /* only realized NVDIMMs matter */
> > > diff --git a/hw/i386/pc.c b/hw/i386/pc.c
> > > index 459260b..fadacf5 100644
> > > --- a/hw/i386/pc.c
> > > +++ b/hw/i386/pc.c
> > > @@ -1186,7 +1186,11 @@ void pc_guest_info_machine_done(Notifier *notifier, void *data)
> > >          }
> > >      }
> > >  
> > > -    acpi_setup(&guest_info_state->info);
> > > +    if (!xen_enabled()) {
> > > +        acpi_setup(&guest_info_state->info);
> > > +    } else if (xen_hvm_acpi_setup(PC_MACHINE(qdev_get_machine()))) {
> > > +        error_report("Warning: failed to initialize Xen HVM ACPI tables");
> > > +    }
> > >  }
> > >  
> > >  PcGuestInfo *pc_guest_info_init(PCMachineState *pcms)
> > > diff --git a/include/hw/xen/xen.h b/include/hw/xen/xen.h
> > > index e90931a..8b705e1 100644
> > > --- a/include/hw/xen/xen.h
> > > +++ b/include/hw/xen/xen.h
> > > @@ -51,4 +51,6 @@ void xen_register_framebuffer(struct MemoryRegion *mr);
> > >  #  define HVM_MAX_VCPUS 32
> > >  #endif
> > >  
> > > +int xen_hvm_acpi_setup(PCMachineState *pcms);
> > > +
> > >  #endif /* QEMU_HW_XEN_H */
> > > diff --git a/xen-hvm.c b/xen-hvm.c
> > > index 6ebf43f..f1f5e77 100644
> > > --- a/xen-hvm.c
> > > +++ b/xen-hvm.c
> > > @@ -26,6 +26,13 @@
> > >  #include <xen/hvm/params.h>
> > >  #include <xen/hvm/e820.h>
> > >  
> > > +#include "qemu/error-report.h"
> > > +#include "hw/acpi/acpi.h"
> > > +#include "hw/acpi/aml-build.h"
> > > +#include "hw/acpi/bios-linker-loader.h"
> > > +#include "hw/mem/nvdimm.h"
> > > +#include "hw/mem/pc-nvdimm.h"
> > > +
> > >  //#define DEBUG_XEN_HVM
> > >  
> > >  #ifdef DEBUG_XEN_HVM
> > > @@ -1330,6 +1337,70 @@ int xen_hvm_init(PCMachineState *pcms,
> > >      return 0;
> > >  }
> > >  
> > > +int xen_hvm_acpi_setup(PCMachineState *pcms)
> > > +{
> > > +    AcpiBuildTables *hvm_acpi_tables;
> > > +    GArray *tables_blob, *table_offsets;
> > > +
> > > +    ram_addr_t acpi_tables_addr, acpi_tables_size;
> > > +    void *host;
> > > +
> > > +    struct xs_handle *xs = NULL;
> > > +    char path[80], value[17];
> > > +
> > > +    if (!pcms->nvdimm) {
> > > +        return 0;
> > > +    }
> > > +
> > > +    hvm_acpi_tables = g_malloc0(sizeof(AcpiBuildTables));
> > > +    if (!hvm_acpi_tables) {
> > > +        return -1;
> > > +    }
> > > +    acpi_build_tables_init(hvm_acpi_tables);
> > > +    tables_blob = hvm_acpi_tables->table_data;
> > > +    table_offsets = g_array_new(false, true, sizeof(uint32_t));
> > > +    bios_linker_loader_alloc(hvm_acpi_tables->linker,
> > > +                             ACPI_BUILD_TABLE_FILE, 64, false);
> > > +
> > > +    /* build NFIT tables */
> > > +    nvdimm_build_acpi(table_offsets, tables_blob, hvm_acpi_tables->linker);
> > > +    g_array_free(table_offsets, true);
> > > +
> > > +    /* copy APCI tables into VM */
> > > +    acpi_tables_size = tables_blob->len;
> > > +    acpi_tables_addr =
> > > +        (pcms->below_4g_mem_size - acpi_tables_size) & XC_PAGE_MASK;
> > > +    host = xc_map_foreign_range(xen_xc, xen_domid,
> > > +                                ROUND_UP(acpi_tables_size, XC_PAGE_SIZE),
> > > +                                PROT_READ | PROT_WRITE,
> > > +                                acpi_tables_addr >> XC_PAGE_SHIFT);
> > > +    memcpy(host, tables_blob->data, acpi_tables_size);
> > > +    munmap(host, ROUND_UP(acpi_tables_size, XC_PAGE_SIZE));
> > > +
> > > +    /* write address and size of ACPI tables to xenstore */
> > > +    xs = xs_open(0);
> > > +    if (xs == NULL) {
> > > +        error_report("could not contact XenStore\n");
> > > +        return -1;
> > > +    }
> > > +    snprintf(path, sizeof(path),
> > > +             "/local/domain/%d/hvmloader/dm-acpi/address", xen_domid);
> > > +    snprintf(value, sizeof(value), "%"PRIu64, (uint64_t) acpi_tables_addr);
> > > +    if (!xs_write(xs, 0, path, value, strlen(value))) {
> > > +        error_report("failed to write NFIT base address to xenstore\n");
> > > +        return -1;
> > > +    }
> > > +    snprintf(path, sizeof(path),
> > > +             "/local/domain/%d/hvmloader/dm-acpi/length", xen_domid);
> > > +    snprintf(value, sizeof(value), "%"PRIu64, (uint64_t) acpi_tables_size);
> > > +    if (!xs_write(xs, 0, path, value, strlen(value))) {
> > > +        error_report("failed to write NFIT size to xenstore\n");
> > > +        return -1;
> > > +    }
> > > +
> > > +    return 0;
> > > +}
> > > +
> > >  void destroy_hvm_domain(bool reboot)
> > >  {
> > >      XenXC xc_handle;
> > > -- 
> > > 2.4.8
> > > 
> > > 
> > > _______________________________________________
> > > Xen-devel mailing list
> > > Xen-devel@lists.xen.org
> > > http://lists.xen.org/xen-devel
> > > 
> > 
> > _______________________________________________
> > Xen-devel mailing list
> > Xen-devel@lists.xen.org
> > http://lists.xen.org/xen-devel
> 
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xen.org
> http://lists.xen.org/xen-devel
> 

Re: [PATCH 2/2] pc-nvdimm acpi: build ACPI tables for pc-nvdimm devices

[Haozhong Zhang]

On 01/05/16 11:00, Stefano Stabellini wrote:
> On Mon, 4 Jan 2016, Konrad Rzeszutek Wilk wrote:
> > On Mon, Jan 04, 2016 at 04:01:08PM +0000, Stefano Stabellini wrote:
> > > CC'ing the Xen tools maintainers and Anthony.
> > > 
> > > On Tue, 29 Dec 2015, Haozhong Zhang wrote:
> > > > Reuse existing NVDIMM ACPI code to build ACPI tables for pc-nvdimm
> > > > devices. The resulting tables are then copied into Xen guest domain so
> > > > tha they can be later loaded by Xen hvmloader.
> > > > 
> > > > Signed-off-by: Haozhong Zhang <haozhong.zhang@intel.com>
> > > 
> > > How much work would it be to generate the nvdimm acpi tables from the
> > > Xen toolstack?
> > 
> > Why duplicate the code? The QEMU generates the NFIT tables and its sub-tables.
> >
> >
> > > Getting the tables from QEMU doesn't seem like a good idea to me, unless
> > > we start getting the whole set of ACPI tables that way.
> > 
> > There is also the ACPI DSDT code - which requires an memory region
> > to be reserved for the AML code to drop the parameters so that QEMU
> > can scan the NVDIMM for failures. The region (and size) should be
> > determined by QEMU since it works on this data.
> 
> QEMU can generate the whole set of ACPI tables. Why should we take only
> the nvdimm tables and not the others?
>
NVDIMM tables are the only tables required to support vNVDIMM in this
patch series, and they are self-contained and not conflict with other
existing tables built by hvmloader. For other tables built by QEMU, I
have no idea whether they could work with Xen, so I only take nvdimm
tables from QEMU.

> I don't think it is wise to have two components which both think are in
> control of generating ACPI tables, hvmloader (soon to be the toolstack
> with Anthony's work) and QEMU. From an architectural perspective, it
> doesn't look robust to me.
>
Do you mean whenever nvdimm code in QEMU is changed, we would have to
make more efforts to ensure it still works with Xen?

> Could we take this opportunity to switch to QEMU generating the whole
> set of ACPI tables?
>
Not quite sure how much effort would be taken on this change. CCed
hvmloader maintainers for their comments.

Thanks,
Haozhong

Re: [PATCH 2/2] pc-nvdimm acpi: build ACPI tables for pc-nvdimm devices

[Konrad Rzeszutek Wilk]

On Tue, Jan 05, 2016 at 10:01:26PM +0800, Haozhong Zhang wrote:
> On 01/05/16 11:00, Stefano Stabellini wrote:
> > On Mon, 4 Jan 2016, Konrad Rzeszutek Wilk wrote:
> > > On Mon, Jan 04, 2016 at 04:01:08PM +0000, Stefano Stabellini wrote:
> > > > CC'ing the Xen tools maintainers and Anthony.
> > > > 
> > > > On Tue, 29 Dec 2015, Haozhong Zhang wrote:
> > > > > Reuse existing NVDIMM ACPI code to build ACPI tables for pc-nvdimm
> > > > > devices. The resulting tables are then copied into Xen guest domain so
> > > > > tha they can be later loaded by Xen hvmloader.
> > > > > 
> > > > > Signed-off-by: Haozhong Zhang <haozhong.zhang@intel.com>
> > > > 
> > > > How much work would it be to generate the nvdimm acpi tables from the
> > > > Xen toolstack?
> > > 
> > > Why duplicate the code? The QEMU generates the NFIT tables and its sub-tables.
> > >
> > >
> > > > Getting the tables from QEMU doesn't seem like a good idea to me, unless
> > > > we start getting the whole set of ACPI tables that way.
> > > 
> > > There is also the ACPI DSDT code - which requires an memory region
> > > to be reserved for the AML code to drop the parameters so that QEMU
> > > can scan the NVDIMM for failures. The region (and size) should be
> > > determined by QEMU since it works on this data.
> > 
> > QEMU can generate the whole set of ACPI tables. Why should we take only
> > the nvdimm tables and not the others?
> >
> NVDIMM tables are the only tables required to support vNVDIMM in this
> patch series, and they are self-contained and not conflict with other
> existing tables built by hvmloader. For other tables built by QEMU, I
> have no idea whether they could work with Xen, so I only take nvdimm
> tables from QEMU.

But you also have to deal with the SSDT code right? And I was under the
impression that if you use hvmloader - it loads the ACPI DSDT which
it had compiled at build-time - only - but not the SSDT?

In other way - just having the ACPI NFIT won't give you the whole
picture - unless you also jam in ACPI _DSM (ACPI0012) methods as
part of the SSDT right?

Or does the ACPI tables generation also include an ACPI SSDT with the
ACPI _DSM methods as part of it? I only see 'nvdimm_build_acpi' and
NFIT comments, nothing about SSDT?

> 
> > I don't think it is wise to have two components which both think are in
> > control of generating ACPI tables, hvmloader (soon to be the toolstack
> > with Anthony's work) and QEMU. From an architectural perspective, it
> > doesn't look robust to me.
> >
> Do you mean whenever nvdimm code in QEMU is changed, we would have to
> make more efforts to ensure it still works with Xen?

Not sure I follow that. How is it different from the efforts to ensure
that the patches here (which only add one ACPI MADT table)
provide the proper support? Wouldn't you do the same type of checking
every time you modify the nvdimm_build_acpi code?


> 
> > Could we take this opportunity to switch to QEMU generating the whole
> > set of ACPI tables?
> >
> Not quite sure how much effort would be taken on this change. CCed
> hvmloader maintainers for their comments.
> 
> Thanks,
> Haozhong

Re: [PATCH 2/2] pc-nvdimm acpi: build ACPI tables for pc-nvdimm devices

[Haozhong Zhang]

On 01/06/16 09:50, Konrad Rzeszutek Wilk wrote:
> On Tue, Jan 05, 2016 at 10:01:26PM +0800, Haozhong Zhang wrote:
> > On 01/05/16 11:00, Stefano Stabellini wrote:
> > > On Mon, 4 Jan 2016, Konrad Rzeszutek Wilk wrote:
> > > > On Mon, Jan 04, 2016 at 04:01:08PM +0000, Stefano Stabellini wrote:
> > > > > CC'ing the Xen tools maintainers and Anthony.
> > > > > 
> > > > > On Tue, 29 Dec 2015, Haozhong Zhang wrote:
> > > > > > Reuse existing NVDIMM ACPI code to build ACPI tables for pc-nvdimm
> > > > > > devices. The resulting tables are then copied into Xen guest domain so
> > > > > > tha they can be later loaded by Xen hvmloader.
> > > > > > 
> > > > > > Signed-off-by: Haozhong Zhang <haozhong.zhang@intel.com>
> > > > > 
> > > > > How much work would it be to generate the nvdimm acpi tables from the
> > > > > Xen toolstack?
> > > > 
> > > > Why duplicate the code? The QEMU generates the NFIT tables and its sub-tables.
> > > >
> > > >
> > > > > Getting the tables from QEMU doesn't seem like a good idea to me, unless
> > > > > we start getting the whole set of ACPI tables that way.
> > > > 
> > > > There is also the ACPI DSDT code - which requires an memory region
> > > > to be reserved for the AML code to drop the parameters so that QEMU
> > > > can scan the NVDIMM for failures. The region (and size) should be
> > > > determined by QEMU since it works on this data.
> > > 
> > > QEMU can generate the whole set of ACPI tables. Why should we take only
> > > the nvdimm tables and not the others?
> > >
> > NVDIMM tables are the only tables required to support vNVDIMM in this
> > patch series, and they are self-contained and not conflict with other
> > existing tables built by hvmloader. For other tables built by QEMU, I
> > have no idea whether they could work with Xen, so I only take nvdimm
> > tables from QEMU.
> 
> But you also have to deal with the SSDT code right?
NVDIMM's SSDT tables including DSM methods are also copied from QEMU.

> And I was under the
> impression that if you use hvmloader - it loads the ACPI DSDT which
> it had compiled at build-time - only - but not the SSDT?
>

(may be I missed something) Why do we bother with DSDT? It does not
contain information or methods of NVDIMM. 

> In other way - just having the ACPI NFIT won't give you the whole
> picture - unless you also jam in ACPI _DSM (ACPI0012) methods as
> part of the SSDT right?
>

Right, _DSM methods are also in SSDT and copied from QEMU. 

> Or does the ACPI tables generation also include an ACPI SSDT with the
> ACPI _DSM methods as part of it? I only see 'nvdimm_build_acpi' and
> NFIT comments, nothing about SSDT?
>

Not sure if we are looking at the same code. For QEMU commit 38a762f,
nvdimm_build_acpi() calls nvdimm_build_ssdt() to build SSDT of NVDIMM.

> > 
> > > I don't think it is wise to have two components which both think are in
> > > control of generating ACPI tables, hvmloader (soon to be the toolstack
> > > with Anthony's work) and QEMU. From an architectural perspective, it
> > > doesn't look robust to me.
> > >
> > Do you mean whenever nvdimm code in QEMU is changed, we would have to
> > make more efforts to ensure it still works with Xen?
> 
> Not sure I follow that. How is it different from the efforts to ensure
> that the patches here (which only add one ACPI MADT table)
> provide the proper support? Wouldn't you do the same type of checking
> every time you modify the nvdimm_build_acpi code?
>

Yes, I do have to check. I was just not clear what 'robust' meant here
and asked so.

Haozhong

> 
> > 
> > > Could we take this opportunity to switch to QEMU generating the whole
> > > set of ACPI tables?
> > >
> > Not quite sure how much effort would be taken on this change. CCed
> > hvmloader maintainers for their comments.
> > 
> > Thanks,
> > Haozhong
> 
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xen.org
> http://lists.xen.org/xen-devel