* [RFC PATCH 0/3] hw/block/pflash: Mmap read-only backend files with MAP_SHARED
@ 2021-02-25 23:02 Philippe Mathieu-Daudé
2021-02-25 23:02 ` [RFC PATCH 1/3] exec/memory: Introduce memory_region_init_rom_device_from_file() Philippe Mathieu-Daudé
` (2 more replies)
0 siblings, 3 replies; 11+ messages in thread
From: Philippe Mathieu-Daudé @ 2021-02-25 23:02 UTC (permalink / raw)
To: qemu-devel, David Edmondson
Cc: Kevin Wolf, qemu-block, Xu Yandong, Markus Armbruster, Max Reitz,
Zheng Xiang, haibinzhang, Stefan Hajnoczi, Paolo Bonzini,
Philippe Mathieu-Daudé,
Stefano Garzarella
Hi,
This series aims to reduce the memory footprint of flash devices
when the backing file is read-only.
When a backing file is read-only, the model considers the flash
is in "protected" mode. No write are allowed, but the MMIO
state machine is still usable.
This series introduces a new memory region helper to mmap files
and use it with the pflash device (only with read-only backing
files).
The goal is to reduce QEMU's memory footprint when multiple VMs
are instantiated using the same read-only backing file, which is
the case with the CODE flash from OVMF and AAVMF.
Previous attempts:
- Huawei
https://www.mail-archive.com/qemu-devel@nongnu.org/msg607292.html
- Tencent
https://www.mail-archive.com/qemu-devel@nongnu.org/msg742066.html
- Oracle
https://www.mail-archive.com/qemu-devel@nongnu.org/msg760065.html
RFC because yet another approach to tackle this technical debt,
and very little tested.
Regards,
Phil.
Philippe Mathieu-Daudé (3):
exec/memory: Introduce memory_region_init_rom_device_from_file()
hw/block/pflash: Move code around
hw/block/pflash: use memory_region_init_rom_device_from_file()
include/exec/memory.h | 85 +++++++++++++++++++++++++++++++++++
hw/block/pflash_cfi01.c | 34 ++++++++------
hw/block/pflash_cfi02.c | 30 ++++++++-----
softmmu/memory.c | 98 +++++++++++++++++++++++++++++++++++++++++
4 files changed, 224 insertions(+), 23 deletions(-)
--
2.26.2
^ permalink raw reply [flat|nested] 11+ messages in thread
* [RFC PATCH 1/3] exec/memory: Introduce memory_region_init_rom_device_from_file()
2021-02-25 23:02 [RFC PATCH 0/3] hw/block/pflash: Mmap read-only backend files with MAP_SHARED Philippe Mathieu-Daudé
@ 2021-02-25 23:02 ` Philippe Mathieu-Daudé
2021-03-01 11:53 ` Stefano Garzarella
2021-02-25 23:02 ` [RFC PATCH 2/3] hw/block/pflash: Move code around Philippe Mathieu-Daudé
2021-02-25 23:02 ` [RFC PATCH 3/3] hw/block/pflash: use memory_region_init_rom_device_from_file() Philippe Mathieu-Daudé
2 siblings, 1 reply; 11+ messages in thread
From: Philippe Mathieu-Daudé @ 2021-02-25 23:02 UTC (permalink / raw)
To: qemu-devel, David Edmondson
Cc: Kevin Wolf, qemu-block, Xu Yandong, Markus Armbruster, Max Reitz,
Zheng Xiang, haibinzhang, Stefan Hajnoczi, Paolo Bonzini,
Philippe Mathieu-Daudé,
Stefano Garzarella
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
^ permalink raw reply related [flat|nested] 11+ messages in thread
* [RFC PATCH 2/3] hw/block/pflash: Move code around
2021-02-25 23:02 [RFC PATCH 0/3] hw/block/pflash: Mmap read-only backend files with MAP_SHARED Philippe Mathieu-Daudé
2021-02-25 23:02 ` [RFC PATCH 1/3] exec/memory: Introduce memory_region_init_rom_device_from_file() Philippe Mathieu-Daudé
@ 2021-02-25 23:02 ` Philippe Mathieu-Daudé
2021-02-26 8:21 ` David Edmondson
2021-02-25 23:02 ` [RFC PATCH 3/3] hw/block/pflash: use memory_region_init_rom_device_from_file() Philippe Mathieu-Daudé
2 siblings, 1 reply; 11+ messages in thread
From: Philippe Mathieu-Daudé @ 2021-02-25 23:02 UTC (permalink / raw)
To: qemu-devel, David Edmondson
Cc: Kevin Wolf, qemu-block, Xu Yandong, Markus Armbruster, Max Reitz,
Zheng Xiang, haibinzhang, Stefan Hajnoczi, Paolo Bonzini,
Philippe Mathieu-Daudé,
Stefano Garzarella
First do the block checks, so we know if it is read-only or not.
Then create the MemoryRegion. This will allow optimization in
the next commit.
Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
---
hw/block/pflash_cfi01.c | 24 ++++++++++++------------
hw/block/pflash_cfi02.c | 18 +++++++++---------
2 files changed, 21 insertions(+), 21 deletions(-)
diff --git a/hw/block/pflash_cfi01.c b/hw/block/pflash_cfi01.c
index 22287a1522e..a5fa8d8b74a 100644
--- a/hw/block/pflash_cfi01.c
+++ b/hw/block/pflash_cfi01.c
@@ -731,18 +731,6 @@ static void pflash_cfi01_realize(DeviceState *dev, Error **errp)
}
device_len = sector_len_per_device * blocks_per_device;
- memory_region_init_rom_device(
- &pfl->mem, OBJECT(dev),
- &pflash_cfi01_ops,
- pfl,
- pfl->name, total_len, errp);
- if (*errp) {
- return;
- }
-
- pfl->storage = memory_region_get_ram_ptr(&pfl->mem);
- sysbus_init_mmio(SYS_BUS_DEVICE(dev), &pfl->mem);
-
if (pfl->blk) {
uint64_t perm;
pfl->ro = !blk_supports_write_perm(pfl->blk);
@@ -755,6 +743,18 @@ static void pflash_cfi01_realize(DeviceState *dev, Error **errp)
pfl->ro = 0;
}
+ memory_region_init_rom_device(
+ &pfl->mem, OBJECT(dev),
+ &pflash_cfi01_ops,
+ pfl,
+ pfl->name, total_len, errp);
+ if (*errp) {
+ return;
+ }
+
+ pfl->storage = memory_region_get_ram_ptr(&pfl->mem);
+ sysbus_init_mmio(SYS_BUS_DEVICE(dev), &pfl->mem);
+
if (pfl->blk) {
if (!blk_check_size_and_read_all(pfl->blk, pfl->storage, total_len,
errp)) {
diff --git a/hw/block/pflash_cfi02.c b/hw/block/pflash_cfi02.c
index 7962cff7455..4f62ce8917d 100644
--- a/hw/block/pflash_cfi02.c
+++ b/hw/block/pflash_cfi02.c
@@ -791,15 +791,6 @@ static void pflash_cfi02_realize(DeviceState *dev, Error **errp)
return;
}
- memory_region_init_rom_device(&pfl->orig_mem, OBJECT(pfl),
- &pflash_cfi02_ops, pfl, pfl->name,
- pfl->chip_len, errp);
- if (*errp) {
- return;
- }
-
- pfl->storage = memory_region_get_ram_ptr(&pfl->orig_mem);
-
if (pfl->blk) {
uint64_t perm;
pfl->ro = !blk_supports_write_perm(pfl->blk);
@@ -812,6 +803,15 @@ static void pflash_cfi02_realize(DeviceState *dev, Error **errp)
pfl->ro = 0;
}
+ memory_region_init_rom_device(&pfl->orig_mem, OBJECT(pfl),
+ &pflash_cfi02_ops, pfl, pfl->name,
+ pfl->chip_len, errp);
+ if (*errp) {
+ return;
+ }
+
+ pfl->storage = memory_region_get_ram_ptr(&pfl->orig_mem);
+
if (pfl->blk) {
if (!blk_check_size_and_read_all(pfl->blk, pfl->storage,
pfl->chip_len, errp)) {
--
2.26.2
^ permalink raw reply related [flat|nested] 11+ messages in thread
* [RFC PATCH 3/3] hw/block/pflash: use memory_region_init_rom_device_from_file()
2021-02-25 23:02 [RFC PATCH 0/3] hw/block/pflash: Mmap read-only backend files with MAP_SHARED Philippe Mathieu-Daudé
2021-02-25 23:02 ` [RFC PATCH 1/3] exec/memory: Introduce memory_region_init_rom_device_from_file() Philippe Mathieu-Daudé
2021-02-25 23:02 ` [RFC PATCH 2/3] hw/block/pflash: Move code around Philippe Mathieu-Daudé
@ 2021-02-25 23:02 ` Philippe Mathieu-Daudé
2021-02-26 8:23 ` David Edmondson
2 siblings, 1 reply; 11+ messages in thread
From: Philippe Mathieu-Daudé @ 2021-02-25 23:02 UTC (permalink / raw)
To: qemu-devel, David Edmondson
Cc: Kevin Wolf, qemu-block, Xu Yandong, Markus Armbruster, Max Reitz,
Zheng Xiang, haibinzhang, Stefan Hajnoczi, Paolo Bonzini,
Philippe Mathieu-Daudé,
Stefano Garzarella
If the block drive is read-only we will model a "protected" flash
device. We can thus use memory_region_init_rom_device_from_file()
which mmap the backing file when creating the MemoryRegion.
If the same backing file is used by multiple QEMU instances, this
reduces the memory footprint (this is often the case with the
CODE flash image from OVMF and AAVMF).
Suggested-by: Stefan Hajnoczi <stefanha@redhat.com>
Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
---
hw/block/pflash_cfi01.c | 20 ++++++++++++++------
hw/block/pflash_cfi02.c | 18 ++++++++++++++----
2 files changed, 28 insertions(+), 10 deletions(-)
diff --git a/hw/block/pflash_cfi01.c b/hw/block/pflash_cfi01.c
index a5fa8d8b74a..5757391df1c 100644
--- a/hw/block/pflash_cfi01.c
+++ b/hw/block/pflash_cfi01.c
@@ -743,11 +743,19 @@ static void pflash_cfi01_realize(DeviceState *dev, Error **errp)
pfl->ro = 0;
}
- memory_region_init_rom_device(
- &pfl->mem, OBJECT(dev),
- &pflash_cfi01_ops,
- pfl,
- pfl->name, total_len, errp);
+ if (pfl->blk && pfl->ro) {
+ memory_region_init_rom_device_from_file(&pfl->mem, OBJECT(dev),
+ &pflash_cfi01_ops, pfl,
+ pfl->name, total_len,
+ qemu_real_host_page_size,
+ RAM_SHARED,
+ blk_bs(pfl->blk)->filename,
+ true, errp);
+ } else {
+ memory_region_init_rom_device(&pfl->mem, OBJECT(dev),
+ &pflash_cfi01_ops, pfl,
+ pfl->name, total_len, errp);
+ }
if (*errp) {
return;
}
@@ -755,7 +763,7 @@ static void pflash_cfi01_realize(DeviceState *dev, Error **errp)
pfl->storage = memory_region_get_ram_ptr(&pfl->mem);
sysbus_init_mmio(SYS_BUS_DEVICE(dev), &pfl->mem);
- if (pfl->blk) {
+ if (pfl->blk && !pfl->ro) {
if (!blk_check_size_and_read_all(pfl->blk, pfl->storage, total_len,
errp)) {
vmstate_unregister_ram(&pfl->mem, DEVICE(pfl));
diff --git a/hw/block/pflash_cfi02.c b/hw/block/pflash_cfi02.c
index 4f62ce8917d..d57f64d7732 100644
--- a/hw/block/pflash_cfi02.c
+++ b/hw/block/pflash_cfi02.c
@@ -803,16 +803,26 @@ static void pflash_cfi02_realize(DeviceState *dev, Error **errp)
pfl->ro = 0;
}
- memory_region_init_rom_device(&pfl->orig_mem, OBJECT(pfl),
- &pflash_cfi02_ops, pfl, pfl->name,
- pfl->chip_len, errp);
+ if (pfl->blk && pfl->ro) {
+ memory_region_init_rom_device_from_file(&pfl->orig_mem, OBJECT(pfl),
+ &pflash_cfi02_ops, pfl,
+ pfl->name, pfl->chip_len,
+ qemu_real_host_page_size,
+ RAM_SHARED,
+ blk_bs(pfl->blk)->filename,
+ true, errp);
+ } else {
+ memory_region_init_rom_device(&pfl->orig_mem, OBJECT(pfl),
+ &pflash_cfi02_ops, pfl, pfl->name,
+ pfl->chip_len, errp);
+ }
if (*errp) {
return;
}
pfl->storage = memory_region_get_ram_ptr(&pfl->orig_mem);
- if (pfl->blk) {
+ if (pfl->blk && !pfl->ro) {
if (!blk_check_size_and_read_all(pfl->blk, pfl->storage,
pfl->chip_len, errp)) {
vmstate_unregister_ram(&pfl->orig_mem, DEVICE(pfl));
--
2.26.2
^ permalink raw reply related [flat|nested] 11+ messages in thread
* Re: [RFC PATCH 2/3] hw/block/pflash: Move code around
2021-02-25 23:02 ` [RFC PATCH 2/3] hw/block/pflash: Move code around Philippe Mathieu-Daudé
@ 2021-02-26 8:21 ` David Edmondson
0 siblings, 0 replies; 11+ messages in thread
From: David Edmondson @ 2021-02-26 8:21 UTC (permalink / raw)
To: Philippe Mathieu-Daudé, qemu-devel
Cc: Kevin Wolf, qemu-block, Xu Yandong, Markus Armbruster, Max Reitz,
Zheng Xiang, haibinzhang, Stefan Hajnoczi, Paolo Bonzini,
Philippe Mathieu-Daudé,
Stefano Garzarella
On Friday, 2021-02-26 at 00:02:37 +01, Philippe Mathieu-Daudé wrote:
> First do the block checks, so we know if it is read-only or not.
> Then create the MemoryRegion. This will allow optimization in
> the next commit.
>
> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
Reviewed-by: David Edmondson <david.edmondson@oracle.com>
> ---
> hw/block/pflash_cfi01.c | 24 ++++++++++++------------
> hw/block/pflash_cfi02.c | 18 +++++++++---------
> 2 files changed, 21 insertions(+), 21 deletions(-)
>
> diff --git a/hw/block/pflash_cfi01.c b/hw/block/pflash_cfi01.c
> index 22287a1522e..a5fa8d8b74a 100644
> --- a/hw/block/pflash_cfi01.c
> +++ b/hw/block/pflash_cfi01.c
> @@ -731,18 +731,6 @@ static void pflash_cfi01_realize(DeviceState *dev, Error **errp)
> }
> device_len = sector_len_per_device * blocks_per_device;
>
> - memory_region_init_rom_device(
> - &pfl->mem, OBJECT(dev),
> - &pflash_cfi01_ops,
> - pfl,
> - pfl->name, total_len, errp);
> - if (*errp) {
> - return;
> - }
> -
> - pfl->storage = memory_region_get_ram_ptr(&pfl->mem);
> - sysbus_init_mmio(SYS_BUS_DEVICE(dev), &pfl->mem);
> -
> if (pfl->blk) {
> uint64_t perm;
> pfl->ro = !blk_supports_write_perm(pfl->blk);
> @@ -755,6 +743,18 @@ static void pflash_cfi01_realize(DeviceState *dev, Error **errp)
> pfl->ro = 0;
> }
>
> + memory_region_init_rom_device(
> + &pfl->mem, OBJECT(dev),
> + &pflash_cfi01_ops,
> + pfl,
> + pfl->name, total_len, errp);
> + if (*errp) {
> + return;
> + }
> +
> + pfl->storage = memory_region_get_ram_ptr(&pfl->mem);
> + sysbus_init_mmio(SYS_BUS_DEVICE(dev), &pfl->mem);
> +
> if (pfl->blk) {
> if (!blk_check_size_and_read_all(pfl->blk, pfl->storage, total_len,
> errp)) {
> diff --git a/hw/block/pflash_cfi02.c b/hw/block/pflash_cfi02.c
> index 7962cff7455..4f62ce8917d 100644
> --- a/hw/block/pflash_cfi02.c
> +++ b/hw/block/pflash_cfi02.c
> @@ -791,15 +791,6 @@ static void pflash_cfi02_realize(DeviceState *dev, Error **errp)
> return;
> }
>
> - memory_region_init_rom_device(&pfl->orig_mem, OBJECT(pfl),
> - &pflash_cfi02_ops, pfl, pfl->name,
> - pfl->chip_len, errp);
> - if (*errp) {
> - return;
> - }
> -
> - pfl->storage = memory_region_get_ram_ptr(&pfl->orig_mem);
> -
> if (pfl->blk) {
> uint64_t perm;
> pfl->ro = !blk_supports_write_perm(pfl->blk);
> @@ -812,6 +803,15 @@ static void pflash_cfi02_realize(DeviceState *dev, Error **errp)
> pfl->ro = 0;
> }
>
> + memory_region_init_rom_device(&pfl->orig_mem, OBJECT(pfl),
> + &pflash_cfi02_ops, pfl, pfl->name,
> + pfl->chip_len, errp);
> + if (*errp) {
> + return;
> + }
> +
> + pfl->storage = memory_region_get_ram_ptr(&pfl->orig_mem);
> +
> if (pfl->blk) {
> if (!blk_check_size_and_read_all(pfl->blk, pfl->storage,
> pfl->chip_len, errp)) {
> --
> 2.26.2
dme.
--
She's as sweet as Tupelo honey, she's an angel of the first degree.
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [RFC PATCH 3/3] hw/block/pflash: use memory_region_init_rom_device_from_file()
2021-02-25 23:02 ` [RFC PATCH 3/3] hw/block/pflash: use memory_region_init_rom_device_from_file() Philippe Mathieu-Daudé
@ 2021-02-26 8:23 ` David Edmondson
2021-03-01 11:50 ` Philippe Mathieu-Daudé
0 siblings, 1 reply; 11+ messages in thread
From: David Edmondson @ 2021-02-26 8:23 UTC (permalink / raw)
To: Philippe Mathieu-Daudé, qemu-devel
Cc: Kevin Wolf, qemu-block, Xu Yandong, Markus Armbruster, Max Reitz,
Zheng Xiang, haibinzhang, Stefan Hajnoczi, Paolo Bonzini,
Philippe Mathieu-Daudé,
Stefano Garzarella
On Friday, 2021-02-26 at 00:02:38 +01, Philippe Mathieu-Daudé wrote:
> If the block drive is read-only we will model a "protected" flash
> device. We can thus use memory_region_init_rom_device_from_file()
> which mmap the backing file when creating the MemoryRegion.
> If the same backing file is used by multiple QEMU instances, this
> reduces the memory footprint (this is often the case with the
> CODE flash image from OVMF and AAVMF).
>
> Suggested-by: Stefan Hajnoczi <stefanha@redhat.com>
> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
> ---
> hw/block/pflash_cfi01.c | 20 ++++++++++++++------
> hw/block/pflash_cfi02.c | 18 ++++++++++++++----
> 2 files changed, 28 insertions(+), 10 deletions(-)
>
> diff --git a/hw/block/pflash_cfi01.c b/hw/block/pflash_cfi01.c
> index a5fa8d8b74a..5757391df1c 100644
> --- a/hw/block/pflash_cfi01.c
> +++ b/hw/block/pflash_cfi01.c
> @@ -743,11 +743,19 @@ static void pflash_cfi01_realize(DeviceState *dev, Error **errp)
> pfl->ro = 0;
> }
>
> - memory_region_init_rom_device(
> - &pfl->mem, OBJECT(dev),
> - &pflash_cfi01_ops,
> - pfl,
> - pfl->name, total_len, errp);
> + if (pfl->blk && pfl->ro) {
> + memory_region_init_rom_device_from_file(&pfl->mem, OBJECT(dev),
> + &pflash_cfi01_ops, pfl,
> + pfl->name, total_len,
> + qemu_real_host_page_size,
> + RAM_SHARED,
> + blk_bs(pfl->blk)->filename,
How will this behave if someone does:
-drive file=OVMF_CODE.fd.qcow2,index=0,if=pflash,format=qcow2,readonly=on
Honestly, I'm not sure why they would, but it works today.
> + true, errp);
> + } else {
> + memory_region_init_rom_device(&pfl->mem, OBJECT(dev),
> + &pflash_cfi01_ops, pfl,
> + pfl->name, total_len, errp);
> + }
> if (*errp) {
> return;
> }
> @@ -755,7 +763,7 @@ static void pflash_cfi01_realize(DeviceState *dev, Error **errp)
> pfl->storage = memory_region_get_ram_ptr(&pfl->mem);
> sysbus_init_mmio(SYS_BUS_DEVICE(dev), &pfl->mem);
>
> - if (pfl->blk) {
> + if (pfl->blk && !pfl->ro) {
> if (!blk_check_size_and_read_all(pfl->blk, pfl->storage, total_len,
> errp)) {
> vmstate_unregister_ram(&pfl->mem, DEVICE(pfl));
> diff --git a/hw/block/pflash_cfi02.c b/hw/block/pflash_cfi02.c
> index 4f62ce8917d..d57f64d7732 100644
> --- a/hw/block/pflash_cfi02.c
> +++ b/hw/block/pflash_cfi02.c
> @@ -803,16 +803,26 @@ static void pflash_cfi02_realize(DeviceState *dev, Error **errp)
> pfl->ro = 0;
> }
>
> - memory_region_init_rom_device(&pfl->orig_mem, OBJECT(pfl),
> - &pflash_cfi02_ops, pfl, pfl->name,
> - pfl->chip_len, errp);
> + if (pfl->blk && pfl->ro) {
> + memory_region_init_rom_device_from_file(&pfl->orig_mem, OBJECT(pfl),
> + &pflash_cfi02_ops, pfl,
> + pfl->name, pfl->chip_len,
> + qemu_real_host_page_size,
> + RAM_SHARED,
> + blk_bs(pfl->blk)->filename,
> + true, errp);
> + } else {
> + memory_region_init_rom_device(&pfl->orig_mem, OBJECT(pfl),
> + &pflash_cfi02_ops, pfl, pfl->name,
> + pfl->chip_len, errp);
> + }
> if (*errp) {
> return;
> }
>
> pfl->storage = memory_region_get_ram_ptr(&pfl->orig_mem);
>
> - if (pfl->blk) {
> + if (pfl->blk && !pfl->ro) {
> if (!blk_check_size_and_read_all(pfl->blk, pfl->storage,
> pfl->chip_len, errp)) {
> vmstate_unregister_ram(&pfl->orig_mem, DEVICE(pfl));
> --
> 2.26.2
dme.
--
And you're standing here beside me, I love the passing of time.
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [RFC PATCH 3/3] hw/block/pflash: use memory_region_init_rom_device_from_file()
2021-02-26 8:23 ` David Edmondson
@ 2021-03-01 11:50 ` Philippe Mathieu-Daudé
2021-03-01 13:38 ` David Edmondson
0 siblings, 1 reply; 11+ messages in thread
From: Philippe Mathieu-Daudé @ 2021-03-01 11:50 UTC (permalink / raw)
To: David Edmondson, qemu-devel
Cc: Kevin Wolf, qemu-block, Xu Yandong, Markus Armbruster, Max Reitz,
Zheng Xiang, haibinzhang, Stefan Hajnoczi, Paolo Bonzini,
Stefano Garzarella
On 2/26/21 9:23 AM, David Edmondson wrote:
> On Friday, 2021-02-26 at 00:02:38 +01, Philippe Mathieu-Daudé wrote:
>
>> If the block drive is read-only we will model a "protected" flash
>> device. We can thus use memory_region_init_rom_device_from_file()
>> which mmap the backing file when creating the MemoryRegion.
>> If the same backing file is used by multiple QEMU instances, this
>> reduces the memory footprint (this is often the case with the
>> CODE flash image from OVMF and AAVMF).
>>
>> Suggested-by: Stefan Hajnoczi <stefanha@redhat.com>
>> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
>> ---
>> hw/block/pflash_cfi01.c | 20 ++++++++++++++------
>> hw/block/pflash_cfi02.c | 18 ++++++++++++++----
>> 2 files changed, 28 insertions(+), 10 deletions(-)
>>
>> diff --git a/hw/block/pflash_cfi01.c b/hw/block/pflash_cfi01.c
>> index a5fa8d8b74a..5757391df1c 100644
>> --- a/hw/block/pflash_cfi01.c
>> +++ b/hw/block/pflash_cfi01.c
>> @@ -743,11 +743,19 @@ static void pflash_cfi01_realize(DeviceState *dev, Error **errp)
>> pfl->ro = 0;
>> }
>>
>> - memory_region_init_rom_device(
>> - &pfl->mem, OBJECT(dev),
>> - &pflash_cfi01_ops,
>> - pfl,
>> - pfl->name, total_len, errp);
>> + if (pfl->blk && pfl->ro) {
>> + memory_region_init_rom_device_from_file(&pfl->mem, OBJECT(dev),
>> + &pflash_cfi01_ops, pfl,
>> + pfl->name, total_len,
>> + qemu_real_host_page_size,
>> + RAM_SHARED,
>> + blk_bs(pfl->blk)->filename,
>
> How will this behave if someone does:
>
> -drive file=OVMF_CODE.fd.qcow2,index=0,if=pflash,format=qcow2,readonly=on
>
> Honestly, I'm not sure why they would, but it works today.
OK I can add a check for "raw" driver, but I don't know to check for
offset == 0.
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [RFC PATCH 1/3] exec/memory: Introduce memory_region_init_rom_device_from_file()
2021-02-25 23:02 ` [RFC PATCH 1/3] exec/memory: Introduce memory_region_init_rom_device_from_file() Philippe Mathieu-Daudé
@ 2021-03-01 11:53 ` Stefano Garzarella
2021-03-01 11:59 ` Philippe Mathieu-Daudé
0 siblings, 1 reply; 11+ messages in thread
From: Stefano Garzarella @ 2021-03-01 11:53 UTC (permalink / raw)
To: Philippe Mathieu-Daudé
Cc: Kevin Wolf, qemu-block, Xu Yandong, qemu-devel,
Markus Armbruster, David Edmondson, Zheng Xiang, haibinzhang,
Stefan Hajnoczi, Paolo Bonzini, Max Reitz
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
>
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [RFC PATCH 1/3] exec/memory: Introduce memory_region_init_rom_device_from_file()
2021-03-01 11:53 ` Stefano Garzarella
@ 2021-03-01 11:59 ` Philippe Mathieu-Daudé
0 siblings, 0 replies; 11+ messages in thread
From: Philippe Mathieu-Daudé @ 2021-03-01 11:59 UTC (permalink / raw)
To: Stefano Garzarella
Cc: Kevin Wolf, qemu-block, Xu Yandong, qemu-devel,
Markus Armbruster, David Edmondson, Zheng Xiang, haibinzhang,
Stefan Hajnoczi, Paolo Bonzini, Max Reitz
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
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [RFC PATCH 3/3] hw/block/pflash: use memory_region_init_rom_device_from_file()
2021-03-01 11:50 ` Philippe Mathieu-Daudé
@ 2021-03-01 13:38 ` David Edmondson
2021-03-01 13:58 ` Philippe Mathieu-Daudé
0 siblings, 1 reply; 11+ messages in thread
From: David Edmondson @ 2021-03-01 13:38 UTC (permalink / raw)
To: Philippe Mathieu-Daudé, qemu-devel
Cc: Kevin Wolf, qemu-block, Xu Yandong, Markus Armbruster, Max Reitz,
Zheng Xiang, haibinzhang, Stefan Hajnoczi, Paolo Bonzini,
Stefano Garzarella
On Monday, 2021-03-01 at 12:50:33 +01, Philippe Mathieu-Daudé wrote:
> On 2/26/21 9:23 AM, David Edmondson wrote:
>> On Friday, 2021-02-26 at 00:02:38 +01, Philippe Mathieu-Daudé wrote:
>>
>>> If the block drive is read-only we will model a "protected" flash
>>> device. We can thus use memory_region_init_rom_device_from_file()
>>> which mmap the backing file when creating the MemoryRegion.
>>> If the same backing file is used by multiple QEMU instances, this
>>> reduces the memory footprint (this is often the case with the
>>> CODE flash image from OVMF and AAVMF).
>>>
>>> Suggested-by: Stefan Hajnoczi <stefanha@redhat.com>
>>> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
>>> ---
>>> hw/block/pflash_cfi01.c | 20 ++++++++++++++------
>>> hw/block/pflash_cfi02.c | 18 ++++++++++++++----
>>> 2 files changed, 28 insertions(+), 10 deletions(-)
>>>
>>> diff --git a/hw/block/pflash_cfi01.c b/hw/block/pflash_cfi01.c
>>> index a5fa8d8b74a..5757391df1c 100644
>>> --- a/hw/block/pflash_cfi01.c
>>> +++ b/hw/block/pflash_cfi01.c
>>> @@ -743,11 +743,19 @@ static void pflash_cfi01_realize(DeviceState *dev, Error **errp)
>>> pfl->ro = 0;
>>> }
>>>
>>> - memory_region_init_rom_device(
>>> - &pfl->mem, OBJECT(dev),
>>> - &pflash_cfi01_ops,
>>> - pfl,
>>> - pfl->name, total_len, errp);
>>> + if (pfl->blk && pfl->ro) {
>>> + memory_region_init_rom_device_from_file(&pfl->mem, OBJECT(dev),
>>> + &pflash_cfi01_ops, pfl,
>>> + pfl->name, total_len,
>>> + qemu_real_host_page_size,
>>> + RAM_SHARED,
>>> + blk_bs(pfl->blk)->filename,
>>
>> How will this behave if someone does:
>>
>> -drive file=OVMF_CODE.fd.qcow2,index=0,if=pflash,format=qcow2,readonly=on
>>
>> Honestly, I'm not sure why they would, but it works today.
>
> OK I can add a check for "raw" driver, but I don't know to check for
> offset == 0.
This is pretty much where I got to when I tried using mmap() and gave up
(mostly because I figured that adding layer violating checks to the
pflash driver would not be well received, but also because we don't
share the same underlying file between multiple VMs and I wasn't sure
that it would eventually work well for writable devices).
dme.
--
Driving at 90 down those country lanes, singing to "Tiny Dancer".
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [RFC PATCH 3/3] hw/block/pflash: use memory_region_init_rom_device_from_file()
2021-03-01 13:38 ` David Edmondson
@ 2021-03-01 13:58 ` Philippe Mathieu-Daudé
0 siblings, 0 replies; 11+ messages in thread
From: Philippe Mathieu-Daudé @ 2021-03-01 13:58 UTC (permalink / raw)
To: David Edmondson, qemu-devel
Cc: Kevin Wolf, qemu-block, Xu Yandong, Markus Armbruster, Max Reitz,
Zheng Xiang, haibinzhang, Stefan Hajnoczi, Paolo Bonzini,
Stefano Garzarella
On 3/1/21 2:38 PM, David Edmondson wrote:
> On Monday, 2021-03-01 at 12:50:33 +01, Philippe Mathieu-Daudé wrote:
>
>> On 2/26/21 9:23 AM, David Edmondson wrote:
>>> On Friday, 2021-02-26 at 00:02:38 +01, Philippe Mathieu-Daudé wrote:
>>>
>>>> If the block drive is read-only we will model a "protected" flash
>>>> device. We can thus use memory_region_init_rom_device_from_file()
>>>> which mmap the backing file when creating the MemoryRegion.
>>>> If the same backing file is used by multiple QEMU instances, this
>>>> reduces the memory footprint (this is often the case with the
>>>> CODE flash image from OVMF and AAVMF).
>>>>
>>>> Suggested-by: Stefan Hajnoczi <stefanha@redhat.com>
>>>> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
>>>> ---
>>>> hw/block/pflash_cfi01.c | 20 ++++++++++++++------
>>>> hw/block/pflash_cfi02.c | 18 ++++++++++++++----
>>>> 2 files changed, 28 insertions(+), 10 deletions(-)
>>>>
>>>> diff --git a/hw/block/pflash_cfi01.c b/hw/block/pflash_cfi01.c
>>>> index a5fa8d8b74a..5757391df1c 100644
>>>> --- a/hw/block/pflash_cfi01.c
>>>> +++ b/hw/block/pflash_cfi01.c
>>>> @@ -743,11 +743,19 @@ static void pflash_cfi01_realize(DeviceState *dev, Error **errp)
>>>> pfl->ro = 0;
>>>> }
>>>>
>>>> - memory_region_init_rom_device(
>>>> - &pfl->mem, OBJECT(dev),
>>>> - &pflash_cfi01_ops,
>>>> - pfl,
>>>> - pfl->name, total_len, errp);
>>>> + if (pfl->blk && pfl->ro) {
>>>> + memory_region_init_rom_device_from_file(&pfl->mem, OBJECT(dev),
>>>> + &pflash_cfi01_ops, pfl,
>>>> + pfl->name, total_len,
>>>> + qemu_real_host_page_size,
>>>> + RAM_SHARED,
>>>> + blk_bs(pfl->blk)->filename,
>>>
>>> How will this behave if someone does:
>>>
>>> -drive file=OVMF_CODE.fd.qcow2,index=0,if=pflash,format=qcow2,readonly=on
>>>
>>> Honestly, I'm not sure why they would, but it works today.
>>
>> OK I can add a check for "raw" driver, but I don't know to check for
>> offset == 0.
>
> This is pretty much where I got to when I tried using mmap() and gave up
> (mostly because I figured that adding layer violating checks to the
> pflash driver would not be well received, but also because we don't
> share the same underlying file between multiple VMs and I wasn't sure
> that it would eventually work well for writable devices).
Kevin suggested on IRC (#qemu-block, you are welcome to join) to
introduce a new blk_*() interface to mmap an image (or possibly
part of it), and have it work with non-zero raw offsets.
^ permalink raw reply [flat|nested] 11+ messages in thread
end of thread, other threads:[~2021-03-01 14:08 UTC | newest]
Thread overview: 11+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-02-25 23:02 [RFC PATCH 0/3] hw/block/pflash: Mmap read-only backend files with MAP_SHARED Philippe Mathieu-Daudé
2021-02-25 23:02 ` [RFC PATCH 1/3] exec/memory: Introduce memory_region_init_rom_device_from_file() Philippe Mathieu-Daudé
2021-03-01 11:53 ` Stefano Garzarella
2021-03-01 11:59 ` Philippe Mathieu-Daudé
2021-02-25 23:02 ` [RFC PATCH 2/3] hw/block/pflash: Move code around Philippe Mathieu-Daudé
2021-02-26 8:21 ` David Edmondson
2021-02-25 23:02 ` [RFC PATCH 3/3] hw/block/pflash: use memory_region_init_rom_device_from_file() Philippe Mathieu-Daudé
2021-02-26 8:23 ` David Edmondson
2021-03-01 11:50 ` Philippe Mathieu-Daudé
2021-03-01 13:38 ` David Edmondson
2021-03-01 13:58 ` Philippe Mathieu-Daudé
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.