[Qemu-devel] [RFC QEMU PATCH v4 00/10] Implement vNVDIMM for Xen HVM guest

[Qemu-devel] [RFC QEMU PATCH v4 00/10] Implement vNVDIMM for Xen HVM guest

[Haozhong Zhang]

This is the QEMU part patches that works with the associated Xen
patches to enable vNVDIMM support for Xen HVM domains. Xen relies on
QEMU to build guest NFIT and NVDIMM namespace devices, and allocate
guest address space for vNVDIMM devices.

All patches can also be found at
  Xen:  https://github.com/hzzhan9/xen.git nvdimm-rfc-v4
  QEMU: https://github.com/hzzhan9/qemu.git xen-nvdimm-rfc-v4

RFC v3 can be found at
  https://lists.nongnu.org/archive/html/qemu-devel/2017-09/msg02406.html

Changes in v4:
  * The primary change in this version is to use the existing fw_cfg
    and BIOSLinkerLoader interface to pass ACPI to Xen guest, rather
    than introducing a Xen-specific mechanism. (Patch 5-10)

    Following Xen-specific are still left in ACPI code:
     (1) (Patch 6) xen_acpi_build() is called in acpi_build() to only
         build Xen guest required ACPI tables. The consequent code
         path in acpi_build() is bypassed.
     (2) (Patch 8) Add Xen-specific functions to access DSM memory,
         because the existing cpu_physical_memory_rw does not work on Xen.
     (3) (Patch 9) Implement a workaround for different AML integer
         widths between ACPI 1.0 (QEMU) and 2.0 (Xen).

Patch 1 is a trivial code cleanup.

Patch 2-3 add a memory backend dedicated for Xen usage and a hotplug
memory region for Xen guest, in order to make the existing nvdimm
device plugging path work on Xen.

Patch 4 is to avoid dereferencing the NULL pointer to non-existing
label data, as the Xen side support for labels is not implemented yet.

Patch 5-10 enable building ACPI tables and passing them to Xen HVM
domains.

Haozhong Zhang (10):
  [01/10] xen-hvm: remove a trailing space
  [02/10] xen-hvm: create the hotplug memory region on Xen
  [03/10] hostmem-xen: add a host memory backend for Xen
  [04/10] nvdimm: do not intiailize nvdimm->label_data if label size is zero
  [05/10] xen-hvm: initialize fw_cfg interface
  [06/10] hw/acpi-build, xen-hvm: introduce a Xen-specific ACPI builder
  [07/10] xen-hvm: add functions to copy data from/to HVM memory
  [08/10] nvdimm acpi: add functions to access DSM memory on Xen
  [09/10] nvdimm acpi: add compatibility for 64-bit integer in ACPI 2.0 and later
  [10/10] xen-hvm: enable building NFIT and SSDT of vNVDIMM for HVM domains

 backends/Makefile.objs      |   1 +
 backends/hostmem-xen.c      | 108 ++++++++++++++++++++++++++++++++++++++++++++
 backends/hostmem.c          |   9 ++++
 hw/acpi/nvdimm.c            |  51 +++++++++++++++++----
 hw/i386/acpi-build.c        |   9 +++-
 hw/i386/pc.c                |  86 +++++++++++++++++++----------------
 hw/i386/xen/xen-hvm.c       | 105 ++++++++++++++++++++++++++++++++++++++++--
 hw/mem/nvdimm.c             |  10 +++-
 hw/mem/pc-dimm.c            |   6 ++-
 include/hw/acpi/aml-build.h |   4 ++
 include/hw/i386/pc.h        |   1 +
 include/hw/xen/xen.h        |   7 +++
 stubs/xen-hvm.c             |  15 ++++++
 13 files changed, 359 insertions(+), 53 deletions(-)
 create mode 100644 backends/hostmem-xen.c

-- 
2.15.1

[Qemu-devel] [RFC QEMU PATCH v4 01/10] xen-hvm: remove a trailing space

[Haozhong Zhang]

Signed-off-by: Haozhong Zhang <haozhong.zhang@intel.com>
---
Cc: Stefano Stabellini <sstabellini@kernel.org>
Cc: Anthony Perard <anthony.perard@citrix.com>
Cc: Paolo Bonzini <pbonzini@redhat.com>
Cc: Richard Henderson <rth@twiddle.net>
Cc: Eduardo Habkost <ehabkost@redhat.com>
Cc: "Michael S. Tsirkin" <mst@redhat.com>
---
 hw/i386/xen/xen-hvm.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/hw/i386/xen/xen-hvm.c b/hw/i386/xen/xen-hvm.c
index 8028bed6fd..02d92fd268 100644
--- a/hw/i386/xen/xen-hvm.c
+++ b/hw/i386/xen/xen-hvm.c
@@ -248,7 +248,7 @@ void xen_ram_alloc(ram_addr_t ram_addr, ram_addr_t size, MemoryRegion *mr,
         /* RAM already populated in Xen */
         fprintf(stderr, "%s: do not alloc "RAM_ADDR_FMT
                 " bytes of ram at "RAM_ADDR_FMT" when runstate is INMIGRATE\n",
-                __func__, size, ram_addr); 
+                __func__, size, ram_addr);
         return;
     }
 
-- 
2.15.1

[Qemu-devel] [RFC QEMU PATCH v4 02/10] xen-hvm: create the hotplug memory region on Xen

[Haozhong Zhang]

The guest physical address of vNVDIMM is allocated from the hotplug
memory region, which is not created when QEMU is used as Xen device
model. In order to use vNVDIMM for Xen HVM domains, this commit reuses
the code for pc machine type to create the hotplug memory region for
Xen HVM domains.

Signed-off-by: Haozhong Zhang <haozhong.zhang@intel.com>
---
Cc: "Michael S. Tsirkin" <mst@redhat.com>
Cc: Paolo Bonzini <pbonzini@redhat.com>
Cc: Richard Henderson <rth@twiddle.net>
Cc: Eduardo Habkost <ehabkost@redhat.com>
Cc: Stefano Stabellini <sstabellini@kernel.org>
Cc: Anthony Perard <anthony.perard@citrix.com>
---
 hw/i386/pc.c          | 86 ++++++++++++++++++++++++++++-----------------------
 hw/i386/xen/xen-hvm.c |  2 ++
 include/hw/i386/pc.h  |  1 +
 3 files changed, 51 insertions(+), 38 deletions(-)

diff --git a/hw/i386/pc.c b/hw/i386/pc.c
index 186545d2a4..9f46c8df79 100644
--- a/hw/i386/pc.c
+++ b/hw/i386/pc.c
@@ -1315,6 +1315,53 @@ void xen_load_linux(PCMachineState *pcms)
     pcms->fw_cfg = fw_cfg;
 }
 
+void pc_memory_hotplug_init(PCMachineState *pcms, MemoryRegion *system_memory)
+{
+    MachineState *machine = MACHINE(pcms);
+    PCMachineClass *pcmc = PC_MACHINE_GET_CLASS(pcms);
+    ram_addr_t hotplug_mem_size = machine->maxram_size - machine->ram_size;
+
+    if (!pcmc->has_reserved_memory || machine->ram_size >= machine->maxram_size)
+        return;
+
+    if (memory_region_size(&pcms->hotplug_memory.mr)) {
+        error_report("hotplug memory region has been initialized");
+        exit(EXIT_FAILURE);
+    }
+
+    if (machine->ram_slots > ACPI_MAX_RAM_SLOTS) {
+        error_report("unsupported amount of memory slots: %"PRIu64,
+                     machine->ram_slots);
+        exit(EXIT_FAILURE);
+    }
+
+    if (QEMU_ALIGN_UP(machine->maxram_size,
+                      TARGET_PAGE_SIZE) != machine->maxram_size) {
+        error_report("maximum memory size must by aligned to multiple of "
+                     "%d bytes", TARGET_PAGE_SIZE);
+        exit(EXIT_FAILURE);
+    }
+
+    pcms->hotplug_memory.base =
+        ROUND_UP(0x100000000ULL + pcms->above_4g_mem_size, 1ULL << 30);
+
+    if (pcmc->enforce_aligned_dimm) {
+        /* size hotplug region assuming 1G page max alignment per slot */
+        hotplug_mem_size += (1ULL << 30) * machine->ram_slots;
+    }
+
+    if ((pcms->hotplug_memory.base + hotplug_mem_size) < hotplug_mem_size) {
+        error_report("unsupported amount of maximum memory: " RAM_ADDR_FMT,
+                     machine->maxram_size);
+        exit(EXIT_FAILURE);
+    }
+
+    memory_region_init(&pcms->hotplug_memory.mr, OBJECT(pcms),
+                       "hotplug-memory", hotplug_mem_size);
+    memory_region_add_subregion(system_memory, pcms->hotplug_memory.base,
+                                &pcms->hotplug_memory.mr);
+}
+
 void pc_memory_init(PCMachineState *pcms,
                     MemoryRegion *system_memory,
                     MemoryRegion *rom_memory,
@@ -1366,44 +1413,7 @@ void pc_memory_init(PCMachineState *pcms,
     }
 
     /* initialize hotplug memory address space */
-    if (pcmc->has_reserved_memory &&
-        (machine->ram_size < machine->maxram_size)) {
-        ram_addr_t hotplug_mem_size =
-            machine->maxram_size - machine->ram_size;
-
-        if (machine->ram_slots > ACPI_MAX_RAM_SLOTS) {
-            error_report("unsupported amount of memory slots: %"PRIu64,
-                         machine->ram_slots);
-            exit(EXIT_FAILURE);
-        }
-
-        if (QEMU_ALIGN_UP(machine->maxram_size,
-                          TARGET_PAGE_SIZE) != machine->maxram_size) {
-            error_report("maximum memory size must by aligned to multiple of "
-                         "%d bytes", TARGET_PAGE_SIZE);
-            exit(EXIT_FAILURE);
-        }
-
-        pcms->hotplug_memory.base =
-            ROUND_UP(0x100000000ULL + pcms->above_4g_mem_size, 1ULL << 30);
-
-        if (pcmc->enforce_aligned_dimm) {
-            /* size hotplug region assuming 1G page max alignment per slot */
-            hotplug_mem_size += (1ULL << 30) * machine->ram_slots;
-        }
-
-        if ((pcms->hotplug_memory.base + hotplug_mem_size) <
-            hotplug_mem_size) {
-            error_report("unsupported amount of maximum memory: " RAM_ADDR_FMT,
-                         machine->maxram_size);
-            exit(EXIT_FAILURE);
-        }
-
-        memory_region_init(&pcms->hotplug_memory.mr, OBJECT(pcms),
-                           "hotplug-memory", hotplug_mem_size);
-        memory_region_add_subregion(system_memory, pcms->hotplug_memory.base,
-                                    &pcms->hotplug_memory.mr);
-    }
+    pc_memory_hotplug_init(pcms, system_memory);
 
     /* Initialize PC system firmware */
     pc_system_firmware_init(rom_memory, !pcmc->pci_enabled);
diff --git a/hw/i386/xen/xen-hvm.c b/hw/i386/xen/xen-hvm.c
index 02d92fd268..fe01b7a025 100644
--- a/hw/i386/xen/xen-hvm.c
+++ b/hw/i386/xen/xen-hvm.c
@@ -235,6 +235,8 @@ static void xen_ram_init(PCMachineState *pcms,
                                  pcms->above_4g_mem_size);
         memory_region_add_subregion(sysmem, 0x100000000ULL, &ram_hi);
     }
