Message ID | 20210225230238.3719051-2-philmd@redhat.com (mailing list archive) |
---|---|
State | New, archived |
Headers | show |
Series | hw/block/pflash: Mmap read-only backend files with MAP_SHARED | expand |
I don't know this code very well, but I have a couple of comments below :-) On Fri, Feb 26, 2021 at 12:02:36AM +0100, Philippe Mathieu-Daudé wrote: >Introduce memory_region_init_rom_device_from_file() which mmap >the backing file of ROM devices. This allows to reduce QEMU memory >footprint as the same file can be shared between multiple instances >of QEMU. > >Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com> >--- > include/exec/memory.h | 85 +++++++++++++++++++++++++++++++++++++ > softmmu/memory.c | 98 +++++++++++++++++++++++++++++++++++++++++++ > 2 files changed, 183 insertions(+) > >diff --git a/include/exec/memory.h b/include/exec/memory.h >index c6fb714e499..bacf7495003 100644 >--- a/include/exec/memory.h >+++ b/include/exec/memory.h >@@ -487,6 +487,9 @@ struct MemoryRegion { > const char *name; > unsigned ioeventfd_nb; > MemoryRegionIoeventfd *ioeventfds; >+#ifndef CONFIG_POSIX >+ gchar *contents; >+#endif > }; > > struct IOMMUMemoryRegion { >@@ -1131,6 +1134,43 @@ void memory_region_init_rom_device_nomigrate(MemoryRegion *mr, > uint64_t size, > Error **errp); > >+/** >+ * memory_region_init_rom_device_from_file_nomigrate: >+ * Initialize a ROM memory region from the specified backing file. >+ * Writes are handled via callbacks. >+ * >+ * Note that this function does not do anything to cause the data in the >+ * RAM side of the memory region to be migrated; that is the responsibility >+ * of the caller. >+ * >+ * @mr: the #MemoryRegion to be initialized. >+ * @owner: the object that tracks the region's reference count >+ * @ops: callbacks for write access handling (must not be NULL). >+ * @opaque: passed to the read and write callbacks of the @ops structure. >+ * @name: Region name, becomes part of RAMBlock name used in migration stream >+ * must be unique within any device >+ * @size: size of the region. >+ * @ram_flags: specify the properties of the ram block, which can be one >+ * or bit-or of following values >+ * - RAM_SHARED: mmap the backing file or device with MAP_SHARED >+ * - RAM_PMEM: the backend @mem_path is persistent memory >+ * Other bits are ignored. >+ * @path: specify the backing file >+ * @readonly: true to open @path for reading, false for read/write. >+ * @errp: pointer to Error*, to store an error if it happens. >+ */ >+void memory_region_init_rom_device_from_file_nomigrate(MemoryRegion *mr, >+ Object *owner, >+ const MemoryRegionOps *ops, >+ void *opaque, >+ const char >*name, >+ uint64_t size, >+ uint64_t align, >+ uint32_t ram_flags, >+ const char *path, >+ bool readonly, >+ Error **errp); >+ > /** > * memory_region_init_iommu: Initialize a memory region of a custom type > * that translates addresses >@@ -1249,6 +1289,51 @@ void memory_region_init_rom_device(MemoryRegion *mr, > Error **errp); > > >+/** >+ * memory_region_init_rom_device_from_file: >+ * Initialize a ROM memory region from the specified backing file. >+ * Writes are handled via callbacks. >+ * >+ * This function initializes a memory region backed by RAM for reads >+ * and callbacks for writes, and arranges for the RAM backing to >+ * be migrated (by calling vmstate_register_ram() >+ * if @owner is a DeviceState, or vmstate_register_ram_global() if >+ * @owner is NULL). >+ * >+ * TODO: Currently we restrict @owner to being either NULL (for >+ * global RAM regions with no owner) or devices, so that we can >+ * give the RAM block a unique name for migration purposes. >+ * We should lift this restriction and allow arbitrary Objects. >+ * If you pass a non-NULL non-device @owner then we will assert. >+ * >+ * @mr: the #MemoryRegion to be initialized. >+ * @owner: the object that tracks the region's reference count >+ * @ops: callbacks for write access handling (must not be NULL). >+ * @opaque: passed to the read and write callbacks of the @ops structure. >+ * @name: Region name, becomes part of RAMBlock name used in migration stream >+ * must be unique within any device >+ * @size: size of the region. >+ * @ram_flags: specify the properties of the ram block, which can be one >+ * or bit-or of following values >+ * - RAM_SHARED: mmap the backing file or device with MAP_SHARED >+ * - RAM_PMEM: the backend @mem_path is persistent memory >+ * Other bits are ignored. >+ * @path: specify the backing file >+ * @readonly: true to open @path for reading, false for read/write. >+ * @errp: pointer to Error*, to store an error if it happens. >+ */ >+void memory_region_init_rom_device_from_file(MemoryRegion *mr, >+ Object *owner, >+ const MemoryRegionOps *ops, >+ void *opaque, >+ const char *name, >+ uint64_t size, >+ uint64_t align, >+ uint32_t ram_flags, >+ const char *path, >+ bool readonly, >+ Error **errp); >+ > /** > * memory_region_owner: get a memory region's owner. > * >diff --git a/softmmu/memory.c b/softmmu/memory.c >index 874a8fccdee..ea1892a8cd6 100644 >--- a/softmmu/memory.c >+++ b/softmmu/memory.c >@@ -1120,6 +1120,14 @@ static void memory_region_destructor_ram(MemoryRegion *mr) > qemu_ram_free(mr->ram_block); > } > >+#ifndef CONFIG_POSIX >+static void memory_region_destructor_contents(MemoryRegion *mr) >+{ >+ qemu_ram_free(mr->ram_block); >+ g_free(mr->contents); >+} >+#endif >+ > static bool memory_region_need_escape(char c) > { > return c == '/' || c == '[' || c == '\\' || c == ']'; >@@ -1712,6 +1720,96 @@ void memory_region_init_rom_device_nomigrate(MemoryRegion *mr, > } > } > >+void memory_region_init_rom_device_from_file_nomigrate(MemoryRegion *mr, >+ Object *owner, >+ const MemoryRegionOps *ops, >+ void *opaque, >+ const char *name, >+ uint64_t size, >+ uint64_t align, >+ uint32_t ram_flags, >+ const char *path, >+ bool readonly, >+ Error **errp) >+{ >+ Error *err = NULL; >+ >+ assert(ops); >+#ifdef CONFIG_POSIX >+ memory_region_init(mr, owner, name, size); >+ mr->opaque = opaque; >+ mr->ops = ops; >+ mr->rom_device = true; >+ mr->readonly = readonly; >+ mr->ram = true; >+ mr->align = align; >+ mr->terminates = true; >+ mr->destructor = memory_region_destructor_ram; >+ mr->ram_block = qemu_ram_alloc_from_file(size, mr, ram_flags, path, >+ readonly, &err); >+ if (err) { >+ mr->size = int128_zero(); >+ object_unparent(OBJECT(mr)); >+ error_propagate(errp, err); >+ } >+#else >+ g_autoptr(GError) gerr = NULL; >+ gsize len; >+ >+ memory_region_init(mr, owner, name, size); >+ mr->ops = ops; >+ mr->opaque = opaque; >+ mr->terminates = true; >+ mr->rom_device = true; Why when CONFIG_POSIX is defined we set 'mr->ram', 'mr->align', and 'mr->readonly = readonly' but not here? (I honestly don't know if they are important, I ask out of curiosity. :-) >+ >+ if (!g_file_get_contents(path, &mr->contents, &len, &gerr)) { Should we do these steps in case of an error? mr->size = int128_zero(); object_unparent(OBJECT(mr)); >+ error_setg(errp, "Unable to read '%s': %s", path, gerr->message); >+ return; >+ } >+ mr->destructor = memory_region_destructor_contents; >+ mr->contents = g_realloc(mr->contents, size); >+ mr->ram_block = qemu_ram_alloc_from_ptr(size, mr->contents, mr, &err); >+ if (err) { >+ mr->size = int128_zero(); >+ object_unparent(OBJECT(mr)); >+ error_propagate(errp, err); >+ } >+#endif Maybe I would reorganize the code inside ifdef like this: memory_region_init(mr, owner, name, size); mr->opaque = opaque; ... #ifdef CONFIG_POSIX mr->destructor = memory_region_destructor_ram; mr->ram_block = qemu_ram_alloc_from_file(...); #else if (!g_file_get_contents(..)) { ... } mr->destructor = memory_region_destructor_contents; mr->contents = g_realloc(mr->contents, size); mr->ram_block = qemu_ram_alloc_from_ptr(...) #endif if (err) { ... } I don't have a strong opinion, just an idea. Thanks, Stefano >+} >+ >+void memory_region_init_rom_device_from_file(MemoryRegion *mr, >+ Object *owner, >+ const MemoryRegionOps >*ops, >+ void *opaque, >+ const char *name, >+ uint64_t size, >+ uint64_t align, >+ uint32_t ram_flags, >+ const char *path, >+ bool readonly, >+ Error **errp) >+{ >+ DeviceState *owner_dev; >+ Error *err = NULL; >+ >+ memory_region_init_rom_device_from_file_nomigrate(mr, owner, ops, opaque, >+ name, size, align, >+ ram_flags, path, readonly, >+ &err); >+ if (err) { >+ error_propagate(errp, err); >+ return; >+ } >+ /* This will assert if owner is neither NULL nor a DeviceState. >+ * We only want the owner here for the purposes of defining a >+ * unique name for migration. TODO: Ideally we should implement >+ * a naming scheme for Objects which are not DeviceStates, in >+ * which case we can relax this restriction. >+ */ >+ owner_dev = DEVICE(owner); >+ vmstate_register_ram(mr, owner_dev); >+} >+ > void memory_region_init_iommu(void *_iommu_mr, > size_t instance_size, > const char *mrtypename, >-- >2.26.2 >
On 3/1/21 12:53 PM, Stefano Garzarella wrote: > I don't know this code very well, but I have a couple of comments below :-) > > On Fri, Feb 26, 2021 at 12:02:36AM +0100, Philippe Mathieu-Daudé wrote: >> Introduce memory_region_init_rom_device_from_file() which mmap >> the backing file of ROM devices. This allows to reduce QEMU memory >> footprint as the same file can be shared between multiple instances >> of QEMU. >> >> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com> >> --- >> include/exec/memory.h | 85 +++++++++++++++++++++++++++++++++++++ >> softmmu/memory.c | 98 +++++++++++++++++++++++++++++++++++++++++++ >> 2 files changed, 183 insertions(+) ... >> +void memory_region_init_rom_device_from_file_nomigrate(MemoryRegion *mr, >> + Object *owner, >> + const >> MemoryRegionOps *ops, >> + void *opaque, >> + const char *name, >> + uint64_t size, >> + uint64_t align, >> + uint32_t >> ram_flags, >> + const char *path, >> + bool readonly, >> + Error **errp) >> +{ >> + Error *err = NULL; >> + >> + assert(ops); >> +#ifdef CONFIG_POSIX >> + memory_region_init(mr, owner, name, size); >> + mr->opaque = opaque; >> + mr->ops = ops; >> + mr->rom_device = true; >> + mr->readonly = readonly; >> + mr->ram = true; >> + mr->align = align; >> + mr->terminates = true; >> + mr->destructor = memory_region_destructor_ram; >> + mr->ram_block = qemu_ram_alloc_from_file(size, mr, ram_flags, path, >> + readonly, &err); >> + if (err) { >> + mr->size = int128_zero(); >> + object_unparent(OBJECT(mr)); >> + error_propagate(errp, err); >> + } >> +#else >> + g_autoptr(GError) gerr = NULL; >> + gsize len; >> + >> + memory_region_init(mr, owner, name, size); >> + mr->ops = ops; >> + mr->opaque = opaque; >> + mr->terminates = true; >> + mr->rom_device = true; > > Why when CONFIG_POSIX is defined we set 'mr->ram', 'mr->align', and > 'mr->readonly = readonly' but not here? > (I honestly don't know if they are important, I ask out of curiosity. :-) I suppose we should and I forgot :/ > >> + >> + if (!g_file_get_contents(path, &mr->contents, &len, &gerr)) { > > Should we do these steps in case of an error? > > mr->size = int128_zero(); > object_unparent(OBJECT(mr)); Yes... > >> + error_setg(errp, "Unable to read '%s': %s", path, >> gerr->message); >> + return; >> + } >> + mr->destructor = memory_region_destructor_contents; >> + mr->contents = g_realloc(mr->contents, size); >> + mr->ram_block = qemu_ram_alloc_from_ptr(size, mr->contents, mr, >> &err); >> + if (err) { >> + mr->size = int128_zero(); >> + object_unparent(OBJECT(mr)); >> + error_propagate(errp, err); >> + } >> +#endif > > Maybe I would reorganize the code inside ifdef like this: > > memory_region_init(mr, owner, name, size); > mr->opaque = opaque; > ... > #ifdef CONFIG_POSIX > mr->destructor = memory_region_destructor_ram; > mr->ram_block = qemu_ram_alloc_from_file(...); > #else > if (!g_file_get_contents(..)) { > ... > } > mr->destructor = memory_region_destructor_contents; > mr->contents = g_realloc(mr->contents, size); > mr->ram_block = qemu_ram_alloc_from_ptr(...) > #endif > > if (err) { > ... > } Yes, thanks :) > > I don't have a strong opinion, just an idea. > > Thanks, > Stefano
diff --git a/include/exec/memory.h b/include/exec/memory.h index c6fb714e499..bacf7495003 100644 --- a/include/exec/memory.h +++ b/include/exec/memory.h @@ -487,6 +487,9 @@ struct MemoryRegion { const char *name; unsigned ioeventfd_nb; MemoryRegionIoeventfd *ioeventfds; +#ifndef CONFIG_POSIX + gchar *contents; +#endif }; struct IOMMUMemoryRegion { @@ -1131,6 +1134,43 @@ void memory_region_init_rom_device_nomigrate(MemoryRegion *mr, uint64_t size, Error **errp); +/** + * memory_region_init_rom_device_from_file_nomigrate: + * Initialize a ROM memory region from the specified backing file. + * Writes are handled via callbacks. + * + * Note that this function does not do anything to cause the data in the + * RAM side of the memory region to be migrated; that is the responsibility + * of the caller. + * + * @mr: the #MemoryRegion to be initialized. + * @owner: the object that tracks the region's reference count + * @ops: callbacks for write access handling (must not be NULL). + * @opaque: passed to the read and write callbacks of the @ops structure. + * @name: Region name, becomes part of RAMBlock name used in migration stream + * must be unique within any device + * @size: size of the region. + * @ram_flags: specify the properties of the ram block, which can be one + * or bit-or of following values + * - RAM_SHARED: mmap the backing file or device with MAP_SHARED + * - RAM_PMEM: the backend @mem_path is persistent memory + * Other bits are ignored. + * @path: specify the backing file + * @readonly: true to open @path for reading, false for read/write. + * @errp: pointer to Error*, to store an error if it happens. + */ +void memory_region_init_rom_device_from_file_nomigrate(MemoryRegion *mr, + Object *owner, + const MemoryRegionOps *ops, + void *opaque, + const char *name, + uint64_t size, + uint64_t align, + uint32_t ram_flags, + const char *path, + bool readonly, + Error **errp); + /** * memory_region_init_iommu: Initialize a memory region of a custom type * that translates addresses @@ -1249,6 +1289,51 @@ void memory_region_init_rom_device(MemoryRegion *mr, Error **errp); +/** + * memory_region_init_rom_device_from_file: + * Initialize a ROM memory region from the specified backing file. + * Writes are handled via callbacks. + * + * This function initializes a memory region backed by RAM for reads + * and callbacks for writes, and arranges for the RAM backing to + * be migrated (by calling vmstate_register_ram() + * if @owner is a DeviceState, or vmstate_register_ram_global() if + * @owner is NULL). + * + * TODO: Currently we restrict @owner to being either NULL (for + * global RAM regions with no owner) or devices, so that we can + * give the RAM block a unique name for migration purposes. + * We should lift this restriction and allow arbitrary Objects. + * If you pass a non-NULL non-device @owner then we will assert. + * + * @mr: the #MemoryRegion to be initialized. + * @owner: the object that tracks the region's reference count + * @ops: callbacks for write access handling (must not be NULL). + * @opaque: passed to the read and write callbacks of the @ops structure. + * @name: Region name, becomes part of RAMBlock name used in migration stream + * must be unique within any device + * @size: size of the region. + * @ram_flags: specify the properties of the ram block, which can be one + * or bit-or of following values + * - RAM_SHARED: mmap the backing file or device with MAP_SHARED + * - RAM_PMEM: the backend @mem_path is persistent memory + * Other bits are ignored. + * @path: specify the backing file + * @readonly: true to open @path for reading, false for read/write. + * @errp: pointer to Error*, to store an error if it happens. + */ +void memory_region_init_rom_device_from_file(MemoryRegion *mr, + Object *owner, + const MemoryRegionOps *ops, + void *opaque, + const char *name, + uint64_t size, + uint64_t align, + uint32_t ram_flags, + const char *path, + bool readonly, + Error **errp); + /** * memory_region_owner: get a memory region's owner. * diff --git a/softmmu/memory.c b/softmmu/memory.c index 874a8fccdee..ea1892a8cd6 100644 --- a/softmmu/memory.c +++ b/softmmu/memory.c @@ -1120,6 +1120,14 @@ static void memory_region_destructor_ram(MemoryRegion *mr) qemu_ram_free(mr->ram_block); } +#ifndef CONFIG_POSIX +static void memory_region_destructor_contents(MemoryRegion *mr) +{ + qemu_ram_free(mr->ram_block); + g_free(mr->contents); +} +#endif + static bool memory_region_need_escape(char c) { return c == '/' || c == '[' || c == '\\' || c == ']'; @@ -1712,6 +1720,96 @@ void memory_region_init_rom_device_nomigrate(MemoryRegion *mr, } } +void memory_region_init_rom_device_from_file_nomigrate(MemoryRegion *mr, + Object *owner, + const MemoryRegionOps *ops, + void *opaque, + const char *name, + uint64_t size, + uint64_t align, + uint32_t ram_flags, + const char *path, + bool readonly, + Error **errp) +{ + Error *err = NULL; + + assert(ops); +#ifdef CONFIG_POSIX + memory_region_init(mr, owner, name, size); + mr->opaque = opaque; + mr->ops = ops; + mr->rom_device = true; + mr->readonly = readonly; + mr->ram = true; + mr->align = align; + mr->terminates = true; + mr->destructor = memory_region_destructor_ram; + mr->ram_block = qemu_ram_alloc_from_file(size, mr, ram_flags, path, + readonly, &err); + if (err) { + mr->size = int128_zero(); + object_unparent(OBJECT(mr)); + error_propagate(errp, err); + } +#else + g_autoptr(GError) gerr = NULL; + gsize len; + + memory_region_init(mr, owner, name, size); + mr->ops = ops; + mr->opaque = opaque; + mr->terminates = true; + mr->rom_device = true; + + if (!g_file_get_contents(path, &mr->contents, &len, &gerr)) { + error_setg(errp, "Unable to read '%s': %s", path, gerr->message); + return; + } + mr->destructor = memory_region_destructor_contents; + mr->contents = g_realloc(mr->contents, size); + mr->ram_block = qemu_ram_alloc_from_ptr(size, mr->contents, mr, &err); + if (err) { + mr->size = int128_zero(); + object_unparent(OBJECT(mr)); + error_propagate(errp, err); + } +#endif +} + +void memory_region_init_rom_device_from_file(MemoryRegion *mr, + Object *owner, + const MemoryRegionOps *ops, + void *opaque, + const char *name, + uint64_t size, + uint64_t align, + uint32_t ram_flags, + const char *path, + bool readonly, + Error **errp) +{ + DeviceState *owner_dev; + Error *err = NULL; + + memory_region_init_rom_device_from_file_nomigrate(mr, owner, ops, opaque, + name, size, align, + ram_flags, path, readonly, + &err); + if (err) { + error_propagate(errp, err); + return; + } + /* This will assert if owner is neither NULL nor a DeviceState. + * We only want the owner here for the purposes of defining a + * unique name for migration. TODO: Ideally we should implement + * a naming scheme for Objects which are not DeviceStates, in + * which case we can relax this restriction. + */ + owner_dev = DEVICE(owner); + vmstate_register_ram(mr, owner_dev); +} + void memory_region_init_iommu(void *_iommu_mr, size_t instance_size, const char *mrtypename,
Introduce memory_region_init_rom_device_from_file() which mmap the backing file of ROM devices. This allows to reduce QEMU memory footprint as the same file can be shared between multiple instances of QEMU. Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com> --- include/exec/memory.h | 85 +++++++++++++++++++++++++++++++++++++ softmmu/memory.c | 98 +++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 183 insertions(+)