Re: [RFC PATCH v2 3/3] hw/block/pflash: use memory_region_init_rom_device_from_file()

[Stefan Hajnoczi]

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 <stefanha@redhat.com>
> Signed-off-by: Philippe Mathieu-Daudé <philmd@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