| Message ID | 20230822114504.239505-3-david@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | memory-backend-file related improvements and VM templating support | expand |
Hello, At 2023-08-22 19:44:50, "David Hildenbrand" <david@redhat.com> wrote: >There is a difference between how we open a file and how we mmap it, >and we want to support writable private mappings of readonly files. Let's >define RAM_READONLY and RAM_READONLY_FD flags, to replace the single >"readonly" parameter for file-related functions. > >In memory_region_init_ram_from_fd() and memory_region_init_ram_from_file(), >initialize mr->readonly based on the new RAM_READONLY flag. > >While at it, add some RAM_* flags we missed to add to the list of accepted >flags in the documentation of some functions. > >No change in functionality intended. We'll make use of both flags next >and start setting them independently for memory-backend-file. > >Signed-off-by: David Hildenbrand <david@redhat.com> >--- > backends/hostmem-file.c | 4 ++-- > include/exec/memory.h | 14 ++++++++++---- > include/exec/ram_addr.h | 8 ++++---- > softmmu/memory.c | 8 ++++---- > softmmu/physmem.c | 21 ++++++++++----------- > 5 files changed, 30 insertions(+), 25 deletions(-) > >diff --git a/backends/hostmem-file.c b/backends/hostmem-file.c >index b4335a80e6..ef2d5533af 100644 >--- a/backends/hostmem-file.c >+++ b/backends/hostmem-file.c >@@ -55,13 +55,13 @@ file_backend_memory_alloc(HostMemoryBackend *backend, Error **errp) > > name = host_memory_backend_get_name(backend); > ram_flags = backend->share ? RAM_SHARED : 0; >+ ram_flags |= fb->readonly ? RAM_READONLY | RAM_READONLY_FD : 0; > ram_flags |= backend->reserve ? 0 : RAM_NORESERVE; > ram_flags |= fb->is_pmem ? RAM_PMEM : 0; > ram_flags |= RAM_NAMED_FILE; > memory_region_init_ram_from_file(&backend->mr, OBJECT(backend), name, > backend->size, fb->align, ram_flags, >- fb->mem_path, fb->offset, fb->readonly, >- errp); >+ fb->mem_path, fb->offset, errp); > g_free(name); > #endif > } >diff --git a/include/exec/memory.h b/include/exec/memory.h >index 68284428f8..cc68249eda 100644 >--- a/include/exec/memory.h >+++ b/include/exec/memory.h >@@ -235,6 +235,12 @@ typedef struct IOMMUTLBEvent { > /* RAM is an mmap-ed named file */ > #define RAM_NAMED_FILE (1 << 9) > >+/* RAM is mmap-ed read-only */ >+#define RAM_READONLY (1 << 10) >+ >+/* RAM FD is opened read-only */ >+#define RAM_READONLY_FD (1 << 11) >+ > static inline void iommu_notifier_init(IOMMUNotifier *n, IOMMUNotify fn, > IOMMUNotifierFlag flags, > hwaddr start, hwaddr end, >@@ -1331,10 +1337,10 @@ void memory_region_init_resizeable_ram(MemoryRegion *mr, > * @align: alignment of the region base address; if 0, the default alignment > * (getpagesize()) will be used. > * @ram_flags: RamBlock flags. Supported flags: RAM_SHARED, RAM_PMEM, >- * RAM_NORESERVE, >+ * RAM_NORESERVE, RAM_PROTECTED, RAM_NAMED_FILE, RAM_READONLY, >+ * RAM_READONLY_FD > * @path: the path in which to allocate the RAM. > * @offset: offset within the file referenced by path >- * @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 >@@ -1348,7 +1354,6 @@ void memory_region_init_ram_from_file(MemoryRegion *mr, > uint32_t ram_flags, > const char *path, > ram_addr_t offset, >- bool readonly, > Error **errp); > > /** >@@ -1360,7 +1365,8 @@ void memory_region_init_ram_from_file(MemoryRegion *mr, > * @name: the name of the region. > * @size: size of the region. > * @ram_flags: RamBlock flags. Supported flags: RAM_SHARED, RAM_PMEM, >- * RAM_NORESERVE, RAM_PROTECTED. >+ * RAM_NORESERVE, RAM_PROTECTED, RAM_NAMED_FILE, RAM_READONLY, >+ * RAM_READONLY_FD > * @fd: the fd to mmap. > * @offset: offset within the file referenced by fd > * @errp: pointer to Error*, to store an error if it happens. >diff --git a/include/exec/ram_addr.h b/include/exec/ram_addr.h >index 9f2e3893f5..90676093f5 100644 >--- a/include/exec/ram_addr.h >+++ b/include/exec/ram_addr.h >@@ -108,10 +108,10 @@ long qemu_maxrampagesize(void); > * @size: the size in bytes of the ram block > * @mr: the memory region where the ram block is > * @ram_flags: RamBlock flags. Supported flags: RAM_SHARED, RAM_PMEM, >- * RAM_NORESERVE. >+ * RAM_NORESERVE, RAM_PROTECTED, RAM_NAMED_FILE, RAM_READONLY, >+ * RAM_READONLY_FD > * @mem_path or @fd: specify the backing file or device > * @offset: Offset into target file >- * @readonly: true to open @path for reading, false for read/write. > * @errp: pointer to Error*, to store an error if it happens > * > * Return: >@@ -120,10 +120,10 @@ long qemu_maxrampagesize(void); > */ > RAMBlock *qemu_ram_alloc_from_file(ram_addr_t size, MemoryRegion *mr, > uint32_t ram_flags, const char *mem_path, >- off_t offset, bool readonly, Error **errp); >+ off_t offset, Error **errp); > RAMBlock *qemu_ram_alloc_from_fd(ram_addr_t size, MemoryRegion *mr, > uint32_t ram_flags, int fd, off_t offset, >- bool readonly, Error **errp); >+ Error **errp); > > RAMBlock *qemu_ram_alloc_from_ptr(ram_addr_t size, void *host, > MemoryRegion *mr, Error **errp); >diff --git a/softmmu/memory.c b/softmmu/memory.c >index 7d9494ce70..d8974a1e65 100644 >--- a/softmmu/memory.c >+++ b/softmmu/memory.c >@@ -1620,18 +1620,17 @@ void memory_region_init_ram_from_file(MemoryRegion *mr, > uint32_t ram_flags, > const char *path, > ram_addr_t offset, >- bool readonly, > Error **errp) > { > Error *err = NULL; > memory_region_init(mr, owner, name, size); > mr->ram = true; >- mr->readonly = readonly; >+ mr->readonly = ram_flags & RAM_READONLY; I only did a quick code auditing, but I suspect that ``` mr->readonly = !!(ram_flags & RAM_READONLY); ``` is safer. So is the other parts of the code probably. > 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, >- offset, readonly, &err); >+ offset, &err); > if (err) { > mr->size = int128_zero(); > object_unparent(OBJECT(mr)); >@@ -1651,10 +1650,11 @@ void memory_region_init_ram_from_fd(MemoryRegion *mr, > Error *err = NULL; > memory_region_init(mr, owner, name, size); > mr->ram = true; >+ mr->readonly = ram_flags & RAM_READONLY; for example here. -- Regards, logoerthiner
On 22.08.23 15:13, ThinerLogoer wrote: > Hello, > > At 2023-08-22 19:44:50, "David Hildenbrand" <david@redhat.com> wrote: >> There is a difference between how we open a file and how we mmap it, >> and we want to support writable private mappings of readonly files. Let's >> define RAM_READONLY and RAM_READONLY_FD flags, to replace the single >> "readonly" parameter for file-related functions. >> >> In memory_region_init_ram_from_fd() and memory_region_init_ram_from_file(), >> initialize mr->readonly based on the new RAM_READONLY flag. >> >> While at it, add some RAM_* flags we missed to add to the list of accepted >> flags in the documentation of some functions. >> >> No change in functionality intended. We'll make use of both flags next >> and start setting them independently for memory-backend-file. >> >> Signed-off-by: David Hildenbrand <david@redhat.com> >> --- >> backends/hostmem-file.c | 4 ++-- >> include/exec/memory.h | 14 ++++++++++---- >> include/exec/ram_addr.h | 8 ++++---- >> softmmu/memory.c | 8 ++++---- >> softmmu/physmem.c | 21 ++++++++++----------- >> 5 files changed, 30 insertions(+), 25 deletions(-) >> >> diff --git a/backends/hostmem-file.c b/backends/hostmem-file.c >> index b4335a80e6..ef2d5533af 100644 >> --- a/backends/hostmem-file.c >> +++ b/backends/hostmem-file.c >> @@ -55,13 +55,13 @@ file_backend_memory_alloc(HostMemoryBackend *backend, Error **errp) >> >> name = host_memory_backend_get_name(backend); >> ram_flags = backend->share ? RAM_SHARED : 0; >> + ram_flags |= fb->readonly ? RAM_READONLY | RAM_READONLY_FD : 0; >> ram_flags |= backend->reserve ? 0 : RAM_NORESERVE; >> ram_flags |= fb->is_pmem ? RAM_PMEM : 0; >> ram_flags |= RAM_NAMED_FILE; >> memory_region_init_ram_from_file(&backend->mr, OBJECT(backend), name, >> backend->size, fb->align, ram_flags, >> - fb->mem_path, fb->offset, fb->readonly, >> - errp); >> + fb->mem_path, fb->offset, errp); >> g_free(name); >> #endif >> } >> diff --git a/include/exec/memory.h b/include/exec/memory.h >> index 68284428f8..cc68249eda 100644 >> --- a/include/exec/memory.h >> +++ b/include/exec/memory.h >> @@ -235,6 +235,12 @@ typedef struct IOMMUTLBEvent { >> /* RAM is an mmap-ed named file */ >> #define RAM_NAMED_FILE (1 << 9) >> >> +/* RAM is mmap-ed read-only */ >> +#define RAM_READONLY (1 << 10) >> + >> +/* RAM FD is opened read-only */ >> +#define RAM_READONLY_FD (1 << 11) >> + >> static inline void iommu_notifier_init(IOMMUNotifier *n, IOMMUNotify fn, >> IOMMUNotifierFlag flags, >> hwaddr start, hwaddr end, >> @@ -1331,10 +1337,10 @@ void memory_region_init_resizeable_ram(MemoryRegion *mr, >> * @align: alignment of the region base address; if 0, the default alignment >> * (getpagesize()) will be used. >> * @ram_flags: RamBlock flags. Supported flags: RAM_SHARED, RAM_PMEM, >> - * RAM_NORESERVE, >> + * RAM_NORESERVE, RAM_PROTECTED, RAM_NAMED_FILE, RAM_READONLY, >> + * RAM_READONLY_FD >> * @path: the path in which to allocate the RAM. >> * @offset: offset within the file referenced by path >> - * @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 >> @@ -1348,7 +1354,6 @@ void memory_region_init_ram_from_file(MemoryRegion *mr, >> uint32_t ram_flags, >> const char *path, >> ram_addr_t offset, >> - bool readonly, >> Error **errp); >> >> /** >> @@ -1360,7 +1365,8 @@ void memory_region_init_ram_from_file(MemoryRegion *mr, >> * @name: the name of the region. >> * @size: size of the region. >> * @ram_flags: RamBlock flags. Supported flags: RAM_SHARED, RAM_PMEM, >> - * RAM_NORESERVE, RAM_PROTECTED. >> + * RAM_NORESERVE, RAM_PROTECTED, RAM_NAMED_FILE, RAM_READONLY, >> + * RAM_READONLY_FD >> * @fd: the fd to mmap. >> * @offset: offset within the file referenced by fd >> * @errp: pointer to Error*, to store an error if it happens. >> diff --git a/include/exec/ram_addr.h b/include/exec/ram_addr.h >> index 9f2e3893f5..90676093f5 100644 >> --- a/include/exec/ram_addr.h >> +++ b/include/exec/ram_addr.h >> @@ -108,10 +108,10 @@ long qemu_maxrampagesize(void); >> * @size: the size in bytes of the ram block >> * @mr: the memory region where the ram block is >> * @ram_flags: RamBlock flags. Supported flags: RAM_SHARED, RAM_PMEM, >> - * RAM_NORESERVE. >> + * RAM_NORESERVE, RAM_PROTECTED, RAM_NAMED_FILE, RAM_READONLY, >> + * RAM_READONLY_FD >> * @mem_path or @fd: specify the backing file or device >> * @offset: Offset into target file >> - * @readonly: true to open @path for reading, false for read/write. >> * @errp: pointer to Error*, to store an error if it happens >> * >> * Return: >> @@ -120,10 +120,10 @@ long qemu_maxrampagesize(void); >> */ >> RAMBlock *qemu_ram_alloc_from_file(ram_addr_t size, MemoryRegion *mr, >> uint32_t ram_flags, const char *mem_path, >> - off_t offset, bool readonly, Error **errp); >> + off_t offset, Error **errp); >> RAMBlock *qemu_ram_alloc_from_fd(ram_addr_t size, MemoryRegion *mr, >> uint32_t ram_flags, int fd, off_t offset, >> - bool readonly, Error **errp); >> + Error **errp); >> >> RAMBlock *qemu_ram_alloc_from_ptr(ram_addr_t size, void *host, >> MemoryRegion *mr, Error **errp); >> diff --git a/softmmu/memory.c b/softmmu/memory.c >> index 7d9494ce70..d8974a1e65 100644 >> --- a/softmmu/memory.c >> +++ b/softmmu/memory.c >> @@ -1620,18 +1620,17 @@ void memory_region_init_ram_from_file(MemoryRegion *mr, >> uint32_t ram_flags, >> const char *path, >> ram_addr_t offset, >> - bool readonly, >> Error **errp) >> { >> Error *err = NULL; >> memory_region_init(mr, owner, name, size); >> mr->ram = true; >> - mr->readonly = readonly; >> + mr->readonly = ram_flags & RAM_READONLY; > > I only did a quick code auditing, but I suspect that > ``` > mr->readonly = !!(ram_flags & RAM_READONLY); > ``` > is safer. So is the other parts of the code probably. Yes, looks cleaner, thanks!
diff --git a/backends/hostmem-file.c b/backends/hostmem-file.c index b4335a80e6..ef2d5533af 100644 --- a/backends/hostmem-file.c +++ b/backends/hostmem-file.c @@ -55,13 +55,13 @@ file_backend_memory_alloc(HostMemoryBackend *backend, Error **errp) name = host_memory_backend_get_name(backend); ram_flags = backend->share ? RAM_SHARED : 0; + ram_flags |= fb->readonly ? RAM_READONLY | RAM_READONLY_FD : 0; ram_flags |= backend->reserve ? 0 : RAM_NORESERVE; ram_flags |= fb->is_pmem ? RAM_PMEM : 0; ram_flags |= RAM_NAMED_FILE; memory_region_init_ram_from_file(&backend->mr, OBJECT(backend), name, backend->size, fb->align, ram_flags, - fb->mem_path, fb->offset, fb->readonly, - errp); + fb->mem_path, fb->offset, errp); g_free(name); #endif } diff --git a/include/exec/memory.h b/include/exec/memory.h index 68284428f8..cc68249eda 100644 --- a/include/exec/memory.h +++ b/include/exec/memory.h @@ -235,6 +235,12 @@ typedef struct IOMMUTLBEvent { /* RAM is an mmap-ed named file */ #define RAM_NAMED_FILE (1 << 9) +/* RAM is mmap-ed read-only */ +#define RAM_READONLY (1 << 10) + +/* RAM FD is opened read-only */ +#define RAM_READONLY_FD (1 << 11) + static inline void iommu_notifier_init(IOMMUNotifier *n, IOMMUNotify fn, IOMMUNotifierFlag flags, hwaddr start, hwaddr end, @@ -1331,10 +1337,10 @@ void memory_region_init_resizeable_ram(MemoryRegion *mr, * @align: alignment of the region base address; if 0, the default alignment * (getpagesize()) will be used. * @ram_flags: RamBlock flags. Supported flags: RAM_SHARED, RAM_PMEM, - * RAM_NORESERVE, + * RAM_NORESERVE, RAM_PROTECTED, RAM_NAMED_FILE, RAM_READONLY, + * RAM_READONLY_FD * @path: the path in which to allocate the RAM. * @offset: offset within the file referenced by path - * @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 @@ -1348,7 +1354,6 @@ void memory_region_init_ram_from_file(MemoryRegion *mr, uint32_t ram_flags, const char *path, ram_addr_t offset, - bool readonly, Error **errp); /** @@ -1360,7 +1365,8 @@ void memory_region_init_ram_from_file(MemoryRegion *mr, * @name: the name of the region. * @size: size of the region. * @ram_flags: RamBlock flags. Supported flags: RAM_SHARED, RAM_PMEM, - * RAM_NORESERVE, RAM_PROTECTED. + * RAM_NORESERVE, RAM_PROTECTED, RAM_NAMED_FILE, RAM_READONLY, + * RAM_READONLY_FD * @fd: the fd to mmap. * @offset: offset within the file referenced by fd * @errp: pointer to Error*, to store an error if it happens. diff --git a/include/exec/ram_addr.h b/include/exec/ram_addr.h index 9f2e3893f5..90676093f5 100644 --- a/include/exec/ram_addr.h +++ b/include/exec/ram_addr.h @@ -108,10 +108,10 @@ long qemu_maxrampagesize(void); * @size: the size in bytes of the ram block * @mr: the memory region where the ram block is * @ram_flags: RamBlock flags. Supported flags: RAM_SHARED, RAM_PMEM, - * RAM_NORESERVE. + * RAM_NORESERVE, RAM_PROTECTED, RAM_NAMED_FILE, RAM_READONLY, + * RAM_READONLY_FD * @mem_path or @fd: specify the backing file or device * @offset: Offset into target file - * @readonly: true to open @path for reading, false for read/write. * @errp: pointer to Error*, to store an error if it happens * * Return: @@ -120,10 +120,10 @@ long qemu_maxrampagesize(void); */ RAMBlock *qemu_ram_alloc_from_file(ram_addr_t size, MemoryRegion *mr, uint32_t ram_flags, const char *mem_path, - off_t offset, bool readonly, Error **errp); + off_t offset, Error **errp); RAMBlock *qemu_ram_alloc_from_fd(ram_addr_t size, MemoryRegion *mr, uint32_t ram_flags, int fd, off_t offset, - bool readonly, Error **errp); + Error **errp); RAMBlock *qemu_ram_alloc_from_ptr(ram_addr_t size, void *host, MemoryRegion *mr, Error **errp); diff --git a/softmmu/memory.c b/softmmu/memory.c index 7d9494ce70..d8974a1e65 100644 --- a/softmmu/memory.c +++ b/softmmu/memory.c @@ -1620,18 +1620,17 @@ void memory_region_init_ram_from_file(MemoryRegion *mr, uint32_t ram_flags, const char *path, ram_addr_t offset, - bool readonly, Error **errp) { Error *err = NULL; memory_region_init(mr, owner, name, size); mr->ram = true; - mr->readonly = readonly; + mr->readonly = ram_flags & RAM_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, - offset, readonly, &err); + offset, &err); if (err) { mr->size = int128_zero(); object_unparent(OBJECT(mr)); @@ -1651,10 +1650,11 @@ void memory_region_init_ram_from_fd(MemoryRegion *mr, Error *err = NULL; memory_region_init(mr, owner, name, size); mr->ram = true; + mr->readonly = ram_flags & RAM_READONLY; mr->terminates = true; mr->destructor = memory_region_destructor_ram; mr->ram_block = qemu_ram_alloc_from_fd(size, mr, ram_flags, fd, offset, - false, &err); + &err); if (err) { mr->size = int128_zero(); object_unparent(OBJECT(mr)); diff --git a/softmmu/physmem.c b/softmmu/physmem.c index 3df73542e1..16d7a16aa8 100644 --- a/softmmu/physmem.c +++ b/softmmu/physmem.c @@ -1351,7 +1351,6 @@ 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, off_t offset, Error **errp) @@ -1409,7 +1408,7 @@ static void *file_ram_alloc(RAMBlock *block, perror("ftruncate"); } - qemu_map_flags = readonly ? QEMU_MAP_READONLY : 0; + qemu_map_flags = (block->flags & RAM_READONLY) ? QEMU_MAP_READONLY : 0; qemu_map_flags |= (block->flags & RAM_SHARED) ? QEMU_MAP_SHARED : 0; qemu_map_flags |= (block->flags & RAM_PMEM) ? QEMU_MAP_SYNC : 0; qemu_map_flags |= (block->flags & RAM_NORESERVE) ? QEMU_MAP_NORESERVE : 0; @@ -1877,7 +1876,7 @@ static void ram_block_add(RAMBlock *new_block, Error **errp) #ifdef CONFIG_POSIX RAMBlock *qemu_ram_alloc_from_fd(ram_addr_t size, MemoryRegion *mr, uint32_t ram_flags, int fd, off_t offset, - bool readonly, Error **errp) + Error **errp) { RAMBlock *new_block; Error *local_err = NULL; @@ -1885,7 +1884,8 @@ RAMBlock *qemu_ram_alloc_from_fd(ram_addr_t size, MemoryRegion *mr, /* Just support these ram flags by now. */ assert((ram_flags & ~(RAM_SHARED | RAM_PMEM | RAM_NORESERVE | - RAM_PROTECTED | RAM_NAMED_FILE)) == 0); + RAM_PROTECTED | RAM_NAMED_FILE | RAM_READONLY | + RAM_READONLY_FD)) == 0); if (xen_enabled()) { error_setg(errp, "-mem-path not supported with Xen"); @@ -1920,8 +1920,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, readonly, - !file_size, offset, errp); + new_block->host = file_ram_alloc(new_block, size, fd, !file_size, offset, + errp); if (!new_block->host) { g_free(new_block); return NULL; @@ -1940,20 +1940,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, - off_t offset, bool readonly, Error **errp) + off_t offset, Error **errp) { int fd; bool created; RAMBlock *block; - fd = file_ram_open(mem_path, memory_region_name(mr), readonly, &created, - errp); + fd = file_ram_open(mem_path, memory_region_name(mr), + ram_flags & RAM_READONLY_FD, &created, errp); if (fd < 0) { return NULL; } - block = qemu_ram_alloc_from_fd(size, mr, ram_flags, fd, offset, readonly, - errp); + block = qemu_ram_alloc_from_fd(size, mr, ram_flags, fd, offset, errp); if (!block) { if (created) { unlink(mem_path);
There is a difference between how we open a file and how we mmap it, and we want to support writable private mappings of readonly files. Let's define RAM_READONLY and RAM_READONLY_FD flags, to replace the single "readonly" parameter for file-related functions. In memory_region_init_ram_from_fd() and memory_region_init_ram_from_file(), initialize mr->readonly based on the new RAM_READONLY flag. While at it, add some RAM_* flags we missed to add to the list of accepted flags in the documentation of some functions. No change in functionality intended. We'll make use of both flags next and start setting them independently for memory-backend-file. Signed-off-by: David Hildenbrand <david@redhat.com> --- backends/hostmem-file.c | 4 ++-- include/exec/memory.h | 14 ++++++++++---- include/exec/ram_addr.h | 8 ++++---- softmmu/memory.c | 8 ++++---- softmmu/physmem.c | 21 ++++++++++----------- 5 files changed, 30 insertions(+), 25 deletions(-)