| Message ID | 20210225230238.3719051-4-philmd@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | hw/block/pflash: Mmap read-only backend files with MAP_SHARED | expand |
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.
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.
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.
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.
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, + 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));
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(-)