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 <[email protected]> >> Signed-off-by: Philippe Mathieu-Daudé <[email protected]> >> --- >> 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.
