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;
+
+ 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,
--
2.26.2
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
© 2016 - 2025 Red Hat, Inc.