[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [RFC PATCH 1/3] exec/memory: Introduce memory_region_init_rom_device
From: |
Philippe Mathieu-Daudé |
Subject: |
Re: [RFC PATCH 1/3] exec/memory: Introduce memory_region_init_rom_device_from_file() |
Date: |
Mon, 1 Mar 2021 12:59:25 +0100 |
User-agent: |
Mozilla/5.0 (X11; Linux x86_64; rv:78.0) Gecko/20100101 Thunderbird/78.7.0 |
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