All of lore.kernel.org
 help / color / mirror / Atom feed
* [Qemu-devel] [PULL 0/7] pc: fixes
@ 2018-08-20 20:24 Michael S. Tsirkin
  2018-08-20 20:24 ` [Qemu-devel] [PULL 1/7] memory, exec: Expose all memory block related flags Michael S. Tsirkin
                   ` (7 more replies)
  0 siblings, 8 replies; 14+ messages in thread
From: Michael S. Tsirkin @ 2018-08-20 20:24 UTC (permalink / raw)
  To: qemu-devel; +Cc: Peter Maydell

The following changes since commit f7502360397d291be04bc040e9f96c92ff2d8030:

  Update version for v3.0.0-rc3 release (2018-07-31 19:30:17 +0100)

are available in the Git repository at:

  git://git.kernel.org/pub/scm/virt/kvm/mst/qemu.git tags/for_upstream

for you to fetch changes up to 56eb90af39abf66c0e80588a9f50c31e7df7320b:

  migration/ram: ensure write persistence on loading all data to PMEM. (2018-08-10 13:29:39 +0300)

----------------------------------------------------------------
pc: fixes

This includes nvdimm persistence fixes queued before the release.

Signed-off-by: Michael S. Tsirkin <mst@redhat.com>

----------------------------------------------------------------
Junyan He (7):
      memory, exec: Expose all memory block related flags.
      memory, exec: switch file ram allocation functions to 'flags' parameters
      configure: add libpmem support
      hostmem-file: add the 'pmem' option
      mem/nvdimm: ensure write persistence to PMEM in label emulation
      migration/ram: Add check and info message to nvdimm post copy.
      migration/ram: ensure write persistence on loading all data to PMEM.

 docs/nvdimm.txt         | 22 ++++++++++++++++++++++
 configure               | 29 +++++++++++++++++++++++++++++
 include/exec/memory.h   | 31 +++++++++++++++++++++++++++++--
 include/exec/ram_addr.h | 28 ++++++++++++++++++++++++++--
 include/qemu/pmem.h     | 36 ++++++++++++++++++++++++++++++++++++
 backends/hostmem-file.c | 44 ++++++++++++++++++++++++++++++++++++++++++--
 exec.c                  | 38 +++++++++++++-------------------------
 hw/mem/nvdimm.c         |  9 ++++++++-
 memory.c                |  8 +++++---
 migration/ram.c         | 17 +++++++++++++++++
 numa.c                  |  2 +-
 qemu-options.hx         |  7 +++++++
 12 files changed, 235 insertions(+), 36 deletions(-)
 create mode 100644 include/qemu/pmem.h

^ permalink raw reply	[flat|nested] 14+ messages in thread

* [Qemu-devel] [PULL 1/7] memory, exec: Expose all memory block related flags.
  2018-08-20 20:24 [Qemu-devel] [PULL 0/7] pc: fixes Michael S. Tsirkin
@ 2018-08-20 20:24 ` Michael S. Tsirkin
  2018-08-20 20:24 ` [Qemu-devel] [PULL 2/7] memory, exec: switch file ram allocation functions to 'flags' parameters Michael S. Tsirkin
                   ` (6 subsequent siblings)
  7 siblings, 0 replies; 14+ messages in thread
From: Michael S. Tsirkin @ 2018-08-20 20:24 UTC (permalink / raw)
  To: qemu-devel
  Cc: Peter Maydell, Junyan He, Stefan Hajnoczi, Igor Mammedov,
	Richard Henderson, Paolo Bonzini, Peter Crosthwaite,
	Richard Henderson

From: Junyan He <junyan.he@intel.com>

We need to use these flags in other files rather than just in exec.c,
For example, RAM_SHARED should be used when create a ram block from file.
We expose them the exec/memory.h

Signed-off-by: Junyan He <junyan.he@intel.com>
Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>
Reviewed-by: Igor Mammedov <imammedo@redhat.com>
Reviewed-by: Richard Henderson <richard.henderson@linaro.org>
Reviewed-by: Michael S. Tsirkin <mst@redhat.com>
Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
---
 include/exec/memory.h | 20 ++++++++++++++++++++
 exec.c                | 20 --------------------
 2 files changed, 20 insertions(+), 20 deletions(-)

diff --git a/include/exec/memory.h b/include/exec/memory.h
index 448d41a752..6d0af29155 100644
--- a/include/exec/memory.h
+++ b/include/exec/memory.h
@@ -103,6 +103,26 @@ struct IOMMUNotifier {
 };
 typedef struct IOMMUNotifier IOMMUNotifier;
 
+/* RAM is pre-allocated and passed into qemu_ram_alloc_from_ptr */
+#define RAM_PREALLOC   (1 << 0)
+
+/* RAM is mmap-ed with MAP_SHARED */
+#define RAM_SHARED     (1 << 1)
+
+/* Only a portion of RAM (used_length) is actually used, and migrated.
+ * This used_length size can change across reboots.
+ */
+#define RAM_RESIZEABLE (1 << 2)
+
+/* UFFDIO_ZEROPAGE is available on this RAMBlock to atomically
+ * zero the page and wake waiting processes.
+ * (Set during postcopy)
+ */
+#define RAM_UF_ZEROPAGE (1 << 3)
+
+/* RAM can be migrated */
+#define RAM_MIGRATABLE (1 << 4)
+
 static inline void iommu_notifier_init(IOMMUNotifier *n, IOMMUNotify fn,
                                        IOMMUNotifierFlag flags,
                                        hwaddr start, hwaddr end,
diff --git a/exec.c b/exec.c
index 4f5df07b6a..cc042dcefd 100644
--- a/exec.c
+++ b/exec.c
@@ -87,26 +87,6 @@ AddressSpace address_space_memory;
 
 MemoryRegion io_mem_rom, io_mem_notdirty;
 static MemoryRegion io_mem_unassigned;
-
-/* RAM is pre-allocated and passed into qemu_ram_alloc_from_ptr */
-#define RAM_PREALLOC   (1 << 0)
-
-/* RAM is mmap-ed with MAP_SHARED */
-#define RAM_SHARED     (1 << 1)
-
-/* Only a portion of RAM (used_length) is actually used, and migrated.
- * This used_length size can change across reboots.
- */
-#define RAM_RESIZEABLE (1 << 2)
-
-/* UFFDIO_ZEROPAGE is available on this RAMBlock to atomically
- * zero the page and wake waiting processes.
- * (Set during postcopy)
- */
-#define RAM_UF_ZEROPAGE (1 << 3)
-
-/* RAM can be migrated */
-#define RAM_MIGRATABLE (1 << 4)
 #endif
 
 #ifdef TARGET_PAGE_BITS_VARY
-- 
MST

^ permalink raw reply related	[flat|nested] 14+ messages in thread

* [Qemu-devel] [PULL 2/7] memory, exec: switch file ram allocation functions to 'flags' parameters
  2018-08-20 20:24 [Qemu-devel] [PULL 0/7] pc: fixes Michael S. Tsirkin
  2018-08-20 20:24 ` [Qemu-devel] [PULL 1/7] memory, exec: Expose all memory block related flags Michael S. Tsirkin
