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

[Philippe Mathieu-Daudé]

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.