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

[Qemu-devel] [RFC PATCH v7 0/4] reload images from host rootfs on 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 uimage/initrd/vmlinux
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
  use uimage_reset to reload uimage
  use elf_reset to reload elf image
  free rom->data when rom_reset

 elf.h        |   10 ++++++
 hw/elf_ops.h |   44 +++++++++++++++++++++++---
 hw/loader.c  |   99 ++++++++++++++++++++++++++++++++++++++++++++++++----------
 hw/loader.h  |    9 +++++
 4 files changed, 140 insertions(+), 22 deletions(-)

v7: clean structure ImageFile

v6:
patch 3/4: load symbol only when startup

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.

v4: fix elf reload and register reset handler in hw/elf_ops.h

v3: reload elf

v2: Rename target_phys_addr_t to hwaddr

v1: support uimage/initrd reload

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

[Olivia Yin]

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

diff --git a/hw/loader.c b/hw/loader.c
index ba01ca6..f62aa7c 100644
--- a/hw/loader.c
+++ b/hw/loader.c
@@ -86,6 +86,24 @@ 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;
+
+    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 +131,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..9e76ebd 100644
--- a/hw/loader.h
+++ b/hw/loader.h
@@ -1,6 +1,12 @@
 #ifndef LOADER_H
 #define LOADER_H
 
+typedef struct ImageFile ImageFile;
+struct ImageFile {
+    char *name;
+    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 v7 2/4] use uimage_reset to reload uimage

[Olivia Yin]

Signed-off-by: Olivia Yin <hong-hua.yin@freescale.com>
---
 hw/loader.c |   57 +++++++++++++++++++++++++++++++++++++++++++++------------
 hw/loader.h |    3 +++
 2 files changed, 48 insertions(+), 12 deletions(-)

diff --git a/hw/loader.c b/hw/loader.c
index f62aa7c..151ef20 100644
--- a/hw/loader.c
+++ b/hw/loader.c
@@ -457,15 +457,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);
@@ -507,9 +507,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;
     }
@@ -519,11 +519,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");
@@ -532,7 +532,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;
@@ -540,12 +544,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.
diff --git a/hw/loader.h b/hw/loader.h
index 9e76ebd..97b3aab 100644
--- a/hw/loader.h
+++ b/hw/loader.h
@@ -5,6 +5,9 @@ typedef struct ImageFile ImageFile;
 struct ImageFile {
     char *name;
     hwaddr addr;
+    /* uimage */
+    hwaddr *ep;
+    int *is_linux; 
 };
 
 /* loader.c */
-- 
1.7.1

[Qemu-devel] [RFC PATCH v7 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 |   44 +++++++++++++++++++++++++++++++++++++++-----
 hw/loader.c  |   11 +++++++++++
 3 files changed, 60 insertions(+), 5 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..3ff8ffd 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) {
@@ -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);
@@ -280,8 +285,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 +313,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 151ef20..10e5726 100644
--- a/hw/loader.c
+++ b/hw/loader.c
@@ -342,13 +342,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 v7 4/4] free rom->data when rom_reset

[Olivia Yin]

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

diff --git a/hw/loader.c b/hw/loader.c
index 10e5726..c6f73bb 100644
--- a/hw/loader.c
+++ b/hw/loader.c
@@ -737,11 +737,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;
     }
 }
 
-- 
1.7.1

Re: [Qemu-devel] [Qemu-ppc] [RFC PATCH v7 3/4] use elf_reset to reload elf image

[Alexander Graf]

Missing patch description

On 29.11.2012, at 06:26, Olivia Yin wrote:

> Signed-off-by: Olivia Yin <hong-hua.yin@freescale.com>
> ---
> elf.h        |   10 ++++++++++
> hw/elf_ops.h |   44 +++++++++++++++++++++++++++++++++++++++-----
> hw/loader.c  |   11 +++++++++++
> 3 files changed, 60 insertions(+), 5 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..3ff8ffd 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) {
> @@ -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);
> @@ -280,8 +285,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 +313,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 151ef20..10e5726 100644
> --- a/hw/loader.c
> +++ b/hw/loader.c
> @@ -342,13 +342,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;