+
+    pc_memory_hotplug_init(pcms, sysmem);
 }
 
 void xen_ram_alloc(ram_addr_t ram_addr, ram_addr_t size, MemoryRegion *mr,
diff --git a/include/hw/i386/pc.h b/include/hw/i386/pc.h
index ef438bd765..86e375b616 100644
--- a/include/hw/i386/pc.h
+++ b/include/hw/i386/pc.h
@@ -249,6 +249,7 @@ void pc_memory_init(PCMachineState *pcms,
                     MemoryRegion *rom_memory,
                     MemoryRegion **ram_memory);
 uint64_t pc_pci_hole64_start(void);
+void pc_memory_hotplug_init(PCMachineState *pcms, MemoryRegion *system_memory);
 qemu_irq pc_allocate_cpu_irq(void);
 DeviceState *pc_vga_init(ISABus *isa_bus, PCIBus *pci_bus);
 void pc_basic_device_init(ISABus *isa_bus, qemu_irq *gsi,
-- 
2.15.1

[Qemu-devel] [RFC QEMU PATCH v4 03/10] hostmem-xen: add a host memory backend for Xen

[Haozhong Zhang]

vNVDIMM requires a host memory backend to allocate its backend
resources to the guest. When QEMU is used as Xen device model, the
backend resource allocation of vNVDIMM is managed out of QEMU. A new
host memory backend 'memory-backend-xen' is introduced to represent
the backend resource allocated by Xen. It simply creates a memory
region of the specified size as a placeholder in the guest address
space, which will be mapped by Xen to the actual backend resource.

Following example QEMU options create a vNVDIMM device backed by a 4GB
host PMEM region at host physical address 0x100000000:
   -object memory-backend-xen,id=mem1,host-addr=0x100000000,size=4G
   -device nvdimm,id=nvdimm1,memdev=mem1

Signed-off-by: Haozhong Zhang <haozhong.zhang@intel.com>
---
Cc: Eduardo Habkost <ehabkost@redhat.com>
Cc: Igor Mammedov <imammedo@redhat.com>
Cc: "Michael S. Tsirkin" <mst@redhat.com>
---
 backends/Makefile.objs |   1 +
 backends/hostmem-xen.c | 108 +++++++++++++++++++++++++++++++++++++++++++++++++
 backends/hostmem.c     |   9 +++++
 hw/mem/pc-dimm.c       |   6 ++-
 4 files changed, 123 insertions(+), 1 deletion(-)
 create mode 100644 backends/hostmem-xen.c

diff --git a/backends/Makefile.objs b/backends/Makefile.objs
index 0400799efd..3096fde21f 100644
--- a/backends/Makefile.objs
+++ b/backends/Makefile.objs
@@ -5,6 +5,7 @@ common-obj-$(CONFIG_TPM) += tpm.o
 
 common-obj-y += hostmem.o hostmem-ram.o
 common-obj-$(CONFIG_LINUX) += hostmem-file.o
+common-obj-${CONFIG_XEN_BACKEND} += hostmem-xen.o
 
 common-obj-y += cryptodev.o
 common-obj-y += cryptodev-builtin.o
diff --git a/backends/hostmem-xen.c b/backends/hostmem-xen.c
new file mode 100644
index 0000000000..99211efd81
--- /dev/null
+++ b/backends/hostmem-xen.c
@@ -0,0 +1,108 @@
+/*
+ * QEMU Host Memory Backend for Xen
+ *
+ * Copyright(C) 2017 Intel Corporation.
+ *
+ * Author:
+ *   Haozhong Zhang <haozhong.zhang@intel.com>
+ *
+ * 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 "qemu/osdep.h"
+#include "sysemu/hostmem.h"
+#include "qapi/error.h"
+#include "qom/object_interfaces.h"
+
+#define TYPE_MEMORY_BACKEND_XEN "memory-backend-xen"
+
+#define MEMORY_BACKEND_XEN(obj) \
+    OBJECT_CHECK(HostMemoryBackendXen, (obj), TYPE_MEMORY_BACKEND_XEN)
+
+typedef struct HostMemoryBackendXen HostMemoryBackendXen;
+
+struct HostMemoryBackendXen {
+    HostMemoryBackend parent_obj;
+
+    uint64_t host_addr;
+};
+
+static void xen_backend_get_host_addr(Object *obj, Visitor *v, const char *name,
+                                      void *opaque, Error **errp)
+{
+    HostMemoryBackendXen *backend = MEMORY_BACKEND_XEN(obj);
+    uint64_t value = backend->host_addr;
+
+    visit_type_size(v, name, &value, errp);
+}
+
+static void xen_backend_set_host_addr(Object *obj, Visitor *v, const char *name,
+                                      void *opaque, Error **errp)
+{
+    HostMemoryBackend *backend = MEMORY_BACKEND(obj);
+    HostMemoryBackendXen *xb = MEMORY_BACKEND_XEN(obj);
+    Error *local_err = NULL;
+    uint64_t value;
+
+    if (memory_region_size(&backend->mr)) {
+        error_setg(&local_err, "cannot change property value");
+        goto out;
+    }
+
+    visit_type_size(v, name, &value, &local_err);
+    if (local_err) {
+        goto out;
+    }
+    xb->host_addr = value;
+
+ out:
+    error_propagate(errp, local_err);
+}
+
+static void xen_backend_alloc(HostMemoryBackend *backend, Error **errp)
+{
+    if (!backend->size) {
+        error_setg(errp, "can't create backend with size 0");
+        return;
+    }
+    memory_region_init(&backend->mr, OBJECT(backend), "hostmem-xen",
+                       backend->size);
+    backend->mr.align = getpagesize();
+}
+
+static void xen_backend_class_init(ObjectClass *oc, void *data)
+{
+    HostMemoryBackendClass *bc = MEMORY_BACKEND_CLASS(oc);
+
+    bc->alloc = xen_backend_alloc;
+
+    object_class_property_add(oc, "host-addr", "int",
+                              xen_backend_get_host_addr,
+                              xen_backend_set_host_addr,
+                              NULL, NULL, &error_abort);
+}
+
+static const TypeInfo xen_backend_info = {
+    .name = TYPE_MEMORY_BACKEND_XEN,
+    .parent = TYPE_MEMORY_BACKEND,
+    .class_init = xen_backend_class_init,
+    .instance_size = sizeof(HostMemoryBackendXen),
+};
+
+static void register_types(void)
+{
+    type_register_static(&xen_backend_info);
+}
+
+type_init(register_types);
diff --git a/backends/hostmem.c b/backends/hostmem.c
index ee2c2d5bfd..ba13a52994 100644
--- a/backends/hostmem.c
+++ b/backends/hostmem.c
@@ -12,6 +12,7 @@
 #include "qemu/osdep.h"
 #include "sysemu/hostmem.h"
 #include "hw/boards.h"
+#include "hw/xen/xen.h"
 #include "qapi/error.h"
 #include "qapi/visitor.h"
 #include "qapi-types.h"
@@ -277,6 +278,14 @@ host_memory_backend_memory_complete(UserCreatable *uc, Error **errp)
             goto out;
         }
 
+        /*
+         * The backend storage of MEMORY_BACKEND_XEN is managed by Xen,
+         * so no further work in this function is needed.
+         */
+        if (xen_enabled() && !backend->mr.ram_block) {
+            goto out;
+        }
+
         ptr = memory_region_get_ram_ptr(&backend->mr);
         sz = memory_region_size(&backend->mr);
 
diff --git a/hw/mem/pc-dimm.c b/hw/mem/pc-dimm.c
index 66eace5a5c..dcbfce33d5 100644
--- a/hw/mem/pc-dimm.c
+++ b/hw/mem/pc-dimm.c
@@ -28,6 +28,7 @@
 #include "sysemu/kvm.h"
 #include "trace.h"
 #include "hw/virtio/vhost.h"
+#include "hw/xen/xen.h"
 
 typedef struct pc_dimms_capacity {
      uint64_t size;
@@ -108,7 +109,10 @@ void pc_dimm_memory_plug(DeviceState *dev, MemoryHotplugState *hpms,
     }
 
     memory_region_add_subregion(&hpms->mr, addr - hpms->base, mr);
-    vmstate_register_ram(vmstate_mr, dev);
+    /* memory-backend-xen is not backed by RAM. */
+    if (!xen_enabled()) {
+        vmstate_register_ram(vmstate_mr, dev);
+    }
     numa_set_mem_node_id(addr, memory_region_size(mr), dimm->node);
 
 out:
-- 
2.15.1

[Qemu-devel] [RFC QEMU PATCH v4 04/10] nvdimm: do not intiailize nvdimm->label_data if label size is zero

[Haozhong Zhang]

The memory region of vNVDIMM on Xen is a RAM memory region, so
memory_region_get_ram_ptr() cannot be used in nvdimm_realize() to get
a pointer to the label data area in that region. To be worse, it may
abort QEMU. As Xen currently does not support labels (i.e. label size
is 0) and every access in QEMU to labels is led by a label size check,
let's not intiailize nvdimm->label_data if the label size is 0.

Signed-off-by: Haozhong Zhang <haozhong.zhang@intel.com>
---
Cc: Xiao Guangrong <xiaoguangrong.eric@gmail.com>
Cc: "Michael S. Tsirkin" <mst@redhat.com>
Cc: Igor Mammedov <imammedo@redhat.com>
---
 hw/mem/nvdimm.c | 10 +++++++++-
 1 file changed, 9 insertions(+), 1 deletion(-)

diff --git a/hw/mem/nvdimm.c b/hw/mem/nvdimm.c
index 952fce5ec8..3e58538b99 100644
--- a/hw/mem/nvdimm.c
+++ b/hw/mem/nvdimm.c
@@ -87,7 +87,15 @@ static void nvdimm_realize(PCDIMMDevice *dimm, Error **errp)
     align = memory_region_get_alignment(mr);
 
     pmem_size = size - nvdimm->label_size;
-    nvdimm->label_data = memory_region_get_ram_ptr(mr) + pmem_size;
+    /*
+     * The memory region of vNVDIMM on Xen is not a RAM memory region,
+     * so memory_region_get_ram_ptr() below will abort QEMU. In
+     * addition that Xen currently does not support vNVDIMM labels
+     * (i.e. label_size is zero here), let's not initialize of the
+     * pointer to label data if the label size is zero.
+     */
+    if (nvdimm->label_size)
+        nvdimm->label_data = memory_region_get_ram_ptr(mr) + pmem_size;
     pmem_size = QEMU_ALIGN_DOWN(pmem_size, align);
 
     if (size <= nvdimm->label_size || !pmem_size) {
-- 
2.15.1

[Qemu-devel] [RFC QEMU PATCH v4 05/10] xen-hvm: initialize fw_cfg interface

[Haozhong Zhang]

Xen is going to reuse QEMU to build ACPI of some devices (e.g., NFIT
and SSDT for NVDIMM) for HVM domains. The existing QEMU ACPI build
code requires a fw_cfg interface which will also be used to pass QEMU
built ACPI to Xen. Therefore, we need to initialize fw_cfg when any
ACPI is going to be built by QEMU.

Signed-off-by: Haozhong Zhang <haozhong.zhang@intel.com>
---
Cc: Stefano Stabellini <sstabellini@kernel.org>
Cc: Anthony Perard <anthony.perard@citrix.com>
Cc: "Michael S. Tsirkin" <mst@redhat.com>
Cc: Paolo Bonzini <pbonzini@redhat.com>
Cc: Richard Henderson <rth@twiddle.net>
Cc: Eduardo Habkost <ehabkost@redhat.com>
---
 hw/i386/xen/xen-hvm.c | 12 ++++++++++++
 1 file changed, 12 insertions(+)

diff --git a/hw/i386/xen/xen-hvm.c b/hw/i386/xen/xen-hvm.c
index fe01b7a025..4b29f4052b 100644
--- a/hw/i386/xen/xen-hvm.c
+++ b/hw/i386/xen/xen-hvm.c
@@ -14,6 +14,7 @@
 #include "hw/pci/pci.h"
 #include "hw/i386/pc.h"
 #include "hw/i386/apic-msidef.h"
+#include "hw/loader.h"
 #include "hw/xen/xen_common.h"
 #include "hw/xen/xen_backend.h"
 #include "qmp-commands.h"
@@ -1234,6 +1235,14 @@ static void xen_wakeup_notifier(Notifier *notifier, void *data)
     xc_set_hvm_param(xen_xc, xen_domid, HVM_PARAM_ACPI_S_STATE, 0);
 }
 
+static void xen_fw_cfg_init(PCMachineState *pcms)
+{
+    FWCfgState *fw_cfg = fw_cfg_init_io(FW_CFG_IO_BASE);
+
+    rom_set_fw(fw_cfg);
+    pcms->fw_cfg = fw_cfg;
+}
+
 void xen_hvm_init(PCMachineState *pcms, MemoryRegion **ram_memory)
 {
     int i, rc;
@@ -1384,6 +1393,9 @@ void xen_hvm_init(PCMachineState *pcms, MemoryRegion **ram_memory)
 
     /* Disable ACPI build because Xen handles it */
     pcms->acpi_build_enabled = false;
+    if (pcms->acpi_build_enabled) {
+        xen_fw_cfg_init(pcms);
+    }
 
     return;
 
-- 
2.15.1

[Qemu-devel] [RFC QEMU PATCH v4 06/10] hw/acpi-build, xen-hvm: introduce a Xen-specific ACPI builder

[Haozhong Zhang]

QEMU on KVM/TCG and Xen requires different sets of guest ACPI tables.
When QEMU builds ACPI for Xen HVM domains, the new Xen-specific ACPI
build function xen_acpi_build() is called instead of the existing path
from acpi_build().

Signed-off-by: Haozhong Zhang <haozhong.zhang@intel.com>
---
Cc: Stefano Stabellini <sstabellini@kernel.org>
Cc: Anthony Perard <anthony.perard@citrix.com>
Cc: "Michael S. Tsirkin" <mst@redhat.com>
Cc: Paolo Bonzini <pbonzini@redhat.com>
Cc: Richard Henderson <rth@twiddle.net>
Cc: Eduardo Habkost <ehabkost@redhat.com>
---
 hw/i386/acpi-build.c        |  9 ++++++++-
 hw/i386/xen/xen-hvm.c       | 21 +++++++++++++++++++++
 include/hw/acpi/aml-build.h |  4 ++++
 include/hw/xen/xen.h        |  4 ++++
 stubs/xen-hvm.c             |  5 +++++
 5 files changed, 42 insertions(+), 1 deletion(-)

diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
index 73519ab3ac..9007ecdaed 100644
--- a/hw/i386/acpi-build.c
+++ b/hw/i386/acpi-build.c
@@ -60,6 +60,7 @@
 #include "qom/qom-qobject.h"
 #include "hw/i386/amd_iommu.h"
 #include "hw/i386/intel_iommu.h"
+#include "hw/xen/xen.h"
 
 #include "hw/acpi/ipmi.h"
 
@@ -2556,7 +2557,7 @@ build_amd_iommu(GArray *table_data, BIOSLinker *linker)
                  "IVRS", table_data->len - iommu_start, 1, NULL, NULL);
 }
 
-static GArray *
+GArray *
 build_rsdp(GArray *rsdp_table, BIOSLinker *linker, unsigned rsdt_tbl_offset)
 {
     AcpiRsdpDescriptor *rsdp = acpi_data_push(rsdp_table, sizeof *rsdp);
@@ -2646,6 +2647,11 @@ void acpi_build(AcpiBuildTables *tables, MachineState *machine)
                              64 /* Ensure FACS is aligned */,
                              false /* high memory */);
 
+    if (xen_enabled()) {
+        xen_acpi_build(tables, table_offsets, machine);
+        goto done;
+    }
+
     /*
      * FACS is pointed to by FADT.
      * We place it first since it's the only table that has alignment
@@ -2788,6 +2794,7 @@ void acpi_build(AcpiBuildTables *tables, MachineState *machine)
 
     acpi_align_size(tables->linker->cmd_blob, ACPI_BUILD_ALIGN_SIZE);
 
+ done:
     /* Cleanup memory that's no longer used. */
     g_array_free(table_offsets, true);
 }
diff --git a/hw/i386/xen/xen-hvm.c b/hw/i386/xen/xen-hvm.c
index 4b29f4052b..3df20ff282 100644
--- a/hw/i386/xen/xen-hvm.c
+++ b/hw/i386/xen/xen-hvm.c
@@ -11,6 +11,7 @@
 #include "qemu/osdep.h"
 
 #include "cpu.h"
+#include "hw/acpi/aml-build.h"
 #include "hw/pci/pci.h"
 #include "hw/i386/pc.h"
 #include "hw/i386/apic-msidef.h"
@@ -1473,3 +1474,23 @@ void qmp_xen_set_global_dirty_log(bool enable, Error **errp)
         memory_global_dirty_log_stop();
     }
 }