@ 2018-08-20 20:24 ` Michael S. Tsirkin
  2018-08-20 20:24 ` [Qemu-devel] [PULL 3/7] configure: add libpmem support Michael S. Tsirkin
                   ` (5 subsequent siblings)
  7 siblings, 0 replies; 14+ messages in thread
From: Michael S. Tsirkin @ 2018-08-20 20:24 UTC (permalink / raw)
  To: qemu-devel
  Cc: Peter Maydell, Junyan He, Haozhong Zhang, Stefan Hajnoczi,
	Igor Mammedov, Richard Henderson, Eduardo Habkost, Paolo Bonzini,
	Peter Crosthwaite, Richard Henderson

From: Junyan He <junyan.he@intel.com>

As more flag parameters besides the existing 'share' are going to be
added to following functions
memory_region_init_ram_from_file
qemu_ram_alloc_from_fd
qemu_ram_alloc_from_file
let's switch them to use the 'flags' parameters so as to ease future
flag additions.

The existing 'share' flag is converted to the RAM_SHARED bit in ram_flags,
and other flag bits are ignored by above functions right now.

Signed-off-by: Junyan He <junyan.he@intel.com>
Signed-off-by: Haozhong Zhang <haozhong.zhang@intel.com>
Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>
Reviewed-by: Igor Mammedov <imammedo@redhat.com>
Reviewed-by: Michael S. Tsirkin <mst@redhat.com>
Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
Reviewed-by: Richard Henderson <richard.henderson@linaro.org>
---
 include/exec/memory.h   |  7 +++++--
 include/exec/ram_addr.h | 25 +++++++++++++++++++++++--
 backends/hostmem-file.c |  3 ++-
 exec.c                  | 10 +++++-----
 memory.c                |  8 +++++---
 numa.c                  |  2 +-
 6 files changed, 41 insertions(+), 14 deletions(-)

diff --git a/include/exec/memory.h b/include/exec/memory.h
index 6d0af29155..30e7166dd1 100644
--- a/include/exec/memory.h
+++ b/include/exec/memory.h
@@ -640,6 +640,7 @@ void memory_region_init_resizeable_ram(MemoryRegion *mr,
                                                        void *host),
                                        Error **errp);
 #ifdef __linux__
+
 /**
  * memory_region_init_ram_from_file:  Initialize RAM memory region with a
  *                                    mmap-ed backend.
@@ -651,7 +652,9 @@ void memory_region_init_resizeable_ram(MemoryRegion *mr,
  * @size: size of the region.
  * @align: alignment of the region base address; if 0, the default alignment
  *         (getpagesize()) will be used.
- * @share: %true if memory must be mmaped with the MAP_SHARED flag
+ * @ram_flags: Memory region features:
+ *             - RAM_SHARED: memory must be mmaped with the MAP_SHARED flag
+ *             Other bits are ignored now.
  * @path: the path in which to allocate the RAM.
  * @errp: pointer to Error*, to store an error if it happens.
  *
@@ -663,7 +666,7 @@ void memory_region_init_ram_from_file(MemoryRegion *mr,
                                       const char *name,
                                       uint64_t size,
                                       uint64_t align,
-                                      bool share,
+                                      uint32_t ram_flags,
                                       const char *path,
                                       Error **errp);
 
diff --git a/include/exec/ram_addr.h b/include/exec/ram_addr.h
index cf4ce06248..8a4a9bc614 100644
--- a/include/exec/ram_addr.h
+++ b/include/exec/ram_addr.h
@@ -71,12 +71,33 @@ static inline unsigned long int ramblock_recv_bitmap_offset(void *host_addr,
 }
 
 long qemu_getrampagesize(void);
+
+/**
+ * qemu_ram_alloc_from_file,
+ * qemu_ram_alloc_from_fd:  Allocate a ram block from the specified backing
+ *                          file or device
+ *
+ * Parameters:
+ *  @size: the size in bytes of the ram block
+ *  @mr: the memory region where the ram block is
+ *  @ram_flags: specify the properties of the ram block, which can be one
+ *              or bit-or of following values
+ *              - RAM_SHARED: mmap the backing file or device with MAP_SHARED
+ *              Other bits are ignored.
+ *  @mem_path or @fd: specify the backing file or device
+ *  @errp: pointer to Error*, to store an error if it happens
+ *
+ * Return:
+ *  On success, return a pointer to the ram block.
+ *  On failure, return NULL.
+ */
 RAMBlock *qemu_ram_alloc_from_file(ram_addr_t size, MemoryRegion *mr,
-                                   bool share, const char *mem_path,
+                                   uint32_t ram_flags, const char *mem_path,
                                    Error **errp);
 RAMBlock *qemu_ram_alloc_from_fd(ram_addr_t size, MemoryRegion *mr,
-                                 bool share, int fd,
+                                 uint32_t ram_flags, int fd,
                                  Error **errp);
+
 RAMBlock *qemu_ram_alloc_from_ptr(ram_addr_t size, void *host,
                                   MemoryRegion *mr, Error **errp);
 RAMBlock *qemu_ram_alloc(ram_addr_t size, bool share, MemoryRegion *mr,
diff --git a/backends/hostmem-file.c b/backends/hostmem-file.c
index 134b08d63a..34c68bb081 100644
--- a/backends/hostmem-file.c
+++ b/backends/hostmem-file.c
@@ -58,7 +58,8 @@ file_backend_memory_alloc(HostMemoryBackend *backend, Error **errp)
         path = object_get_canonical_path(OBJECT(backend));
         memory_region_init_ram_from_file(&backend->mr, OBJECT(backend),
                                  path,
-                                 backend->size, fb->align, backend->share,
+                                 backend->size, fb->align,
+                                 backend->share ? RAM_SHARED : 0,
                                  fb->mem_path, errp);
         g_free(path);
     }
diff --git a/exec.c b/exec.c
index cc042dcefd..3b8f91448d 100644
--- a/exec.c
+++ b/exec.c
@@ -2238,7 +2238,7 @@ static void ram_block_add(RAMBlock *new_block, Error **errp, bool shared)
 
 #ifdef __linux__
 RAMBlock *qemu_ram_alloc_from_fd(ram_addr_t size, MemoryRegion *mr,
-                                 bool share, int fd,
+                                 uint32_t ram_flags, int fd,
                                  Error **errp)
 {
     RAMBlock *new_block;
@@ -2280,14 +2280,14 @@ RAMBlock *qemu_ram_alloc_from_fd(ram_addr_t size, MemoryRegion *mr,
     new_block->mr = mr;
     new_block->used_length = size;
     new_block->max_length = size;
-    new_block->flags = share ? RAM_SHARED : 0;
+    new_block->flags = ram_flags;
     new_block->host = file_ram_alloc(new_block, size, fd, !file_size, errp);
     if (!new_block->host) {
         g_free(new_block);
         return NULL;
     }
 
-    ram_block_add(new_block, &local_err, share);
+    ram_block_add(new_block, &local_err, ram_flags & RAM_SHARED);
     if (local_err) {
         g_free(new_block);
         error_propagate(errp, local_err);
@@ -2299,7 +2299,7 @@ RAMBlock *qemu_ram_alloc_from_fd(ram_addr_t size, MemoryRegion *mr,
 
 
 RAMBlock *qemu_ram_alloc_from_file(ram_addr_t size, MemoryRegion *mr,
-                                   bool share, const char *mem_path,
+                                   uint32_t ram_flags, const char *mem_path,
                                    Error **errp)
 {
     int fd;
@@ -2311,7 +2311,7 @@ RAMBlock *qemu_ram_alloc_from_file(ram_addr_t size, MemoryRegion *mr,
         return NULL;
     }
 
-    block = qemu_ram_alloc_from_fd(size, mr, share, fd, errp);
+    block = qemu_ram_alloc_from_fd(size, mr, ram_flags, fd, errp);
     if (!block) {
         if (created) {
             unlink(mem_path);
diff --git a/memory.c b/memory.c
index e9cd446968..d20f0a76fe 100644
--- a/memory.c
+++ b/memory.c
@@ -1551,7 +1551,7 @@ void memory_region_init_ram_from_file(MemoryRegion *mr,
                                       const char *name,
                                       uint64_t size,
                                       uint64_t align,
-                                      bool share,
+                                      uint32_t ram_flags,
                                       const char *path,
                                       Error **errp)
 {
@@ -1560,7 +1560,7 @@ void memory_region_init_ram_from_file(MemoryRegion *mr,
     mr->terminates = true;
     mr->destructor = memory_region_destructor_ram;
     mr->align = align;
-    mr->ram_block = qemu_ram_alloc_from_file(size, mr, share, path, errp);
+    mr->ram_block = qemu_ram_alloc_from_file(size, mr, ram_flags, path, errp);
     mr->dirty_log_mask = tcg_enabled() ? (1 << DIRTY_MEMORY_CODE) : 0;
 }
 
@@ -1576,7 +1576,9 @@ void memory_region_init_ram_from_fd(MemoryRegion *mr,
     mr->ram = true;
     mr->terminates = true;
     mr->destructor = memory_region_destructor_ram;
-    mr->ram_block = qemu_ram_alloc_from_fd(size, mr, share, fd, errp);
+    mr->ram_block = qemu_ram_alloc_from_fd(size, mr,
+                                           share ? RAM_SHARED : 0,
+                                           fd, errp);
     mr->dirty_log_mask = tcg_enabled() ? (1 << DIRTY_MEMORY_CODE) : 0;
 }
 #endif
diff --git a/numa.c b/numa.c
index 5f6367b989..81542d4ebb 100644
--- a/numa.c
+++ b/numa.c
@@ -479,7 +479,7 @@ static void allocate_system_memory_nonnuma(MemoryRegion *mr, Object *owner,
     if (mem_path) {
 #ifdef __linux__
         Error *err = NULL;
-        memory_region_init_ram_from_file(mr, owner, name, ram_size, 0, false,
+        memory_region_init_ram_from_file(mr, owner, name, ram_size, 0, 0,
                                          mem_path, &err);
         if (err) {
             error_report_err(err);
-- 
MST

^ permalink raw reply related	[flat|nested] 14+ messages in thread

* [Qemu-devel] [PULL 3/7] configure: add libpmem support
  2018-08-20 20:24 [Qemu-devel] [PULL 0/7] pc: fixes Michael S. Tsirkin
  2018-08-20 20:24 ` [Qemu-devel] [PULL 1/7] memory, exec: Expose all memory block related flags Michael S. Tsirkin
  2018-08-20 20:24 ` [Qemu-devel] [PULL 2/7] memory, exec: switch file ram allocation functions to 'flags' parameters Michael S. Tsirkin
@ 2018-08-20 20:24 ` Michael S. Tsirkin
  2018-08-20 20:24 ` [Qemu-devel] [PULL 4/7] hostmem-file: add the 'pmem' option Michael S. Tsirkin
                   ` (4 subsequent siblings)
  7 siblings, 0 replies; 14+ messages in thread
