On Mon, Mar 01, 2021 at 12:53:29PM +0100, 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 <stefa...@redhat.com>
> Signed-off-by: Philippe Mathieu-Daudé <phi...@redhat.com>
> ---
> hw/block/pflash_cfi01.c | 39 +++++++++++++++++++++++++++++++--------
> 1 file changed, 31 insertions(+), 8 deletions(-)
>
> diff --git a/hw/block/pflash_cfi01.c b/hw/block/pflash_cfi01.c
> index a5fa8d8b74a..ec290636298 100644
> --- a/hw/block/pflash_cfi01.c
> +++ b/hw/block/pflash_cfi01.c
> @@ -702,6 +702,7 @@ static void pflash_cfi01_realize(DeviceState *dev, Error
> **errp)
> int ret;
> uint64_t blocks_per_device, sector_len_per_device, device_len;
> int num_devices;
> + bool romd_mr_shared_mapped;
>
> if (pfl->sector_len == 0) {
> error_setg(errp, "attribute \"sector-length\" not specified or
> zero.");
> @@ -743,19 +744,41 @@ 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;
> + if (pfl->ro && pfl->blk) {
> + BlockDriverState *bs = blk_bs(pfl->blk);
> +
> + /* If "raw" driver used, try to mmap the backing file as RAM_SHARED
> */
> + if (bs->drv == &bdrv_raw) { /* FIXME check offset=0 ? */
Bypassing the block layer is tricky because there are a lot of features
that conflict (you already pointed out the offset= option). Checking
bdrv_raw is not enough because the underlying protocol driver could be
GlusterFS, iSCSI, etc.
I think the goal here is to avoid changing the command-line/QMP so that
users don't need to modify their guests. Therefore changing the pflash
qdev properties is not desirable (we could have added a separate code
path that bypasses the block layer cleanly). This seems like a
worthwhile optimization that the block layer should support. I suggest
adding a new API like:
/* Returns a filename string if @blk supports read-only mmap */
char *blk_get_read_only_mmap_filename(BlockBackend *blk, Error **errp);
Then block/raw-format.c would forward the call to bs->file and
block/raw-posix.c would implement it by returning a new filename string
when bs->read_only is true.
FWIW this API isn't perfect because the file could be reopened with QMP
and the existing mmap would remain in place.
> + Error *local_err = NULL;
> +
> + 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,
> + bs->exact_filename,
> + true, &local_err);
> + if (local_err) {
> + error_report_err(local_err);
> + /* fall back to memory_region_init_rom_device() */
> + } else {
> + romd_mr_shared_mapped = true;
> + }
> + }
> + }
> + if (!romd_mr_shared_mapped) {
> + 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 (pfl->blk && !romd_mr_shared_mapped) {
> if (!blk_check_size_and_read_all(pfl->blk, pfl->storage, total_len,
> errp)) {
> vmstate_unregister_ram(&pfl->mem, DEVICE(pfl));
> --
> 2.26.2
>
signature.asc
Description: PGP signature
