On 04/03/19 16:12, Xiang Zheng wrote:
> Hi Laszlo and Markus,
>
> Thanks for your useful suggestions and comments! :)
>
> On 2019/3/27 2:36, Markus Armbruster wrote:
>> Laszlo Ersek <ler...@redhat.com> writes:
>>
>>> On 03/26/19 17:39, Markus Armbruster wrote:
>>>> Laszlo Ersek <ler...@redhat.com> writes:
>>>
>>>>> With the dynamic sizing in QEMU (which, IIRC, I had originally
>>>>> introduced still in the 1MB times, due to the split between the
>>>>> executable and varstore parts), both the 1MB->2MB switch, and the
>>>>> 2MB->4MB switch in the firmware caused zero pain in QEMU. And right now,
>>>>> 4MB looks like a "sweet spot", with some elbow room left.
>>>>
>>>> Explicit configuration would've been exactly as painless. Even with
>>>> pflash sizes restricted to powers of two.
>>>
>>> I wrote the patch that ended up as commit 637a5acb46b3 -- with your R-b
>>> -- in 2013. I'm unsure if machine type properties existed back then, but
>>> even if they did, do you think I knew about them? :)
>>>
>>> You are right, of course; it's just that we can't tell the future.
>>
>> True! All we can do is continue to design as well as we can given the
>> information, experience and resources we have, and when the inevitable
>> design mistakes become apparent, limit their impact.
>>
>> Some of the things we now consider mistakes we just didn't see. Others
>> we saw (e.g. multiple pflash devices, unlike physical hardware), but
>> underestimated their impact.
>>
>
> I thought about your comments and wrote the following patch (just for test)
> which uses a file mapping to replace the anonymous mapping. UEFI seems to work
> fine. So why not use a file mapping to read or write a pflash device?
Honestly I can't answer "why not do this". Maybe we should.
Regarding "why not do this *exactly as shown below*" -- probably because
then we'd have updates to the same underlying regular file via both
mmap() and write(), and the interactions between those are really tricky
(= best avoided).
One of my favorite questions over the years used to be posting a matrix
of possible mmap() and file descriptor r/w/sync interactions, with the
outcomes as they were "implied" by POSIX, and then asking at the bottom,
"is my understanding correct?" I've never ever received an answer to
that. :)
Also... although we don't really use them in practice, some QCOW2
features for pflash block backends are -- or would be -- nice. (Not the
internal snapshots or the live RAM dumps, of course.)
Thanks
Laszlo
> Except this way, I don't know how to share the pflash memory among VMs or
> reduce the consumption for resource. :(
>
> ---8>---
>
> diff --git a/hw/arm/virt.c b/hw/arm/virt.c
> index ce2664a..12c78f2 100644
> --- a/hw/arm/virt.c
> +++ b/hw/arm/virt.c
> @@ -898,6 +898,7 @@ static void create_one_flash(const char *name, hwaddr
> flashbase,
> qdev_prop_set_uint16(dev, "id2", 0x00);
> qdev_prop_set_uint16(dev, "id3", 0x00);
> qdev_prop_set_string(dev, "name", name);
> + qdev_prop_set_bit(dev, "prealloc", false);
> qdev_init_nofail(dev);
>
> memory_region_add_subregion(sysmem, flashbase,
> diff --git a/hw/block/pflash_cfi01.c b/hw/block/pflash_cfi01.c
> index 16dfae1..23a85bc 100644
> --- a/hw/block/pflash_cfi01.c
> +++ b/hw/block/pflash_cfi01.c
> @@ -92,6 +92,7 @@ struct PFlashCFI01 {
> void *storage;
> VMChangeStateEntry *vmstate;
> bool old_multiple_chip_handling;
> + bool prealloc;
> };
>
> static int pflash_post_load(void *opaque, int version_id);
> @@ -731,11 +732,21 @@ static void pflash_cfi01_realize(DeviceState *dev,
> Error **errp)
> }
> device_len = sector_len_per_device * blocks_per_device;
>
> - memory_region_init_rom_device(
> - &pfl->mem, OBJECT(dev),
> - &pflash_cfi01_ops,
> - pfl,
> - pfl->name, total_len, &local_err);
> + if (pfl->blk && !pfl->prealloc) {
> + memory_region_init_rom_device_from_file(
> + &pfl->mem, OBJECT(dev),
> + &pflash_cfi01_ops,
> + pfl,
> + pfl->name, total_len,
> + blk_is_read_only(pfl->blk) ? RAM_SHARED : RAM_PMEM,
> + blk_bs(pfl->blk)->filename, &local_err);
> + } else {
> + memory_region_init_rom_device(
> + &pfl->mem, OBJECT(dev),
> + &pflash_cfi01_ops,
> + pfl,
> + pfl->name, total_len, &local_err);
> + }
> if (local_err) {
> error_propagate(errp, local_err);
> return;
> @@ -899,6 +910,7 @@ static Property pflash_cfi01_properties[] = {
> DEFINE_PROP_STRING("name", PFlashCFI01, name),
> DEFINE_PROP_BOOL("old-multiple-chip-handling", PFlashCFI01,
> old_multiple_chip_handling, false),
> + DEFINE_PROP_BOOL("prealloc", PFlashCFI01, prealloc, true),
> DEFINE_PROP_END_OF_LIST(),
> };
>
> diff --git a/include/exec/memory.h b/include/exec/memory.h
> index 1625913..1b16d3b 100644
> --- a/include/exec/memory.h
> +++ b/include/exec/memory.h
> @@ -804,6 +804,16 @@ void
> memory_region_init_rom_device_nomigrate(MemoryRegion *mr,
> uint64_t size,
> Error **errp);
>
> +void memory_region_init_rom_device_from_file(MemoryRegion *mr,
> + struct Object *owner,
> + const MemoryRegionOps *ops,
> + void *opaque,
> + const char *name,
> + uint64_t size,
> + uint32_t ram_flags,
> + const char *path,
> + Error **errp);
> +
> /**
> * memory_region_init_iommu: Initialize a memory region of a custom type
> * that translates addresses
> diff --git a/memory.c b/memory.c
> index 9fbca52..905956b 100644
> --- a/memory.c
> +++ b/memory.c
> @@ -1719,6 +1719,36 @@ void
> memory_region_init_rom_device_nomigrate(MemoryRegion *mr,
> }
> }
>
> +void memory_region_init_rom_device_from_file(MemoryRegion *mr,
> + Object *owner,
> + const MemoryRegionOps *ops,
> + void *opaque,
> + const char *name,
> + uint64_t size,
> + uint32_t ram_flags,
> + const char *path,
> + Error **errp)
> +{
> + DeviceState *owner_dev;
> + Error *err = NULL;
> + assert(ops);
> + memory_region_init(mr, owner, name, size);
> + mr->ops = ops;
> + mr->opaque = opaque;
> + mr->terminates = true;
> + mr->rom_device = true;
> + mr->destructor = memory_region_destructor_ram;
> + mr->ram_block = qemu_ram_alloc_from_file(size, mr, ram_flags, path,
> &err);
> + if (err) {
> + mr->size = int128_zero();
> + object_unparent(OBJECT(mr));
> + error_propagate(errp, err);
> + return ;
> + }
> + owner_dev = DEVICE(owner);
> + vmstate_register_ram(mr, owner_dev);
> +}
> +
> void memory_region_init_iommu(void *_iommu_mr,
> size_t instance_size,
> const char *mrtypename,
>
