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

[Stefano Garzarella]

On Tue, Jul 23, 2019 at 10:32:34AM +0100, Peter Maydell wrote:
> 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.
> 

Reading the 'g_mapped_file_new_from_fd()' docs [1]:
"If writable is TRUE, the mapped buffer may be modified, otherwise it is an
error to modify the mapped buffer. Modifications to the buffer are not visible
to other processes mapping the same file, and are not written back to the file."

I don't know what "error" means, but reading the second part I thought
the changes in that case were only visible at the current process.

I'll test it to understand better the behavior. If we can't touch it, then we
have to make a copy in these cases.

> (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.)

Yes, I agree.


Thanks,
Stefano

[1] 
https://developer.gnome.org/glib/stable/glib-File-Utilities.html#g-mapped-file-new-from-fd