From: Michael S. Tsirkin @ 2018-08-20 20:24 UTC (permalink / raw)
  To: qemu-devel
  Cc: Peter Maydell, Junyan He, Haozhong Zhang, Stefan Hajnoczi,
	Igor Mammedov, Richard Henderson, Gerd Hoffmann, Paolo Bonzini,
	Fam Zheng, Philippe Mathieu-Daudé

From: Junyan He <junyan.he@intel.com>

Add a pair of configure options --{enable,disable}-libpmem to control
whether QEMU is compiled with PMDK libpmem [1].

QEMU may write to the host persistent memory (e.g. in vNVDIMM label
emulation and live migration), so it must take the proper operations
to ensure the persistence of its own writes. Depending on the CPU
models and available instructions, the optimal operation can vary [2].
PMDK libpmem have already implemented those operations on multiple CPU
models (x86 and ARM) and the logic to select the optimal ones, so QEMU
can just use libpmem rather than re-implement them.

Libpem is a part of PMDK project(formerly known as NMVL).
The project's home page is: http://pmem.io/pmdk/
And the project's repository is: https://github.com/pmem/pmdk/

For more information about libpmem APIs, you can refer to the comments
in source code of: pmdk/src/libpmem/pmem.c, begin at line 33.

Signed-off-by: Junyan He <junyan.he@intel.com>
Signed-off-by: Haozhong Zhang <haozhong.zhang@intel.com>
Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>
Reviewed-by: Igor Mammedov <imammedo@redhat.com>
Reviewed-by: Richard Henderson <richard.henderson@linaro.org>
Reviewed-by: Michael S. Tsirkin <mst@redhat.com>
Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
---
 configure | 29 +++++++++++++++++++++++++++++
 1 file changed, 29 insertions(+)

diff --git a/configure b/configure
index 2a7796ea80..1c9288b17b 100755
--- a/configure
+++ b/configure
@@ -475,6 +475,7 @@ vxhs=""
 libxml2=""
 docker="no"
 debug_mutex="no"
+libpmem=""
 
 # cross compilers defaults, can be overridden with --cross-cc-ARCH
 cross_cc_aarch64="aarch64-linux-gnu-gcc"
@@ -1435,6 +1436,10 @@ for opt do
   ;;
   --disable-debug-mutex) debug_mutex=no
   ;;
+  --enable-libpmem) libpmem=yes
+  ;;
+  --disable-libpmem) libpmem=no
+  ;;
   *)
       echo "ERROR: unknown option $opt"
       echo "Try '$0 --help' for more information"
@@ -1710,6 +1715,7 @@ disabled with --disable-FEATURE, default is enabled if available:
   vhost-user      vhost-user support
   capstone        capstone disassembler support
   debug-mutex     mutex debugging support
+  libpmem         libpmem support
 
 NOTE: The object files are built at the place where configure is launched
 EOF
@@ -5545,6 +5551,24 @@ if has "docker"; then
     docker=$($python $source_path/tests/docker/docker.py probe)
 fi
 
+##########################################
+# check for libpmem
+
+if test "$libpmem" != "no"; then
+	if $pkg_config --exists "libpmem"; then
+		libpmem="yes"
+		libpmem_libs=$($pkg_config --libs libpmem)
+		libpmem_cflags=$($pkg_config --cflags libpmem)
+		libs_softmmu="$libs_softmmu $libpmem_libs"
+		QEMU_CFLAGS="$QEMU_CFLAGS $libpmem_cflags"
+	else
+		if test "$libpmem" = "yes" ; then
+			feature_not_found "libpmem" "Install nvml or pmdk"
+		fi
+		libpmem="no"
+	fi
+fi
+
 ##########################################
 # End of CC checks
 # After here, no more $cc or $ld runs
@@ -6010,6 +6034,7 @@ echo "replication support $replication"
 echo "VxHS block device $vxhs"
 echo "capstone          $capstone"
 echo "docker            $docker"
+echo "libpmem support   $libpmem"
 
 if test "$sdl_too_old" = "yes"; then
 echo "-> Your SDL version is too old - please upgrade to have SDL support"
@@ -6763,6 +6788,10 @@ if test "$vxhs" = "yes" ; then
   echo "VXHS_LIBS=$vxhs_libs" >> $config_host_mak
 fi
 
+if test "$libpmem" = "yes" ; then
+  echo "CONFIG_LIBPMEM=y" >> $config_host_mak
+fi
+
 if test "$tcg_interpreter" = "yes"; then
   QEMU_INCLUDES="-iquote \$(SRC_PATH)/tcg/tci $QEMU_INCLUDES"
 elif test "$ARCH" = "sparc64" ; then
-- 
MST

^ permalink raw reply related	[flat|nested] 14+ messages in thread

* [Qemu-devel] [PULL 4/7] hostmem-file: add the 'pmem' option
  2018-08-20 20:24 [Qemu-devel] [PULL 0/7] pc: fixes Michael S. Tsirkin
                   ` (2 preceding siblings ...)
  2018-08-20 20:24 ` [Qemu-devel] [PULL 3/7] configure: add libpmem support Michael S. Tsirkin
@ 2018-08-20 20:24 ` Michael S. Tsirkin
  2018-08-24 15:13   ` Peter Maydell
  2018-08-20 20:24 ` [Qemu-devel] [PULL 5/7] mem/nvdimm: ensure write persistence to PMEM in label emulation Michael S. Tsirkin
                   ` (3 subsequent siblings)
  7 siblings, 1 reply; 14+ messages in thread
