[Qemu-devel] [RFC PATCH v5 0/4] reload images from host rootfs once reset to save footprint

[Qemu-devel] [RFC PATCH v5 0/4] reload images from host rootfs once reset to save footprint

[Olivia Yin]

The current model of loader copy "rom blobs" and kept in memory until 
a reset occurs and waste host memory.

This serial of patches uses private reset handlers to load from hard 
disk on reset, which could make loader framework more dynamic and 
reduce the memory consumption of QEMU process.

Olivia Yin (4):
  use image_file_reset to reload initrd image
  use uimage_reset to reload uimage
  use elf_reset to reload elf image
  free the memory malloced by load_at()

 elf.h        |   10 +++++
 hw/elf_ops.h |   42 +++++++++++++++++++--
 hw/loader.c  |  114 +++++++++++++++++++++++++++++++++++++++++++++++++---------
 hw/loader.h  |   13 +++++++
 4 files changed, 158 insertions(+), 21 deletions(-)

v5: 
patch 2/4: remove global variables is_linux and kernel_loaded.
patch 3/4: register reset handlers in loader.c for elf images.
           extract the duplicated source code into function elf_phy_loader().
patch 4/4: fix the issue of memory increasing (about 1.4MB) once reload elf image.

[Qemu-devel] [RFC PATCH v5 1/4] use image_file_reset to reload initrd image

[Olivia Yin]

Signed-off-by: Olivia Yin <hong-hua.yin@freescale.com>
---
 hw/loader.c |   39 +++++++++++++++++++++++++++++++++++++++
 hw/loader.h |    7 +++++++
 2 files changed, 46 insertions(+), 0 deletions(-)

diff --git a/hw/loader.c b/hw/loader.c
index ba01ca6..a8a0a09 100644
--- a/hw/loader.c
+++ b/hw/loader.c
@@ -86,6 +86,39 @@ int load_image(const char *filename, uint8_t *addr)
     return size;
 }
 
+static void image_file_reset(void *opaque)
+{
+    ImageFile *image = opaque;
+    GError *err = NULL;
+    gboolean res;
+    gchar *content;
+    gsize size;
+
+    if(image->dir) {
+        const char *basename;
+        char fw_file_name[56];
+
+        basename = strrchr(image->name, '/');
+        if (basename) {
+            basename++;
+        } else {
+            basename = image->name;
+        }
+        snprintf(fw_file_name, sizeof(fw_file_name), "%s/%s", image->dir,
+                 basename);
+        image->name = g_strdup(fw_file_name);
+    }
+
+    res = g_file_get_contents(image->name, &content, &size, &err);
+    if (res == FALSE) {
+       error_report("failed to read image file: %s\n", image->name);
+       g_error_free(err);
+    } else {
+       cpu_physical_memory_write(image->addr, (uint8_t *)content, size);
+       g_free(content);
+    }
+}
+
 /* read()-like version */
 ssize_t read_targphys(const char *name,
                       int fd, hwaddr dst_addr, size_t nbytes)
@@ -113,6 +146,12 @@ int load_image_targphys(const char *filename,
     }
     if (size > 0) {
         rom_add_file_fixed(filename, addr, -1);
+        ImageFile *image;
+        image = g_malloc0(sizeof(*image));
+        image->name = g_strdup(filename);
+        image->addr = addr;
+ 
+        qemu_register_reset(image_file_reset, image);
     }
     return size;
 }
diff --git a/hw/loader.h b/hw/loader.h
index 26480ad..d021629 100644
--- a/hw/loader.h
+++ b/hw/loader.h
@@ -1,6 +1,13 @@
 #ifndef LOADER_H
 #define LOADER_H
 
+typedef struct ImageFile ImageFile;
+struct ImageFile {
+    char *name;
+    char *dir;
+    hwaddr addr;
+};
+
 /* loader.c */
 int get_image_size(const char *filename);
 int load_image(const char *filename, uint8_t *addr); /* deprecated */
-- 
1.7.1

[Qemu-devel] [RFC PATCH v5 2/4] use uimage_reset to reload uimage