See my other mails on comments about these fields.


Alex

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

Re: [Qemu-devel] [Qemu-ppc] [RFC PATCH v7 2/4] use uimage_reset to reload uimage

[Alexander Graf]

On 29.11.2012, at 06:26, Olivia Yin wrote:

> Signed-off-by: Olivia Yin <hong-hua.yin@freescale.com>
> ---
> hw/loader.c |   57 +++++++++++++++++++++++++++++++++++++++++++++------------
> hw/loader.h |    3 +++
> 2 files changed, 48 insertions(+), 12 deletions(-)
> 
> diff --git a/hw/loader.c b/hw/loader.c
> index f62aa7c..151ef20 100644
> --- a/hw/loader.c
> +++ b/hw/loader.c
> @@ -457,15 +457,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);
> @@ -507,9 +507,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;
>     }
> @@ -519,11 +519,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");
> @@ -532,7 +532,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;
> @@ -540,12 +544,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;

This variable could reside on the stack and be gone by the time you use it.

> +        image->is_linux = is_linux;

Same as above.

> +        qemu_register_reset(uimage_reset, image);
> +    }
> +    return size;
> +}
> +
> /*
>  * Functions for reboot-persistent memory regions.
>  *  - used for vga bios and option roms.
> diff --git a/hw/loader.h b/hw/loader.h
> index 9e76ebd..97b3aab 100644
> --- a/hw/loader.h
> +++ b/hw/loader.h
> @@ -5,6 +5,9 @@ typedef struct ImageFile ImageFile;
> struct ImageFile {
>     char *name;
>     hwaddr addr;
> +    /* uimage */

Why not create a new struct for uImage style images? You could add that to the union I was laying out in my previous mail and have all fields that are special to uImage be in there. If there is no special field, sharing it with the raw image struct works too of course.


Alex

> +    hwaddr *ep;
> +    int *is_linux; 
> };
> 
> /* loader.c */
> -- 
> 1.7.1
> 
> 
> 

Re: [Qemu-devel] [Qemu-ppc] [RFC PATCH v7 1/4] use image_file_reset to reload initrd

[Alexander Graf]

Missing patch description

On 29.11.2012, at 06:26, Olivia Yin wrote:

> Signed-off-by: Olivia Yin <hong-hua.yin@freescale.com>
> ---
> hw/loader.c |   24 ++++++++++++++++++++++++
> hw/loader.h |    6 ++++++
> 2 files changed, 30 insertions(+), 0 deletions(-)
> 
> diff --git a/hw/loader.c b/hw/loader.c
> index ba01ca6..f62aa7c 100644
> --- a/hw/loader.c
> +++ b/hw/loader.c
> @@ -86,6 +86,24 @@ 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;
> +
> +    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);
> +    }

If I compare this function to rom_add_file(), it seems like there's a lot of logic missing.

> +}
> +
> /* read()-like version */
> ssize_t read_targphys(const char *name,
>                       int fd, hwaddr dst_addr, size_t nbytes)
> @@ -113,6 +131,12 @@ int load_image_targphys(const char *filename,

Up here is:

>     int size;
> 
>     size = get_image_size(filename);
>     if (size > max_sz) {
>         return -1;

which could be easily replaced by the glib helper function. It passes size and a proper return code already.

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

You now have 2 competing reset handlers: The rom based one and the ImageFile based one.

Why bother with the rom based one? Traditionally, having the rom caller buys you 2 things:

  1) Reset restoration of the rom data

This one is obsolete with the new dynamic loader.

  2) Listing of the rom region in "info roms"

You can replace the Rom struct in loader.c with a new struct that is more clever:

struct RomImage {
    union {
        ImageFile *image;
    } u;
    QTAILQ_ENTRY(RomImage) next;
}

