Re: [Qemu-devel] [PATCH 1/2] elf-ops.h: Map into memory the ELF to load

[Peter Maydell]

On Tue, 23 Jul 2019 at 10:08, Stefano Garzarella <address@hidden> wrote:
>
> 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 <address@hidden>
> Suggested-by: Paolo Bonzini <address@hidden>
> Signed-off-by: Stefano Garzarella <address@hidden>
> ---
>  include/hw/elf_ops.h | 59 ++++++++++++++++++++++----------------------
>  1 file changed, 30 insertions(+), 29 deletions(-)
>
> diff --git a/include/hw/elf_ops.h b/include/hw/elf_ops.h
> index 690f9238c8..69ce8dea74 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,26 @@ static int glue(load_elf, SZ)(const char *name, int fd,
>          }
>      }
>
> +    gmf = g_mapped_file_new_from_fd(fd, false, NULL);

Hmm. Here we pass 'false' for the writable argument,
meaning we promise not to modify the mapped buffer...

> +    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;

...but here we set up the 'data' pointer from the mapped contents,
and then in following code we will write to it in some situations --
look at the "if (data_swab)" case or the call to elf_reloc if we
have a translate_fn, for instance.

(We can't get out of this by just passing writable=true, because
we definitely don't want to be writing back to the underlying file.)

thanks
-- PMM