diff mbox series

[RFC,1/3] exec/memory: Introduce memory_region_init_rom_device_from_file()

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

Commit Message

Philippe Mathieu-Daudé Feb. 25, 2021, 11:02 p.m. UTC
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(+)

Comments

Stefano Garzarella March 1, 2021, 11:53 a.m. UTC | #1
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
>
Philippe Mathieu-Daudé March 1, 2021, 11:59 a.m. UTC | #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 mbox series

Patch

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,