From: Michael S. Tsirkin @ 2018-08-20 20:24 UTC (permalink / raw)
  To: qemu-devel
  Cc: Peter Maydell, Junyan He, Haozhong Zhang, Stefan Hajnoczi,
	Igor Mammedov, Richard Henderson, Eduardo Habkost, Paolo Bonzini,
	Peter Crosthwaite, Richard Henderson

From: Junyan He <junyan.he@intel.com>

When QEMU emulates vNVDIMM labels and migrates vNVDIMM devices, it
needs to know whether the backend storage is a real persistent memory,
in order to decide whether special operations should be performed to
ensure the data persistence.

This boolean option 'pmem' allows users to specify whether the backend
storage of memory-backend-file is a real persistent memory. If
'pmem=on', QEMU will set the flag RAM_PMEM in the RAM block of the
corresponding memory region. If 'pmem' is set while lack of libpmem
support, a error is generated.

Signed-off-by: Junyan He <junyan.he@intel.com>
Signed-off-by: Haozhong Zhang <haozhong.zhang@intel.com>
Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>
Reviewed-by: Igor Mammedov <imammedo@redhat.com>
Reviewed-by: Richard Henderson <richard.henderson@linaro.org>
Reviewed-by: Michael S. Tsirkin <mst@redhat.com>
Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
---
 docs/nvdimm.txt         | 22 +++++++++++++++++++++
 include/exec/memory.h   |  4 ++++
 include/exec/ram_addr.h |  3 +++
 backends/hostmem-file.c | 43 +++++++++++++++++++++++++++++++++++++++--
 exec.c                  |  8 ++++++++
 qemu-options.hx         |  7 +++++++
 6 files changed, 85 insertions(+), 2 deletions(-)

diff --git a/docs/nvdimm.txt b/docs/nvdimm.txt
index 24b443b655..5f158a6170 100644
--- a/docs/nvdimm.txt
+++ b/docs/nvdimm.txt
@@ -173,3 +173,25 @@ There are currently two valid values for this option:
              the NVDIMMs in the event of power loss.  This implies that the
              platform also supports flushing dirty data through the memory
              controller on power loss.
+
+If the vNVDIMM backend is in host persistent memory that can be accessed in
+SNIA NVM Programming Model [1] (e.g., Intel NVDIMM), it's suggested to set
+the 'pmem' option of memory-backend-file to 'on'. When 'pmem' is 'on' and QEMU
+is built with libpmem [2] support (configured with --enable-libpmem), QEMU
+will take necessary operations to guarantee the persistence of its own writes
+to the vNVDIMM backend(e.g., in vNVDIMM label emulation and live migration).
+If 'pmem' is 'on' while there is no libpmem support, qemu will exit and report
+a "lack of libpmem support" message to ensure the persistence is available.
+For example, if we want to ensure the persistence for some backend file,
+use the QEMU command line:
+
+    -object memory-backend-file,id=nv_mem,mem-path=/XXX/yyy,size=4G,pmem=on
+
+References
+----------
+
+[1] NVM Programming Model (NPM)
+	Version 1.2
+    https://www.snia.org/sites/default/files/technical_work/final/NVMProgrammingModel_v1.2.pdf
+[2] Persistent Memory Development Kit (PMDK), formerly known as NVML project, home page:
+    http://pmem.io/pmdk/
diff --git a/include/exec/memory.h b/include/exec/memory.h
index 30e7166dd1..cd62029a7d 100644
--- a/include/exec/memory.h
+++ b/include/exec/memory.h
@@ -123,6 +123,9 @@ typedef struct IOMMUNotifier IOMMUNotifier;
 /* RAM can be migrated */
 #define RAM_MIGRATABLE (1 << 4)
 
