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

2021-03-01 Thread 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 
 Signed-off-by: Philippe Mathieu-Daudé 
 ---
  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(
 ->mem, OBJECT(dev),
 -_cfi01_ops,
 -pfl,
 -pfl->name, total_len, errp);
 +if (pfl->blk && pfl->ro) {
 +memory_region_init_rom_device_from_file(>mem, OBJECT(dev),
 +_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.

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

2021-03-01 Thread David Edmondson 
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 
>>> Signed-off-by: Philippe Mathieu-Daudé 
>>> ---
>>>  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(
>>> ->mem, OBJECT(dev),
>>> -_cfi01_ops,
>>> -pfl,
>>> -pfl->name, total_len, errp);
>>> +if (pfl->blk && pfl->ro) {
>>> +memory_region_init_rom_device_from_file(>mem, OBJECT(dev),
>>> +_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".

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

2021-03-01 Thread Philippe Mathieu-Daudé 
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 
>> Signed-off-by: Philippe Mathieu-Daudé 
>> ---
>>  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(
>> ->mem, OBJECT(dev),
>> -_cfi01_ops,
>> -pfl,
>> -pfl->name, total_len, errp);
>> +if (pfl->blk && pfl->ro) {
>> +memory_region_init_rom_device_from_file(>mem, OBJECT(dev),
>> +_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.

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

2021-02-26 Thread David Edmondson 
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 
> Signed-off-by: Philippe Mathieu-Daudé 
> ---
>  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(
> ->mem, OBJECT(dev),
> -_cfi01_ops,
> -pfl,
> -pfl->name, total_len, errp);
> +if (pfl->blk && pfl->ro) {
> +memory_region_init_rom_device_from_file(>mem, OBJECT(dev),
> +_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(>mem, OBJECT(dev),
> +  _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(>mem);
>  sysbus_init_mmio(SYS_BUS_DEVICE(dev), >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(>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(>orig_mem, OBJECT(pfl),
> -  _cfi02_ops, pfl, pfl->name,
> -  pfl->chip_len, errp);
> +if (pfl->blk && pfl->ro) {
> +memory_region_init_rom_device_from_file(>orig_mem, OBJECT(pfl),
> +_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(>orig_mem, OBJECT(pfl),
> +  _cfi02_ops, pfl, pfl->name,
> +  pfl->chip_len, errp);
> +}
>  if (*errp) {
>  return;
>  }
>  
>  pfl->storage = memory_region_get_ram_ptr(>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(>orig_mem, DEVICE(pfl));
> -- 
> 2.26.2

dme.
-- 
And you're standing here beside me, I love the passing of time.

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

2021-02-25 Thread Philippe Mathieu-Daudé 
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 
Signed-off-by: Philippe Mathieu-Daudé 
---
 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(
->mem, OBJECT(dev),
-_cfi01_ops,
-pfl,
-pfl->name, total_len, errp);
+if (pfl->blk && pfl->ro) {
+memory_region_init_rom_device_from_file(>mem, OBJECT(dev),
+_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(>mem, OBJECT(dev),
+  _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(>mem);
 sysbus_init_mmio(SYS_BUS_DEVICE(dev), >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(>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(>orig_mem, OBJECT(pfl),
-  _cfi02_ops, pfl, pfl->name,
-  pfl->chip_len, errp);
+if (pfl->blk && pfl->ro) {
+memory_region_init_rom_device_from_file(>orig_mem, OBJECT(pfl),
+_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(>orig_mem, OBJECT(pfl),
+  _cfi02_ops, pfl, pfl->name,
+  pfl->chip_len, errp);
+}
 if (*errp) {
 return;
 }
 
 pfl->storage = memory_region_get_ram_ptr(>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(>orig_mem, DEVICE(pfl));
-- 
2.26.2