* [PATCH 0/3] nvdimm: read-only file support
@ 2020-08-04 10:12 Stefan Hajnoczi
2020-08-04 10:12 ` [PATCH 1/3] memory: add readonly support to memory_region_init_ram_from_file() Stefan Hajnoczi
` (4 more replies)
0 siblings, 5 replies; 15+ messages in thread
From: Stefan Hajnoczi @ 2020-08-04 10:12 UTC (permalink / raw)
To: qemu-devel
Cc: Eduardo Habkost, Michael S. Tsirkin, Julio Montes,
Xiao Guangrong, Stefan Hajnoczi, Paolo Bonzini, Igor Mammedov,
Richard Henderson
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] 15+ messages in thread
* [PATCH 1/3] memory: add readonly support to memory_region_init_ram_from_file()
2020-08-04 10:12 [PATCH 0/3] nvdimm: read-only file support Stefan Hajnoczi
@ 2020-08-04 10:12 ` Stefan Hajnoczi
2020-08-04 12:25 ` Philippe Mathieu-Daudé
2020-08-04 10:12 ` [PATCH 2/3] hostmem-file: add readonly=on|off option Stefan Hajnoczi
` (3 subsequent siblings)
4 siblings, 1 reply; 15+ messages in thread
From: Stefan Hajnoczi @ 2020-08-04 10:12 UTC (permalink / raw)
To: qemu-devel
Cc: Eduardo Habkost, Michael S. Tsirkin, Julio Montes,
Xiao Guangrong, Stefan Hajnoczi, Paolo Bonzini, Igor Mammedov,
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.
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 307e527835..1ae7b31e3a 100644
--- a/include/exec/memory.h
+++ b/include/exec/memory.h
@@ -884,6 +884,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
@@ -896,6 +897,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 5b819020b4..37c70acfe2 100644
--- a/backends/hostmem-file.c
+++ b/backends/hostmem-file.c
@@ -57,7 +57,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 6f381f98e2..5874e999ab 100644
--- a/exec.c
+++ b/exec.c
@@ -1769,6 +1769,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)
{
@@ -1779,7 +1780,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;
@@ -1831,6 +1832,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)
{
@@ -1881,7 +1883,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,
@@ -2313,7 +2315,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;
@@ -2367,7 +2369,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;
@@ -2386,18 +2389,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 af25987518..d228635bb3 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] 15+ messages in thread
* [PATCH 2/3] hostmem-file: add readonly=on|off option
2020-08-04 10:12 [PATCH 0/3] nvdimm: read-only file support Stefan Hajnoczi
2020-08-04 10:12 ` [PATCH 1/3] memory: add readonly support to memory_region_init_ram_from_file() Stefan Hajnoczi
@ 2020-08-04 10:12 ` Stefan Hajnoczi
2020-08-21 12:50 ` Philippe Mathieu-Daudé
2020-08-04 10:12 ` [PATCH 3/3] nvdimm: honor -object memory-backend-file, readonly=on option Stefan Hajnoczi
` (2 subsequent siblings)
4 siblings, 1 reply; 15+ messages in thread
From: Stefan Hajnoczi @ 2020-08-04 10:12 UTC (permalink / raw)
To: qemu-devel
Cc: Eduardo Habkost, Michael S. Tsirkin, Julio Montes,
Xiao Guangrong, Stefan Hajnoczi, Paolo Bonzini, Igor Mammedov,
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.
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 37c70acfe2..6bd5bf9b91 100644
--- a/backends/hostmem-file.c
+++ b/backends/hostmem-file.c
@@ -30,6 +30,7 @@ struct HostMemoryBackendFile {
uint64_t align;
bool discard_data;
bool is_pmem;
+ bool readonly;
};
static void
@@ -57,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, false, errp);
+ fb->mem_path, fb->readonly, errp);
g_free(name);
#endif
}
@@ -152,6 +153,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);
@@ -183,6 +204,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 708583b4ce..d834e00b0d 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] 15+ messages in thread
* [PATCH 3/3] nvdimm: honor -object memory-backend-file, readonly=on option
2020-08-04 10:12 [PATCH 0/3] nvdimm: read-only file support Stefan Hajnoczi
2020-08-04 10:12 ` [PATCH 1/3] memory: add readonly support to memory_region_init_ram_from_file() Stefan Hajnoczi
2020-08-04 10:12 ` [PATCH 2/3] hostmem-file: add readonly=on|off option Stefan Hajnoczi
@ 2020-08-04 10:12 ` Stefan Hajnoczi
2020-08-21 13:03 ` Philippe Mathieu-Daudé
2020-08-04 12:28 ` [PATCH 0/3] nvdimm: read-only file support Michael S. Tsirkin
2020-08-21 12:18 ` Stefan Hajnoczi
4 siblings, 1 reply; 15+ messages in thread
From: Stefan Hajnoczi @ 2020-08-04 10:12 UTC (permalink / raw)
To: qemu-devel
Cc: Eduardo Habkost, Michael S. Tsirkin, Julio Montes,
Xiao Guangrong, Stefan Hajnoczi, Paolo Bonzini, Igor Mammedov,
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.
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..c0b52de111 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 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..694223450e 100644
--- a/hw/mem/nvdimm.c
+++ b/hw/mem/nvdimm.c
@@ -151,6 +151,10 @@ static void nvdimm_prepare_memory_region(NVDIMMDevice *nvdimm, Error **errp)
"nvdimm-memory", mr, 0, pmem_size);
memory_region_set_nonvolatile(nvdimm->nvdimm_mr, true);
nvdimm->nvdimm_mr->align = align;
+
+ if (memory_region_is_rom(mr)) {
+ nvdimm->unarmed = true; /* this device is read-only */
+ }
}
static MemoryRegion *nvdimm_md_get_memory_region(MemoryDeviceState *md,
--
2.26.2
^ permalink raw reply related [flat|nested] 15+ messages in thread
* Re: [PATCH 1/3] memory: add readonly support to memory_region_init_ram_from_file()
2020-08-04 10:12 ` [PATCH 1/3] memory: add readonly support to memory_region_init_ram_from_file() Stefan Hajnoczi
@ 2020-08-04 12:25 ` Philippe Mathieu-Daudé
2020-08-04 12:26 ` Philippe Mathieu-Daudé
0 siblings, 1 reply; 15+ messages in thread
From: Philippe Mathieu-Daudé @ 2020-08-04 12:25 UTC (permalink / raw)
To: Stefan Hajnoczi, qemu-devel
Cc: Xiao Guangrong, Michael S. Tsirkin, Julio Montes, Igor Mammedov,
Paolo Bonzini, Richard Henderson, Eduardo Habkost
Hi Stefan,
On 8/4/20 12:12 PM, Stefan Hajnoczi 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.
What happens if we call:
memory_region_init_ram_from_file(mr, ..., readonly=false, ...);
memory_region_set_readonly(mr, false);
?
>
> 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.
>
> 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 307e527835..1ae7b31e3a 100644
> --- a/include/exec/memory.h
> +++ b/include/exec/memory.h
> @@ -884,6 +884,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
> @@ -896,6 +897,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/softmmu/memory.c b/softmmu/memory.c
> index af25987518..d228635bb3 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) {
[...]
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH 1/3] memory: add readonly support to memory_region_init_ram_from_file()
2020-08-04 12:25 ` Philippe Mathieu-Daudé
@ 2020-08-04 12:26 ` Philippe Mathieu-Daudé
2020-08-04 13:47 ` Stefan Hajnoczi
0 siblings, 1 reply; 15+ messages in thread
From: Philippe Mathieu-Daudé @ 2020-08-04 12:26 UTC (permalink / raw)
To: Stefan Hajnoczi, qemu-devel
Cc: Xiao Guangrong, Michael S. Tsirkin, Julio Montes, Igor Mammedov,
Paolo Bonzini, Richard Henderson, Eduardo Habkost
On 8/4/20 2:25 PM, Philippe Mathieu-Daudé wrote:
> Hi Stefan,
>
> On 8/4/20 12:12 PM, Stefan Hajnoczi 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.
>
> What happens if we call:
>
> memory_region_init_ram_from_file(mr, ..., readonly=false, ...);
> memory_region_set_readonly(mr, false);
In case my error is not obvious, I meant:
memory_region_init_ram_from_file(mr, ..., readonly=true, ...);
memory_region_set_readonly(mr, false);
>
> ?
>
>>
>> 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.
>>
>> 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 307e527835..1ae7b31e3a 100644
>> --- a/include/exec/memory.h
>> +++ b/include/exec/memory.h
>> @@ -884,6 +884,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
>> @@ -896,6 +897,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/softmmu/memory.c b/softmmu/memory.c
>> index af25987518..d228635bb3 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) {
> [...]
>
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH 0/3] nvdimm: read-only file support
2020-08-04 10:12 [PATCH 0/3] nvdimm: read-only file support Stefan Hajnoczi
` (2 preceding siblings ...)
2020-08-04 10:12 ` [PATCH 3/3] nvdimm: honor -object memory-backend-file, readonly=on option Stefan Hajnoczi
@ 2020-08-04 12:28 ` Michael S. Tsirkin
2020-08-21 12:18 ` Stefan Hajnoczi
4 siblings, 0 replies; 15+ messages in thread
From: Michael S. Tsirkin @ 2020-08-04 12:28 UTC (permalink / raw)
To: Stefan Hajnoczi
Cc: Eduardo Habkost, Julio Montes, Xiao Guangrong, qemu-devel,
Paolo Bonzini, Igor Mammedov, Richard Henderson
On Tue, Aug 04, 2020 at 11:12:41AM +0100, Stefan Hajnoczi wrote:
> 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/
makes sense:
Acked-by: Michael S. Tsirkin <mst@redhat.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] 15+ messages in thread
* Re: [PATCH 1/3] memory: add readonly support to memory_region_init_ram_from_file()
2020-08-04 12:26 ` Philippe Mathieu-Daudé
@ 2020-08-04 13:47 ` Stefan Hajnoczi
2020-08-04 13:57 ` Philippe Mathieu-Daudé
0 siblings, 1 reply; 15+ messages in thread
From: Stefan Hajnoczi @ 2020-08-04 13:47 UTC (permalink / raw)
To: Philippe Mathieu-Daudé
Cc: Xiao Guangrong, Michael S. Tsirkin, Julio Montes, qemu-devel,
Igor Mammedov, Paolo Bonzini, Richard Henderson, Eduardo Habkost
[-- Attachment #1: Type: text/plain, Size: 1283 bytes --]
On Tue, Aug 04, 2020 at 02:26:22PM +0200, Philippe Mathieu-Daudé wrote:
> On 8/4/20 2:25 PM, Philippe Mathieu-Daudé wrote:
> > Hi Stefan,
> >
> > On 8/4/20 12:12 PM, Stefan Hajnoczi 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.
> >
> > What happens if we call:
> >
> > memory_region_init_ram_from_file(mr, ..., readonly=false, ...);
> > memory_region_set_readonly(mr, false);
>
> In case my error is not obvious, I meant:
>
> memory_region_init_ram_from_file(mr, ..., readonly=true, ...);
> memory_region_set_readonly(mr, false);
Since the mmap was made using PROT_READ any store instructions to the
memory will fault.
Is there some scenario where memory_region_set_readonly() is called? I
can't find one.
Stefan
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH 1/3] memory: add readonly support to memory_region_init_ram_from_file()
2020-08-04 13:47 ` Stefan Hajnoczi
@ 2020-08-04 13:57 ` Philippe Mathieu-Daudé
0 siblings, 0 replies; 15+ messages in thread
From: Philippe Mathieu-Daudé @ 2020-08-04 13:57 UTC (permalink / raw)
To: Stefan Hajnoczi
Cc: Xiao Guangrong, Michael S. Tsirkin, Julio Montes, qemu-devel,
Igor Mammedov, Paolo Bonzini, Richard Henderson, Eduardo Habkost
On 8/4/20 3:47 PM, Stefan Hajnoczi wrote:
> On Tue, Aug 04, 2020 at 02:26:22PM +0200, Philippe Mathieu-Daudé wrote:
>> On 8/4/20 2:25 PM, Philippe Mathieu-Daudé wrote:
>>> Hi Stefan,
>>>
>>> On 8/4/20 12:12 PM, Stefan Hajnoczi 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.
>>>
>>> What happens if we call:
>>>
>>> memory_region_init_ram_from_file(mr, ..., readonly=false, ...);
>>> memory_region_set_readonly(mr, false);
>>
>> In case my error is not obvious, I meant:
>>
>> memory_region_init_ram_from_file(mr, ..., readonly=true, ...);
>> memory_region_set_readonly(mr, false);
>
> Since the mmap was made using PROT_READ any store instructions to the
> memory will fault.
>
> Is there some scenario where memory_region_set_readonly() is called? I
> can't find one.
Not in the current code base, but I was wondering about the API abuses.
I see in the next patch the property is protected:
if (host_memory_backend_mr_inited(backend)) {
error_setg(errp, "cannot change property 'readonly' of %s.",
object_get_typename(o));
return;
}
By using memory_region_set_readonly() you bypass this protection.
Maybe not something to worry.
Anyway for the patch:
Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>
>
> Stefan
>
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH 0/3] nvdimm: read-only file support
2020-08-04 10:12 [PATCH 0/3] nvdimm: read-only file support Stefan Hajnoczi
` (3 preceding siblings ...)
2020-08-04 12:28 ` [PATCH 0/3] nvdimm: read-only file support Michael S. Tsirkin
@ 2020-08-21 12:18 ` Stefan Hajnoczi
4 siblings, 0 replies; 15+ messages in thread
From: Stefan Hajnoczi @ 2020-08-21 12:18 UTC (permalink / raw)
To: Eduardo Habkost, Igor Mammedov
Cc: Xiao Guangrong, Michael S. Tsirkin, Julio Montes, qemu-devel,
Paolo Bonzini, Richard Henderson
[-- Attachment #1: Type: text/plain, Size: 1237 bytes --]
On Tue, Aug 04, 2020 at 11:12:41AM +0100, Stefan Hajnoczi wrote:
> 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(-)
Ping
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH 2/3] hostmem-file: add readonly=on|off option
2020-08-04 10:12 ` [PATCH 2/3] hostmem-file: add readonly=on|off option Stefan Hajnoczi
@ 2020-08-21 12:50 ` Philippe Mathieu-Daudé
2020-09-16 9:31 ` Stefan Hajnoczi
0 siblings, 1 reply; 15+ messages in thread
From: Philippe Mathieu-Daudé @ 2020-08-21 12:50 UTC (permalink / raw)
To: Stefan Hajnoczi, qemu-devel
Cc: Xiao Guangrong, Michael S. Tsirkin, Julio Montes, Igor Mammedov,
Paolo Bonzini, Richard Henderson, Eduardo Habkost
On 8/4/20 12:12 PM, Stefan Hajnoczi 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.
>
> 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 37c70acfe2..6bd5bf9b91 100644
> --- a/backends/hostmem-file.c
> +++ b/backends/hostmem-file.c
> @@ -30,6 +30,7 @@ struct HostMemoryBackendFile {
> uint64_t align;
> bool discard_data;
> bool is_pmem;
> + bool readonly;
> };
>
> static void
> @@ -57,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, false, errp);
> + fb->mem_path, fb->readonly, errp);
> g_free(name);
> #endif
> }
> @@ -152,6 +153,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));
The 'host_memory_backend_mr_inited()' function is not documented;
my understanding is a backend is considered initialized once it has
a MemoryRegion assigned to it.
So this error message is not very helpful, it doesn't explain the
reason. I see all other setters in this file use the same error,
so it is almost a predating issue.
Still I'd rather use a different message, something like:
"'%s' already initialized, can not set it 'readonly'".
Preferably with the error message reworded:
Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>
> + return;
> + }
> +
> + fb->readonly = value;
> +}
> +
> static void file_backend_unparent(Object *obj)
> {
> HostMemoryBackend *backend = MEMORY_BACKEND(obj);
> @@ -183,6 +204,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 708583b4ce..d834e00b0d 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] 15+ messages in thread
* Re: [PATCH 3/3] nvdimm: honor -object memory-backend-file, readonly=on option
2020-08-04 10:12 ` [PATCH 3/3] nvdimm: honor -object memory-backend-file, readonly=on option Stefan Hajnoczi
@ 2020-08-21 13:03 ` Philippe Mathieu-Daudé
2020-09-16 9:39 ` Stefan Hajnoczi
0 siblings, 1 reply; 15+ messages in thread
From: Philippe Mathieu-Daudé @ 2020-08-21 13:03 UTC (permalink / raw)
To: Stefan Hajnoczi, qemu-devel, Igor Mammedov
Cc: Xiao Guangrong, Michael S. Tsirkin, Julio Montes,
Eduardo Habkost, Paolo Bonzini, Richard Henderson
On 8/4/20 12:12 PM, Stefan Hajnoczi wrote:
> Make it possible to present read-only files to the guest as "unarmed"
> NVDIMMs. The Linux NVDIMM device (/dev/pmemX) is read-only.
>
> 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..c0b52de111 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 the file $PATH is opened read-only or
Double "the the".
> + read/write (default). "readonly=on" sets the ACPI NFIT NVDIMM Region Mapping
NFIT acronym for 'NVDIMM Firmware Interface Table', "NVDIMM" sounds
redundant but makes it easier to understand, so OK.
> + 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..694223450e 100644
> --- a/hw/mem/nvdimm.c
> +++ b/hw/mem/nvdimm.c
> @@ -151,6 +151,10 @@ static void nvdimm_prepare_memory_region(NVDIMMDevice *nvdimm, Error **errp)
> "nvdimm-memory", mr, 0, pmem_size);
> memory_region_set_nonvolatile(nvdimm->nvdimm_mr, true);
> nvdimm->nvdimm_mr->align = align;
> +
> + if (memory_region_is_rom(mr)) {
> + nvdimm->unarmed = true; /* this device is read-only */
> + }
Can you move this hunk before the alias creation?
(Just before nvdimm->nvdimm_mr = ...).
> }
>
> static MemoryRegion *nvdimm_md_get_memory_region(MemoryDeviceState *md,
I don't understand why MemoryDeviceClass::get_memory_region() implicitly
calls DeviceClass::realize()...
Anyway unrelated to this patch.
With the comments addressed:
Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH 2/3] hostmem-file: add readonly=on|off option
2020-08-21 12:50 ` Philippe Mathieu-Daudé
@ 2020-09-16 9:31 ` Stefan Hajnoczi
2020-09-16 10:17 ` Philippe Mathieu-Daudé
0 siblings, 1 reply; 15+ messages in thread
From: Stefan Hajnoczi @ 2020-09-16 9:31 UTC (permalink / raw)
To: Philippe Mathieu-Daudé
Cc: Xiao Guangrong, Michael S. Tsirkin, Julio Montes, qemu-devel,
Igor Mammedov, Paolo Bonzini, Richard Henderson, Eduardo Habkost
[-- Attachment #1: Type: text/plain, Size: 3416 bytes --]
On Fri, Aug 21, 2020 at 02:50:42PM +0200, Philippe Mathieu-Daudé wrote:
> On 8/4/20 12:12 PM, Stefan Hajnoczi 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.
> >
> > 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 37c70acfe2..6bd5bf9b91 100644
> > --- a/backends/hostmem-file.c
> > +++ b/backends/hostmem-file.c
> > @@ -30,6 +30,7 @@ struct HostMemoryBackendFile {
> > uint64_t align;
> > bool discard_data;
> > bool is_pmem;
> > + bool readonly;
> > };
> >
> > static void
> > @@ -57,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, false, errp);
> > + fb->mem_path, fb->readonly, errp);
> > g_free(name);
> > #endif
> > }
> > @@ -152,6 +153,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));
>
>
> The 'host_memory_backend_mr_inited()' function is not documented;
> my understanding is a backend is considered initialized once it has
> a MemoryRegion assigned to it.
>
> So this error message is not very helpful, it doesn't explain the
> reason. I see all other setters in this file use the same error,
> so it is almost a predating issue.
>
> Still I'd rather use a different message, something like:
> "'%s' already initialized, can not set it 'readonly'".
>
> Preferably with the error message reworded:
> Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>
I haven't reworded the error message because it's used in
hostmem-file.c, hostmem-memfd.c, and hostmem.c. A separate patch would
need to change the error messages across these files.
There is no time when users can actually change these QOM properties, so
"cannot change FOO" is a reasonable wording form the user perspective.
Telling the user that there is a pre-initialization state when the
property can be change isn't useful because they cannot observe that
state (the object is created and ->complete is called in a single step).
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH 3/3] nvdimm: honor -object memory-backend-file, readonly=on option
2020-08-21 13:03 ` Philippe Mathieu-Daudé
@ 2020-09-16 9:39 ` Stefan Hajnoczi
0 siblings, 0 replies; 15+ messages in thread
From: Stefan Hajnoczi @ 2020-09-16 9:39 UTC (permalink / raw)
To: Philippe Mathieu-Daudé
Cc: Xiao Guangrong, Michael S. Tsirkin, Julio Montes, qemu-devel,
Paolo Bonzini, Igor Mammedov, Richard Henderson, Eduardo Habkost
[-- Attachment #1: Type: text/plain, Size: 1955 bytes --]
On Fri, Aug 21, 2020 at 03:03:50PM +0200, Philippe Mathieu-Daudé wrote:
> On 8/4/20 12:12 PM, Stefan Hajnoczi wrote:
> > Make it possible to present read-only files to the guest as "unarmed"
> > NVDIMMs. The Linux NVDIMM device (/dev/pmemX) is read-only.
> >
> > 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..c0b52de111 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 the file $PATH is opened read-only or
>
> Double "the the".
Will fix.
> > diff --git a/hw/mem/nvdimm.c b/hw/mem/nvdimm.c
> > index e1574bc07c..694223450e 100644
> > --- a/hw/mem/nvdimm.c
> > +++ b/hw/mem/nvdimm.c
> > @@ -151,6 +151,10 @@ static void nvdimm_prepare_memory_region(NVDIMMDevice *nvdimm, Error **errp)
> > "nvdimm-memory", mr, 0, pmem_size);
> > memory_region_set_nonvolatile(nvdimm->nvdimm_mr, true);
> > nvdimm->nvdimm_mr->align = align;
> > +
> > + if (memory_region_is_rom(mr)) {
> > + nvdimm->unarmed = true; /* this device is read-only */
> > + }
>
> Can you move this hunk before the alias creation?
> (Just before nvdimm->nvdimm_mr = ...).
Will fix.
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH 2/3] hostmem-file: add readonly=on|off option
2020-09-16 9:31 ` Stefan Hajnoczi
@ 2020-09-16 10:17 ` Philippe Mathieu-Daudé
0 siblings, 0 replies; 15+ messages in thread
From: Philippe Mathieu-Daudé @ 2020-09-16 10:17 UTC (permalink / raw)
To: Stefan Hajnoczi
Cc: Xiao Guangrong, Michael S. Tsirkin, Julio Montes, qemu-devel,
Igor Mammedov, Paolo Bonzini, Richard Henderson, Eduardo Habkost
On 9/16/20 11:31 AM, Stefan Hajnoczi wrote:
> On Fri, Aug 21, 2020 at 02:50:42PM +0200, Philippe Mathieu-Daudé wrote:
>> On 8/4/20 12:12 PM, Stefan Hajnoczi 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.
>>>
[...]
>>> +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));
>>
>>
>> The 'host_memory_backend_mr_inited()' function is not documented;
>> my understanding is a backend is considered initialized once it has
>> a MemoryRegion assigned to it.
>>
>> So this error message is not very helpful, it doesn't explain the
>> reason. I see all other setters in this file use the same error,
>> so it is almost a predating issue.
>>
>> Still I'd rather use a different message, something like:
>> "'%s' already initialized, can not set it 'readonly'".
>>
>> Preferably with the error message reworded:
>> Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>
>
> I haven't reworded the error message because it's used in
> hostmem-file.c, hostmem-memfd.c, and hostmem.c. A separate patch would
> need to change the error messages across these files.
>
> There is no time when users can actually change these QOM properties, so
> "cannot change FOO" is a reasonable wording form the user perspective.
> Telling the user that there is a pre-initialization state when the
> property can be change isn't useful because they cannot observe that
> state (the object is created and ->complete is called in a single step).
OK, understood.
^ permalink raw reply [flat|nested] 15+ messages in thread
end of thread, other threads:[~2020-09-16 10:18 UTC | newest]
Thread overview: 15+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-08-04 10:12 [PATCH 0/3] nvdimm: read-only file support Stefan Hajnoczi
2020-08-04 10:12 ` [PATCH 1/3] memory: add readonly support to memory_region_init_ram_from_file() Stefan Hajnoczi
2020-08-04 12:25 ` Philippe Mathieu-Daudé
2020-08-04 12:26 ` Philippe Mathieu-Daudé
2020-08-04 13:47 ` Stefan Hajnoczi
2020-08-04 13:57 ` Philippe Mathieu-Daudé
2020-08-04 10:12 ` [PATCH 2/3] hostmem-file: add readonly=on|off option Stefan Hajnoczi
2020-08-21 12:50 ` Philippe Mathieu-Daudé
2020-09-16 9:31 ` Stefan Hajnoczi
2020-09-16 10:17 ` Philippe Mathieu-Daudé
2020-08-04 10:12 ` [PATCH 3/3] nvdimm: honor -object memory-backend-file, readonly=on option Stefan Hajnoczi
2020-08-21 13:03 ` Philippe Mathieu-Daudé
2020-09-16 9:39 ` Stefan Hajnoczi
2020-08-04 12:28 ` [PATCH 0/3] nvdimm: read-only file support Michael S. Tsirkin
2020-08-21 12:18 ` 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.