* [PATCH v2 0/3] nvdimm: read-only file support
@ 2020-09-16 9:51 Stefan Hajnoczi
2020-09-16 9:51 ` [PATCH v2 1/3] memory: add readonly support to memory_region_init_ram_from_file() Stefan Hajnoczi
` (4 more replies)
0 siblings, 5 replies; 13+ messages in thread
From: Stefan Hajnoczi @ 2020-09-16 9:51 UTC (permalink / raw)
To: qemu-devel
Cc: Eduardo Habkost, Michael S. Tsirkin, Julio Montes,
Xiao Guangrong, eric.g.ernst, Stefan Hajnoczi, Igor Mammedov,
Paolo Bonzini, Richard Henderson
v2:
* s/the the/the/ in documentation [Philippe]
* Assign nvdimm->unarmed earlier [Philippe]
There is currently no way to back an NVDIMM with a read-only file so it can be
safely shared between untrusted guests.
Introduce an -object memory-backend-file,readonly=on|off option.
Julio Montes sent an earlier patch here:
https://patchew.org/QEMU/20190708211936.8037-1-julio.montes@intel.com/
Eric Ernst requested this feature again for Kata Containers so I gave it a try.
Stefan Hajnoczi (3):
memory: add readonly support to memory_region_init_ram_from_file()
hostmem-file: add readonly=on|off option
nvdimm: honor -object memory-backend-file,readonly=on option
docs/nvdimm.txt | 8 +++++++-
include/exec/memory.h | 2 ++
include/exec/ram_addr.h | 5 +++--
include/qemu/mmap-alloc.h | 2 ++
backends/hostmem-file.c | 26 +++++++++++++++++++++++++-
exec.c | 18 +++++++++++-------
hw/mem/nvdimm.c | 4 ++++
softmmu/memory.c | 7 +++++--
util/mmap-alloc.c | 10 ++++++----
util/oslib-posix.c | 2 +-
qemu-options.hx | 5 ++++-
11 files changed, 70 insertions(+), 19 deletions(-)
--
2.26.2
^ permalink raw reply [flat|nested] 13+ messages in thread
* [PATCH v2 1/3] memory: add readonly support to memory_region_init_ram_from_file()
2020-09-16 9:51 [PATCH v2 0/3] nvdimm: read-only file support Stefan Hajnoczi
@ 2020-09-16 9:51 ` Stefan Hajnoczi
2020-12-14 11:01 ` Igor Mammedov
2020-09-16 9:51 ` [PATCH v2 2/3] hostmem-file: add readonly=on|off option Stefan Hajnoczi
` (3 subsequent siblings)
4 siblings, 1 reply; 13+ messages in thread
From: Stefan Hajnoczi @ 2020-09-16 9:51 UTC (permalink / raw)
To: qemu-devel
Cc: Eduardo Habkost, Michael S. Tsirkin, Julio Montes,
Xiao Guangrong, eric.g.ernst, Stefan Hajnoczi, Igor Mammedov,
Paolo Bonzini, Philippe Mathieu-Daudé,
Richard Henderson
There is currently no way to open(O_RDONLY) and mmap(PROT_READ) when
creating a memory region from a file. This functionality is needed since
the underlying host file may not allow writing.
Add a bool readonly argument to memory_region_init_ram_from_file() and
the APIs it calls.
Extend memory_region_init_ram_from_file() rather than introducing a
memory_region_init_rom_from_file() API so that callers can easily make a
choice between read/write and read-only at runtime without calling
different APIs.
No new RAMBlock flag is introduced for read-only because it's unclear
whether RAMBlocks need to know that they are read-only. Pass a bool
readonly argument instead.
Both of these design decisions can be changed in the future. It just
seemed like the simplest approach to me.
Acked-by: Michael S. Tsirkin <mst@redhat.com>
Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>
Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
---
include/exec/memory.h | 2 ++
include/exec/ram_addr.h | 5 +++--
include/qemu/mmap-alloc.h | 2 ++
backends/hostmem-file.c | 2 +-
exec.c | 18 +++++++++++-------
softmmu/memory.c | 7 +++++--
util/mmap-alloc.c | 10 ++++++----
util/oslib-posix.c | 2 +-
8 files changed, 31 insertions(+), 17 deletions(-)
diff --git a/include/exec/memory.h b/include/exec/memory.h
index f1bb2a7df5..a81fa26165 100644
--- a/include/exec/memory.h
+++ b/include/exec/memory.h
@@ -879,6 +879,7 @@ void memory_region_init_resizeable_ram(MemoryRegion *mr,
* - RAM_PMEM: the memory is persistent memory
* Other bits are ignored now.
* @path: the path in which to allocate the RAM.
+ * @readonly: true to open @path for reading, false for read/write.
* @errp: pointer to Error*, to store an error if it happens.
*
* Note that this function does not do anything to cause the data in the
@@ -891,6 +892,7 @@ void memory_region_init_ram_from_file(MemoryRegion *mr,
uint64_t align,
uint32_t ram_flags,
const char *path,
+ bool readonly,
Error **errp);
/**
diff --git a/include/exec/ram_addr.h b/include/exec/ram_addr.h
index 3ef729a23c..2a0360a0f2 100644
--- a/include/exec/ram_addr.h
+++ b/include/exec/ram_addr.h
@@ -110,6 +110,7 @@ long qemu_maxrampagesize(void);
* - 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
+ * @readonly: true to open @path for reading, false for read/write.
* @errp: pointer to Error*, to store an error if it happens
*
* Return:
@@ -118,9 +119,9 @@ long qemu_maxrampagesize(void);
*/
RAMBlock *qemu_ram_alloc_from_file(ram_addr_t size, MemoryRegion *mr,
uint32_t ram_flags, const char *mem_path,
- Error **errp);
+ bool readonly, Error **errp);
RAMBlock *qemu_ram_alloc_from_fd(ram_addr_t size, MemoryRegion *mr,
- uint32_t ram_flags, int fd,
+ uint32_t ram_flags, int fd, bool readonly,
Error **errp);
RAMBlock *qemu_ram_alloc_from_ptr(ram_addr_t size, void *host,
diff --git a/include/qemu/mmap-alloc.h b/include/qemu/mmap-alloc.h
index e786266b92..8b7a5c70f3 100644
--- a/include/qemu/mmap-alloc.h
+++ b/include/qemu/mmap-alloc.h
@@ -14,6 +14,7 @@ size_t qemu_mempath_getpagesize(const char *mem_path);
* @size: the number of bytes to be mmaped
* @align: if not zero, specify the alignment of the starting mapping address;
* otherwise, the alignment in use will be determined by QEMU.
+ * @readonly: true for a read-only mapping, false for read/write.
* @shared: map has RAM_SHARED flag.
* @is_pmem: map has RAM_PMEM flag.
*
@@ -24,6 +25,7 @@ size_t qemu_mempath_getpagesize(const char *mem_path);
void *qemu_ram_mmap(int fd,
size_t size,
size_t align,
+ bool readonly,
bool shared,
bool is_pmem);
diff --git a/backends/hostmem-file.c b/backends/hostmem-file.c
index a3b2e8209e..dffdf142e0 100644
--- a/backends/hostmem-file.c
+++ b/backends/hostmem-file.c
@@ -58,7 +58,7 @@ file_backend_memory_alloc(HostMemoryBackend *backend, Error **errp)
backend->size, fb->align,
(backend->share ? RAM_SHARED : 0) |
(fb->is_pmem ? RAM_PMEM : 0),
- fb->mem_path, errp);
+ fb->mem_path, false, errp);
g_free(name);
#endif
}
diff --git a/exec.c b/exec.c
index e34b602bdf..f1e82dad7a 100644
--- a/exec.c
+++ b/exec.c
@@ -1770,6 +1770,7 @@ static int64_t get_file_align(int fd)
static int file_ram_open(const char *path,
const char *region_name,
+ bool readonly,
bool *created,
Error **errp)
{
@@ -1780,7 +1781,7 @@ static int file_ram_open(const char *path,
*created = false;
for (;;) {
- fd = open(path, O_RDWR);
+ fd = open(path, readonly ? O_RDONLY : O_RDWR);
if (fd >= 0) {
/* @path names an existing file, use it */
break;
@@ -1832,6 +1833,7 @@ static int file_ram_open(const char *path,
static void *file_ram_alloc(RAMBlock *block,
ram_addr_t memory,
int fd,
+ bool readonly,
bool truncate,
Error **errp)
{
@@ -1882,7 +1884,7 @@ static void *file_ram_alloc(RAMBlock *block,
perror("ftruncate");
}
- area = qemu_ram_mmap(fd, memory, block->mr->align,
+ area = qemu_ram_mmap(fd, memory, block->mr->align, readonly,
block->flags & RAM_SHARED, block->flags & RAM_PMEM);
if (area == MAP_FAILED) {
error_setg_errno(errp, errno,
@@ -2314,7 +2316,7 @@ static void ram_block_add(RAMBlock *new_block, Error **errp, bool shared)
#ifdef CONFIG_POSIX
RAMBlock *qemu_ram_alloc_from_fd(ram_addr_t size, MemoryRegion *mr,
- uint32_t ram_flags, int fd,
+ uint32_t ram_flags, int fd, bool readonly,
Error **errp)
{
RAMBlock *new_block;
@@ -2368,7 +2370,8 @@ RAMBlock *qemu_ram_alloc_from_fd(ram_addr_t size, MemoryRegion *mr,
new_block->used_length = size;
new_block->max_length = size;
new_block->flags = ram_flags;
- new_block->host = file_ram_alloc(new_block, size, fd, !file_size, errp);
+ new_block->host = file_ram_alloc(new_block, size, fd, readonly,
+ !file_size, errp);
if (!new_block->host) {
g_free(new_block);
return NULL;
@@ -2387,18 +2390,19 @@ RAMBlock *qemu_ram_alloc_from_fd(ram_addr_t size, MemoryRegion *mr,
RAMBlock *qemu_ram_alloc_from_file(ram_addr_t size, MemoryRegion *mr,
uint32_t ram_flags, const char *mem_path,
- Error **errp)
+ bool readonly, Error **errp)
{
int fd;
bool created;
RAMBlock *block;
- fd = file_ram_open(mem_path, memory_region_name(mr), &created, errp);
+ fd = file_ram_open(mem_path, memory_region_name(mr), readonly, &created,
+ errp);
if (fd < 0) {
return NULL;
}
- block = qemu_ram_alloc_from_fd(size, mr, ram_flags, fd, errp);
+ block = qemu_ram_alloc_from_fd(size, mr, ram_flags, fd, readonly, errp);
if (!block) {
if (created) {
unlink(mem_path);
diff --git a/softmmu/memory.c b/softmmu/memory.c
index d030eb6f7c..1b0d1d42c6 100644
--- a/softmmu/memory.c
+++ b/softmmu/memory.c
@@ -1553,15 +1553,18 @@ void memory_region_init_ram_from_file(MemoryRegion *mr,
uint64_t align,
uint32_t ram_flags,
const char *path,
+ bool readonly,
Error **errp)
{
Error *err = NULL;
memory_region_init(mr, owner, name, size);
mr->ram = true;
+ mr->readonly = readonly;
mr->terminates = true;
mr->destructor = memory_region_destructor_ram;
mr->align = align;
- mr->ram_block = qemu_ram_alloc_from_file(size, mr, ram_flags, path, &err);
+ mr->ram_block = qemu_ram_alloc_from_file(size, mr, ram_flags, path,
+ readonly, &err);
mr->dirty_log_mask = tcg_enabled() ? (1 << DIRTY_MEMORY_CODE) : 0;
if (err) {
mr->size = int128_zero();
@@ -1585,7 +1588,7 @@ void memory_region_init_ram_from_fd(MemoryRegion *mr,
mr->destructor = memory_region_destructor_ram;
mr->ram_block = qemu_ram_alloc_from_fd(size, mr,
share ? RAM_SHARED : 0,
- fd, &err);
+ fd, false, &err);
mr->dirty_log_mask = tcg_enabled() ? (1 << DIRTY_MEMORY_CODE) : 0;
if (err) {
mr->size = int128_zero();
diff --git a/util/mmap-alloc.c b/util/mmap-alloc.c
index 27dcccd8ec..890fda6a35 100644
--- a/util/mmap-alloc.c
+++ b/util/mmap-alloc.c
@@ -85,9 +85,11 @@ size_t qemu_mempath_getpagesize(const char *mem_path)
void *qemu_ram_mmap(int fd,
size_t size,
size_t align,
+ bool readonly,
bool shared,
bool is_pmem)
{
+ int prot;
int flags;
int map_sync_flags = 0;
int guardfd;
@@ -146,8 +148,9 @@ void *qemu_ram_mmap(int fd,
offset = QEMU_ALIGN_UP((uintptr_t)guardptr, align) - (uintptr_t)guardptr;
- ptr = mmap(guardptr + offset, size, PROT_READ | PROT_WRITE,
- flags | map_sync_flags, fd, 0);
+ prot = PROT_READ | (readonly ? 0 : PROT_WRITE);
+
+ ptr = mmap(guardptr + offset, size, prot, flags | map_sync_flags, fd, 0);
if (ptr == MAP_FAILED && map_sync_flags) {
if (errno == ENOTSUP) {
@@ -171,8 +174,7 @@ void *qemu_ram_mmap(int fd,
* if map failed with MAP_SHARED_VALIDATE | MAP_SYNC,
* we will remove these flags to handle compatibility.
*/
- ptr = mmap(guardptr + offset, size, PROT_READ | PROT_WRITE,
- flags, fd, 0);
+ ptr = mmap(guardptr + offset, size, prot, flags, fd, 0);
}
if (ptr == MAP_FAILED) {
diff --git a/util/oslib-posix.c b/util/oslib-posix.c
index ad8001a4ad..236b3a88c1 100644
--- a/util/oslib-posix.c
+++ b/util/oslib-posix.c
@@ -227,7 +227,7 @@ void *qemu_memalign(size_t alignment, size_t size)
void *qemu_anon_ram_alloc(size_t size, uint64_t *alignment, bool shared)
{
size_t align = QEMU_VMALLOC_ALIGN;
- void *ptr = qemu_ram_mmap(-1, size, align, shared, false);
+ void *ptr = qemu_ram_mmap(-1, size, align, false, shared, false);
if (ptr == MAP_FAILED) {
return NULL;
--
2.26.2
^ permalink raw reply related [flat|nested] 13+ messages in thread
* [PATCH v2 2/3] hostmem-file: add readonly=on|off option
2020-09-16 9:51 [PATCH v2 0/3] nvdimm: read-only file support Stefan Hajnoczi
2020-09-16 9:51 ` [PATCH v2 1/3] memory: add readonly support to memory_region_init_ram_from_file() Stefan Hajnoczi
@ 2020-09-16 9:51 ` Stefan Hajnoczi
2020-12-14 11:10 ` Igor Mammedov
2020-09-16 9:51 ` [PATCH v2 3/3] nvdimm: honor -object memory-backend-file, readonly=on option Stefan Hajnoczi
` (2 subsequent siblings)
4 siblings, 1 reply; 13+ messages in thread
From: Stefan Hajnoczi @ 2020-09-16 9:51 UTC (permalink / raw)
To: qemu-devel
Cc: Eduardo Habkost, Michael S. Tsirkin, Julio Montes,
Xiao Guangrong, eric.g.ernst, Stefan Hajnoczi, Igor Mammedov,
Paolo Bonzini, Philippe Mathieu-Daudé,
Richard Henderson
Let -object memory-backend-file work on read-only files when the
readonly=on option is given. This can be used to share the contents of a
file between multiple guests while preventing them from consuming
Copy-on-Write memory if guests dirty the pages, for example.
Acked-by: Michael S. Tsirkin <mst@redhat.com>
Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>
Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
---
backends/hostmem-file.c | 26 +++++++++++++++++++++++++-
qemu-options.hx | 5 ++++-
2 files changed, 29 insertions(+), 2 deletions(-)
diff --git a/backends/hostmem-file.c b/backends/hostmem-file.c
index dffdf142e0..da585e4300 100644
--- a/backends/hostmem-file.c
+++ b/backends/hostmem-file.c
@@ -31,6 +31,7 @@ struct HostMemoryBackendFile {
uint64_t align;
bool discard_data;
bool is_pmem;
+ bool readonly;
};
static void
@@ -58,7 +59,7 @@ file_backend_memory_alloc(HostMemoryBackend *backend, Error **errp)
backend->size, fb->align,
(backend->share ? RAM_SHARED : 0) |
(fb->is_pmem ? RAM_PMEM : 0),
- fb->mem_path, false, errp);
+ fb->mem_path, fb->readonly, errp);
g_free(name);
#endif
}
@@ -153,6 +154,26 @@ static void file_memory_backend_set_pmem(Object *o, bool value, Error **errp)
fb->is_pmem = value;
}
+static bool file_memory_backend_get_readonly(Object *o, Error **errp)
+{
+ return MEMORY_BACKEND_FILE(o)->readonly;
+}
+
+static void file_memory_backend_set_readonly(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 'readonly' of %s.",
+ object_get_typename(o));
+ return;
+ }
+
+ fb->readonly = value;
+}
+
static void file_backend_unparent(Object *obj)
{
HostMemoryBackend *backend = MEMORY_BACKEND(obj);
@@ -184,6 +205,9 @@ file_backend_class_init(ObjectClass *oc, void *data)
NULL, NULL);
object_class_property_add_bool(oc, "pmem",
file_memory_backend_get_pmem, file_memory_backend_set_pmem);
+ object_class_property_add_bool(oc, "readonly",
+ file_memory_backend_get_readonly,
+ file_memory_backend_set_readonly);
}
static void file_backend_instance_finalize(Object *o)
diff --git a/qemu-options.hx b/qemu-options.hx
index b0f020594e..3dfaaddd62 100644
--- a/qemu-options.hx
+++ b/qemu-options.hx
@@ -4369,7 +4369,7 @@ SRST
they are specified. Note that the 'id' property must be set. These
objects are placed in the '/objects' path.
- ``-object memory-backend-file,id=id,size=size,mem-path=dir,share=on|off,discard-data=on|off,merge=on|off,dump=on|off,prealloc=on|off,host-nodes=host-nodes,policy=default|preferred|bind|interleave,align=align``
+ ``-object memory-backend-file,id=id,size=size,mem-path=dir,share=on|off,discard-data=on|off,merge=on|off,dump=on|off,prealloc=on|off,host-nodes=host-nodes,policy=default|preferred|bind|interleave,align=align,readonly=on|off``
Creates a memory file backend object, which can be used to back
the guest RAM with huge pages.
@@ -4452,6 +4452,9 @@ SRST
4.15) and the filesystem of ``mem-path`` mounted with DAX
option.
+ The ``readonly`` option specifies whether the backing file is opened
+ read-only or read-write (default).
+
``-object memory-backend-ram,id=id,merge=on|off,dump=on|off,share=on|off,prealloc=on|off,size=size,host-nodes=host-nodes,policy=default|preferred|bind|interleave``
Creates a memory backend object, which can be used to back the
guest RAM. Memory backend objects offer more control than the
--
2.26.2
^ permalink raw reply related [flat|nested] 13+ messages in thread
* [PATCH v2 3/3] nvdimm: honor -object memory-backend-file, readonly=on option
2020-09-16 9:51 [PATCH v2 0/3] nvdimm: read-only file support Stefan Hajnoczi
2020-09-16 9:51 ` [PATCH v2 1/3] memory: add readonly support to memory_region_init_ram_from_file() Stefan Hajnoczi
2020-09-16 9:51 ` [PATCH v2 2/3] hostmem-file: add readonly=on|off option Stefan Hajnoczi
@ 2020-09-16 9:51 ` Stefan Hajnoczi
2020-12-14 11:19 ` Igor Mammedov
2020-09-23 12:41 ` [PATCH v2 0/3] nvdimm: read-only file support Stefan Hajnoczi
2020-12-10 16:48 ` Liam Merwick
4 siblings, 1 reply; 13+ messages in thread
From: Stefan Hajnoczi @ 2020-09-16 9:51 UTC (permalink / raw)
To: qemu-devel
Cc: Eduardo Habkost, Michael S. Tsirkin, Julio Montes,
Xiao Guangrong, eric.g.ernst, Stefan Hajnoczi, Igor Mammedov,
Paolo Bonzini, Philippe Mathieu-Daudé,
Richard Henderson
Make it possible to present read-only files to the guest as "unarmed"
NVDIMMs. The Linux NVDIMM device (/dev/pmemX) is read-only.
Acked-by: Michael S. Tsirkin <mst@redhat.com>
Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>
Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
---
docs/nvdimm.txt | 8 +++++++-
hw/mem/nvdimm.c | 4 ++++
2 files changed, 11 insertions(+), 1 deletion(-)
diff --git a/docs/nvdimm.txt b/docs/nvdimm.txt
index c2c6e441b3..06c2008107 100644
--- a/docs/nvdimm.txt
+++ b/docs/nvdimm.txt
@@ -17,7 +17,7 @@ following command line options:
-machine pc,nvdimm
-m $RAM_SIZE,slots=$N,maxmem=$MAX_SIZE
- -object memory-backend-file,id=mem1,share=on,mem-path=$PATH,size=$NVDIMM_SIZE
+ -object memory-backend-file,id=mem1,share=on,mem-path=$PATH,size=$NVDIMM_SIZE,readonly=off
-device nvdimm,id=nvdimm1,memdev=mem1
Where,
@@ -42,6 +42,12 @@ Where,
"share=off", then guest writes won't be applied to the backend
file and thus will be invisible to other guests.
+ "readonly=on/off" controls whether the file $PATH is opened read-only or
+ read/write (default). "readonly=on" sets the ACPI NFIT NVDIMM Region Mapping
+ Structure "NVDIMM State Flags" Bit 3 indicating that the device is "unarmed"
+ and cannot accept persistent writes. Linux guest drivers set the device to
+ read-only when this bit is present.
+
- "device nvdimm,id=nvdimm1,memdev=mem1" creates a virtual NVDIMM
device whose storage is provided by above memory backend device.
diff --git a/hw/mem/nvdimm.c b/hw/mem/nvdimm.c
index e1574bc07c..848cd65917 100644
--- a/hw/mem/nvdimm.c
+++ b/hw/mem/nvdimm.c
@@ -146,6 +146,10 @@ static void nvdimm_prepare_memory_region(NVDIMMDevice *nvdimm, Error **errp)
return;
}
+ if (memory_region_is_rom(mr)) {
+ nvdimm->unarmed = true; /* this device is read-only */
+ }
+
nvdimm->nvdimm_mr = g_new(MemoryRegion, 1);
memory_region_init_alias(nvdimm->nvdimm_mr, OBJECT(dimm),
"nvdimm-memory", mr, 0, pmem_size);
--
2.26.2
^ permalink raw reply related [flat|nested] 13+ messages in thread
* Re: [PATCH v2 0/3] nvdimm: read-only file support
2020-09-16 9:51 [PATCH v2 0/3] nvdimm: read-only file support Stefan Hajnoczi
` (2 preceding siblings ...)
2020-09-16 9:51 ` [PATCH v2 3/3] nvdimm: honor -object memory-backend-file, readonly=on option Stefan Hajnoczi
@ 2020-09-23 12:41 ` Stefan Hajnoczi
2020-12-10 16:48 ` Liam Merwick
4 siblings, 0 replies; 13+ messages in thread
From: Stefan Hajnoczi @ 2020-09-23 12:41 UTC (permalink / raw)
To: qemu-devel
Cc: Eduardo Habkost, Michael S. Tsirkin, Julio Montes,
Xiao Guangrong, eric.g.ernst, Igor Mammedov, Paolo Bonzini,
Richard Henderson
[-- Attachment #1: Type: text/plain, Size: 1368 bytes --]
On Wed, Sep 16, 2020 at 10:51:47AM +0100, Stefan Hajnoczi wrote:
> v2:
> * s/the the/the/ in documentation [Philippe]
> * Assign nvdimm->unarmed earlier [Philippe]
Ping
> There is currently no way to back an NVDIMM with a read-only file so it can be
> safely shared between untrusted guests.
>
> Introduce an -object memory-backend-file,readonly=on|off option.
>
> Julio Montes sent an earlier patch here:
> https://patchew.org/QEMU/20190708211936.8037-1-julio.montes@intel.com/
>
> Eric Ernst requested this feature again for Kata Containers so I gave it a try.
>
> Stefan Hajnoczi (3):
> memory: add readonly support to memory_region_init_ram_from_file()
> hostmem-file: add readonly=on|off option
> nvdimm: honor -object memory-backend-file,readonly=on option
>
> docs/nvdimm.txt | 8 +++++++-
> include/exec/memory.h | 2 ++
> include/exec/ram_addr.h | 5 +++--
> include/qemu/mmap-alloc.h | 2 ++
> backends/hostmem-file.c | 26 +++++++++++++++++++++++++-
> exec.c | 18 +++++++++++-------
> hw/mem/nvdimm.c | 4 ++++
> softmmu/memory.c | 7 +++++--
> util/mmap-alloc.c | 10 ++++++----
> util/oslib-posix.c | 2 +-
> qemu-options.hx | 5 ++++-
> 11 files changed, 70 insertions(+), 19 deletions(-)
>
> --
> 2.26.2
>
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 484 bytes --]
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH v2 0/3] nvdimm: read-only file support
2020-09-16 9:51 [PATCH v2 0/3] nvdimm: read-only file support Stefan Hajnoczi
` (3 preceding siblings ...)
2020-09-23 12:41 ` [PATCH v2 0/3] nvdimm: read-only file support Stefan Hajnoczi
@ 2020-12-10 16:48 ` Liam Merwick
2021-01-04 15:43 ` Stefan Hajnoczi
4 siblings, 1 reply; 13+ messages in thread
From: Liam Merwick @ 2020-12-10 16:48 UTC (permalink / raw)
To: Stefan Hajnoczi, qemu-devel
Cc: Xiao Guangrong, Michael S. Tsirkin, Julio Montes, eric.g.ernst,
Paolo Bonzini, Igor Mammedov, Richard Henderson, Eduardo Habkost
On 16/09/2020 10:51, Stefan Hajnoczi wrote:
> v2:
> * s/the the/the/ in documentation [Philippe]
> * Assign nvdimm->unarmed earlier [Philippe]
>
> There is currently no way to back an NVDIMM with a read-only file so it can be
> safely shared between untrusted guests.
>
> Introduce an -object memory-backend-file,readonly=on|off option.
>
> Julio Montes sent an earlier patch here:
> https://patchew.org/QEMU/20190708211936.8037-1-julio.montes@intel.com/
>
> Eric Ernst requested this feature again for Kata Containers so I gave it a try.
>
> Stefan Hajnoczi (3):
> memory: add readonly support to memory_region_init_ram_from_file()
> hostmem-file: add readonly=on|off option
> nvdimm: honor -object memory-backend-file,readonly=on option
>
Reviewed-by: Liam Merwick <liam.merwick@oracle.com>
Tested-by: Liam Merwick <liam.merwick@oracle.com>
(I just quickly modified kata-runtime to unconditionally pass
readonly=on and verified that the root filesystem couldn't be remounted
as rw)
Is this a candidate for 6.0?
Regards,
Liam
> docs/nvdimm.txt | 8 +++++++-
> include/exec/memory.h | 2 ++
> include/exec/ram_addr.h | 5 +++--
> include/qemu/mmap-alloc.h | 2 ++
> backends/hostmem-file.c | 26 +++++++++++++++++++++++++-
> exec.c | 18 +++++++++++-------
> hw/mem/nvdimm.c | 4 ++++
> softmmu/memory.c | 7 +++++--
> util/mmap-alloc.c | 10 ++++++----
> util/oslib-posix.c | 2 +-
> qemu-options.hx | 5 ++++-
> 11 files changed, 70 insertions(+), 19 deletions(-)
>
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH v2 1/3] memory: add readonly support to memory_region_init_ram_from_file()
2020-09-16 9:51 ` [PATCH v2 1/3] memory: add readonly support to memory_region_init_ram_from_file() Stefan Hajnoczi
@ 2020-12-14 11:01 ` Igor Mammedov
0 siblings, 0 replies; 13+ messages in thread
From: Igor Mammedov @ 2020-12-14 11:01 UTC (permalink / raw)
To: Stefan Hajnoczi
Cc: Eduardo Habkost, Michael S. Tsirkin, Julio Montes,
Xiao Guangrong, qemu-devel, eric.g.ernst, Paolo Bonzini,
Philippe Mathieu-Daudé,
Richard Henderson
On Wed, 16 Sep 2020 10:51:48 +0100
Stefan Hajnoczi <stefanha@redhat.com> wrote:
> There is currently no way to open(O_RDONLY) and mmap(PROT_READ) when
> creating a memory region from a file. This functionality is needed since
> the underlying host file may not allow writing.
>
> Add a bool readonly argument to memory_region_init_ram_from_file() and
> the APIs it calls.
>
> Extend memory_region_init_ram_from_file() rather than introducing a
> memory_region_init_rom_from_file() API so that callers can easily make a
> choice between read/write and read-only at runtime without calling
> different APIs.
>
> No new RAMBlock flag is introduced for read-only because it's unclear
> whether RAMBlocks need to know that they are read-only. Pass a bool
> readonly argument instead.
>
> Both of these design decisions can be changed in the future. It just
> seemed like the simplest approach to me.
>
> Acked-by: Michael S. Tsirkin <mst@redhat.com>
> Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>
> Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
Reviewed-by: Igor Mammedov <imammedo@redhat.com>
> ---
> include/exec/memory.h | 2 ++
> include/exec/ram_addr.h | 5 +++--
> include/qemu/mmap-alloc.h | 2 ++
> backends/hostmem-file.c | 2 +-
> exec.c | 18 +++++++++++-------
> softmmu/memory.c | 7 +++++--
> util/mmap-alloc.c | 10 ++++++----
> util/oslib-posix.c | 2 +-
> 8 files changed, 31 insertions(+), 17 deletions(-)
>
> diff --git a/include/exec/memory.h b/include/exec/memory.h
> index f1bb2a7df5..a81fa26165 100644
> --- a/include/exec/memory.h
> +++ b/include/exec/memory.h
> @@ -879,6 +879,7 @@ void memory_region_init_resizeable_ram(MemoryRegion *mr,
> * - RAM_PMEM: the memory is persistent memory
> * Other bits are ignored now.
> * @path: the path in which to allocate the RAM.
> + * @readonly: true to open @path for reading, false for read/write.
> * @errp: pointer to Error*, to store an error if it happens.
> *
> * Note that this function does not do anything to cause the data in the
> @@ -891,6 +892,7 @@ void memory_region_init_ram_from_file(MemoryRegion *mr,
> uint64_t align,
> uint32_t ram_flags,
> const char *path,
> + bool readonly,
> Error **errp);
>
> /**
> diff --git a/include/exec/ram_addr.h b/include/exec/ram_addr.h
> index 3ef729a23c..2a0360a0f2 100644
> --- a/include/exec/ram_addr.h
> +++ b/include/exec/ram_addr.h
> @@ -110,6 +110,7 @@ long qemu_maxrampagesize(void);
> * - 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
> + * @readonly: true to open @path for reading, false for read/write.
> * @errp: pointer to Error*, to store an error if it happens
> *
> * Return:
> @@ -118,9 +119,9 @@ long qemu_maxrampagesize(void);
> */
> RAMBlock *qemu_ram_alloc_from_file(ram_addr_t size, MemoryRegion *mr,
> uint32_t ram_flags, const char *mem_path,
> - Error **errp);
> + bool readonly, Error **errp);
> RAMBlock *qemu_ram_alloc_from_fd(ram_addr_t size, MemoryRegion *mr,
> - uint32_t ram_flags, int fd,
> + uint32_t ram_flags, int fd, bool readonly,
> Error **errp);
>
> RAMBlock *qemu_ram_alloc_from_ptr(ram_addr_t size, void *host,
> diff --git a/include/qemu/mmap-alloc.h b/include/qemu/mmap-alloc.h
> index e786266b92..8b7a5c70f3 100644
> --- a/include/qemu/mmap-alloc.h
> +++ b/include/qemu/mmap-alloc.h
> @@ -14,6 +14,7 @@ size_t qemu_mempath_getpagesize(const char *mem_path);
> * @size: the number of bytes to be mmaped
> * @align: if not zero, specify the alignment of the starting mapping address;
> * otherwise, the alignment in use will be determined by QEMU.
> + * @readonly: true for a read-only mapping, false for read/write.
> * @shared: map has RAM_SHARED flag.
> * @is_pmem: map has RAM_PMEM flag.
> *
> @@ -24,6 +25,7 @@ size_t qemu_mempath_getpagesize(const char *mem_path);
> void *qemu_ram_mmap(int fd,
> size_t size,
> size_t align,
> + bool readonly,
> bool shared,
> bool is_pmem);
>
> diff --git a/backends/hostmem-file.c b/backends/hostmem-file.c
> index a3b2e8209e..dffdf142e0 100644
> --- a/backends/hostmem-file.c
> +++ b/backends/hostmem-file.c
> @@ -58,7 +58,7 @@ file_backend_memory_alloc(HostMemoryBackend *backend, Error **errp)
> backend->size, fb->align,
> (backend->share ? RAM_SHARED : 0) |
> (fb->is_pmem ? RAM_PMEM : 0),
> - fb->mem_path, errp);
> + fb->mem_path, false, errp);
> g_free(name);
> #endif
> }
> diff --git a/exec.c b/exec.c
> index e34b602bdf..f1e82dad7a 100644
> --- a/exec.c
> +++ b/exec.c
> @@ -1770,6 +1770,7 @@ static int64_t get_file_align(int fd)
>
> static int file_ram_open(const char *path,
> const char *region_name,
> + bool readonly,
> bool *created,
> Error **errp)
> {
> @@ -1780,7 +1781,7 @@ static int file_ram_open(const char *path,
>
> *created = false;
> for (;;) {
> - fd = open(path, O_RDWR);
> + fd = open(path, readonly ? O_RDONLY : O_RDWR);
> if (fd >= 0) {
> /* @path names an existing file, use it */
> break;
> @@ -1832,6 +1833,7 @@ static int file_ram_open(const char *path,
> static void *file_ram_alloc(RAMBlock *block,
> ram_addr_t memory,
> int fd,
> + bool readonly,
> bool truncate,
> Error **errp)
> {
> @@ -1882,7 +1884,7 @@ static void *file_ram_alloc(RAMBlock *block,
> perror("ftruncate");
> }
>
> - area = qemu_ram_mmap(fd, memory, block->mr->align,
> + area = qemu_ram_mmap(fd, memory, block->mr->align, readonly,
> block->flags & RAM_SHARED, block->flags & RAM_PMEM);
> if (area == MAP_FAILED) {
> error_setg_errno(errp, errno,
> @@ -2314,7 +2316,7 @@ static void ram_block_add(RAMBlock *new_block, Error **errp, bool shared)
>
> #ifdef CONFIG_POSIX
> RAMBlock *qemu_ram_alloc_from_fd(ram_addr_t size, MemoryRegion *mr,
> - uint32_t ram_flags, int fd,
> + uint32_t ram_flags, int fd, bool readonly,
> Error **errp)
> {
> RAMBlock *new_block;
> @@ -2368,7 +2370,8 @@ RAMBlock *qemu_ram_alloc_from_fd(ram_addr_t size, MemoryRegion *mr,
> new_block->used_length = size;
> new_block->max_length = size;
> new_block->flags = ram_flags;
> - new_block->host = file_ram_alloc(new_block, size, fd, !file_size, errp);
> + new_block->host = file_ram_alloc(new_block, size, fd, readonly,
> + !file_size, errp);
> if (!new_block->host) {
> g_free(new_block);
> return NULL;
> @@ -2387,18 +2390,19 @@ RAMBlock *qemu_ram_alloc_from_fd(ram_addr_t size, MemoryRegion *mr,
>
> RAMBlock *qemu_ram_alloc_from_file(ram_addr_t size, MemoryRegion *mr,
> uint32_t ram_flags, const char *mem_path,
> - Error **errp)
> + bool readonly, Error **errp)
> {
> int fd;
> bool created;
> RAMBlock *block;
>
> - fd = file_ram_open(mem_path, memory_region_name(mr), &created, errp);
> + fd = file_ram_open(mem_path, memory_region_name(mr), readonly, &created,
> + errp);
> if (fd < 0) {
> return NULL;
> }
>
> - block = qemu_ram_alloc_from_fd(size, mr, ram_flags, fd, errp);
> + block = qemu_ram_alloc_from_fd(size, mr, ram_flags, fd, readonly, errp);
> if (!block) {
> if (created) {
> unlink(mem_path);
> diff --git a/softmmu/memory.c b/softmmu/memory.c
> index d030eb6f7c..1b0d1d42c6 100644
> --- a/softmmu/memory.c
> +++ b/softmmu/memory.c
> @@ -1553,15 +1553,18 @@ void memory_region_init_ram_from_file(MemoryRegion *mr,
> uint64_t align,
> uint32_t ram_flags,
> const char *path,
> + bool readonly,
> Error **errp)
> {
> Error *err = NULL;
> memory_region_init(mr, owner, name, size);
> mr->ram = true;
> + mr->readonly = readonly;
> mr->terminates = true;
> mr->destructor = memory_region_destructor_ram;
> mr->align = align;
> - mr->ram_block = qemu_ram_alloc_from_file(size, mr, ram_flags, path, &err);
> + mr->ram_block = qemu_ram_alloc_from_file(size, mr, ram_flags, path,
> + readonly, &err);
> mr->dirty_log_mask = tcg_enabled() ? (1 << DIRTY_MEMORY_CODE) : 0;
> if (err) {
> mr->size = int128_zero();
> @@ -1585,7 +1588,7 @@ void memory_region_init_ram_from_fd(MemoryRegion *mr,
> mr->destructor = memory_region_destructor_ram;
> mr->ram_block = qemu_ram_alloc_from_fd(size, mr,
> share ? RAM_SHARED : 0,
> - fd, &err);
> + fd, false, &err);
> mr->dirty_log_mask = tcg_enabled() ? (1 << DIRTY_MEMORY_CODE) : 0;
> if (err) {
> mr->size = int128_zero();
> diff --git a/util/mmap-alloc.c b/util/mmap-alloc.c
> index 27dcccd8ec..890fda6a35 100644
> --- a/util/mmap-alloc.c
> +++ b/util/mmap-alloc.c
> @@ -85,9 +85,11 @@ size_t qemu_mempath_getpagesize(const char *mem_path)
> void *qemu_ram_mmap(int fd,
> size_t size,
> size_t align,
> + bool readonly,
> bool shared,
> bool is_pmem)
> {
> + int prot;
> int flags;
> int map_sync_flags = 0;
> int guardfd;
> @@ -146,8 +148,9 @@ void *qemu_ram_mmap(int fd,
>
> offset = QEMU_ALIGN_UP((uintptr_t)guardptr, align) - (uintptr_t)guardptr;
>
> - ptr = mmap(guardptr + offset, size, PROT_READ | PROT_WRITE,
> - flags | map_sync_flags, fd, 0);
> + prot = PROT_READ | (readonly ? 0 : PROT_WRITE);
> +
> + ptr = mmap(guardptr + offset, size, prot, flags | map_sync_flags, fd, 0);
>
> if (ptr == MAP_FAILED && map_sync_flags) {
> if (errno == ENOTSUP) {
> @@ -171,8 +174,7 @@ void *qemu_ram_mmap(int fd,
> * if map failed with MAP_SHARED_VALIDATE | MAP_SYNC,
> * we will remove these flags to handle compatibility.
> */
> - ptr = mmap(guardptr + offset, size, PROT_READ | PROT_WRITE,
> - flags, fd, 0);
> + ptr = mmap(guardptr + offset, size, prot, flags, fd, 0);
> }
>
> if (ptr == MAP_FAILED) {
> diff --git a/util/oslib-posix.c b/util/oslib-posix.c
> index ad8001a4ad..236b3a88c1 100644
> --- a/util/oslib-posix.c
> +++ b/util/oslib-posix.c
> @@ -227,7 +227,7 @@ void *qemu_memalign(size_t alignment, size_t size)
> void *qemu_anon_ram_alloc(size_t size, uint64_t *alignment, bool shared)
> {
> size_t align = QEMU_VMALLOC_ALIGN;
> - void *ptr = qemu_ram_mmap(-1, size, align, shared, false);
> + void *ptr = qemu_ram_mmap(-1, size, align, false, shared, false);
>
> if (ptr == MAP_FAILED) {
> return NULL;
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH v2 2/3] hostmem-file: add readonly=on|off option
2020-09-16 9:51 ` [PATCH v2 2/3] hostmem-file: add readonly=on|off option Stefan Hajnoczi
@ 2020-12-14 11:10 ` Igor Mammedov
2021-01-04 15:42 ` Stefan Hajnoczi
0 siblings, 1 reply; 13+ messages in thread
From: Igor Mammedov @ 2020-12-14 11:10 UTC (permalink / raw)
To: Stefan Hajnoczi
Cc: Eduardo Habkost, Michael S. Tsirkin, Julio Montes,
Xiao Guangrong, qemu-devel, eric.g.ernst, Paolo Bonzini,
Philippe Mathieu-Daudé,
Richard Henderson
On Wed, 16 Sep 2020 10:51:49 +0100
Stefan Hajnoczi <stefanha@redhat.com> wrote:
> Let -object memory-backend-file work on read-only files when the
> readonly=on option is given. This can be used to share the contents of a
> file between multiple guests while preventing them from consuming
> Copy-on-Write memory if guests dirty the pages, for example.
>
> Acked-by: Michael S. Tsirkin <mst@redhat.com>
> Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>
> Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
cosmetic/style nits only
s/Object *o/Object *obj/
for consistency with the rest of the code in file.
> ---
> backends/hostmem-file.c | 26 +++++++++++++++++++++++++-
> qemu-options.hx | 5 ++++-
> 2 files changed, 29 insertions(+), 2 deletions(-)
>
> diff --git a/backends/hostmem-file.c b/backends/hostmem-file.c
> index dffdf142e0..da585e4300 100644
> --- a/backends/hostmem-file.c
> +++ b/backends/hostmem-file.c
> @@ -31,6 +31,7 @@ struct HostMemoryBackendFile {
> uint64_t align;
> bool discard_data;
> bool is_pmem;
> + bool readonly;
> };
>
> static void
> @@ -58,7 +59,7 @@ file_backend_memory_alloc(HostMemoryBackend *backend, Error **errp)
> backend->size, fb->align,
> (backend->share ? RAM_SHARED : 0) |
> (fb->is_pmem ? RAM_PMEM : 0),
> - fb->mem_path, false, errp);
> + fb->mem_path, fb->readonly, errp);
> g_free(name);
> #endif
> }
> @@ -153,6 +154,26 @@ static void file_memory_backend_set_pmem(Object *o, bool value, Error **errp)
> fb->is_pmem = value;
> }
>
> +static bool file_memory_backend_get_readonly(Object *o, Error **errp)
> +{
> + return MEMORY_BACKEND_FILE(o)->readonly;
I thought using macro this way not acceptable and one should use
HostMemoryBackendFile *fb = MEMORY_BACKEND_FILE(o);
return fb->readonly;
> +}
> +
> +static void file_memory_backend_set_readonly(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 'readonly' of %s.",
> + object_get_typename(o));
> + return;
> + }
> +
> + fb->readonly = value;
> +}
> +
> static void file_backend_unparent(Object *obj)
> {
> HostMemoryBackend *backend = MEMORY_BACKEND(obj);
> @@ -184,6 +205,9 @@ file_backend_class_init(ObjectClass *oc, void *data)
> NULL, NULL);
> object_class_property_add_bool(oc, "pmem",
> file_memory_backend_get_pmem, file_memory_backend_set_pmem);
> + object_class_property_add_bool(oc, "readonly",
> + file_memory_backend_get_readonly,
> + file_memory_backend_set_readonly);
> }
>
> static void file_backend_instance_finalize(Object *o)
> diff --git a/qemu-options.hx b/qemu-options.hx
> index b0f020594e..3dfaaddd62 100644
> --- a/qemu-options.hx
> +++ b/qemu-options.hx
> @@ -4369,7 +4369,7 @@ SRST
> they are specified. Note that the 'id' property must be set. These
> objects are placed in the '/objects' path.
>
> - ``-object memory-backend-file,id=id,size=size,mem-path=dir,share=on|off,discard-data=on|off,merge=on|off,dump=on|off,prealloc=on|off,host-nodes=host-nodes,policy=default|preferred|bind|interleave,align=align``
> + ``-object memory-backend-file,id=id,size=size,mem-path=dir,share=on|off,discard-data=on|off,merge=on|off,dump=on|off,prealloc=on|off,host-nodes=host-nodes,policy=default|preferred|bind|interleave,align=align,readonly=on|off``
> Creates a memory file backend object, which can be used to back
> the guest RAM with huge pages.
>
> @@ -4452,6 +4452,9 @@ SRST
> 4.15) and the filesystem of ``mem-path`` mounted with DAX
> option.
>
> + The ``readonly`` option specifies whether the backing file is opened
> + read-only or read-write (default).
> +
> ``-object memory-backend-ram,id=id,merge=on|off,dump=on|off,share=on|off,prealloc=on|off,size=size,host-nodes=host-nodes,policy=default|preferred|bind|interleave``
> Creates a memory backend object, which can be used to back the
> guest RAM. Memory backend objects offer more control than the
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH v2 3/3] nvdimm: honor -object memory-backend-file, readonly=on option
2020-09-16 9:51 ` [PATCH v2 3/3] nvdimm: honor -object memory-backend-file, readonly=on option Stefan Hajnoczi
@ 2020-12-14 11:19 ` Igor Mammedov
2021-01-04 16:05 ` Stefan Hajnoczi
0 siblings, 1 reply; 13+ messages in thread
From: Igor Mammedov @ 2020-12-14 11:19 UTC (permalink / raw)
To: Stefan Hajnoczi
Cc: Xiao Guangrong, Michael S. Tsirkin, Julio Montes, qemu-devel,
eric.g.ernst, Paolo Bonzini, Richard Henderson,
Philippe Mathieu-Daudé,
Eduardo Habkost
On Wed, 16 Sep 2020 10:51:50 +0100
Stefan Hajnoczi <stefanha@redhat.com> wrote:
> Make it possible to present read-only files to the guest as "unarmed"
> NVDIMMs. The Linux NVDIMM device (/dev/pmemX) is read-only.
>
> Acked-by: Michael S. Tsirkin <mst@redhat.com>
> Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>
> Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
> ---
> docs/nvdimm.txt | 8 +++++++-
> hw/mem/nvdimm.c | 4 ++++
> 2 files changed, 11 insertions(+), 1 deletion(-)
>
> diff --git a/docs/nvdimm.txt b/docs/nvdimm.txt
> index c2c6e441b3..06c2008107 100644
> --- a/docs/nvdimm.txt
> +++ b/docs/nvdimm.txt
> @@ -17,7 +17,7 @@ following command line options:
>
> -machine pc,nvdimm
> -m $RAM_SIZE,slots=$N,maxmem=$MAX_SIZE
> - -object memory-backend-file,id=mem1,share=on,mem-path=$PATH,size=$NVDIMM_SIZE
> + -object memory-backend-file,id=mem1,share=on,mem-path=$PATH,size=$NVDIMM_SIZE,readonly=off
> -device nvdimm,id=nvdimm1,memdev=mem1
>
> Where,
> @@ -42,6 +42,12 @@ Where,
> "share=off", then guest writes won't be applied to the backend
> file and thus will be invisible to other guests.
>
> + "readonly=on/off" controls whether the file $PATH is opened read-only or
> + read/write (default). "readonly=on" sets the ACPI NFIT NVDIMM Region Mapping
> + Structure "NVDIMM State Flags" Bit 3 indicating that the device is "unarmed"
> + and cannot accept persistent writes. Linux guest drivers set the device to
> + read-only when this bit is present.
> +
> - "device nvdimm,id=nvdimm1,memdev=mem1" creates a virtual NVDIMM
> device whose storage is provided by above memory backend device.
>
> diff --git a/hw/mem/nvdimm.c b/hw/mem/nvdimm.c
> index e1574bc07c..848cd65917 100644
> --- a/hw/mem/nvdimm.c
> +++ b/hw/mem/nvdimm.c
> @@ -146,6 +146,10 @@ static void nvdimm_prepare_memory_region(NVDIMMDevice *nvdimm, Error **errp)
> return;
> }
>
> + if (memory_region_is_rom(mr)) {
> + nvdimm->unarmed = true; /* this device is read-only */
> + }
we probably should error out in case 'backend,readonly=true + nvdimm,unarmed=false'
instead of silently ignoring user's input
> nvdimm->nvdimm_mr = g_new(MemoryRegion, 1);
> memory_region_init_alias(nvdimm->nvdimm_mr, OBJECT(dimm),
> "nvdimm-memory", mr, 0, pmem_size);
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH v2 2/3] hostmem-file: add readonly=on|off option
2020-12-14 11:10 ` Igor Mammedov
@ 2021-01-04 15:42 ` Stefan Hajnoczi
2021-01-04 21:20 ` Eduardo Habkost
0 siblings, 1 reply; 13+ messages in thread
From: Stefan Hajnoczi @ 2021-01-04 15:42 UTC (permalink / raw)
To: Igor Mammedov
Cc: Eduardo Habkost, Michael S. Tsirkin, Julio Montes,
Xiao Guangrong, qemu-devel, eric.g.ernst, Paolo Bonzini,
Philippe Mathieu-Daudé,
Richard Henderson
[-- Attachment #1: Type: text/plain, Size: 1299 bytes --]
On Mon, Dec 14, 2020 at 12:10:15PM +0100, Igor Mammedov wrote:
> On Wed, 16 Sep 2020 10:51:49 +0100
> Stefan Hajnoczi <stefanha@redhat.com> wrote:
>
> > Let -object memory-backend-file work on read-only files when the
> > readonly=on option is given. This can be used to share the contents of a
> > file between multiple guests while preventing them from consuming
> > Copy-on-Write memory if guests dirty the pages, for example.
> >
> > Acked-by: Michael S. Tsirkin <mst@redhat.com>
> > Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>
> > Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
>
> cosmetic/style nits only
>
> s/Object *o/Object *obj/
>
> for consistency with the rest of the code in file.
Will fix.
> > @@ -153,6 +154,26 @@ static void file_memory_backend_set_pmem(Object *o, bool value, Error **errp)
> > fb->is_pmem = value;
> > }
> >
> > +static bool file_memory_backend_get_readonly(Object *o, Error **errp)
> > +{
> > + return MEMORY_BACKEND_FILE(o)->readonly;
>
> I thought using macro this way not acceptable and one should use
>
> HostMemoryBackendFile *fb = MEMORY_BACKEND_FILE(o);
>
> return fb->readonly;
I'm not sure where this is forbidden or why? I've updated the patch as
suggested anyway.
Stefan
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH v2 0/3] nvdimm: read-only file support
2020-12-10 16:48 ` Liam Merwick
@ 2021-01-04 15:43 ` Stefan Hajnoczi
0 siblings, 0 replies; 13+ messages in thread
From: Stefan Hajnoczi @ 2021-01-04 15:43 UTC (permalink / raw)
To: Liam Merwick
Cc: Xiao Guangrong, Michael S. Tsirkin, Julio Montes, qemu-devel,
eric.g.ernst, Paolo Bonzini, Igor Mammedov, Richard Henderson,
Eduardo Habkost
[-- Attachment #1: Type: text/plain, Size: 1229 bytes --]
On Thu, Dec 10, 2020 at 04:48:57PM +0000, Liam Merwick wrote:
> On 16/09/2020 10:51, Stefan Hajnoczi wrote:
> > v2:
> > * s/the the/the/ in documentation [Philippe]
> > * Assign nvdimm->unarmed earlier [Philippe]
> >
> > There is currently no way to back an NVDIMM with a read-only file so it can be
> > safely shared between untrusted guests.
> >
> > Introduce an -object memory-backend-file,readonly=on|off option.
> >
> > Julio Montes sent an earlier patch here:
> > https://patchew.org/QEMU/20190708211936.8037-1-julio.montes@intel.com/
> >
> > Eric Ernst requested this feature again for Kata Containers so I gave it a try.
> >
> > Stefan Hajnoczi (3):
> > memory: add readonly support to memory_region_init_ram_from_file()
> > hostmem-file: add readonly=on|off option
> > nvdimm: honor -object memory-backend-file,readonly=on option
> >
>
>
> Reviewed-by: Liam Merwick <liam.merwick@oracle.com>
> Tested-by: Liam Merwick <liam.merwick@oracle.com>
>
> (I just quickly modified kata-runtime to unconditionally pass readonly=on
> and verified that the root filesystem couldn't be remounted as rw)
>
> Is this a candidate for 6.0?
Thanks! Yes, this is for 6.0.
Stefan
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH v2 3/3] nvdimm: honor -object memory-backend-file, readonly=on option
2020-12-14 11:19 ` Igor Mammedov
@ 2021-01-04 16:05 ` Stefan Hajnoczi
0 siblings, 0 replies; 13+ messages in thread
From: Stefan Hajnoczi @ 2021-01-04 16:05 UTC (permalink / raw)
To: Igor Mammedov
Cc: Xiao Guangrong, Michael S. Tsirkin, Julio Montes, qemu-devel,
eric.g.ernst, Paolo Bonzini, Richard Henderson,
Philippe Mathieu-Daudé,
Eduardo Habkost
[-- Attachment #1: Type: text/plain, Size: 709 bytes --]
On Mon, Dec 14, 2020 at 12:19:05PM +0100, Igor Mammedov wrote:
> On Wed, 16 Sep 2020 10:51:50 +0100
> Stefan Hajnoczi <stefanha@redhat.com> wrote:
> > diff --git a/hw/mem/nvdimm.c b/hw/mem/nvdimm.c
> > index e1574bc07c..848cd65917 100644
> > --- a/hw/mem/nvdimm.c
> > +++ b/hw/mem/nvdimm.c
> > @@ -146,6 +146,10 @@ static void nvdimm_prepare_memory_region(NVDIMMDevice *nvdimm, Error **errp)
> > return;
> > }
> >
> > + if (memory_region_is_rom(mr)) {
> > + nvdimm->unarmed = true; /* this device is read-only */
> > + }
> we probably should error out in case 'backend,readonly=true + nvdimm,unarmed=false'
> instead of silently ignoring user's input
Will fix.
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH v2 2/3] hostmem-file: add readonly=on|off option
2021-01-04 15:42 ` Stefan Hajnoczi
@ 2021-01-04 21:20 ` Eduardo Habkost
0 siblings, 0 replies; 13+ messages in thread
From: Eduardo Habkost @ 2021-01-04 21:20 UTC (permalink / raw)
To: Stefan Hajnoczi
Cc: Xiao Guangrong, Michael S. Tsirkin, Julio Montes, qemu-devel,
eric.g.ernst, Paolo Bonzini, Igor Mammedov,
Philippe Mathieu-Daudé,
Richard Henderson
On Mon, Jan 04, 2021 at 03:42:23PM +0000, Stefan Hajnoczi wrote:
> On Mon, Dec 14, 2020 at 12:10:15PM +0100, Igor Mammedov wrote:
> > On Wed, 16 Sep 2020 10:51:49 +0100
> > Stefan Hajnoczi <stefanha@redhat.com> wrote:
> >
> > > Let -object memory-backend-file work on read-only files when the
> > > readonly=on option is given. This can be used to share the contents of a
> > > file between multiple guests while preventing them from consuming
> > > Copy-on-Write memory if guests dirty the pages, for example.
> > >
> > > Acked-by: Michael S. Tsirkin <mst@redhat.com>
> > > Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>
> > > Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
> >
> > cosmetic/style nits only
> >
> > s/Object *o/Object *obj/
> >
> > for consistency with the rest of the code in file.
>
> Will fix.
>
> > > @@ -153,6 +154,26 @@ static void file_memory_backend_set_pmem(Object *o, bool value, Error **errp)
> > > fb->is_pmem = value;
> > > }
> > >
> > > +static bool file_memory_backend_get_readonly(Object *o, Error **errp)
> > > +{
> > > + return MEMORY_BACKEND_FILE(o)->readonly;
> >
> > I thought using macro this way not acceptable and one should use
> >
> > HostMemoryBackendFile *fb = MEMORY_BACKEND_FILE(o);
> >
> > return fb->readonly;
>
> I'm not sure where this is forbidden or why? I've updated the patch as
> suggested anyway.
I have a vague memory of seeing this documented somewhere, but I
can't find it anywhere in the QOM documentation or git log.
I don't think we need to make this a rule, though.
--
Eduardo
^ permalink raw reply [flat|nested] 13+ messages in thread
end of thread, other threads:[~2021-01-04 21:22 UTC | newest]
Thread overview: 13+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-09-16 9:51 [PATCH v2 0/3] nvdimm: read-only file support Stefan Hajnoczi
2020-09-16 9:51 ` [PATCH v2 1/3] memory: add readonly support to memory_region_init_ram_from_file() Stefan Hajnoczi
2020-12-14 11:01 ` Igor Mammedov
2020-09-16 9:51 ` [PATCH v2 2/3] hostmem-file: add readonly=on|off option Stefan Hajnoczi
2020-12-14 11:10 ` Igor Mammedov
2021-01-04 15:42 ` Stefan Hajnoczi
2021-01-04 21:20 ` Eduardo Habkost
2020-09-16 9:51 ` [PATCH v2 3/3] nvdimm: honor -object memory-backend-file, readonly=on option Stefan Hajnoczi
2020-12-14 11:19 ` Igor Mammedov
2021-01-04 16:05 ` Stefan Hajnoczi
2020-09-23 12:41 ` [PATCH v2 0/3] nvdimm: read-only file support Stefan Hajnoczi
2020-12-10 16:48 ` Liam Merwick
2021-01-04 15:43 ` Stefan Hajnoczi
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.