+
+void xen_acpi_build(AcpiBuildTables *tables, GArray *table_offsets,
+                    MachineState *machine)
+{
+    PCMachineState *pcms = PC_MACHINE(machine);
+    GArray *tables_blob = tables->table_data;
+    unsigned int rsdt;
+
+    if (!pcms->acpi_build_enabled) {
+        return;
+    }
+
+    /*
+     * QEMU RSDP and RSDT are only used by hvmloader to enumerate
+     * QEMU-built tables. HVM domains still use Xen-built RSDP and RSDT.
+     */
+    rsdt = tables_blob->len;
+    build_rsdt(tables_blob, tables->linker, table_offsets, 0, 0);
+    build_rsdp(tables->rsdp, tables->linker, rsdt);
+}
diff --git a/include/hw/acpi/aml-build.h b/include/hw/acpi/aml-build.h
index 88d0738d76..03369bb7ea 100644
--- a/include/hw/acpi/aml-build.h
+++ b/include/hw/acpi/aml-build.h
@@ -393,4 +393,8 @@ void build_srat_memory(AcpiSratMemoryAffinity *numamem, uint64_t base,
                        uint64_t len, int node, MemoryAffinityFlags flags);
 
 void build_slit(GArray *table_data, BIOSLinker *linker);
+
+GArray *build_rsdp(GArray *rsdp_table, BIOSLinker *linker,
+                   unsigned rsdt_tbl_offset);
+
 #endif
diff --git a/include/hw/xen/xen.h b/include/hw/xen/xen.h
index 7efcdaa8fe..2785b8fd35 100644
--- a/include/hw/xen/xen.h
+++ b/include/hw/xen/xen.h
@@ -10,6 +10,7 @@
 
 #include "qemu-common.h"
 #include "exec/cpu-common.h"
+#include "hw/acpi/aml-build.h"
 #include "hw/irq.h"
 
 /* xen-machine.c */
@@ -48,4 +49,7 @@ void xen_hvm_modified_memory(ram_addr_t start, ram_addr_t length);
 
 void xen_register_framebuffer(struct MemoryRegion *mr);
 
