Re: [RFC PATCH 1/3] exec/memory: Introduce memory_region_init_rom_device_from_file()

[Philippe Mathieu-Daudé]

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é <philmd@redhat.com>
>> ---
>> 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