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 <[email protected]> > --- > 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) { [...]
