| Message ID | 20180820104045.133968-5-mst@redhat.com |
|---|---|
| State | New |
| Headers | show |
| Series | [PULL,1/7] memory, exec: Expose all memory block related flags. | expand |
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
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; }
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
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; }
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.
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.