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é <phi...@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