[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [RFC PATCH v2 3/3] hw/block/pflash: use memory_region_init_rom_devic
From: |
Philippe Mathieu-Daudé |
Subject: |
Re: [RFC PATCH v2 3/3] hw/block/pflash: use memory_region_init_rom_device_from_file() |
Date: |
Tue, 2 Mar 2021 08:04:46 +0100 |
User-agent: |
Mozilla/5.0 (X11; Linux x86_64; rv:78.0) Gecko/20100101 Thunderbird/78.7.0 |
On 3/1/21 7:13 PM, Stefan Hajnoczi wrote:
> 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.
OK.
> 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).
Yes, this is the limitation.
> 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.
Thanks :) Kevin suggested something similar too.
>
> FWIW this API isn't perfect because the file could be reopened with QMP
> and the existing mmap would remain in place.
Can you show me a QMP example or point me at the command?
This shouldn't happen with the pflash.
Thanks for reviewing,
Phil.