[Qemu-devel] [PATCH v3 0/3] pc: mmap kernel (ELF image) and initrd

[Qemu-devel] [PATCH v3 0/3] pc: mmap kernel (ELF image) and initrd

[Stefano Garzarella]

In order to reduce the memory footprint when PVH kernel and initrd
are used, we map them into memory instead of reading them.
In this way we can share them between multiple instances of QEMU.

v3:
- Added patch 1 to handle memory-mapped ELFs in rom_add_elf_program() [Paolo]
- Patch 2:
  ~ passed the GMappedFile* to rom_add_elf_program() [Paolo]
  ~ renamed 'GMappedFile *gmf' in 'GMappedFile *mapped_filed' for readability
  ~ set 'data' pointer only if 'file_size > 0' as the original behaviour
    [check-qtest-ppc64 fails without it]
- Patch 3:
  ~ stored the initrd GMappedFile* in PCMachineState to avoid Coverity
    issue [Paolo]
  ~ renamed 'GMappedFile *gmf' in 'GMappedFile *mapped_filed' for readability

v2: https://patchew.org/QEMU/20190723140445.12748-1-sgarzare@redhat.com/

These are the results using a PVH kernel and initrd (cpio):
- memory footprint (using smem) [MB]
        QEMU              before                   now
    # instances        USS      PSS            USS      PSS
         1           102.0M   105.8M         102.3M   106.2M
         2            94.6M   101.2M          72.3M    90.1M
         4            94.1M    98.0M          72.0M    81.5M
         8            94.0M    96.2M          71.8M    76.9M
        16            93.9M    95.1M          71.6M    74.3M

    Initrd size: 3.0M
    Kernel
        image size: 28M
        sections size [size -A -d vmlinux]:  18.9M

- boot time [ms]
                          before                   now
 qemu_init_end:           63.85                   55.91
 linux_start_kernel:      82.11 (+18.26)          74.51 (+18.60)
 linux_start_user:       169.94 (+87.83)         159.06 (+84.56)

QEMU command used:
./qemu-system-x86_64 -bios /path/to/seabios/out/bios.bin -no-hpet \
    -machine q35,accel=kvm,kernel_irqchip,nvdimm,sata=off,smbus=off,vmport=off \
    -cpu host -m 1G -smp 1 -vga none -display none -no-user-config -nodefaults \
    -kernel /path/to/vmlinux -initrd /path/to/rootfs.cpio \
    -append 'root=/dev/mem0 ro console=hvc0 pci=lastbus=0 nosmap'

Stefano Garzarella (3):
  loader: Handle memory-mapped ELFs
  elf-ops.h: Map into memory the ELF to load
  hw/i386/pc: Map into memory the initrd

 hw/core/loader.c     | 37 +++++++++++++++++++-----
 hw/i386/pc.c         | 17 ++++++++---
 include/hw/elf_ops.h | 68 ++++++++++++++++++++++++++------------------
 include/hw/i386/pc.h |  1 +
 include/hw/loader.h  |  5 ++--
 5 files changed, 88 insertions(+), 40 deletions(-)

-- 
2.20.1

[Qemu-devel] [PATCH v3 1/3] loader: Handle memory-mapped ELFs

[Stefano Garzarella]

This patch allows handling an ELF memory-mapped, taking care
the reference count of the GMappedFile* passed through
rom_add_elf_program().
In this case, the 'data' pointer is not heap-allocated, so
we cannot free it.

Suggested-by: Paolo Bonzini <pbonzini@redhat.com>
Signed-off-by: Stefano Garzarella <sgarzare@redhat.com>
---
 hw/core/loader.c     | 37 ++++++++++++++++++++++++++++++-------
 include/hw/elf_ops.h |  2 +-
 include/hw/loader.h  |  5 +++--
 3 files changed, 34 insertions(+), 10 deletions(-)

