* [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
* 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 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
* [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
* 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 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 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
* [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 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 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 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 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
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.