+void xen_acpi_build(AcpiBuildTables *tables, GArray *table_offsets,
+                    MachineState *machine);
+
 #endif /* QEMU_HW_XEN_H */
diff --git a/stubs/xen-hvm.c b/stubs/xen-hvm.c
index 3ca6c51b21..58017c1457 100644
--- a/stubs/xen-hvm.c
+++ b/stubs/xen-hvm.c
@@ -61,3 +61,8 @@ void xen_hvm_init(PCMachineState *pcms, MemoryRegion **ram_memory)
 void qmp_xen_set_global_dirty_log(bool enable, Error **errp)
 {
 }
+
+void xen_acpi_build(AcpiBuildTables *tables, GArray *table_offsets,
+                    MachineState *machine)
+{
+}
-- 
2.15.1

[Qemu-devel] [RFC QEMU PATCH v4 07/10] xen-hvm: add functions to copy data from/to HVM memory

[Haozhong Zhang]

Signed-off-by: Haozhong Zhang <haozhong.zhang@intel.com>
---
Cc: Stefano Stabellini <sstabellini@kernel.org>
Cc: Anthony Perard <anthony.perard@citrix.com>
Cc: Paolo Bonzini <pbonzini@redhat.com>
Cc: Richard Henderson <rth@twiddle.net>
Cc: Eduardo Habkost <ehabkost@redhat.com>
Cc: "Michael S. Tsirkin" <mst@redhat.com>
---
 hw/i386/xen/xen-hvm.c | 55 +++++++++++++++++++++++++++++++++++++++++++++++++++
 include/hw/xen/xen.h  |  3 +++
 stubs/xen-hvm.c       | 10 ++++++++++
 3 files changed, 68 insertions(+)

diff --git a/hw/i386/xen/xen-hvm.c b/hw/i386/xen/xen-hvm.c
index 3df20ff282..a7e99bd438 100644
--- a/hw/i386/xen/xen-hvm.c
+++ b/hw/i386/xen/xen-hvm.c
@@ -1494,3 +1494,58 @@ void xen_acpi_build(AcpiBuildTables *tables, GArray *table_offsets,
     build_rsdt(tables_blob, tables->linker, table_offsets, 0, 0);
     build_rsdp(tables->rsdp, tables->linker, rsdt);
 }
+
+static size_t xen_rw_guest(ram_addr_t gpa,
+                           void *buf, size_t length, bool is_write)
+{
+    size_t copied = 0, size;
+    ram_addr_t s, e, offset, cur = gpa;
+    xen_pfn_t cur_pfn;
+    void *page;
+    int prot = is_write ? PROT_WRITE : PROT_READ;
+
+    if (!buf || !length) {
+        return 0;
+    }
+
+    s = gpa & TARGET_PAGE_MASK;
+    e = gpa + length;
+    if (e < s) {
+        return 0;
+    }
+
+    while (cur < e) {
+        cur_pfn = cur >> TARGET_PAGE_BITS;
+        offset = cur - (cur_pfn << TARGET_PAGE_BITS);
+        size = MIN(length, TARGET_PAGE_SIZE - offset);
+
+        page = xenforeignmemory_map(xen_fmem, xen_domid, prot, 1, &cur_pfn, NULL);
+        if (!page) {
+            break;
+        }
+
+        if (is_write) {
+            memcpy(page + offset, buf, size);
+        } else {
+            memcpy(buf, page + offset, size);
+        }
+        xenforeignmemory_unmap(xen_fmem, page, 1);
+
+        copied += size;
+        buf += size;
+        cur += size;
+        length -= size;
+    }
+
+    return copied;
+}
+
+size_t xen_copy_to_guest(ram_addr_t gpa, void *buf, size_t length)
+{
+    return xen_rw_guest(gpa, buf, length, true);
+}
+
+size_t xen_copy_from_guest(ram_addr_t gpa, void *buf, size_t length)
+{
+    return xen_rw_guest(gpa, buf, length, false);
+}
diff --git a/include/hw/xen/xen.h b/include/hw/xen/xen.h
index 2785b8fd35..cc40d45aeb 100644
--- a/include/hw/xen/xen.h
+++ b/include/hw/xen/xen.h
@@ -52,4 +52,7 @@ void xen_register_framebuffer(struct MemoryRegion *mr);
 void xen_acpi_build(AcpiBuildTables *tables, GArray *table_offsets,
                     MachineState *machine);
 
+size_t xen_copy_to_guest(ram_addr_t gpa, void *buf, size_t length);
+size_t xen_copy_from_guest(ram_addr_t gpa, void *buf, size_t length);
+
 #endif /* QEMU_HW_XEN_H */
diff --git a/stubs/xen-hvm.c b/stubs/xen-hvm.c
index 58017c1457..5de02842a3 100644
--- a/stubs/xen-hvm.c
+++ b/stubs/xen-hvm.c
@@ -66,3 +66,13 @@ void xen_acpi_build(AcpiBuildTables *tables, GArray *table_offsets,
                     MachineState *machine)
 {
 }
+
+size_t xen_copy_to_guest(ram_addr_t gpa, void *buf, size_t length)
+{
+    return 0;
+}
+
+size_t xen_copy_from_guest(ram_addr_t gpa, void *buf, size_t length)
+{
+    return 0;
+}
-- 
2.15.1

[Qemu-devel] [RFC QEMU PATCH v4 08/10] nvdimm acpi: add functions to access DSM memory on Xen

[Haozhong Zhang]

Xen hvmloader can load QEMU-built NVDIMM ACPI tables via the
BIOSLinkerLoader interface, but it allocates memory in an area not
covered by any memory regions in QEMU, i.e., the hvmloader memory
cannot be accessed via the normal cpu_physical_memory_{read,write}().
If QEMU on Xen has to access the hvmloader memory in DSM emulation,
it has to take a different path, i.e., xen_copy_{from,to}_guest().

Signed-off-by: Haozhong Zhang <haozhong.zhang@intel.com>
---
Cc: Xiao Guangrong <xiaoguangrong.eric@gmail.com>
Cc: "Michael S. Tsirkin" <mst@redhat.com>
Cc: Igor Mammedov <imammedo@redhat.com>
---
 hw/acpi/nvdimm.c | 44 +++++++++++++++++++++++++++++++++++++-------
 1 file changed, 37 insertions(+), 7 deletions(-)

diff --git a/hw/acpi/nvdimm.c b/hw/acpi/nvdimm.c
index 6ceea196e7..7b3062e001 100644
--- a/hw/acpi/nvdimm.c
+++ b/hw/acpi/nvdimm.c
@@ -32,6 +32,7 @@
 #include "hw/acpi/bios-linker-loader.h"
 #include "hw/nvram/fw_cfg.h"
 #include "hw/mem/nvdimm.h"
+#include "hw/xen/xen.h"
 
 static int nvdimm_device_list(Object *obj, void *opaque)
 {
@@ -497,6 +498,35 @@ struct NvdimmFuncReadFITOut {
 typedef struct NvdimmFuncReadFITOut NvdimmFuncReadFITOut;
 QEMU_BUILD_BUG_ON(sizeof(NvdimmFuncReadFITOut) > NVDIMM_DSM_MEMORY_SIZE);
 
+/*
+ * Xen hvmloader can load QEMU-built NVDIMM ACPI tables via the
+ * BIOSLinkerLoader interface, but it allocates memory in an area not
+ * covered by any memory regions in QEMU, i.e., the hvmloader memory
+ * cannot be accessed via the normal cpu_physical_memory_{read,write}().
+ * If QEMU on Xen has to access the hvmloader memory in DSM emulation,
+ * it has to take a different path, i.e., xen_copy_{from,to}_guest().
+ */
+
+static void
+nvdimm_copy_from_dsm_mem(hwaddr dsm_mem_addr, void *dst, unsigned size)
+{
+    if (xen_enabled()) {
+        xen_copy_from_guest(dsm_mem_addr, dst, size);
+    } else {
+        cpu_physical_memory_read(dsm_mem_addr, dst, size);
+    }
+}
+
+static void
+nvdimm_copy_to_dsm_mem(hwaddr dsm_mem_addr, void *src, unsigned size)
+{
+    if (xen_enabled()) {
+        xen_copy_to_guest(dsm_mem_addr, src, size);
+    } else {
+        cpu_physical_memory_write(dsm_mem_addr, src, size);
+    }
+}
+
 static void
 nvdimm_dsm_function0(uint32_t supported_func, hwaddr dsm_mem_addr)
 {
@@ -504,7 +534,7 @@ nvdimm_dsm_function0(uint32_t supported_func, hwaddr dsm_mem_addr)
         .len = cpu_to_le32(sizeof(func0)),
         .supported_func = cpu_to_le32(supported_func),
     };
-    cpu_physical_memory_write(dsm_mem_addr, &func0, sizeof(func0));
+    nvdimm_copy_to_dsm_mem(dsm_mem_addr, &func0, sizeof(func0));
 }
 
 static void
@@ -514,7 +544,7 @@ nvdimm_dsm_no_payload(uint32_t func_ret_status, hwaddr dsm_mem_addr)
         .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));
+    nvdimm_copy_to_dsm_mem(dsm_mem_addr, &out, sizeof(out));
 }
 
 #define NVDIMM_DSM_RET_STATUS_SUCCESS        0 /* Success */
@@ -569,7 +599,7 @@ exit:
     read_fit_out->func_ret_status = cpu_to_le32(func_ret_status);
     memcpy(read_fit_out->fit, fit->data + read_fit->offset, read_len);
 
-    cpu_physical_memory_write(dsm_mem_addr, read_fit_out, size);
+    nvdimm_copy_to_dsm_mem(dsm_mem_addr, read_fit_out, size);
 
     g_free(read_fit_out);
 }
@@ -655,8 +685,8 @@ static void nvdimm_dsm_label_size(NVDIMMDevice *nvdimm, hwaddr dsm_mem_addr)
     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));
+    nvdimm_copy_to_dsm_mem(dsm_mem_addr, &label_size_out,
+                           sizeof(label_size_out));
 }
 
 static uint32_t nvdimm_rw_label_data_check(NVDIMMDevice *nvdimm,
@@ -721,7 +751,7 @@ static void nvdimm_dsm_get_label_data(NVDIMMDevice *nvdimm, NvdimmDsmIn *in,
     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);
+    nvdimm_copy_to_dsm_mem(dsm_mem_addr, get_label_data_out, size);
     g_free(get_label_data_out);
 }
 
