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

[Qemu-devel] [PATCH 0/2] 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.

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 (2):
  elf-ops.h: Map into memory the ELF to load
  hw/i386/pc: Map into memory the initrd

 hw/i386/pc.c         | 15 ++++++++---
 include/hw/elf_ops.h | 59 ++++++++++++++++++++++----------------------
 2 files changed, 41 insertions(+), 33 deletions(-)

-- 
2.20.1

[Qemu-devel] [PATCH 1/2] 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>
---
 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);
+    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 +518,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 +535,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 +550,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 +572,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 +583,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

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 <sgarzare@redhat.com> 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 <dgilbert@redhat.com>
> Suggested-by: Paolo Bonzini <pbonzini@redhat.com>
> Signed-off-by: Stefano Garzarella <sgarzare@redhat.com>
> ---
>  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

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 <sgarzare@redhat.com> 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 <dgilbert@redhat.com>
> > Suggested-by: Paolo Bonzini <pbonzini@redhat.com>
> > Signed-off-by: Stefano Garzarella <sgarzare@redhat.com>
> > ---
> >  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

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

[Paolo Bonzini]

On 23/07/19 11:42, Stefano Garzarella wrote:
> "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.

My reading would be that the second part applies to the writable==TRUE
case.  In fact, the glib source code agrees:

      file->contents = (gchar *) mmap (NULL,  file->length,
                           writable ? PROT_READ|PROT_WRITE : PROT_READ,
                           MAP_PRIVATE, fd, 0);

meaning that we could after all just use writable == true.

Paolo

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

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

[Stefano Garzarella]

On Tue, Jul 23, 2019 at 11:49:13AM +0200, Paolo Bonzini wrote:
> On 23/07/19 11:42, Stefano Garzarella wrote:
> > "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.
> 
> My reading would be that the second part applies to the writable==TRUE
> case.  In fact, the glib source code agrees:
> 
>       file->contents = (gchar *) mmap (NULL,  file->length,
>                            writable ? PROT_READ|PROT_WRITE : PROT_READ,
>                            MAP_PRIVATE, fd, 0);
> 
> meaning that we could after all just use writable == true.

Thanks for checking! I'll use writable == true in the v2!

Stefano

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:42, Stefano Garzarella <sgarzare@redhat.com> wrote:
> 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.

Ah, I misread the docs here (and thought the following paragraph
which talks about changes to the underlying file becoming visible
to the mapping process was talking about changes in the mapping
process becoming visible to the file).

So I think the answer is that we do want to pass writable=true.

Looking at the implementation, we always use mmap()'s MAP_PRIVATE,
so we get a copy-on-write mapping that doesn't change the underlying
file. The effect of the 'writable' flag is that we use PROT_READ|PROT_WRITE,
so if we don't pass writable=true we're liable to get a segfault.

thanks
-- PMM

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:50:24AM +0100, Peter Maydell wrote:
> On Tue, 23 Jul 2019 at 10:42, Stefano Garzarella <sgarzare@redhat.com> wrote:
> > 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.
> 
> Ah, I misread the docs here (and thought the following paragraph
> which talks about changes to the underlying file becoming visible
> to the mapping process was talking about changes in the mapping
> process becoming visible to the file).

I misread too...

> 
> So I think the answer is that we do want to pass writable=true.

Yes, I'll do in the v2!

> 
> Looking at the implementation, we always use mmap()'s MAP_PRIVATE,
> so we get a copy-on-write mapping that doesn't change the underlying
> file. The effect of the 'writable' flag is that we use PROT_READ|PROT_WRITE,
> so if we don't pass writable=true we're liable to get a segfault.

Yes, I just tried and I got the segfault.

Thanks,
Stefano

[Qemu-devel] [PATCH 2/2] 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>
---
 hw/i386/pc.c | 15 +++++++++++----
 1 file changed, 11 insertions(+), 4 deletions(-)

diff --git a/hw/i386/pc.c b/hw/i386/pc.c
index 549c437050..b139589777 100644
--- a/hw/i386/pc.c
+++ b/hw/i386/pc.c
@@ -1241,17 +1241,20 @@ static void load_linux(PCMachineState *pcms,
 
             /* load initrd */
             if (initrd_filename) {
+                GMappedFile *gmf;
                 gsize initrd_size;
                 gchar *initrd_data;
                 GError *gerr = NULL;
 
-                if (!g_file_get_contents(initrd_filename, &initrd_data,
-                            &initrd_size, &gerr)) {
+                gmf = g_mapped_file_new(initrd_filename, false, &gerr);
+                if (!gmf) {
                     fprintf(stderr, "qemu: error reading initrd %s: %s\n",
                             initrd_filename, gerr->message);
                     exit(1);
                 }
 
+                initrd_data = g_mapped_file_get_contents(gmf);
+                initrd_size = g_mapped_file_get_length(gmf);
                 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 +1381,7 @@ static void load_linux(PCMachineState *pcms,
 
     /* load initrd */
     if (initrd_filename) {
+        GMappedFile *gmf;
         gsize initrd_size;
         gchar *initrd_data;
         GError *gerr = NULL;
@@ -1387,12 +1391,15 @@ static void load_linux(PCMachineState *pcms,
             exit(1);
         }
 
-        if (!g_file_get_contents(initrd_filename, &initrd_data,
-                                 &initrd_size, &gerr)) {
+        gmf = g_mapped_file_new(initrd_filename, false, &gerr);
+        if (!gmf) {
             fprintf(stderr, "qemu: error reading initrd %s: %s\n",
                     initrd_filename, gerr->message);
             exit(1);
         }
+
+        initrd_data = g_mapped_file_get_contents(gmf);
+        initrd_size = g_mapped_file_get_length(gmf);
         if (initrd_size >= initrd_max) {
             fprintf(stderr, "qemu: initrd is too large, cannot support."
                     "(max: %"PRIu32", need %"PRId64")\n",
-- 
2.20.1