+/* RAM is a persistent kind memory */
+#define RAM_PMEM (1 << 5)
+
 static inline void iommu_notifier_init(IOMMUNotifier *n, IOMMUNotify fn,
                                        IOMMUNotifierFlag flags,
                                        hwaddr start, hwaddr end,
@@ -654,6 +657,7 @@ void memory_region_init_resizeable_ram(MemoryRegion *mr,
  *         (getpagesize()) will be used.
  * @ram_flags: Memory region features:
  *             - RAM_SHARED: memory must be mmaped with the MAP_SHARED flag
+ *             - RAM_PMEM: the memory is persistent memory
  *             Other bits are ignored now.
  * @path: the path in which to allocate the RAM.
  * @errp: pointer to Error*, to store an error if it happens.
diff --git a/include/exec/ram_addr.h b/include/exec/ram_addr.h
index 8a4a9bc614..3abb639056 100644
--- a/include/exec/ram_addr.h
+++ b/include/exec/ram_addr.h
@@ -70,6 +70,8 @@ static inline unsigned long int ramblock_recv_bitmap_offset(void *host_addr,
     return host_addr_offset >> TARGET_PAGE_BITS;
 }
 
+bool ramblock_is_pmem(RAMBlock *rb);
+
 long qemu_getrampagesize(void);
 
 /**
@@ -83,6 +85,7 @@ long qemu_getrampagesize(void);
  *  @ram_flags: specify the properties of the ram block, which can be one
  *              or bit-or of following values
  *              - RAM_SHARED: mmap the backing file or device with MAP_SHARED
+ *              - RAM_PMEM: the backend @mem_path or @fd is persistent memory
  *              Other bits are ignored.
  *  @mem_path or @fd: specify the backing file or device
  *  @errp: pointer to Error*, to store an error if it happens
diff --git a/backends/hostmem-file.c b/backends/hostmem-file.c
index 34c68bb081..2476dcb435 100644
--- a/backends/hostmem-file.c
+++ b/backends/hostmem-file.c
@@ -12,6 +12,7 @@
 #include "qemu/osdep.h"
 #include "qapi/error.h"
 #include "qemu-common.h"
+#include "qemu/error-report.h"
 #include "sysemu/hostmem.h"
 #include "sysemu/sysemu.h"
 #include "qom/object_interfaces.h"
@@ -31,9 +32,10 @@ typedef struct HostMemoryBackendFile HostMemoryBackendFile;
 struct HostMemoryBackendFile {
     HostMemoryBackend parent_obj;
 
-    bool discard_data;
     char *mem_path;
     uint64_t align;
+    bool discard_data;
+    bool is_pmem;
 };
 
 static void
@@ -59,7 +61,8 @@ file_backend_memory_alloc(HostMemoryBackend *backend, Error **errp)
         memory_region_init_ram_from_file(&backend->mr, OBJECT(backend),
                                  path,
                                  backend->size, fb->align,
-                                 backend->share ? RAM_SHARED : 0,
+                                 (backend->share ? RAM_SHARED : 0) |
+                                 (fb->is_pmem ? RAM_PMEM : 0),
                                  fb->mem_path, errp);
         g_free(path);
     }
@@ -131,6 +134,39 @@ static void file_memory_backend_set_align(Object *o, Visitor *v,
     error_propagate(errp, local_err);
 }
 
+static bool file_memory_backend_get_pmem(Object *o, Error **errp)
+{
+    return MEMORY_BACKEND_FILE(o)->is_pmem;
+}
+
+static void file_memory_backend_set_pmem(Object *o, bool value, Error **errp)
+{
+    HostMemoryBackend *backend = MEMORY_BACKEND(o);
+    HostMemoryBackendFile *fb = MEMORY_BACKEND_FILE(o);
+
+    if (host_memory_backend_mr_inited(backend)) {
+        error_setg(errp, "cannot change property 'pmem' of %s '%s'",
+                   object_get_typename(o),
+                   object_get_canonical_path_component(o));
+        return;
+    }
+
+#ifndef CONFIG_LIBPMEM
+    if (value) {
+        Error *local_err = NULL;
+        error_setg(&local_err,
+                   "Lack of libpmem support while setting the 'pmem=on'"
+                   " of %s '%s'. We can't ensure data persistence.",
+                   object_get_typename(o),
+                   object_get_canonical_path_component(o));
+        error_propagate(errp, local_err);
+        return;
+    }
+#endif
+
+    fb->is_pmem = value;
+}
+
 static void file_backend_unparent(Object *obj)
 {
     HostMemoryBackend *backend = MEMORY_BACKEND(obj);
@@ -162,6 +198,9 @@ file_backend_class_init(ObjectClass *oc, void *data)
         file_memory_backend_get_align,
         file_memory_backend_set_align,
         NULL, NULL, &error_abort);
+    object_class_property_add_bool(oc, "pmem",
+        file_memory_backend_get_pmem, file_memory_backend_set_pmem,
+        &error_abort);
 }
 
 static void file_backend_instance_finalize(Object *o)
diff --git a/exec.c b/exec.c
index 3b8f91448d..fa3fbc6646 100644
--- a/exec.c
+++ b/exec.c
@@ -2245,6 +2245,9 @@ RAMBlock *qemu_ram_alloc_from_fd(ram_addr_t size, MemoryRegion *mr,
     Error *local_err = NULL;
     int64_t file_size;
 
+    /* Just support these ram flags by now. */
+    assert((ram_flags & ~(RAM_SHARED | RAM_PMEM)) == 0);
+
     if (xen_enabled()) {
         error_setg(errp, "-mem-path not supported with Xen");
         return NULL;
@@ -4072,6 +4075,11 @@ err:
     return ret;
 }
 
+bool ramblock_is_pmem(RAMBlock *rb)
+{
+    return rb->flags & RAM_PMEM;
+}
+
 #endif
 
 void page_size_init(void)
diff --git a/qemu-options.hx b/qemu-options.hx
index b1bf0f485f..9b920f294f 100644
--- a/qemu-options.hx
+++ b/qemu-options.hx
@@ -4070,6 +4070,13 @@ requires an alignment different than the default one used by QEMU, eg
 the device DAX /dev/dax0.0 requires 2M alignment rather than 4K. In
 such cases, users can specify the required alignment via this option.
 
+The @option{pmem} option specifies whether the backing file specified
+by @option{mem-path} is in host persistent memory that can be accessed
+using the SNIA NVM programming model (e.g. Intel NVDIMM).
+If @option{pmem} is set to 'on', QEMU will take necessary operations to
+guarantee the persistence of its own writes to @option{mem-path}
+(e.g. in vNVDIMM label emulation and live migration).
+
 @item -object memory-backend-ram,id=@var{id},merge=@var{on|off},dump=@var{on|off},share=@var{on|off},prealloc=@var{on|off},size=@var{size},host-nodes=@var{host-nodes},policy=@var{default|preferred|bind|interleave}
 
 Creates a memory backend object, which can be used to back the guest RAM.
-- 
MST

^ permalink raw reply related	[flat|nested] 14+ messages in thread

* [Qemu-devel] [PULL 5/7] mem/nvdimm: ensure write persistence to PMEM in label emulation
  2018-08-20 20:24 [Qemu-devel] [PULL 0/7] pc: fixes Michael S. Tsirkin
                   ` (3 preceding siblings ...)
  2018-08-20 20:24 ` [Qemu-devel] [PULL 4/7] hostmem-file: add the 'pmem' option Michael S. Tsirkin
@ 2018-08-20 20:24 ` Michael S. Tsirkin
  2018-08-20 20:24 ` [Qemu-devel] [PULL 6/7] migration/ram: Add check and info message to nvdimm post copy Michael S. Tsirkin
                   ` (2 subsequent siblings)
  7 siblings, 0 replies; 14+ messages in thread
From: Michael S. Tsirkin @ 2018-08-20 20:24 UTC (permalink / raw)
  To: qemu-devel
  Cc: Peter Maydell, Junyan He, Haozhong Zhang, Stefan Hajnoczi,
	Igor Mammedov, Richard Henderson, Xiao Guangrong

From: Junyan He <junyan.he@intel.com>

Guest writes to vNVDIMM labels are intercepted and performed on the
backend by QEMU. When the backend is a real persistent memort, QEMU
needs to take proper operations to ensure its write persistence on the
persistent memory. Otherwise, a host power failure may result in the
loss of guest label configurations.

Signed-off-by: Haozhong Zhang <haozhong.zhang@intel.com>
Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>
Reviewed-by: Igor Mammedov <imammedo@redhat.com>
Reviewed-by: Michael S. Tsirkin <mst@redhat.com>
Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
Reviewed-by: Richard Henderson <richard.henderson@linaro.org>
---
 include/qemu/pmem.h | 30 ++++++++++++++++++++++++++++++
 hw/mem/nvdimm.c     |  9 ++++++++-
 2 files changed, 38 insertions(+), 1 deletion(-)
 create mode 100644 include/qemu/pmem.h

diff --git a/include/qemu/pmem.h b/include/qemu/pmem.h
new file mode 100644
index 0000000000..ebdb070210
--- /dev/null
+++ b/include/qemu/pmem.h
@@ -0,0 +1,30 @@
+/*
+ * QEMU header file for libpmem.
+ *
+ * Copyright (c) 2018 Intel Corporation.
+ *
+ * Author: Haozhong Zhang <address@hidden>
+ *
+ * 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_PMEM_H
+#define QEMU_PMEM_H
+
+#ifdef CONFIG_LIBPMEM
+#include <libpmem.h>
+#else  /* !CONFIG_LIBPMEM */
+
+static inline void *
+pmem_memcpy_persist(void *pmemdest, const void *src, size_t len)
+{
+    /* If 'pmem' option is 'on', we should always have libpmem support,
+       or qemu will report a error and exit, never come here. */
+    g_assert_not_reached();
+    return NULL;
+}
+
+#endif /* CONFIG_LIBPMEM */
+
+#endif /* !QEMU_PMEM_H */
diff --git a/hw/mem/nvdimm.c b/hw/mem/nvdimm.c
index 021d1c3997..1c6674c4ed 100644
--- a/hw/mem/nvdimm.c
+++ b/hw/mem/nvdimm.c
@@ -23,6 +23,7 @@
  */
 
 #include "qemu/osdep.h"
+#include "qemu/pmem.h"
 #include "qapi/error.h"
 #include "qapi/visitor.h"
 #include "hw/mem/nvdimm.h"
@@ -164,11 +165,17 @@ static void nvdimm_write_label_data(NVDIMMDevice *nvdimm, const void *buf,
 {
     MemoryRegion *mr;
     PCDIMMDevice *dimm = PC_DIMM(nvdimm);
+    bool is_pmem = object_property_get_bool(OBJECT(dimm->hostmem),
+                                            "pmem", NULL);
     uint64_t backend_offset;
 
     nvdimm_validate_rw_label_data(nvdimm, size, offset);
 
-    memcpy(nvdimm->label_data + offset, buf, size);
+    if (!is_pmem) {
+        memcpy(nvdimm->label_data + offset, buf, size);
+    } else {
+        pmem_memcpy_persist(nvdimm->label_data + offset, buf, size);
+    }
 
     mr = host_memory_backend_get_memory(dimm->hostmem);
     backend_offset = memory_region_size(mr) - nvdimm->label_size + offset;
-- 
MST

^ permalink raw reply related	[flat|nested] 14+ messages in thread

* [Qemu-devel] [PULL 6/7] migration/ram: Add check and info message to nvdimm post copy.
  2018-08-20 20:24 [Qemu-devel] [PULL 0/7] pc: fixes Michael S. Tsirkin
                   ` (4 preceding siblings ...)
  2018-08-20 20:24 ` [Qemu-devel] [PULL 5/7] mem/nvdimm: ensure write persistence to PMEM in label emulation Michael S. Tsirkin
@ 2018-08-20 20:24 ` Michael S. Tsirkin
  2018-08-20 20:24 ` [Qemu-devel] [PULL 7/7] migration/ram: ensure write persistence on loading all data to PMEM Michael S. Tsirkin
  2018-08-21 10:35 ` [Qemu-devel] [PULL 0/7] pc: fixes Peter Maydell
  7 siblings, 0 replies; 14+ messages in thread
From: Michael S. Tsirkin @ 2018-08-20 20:24 UTC (permalink / raw)
  To: qemu-devel
  Cc: Peter Maydell, Junyan He, Stefan Hajnoczi, Igor Mammedov,
	Juan Quintela, Dr. David Alan Gilbert

From: Junyan He <junyan.he@intel.com>

The nvdimm kind memory does not support post copy now.
We disable post copy if we have nvdimm memory and print some
log hint to user.

Signed-off-by: Junyan He <junyan.he@intel.com>
Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>
Reviewed-by: Igor Mammedov <imammedo@redhat.com>
Reviewed-by: Michael S. Tsirkin <mst@redhat.com>
Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
---
 migration/ram.c | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/migration/ram.c b/migration/ram.c
index 24dea2730c..5beefae7f5 100644
--- a/migration/ram.c
+++ b/migration/ram.c
@@ -3906,6 +3906,15 @@ static int ram_load(QEMUFile *f, void *opaque, int version_id)
 
 static bool ram_has_postcopy(void *opaque)
 {
+    RAMBlock *rb;
+    RAMBLOCK_FOREACH_MIGRATABLE(rb) {
+        if (ramblock_is_pmem(rb)) {
+            info_report("Block: %s, host: %p is a nvdimm memory, postcopy"
+                         "is not supported now!", rb->idstr, rb->host);
+            return false;
+        }
+    }
+
     return migrate_postcopy_ram();
 }
 
-- 
MST

^ permalink raw reply related	[flat|nested] 14+ messages in thread

* [Qemu-devel] [PULL 7/7] migration/ram: ensure write persistence on loading all data to PMEM.
  2018-08-20 20:24 [Qemu-devel] [PULL 0/7] pc: fixes Michael S. Tsirkin
                   ` (5 preceding siblings ...)
  2018-08-20 20:24 ` [Qemu-devel] [PULL 6/7] migration/ram: Add check and info message to nvdimm post copy Michael S. Tsirkin
@ 2018-08-20 20:24 ` Michael S. Tsirkin
  2018-08-21 10:35 ` [Qemu-devel] [PULL 0/7] pc: fixes Peter Maydell
  7 siblings, 0 replies; 14+ messages in thread
From: Michael S. Tsirkin @ 2018-08-20 20:24 UTC (permalink / raw)
  To: qemu-devel
  Cc: Peter Maydell, Junyan He, Stefan Hajnoczi, Igor Mammedov,
	Juan Quintela, Dr. David Alan Gilbert

From: Junyan He <junyan.he@intel.com>

Because we need to make sure the pmem kind memory data is synced
after migration, we choose to call pmem_persist() when the migration
finish. This will make sure the data of pmem is safe and will not
lose if power is off.

Signed-off-by: Junyan He <junyan.he@intel.com>
Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>
Reviewed-by: Igor Mammedov <imammedo@redhat.com>
Reviewed-by: Michael S. Tsirkin <mst@redhat.com>
Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
---
 include/qemu/pmem.h | 6 ++++++
 migration/ram.c     | 8 ++++++++
 2 files changed, 14 insertions(+)

diff --git a/include/qemu/pmem.h b/include/qemu/pmem.h
index ebdb070210..dfb6d0da62 100644
--- a/include/qemu/pmem.h
+++ b/include/qemu/pmem.h
@@ -25,6 +25,12 @@ pmem_memcpy_persist(void *pmemdest, const void *src, size_t len)
     return NULL;
 }
 
+static inline void
+pmem_persist(const void *addr, size_t len)
+{
+    g_assert_not_reached();
+}
+
 #endif /* CONFIG_LIBPMEM */
 
 #endif /* !QEMU_PMEM_H */
diff --git a/migration/ram.c b/migration/ram.c
index 5beefae7f5..fa79d0a5b9 100644
--- a/migration/ram.c
+++ b/migration/ram.c
@@ -33,6 +33,7 @@
 #include "qemu/bitops.h"
 #include "qemu/bitmap.h"
 #include "qemu/main-loop.h"
+#include "qemu/pmem.h"
 #include "xbzrle.h"
 #include "ram.h"
 #include "migration.h"
@@ -3547,6 +3548,13 @@ static int ram_load_setup(QEMUFile *f, void *opaque)
 static int ram_load_cleanup(void *opaque)
 {
     RAMBlock *rb;
+
+    RAMBLOCK_FOREACH_MIGRATABLE(rb) {
+        if (ramblock_is_pmem(rb)) {
+            pmem_persist(rb->host, rb->used_length);
+        }
+    }
+
     xbzrle_load_cleanup();
     compress_threads_load_cleanup();
 
-- 
MST

^ permalink raw reply related	[flat|nested] 14+ messages in thread

* Re: [Qemu-devel] [PULL 0/7] pc: fixes
  2018-08-20 20:24 [Qemu-devel] [PULL 0/7] pc: fixes Michael S. Tsirkin
                   ` (6 preceding siblings ...)
  2018-08-20 20:24 ` [Qemu-devel] [PULL 7/7] migration/ram: ensure write persistence on loading all data to PMEM Michael S. Tsirkin
@ 2018-08-21 10:35 ` Peter Maydell
  7 siblings, 0 replies; 14+ messages in thread
From: Peter Maydell @ 2018-08-21 10:35 UTC (permalink / raw)
  To: Michael S. Tsirkin; +Cc: QEMU Developers

On 20 August 2018 at 21:24, Michael S. Tsirkin <mst@redhat.com> wrote:
> The following changes since commit f7502360397d291be04bc040e9f96c92ff2d8030:
>
>   Update version for v3.0.0-rc3 release (2018-07-31 19:30:17 +0100)
>
> are available in the Git repository at:
>
>   git://git.kernel.org/pub/scm/virt/kvm/mst/qemu.git tags/for_upstream
>
> for you to fetch changes up to 56eb90af39abf66c0e80588a9f50c31e7df7320b:
>
>   migration/ram: ensure write persistence on loading all data to PMEM. (2018-08-10 13:29:39 +0300)
>
> ----------------------------------------------------------------
> pc: fixes
>
> This includes nvdimm persistence fixes queued before the release.
>
> Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
>

Applied, thanks.

-- PMM

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [Qemu-devel] [PULL 4/7] hostmem-file: add the 'pmem' option
  2018-08-20 20:24 ` [Qemu-devel] [PULL 4/7] hostmem-file: add the 'pmem' option Michael S. Tsirkin
@ 2018-08-24 15:13   ` Peter Maydell
  2018-08-24 16:53     ` Michael S. Tsirkin
  0 siblings, 1 reply; 14+ messages in thread
From: Peter Maydell @ 2018-08-24 15:13 UTC (permalink / raw)
  To: Michael S. Tsirkin
  Cc: QEMU Developers, Junyan He, Haozhong Zhang, Stefan Hajnoczi,
	Igor Mammedov, Richard Henderson, Eduardo Habkost, Paolo Bonzini,
	Peter Crosthwaite, Richard Henderson

On 20 August 2018 at 21:24, Michael S. Tsirkin <mst@redhat.com> wrote:
> From: Junyan He <junyan.he@intel.com>
>
> When QEMU emulates vNVDIMM labels and migrates vNVDIMM devices, it
> needs to know whether the backend storage is a real persistent memory,
> in order to decide whether special operations should be performed to
> ensure the data persistence.
>
> This boolean option 'pmem' allows users to specify whether the backend
> storage of memory-backend-file is a real persistent memory. If
> 'pmem=on', QEMU will set the flag RAM_PMEM in the RAM block of the
> corresponding memory region. If 'pmem' is set while lack of libpmem
> support, a error is generated.

Hi; Coverity reports (CID 1395184) that there is a resource leak
in an error-exit path in this function:

> +static void file_memory_backend_set_pmem(Object *o, bool value, Error **errp)
> +{
> +    HostMemoryBackend *backend = MEMORY_BACKEND(o);
> +    HostMemoryBackendFile *fb = MEMORY_BACKEND_FILE(o);
> +
> +    if (host_memory_backend_mr_inited(backend)) {
> +        error_setg(errp, "cannot change property 'pmem' of %s '%s'",
> +                   object_get_typename(o),
> +                   object_get_canonical_path_component(o));
> +        return;
> +    }
> +
> +#ifndef CONFIG_LIBPMEM
> +    if (value) {
> +        Error *local_err = NULL;
> +        error_setg(&local_err,
> +                   "Lack of libpmem support while setting the 'pmem=on'"
> +                   " of %s '%s'. We can't ensure data persistence.",
> +                   object_get_typename(o),
> +                   object_get_canonical_path_component(o));

object_get_canonical_path_component() returns a string which
must be freed using g_free().

> +        error_propagate(errp, local_err);
> +        return;
> +    }
> +#endif
> +
> +    fb->is_pmem = value;
> +}

thanks
-- PMM

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [Qemu-devel] [PULL 4/7] hostmem-file: add the 'pmem' option
  2018-08-24 15:13   ` Peter Maydell
@ 2018-08-24 16:53     ` Michael S. Tsirkin
  2018-08-24 16:57       ` Peter Maydell
  0 siblings, 1 reply; 14+ messages in thread
From: Michael S. Tsirkin @ 2018-08-24 16:53 UTC (permalink / raw)
  To: Peter Maydell
  Cc: QEMU Developers, Junyan He, Haozhong Zhang, Stefan Hajnoczi,
	Igor Mammedov, Richard Henderson, Eduardo Habkost, Paolo Bonzini,
	Peter Crosthwaite, Richard Henderson

On Fri, Aug 24, 2018 at 04:13:18PM +0100, Peter Maydell wrote:
> On 20 August 2018 at 21:24, Michael S. Tsirkin <mst@redhat.com> wrote:
> > From: Junyan He <junyan.he@intel.com>
> >
> > When QEMU emulates vNVDIMM labels and migrates vNVDIMM devices, it
> > needs to know whether the backend storage is a real persistent memory,
> > in order to decide whether special operations should be performed to
> > ensure the data persistence.
> >
> > This boolean option 'pmem' allows users to specify whether the backend
> > storage of memory-backend-file is a real persistent memory. If
> > 'pmem=on', QEMU will set the flag RAM_PMEM in the RAM block of the
> > corresponding memory region. If 'pmem' is set while lack of libpmem
> > support, a error is generated.
> 
> Hi; Coverity reports (CID 1395184) that there is a resource leak
> in an error-exit path in this function:
> 
> > +static void file_memory_backend_set_pmem(Object *o, bool value, Error **errp)
> > +{
> > +    HostMemoryBackend *backend = MEMORY_BACKEND(o);
> > +    HostMemoryBackendFile *fb = MEMORY_BACKEND_FILE(o);
> > +
> > +    if (host_memory_backend_mr_inited(backend)) {
> > +        error_setg(errp, "cannot change property 'pmem' of %s '%s'",
> > +                   object_get_typename(o),
> > +                   object_get_canonical_path_component(o));
> > +        return;
> > +    }
> > +
> > +#ifndef CONFIG_LIBPMEM
> > +    if (value) {
> > +        Error *local_err = NULL;
> > +        error_setg(&local_err,
> > +                   "Lack of libpmem support while setting the 'pmem=on'"
> > +                   " of %s '%s'. We can't ensure data persistence.",
> > +                   object_get_typename(o),
> > +                   object_get_canonical_path_component(o));
> 
> object_get_canonical_path_component() returns a string which
> must be freed using g_free().
> 
> > +        error_propagate(errp, local_err);
> > +        return;
> > +    }
> > +#endif
> > +
> > +    fb->is_pmem = value;
> > +}
> 
> thanks
> -- PMM


Like the following? Junyan, could you pls try this one and confirm?

Signed-off-by: Michael S. Tsirkin <mst@redhat.com>

diff --git a/backends/hostmem-file.c b/backends/hostmem-file.c
index 2476dcb435..d88125b86e 100644
--- a/backends/hostmem-file.c
+++ b/backends/hostmem-file.c
@@ -154,11 +154,14 @@ static void file_memory_backend_set_pmem(Object *o, bool value, Error **errp)
 #ifndef CONFIG_LIBPMEM
     if (value) {
         Error *local_err = NULL;
+        char *path = object_get_canonical_path_component(o);
+
         error_setg(&local_err,
                    "Lack of libpmem support while setting the 'pmem=on'"
                    " of %s '%s'. We can't ensure data persistence.",
                    object_get_typename(o),
-                   object_get_canonical_path_component(o));
+                   );
+        g_free(path);
         error_propagate(errp, local_err);
         return;
     }

^ permalink raw reply related	[flat|nested] 14+ messages in thread

* Re: [Qemu-devel] [PULL 4/7] hostmem-file: add the 'pmem' option
  2018-08-24 16:53     ` Michael S. Tsirkin
@ 2018-08-24 16:57       ` Peter Maydell
  2018-08-24 17:14         ` Michael S. Tsirkin
  0 siblings, 1 reply; 14+ messages in thread
From: Peter Maydell @ 2018-08-24 16:57 UTC (permalink / raw)
  To: Michael S. Tsirkin
  Cc: QEMU Developers, Junyan He, Haozhong Zhang, Stefan Hajnoczi,
	Igor Mammedov, Richard Henderson, Eduardo Habkost, Paolo Bonzini,
	Peter Crosthwaite, Richard Henderson

On 24 August 2018 at 17:53, Michael S. Tsirkin <mst@redhat.com> wrote:
> On Fri, Aug 24, 2018 at 04:13:18PM +0100, Peter Maydell wrote:
>> object_get_canonical_path_component() returns a string which
>> must be freed using g_free().

> Like the following? Junyan, could you pls try this one and confirm?
>
> Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
>
> diff --git a/backends/hostmem-file.c b/backends/hostmem-file.c
> index 2476dcb435..d88125b86e 100644
> --- a/backends/hostmem-file.c
> +++ b/backends/hostmem-file.c
> @@ -154,11 +154,14 @@ static void file_memory_backend_set_pmem(Object *o, bool value, Error **errp)
>  #ifndef CONFIG_LIBPMEM
>      if (value) {
>          Error *local_err = NULL;
> +        char *path = object_get_canonical_path_component(o);
> +
>          error_setg(&local_err,
>                     "Lack of libpmem support while setting the 'pmem=on'"
>                     " of %s '%s'. We can't ensure data persistence.",
>                     object_get_typename(o),
> -                   object_get_canonical_path_component(o));
> +                   );
> +        g_free(path);
>          error_propagate(errp, local_err);
>          return;

Yep, like that (though I would put the closing ");" on
the line with object_get_typename() and delete the trailing comma).

thanks
-- PMM

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [Qemu-devel] [PULL 4/7] hostmem-file: add the 'pmem' option
  2018-08-24 16:57       ` Peter Maydell
@ 2018-08-24 17:14         ` Michael S. Tsirkin
  2018-08-28 15:42           ` Yi Zhang
  0 siblings, 1 reply; 14+ messages in thread
From: Michael S. Tsirkin @ 2018-08-24 17:14 UTC (permalink / raw)
  To: Peter Maydell
  Cc: QEMU Developers, Junyan He, Haozhong Zhang, Stefan Hajnoczi,
	Igor Mammedov, Richard Henderson, Eduardo Habkost, Paolo Bonzini,
	Peter Crosthwaite, Richard Henderson

On Fri, Aug 24, 2018 at 05:57:06PM +0100, Peter Maydell wrote:
> On 24 August 2018 at 17:53, Michael S. Tsirkin <mst@redhat.com> wrote:
> > On Fri, Aug 24, 2018 at 04:13:18PM +0100, Peter Maydell wrote:
> >> object_get_canonical_path_component() returns a string which
> >> must be freed using g_free().
> 
> > Like the following? Junyan, could you pls try this one and confirm?
> >
> > Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> >
> > diff --git a/backends/hostmem-file.c b/backends/hostmem-file.c
> > index 2476dcb435..d88125b86e 100644
> > --- a/backends/hostmem-file.c
> > +++ b/backends/hostmem-file.c
> > @@ -154,11 +154,14 @@ static void file_memory_backend_set_pmem(Object *o, bool value, Error **errp)
> >  #ifndef CONFIG_LIBPMEM
> >      if (value) {
> >          Error *local_err = NULL;
> > +        char *path = object_get_canonical_path_component(o);
> > +
> >          error_setg(&local_err,
> >                     "Lack of libpmem support while setting the 'pmem=on'"
> >                     " of %s '%s'. We can't ensure data persistence.",
> >                     object_get_typename(o),
> > -                   object_get_canonical_path_component(o));
> > +                   );
> > +        g_free(path);
> >          error_propagate(errp, local_err);
> >          return;
> 
> Yep, like that (though I would put the closing ");" on
> the line with object_get_typename() and delete the trailing comma).
> 
> thanks
> -- PMM

oh i forgot to add in "path".
I didn't build with libpmem installed
Should have been (still untested):

Signed-off-by: Michael S. Tsirkin <mst@redhat.com>


diff --git a/backends/hostmem-file.c b/backends/hostmem-file.c
index 2476dcb435..72e7055ee7 100644
--- a/backends/hostmem-file.c
+++ b/backends/hostmem-file.c
@@ -154,11 +154,14 @@ static void file_memory_backend_set_pmem(Object *o, bool value, Error **errp)
 #ifndef CONFIG_LIBPMEM
     if (value) {
         Error *local_err = NULL;
+        char *path = object_get_canonical_path_component(o);
+
         error_setg(&local_err,
                    "Lack of libpmem support while setting the 'pmem=on'"
                    " of %s '%s'. We can't ensure data persistence.",
                    object_get_typename(o),
-                   object_get_canonical_path_component(o));
+                   path);
+        g_free(path);
         error_propagate(errp, local_err);
         return;
     }

^ permalink raw reply related	[flat|nested] 14+ messages in thread

* Re: [Qemu-devel] [PULL 4/7] hostmem-file: add the 'pmem' option
  2018-08-24 17:14         ` Michael S. Tsirkin
@ 2018-08-28 15:42           ` Yi Zhang
  0 siblings, 0 replies; 14+ messages in thread
From: Yi Zhang @ 2018-08-28 15:42 UTC (permalink / raw)
  To: Michael S. Tsirkin
  Cc: Peter Maydell, Haozhong Zhang, Eduardo Habkost,
	Peter Crosthwaite, Richard Henderson, QEMU Developers, Junyan He,
	Stefan Hajnoczi, Paolo Bonzini, Igor Mammedov, Richard Henderson

On 2018-08-24 at 20:14:37 +0300, Michael S. Tsirkin wrote:
> On Fri, Aug 24, 2018 at 05:57:06PM +0100, Peter Maydell wrote:
> > On 24 August 2018 at 17:53, Michael S. Tsirkin <mst@redhat.com> wrote:
> > > On Fri, Aug 24, 2018 at 04:13:18PM +0100, Peter Maydell wrote:
> > >> object_get_canonical_path_component() returns a string which
> > >> must be freed using g_free().
> > 
> > > Like the following? Junyan, could you pls try this one and confirm?
> > >
> > > Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> > >
> > > diff --git a/backends/hostmem-file.c b/backends/hostmem-file.c
> > > index 2476dcb435..d88125b86e 100644
> > > --- a/backends/hostmem-file.c
> > > +++ b/backends/hostmem-file.c
> > > @@ -154,11 +154,14 @@ static void file_memory_backend_set_pmem(Object *o, bool value, Error **errp)
> > >  #ifndef CONFIG_LIBPMEM
> > >      if (value) {
> > >          Error *local_err = NULL;
> > > +        char *path = object_get_canonical_path_component(o);
> > > +
> > >          error_setg(&local_err,
> > >                     "Lack of libpmem support while setting the 'pmem=on'"
> > >                     " of %s '%s'. We can't ensure data persistence.",
> > >                     object_get_typename(o),
> > > -                   object_get_canonical_path_component(o));
> > > +                   );
> > > +        g_free(path);
> > >          error_propagate(errp, local_err);
> > >          return;
> > 
> > Yep, like that (though I would put the closing ");" on
> > the line with object_get_typename() and delete the trailing comma).
> > 
> > thanks
> > -- PMM
>a

Ah.. Thanks Michael/Peter to identify/prepare this fix.

> oh i forgot to add in "path".
> I didn't build with libpmem installed

Maybe you already have libpmem installed yet, 
it is *ifndef* CONFIG_LIBPMEM ^_^

> Should have been (still untested):
> 
> Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> 
> 
> diff --git a/backends/hostmem-file.c b/backends/hostmem-file.c
> index 2476dcb435..72e7055ee7 100644
> --- a/backends/hostmem-file.c
> +++ b/backends/hostmem-file.c
> @@ -154,11 +154,14 @@ static void file_memory_backend_set_pmem(Object *o, bool value, Error **errp)
>  #ifndef CONFIG_LIBPMEM
>      if (value) {
>          Error *local_err = NULL;
> +        char *path = object_get_canonical_path_component(o);
> +
>          error_setg(&local_err,
>                     "Lack of libpmem support while setting the 'pmem=on'"
>                     " of %s '%s'. We can't ensure data persistence.",
>                     object_get_typename(o),
> -                   object_get_canonical_path_component(o));
> +                   path);
> +        g_free(path);
>          error_propagate(errp, local_err);
>          return;
>      }
> 
Ah.. I saw another place still have the seem leak, on the previous few lines in
file_memory_backend_set_pmem, I will prepare another patch to fix the
leaks in these two place.

Regards
Yi.

^ permalink raw reply	[flat|nested] 14+ messages in thread

end of thread, other threads:[~2018-08-28  7:04 UTC | newest]

Thread overview: 14+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-08-20 20:24 [Qemu-devel] [PULL 0/7] pc: fixes Michael S. Tsirkin
2018-08-20 20:24 ` [Qemu-devel] [PULL 1/7] memory, exec: Expose all memory block related flags Michael S. Tsirkin
2018-08-20 20:24 ` [Qemu-devel] [PULL 2/7] memory, exec: switch file ram allocation functions to 'flags' parameters Michael S. Tsirkin
2018-08-20 20:24 ` [Qemu-devel] [PULL 3/7] configure: add libpmem support Michael S. Tsirkin
2018-08-20 20:24 ` [Qemu-devel] [PULL 4/7] hostmem-file: add the 'pmem' option Michael S. Tsirkin
2018-08-24 15:13   ` Peter Maydell
2018-08-24 16:53     ` Michael S. Tsirkin
2018-08-24 16:57       ` Peter Maydell
2018-08-24 17:14         ` Michael S. Tsirkin
2018-08-28 15:42           ` Yi Zhang
2018-08-20 20:24 ` [Qemu-devel] [PULL 5/7] mem/nvdimm: ensure write persistence to PMEM in label emulation Michael S. Tsirkin
2018-08-20 20:24 ` [Qemu-devel] [PULL 6/7] migration/ram: Add check and info message to nvdimm post copy Michael S. Tsirkin
2018-08-20 20:24 ` [Qemu-devel] [PULL 7/7] migration/ram: ensure write persistence on loading all data to PMEM Michael S. Tsirkin
2018-08-21 10:35 ` [Qemu-devel] [PULL 0/7] pc: fixes Peter Maydell

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.