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(-)
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;
}
--
2.20.1
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
© 2016 - 2026 Red Hat, Inc.