On 3/1/21 12:53 PM, Stefano Garzarella wrote:
> I don't know this code very well, but I have a couple of comments below :-)
>
> On Fri, Feb 26, 2021 at 12:02:36AM +0100, Philippe Mathieu-Daudé wrote:
>> Introduce memory_region_init_rom_device_from_file() which mmap
>> the backing file of ROM devices. This allows to reduce QEMU memory
>> footprint as the same file can be shared between multiple instances
>> of QEMU.
>>
>> Signed-off-by: Philippe Mathieu-Daudé <[email protected]>
>> ---
>> include/exec/memory.h | 85 +++++++++++++++++++++++++++++++++++++
>> softmmu/memory.c | 98 +++++++++++++++++++++++++++++++++++++++++++
>> 2 files changed, 183 insertions(+)
...
>> +void memory_region_init_rom_device_from_file_nomigrate(MemoryRegion *mr,
>> + Object *owner,
>> + const
>> MemoryRegionOps *ops,
>> + void *opaque,
>> + const char *name,
>> + uint64_t size,
>> + uint64_t align,
>> + uint32_t
>> ram_flags,
>> + const char *path,
>> + bool readonly,
>> + Error **errp)
>> +{
>> + Error *err = NULL;
>> +
>> + assert(ops);
>> +#ifdef CONFIG_POSIX
>> + memory_region_init(mr, owner, name, size);
>> + mr->opaque = opaque;
>> + mr->ops = ops;
>> + mr->rom_device = true;
>> + mr->readonly = readonly;
>> + mr->ram = true;
>> + mr->align = align;
>> + mr->terminates = true;
>> + mr->destructor = memory_region_destructor_ram;
>> + mr->ram_block = qemu_ram_alloc_from_file(size, mr, ram_flags, path,
>> + readonly, &err);
>> + if (err) {
>> + mr->size = int128_zero();
>> + object_unparent(OBJECT(mr));
>> + error_propagate(errp, err);
>> + }
>> +#else
>> + g_autoptr(GError) gerr = NULL;
>> + gsize len;
>> +
>> + memory_region_init(mr, owner, name, size);
>> + mr->ops = ops;
>> + mr->opaque = opaque;
>> + mr->terminates = true;
>> + mr->rom_device = true;
>
> Why when CONFIG_POSIX is defined we set 'mr->ram', 'mr->align', and
> 'mr->readonly = readonly' but not here?
> (I honestly don't know if they are important, I ask out of curiosity. :-)
I suppose we should and I forgot :/
>
>> +
>> + if (!g_file_get_contents(path, &mr->contents, &len, &gerr)) {
>
> Should we do these steps in case of an error?
>
> mr->size = int128_zero();
> object_unparent(OBJECT(mr));
Yes...
>
>> + error_setg(errp, "Unable to read '%s': %s", path,
>> gerr->message);
>> + return;
>> + }
>> + mr->destructor = memory_region_destructor_contents;
>> + mr->contents = g_realloc(mr->contents, size);
>> + mr->ram_block = qemu_ram_alloc_from_ptr(size, mr->contents, mr,
>> &err);
>> + if (err) {
>> + mr->size = int128_zero();
>> + object_unparent(OBJECT(mr));
>> + error_propagate(errp, err);
>> + }
>> +#endif
>
> Maybe I would reorganize the code inside ifdef like this:
>
> memory_region_init(mr, owner, name, size);
> mr->opaque = opaque;
> ...
> #ifdef CONFIG_POSIX
> mr->destructor = memory_region_destructor_ram;
> mr->ram_block = qemu_ram_alloc_from_file(...);
> #else
> if (!g_file_get_contents(..)) {
> ...
> }
> mr->destructor = memory_region_destructor_contents;
> mr->contents = g_realloc(mr->contents, size);
> mr->ram_block = qemu_ram_alloc_from_ptr(...)
> #endif
>
> if (err) {
> ...
> }
Yes, thanks :)
>
> I don't have a strong opinion, just an idea.
>
> Thanks,
> Stefano