which means that "info roms" can loop through these RomImage types and actually show things like

  ELF image /foo/bar.uImage
    ELF .text section hwaddr=0x... size=0x...
    ELF .data section hwaddr=0x... size=0x...
  Raw image /foo/initrd hwaddr=0x... size=0x...


Alex

>     }
>     return size;
> }
> diff --git a/hw/loader.h b/hw/loader.h
> index 26480ad..9e76ebd 100644
> --- a/hw/loader.h
> +++ b/hw/loader.h
> @@ -1,6 +1,12 @@
> #ifndef LOADER_H
> #define LOADER_H
> 
> +typedef struct ImageFile ImageFile;
> +struct ImageFile {
> +    char *name;
> +    hwaddr addr;
> +};
> +
> /* loader.c */
> int get_image_size(const char *filename);
> int load_image(const char *filename, uint8_t *addr); /* deprecated */
> -- 
> 1.7.1
> 
> 
> 

Re: [Qemu-devel] [Qemu-ppc] [RFC PATCH v7 1/4] use image_file_reset to reload initrd

[Yin Olivia-R63875]

> -----Original Message-----
> From: Alexander Graf [mailto:agraf@suse.de]
> Sent: Sunday, December 02, 2012 7:20 PM
> To: Yin Olivia-R63875
> Cc: qemu-devel@nongnu.org; qemu-ppc@nongnu.org
> Subject: Re: [Qemu-ppc] [RFC PATCH v7 1/4] use image_file_reset to reload
> initrd
> 
> Missing patch description
> 
> On 29.11.2012, at 06:26, Olivia Yin wrote:
> 
> > Signed-off-by: Olivia Yin <hong-hua.yin@freescale.com>
> > ---
> > hw/loader.c |   24 ++++++++++++++++++++++++
> > hw/loader.h |    6 ++++++
> > 2 files changed, 30 insertions(+), 0 deletions(-)
> >
> > diff --git a/hw/loader.c b/hw/loader.c index ba01ca6..f62aa7c 100644
> > --- a/hw/loader.c
> > +++ b/hw/loader.c
> > @@ -86,6 +86,24 @@ 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;
> > +
> > +    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);
> > +    }
> 
> If I compare this function to rom_add_file(), it seems like there's a lot
> of logic missing.
> 
> > +}
> > +
> > /* read()-like version */
> > ssize_t read_targphys(const char *name,
> >                       int fd, hwaddr dst_addr, size_t nbytes) @@
> > -113,6 +131,12 @@ int load_image_targphys(const char *filename,
> 
> Up here is:
> 
> >     int size;
> >
> >     size = get_image_size(filename);
> >     if (size > max_sz) {
> >         return -1;
> 
> which could be easily replaced by the glib helper function. It passes
> size and a proper return code already.

get_image_size() is a public function in QEMU.
	hw/palm.c:        rom_size = get_image_size(option_rom[0].name);
	hw/mips_fulong2e.c:        initrd_size = get_image_size (loaderparams.initrd_filename);
	hw/loader.c:int get_image_size(const char *filename)
	hw/loader.c:    size = get_image_size(filename);
	hw/multiboot.c:            mb_mod_length = get_image_size(initrd_filename);
	hw/pc.c:	initrd_size = get_image_size(initrd_filename);
	hw/mips_mipssim.c:        initrd_size = get_image_size (loaderparams.initrd_filename);
	hw/pc_sysfw.c:        bios_size = get_image_size(filename);
	hw/smbios.c:        int size = get_image_size(buf);
	hw/leon3.c:    bios_size = get_image_size(filename);
	hw/pci.c:    size = get_image_size(path);
	hw/ppc_prep.c:        bios_size = get_image_size(filename);
	hw/mips_r4k.c:        initrd_size = get_image_size (loaderparams.initrd_filename);
	hw/mips_r4k.c:        bios_size = get_image_size(filename);
	hw/alpha_dp264.c:            initrd_size = get_image_size(initrd_filename);
	hw/loader.h:int get_image_size(const char *filename);
	hw/mips_malta.c:        initrd_size = get_image_size (loaderparams.initrd_filename);
	hw/highbank.c:            uint32_t filesize = get_image_size(sysboot_filename);
	device_tree.c:    dt_size = get_image_size(filename_path);

Do you think I should replace get_image_fize() with g_file_get_contents()?
Or just revise get_image_size() function as below?
gsize get_image_size(const char *filename)
{
    gchar *content;
    gsize size;
    g_file_get_contents(image->name, &content, &size, &err);
    return size;
}

> >     }
> >     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);
> 
> You now have 2 competing reset handlers: The rom based one and the
> ImageFile based one.