@@ -831,7 +861,7 @@ nvdimm_dsm_write(void *opaque, hwaddr addr, uint64_t val, unsigned size)
      * this by copying DSM memory to QEMU local memory.
      */
     in = g_new(NvdimmDsmIn, 1);
-    cpu_physical_memory_read(dsm_mem_addr, in, sizeof(*in));
+    nvdimm_copy_from_dsm_mem(dsm_mem_addr, in, sizeof(*in));
 
     le32_to_cpus(&in->revision);
     le32_to_cpus(&in->function);
-- 
2.15.1

[Qemu-devel] [RFC QEMU PATCH v4 09/10] nvdimm acpi: add compatibility for 64-bit integer in ACPI 2.0 and later

[Haozhong Zhang]

When QEMU is used as Xen device model, the QEMU-built NVDIMM ACPI
tables (NFIT and SSDT) may be passed to Xen and merged with Xen-built
ACPI tables. However, different ACPI versions are used between QEMU
(ACPI 1.0) and Xen (ACPI 2.0), and different integer widths are used
between ACPI 1.0 (32 bits) and ACPI 2.0 (64 bits).

Due to the implicit type conversion between ACPI buffer field object
and ACPI integer object (ref. ACPI Spec 6.2, Sect 19.3.5.5, 19.3.5.7 &
19.3.5.8), the following AML in NVDIMM SSDT may behave differently in
ACPI 1.0 and ACPI 2.0:

    Method (NCAL, 5, Serialized)
    {
        Local6 = MEMA /* \MEMA */
        OperationRegion (NPIO, SystemIO, 0x0A18, 0x04)
        OperationRegion (NRAM, SystemMemory, Local6, 0x1000)
        Field (NPIO, DWordAcc, NoLock, Preserve)
        {
            NTFI,   32
        }

        ...

        Field (NRAM, DWordAcc, NoLock, Preserve)
        {
            RLEN,   32,
            ODAT,   32736
        }

        ...

        NTFI = Local6
        Local1 = (RLEN - 0x04)
        Local1 = (Local1 << 0x03)
        CreateField (ODAT, Zero, Local1, OBUF)
        Concatenate (Buffer (Zero){}, OBUF, Local7)
        Return (Local7)
    }

The C layout of the above ODAT is struct NvdimmFuncReadFitOut without
the length field:

    struct {
        uint32_t func_ret_status;
	uint8_t fit[0];
    }

When no error happens and no FIT data is needed to return,
nvdimm_dsm_func_read_fit() fills

    { .func_ret_status = 0 },

i.e., 4 bytes of 0's in ODAT. Because the length of ODAT is no larger
than an integer, OBUF is implicitly converted into an ACPI integer
object during the evaluation of CreateField. Later, when OBUF is
concatenated to another buffer, it needs to be converted to an ACPI
buffer object.  It's converted to a 4 bytes buffer in ACPI 1.0, but
it's converted to a 8 bytes buffer in ACPI 2.0. The extra 4 bytes in
ACPI 2.0 actually corresponds to the apparently incorrect case that

    { .func_ret_status = 0, fit = { 0, 0, 0, 0 } }

is filled in ODAT.

In order to mitigate this issue, we add a 32-bit reserved field after
func_ret_status and always fill it with 0. Therefore, the minimum
length of ODAT in both ACPI 1.0 and ACPI 2.0 is always 8 bytes, so no
extra bytes will be added accidentally by the implicit conversion.

Signed-off-by: Haozhong Zhang <haozhong.zhang@intel.com>
---
Cc: Xiao Guangrong <xiaoguangrong.eric@gmail.com>
Cc: "Michael S. Tsirkin" <mst@redhat.com>
Cc: Igor Mammedov <imammedo@redhat.com>
---
 hw/acpi/nvdimm.c | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/hw/acpi/nvdimm.c b/hw/acpi/nvdimm.c
index 7b3062e001..bceb35e75a 100644
--- a/hw/acpi/nvdimm.c
+++ b/hw/acpi/nvdimm.c
@@ -493,6 +493,7 @@ struct NvdimmFuncReadFITOut {
     /* the size of buffer filled by QEMU. */
     uint32_t len;
     uint32_t func_ret_status; /* return status code. */
+    uint32_t reserved;
     uint8_t fit[0]; /* the FIT data. */
 } QEMU_PACKED;
 typedef struct NvdimmFuncReadFITOut NvdimmFuncReadFITOut;
@@ -597,6 +598,7 @@ exit:
 
     read_fit_out->len = cpu_to_le32(size);
     read_fit_out->func_ret_status = cpu_to_le32(func_ret_status);
+    read_fit_out->reserved = 0;
     memcpy(read_fit_out->fit, fit->data + read_fit->offset, read_len);
 
     nvdimm_copy_to_dsm_mem(dsm_mem_addr, read_fit_out, size);
@@ -1168,7 +1170,8 @@ static void nvdimm_build_fit(Aml *dev)
 
     aml_append(method, aml_store(aml_sizeof(buf), buf_size));
     aml_append(method, aml_subtract(buf_size,
-                                    aml_int(4) /* the size of "STAU" */,
+                                    aml_int(8) /* the size of "STAU" and the
+                                                  consequent reserved field */,
                                     buf_size));
 
     /* if we read the end of fit. */
@@ -1177,7 +1180,7 @@ static void nvdimm_build_fit(Aml *dev)
     aml_append(method, ifctx);
 
     aml_append(method, aml_create_field(buf,
-                            aml_int(4 * BITS_PER_BYTE), /* offset at byte 4.*/
+                            aml_int(8 * BITS_PER_BYTE), /* offset at byte 8. */
                             aml_shiftleft(buf_size, aml_int(3)), "BUFF"));
     aml_append(method, aml_return(aml_name("BUFF")));
     aml_append(dev, method);
-- 
2.15.1

[Qemu-devel] [RFC QEMU PATCH v4 10/10] xen-hvm: enable building NFIT and SSDT of vNVDIMM for HVM domains

[Haozhong Zhang]

When QEMU is used the device model of Xen HVM domain and vNVDIMM
devices are present, enable building ACPI tables related to vNVDIMM.

Signed-off-by: Haozhong Zhang <haozhong.zhang@intel.com>
---
Cc: Stefano Stabellini <sstabellini@kernel.org>
Cc: Anthony Perard <anthony.perard@citrix.com>
Cc: "Michael S. Tsirkin" <mst@redhat.com>
Cc: Paolo Bonzini <pbonzini@redhat.com>
Cc: Richard Henderson <rth@twiddle.net>
Cc: Eduardo Habkost <ehabkost@redhat.com>
---
 hw/i386/xen/xen-hvm.c | 13 +++++++++++--
 1 file changed, 11 insertions(+), 2 deletions(-)

diff --git a/hw/i386/xen/xen-hvm.c b/hw/i386/xen/xen-hvm.c
index a7e99bd438..33447fc482 100644
--- a/hw/i386/xen/xen-hvm.c
+++ b/hw/i386/xen/xen-hvm.c
@@ -1236,6 +1236,11 @@ static void xen_wakeup_notifier(Notifier *notifier, void *data)
     xc_set_hvm_param(xen_xc, xen_domid, HVM_PARAM_ACPI_S_STATE, 0);
 }
 
