| Message ID | 20190723140445.12748-2-sgarzare@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | pc: mmap kernel (ELF image) and initrd | expand |
On 23/07/19 16:04, Stefano Garzarella wrote: > + /* Increments the reference count to avoid the unmap */ > + g_mapped_file_ref(gmf); > /* rom_add_elf_program() seize the ownership of 'data' */ > rom_add_elf_program(label, data, file_size, mem_size, > addr, as); I'm a bit worried about rom_reset g_free'ing rom->data, which goes against the comment on top of rom_free: /* rom->data must be heap-allocated (do not use with rom_add_elf_program()) */ Since this is the only call to rom_add_elf_program, what about adding a GMappedFile* field to struct Rom and passing it here instead of data+file_size? Then the g_mapped_file_ref can be in rom_add_elf_program, and you can have a nice static void rom_free_data(Rom *rom) { if (rom->mapped_file) { g_mapped_file_unref(rom->mapped_file); rom->mapped_file = NULL; } else { g_free(rom->data); } rom->data = NULL; } that is called from both rom_free and rom_reset. Thanks, Paolo > @@ -531,7 +540,6 @@ static int glue(load_elf, SZ)(const char *name, int fd, > address_space_write(as ? as : &address_space_memory, > addr, MEMTXATTRS_UNSPECIFIED, > data, file_size); > - g_free(data); > } > } > > @@ -547,16 +555,15 @@ static int glue(load_elf, SZ)(const char *name, int fd, > struct elf_note *nhdr = NULL; > > file_size = ph->p_filesz; /* Size of the range of ELF notes */ > - data = g_malloc0(file_size); > - if (ph->p_filesz > 0) { > - if (lseek(fd, ph->p_offset, SEEK_SET) < 0) { > - goto fail; > - } > - if (read(fd, data, file_size) != file_size) { > - goto fail; > - } > + data_offset = ph->p_offset; /* Offset where the notes are located */ > + > + if (g_mapped_file_get_length(gmf) < file_size + data_offset) { > + goto fail; > } > > + data = (uint8_t *)g_mapped_file_get_contents(gmf); > + data += data_offset; > + > /* > * Search the ELF notes to find one with a type matching the > * value passed in via 'translate_opaque' > @@ -570,7 +577,6 @@ static int glue(load_elf, SZ)(const char *name, int fd, > sizeof(struct elf_note) == sizeof(struct elf64_note); > elf_note_fn((void *)nhdr, (void *)&ph->p_align, is64); > } > - g_free(data); > data = NULL; > } > } > @@ -582,7 +588,7 @@ static int glue(load_elf, SZ)(const char *name, int fd, > *highaddr = (uint64_t)(elf_sword)high; > return total_size; > fail: > - g_free(data); > + g_mapped_file_unref(gmf); > g_free(phdr); > return ret; > } >
On Tue, Jul 23, 2019 at 04:33:44PM +0200, Paolo Bonzini wrote: > On 23/07/19 16:04, Stefano Garzarella wrote: > > + /* Increments the reference count to avoid the unmap */ > > + g_mapped_file_ref(gmf); > > /* rom_add_elf_program() seize the ownership of 'data' */ > > rom_add_elf_program(label, data, file_size, mem_size, > > addr, as); > > I'm a bit worried about rom_reset g_free'ing rom->data, which goes > against the comment on top of rom_free: > > /* rom->data must be heap-allocated (do not use with > rom_add_elf_program()) */ Thanks for pointing it out. We definitely have to avoid that free in this case. > > > Since this is the only call to rom_add_elf_program, what about adding a > GMappedFile* field to struct Rom and passing it here instead of > data+file_size? Ehm, we currently use a subsection of the mmapped file as a ROM. Should we keep the data+file_size parameter? (plus the new GMappedFile*) At this point, is it better to call 'g_mapped_file_ref(gmf)' in the 'rom_add_elf_program()'? > > Then the g_mapped_file_ref can be in rom_add_elf_program, and you can > have a nice > > static void rom_free_data(Rom *rom) > { > if (rom->mapped_file) { > g_mapped_file_unref(rom->mapped_file); > rom->mapped_file = NULL; > } else { > g_free(rom->data); > } > rom->data = NULL; > } > > that is called from both rom_free and rom_reset. I'll add this in v3. Thanks, Stefano
On 23/07/19 16:57, Stefano Garzarella wrote: > On Tue, Jul 23, 2019 at 04:33:44PM +0200, Paolo Bonzini wrote: >> Since this is the only call to rom_add_elf_program, what about adding a >> GMappedFile* field to struct Rom and passing it here instead of >> data+file_size? > > Ehm, we currently use a subsection of the mmapped file as a ROM. > Should we keep the data+file_size parameter? (plus the new GMappedFile*) Yes, either that or an offset/file_size. > At this point, is it better to call 'g_mapped_file_ref(gmf)' in the > 'rom_add_elf_program()'? Yes, of course. Paolo
diff --git a/include/hw/elf_ops.h b/include/hw/elf_ops.h index 690f9238c8..d80e7ad20c 100644 --- a/include/hw/elf_ops.h +++ b/include/hw/elf_ops.h @@ -323,8 +323,9 @@ static int glue(load_elf, SZ)(const char *name, int fd, struct elfhdr ehdr; struct elf_phdr *phdr = NULL, *ph; int size, i, total_size; - elf_word mem_size, file_size; + elf_word mem_size, file_size, data_offset; uint64_t addr, low = (uint64_t)-1, high = 0; + GMappedFile *gmf = NULL; uint8_t *data = NULL; char label[128]; int ret = ELF_LOAD_FAILED; @@ -409,22 +410,31 @@ static int glue(load_elf, SZ)(const char *name, int fd, } } + /* + * Since we want to be able to modify the mapped buffer, we set the + * 'writeble' parameter to 'true'. Modifications to the buffer are not + * written back to the file. + */ + gmf = g_mapped_file_new_from_fd(fd, true, NULL); + if (!gmf) { + goto fail; + } + total_size = 0; for(i = 0; i < ehdr.e_phnum; i++) { ph = &phdr[i]; if (ph->p_type == PT_LOAD) { mem_size = ph->p_memsz; /* Size of the ROM */ file_size = ph->p_filesz; /* Size of the allocated data */ - data = g_malloc0(file_size); - if (ph->p_filesz > 0) { - if (lseek(fd, ph->p_offset, SEEK_SET) < 0) { - goto fail; - } - if (read(fd, data, file_size) != file_size) { - goto fail; - } + data_offset = ph->p_offset; /* Offset where the data is located */ + + if (g_mapped_file_get_length(gmf) < file_size + data_offset) { + goto fail; } + data = (uint8_t *)g_mapped_file_get_contents(gmf); + data += data_offset; + /* The ELF spec is somewhat vague about the purpose of the * physical address field. One common use in the embedded world * is that physical address field specifies the load address @@ -513,17 +523,16 @@ static int glue(load_elf, SZ)(const char *name, int fd, *pentry = ehdr.e_entry - ph->p_vaddr + ph->p_paddr; } - if (mem_size == 0) { - /* Some ELF files really do have segments of zero size; - * just ignore them rather than trying to create empty - * ROM blobs, because the zero-length blob can falsely - * trigger the overlapping-ROM-blobs check. - */ - g_free(data); - } else { + /* Some ELF files really do have segments of zero size; + * just ignore them rather than trying to create empty + * ROM blobs, because the zero-length blob can falsely + * trigger the overlapping-ROM-blobs check. + */ + if (mem_size != 0) { if (load_rom) { snprintf(label, sizeof(label), "phdr #%d: %s", i, name); - + /* Increments the reference count to avoid the unmap */ + g_mapped_file_ref(gmf); /* rom_add_elf_program() seize the ownership of 'data' */ rom_add_elf_program(label, data, file_size, mem_size, addr, as); @@ -531,7 +540,6 @@ static int glue(load_elf, SZ)(const char *name, int fd, address_space_write(as ? as : &address_space_memory, addr, MEMTXATTRS_UNSPECIFIED, data, file_size); - g_free(data); } } @@ -547,16 +555,15 @@ static int glue(load_elf, SZ)(const char *name, int fd, struct elf_note *nhdr = NULL; file_size = ph->p_filesz; /* Size of the range of ELF notes */ - data = g_malloc0(file_size); - if (ph->p_filesz > 0) { - if (lseek(fd, ph->p_offset, SEEK_SET) < 0) { - goto fail; - } - if (read(fd, data, file_size) != file_size) { - goto fail; - } + data_offset = ph->p_offset; /* Offset where the notes are located */ + + if (g_mapped_file_get_length(gmf) < file_size + data_offset) { + goto fail; } + data = (uint8_t *)g_mapped_file_get_contents(gmf); + data += data_offset; + /* * Search the ELF notes to find one with a type matching the * value passed in via 'translate_opaque' @@ -570,7 +577,6 @@ static int glue(load_elf, SZ)(const char *name, int fd, sizeof(struct elf_note) == sizeof(struct elf64_note); elf_note_fn((void *)nhdr, (void *)&ph->p_align, is64); } - g_free(data); data = NULL; } } @@ -582,7 +588,7 @@ static int glue(load_elf, SZ)(const char *name, int fd, *highaddr = (uint64_t)(elf_sword)high; return total_size; fail: - g_free(data); + g_mapped_file_unref(gmf); g_free(phdr); return ret; }
In order to reduce the memory footprint we map into memory the ELF to load using g_mapped_file_new_from_fd() instead of reading each sections. In this way we can share the ELF pages between multiple instances of QEMU. Suggested-by: Dr. David Alan Gilbert <dgilbert@redhat.com> Suggested-by: Paolo Bonzini <pbonzini@redhat.com> Signed-off-by: Stefano Garzarella <sgarzare@redhat.com> --- v2: - used g_mapped_file_new_from_fd() with 'writeble' set to 'true', since we can modify the mapped buffer. [Paolo, Peter] --- include/hw/elf_ops.h | 64 ++++++++++++++++++++++++-------------------- 1 file changed, 35 insertions(+), 29 deletions(-)