diff mbox series

[v3,2/3] elf-ops.h: Map into memory the ELF to load

Message ID 20190724112531.232260-3-sgarzare@redhat.com
State New
Headers show
Series pc: mmap kernel (ELF image) and initrd | expand

Commit Message

Stefano Garzarella July 24, 2019, 11:25 a.m. UTC
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>
---
v3:
  - renamed 'GMappedFile *gmf' in 'GMappedFile *mapped_filed' for readability.
  - passed the GMappedFile* to rom_add_elf_program() to correctly handle the
    reference count. [Paolo]
  - set 'data' pointer only if 'file_size > 0' as the original behaviour
    [check-qtest-ppc64 fails without it]
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 | 68 ++++++++++++++++++++++++++------------------
 1 file changed, 41 insertions(+), 27 deletions(-)

Comments

Paolo Bonzini July 24, 2019, 11:50 a.m. UTC | #1
On 24/07/19 13:25, Stefano Garzarella wrote:
> @@ -582,7 +596,7 @@ static int glue(load_elf, SZ)(const char *name, int fd,
>          *highaddr = (uint64_t)(elf_sword)high;
>      return total_size;

Isn't the success case missing a g_mapped_file_unref?  It has to be done
unconditionally since now rom_add_elf_program adds a separate reference.

Related to this, the comment

            /* rom_add_elf_program() seize the ownership of 'data' */

refers to the g_free(data) that you are removing and is best changed to just
            /*
             * rom_add_elf_program() takes its own reference to
             * mapped_file.
             */

Thanks,

Paolo

>   fail:
> -    g_free(data);
> +    g_mapped_file_unref(mapped_file);
>      g_free(phdr);
>      return ret;
>  }
>
Stefano Garzarella July 24, 2019, 12:35 p.m. UTC | #2
On Wed, Jul 24, 2019 at 01:50:58PM +0200, Paolo Bonzini wrote:
> On 24/07/19 13:25, Stefano Garzarella wrote:
> > @@ -582,7 +596,7 @@ static int glue(load_elf, SZ)(const char *name, int fd,
> >          *highaddr = (uint64_t)(elf_sword)high;
> >      return total_size;
> 
> Isn't the success case missing a g_mapped_file_unref?  It has to be done
> unconditionally since now rom_add_elf_program adds a separate reference.

Sure, I had this in mind, since I initialized mapped_file to NULL, but
I didn't see the return before "fail:" label!
Maybe I'll change the end of load_elf() in this way:

-    g_free(phdr);
     if (lowaddr)
         *lowaddr = (uint64_t)(elf_sword)low;
     if (highaddr)
         *highaddr = (uint64_t)(elf_sword)high;
-    return total_size;
+    ret = total_size;
  fail:
-    g_free(data);
+    g_mapped_file_unref(mapped_file);
     g_free(phdr);
     return ret;
 }


> 
> Related to this, the comment
> 
>             /* rom_add_elf_program() seize the ownership of 'data' */
> 
> refers to the g_free(data) that you are removing and is best changed to just
>             /*
>              * rom_add_elf_program() takes its own reference to
>              * mapped_file.
>              */

I'll update the comment.

Thanks,
Stefano
Paolo Bonzini July 24, 2019, 1:07 p.m. UTC | #3
On 24/07/19 14:35, Stefano Garzarella wrote:
>> Isn't the success case missing a g_mapped_file_unref?  It has to be done
>> unconditionally since now rom_add_elf_program adds a separate reference.
> Sure, I had this in mind, since I initialized mapped_file to NULL, but
> I didn't see the return before "fail:" label!
> Maybe I'll change the end of load_elf() in this way:
> 
> -    g_free(phdr);
>      if (lowaddr)
>          *lowaddr = (uint64_t)(elf_sword)low;
>      if (highaddr)
>          *highaddr = (uint64_t)(elf_sword)high;
> -    return total_size;
> +    ret = total_size;
>   fail:
> -    g_free(data);
> +    g_mapped_file_unref(mapped_file);
>      g_free(phdr);
>      return ret;
>  }
> 
> 

Yes, this looks better!

Paolo
diff mbox series

Patch

diff --git a/include/hw/elf_ops.h b/include/hw/elf_ops.h
index fede37ee9c..ee85fa30b7 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 *mapped_file = NULL;
     uint8_t *data = NULL;
     char label[128];
     int ret = ELF_LOAD_FAILED;
@@ -409,20 +410,32 @@  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.
+     */
+    mapped_file = g_mapped_file_new_from_fd(fd, true, NULL);
+    if (!mapped_file) {
+        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) {
+            data_offset = ph->p_offset; /* Offset where the data is located */
+
+            if (file_size > 0) {
+                if (g_mapped_file_get_length(mapped_file) <
+                    file_size + data_offset) {
                     goto fail;
                 }
+
+                data = (uint8_t *)g_mapped_file_get_contents(mapped_file);
+                data += data_offset;
             }
 
             /* The ELF spec is somewhat vague about the purpose of the
@@ -513,25 +526,25 @@  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);
 
-                    /* rom_add_elf_program() seize the ownership of 'data' */
-                    rom_add_elf_program(label, NULL, data, file_size, mem_size,
-                                        addr, as);
+                    /*
+                     * rom_add_elf_program() seize the ownership of 'data',
+                     * increasing the reference count of 'mapped_file'.
+                     */
+                    rom_add_elf_program(label, mapped_file, data, file_size,
+                                        mem_size, addr, as);
                 } else {
                     address_space_write(as ? as : &address_space_memory,
                                         addr, MEMTXATTRS_UNSPECIFIED,
                                         data, file_size);
-                    g_free(data);
                 }
             }
 
@@ -547,14 +560,16 @@  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) {
+            data_offset = ph->p_offset; /* Offset where the notes are located */
+
+            if (file_size > 0) {
+                if (g_mapped_file_get_length(mapped_file) <
+                    file_size + data_offset) {
                     goto fail;
                 }
+
+                data = (uint8_t *)g_mapped_file_get_contents(mapped_file);
+                data += data_offset;
             }
 
             /*
@@ -570,7 +585,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 +596,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(mapped_file);
     g_free(phdr);
     return ret;
 }