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

[David Edmondson <dme@dme.org>]

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".