OK. I'll remove rom_reset() since the rom->data could be written when rom_load_all().

> Why bother with the rom based one? Traditionally, having the rom caller
> buys you 2 things:
> 
>   1) Reset restoration of the rom data
> 
> This one is obsolete with the new dynamic loader.
> 
>   2) Listing of the rom region in "info roms"
> 
> You can replace the Rom struct in loader.c with a new struct that is more
> clever:
> 
> struct RomImage {
>     union {
>         ImageFile *image;
>     } u;
>     QTAILQ_ENTRY(RomImage) next;
> }
> 
> which means that "info roms" can loop through these RomImage types and
> actually show things like
> 
>   ELF image /foo/bar.uImage
>     ELF .text section hwaddr=0x... size=0x...
>     ELF .data section hwaddr=0x... size=0x...
>   Raw image /foo/initrd hwaddr=0x... size=0x...

Exactly ehdr.e_phnum = 3 and the only first one is PT_LOAD type and will be added into roms and written into memory.
    for(i = 0; i < ehdr.e_phnum; i++) {
        ph = &phdr[i];
        if (ph->p_type == PT_LOAD) {

> 
> Alex
> 
> >     }
> >     return size;
> > }
> > diff --git a/hw/loader.h b/hw/loader.h index 26480ad..9e76ebd 100644
> > --- a/hw/loader.h
> > +++ b/hw/loader.h
> > @@ -1,6 +1,12 @@
> > #ifndef LOADER_H
> > #define LOADER_H
> >
> > +typedef struct ImageFile ImageFile;
> > +struct ImageFile {
> > +    char *name;
> > +    hwaddr addr;
> > +};
> > +
> > /* loader.c */
> > int get_image_size(const char *filename); int load_image(const char
> > *filename, uint8_t *addr); /* deprecated */
> > --
> > 1.7.1
> >
> >
> >
> 

Re: [Qemu-devel] [Qemu-ppc] [RFC PATCH v7 1/4] use image_file_reset to reload initrd

[Alexander Graf]

On 06.12.2012, at 05:11, Yin Olivia-R63875 wrote:

> 
> 
>> -----Original Message-----
>> From: Alexander Graf [mailto:agraf@suse.de]
>> Sent: Sunday, December 02, 2012 7:20 PM
>> To: Yin Olivia-R63875
>> Cc: qemu-devel@nongnu.org; qemu-ppc@nongnu.org
>> Subject: Re: [Qemu-ppc] [RFC PATCH v7 1/4] use image_file_reset to reload
>> initrd
>> 
>> Missing patch description
>> 
>> On 29.11.2012, at 06:26, Olivia Yin wrote:
>> 
>>> Signed-off-by: Olivia Yin <hong-hua.yin@freescale.com>
>>> ---
>>> hw/loader.c |   24 ++++++++++++++++++++++++
>>> hw/loader.h |    6 ++++++
>>> 2 files changed, 30 insertions(+), 0 deletions(-)
>>> 
>>> diff --git a/hw/loader.c b/hw/loader.c index ba01ca6..f62aa7c 100644
>>> --- a/hw/loader.c
>>> +++ b/hw/loader.c
>>> @@ -86,6 +86,24 @@ 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;
>>> +
>>> +    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);
>>> +    }
>> 
>> If I compare this function to rom_add_file(), it seems like there's a lot
>> of logic missing.
>> 
>>> +}
>>> +
>>> /* read()-like version */
>>> ssize_t read_targphys(const char *name,
>>>                      int fd, hwaddr dst_addr, size_t nbytes) @@
>>> -113,6 +131,12 @@ int load_image_targphys(const char *filename,
>> 
>> Up here is:
>> 
>>>    int size;
>>> 
>>>    size = get_image_size(filename);
>>>    if (size > max_sz) {
>>>        return -1;
>> 
>> which could be easily replaced by the glib helper function. It passes
>> size and a proper return code already.
> 
> get_image_size() is a public function in QEMU.
> 	hw/palm.c:        rom_size = get_image_size(option_rom[0].name);
> 	hw/mips_fulong2e.c:        initrd_size = get_image_size (loaderparams.initrd_filename);
> 	hw/loader.c:int get_image_size(const char *filename)
> 	hw/loader.c:    size = get_image_size(filename);
> 	hw/multiboot.c:            mb_mod_length = get_image_size(initrd_filename);
> 	hw/pc.c:	initrd_size = get_image_size(initrd_filename);
> 	hw/mips_mipssim.c:        initrd_size = get_image_size (loaderparams.initrd_filename);
> 	hw/pc_sysfw.c:        bios_size = get_image_size(filename);
> 	hw/smbios.c:        int size = get_image_size(buf);
> 	hw/leon3.c:    bios_size = get_image_size(filename);
> 	hw/pci.c:    size = get_image_size(path);
> 	hw/ppc_prep.c:        bios_size = get_image_size(filename);
> 	hw/mips_r4k.c:        initrd_size = get_image_size (loaderparams.initrd_filename);
> 	hw/mips_r4k.c:        bios_size = get_image_size(filename);
> 	hw/alpha_dp264.c:            initrd_size = get_image_size(initrd_filename);
> 	hw/loader.h:int get_image_size(const char *filename);
> 	hw/mips_malta.c:        initrd_size = get_image_size (loaderparams.initrd_filename);
> 	hw/highbank.c:            uint32_t filesize = get_image_size(sysboot_filename);
> 	device_tree.c:    dt_size = get_image_size(filename_path);
> 
> Do you think I should replace get_image_fize() with g_file_get_contents()?
> Or just revise get_image_size() function as below?
> gsize get_image_size(const char *filename)
> {
>    gchar *content;
>    gsize size;
>    g_file_get_contents(image->name, &content, &size, &err);
>    return size;
> }

That looks good, yes. Make sure to free it again ;).

> 
>>>    }
>>>    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);
>> 
>> You now have 2 competing reset handlers: The rom based one and the
>> ImageFile based one.
> 
> OK. I'll remove rom_reset() since the rom->data could be written when rom_load_all().
> 
>> Why bother with the rom based one? Traditionally, having the rom caller
>> buys you 2 things:
>> 
>>  1) Reset restoration of the rom data
>> 
>> This one is obsolete with the new dynamic loader.
>> 
>>  2) Listing of the rom region in "info roms"
>> 
>> You can replace the Rom struct in loader.c with a new struct that is more
>> clever:
>> 
>> struct RomImage {
>>    union {
>>        ImageFile *image;
>>    } u;
>>    QTAILQ_ENTRY(RomImage) next;
>> }
>> 
>> which means that "info roms" can loop through these RomImage types and
>> actually show things like
>> 
>>  ELF image /foo/bar.uImage
>>    ELF .text section hwaddr=0x... size=0x...
>>    ELF .data section hwaddr=0x... size=0x...
>>  Raw image /foo/initrd hwaddr=0x... size=0x...
> 
> Exactly ehdr.e_phnum = 3 and the only first one is PT_LOAD type and will be added into roms and written into memory.
>    for(i = 0; i < ehdr.e_phnum; i++) {
>        ph = &phdr[i];
>        if (ph->p_type == PT_LOAD) {

That depends on the image, no?


Alex