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

[David Edmondson <dme@dme.org>]

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.