diff --git a/hw/core/loader.c b/hw/core/loader.c
index 425bf69a99..637d448f42 100644
--- a/hw/core/loader.c
+++ b/hw/core/loader.c
@@ -836,6 +836,7 @@ struct Rom {
     int isrom;
     char *fw_dir;
     char *fw_file;
+    GMappedFile *mapped_file;
 
     bool committed;
 
@@ -846,10 +847,25 @@ struct Rom {
 static FWCfgState *fw_cfg;
 static QTAILQ_HEAD(, Rom) roms = QTAILQ_HEAD_INITIALIZER(roms);
 
-/* rom->data must be heap-allocated (do not use with rom_add_elf_program()) */
+/*
+ * rom->data can be heap-allocated or memory-mapped (e.g. when added with
+ * rom_add_elf_program())
+ */
+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;
+}
+
 static void rom_free(Rom *rom)
 {
-    g_free(rom->data);
+    rom_free_data(rom);
     g_free(rom->path);
     g_free(rom->name);
     g_free(rom->fw_dir);
@@ -1057,10 +1073,12 @@ MemoryRegion *rom_add_blob(const char *name, const void *blob, size_t len,
 /* This function is specific for elf program because we don't need to allocate
  * all the rom. We just allocate the first part and the rest is just zeros. This
  * is why romsize and datasize are different. Also, this function seize the
- * memory ownership of "data", so we don't have to allocate and copy the buffer.
+ * memory ownership of "data", increasing the reference count of "mapped_file",
+ * so we don't have to allocate and copy the buffer.
  */
-int rom_add_elf_program(const char *name, void *data, size_t datasize,
-                        size_t romsize, hwaddr addr, AddressSpace *as)
+int rom_add_elf_program(const char *name, GMappedFile *mapped_file, void *data,
+                        size_t datasize, size_t romsize, hwaddr addr,
+                        AddressSpace *as)
 {
     Rom *rom;
 
@@ -1071,6 +1089,12 @@ int rom_add_elf_program(const char *name, void *data, size_t datasize,
     rom->romsize  = romsize;
     rom->data     = data;
     rom->as       = as;
+
+    if (mapped_file && data) {
+        g_mapped_file_ref(mapped_file);
+        rom->mapped_file = mapped_file;
+    }
+
     rom_insert(rom);
     return 0;
 }
@@ -1105,8 +1129,7 @@ static void rom_reset(void *unused)
         }
         if (rom->isrom) {
             /* rom needs to be written only once */
-            g_free(rom->data);
-            rom->data = NULL;
+            rom_free_data(rom);
         }
         /*
          * The rom loader is really on the same level as firmware in the guest
diff --git a/include/hw/elf_ops.h b/include/hw/elf_ops.h
index 690f9238c8..fede37ee9c 100644
--- a/include/hw/elf_ops.h
+++ b/include/hw/elf_ops.h
@@ -525,7 +525,7 @@ static int glue(load_elf, SZ)(const char *name, int fd,
                     snprintf(label, sizeof(label), "phdr #%d: %s", i, name);
 
                     /* rom_add_elf_program() seize the ownership of 'data' */
-                    rom_add_elf_program(label, data, file_size, mem_size,
+                    rom_add_elf_program(label, NULL, data, file_size, mem_size,
                                         addr, as);
                 } else {
                     address_space_write(as ? as : &address_space_memory,
diff --git a/include/hw/loader.h b/include/hw/loader.h
index 3e1b3a4566..07fd9286e7 100644
--- a/include/hw/loader.h
+++ b/include/hw/loader.h
@@ -258,8 +258,9 @@ MemoryRegion *rom_add_blob(const char *name, const void *blob, size_t len,
                            FWCfgCallback fw_callback,
                            void *callback_opaque, AddressSpace *as,
                            bool read_only);
-int rom_add_elf_program(const char *name, void *data, size_t datasize,
-                        size_t romsize, hwaddr addr, AddressSpace *as);
+int rom_add_elf_program(const char *name, GMappedFile *mapped_file, void *data,
+                        size_t datasize, size_t romsize, hwaddr addr,
+                        AddressSpace *as);
 int rom_check_and_register_reset(void);
 void rom_set_fw(FWCfgState *f);
 void rom_set_order_override(int order);
-- 
2.20.1

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

[Stefano Garzarella]

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

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;
 }
-- 
2.20.1

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

[Paolo Bonzini]

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

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

[Stefano Garzarella]

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

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

[Paolo Bonzini]

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

[Qemu-devel] [PATCH v3 3/3] hw/i386/pc: Map into memory the initrd

[Stefano Garzarella]

In order to reduce the memory footprint we map into memory
the initrd using g_mapped_file_new() instead of reading it.
In this way we can share the initrd pages between multiple
instances of QEMU.

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
  - stored the initrd GMappedFile* in PCMachineState to avoid Coverity
    issue [Paolo]
---
 hw/i386/pc.c         | 17 +++++++++++++----
 include/hw/i386/pc.h |  1 +
 2 files changed, 14 insertions(+), 4 deletions(-)

diff --git a/hw/i386/pc.c b/hw/i386/pc.c
index 549c437050..96f6b89f70 100644
--- a/hw/i386/pc.c
+++ b/hw/i386/pc.c
@@ -1241,17 +1241,21 @@ static void load_linux(PCMachineState *pcms,
 
             /* load initrd */
             if (initrd_filename) {
+                GMappedFile *mapped_file;
                 gsize initrd_size;
                 gchar *initrd_data;
                 GError *gerr = NULL;
 
-                if (!g_file_get_contents(initrd_filename, &initrd_data,
-                            &initrd_size, &gerr)) {
+                mapped_file = g_mapped_file_new(initrd_filename, false, &gerr);
+                if (!mapped_file) {
                     fprintf(stderr, "qemu: error reading initrd %s: %s\n",
                             initrd_filename, gerr->message);
                     exit(1);
                 }
+                pcms->initrd_mapped_file = mapped_file;
 
+                initrd_data = g_mapped_file_get_contents(mapped_file);
+                initrd_size = g_mapped_file_get_length(mapped_file);
                 initrd_max = pcms->below_4g_mem_size - pcmc->acpi_data_size - 1;
                 if (initrd_size >= initrd_max) {
                     fprintf(stderr, "qemu: initrd is too large, cannot support."
@@ -1378,6 +1382,7 @@ static void load_linux(PCMachineState *pcms,
 
     /* load initrd */
     if (initrd_filename) {
+        GMappedFile *mapped_file;
         gsize initrd_size;
         gchar *initrd_data;
         GError *gerr = NULL;
@@ -1387,12 +1392,16 @@ static void load_linux(PCMachineState *pcms,
             exit(1);
         }
 
-        if (!g_file_get_contents(initrd_filename, &initrd_data,
-                                 &initrd_size, &gerr)) {
+        mapped_file = g_mapped_file_new(initrd_filename, false, &gerr);
+        if (!mapped_file) {
             fprintf(stderr, "qemu: error reading initrd %s: %s\n",
                     initrd_filename, gerr->message);
             exit(1);
         }
+        pcms->initrd_mapped_file = mapped_file;
+
+        initrd_data = g_mapped_file_get_contents(mapped_file);
+        initrd_size = g_mapped_file_get_length(mapped_file);
         if (initrd_size >= initrd_max) {
             fprintf(stderr, "qemu: initrd is too large, cannot support."
                     "(max: %"PRIu32", need %"PRId64")\n",
diff --git a/include/hw/i386/pc.h b/include/hw/i386/pc.h
index 859b64c51d..44edc6955e 100644
--- a/include/hw/i386/pc.h
+++ b/include/hw/i386/pc.h
@@ -42,6 +42,7 @@ struct PCMachineState {
     FWCfgState *fw_cfg;
     qemu_irq *gsi;
     PFlashCFI01 *flash[2];
+    GMappedFile *initrd_mapped_file;
 
     /* Configuration options: */
     uint64_t max_ram_below_4g;
-- 
2.20.1