[Olivia Yin]

Signed-off-by: Olivia Yin <hong-hua.yin@freescale.com>
---
 hw/loader.c |   64 +++++++++++++++++++++++++++++++++++++++++++---------------
 hw/loader.h |    8 ++++++-
 2 files changed, 54 insertions(+), 18 deletions(-)

diff --git a/hw/loader.c b/hw/loader.c
index a8a0a09..e1e2a20 100644
--- a/hw/loader.c
+++ b/hw/loader.c
@@ -472,15 +472,15 @@ static ssize_t gunzip(void *dst, size_t dstlen, uint8_t *src,
     return dstbytes;
 }
 
-/* Load a U-Boot image.  */
-int load_uimage(const char *filename, hwaddr *ep,
-                hwaddr *loadaddr, int *is_linux)
+/* write uimage into memory */
+static int uimage_physical_loader(const char *filename, hwaddr *ep, 
+                                  uint8_t **data, hwaddr *loadaddr,
+                                  int *is_linux, int reset)
 {
     int fd;
     int size;
     uboot_image_header_t h;
     uboot_image_header_t *hdr = &h;
-    uint8_t *data = NULL;
     int ret = -1;
 
     fd = open(filename, O_RDONLY | O_BINARY);
@@ -522,9 +522,9 @@ int load_uimage(const char *filename, hwaddr *ep,
     }
 
     *ep = hdr->ih_ep;
-    data = g_malloc(hdr->ih_size);
+    *data = g_malloc(hdr->ih_size);
 
-    if (read(fd, data, hdr->ih_size) != hdr->ih_size) {
+    if (read(fd, *data, hdr->ih_size) != hdr->ih_size) {
         fprintf(stderr, "Error reading file\n");
         goto out;
     }
@@ -534,11 +534,11 @@ int load_uimage(const char *filename, hwaddr *ep,
         size_t max_bytes;
         ssize_t bytes;
 
-        compressed_data = data;
+        compressed_data = *data;
         max_bytes = UBOOT_MAX_GUNZIP_BYTES;
-        data = g_malloc(max_bytes);
+        *data = g_malloc(max_bytes);
 
-        bytes = gunzip(data, max_bytes, compressed_data, hdr->ih_size);
+        bytes = gunzip(*data, max_bytes, compressed_data, hdr->ih_size);
         g_free(compressed_data);
         if (bytes < 0) {
             fprintf(stderr, "Unable to decompress gzipped image!\n");
@@ -547,7 +547,11 @@ int load_uimage(const char *filename, hwaddr *ep,
         hdr->ih_size = bytes;
     }
 
-    rom_add_blob_fixed(filename, data, hdr->ih_size, hdr->ih_load);
+    if (!reset) {
+        rom_add_blob_fixed(filename, *data, hdr->ih_size, hdr->ih_load);
+    } else {
+        cpu_physical_memory_write(hdr->ih_load, *data, hdr->ih_size);
+    }
 
     if (loadaddr)
         *loadaddr = hdr->ih_load;
@@ -555,12 +559,41 @@ int load_uimage(const char *filename, hwaddr *ep,
     ret = hdr->ih_size;
 
 out:
-    if (data)
-        g_free(data);
     close(fd);
     return ret;
 }
 
+static void uimage_reset(void *opaque)
+{
+    ImageFile *image = opaque;
+    uint8_t *data = NULL;
+
+    uimage_physical_loader(image->name, image->ep, &data, &image->addr, 
+                           image->is_linux, 1);
+    g_free(data);
+}
+
+/* Load a U-Boot image.  */
+int load_uimage(const char *filename, hwaddr *ep,
+                hwaddr *loadaddr, int *is_linux)
+{
+    int size;
+    ImageFile *image;
+    uint8_t *data = NULL;
+
+    size= uimage_physical_loader(filename, ep, &data, loadaddr, is_linux, 0);
+    if (size > 0) {
+        g_free(data);
+        image = g_malloc0(sizeof(*image));
+        image->name = g_strdup(filename);
+        image->addr = *loadaddr;
+        image->ep = ep;
+        image->is_linux = is_linux;
+        qemu_register_reset(uimage_reset, image);
+    }
+    return size;
+}
+
 /*
  * Functions for reboot-persistent memory regions.
  *  - used for vga bios and option roms.
@@ -708,11 +741,8 @@ static void rom_reset(void *unused)
             continue;
         }
         cpu_physical_memory_write_rom(rom->addr, rom->data, rom->romsize);
-        if (rom->isrom) {
-            /* rom needs to be written only once */
-            g_free(rom->data);
-            rom->data = NULL;
-        }
+        g_free(rom->data);
+        rom->data = NULL;
     }
 }
 
diff --git a/hw/loader.h b/hw/loader.h
index d021629..08a2f6c 100644
--- a/hw/loader.h
+++ b/hw/loader.h
@@ -4,8 +4,14 @@
 typedef struct ImageFile ImageFile;
 struct ImageFile {
     char *name;
-    char *dir;
     hwaddr addr;
+    int reset;
+    /* rom file */
+    char *dir;
+    /* uimage */
+    hwaddr *ep;
+    int *is_linux; 
+    
 };
 
 /* loader.c */
-- 
1.7.1

[Qemu-devel] [RFC PATCH v5 3/4] use elf_reset to reload elf image

[Olivia Yin]

Signed-off-by: Olivia Yin <hong-hua.yin@freescale.com>
---
 elf.h        |   10 ++++++++++
 hw/elf_ops.h |   40 ++++++++++++++++++++++++++++++++++++----
 hw/loader.c  |   11 +++++++++++
 3 files changed, 57 insertions(+), 4 deletions(-)

diff --git a/elf.h b/elf.h
index a21ea53..335f1af 100644
--- a/elf.h
+++ b/elf.h
@@ -1078,6 +1078,16 @@ typedef struct elf64_hdr {
   Elf64_Half e_shstrndx;
 } Elf64_Ehdr;
 
+typedef struct ImageElf ImageElf;
+struct ImageElf {
+    char *name;
+    uint64_t (*fn)(void *, uint64_t);
+    void *opaque;
+    int swab;
+    int machine;
+    int lsb;
+};
+
 /* These constants define the permissions on sections in the program
    header, p_flags. */
 #define PF_R		0x4
diff --git a/hw/elf_ops.h b/hw/elf_ops.h
index 531a425..b346861 100644
--- a/hw/elf_ops.h
+++ b/hw/elf_ops.h
@@ -187,12 +187,12 @@ static int glue(load_symbols, SZ)(struct elfhdr *ehdr, int fd, int must_swab,
     return -1;
 }
 
-static int glue(load_elf, SZ)(const char *name, int fd,
+static int glue(elf_phy_loader, SZ)(const char *name, int fd,
                               uint64_t (*translate_fn)(void *, uint64_t),
                               void *translate_opaque,
                               int must_swab, uint64_t *pentry,
                               uint64_t *lowaddr, uint64_t *highaddr,
-                              int elf_machine, int clear_lsb)
+                              int elf_machine, int clear_lsb, int reset)
 {
     struct elfhdr ehdr;
     struct elf_phdr *phdr = NULL, *ph;
@@ -202,6 +202,9 @@ static int glue(load_elf, SZ)(const char *name, int fd,
     uint8_t *data = NULL;
     char label[128];
 
+    if (reset) {
+        fd = open(name, O_RDONLY | O_BINARY);
+    }
     if (read(fd, &ehdr, sizeof(ehdr)) != sizeof(ehdr))
         goto fail;
     if (must_swab) {
@@ -280,8 +283,12 @@ static int glue(load_elf, SZ)(const char *name, int fd,
                 *pentry = ehdr.e_entry - ph->p_vaddr + ph->p_paddr;
             }
 
-            snprintf(label, sizeof(label), "phdr #%d: %s", i, name);
-            rom_add_blob_fixed(label, data, mem_size, addr);
+            if (!reset) {
+                snprintf(label, sizeof(label), "phdr #%d: %s", i, name);
+                rom_add_blob_fixed(label, data, mem_size, addr);
+            } else {
+                cpu_physical_memory_write(addr, data, mem_size);
+            }
 
             total_size += mem_size;
             if (addr < low)
@@ -304,3 +311,28 @@ static int glue(load_elf, SZ)(const char *name, int fd,
     g_free(phdr);
     return -1;
 }
+
+static void glue(elf_reset, SZ)(void *opaque)
+{
+    ImageElf *elf = opaque;
+    int fd;
+    
+    fd = open(elf->name, O_RDONLY | O_BINARY);
+    glue(elf_phy_loader, SZ)(elf->name, fd, elf->fn, elf->opaque, elf->swab, 
+                             NULL, NULL, NULL, elf->machine, elf->lsb, 1);
+}
+
+static int glue(load_elf, SZ)(const char *name, int fd,
+                              uint64_t (*translate_fn)(void *, uint64_t),
+                              void *translate_opaque,
+                              int must_swab, uint64_t *pentry,
+                              uint64_t *lowaddr, uint64_t *highaddr,
+                              int elf_machine, int clear_lsb)
+{
+    int ret;
+
+    ret = glue(elf_phy_loader, SZ)(name, fd, (*translate_fn), translate_opaque,
+                                   must_swab, pentry, lowaddr, highaddr,
+                                   elf_machine, clear_lsb, 0);
+    return ret;
+}
diff --git a/hw/loader.c b/hw/loader.c
index e1e2a20..92e2372 100644
--- a/hw/loader.c
+++ b/hw/loader.c
@@ -357,13 +357,24 @@ int load_elf(const char *filename, uint64_t (*translate_fn)(void *, uint64_t),
         goto fail;
     }
 
+    ImageElf *elf;
+    elf = g_malloc0(sizeof(*elf));
+    elf->name = g_strdup(filename);
+    elf->fn = translate_fn;
+    elf->opaque = translate_opaque;
+    elf->swab = must_swab;
+    elf->machine = elf_machine;
+    elf->lsb = clear_lsb;
+
     lseek(fd, 0, SEEK_SET);
     if (e_ident[EI_CLASS] == ELFCLASS64) {
         ret = load_elf64(filename, fd, translate_fn, translate_opaque, must_swab,
                          pentry, lowaddr, highaddr, elf_machine, clear_lsb);
+        qemu_register_reset(elf_reset64, elf);
     } else {
         ret = load_elf32(filename, fd, translate_fn, translate_opaque, must_swab,
                          pentry, lowaddr, highaddr, elf_machine, clear_lsb);
+        qemu_register_reset(elf_reset32, elf);
     }
 
     close(fd);
-- 
1.7.1

[Qemu-devel] [RFC PATCH v5 4/4] free the memory malloced by load_at()

[Olivia Yin]

Signed-off-by: Olivia Yin <hong-hua.yin@freescale.com>
---
 hw/elf_ops.h |    2 ++
 1 files changed, 2 insertions(+), 0 deletions(-)

diff --git a/hw/elf_ops.h b/hw/elf_ops.h
index b346861..9c76a75 100644
--- a/hw/elf_ops.h
+++ b/hw/elf_ops.h
@@ -178,6 +178,8 @@ static int glue(load_symbols, SZ)(struct elfhdr *ehdr, int fd, int must_swab,
     s->disas_strtab = str;
     s->next = syminfos;
     syminfos = s;
+    g_free(syms);
+    g_free(str);
     g_free(shdr_table);
     return 0;
  fail:
-- 
1.7.1

Re: [Qemu-devel] [RFC PATCH v5 4/4] free the memory malloced by load_at()

[Stuart Yoder]

On Wed, Nov 21, 2012 at 8:38 AM, Olivia Yin <hong-hua.yin@freescale.com> wrote:
> Signed-off-by: Olivia Yin <hong-hua.yin@freescale.com>
> ---
>  hw/elf_ops.h |    2 ++
>  1 files changed, 2 insertions(+), 0 deletions(-)
>
> diff --git a/hw/elf_ops.h b/hw/elf_ops.h
> index b346861..9c76a75 100644
> --- a/hw/elf_ops.h
> +++ b/hw/elf_ops.h
> @@ -178,6 +178,8 @@ static int glue(load_symbols, SZ)(struct elfhdr *ehdr, int fd, int must_swab,
>      s->disas_strtab = str;
>      s->next = syminfos;
>      syminfos = s;
> +    g_free(syms);
> +    g_free(str);
>      g_free(shdr_table);
>      return 0;
>   fail:

Olivia, as Alex pointed out there are references to syms and str in
the struct "s"....so you can't just free those I don't think.

The problem that leaves us with is that on every reset when we call
load_elf() that we re-load and re-malloc space for the symbols.

I think the solution may be to factor out the call to load_symbols()
from load_elf().   It looks like what load_symbols does in the end is
set the variable syminfos to point to the loaded symbol info.

If you factor load_symbols() out then in load_elf_32/64() you would do
something like:
      elf_phy_loader_32/64()
      load_symbols_32/64().

We don't need to be reloading symbols on every reset.

Alex, does that make sense?

Stuart

Re: [Qemu-devel] [RFC PATCH v5 4/4] free the memory malloced by load_at()

[Alexander Graf]

On 11/21/2012 07:39 PM, Stuart Yoder wrote:
> On Wed, Nov 21, 2012 at 8:38 AM, Olivia Yin<hong-hua.yin@freescale.com>  wrote:
>> Signed-off-by: Olivia Yin<hong-hua.yin@freescale.com>
>> ---
>>   hw/elf_ops.h |    2 ++
>>   1 files changed, 2 insertions(+), 0 deletions(-)
>>
>> diff --git a/hw/elf_ops.h b/hw/elf_ops.h
>> index b346861..9c76a75 100644
>> --- a/hw/elf_ops.h
>> +++ b/hw/elf_ops.h
>> @@ -178,6 +178,8 @@ static int glue(load_symbols, SZ)(struct elfhdr *ehdr, int fd, int must_swab,
>>       s->disas_strtab = str;
>>       s->next = syminfos;
>>       syminfos = s;
>> +    g_free(syms);
>> +    g_free(str);
>>       g_free(shdr_table);
>>       return 0;
>>    fail:
> Olivia, as Alex pointed out there are references to syms and str in
> the struct "s"....so you can't just free those I don't think.
>
> The problem that leaves us with is that on every reset when we call
> load_elf() that we re-load and re-malloc space for the symbols.
>
> I think the solution may be to factor out the call to load_symbols()
> from load_elf().   It looks like what load_symbols does in the end is
> set the variable syminfos to point to the loaded symbol info.
>
> If you factor load_symbols() out then in load_elf_32/64() you would do
> something like:
>        elf_phy_loader_32/64()
>        load_symbols_32/64().
>
> We don't need to be reloading symbols on every reset.
>
> Alex, does that make sense?

We can also mandate the caller of load_symbols to free the respective 
data :)


Alex

Re: [Qemu-devel] [RFC PATCH v5 4/4] free the memory malloced by load_at()

[Yin Olivia-R63875]

Hi Stuart & Alex,

"syms" and "str" could not be free since "symbols" is a global variable which
need stay in the memory during the whole life of VM. So it will not be free and 
reload when reset.

The only change is to the previous patch of elf loader (hw/elf_ops.h) as below:

@@ -232,7 +235,9 @@ static int glue(load_elf, SZ)(const char *name, int fd,
     if (pentry)
        *pentry = (uint64_t)(elf_sword)ehdr.e_entry;

-    glue(load_symbols, SZ)(&ehdr, fd, must_swab, clear_lsb);
+    if (!reset) {
+        glue(load_symbols, SZ)(&ehdr, fd, must_swab, clear_lsb);
+    }

     size = ehdr.e_phnum * sizeof(phdr[0]);
     lseek(fd, ehdr.e_phoff, SEEK_SET); 

Do you think it is reasonable?

Best Regards,
Olivia

> -----Original Message-----
> From: Alexander Graf [mailto:agraf@suse.de]
> Sent: Thursday, November 22, 2012 2:42 AM
> To: Stuart Yoder
> Cc: Yin Olivia-R63875; qemu-devel@nongnu.org; qemu-ppc@nongnu.org
> Subject: Re: [Qemu-devel] [RFC PATCH v5 4/4] free the memory malloced by
> load_at()
> 
> On 11/21/2012 07:39 PM, Stuart Yoder wrote:
> > On Wed, Nov 21, 2012 at 8:38 AM, Olivia Yin<hong-hua.yin@freescale.com>
> wrote:
> >> Signed-off-by: Olivia Yin<hong-hua.yin@freescale.com>
> >> ---
> >>   hw/elf_ops.h |    2 ++
> >>   1 files changed, 2 insertions(+), 0 deletions(-)
> >>
> >> diff --git a/hw/elf_ops.h b/hw/elf_ops.h index b346861..9c76a75
> >> 100644
> >> --- a/hw/elf_ops.h
> >> +++ b/hw/elf_ops.h
> >> @@ -178,6 +178,8 @@ static int glue(load_symbols, SZ)(struct elfhdr
> *ehdr, int fd, int must_swab,
> >>       s->disas_strtab = str;
> >>       s->next = syminfos;
> >>       syminfos = s;
> >> +    g_free(syms);
> >> +    g_free(str);
> >>       g_free(shdr_table);
> >>       return 0;
> >>    fail:
> > Olivia, as Alex pointed out there are references to syms and str in
> > the struct "s"....so you can't just free those I don't think.
> >
> > The problem that leaves us with is that on every reset when we call
> > load_elf() that we re-load and re-malloc space for the symbols.
> >
> > I think the solution may be to factor out the call to load_symbols()
> > from load_elf().   It looks like what load_symbols does in the end is
> > set the variable syminfos to point to the loaded symbol info.
> >
> > If you factor load_symbols() out then in load_elf_32/64() you would do
> > something like:
> >        elf_phy_loader_32/64()
> >        load_symbols_32/64().
> >
> > We don't need to be reloading symbols on every reset.
> >
> > Alex, does that make sense?
> 
> We can also mandate the caller of load_symbols to free the respective
> data :)
> 
> 
> Alex
> 

Re: [Qemu-devel] [RFC PATCH v5 4/4] free the memory malloced by load_at()

[Alexander Graf]

On 26.11.2012, at 02:53, Yin Olivia-R63875 wrote:

> Hi Stuart & Alex,
> 
> "syms" and "str" could not be free since "symbols" is a global variable which
> need stay in the memory during the whole life of VM. So it will not be free and 
> reload when reset.

Ah, that's used for the debug log output, right?

> The only change is to the previous patch of elf loader (hw/elf_ops.h) as below:
> 
> @@ -232,7 +235,9 @@ static int glue(load_elf, SZ)(const char *name, int fd,
>     if (pentry)
>        *pentry = (uint64_t)(elf_sword)ehdr.e_entry;
> 
> -    glue(load_symbols, SZ)(&ehdr, fd, must_swab, clear_lsb);
> +    if (!reset) {
> +        glue(load_symbols, SZ)(&ehdr, fd, must_swab, clear_lsb);
> +    }
> 
>     size = ehdr.e_phnum * sizeof(phdr[0]);
>     lseek(fd, ehdr.e_phoff, SEEK_SET); 
> 
> Do you think it is reasonable?

I think semantically we want to only load the symbols the first time we load the binary (read: not on reset), yes. Where does the reset variable you're using there come from?


Alex

> 
> Best Regards,
> Olivia
> 
>> -----Original Message-----
>> From: Alexander Graf [mailto:agraf@suse.de]
>> Sent: Thursday, November 22, 2012 2:42 AM
>> To: Stuart Yoder
>> Cc: Yin Olivia-R63875; qemu-devel@nongnu.org; qemu-ppc@nongnu.org
>> Subject: Re: [Qemu-devel] [RFC PATCH v5 4/4] free the memory malloced by
>> load_at()
>> 
>> On 11/21/2012 07:39 PM, Stuart Yoder wrote:
>>> On Wed, Nov 21, 2012 at 8:38 AM, Olivia Yin<hong-hua.yin@freescale.com>
>> wrote:
>>>> Signed-off-by: Olivia Yin<hong-hua.yin@freescale.com>
>>>> ---
>>>>  hw/elf_ops.h |    2 ++
>>>>  1 files changed, 2 insertions(+), 0 deletions(-)
>>>> 
>>>> diff --git a/hw/elf_ops.h b/hw/elf_ops.h index b346861..9c76a75
>>>> 100644
>>>> --- a/hw/elf_ops.h
>>>> +++ b/hw/elf_ops.h
>>>> @@ -178,6 +178,8 @@ static int glue(load_symbols, SZ)(struct elfhdr
>> *ehdr, int fd, int must_swab,
>>>>      s->disas_strtab = str;
>>>>      s->next = syminfos;
>>>>      syminfos = s;
>>>> +    g_free(syms);
>>>> +    g_free(str);
>>>>      g_free(shdr_table);
>>>>      return 0;
>>>>   fail:
>>> Olivia, as Alex pointed out there are references to syms and str in
>>> the struct "s"....so you can't just free those I don't think.
>>> 
>>> The problem that leaves us with is that on every reset when we call
>>> load_elf() that we re-load and re-malloc space for the symbols.
>>> 
>>> I think the solution may be to factor out the call to load_symbols()
>>> from load_elf().   It looks like what load_symbols does in the end is
>>> set the variable syminfos to point to the loaded symbol info.
>>> 
>>> If you factor load_symbols() out then in load_elf_32/64() you would do
>>> something like:
>>>       elf_phy_loader_32/64()
>>>       load_symbols_32/64().
>>> 
>>> We don't need to be reloading symbols on every reset.
>>> 
>>> Alex, does that make sense?
>> 
>> We can also mandate the caller of load_symbols to free the respective
>> data :)
>> 
>> 
>> Alex
>> 
> 
> 

Re: [Qemu-devel] [RFC PATCH v5 4/4] free the memory malloced by load_at()

[Yin Olivia-R63875]

I added parameter 'reset' into the original load_elf32/64() in patch 3/4.
Reserve the most lines and rename this function as elf_phy_loader32/64().

-static int glue(load_elf, SZ)(const char *name, int fd,
+static int glue(elf_phy_loader, SZ)(const char *name, int fd,
                               uint64_t (*translate_fn)(void *, uint64_t),
                               void *translate_opaque,
                               int must_swab, uint64_t *pentry,
                               uint64_t *lowaddr, uint64_t *highaddr,
-                              int elf_machine, int clear_lsb)
+                              int elf_machine, int clear_lsb, int reset)

Then load_elf32/64() and elf_reset32/64() will call this function with different reset values.

static void glue(elf_reset, SZ)(void *opaque)
{
    ImageElf *elf = opaque;
    int fd;

    fd = open(elf->name, O_RDONLY | O_BINARY);
    glue(elf_phy_loader, SZ)(elf->name, fd, elf->fn, elf->opaque, elf->swab,
                             NULL, NULL, NULL, elf->machine, elf->lsb, 1);
}

static int glue(load_elf, SZ)(const char *name, int fd,
                              uint64_t (*translate_fn)(void *, uint64_t),
                              void *translate_opaque,
                              int must_swab, uint64_t *pentry,
                              uint64_t *lowaddr, uint64_t *highaddr,
                              int elf_machine, int clear_lsb)
{
    int ret;

    ret = glue(elf_phy_loader, SZ)(name, fd, (*translate_fn), translate_opaque,
                                   must_swab, pentry, lowaddr, highaddr,
                                   elf_machine, clear_lsb, 0);
    return ret;
}

Best Regards,
Olivia                                                        

> -----Original Message-----
> From: Alexander Graf [mailto:agraf@suse.de]
> Sent: Monday, November 26, 2012 9:03 PM
> To: Yin Olivia-R63875
> Cc: Stuart Yoder; qemu-devel@nongnu.org; qemu-ppc@nongnu.org
> Subject: Re: [Qemu-devel] [RFC PATCH v5 4/4] free the memory malloced by
> load_at()
> 
> 
> On 26.11.2012, at 02:53, Yin Olivia-R63875 wrote:
> 
> > Hi Stuart & Alex,
> >
> > "syms" and "str" could not be free since "symbols" is a global
> > variable which need stay in the memory during the whole life of VM. So
> > it will not be free and reload when reset.
> 
> Ah, that's used for the debug log output, right?
> 
> > The only change is to the previous patch of elf loader (hw/elf_ops.h)
> as below:
> >
> > @@ -232,7 +235,9 @@ static int glue(load_elf, SZ)(const char *name, int
> fd,
> >     if (pentry)
> >        *pentry = (uint64_t)(elf_sword)ehdr.e_entry;
> >
> > -    glue(load_symbols, SZ)(&ehdr, fd, must_swab, clear_lsb);
> > +    if (!reset) {
> > +        glue(load_symbols, SZ)(&ehdr, fd, must_swab, clear_lsb);
> > +    }
> >
> >     size = ehdr.e_phnum * sizeof(phdr[0]);
> >     lseek(fd, ehdr.e_phoff, SEEK_SET);
> >
> > Do you think it is reasonable?
> 
> I think semantically we want to only load the symbols the first time we
> load the binary (read: not on reset), yes. Where does the reset variable
> you're using there come from?
> 
> 
> Alex
> 
> >
> > Best Regards,
> > Olivia
> >
> >> -----Original Message-----
> >> From: Alexander Graf [mailto:agraf@suse.de]
> >> Sent: Thursday, November 22, 2012 2:42 AM
> >> To: Stuart Yoder
> >> Cc: Yin Olivia-R63875; qemu-devel@nongnu.org; qemu-ppc@nongnu.org
> >> Subject: Re: [Qemu-devel] [RFC PATCH v5 4/4] free the memory malloced
> >> by
> >> load_at()
> >>
> >> On 11/21/2012 07:39 PM, Stuart Yoder wrote:
> >>> On Wed, Nov 21, 2012 at 8:38 AM, Olivia
> >>> Yin<hong-hua.yin@freescale.com>
> >> wrote:
> >>>> Signed-off-by: Olivia Yin<hong-hua.yin@freescale.com>
> >>>> ---
> >>>>  hw/elf_ops.h |    2 ++
> >>>>  1 files changed, 2 insertions(+), 0 deletions(-)
> >>>>
> >>>> diff --git a/hw/elf_ops.h b/hw/elf_ops.h index b346861..9c76a75
> >>>> 100644
> >>>> --- a/hw/elf_ops.h
> >>>> +++ b/hw/elf_ops.h
> >>>> @@ -178,6 +178,8 @@ static int glue(load_symbols, SZ)(struct elfhdr
> >> *ehdr, int fd, int must_swab,
> >>>>      s->disas_strtab = str;
> >>>>      s->next = syminfos;
> >>>>      syminfos = s;
> >>>> +    g_free(syms);
> >>>> +    g_free(str);
> >>>>      g_free(shdr_table);
> >>>>      return 0;
> >>>>   fail:
> >>> Olivia, as Alex pointed out there are references to syms and str in
> >>> the struct "s"....so you can't just free those I don't think.
> >>>
> >>> The problem that leaves us with is that on every reset when we call
> >>> load_elf() that we re-load and re-malloc space for the symbols.
> >>>
> >>> I think the solution may be to factor out the call to load_symbols()
> >>> from load_elf().   It looks like what load_symbols does in the end is
> >>> set the variable syminfos to point to the loaded symbol info.
> >>>
> >>> If you factor load_symbols() out then in load_elf_32/64() you would
> >>> do something like:
> >>>       elf_phy_loader_32/64()
> >>>       load_symbols_32/64().
> >>>
> >>> We don't need to be reloading symbols on every reset.
> >>>
> >>> Alex, does that make sense?
> >>
> >> We can also mandate the caller of load_symbols to free the respective
> >> data :)
> >>
> >>
> >> Alex
> >>
> >
> >
>