+static bool xen_dm_acpi_build_enabled(PCMachineState *pcms)
+{
+    return pcms->acpi_nvdimm_state.is_enabled;
+}
+
 static void xen_fw_cfg_init(PCMachineState *pcms)
 {
     FWCfgState *fw_cfg = fw_cfg_init_io(FW_CFG_IO_BASE);
@@ -1392,8 +1397,7 @@ void xen_hvm_init(PCMachineState *pcms, MemoryRegion **ram_memory)
     xen_be_register_common();
     xen_read_physmap(state);
 
-    /* Disable ACPI build because Xen handles it */
-    pcms->acpi_build_enabled = false;
+    pcms->acpi_build_enabled = xen_dm_acpi_build_enabled(pcms);;
     if (pcms->acpi_build_enabled) {
         xen_fw_cfg_init(pcms);
     }
@@ -1486,6 +1490,11 @@ void xen_acpi_build(AcpiBuildTables *tables, GArray *table_offsets,
         return;
     }
 
+    if (pcms->acpi_nvdimm_state.is_enabled) {
+        nvdimm_build_acpi(table_offsets, tables_blob, tables->linker,
+                          &pcms->acpi_nvdimm_state, machine->ram_slots);
+    }
+
     /*
      * QEMU RSDP and RSDT are only used by hvmloader to enumerate
      * QEMU-built tables. HVM domains still use Xen-built RSDP and RSDT.
-- 
2.15.1

Re: [Qemu-devel] [RFC QEMU PATCH v4 02/10] xen-hvm: create the hotplug memory region on Xen

[Anthony PERARD]

On Thu, Dec 07, 2017 at 06:18:04PM +0800, Haozhong Zhang wrote:
> The guest physical address of vNVDIMM is allocated from the hotplug
> memory region, which is not created when QEMU is used as Xen device
> model. In order to use vNVDIMM for Xen HVM domains, this commit reuses
> the code for pc machine type to create the hotplug memory region for
> Xen HVM domains.
> 
> Signed-off-by: Haozhong Zhang <haozhong.zhang@intel.com>
> ---
> Cc: "Michael S. Tsirkin" <mst@redhat.com>
> Cc: Paolo Bonzini <pbonzini@redhat.com>
> Cc: Richard Henderson <rth@twiddle.net>
> Cc: Eduardo Habkost <ehabkost@redhat.com>
> Cc: Stefano Stabellini <sstabellini@kernel.org>
> Cc: Anthony Perard <anthony.perard@citrix.com>
> ---
>  hw/i386/pc.c          | 86 ++++++++++++++++++++++++++++-----------------------
>  hw/i386/xen/xen-hvm.c |  2 ++
>  include/hw/i386/pc.h  |  1 +
>  3 files changed, 51 insertions(+), 38 deletions(-)
> 
> diff --git a/hw/i386/pc.c b/hw/i386/pc.c
> index 186545d2a4..9f46c8df79 100644
> --- a/hw/i386/pc.c
> +++ b/hw/i386/pc.c
> @@ -1315,6 +1315,53 @@ void xen_load_linux(PCMachineState *pcms)
>      pcms->fw_cfg = fw_cfg;
>  }
>  
> +void pc_memory_hotplug_init(PCMachineState *pcms, MemoryRegion *system_memory)

It might be better to have a separate patch which move the code into a function.

> +{
> +    MachineState *machine = MACHINE(pcms);
> +    PCMachineClass *pcmc = PC_MACHINE_GET_CLASS(pcms);
> +    ram_addr_t hotplug_mem_size = machine->maxram_size - machine->ram_size;
> +
> +    if (!pcmc->has_reserved_memory || machine->ram_size >= machine->maxram_size)
> +        return;
> +
> +    if (memory_region_size(&pcms->hotplug_memory.mr)) {

This new check looks like to catch programming error, rather than user
error. Would it be better to be an assert instead?

> +        error_report("hotplug memory region has been initialized");
> +        exit(EXIT_FAILURE);
> +    }
> +

-- 
Anthony PERARD

Re: [Qemu-devel] [RFC QEMU PATCH v4 03/10] hostmem-xen: add a host memory backend for Xen

[Anthony PERARD]

On Thu, Dec 07, 2017 at 06:18:05PM +0800, Haozhong Zhang wrote:
> diff --git a/backends/hostmem.c b/backends/hostmem.c
> index ee2c2d5bfd..ba13a52994 100644
> --- a/backends/hostmem.c
> +++ b/backends/hostmem.c
> @@ -12,6 +12,7 @@
>  #include "qemu/osdep.h"
>  #include "sysemu/hostmem.h"
>  #include "hw/boards.h"
> +#include "hw/xen/xen.h"
>  #include "qapi/error.h"
>  #include "qapi/visitor.h"
>  #include "qapi-types.h"
> @@ -277,6 +278,14 @@ host_memory_backend_memory_complete(UserCreatable *uc, Error **errp)
>              goto out;
>          }
>  
> +        /*
> +         * The backend storage of MEMORY_BACKEND_XEN is managed by Xen,
> +         * so no further work in this function is needed.
> +         */
> +        if (xen_enabled() && !backend->mr.ram_block) {
> +            goto out;
> +        }
> +
>          ptr = memory_region_get_ram_ptr(&backend->mr);
>          sz = memory_region_size(&backend->mr);
>  
> diff --git a/hw/mem/pc-dimm.c b/hw/mem/pc-dimm.c
> index 66eace5a5c..dcbfce33d5 100644
> --- a/hw/mem/pc-dimm.c
> +++ b/hw/mem/pc-dimm.c
> @@ -28,6 +28,7 @@
>  #include "sysemu/kvm.h"
>  #include "trace.h"
>  #include "hw/virtio/vhost.h"
> +#include "hw/xen/xen.h"
>  
>  typedef struct pc_dimms_capacity {
>       uint64_t size;
> @@ -108,7 +109,10 @@ void pc_dimm_memory_plug(DeviceState *dev, MemoryHotplugState *hpms,
>      }
>  
>      memory_region_add_subregion(&hpms->mr, addr - hpms->base, mr);
> -    vmstate_register_ram(vmstate_mr, dev);
> +    /* memory-backend-xen is not backed by RAM. */
> +    if (!xen_enabled()) {

Is it possible to have the same condition as the one used in
host_memory_backend_memory_complete? i.e. base on whether the memory
region is mapped or not (backend->mr.ram_block).

> +        vmstate_register_ram(vmstate_mr, dev);
> +    }
>      numa_set_mem_node_id(addr, memory_region_size(mr), dimm->node);
>  
>  out:
> -- 
> 2.15.1
> 

-- 
Anthony PERARD

Re: [Qemu-devel] [RFC QEMU PATCH v4 05/10] xen-hvm: initialize fw_cfg interface

[Anthony PERARD]

On Thu, Dec 07, 2017 at 06:18:07PM +0800, Haozhong Zhang wrote:
> Xen is going to reuse QEMU to build ACPI of some devices (e.g., NFIT
> and SSDT for NVDIMM) for HVM domains. The existing QEMU ACPI build
> code requires a fw_cfg interface which will also be used to pass QEMU
> built ACPI to Xen. Therefore, we need to initialize fw_cfg when any
> ACPI is going to be built by QEMU.
> 
> Signed-off-by: Haozhong Zhang <haozhong.zhang@intel.com>
> ---
> Cc: Stefano Stabellini <sstabellini@kernel.org>
> Cc: Anthony Perard <anthony.perard@citrix.com>
> Cc: "Michael S. Tsirkin" <mst@redhat.com>
> Cc: Paolo Bonzini <pbonzini@redhat.com>
> Cc: Richard Henderson <rth@twiddle.net>
> Cc: Eduardo Habkost <ehabkost@redhat.com>
> ---
>  hw/i386/xen/xen-hvm.c | 12 ++++++++++++
>  1 file changed, 12 insertions(+)
> 
> diff --git a/hw/i386/xen/xen-hvm.c b/hw/i386/xen/xen-hvm.c
> index fe01b7a025..4b29f4052b 100644
> --- a/hw/i386/xen/xen-hvm.c
> +++ b/hw/i386/xen/xen-hvm.c
> @@ -14,6 +14,7 @@
>  #include "hw/pci/pci.h"
>  #include "hw/i386/pc.h"
>  #include "hw/i386/apic-msidef.h"
> +#include "hw/loader.h"
>  #include "hw/xen/xen_common.h"
>  #include "hw/xen/xen_backend.h"
>  #include "qmp-commands.h"
> @@ -1234,6 +1235,14 @@ static void xen_wakeup_notifier(Notifier *notifier, void *data)
>      xc_set_hvm_param(xen_xc, xen_domid, HVM_PARAM_ACPI_S_STATE, 0);
>  }
>  
> +static void xen_fw_cfg_init(PCMachineState *pcms)
> +{

The fw_cfg interface might already be initialized, it is used for
"direct kernel boot" on hvm. It is initialized in xen_load_linux().

> +    FWCfgState *fw_cfg = fw_cfg_init_io(FW_CFG_IO_BASE);
> +
> +    rom_set_fw(fw_cfg);
> +    pcms->fw_cfg = fw_cfg;
> +}
> +
>  void xen_hvm_init(PCMachineState *pcms, MemoryRegion **ram_memory)
>  {
>      int i, rc;
> @@ -1384,6 +1393,9 @@ void xen_hvm_init(PCMachineState *pcms, MemoryRegion **ram_memory)
>  
>      /* Disable ACPI build because Xen handles it */
>      pcms->acpi_build_enabled = false;
> +    if (pcms->acpi_build_enabled) {
> +        xen_fw_cfg_init(pcms);
> +    }
>  
>      return;
>  
> -- 
> 2.15.1
> 

-- 
Anthony PERARD

Re: [Qemu-devel] [RFC QEMU PATCH v4 00/10] Implement vNVDIMM for Xen HVM guest

[Anthony PERARD]

On Thu, Dec 07, 2017 at 06:18:02PM +0800, Haozhong Zhang wrote:
> This is the QEMU part patches that works with the associated Xen
> patches to enable vNVDIMM support for Xen HVM domains. Xen relies on
> QEMU to build guest NFIT and NVDIMM namespace devices, and allocate
> guest address space for vNVDIMM devices.

I've got other question, and maybe possible improvements.

When QEMU build the ACPI tables, it also initialize some MemoryRegion,
which use more guest memory. Do you know if those regions are used with
your patch series on Xen? Otherwise, we could try to avoid their
creation with this:
In xenfv_machine_options()
m->rom_file_has_mr = false;
(setting this in xen_hvm_init() would probably be better, but I havn't
try)

If this is possible, libxl would not need to allocate more memory for
the guest (dm_acpi_size).

-- 
Anthony PERARD

Re: [Qemu-devel] [RFC QEMU PATCH v4 02/10] xen-hvm: create the hotplug memory region on Xen

[Haozhong Zhang]

On 02/27/18 16:37 +0000, Anthony PERARD wrote:
> On Thu, Dec 07, 2017 at 06:18:04PM +0800, Haozhong Zhang wrote:
> > The guest physical address of vNVDIMM is allocated from the hotplug
> > memory region, which is not created when QEMU is used as Xen device
> > model. In order to use vNVDIMM for Xen HVM domains, this commit reuses
> > the code for pc machine type to create the hotplug memory region for
> > Xen HVM domains.
> > 
> > Signed-off-by: Haozhong Zhang <haozhong.zhang@intel.com>
> > ---
> > Cc: "Michael S. Tsirkin" <mst@redhat.com>
> > Cc: Paolo Bonzini <pbonzini@redhat.com>
> > Cc: Richard Henderson <rth@twiddle.net>
> > Cc: Eduardo Habkost <ehabkost@redhat.com>
> > Cc: Stefano Stabellini <sstabellini@kernel.org>
> > Cc: Anthony Perard <anthony.perard@citrix.com>
> > ---
> >  hw/i386/pc.c          | 86 ++++++++++++++++++++++++++++-----------------------
> >  hw/i386/xen/xen-hvm.c |  2 ++
> >  include/hw/i386/pc.h  |  1 +
> >  3 files changed, 51 insertions(+), 38 deletions(-)
> > 
> > diff --git a/hw/i386/pc.c b/hw/i386/pc.c
> > index 186545d2a4..9f46c8df79 100644
> > --- a/hw/i386/pc.c
> > +++ b/hw/i386/pc.c
> > @@ -1315,6 +1315,53 @@ void xen_load_linux(PCMachineState *pcms)
> >      pcms->fw_cfg = fw_cfg;
> >  }
> >  
> > +void pc_memory_hotplug_init(PCMachineState *pcms, MemoryRegion *system_memory)
> 
> It might be better to have a separate patch which move the code into a function.

will move it to a separate patch

> 
> > +{
> > +    MachineState *machine = MACHINE(pcms);
> > +    PCMachineClass *pcmc = PC_MACHINE_GET_CLASS(pcms);
> > +    ram_addr_t hotplug_mem_size = machine->maxram_size - machine->ram_size;
> > +
> > +    if (!pcmc->has_reserved_memory || machine->ram_size >= machine->maxram_size)
> > +        return;
> > +
> > +    if (memory_region_size(&pcms->hotplug_memory.mr)) {
> 
> This new check looks like to catch programming error, rather than user
> error. Would it be better to be an assert instead?

Well, this was a debugging check and I forgot to remove it before
sending the patch. I'll drop it in the next version.

Thanks,
Haozhong

> 
> > +        error_report("hotplug memory region has been initialized");
> > +        exit(EXIT_FAILURE);
> > +    }
> > +
> 
> -- 
> Anthony PERARD

Re: [Qemu-devel] [RFC QEMU PATCH v4 03/10] hostmem-xen: add a host memory backend for Xen

[Haozhong Zhang]

On 02/27/18 16:41 +0000, Anthony PERARD wrote:
> On Thu, Dec 07, 2017 at 06:18:05PM +0800, Haozhong Zhang wrote:
> > diff --git a/backends/hostmem.c b/backends/hostmem.c
> > index ee2c2d5bfd..ba13a52994 100644
> > --- a/backends/hostmem.c
> > +++ b/backends/hostmem.c
> > @@ -12,6 +12,7 @@
> >  #include "qemu/osdep.h"
> >  #include "sysemu/hostmem.h"
> >  #include "hw/boards.h"
> > +#include "hw/xen/xen.h"
> >  #include "qapi/error.h"
> >  #include "qapi/visitor.h"
> >  #include "qapi-types.h"
> > @@ -277,6 +278,14 @@ host_memory_backend_memory_complete(UserCreatable *uc, Error **errp)
> >              goto out;
> >          }
> >  
> > +        /*
> > +         * The backend storage of MEMORY_BACKEND_XEN is managed by Xen,
> > +         * so no further work in this function is needed.
> > +         */
> > +        if (xen_enabled() && !backend->mr.ram_block) {
> > +            goto out;
> > +        }
> > +
> >          ptr = memory_region_get_ram_ptr(&backend->mr);
> >          sz = memory_region_size(&backend->mr);
> >  
> > diff --git a/hw/mem/pc-dimm.c b/hw/mem/pc-dimm.c
> > index 66eace5a5c..dcbfce33d5 100644
> > --- a/hw/mem/pc-dimm.c
> > +++ b/hw/mem/pc-dimm.c
> > @@ -28,6 +28,7 @@
> >  #include "sysemu/kvm.h"
> >  #include "trace.h"
> >  #include "hw/virtio/vhost.h"
> > +#include "hw/xen/xen.h"
> >  
> >  typedef struct pc_dimms_capacity {
> >       uint64_t size;
> > @@ -108,7 +109,10 @@ void pc_dimm_memory_plug(DeviceState *dev, MemoryHotplugState *hpms,
> >      }
> >  
> >      memory_region_add_subregion(&hpms->mr, addr - hpms->base, mr);
> > -    vmstate_register_ram(vmstate_mr, dev);
> > +    /* memory-backend-xen is not backed by RAM. */
> > +    if (!xen_enabled()) {
> 
> Is it possible to have the same condition as the one used in
> host_memory_backend_memory_complete? i.e. base on whether the memory
> region is mapped or not (backend->mr.ram_block).

Like "if (!xen_enabled() || backend->mr.ram_block))"? No, it will mute
the abortion (vmstate_register_ram --> qemu_ram_set_idstr ) caused by
the case that !backend->mr.ram_block in the non-xen environment.

Haozhong

> 
> > +        vmstate_register_ram(vmstate_mr, dev);
> > +    }
> >      numa_set_mem_node_id(addr, memory_region_size(mr), dimm->node);
> >  
> >  out:
> > -- 
> > 2.15.1
> > 
> 
> -- 
> Anthony PERARD

Re: [Qemu-devel] [RFC QEMU PATCH v4 05/10] xen-hvm: initialize fw_cfg interface

[Haozhong Zhang]

On 02/27/18 16:46 +0000, Anthony PERARD wrote:
> On Thu, Dec 07, 2017 at 06:18:07PM +0800, Haozhong Zhang wrote:
> > Xen is going to reuse QEMU to build ACPI of some devices (e.g., NFIT
> > and SSDT for NVDIMM) for HVM domains. The existing QEMU ACPI build
> > code requires a fw_cfg interface which will also be used to pass QEMU
> > built ACPI to Xen. Therefore, we need to initialize fw_cfg when any
> > ACPI is going to be built by QEMU.
> > 
> > Signed-off-by: Haozhong Zhang <haozhong.zhang@intel.com>
> > ---
> > Cc: Stefano Stabellini <sstabellini@kernel.org>
> > Cc: Anthony Perard <anthony.perard@citrix.com>
> > Cc: "Michael S. Tsirkin" <mst@redhat.com>
> > Cc: Paolo Bonzini <pbonzini@redhat.com>
> > Cc: Richard Henderson <rth@twiddle.net>
> > Cc: Eduardo Habkost <ehabkost@redhat.com>
> > ---
> >  hw/i386/xen/xen-hvm.c | 12 ++++++++++++
> >  1 file changed, 12 insertions(+)
> > 
> > diff --git a/hw/i386/xen/xen-hvm.c b/hw/i386/xen/xen-hvm.c
> > index fe01b7a025..4b29f4052b 100644
> > --- a/hw/i386/xen/xen-hvm.c
> > +++ b/hw/i386/xen/xen-hvm.c
> > @@ -14,6 +14,7 @@
> >  #include "hw/pci/pci.h"
> >  #include "hw/i386/pc.h"
> >  #include "hw/i386/apic-msidef.h"
> > +#include "hw/loader.h"
> >  #include "hw/xen/xen_common.h"
> >  #include "hw/xen/xen_backend.h"
> >  #include "qmp-commands.h"
> > @@ -1234,6 +1235,14 @@ static void xen_wakeup_notifier(Notifier *notifier, void *data)
> >      xc_set_hvm_param(xen_xc, xen_domid, HVM_PARAM_ACPI_S_STATE, 0);
> >  }
> >  
> > +static void xen_fw_cfg_init(PCMachineState *pcms)
> > +{
> 
> The fw_cfg interface might already be initialized, it is used for
> "direct kernel boot" on hvm. It is initialized in xen_load_linux().
>

xen_hvm_init() --> xen_fw_cfg_init() are called before
xen_load_linux(). I'll add a check in xen_load_linux() to avoid
redoing fw_cfg_init_io and rom_set_fw.

Haozhong

> > +    FWCfgState *fw_cfg = fw_cfg_init_io(FW_CFG_IO_BASE);
> > +
> > +    rom_set_fw(fw_cfg);
> > +    pcms->fw_cfg = fw_cfg;
> > +}
> > +
> >  void xen_hvm_init(PCMachineState *pcms, MemoryRegion **ram_memory)
> >  {
> >      int i, rc;
> > @@ -1384,6 +1393,9 @@ void xen_hvm_init(PCMachineState *pcms, MemoryRegion **ram_memory)
> >  
> >      /* Disable ACPI build because Xen handles it */
> >      pcms->acpi_build_enabled = false;
> > +    if (pcms->acpi_build_enabled) {
> > +        xen_fw_cfg_init(pcms);
> > +    }
> >  
> >      return;
> >  
> > -- 
> > 2.15.1
> > 
> 
> -- 
> Anthony PERARD

Re: [Qemu-devel] [RFC QEMU PATCH v4 00/10] Implement vNVDIMM for Xen HVM guest

[Haozhong Zhang]

On 02/27/18 17:22 +0000, Anthony PERARD wrote:
> On Thu, Dec 07, 2017 at 06:18:02PM +0800, Haozhong Zhang wrote:
> > This is the QEMU part patches that works with the associated Xen
> > patches to enable vNVDIMM support for Xen HVM domains. Xen relies on
> > QEMU to build guest NFIT and NVDIMM namespace devices, and allocate
> > guest address space for vNVDIMM devices.
> 
> I've got other question, and maybe possible improvements.
> 
> When QEMU build the ACPI tables, it also initialize some MemoryRegion,
> which use more guest memory. Do you know if those regions are used with
> your patch series on Xen?

Yes, that's why dm_acpi_size is introduced.

> Otherwise, we could try to avoid their
> creation with this:
> In xenfv_machine_options()
> m->rom_file_has_mr = false;
> (setting this in xen_hvm_init() would probably be better, but I havn't
> try)

If my memory is correct, simply setting rom_file_has_mr to false does
not work (though I cannot remind the exact reason). I'll have a look
as the code to refresh my memory.

Haozhong

> 
> If this is possible, libxl would not need to allocate more memory for
> the guest (dm_acpi_size).
> 
> -- 
> Anthony PERARD

Re: [Qemu-devel] [RFC QEMU PATCH v4 03/10] hostmem-xen: add a host memory backend for Xen

[Anthony PERARD]

On Wed, Feb 28, 2018 at 03:56:54PM +0800, Haozhong Zhang wrote:
> On 02/27/18 16:41 +0000, Anthony PERARD wrote:
> > On Thu, Dec 07, 2017 at 06:18:05PM +0800, Haozhong Zhang wrote:
> > > @@ -108,7 +109,10 @@ void pc_dimm_memory_plug(DeviceState *dev, MemoryHotplugState *hpms,
> > >      }
> > >  
> > >      memory_region_add_subregion(&hpms->mr, addr - hpms->base, mr);
> > > -    vmstate_register_ram(vmstate_mr, dev);
> > > +    /* memory-backend-xen is not backed by RAM. */
> > > +    if (!xen_enabled()) {
> > 
> > Is it possible to have the same condition as the one used in
> > host_memory_backend_memory_complete? i.e. base on whether the memory
> > region is mapped or not (backend->mr.ram_block).
> 
> Like "if (!xen_enabled() || backend->mr.ram_block))"? No, it will mute
> the abortion (vmstate_register_ram --> qemu_ram_set_idstr ) caused by
> the case that !backend->mr.ram_block in the non-xen environment.

In non-xen environment, vmstate_register_ram() will be called, because
!xen_enabled() is true, it would not matter if there is a ram_block or
not.

But if there is a memory-backend that can run in a xen environment that
have a ram_block, vmstate_register_ram would not be called in the
origial patch, but if we use (!xen_enabled() || vmstate_mr->ram_block)
as condition then vmstate_register_ram will be called.

Is this make sense?

> > > +        vmstate_register_ram(vmstate_mr, dev);
> > > +    }
> > >      numa_set_mem_node_id(addr, memory_region_size(mr), dimm->node);
> > >  
> > >  out:

-- 
Anthony PERARD

Re: [Qemu-devel] [RFC QEMU PATCH v4 00/10] Implement vNVDIMM for Xen HVM guest

[Anthony PERARD]

On Wed, Feb 28, 2018 at 05:36:59PM +0800, Haozhong Zhang wrote:
> On 02/27/18 17:22 +0000, Anthony PERARD wrote:
> > On Thu, Dec 07, 2017 at 06:18:02PM +0800, Haozhong Zhang wrote:
> > > This is the QEMU part patches that works with the associated Xen
> > > patches to enable vNVDIMM support for Xen HVM domains. Xen relies on
> > > QEMU to build guest NFIT and NVDIMM namespace devices, and allocate
> > > guest address space for vNVDIMM devices.
> > 
> > I've got other question, and maybe possible improvements.
> > 
> > When QEMU build the ACPI tables, it also initialize some MemoryRegion,
> > which use more guest memory. Do you know if those regions are used with
> > your patch series on Xen?
> 
> Yes, that's why dm_acpi_size is introduced.
> 
> > Otherwise, we could try to avoid their
> > creation with this:
> > In xenfv_machine_options()
> > m->rom_file_has_mr = false;
> > (setting this in xen_hvm_init() would probably be better, but I havn't
> > try)
> 
> If my memory is correct, simply setting rom_file_has_mr to false does
> not work (though I cannot remind the exact reason). I'll have a look
> as the code to refresh my memory.

I've played a bit with this idea, but without a proper NVDIMM available
for the guest, so I don't know if it's going to work properly without
the mr.

To make it work, I had to disable some code in acpi_build_update() that
make use of the MemoryRegions, as well as an assert in acpi_setup().
After those small hacks, I could boot the guest, and I've check that the
expected ACPI tables where there, and they looked correct to my eyes.
And least `ndctl list` works and showed the nvdimm (that I have
configured on QEMU's cmdline).

But I may not have been far enough with my tests, and maybe something
later relies on the MRs, especially the _DSM method that I don't know if
it was working properly.

Anyway, that why I proposed the idea, and if we can avoid more
uncertainty about how much guest memory QEMU is going to use, that would
be good.

Thanks,

-- 
Anthony PERARD

Re: [Qemu-devel] [RFC QEMU PATCH v4 03/10] hostmem-xen: add a host memory backend for Xen

[Haozhong Zhang]

On 03/02/18 11:50 +0000, Anthony PERARD wrote:
> On Wed, Feb 28, 2018 at 03:56:54PM +0800, Haozhong Zhang wrote:
> > On 02/27/18 16:41 +0000, Anthony PERARD wrote:
> > > On Thu, Dec 07, 2017 at 06:18:05PM +0800, Haozhong Zhang wrote:
> > > > @@ -108,7 +109,10 @@ void pc_dimm_memory_plug(DeviceState *dev, MemoryHotplugState *hpms,
> > > >      }
> > > >  
> > > >      memory_region_add_subregion(&hpms->mr, addr - hpms->base, mr);
> > > > -    vmstate_register_ram(vmstate_mr, dev);
> > > > +    /* memory-backend-xen is not backed by RAM. */
> > > > +    if (!xen_enabled()) {
> > > 
> > > Is it possible to have the same condition as the one used in
> > > host_memory_backend_memory_complete? i.e. base on whether the memory
> > > region is mapped or not (backend->mr.ram_block).
> > 
> > Like "if (!xen_enabled() || backend->mr.ram_block))"? No, it will mute
> > the abortion (vmstate_register_ram --> qemu_ram_set_idstr ) caused by
> > the case that !backend->mr.ram_block in the non-xen environment.
> 
> In non-xen environment, vmstate_register_ram() will be called, because
> !xen_enabled() is true, it would not matter if there is a ram_block or
> not.

Sorry, I really meant 'if (backend->mr.ram_block)', which may mute the
abortion in non-xen environment. 'if (!xen_enabled())' keeps the
original semantics in non-xen environment, so it's unlikely to break
the non-xen usage.

Haozhong

> 
> But if there is a memory-backend that can run in a xen environment that
> have a ram_block, vmstate_register_ram would not be called in the
> origial patch, but if we use (!xen_enabled() || vmstate_mr->ram_block)
> as condition then vmstate_register_ram will be called.
> 
> Is this make sense?
> 
> > > > +        vmstate_register_ram(vmstate_mr, dev);
> > > > +    }
> > > >      numa_set_mem_node_id(addr, memory_region_size(mr), dimm->node);
> > > >  
> > > >  out:
> 
> -- 
> Anthony PERARD
> 

Re: [Qemu-devel] [Xen-devel] [RFC QEMU PATCH v4 00/10] Implement vNVDIMM for Xen HVM guest

[Haozhong Zhang]

On 03/02/18 12:03 +0000, Anthony PERARD wrote:
> On Wed, Feb 28, 2018 at 05:36:59PM +0800, Haozhong Zhang wrote:
> > On 02/27/18 17:22 +0000, Anthony PERARD wrote:
> > > On Thu, Dec 07, 2017 at 06:18:02PM +0800, Haozhong Zhang wrote:
> > > > This is the QEMU part patches that works with the associated Xen
> > > > patches to enable vNVDIMM support for Xen HVM domains. Xen relies on
> > > > QEMU to build guest NFIT and NVDIMM namespace devices, and allocate
> > > > guest address space for vNVDIMM devices.
> > > 
> > > I've got other question, and maybe possible improvements.
> > > 
> > > When QEMU build the ACPI tables, it also initialize some MemoryRegion,
> > > which use more guest memory. Do you know if those regions are used with
> > > your patch series on Xen?
> > 
> > Yes, that's why dm_acpi_size is introduced.
> > 
> > > Otherwise, we could try to avoid their
> > > creation with this:
> > > In xenfv_machine_options()
> > > m->rom_file_has_mr = false;
> > > (setting this in xen_hvm_init() would probably be better, but I havn't
> > > try)
> > 
> > If my memory is correct, simply setting rom_file_has_mr to false does
> > not work (though I cannot remind the exact reason). I'll have a look
> > as the code to refresh my memory.
> 
> I've played a bit with this idea, but without a proper NVDIMM available
> for the guest, so I don't know if it's going to work properly without
> the mr.
> 
> To make it work, I had to disable some code in acpi_build_update() that
> make use of the MemoryRegions, as well as an assert in acpi_setup().
> After those small hacks, I could boot the guest, and I've check that the
> expected ACPI tables where there, and they looked correct to my eyes.
> And least `ndctl list` works and showed the nvdimm (that I have
> configured on QEMU's cmdline).
> 
> But I may not have been far enough with my tests, and maybe something
> later relies on the MRs, especially the _DSM method that I don't know if
> it was working properly.
> 
> Anyway, that why I proposed the idea, and if we can avoid more
> uncertainty about how much guest memory QEMU is going to use, that would
> be good.
> 

Yes, I also tested some non-trivial _DSM methods and it looks rom
files without memory regions can work with Xen after some
modifications. I'll apply this idea in the next version if no other
issues are found.

Thanks,
Haozhong

Re: [Qemu-devel] [Xen-devel] [RFC QEMU PATCH v4 00/10] Implement vNVDIMM for Xen HVM guest

[Anthony PERARD]

On Tue, Mar 06, 2018 at 12:16:08PM +0800, Haozhong Zhang wrote:
> On 03/02/18 12:03 +0000, Anthony PERARD wrote:
> > On Wed, Feb 28, 2018 at 05:36:59PM +0800, Haozhong Zhang wrote:
> > > On 02/27/18 17:22 +0000, Anthony PERARD wrote:
> > > > On Thu, Dec 07, 2017 at 06:18:02PM +0800, Haozhong Zhang wrote:
> > > > > This is the QEMU part patches that works with the associated Xen
> > > > > patches to enable vNVDIMM support for Xen HVM domains. Xen relies on
> > > > > QEMU to build guest NFIT and NVDIMM namespace devices, and allocate
> > > > > guest address space for vNVDIMM devices.
> > > > 
> > > > I've got other question, and maybe possible improvements.
> > > > 
> > > > When QEMU build the ACPI tables, it also initialize some MemoryRegion,
> > > > which use more guest memory. Do you know if those regions are used with
> > > > your patch series on Xen?
> > > 
> > > Yes, that's why dm_acpi_size is introduced.
> > > 
> > > > Otherwise, we could try to avoid their
> > > > creation with this:
> > > > In xenfv_machine_options()
> > > > m->rom_file_has_mr = false;
> > > > (setting this in xen_hvm_init() would probably be better, but I havn't
> > > > try)
> > > 
> > > If my memory is correct, simply setting rom_file_has_mr to false does
> > > not work (though I cannot remind the exact reason). I'll have a look
> > > as the code to refresh my memory.
> > 
> > I've played a bit with this idea, but without a proper NVDIMM available
> > for the guest, so I don't know if it's going to work properly without
> > the mr.
> > 
> > To make it work, I had to disable some code in acpi_build_update() that
> > make use of the MemoryRegions, as well as an assert in acpi_setup().
> > After those small hacks, I could boot the guest, and I've check that the
> > expected ACPI tables where there, and they looked correct to my eyes.
> > And least `ndctl list` works and showed the nvdimm (that I have
> > configured on QEMU's cmdline).
> > 
> > But I may not have been far enough with my tests, and maybe something
> > later relies on the MRs, especially the _DSM method that I don't know if
> > it was working properly.
> > 
> > Anyway, that why I proposed the idea, and if we can avoid more
> > uncertainty about how much guest memory QEMU is going to use, that would
> > be good.
> > 
> 
> Yes, I also tested some non-trivial _DSM methods and it looks rom
> files without memory regions can work with Xen after some
> modifications. I'll apply this idea in the next version if no other
> issues are found.

Awesome, thanks.

-- 
Anthony PERARD