[Qemu-devel] [RFC PATCH v4 0/3] reload kernel/initrd/elf images when reset

[Qemu-devel] [RFC PATCH v4 0/3] reload kernel/initrd/elf images when reset

[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 (3):
  use image_file_reset to reload initrd image
  use uimage_reset to reload uimage
  use elf_reset to reload elf image

 elf.h        |   10 ++++++
 hw/elf_ops.h |  101 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++
 hw/loader.c  |  103 +++++++++++++++++++++++++++++++++++++++++++++++----------
 hw/loader.h  |    7 ++++
 4 files changed, 203 insertions(+), 18 deletions(-)
---

v4:
 uImage/vmlinux/ramdisk had been verified on PowerPC platforms.
Issue: 
 It seemed that the patch 3/3 could not free the memory for phdr, and the memory map of QEMU will increase 1392K every time.
 If free phdr, system_reset will cause segmentation fault.

[Qemu-devel] [RFC PATCH v4 1/3] 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 v4 2/3] use uimage_reset to reload uimage

[Olivia Yin]

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

diff --git a/hw/loader.c b/hw/loader.c
index a8a0a09..1a909d0 100644
--- a/hw/loader.c
+++ b/hw/loader.c
@@ -55,6 +55,8 @@
 #include <zlib.h>
 
 static int roms_loaded;
+static int kernel_loaded;
+static int *is_linux;
 
 /* return the size or -1 if error */
 int get_image_size(const char *filename)
@@ -472,15 +474,14 @@ 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, uint8_t **data,
+                                  hwaddr *loadaddr, int *is_linux)
 {
     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);
@@ -521,10 +522,9 @@ int load_uimage(const char *filename, hwaddr *ep,
             *is_linux = 0;
     }
 
-    *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,9 @@ 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 (!kernel_loaded) {
+        rom_add_blob_fixed(filename, *data, hdr->ih_size, hdr->ih_load);
+    }
 
     if (loadaddr)
         *loadaddr = hdr->ih_load;
@@ -555,12 +557,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;
+    int size;
+
+    size = uimage_physical_loader(image->name, &data, &image->addr, is_linux);
+    cpu_physical_memory_write(image->addr, data, size);
+    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, &data, loadaddr, is_linux);
+    if (size > 0) {
+        kernel_loaded = 1;
+        g_free(data);
+        image = g_malloc0(sizeof(*image));
+        image->name = g_strdup(filename);
+        image->addr = *loadaddr;
+        qemu_register_reset(uimage_reset, image);
+    }
+    return size;
+}
+
 /*
  * Functions for reboot-persistent memory regions.
  *  - used for vga bios and option roms.
@@ -708,11 +739,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

[Qemu-devel] [RFC PATCH v4 3/3] 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 |  101 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 111 insertions(+), 0 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..89654bb 100644
--- a/hw/elf_ops.h
+++ b/hw/elf_ops.h
@@ -187,6 +187,95 @@ static int glue(load_symbols, SZ)(struct elfhdr *ehdr, int fd, int must_swab,
     return -1;
 }
 
+static void glue(elf_reset, SZ)(void *opaque)
+{
+    ImageElf *elf = opaque;
+    struct elfhdr ehdr;
+    struct elf_phdr *phdr = NULL, *ph;
+    int size, i, fd;
+    elf_word mem_size;
+    uint64_t addr;
+    uint8_t *data = NULL;
+
+    fd = open(elf->name, O_RDONLY | O_BINARY);
+    if (read(fd, &ehdr, sizeof(ehdr)) != sizeof(ehdr))
+        goto fail;
+    if (elf->swab) {
+        glue(bswap_ehdr, SZ)(&ehdr);
+    }
+
+    switch (elf->machine) {
+        case EM_PPC64:
+            if (EM_PPC64 != ehdr.e_machine)
+                if (EM_PPC != ehdr.e_machine)
+                    goto fail;
+            break;
+        case EM_X86_64:
+            if (EM_X86_64 != ehdr.e_machine)
+                if (EM_386 != ehdr.e_machine)
+                    goto fail;
+            break;
+        case EM_MICROBLAZE:
+            if (EM_MICROBLAZE != ehdr.e_machine)
+                if (EM_MICROBLAZE_OLD != ehdr.e_machine)
+                    goto fail;
+            break;
+        default:
+            if (elf->machine != ehdr.e_machine)
+                goto fail;
+    }
+
+    glue(load_symbols, SZ)(&ehdr, fd, elf->swab, elf->lsb);
+
+    size = ehdr.e_phnum * sizeof(phdr[0]);
+    lseek(fd, ehdr.e_phoff, SEEK_SET);
+    phdr = g_malloc0(size);
+    if (!phdr)
+        goto fail;
+    if (read(fd, phdr, size) != size)
+        goto fail;
+    if (elf->swab) {
+        for(i = 0; i < ehdr.e_phnum; i++) {
+            ph = &phdr[i];
+            glue(bswap_phdr, SZ)(ph);
+        }
+    }
+
+    for(i = 0; i < ehdr.e_phnum; i++) {
+        ph = &phdr[i];
+        if (ph->p_type == PT_LOAD) {
+            mem_size = ph->p_memsz;
+            /* XXX: avoid allocating */
+            data = g_malloc0(mem_size);
+            if (ph->p_filesz > 0) {
+                if (lseek(fd, ph->p_offset, SEEK_SET) < 0)
+                    goto fail;
+                if (read(fd, data, ph->p_filesz) != ph->p_filesz)
+                    goto fail;
+            }
+            /* address_offset is hack for kernel images that are
+               linked at the wrong physical address.  */
+            if (elf->fn) {
+                addr = elf->fn(elf->opaque, ph->p_paddr);
+            } else {
+                addr = ph->p_paddr;
+            }
+
+            cpu_physical_memory_write(addr, data, mem_size);
+
+            g_free(data);
+            data = NULL;
+        }
+    }
+    close(fd);
+    g_free(phdr);
+    phdr = NULL;
+ fail:
+    close(fd);
+    g_free(data);
+    g_free(phdr);
+}
+
 static int glue(load_elf, SZ)(const char *name, int fd,
                               uint64_t (*translate_fn)(void *, uint64_t),
                               void *translate_opaque,
@@ -293,6 +382,18 @@ static int glue(load_elf, SZ)(const char *name, int fd,
             data = NULL;
         }
     }
+
+    ImageElf *elf;
+    elf = g_malloc0(sizeof(*elf));
+    elf->name = g_strdup(name);
+    elf->fn = translate_fn;
+    elf->opaque = translate_opaque;
+    elf->swab = must_swab;
+    elf->machine = elf_machine;
+    elf->lsb = clear_lsb;
+
+    qemu_register_reset(glue(elf_reset, SZ), elf);
+
     g_free(phdr);
     if (lowaddr)
         *lowaddr = (uint64_t)(elf_sword)low;
-- 
1.7.1

Re: [Qemu-devel] [RFC PATCH v4 0/3] reload kernel/initrd/elf images when reset

[Yin Olivia-R63875]

I test loading the same images on X86/PowerPC platforms.

Size of image files:
 3432K    uImage
80948K   vmlinux
25180K   ramdisk

Test 1) X86
$ qemu-system-ppc -M mpc8544ds -cpu MPC8544 -kernel vmlinux -initrd ramdisk -m 256M -nographic
$ qemu-system-ppc -M mpc8544ds -cpu MPC8544 -kernel uImage -initrd ramdisk -m 256M -nographic

------------------------------------------------------------------
				Pmap of QEMU on X86
------------------------------------------------------------------
					|	vmlinux	|	uImage	
------------------------------------------------------------------
			| boot	|	500044K	|	498484K
Upstream		| reset 1	|-	500176K	|-	498616K	
			| reset 2	|	500176K	|	498616K	
------------------------------------------------------------------
			| boot	|	476000K	|	469440K
			| reset 1	|-	495640K	|-	491472K
			| reset 2	|	497024K	|	491472K
Loader patches	| reset 3	|	498412K	|	491472K
			| reset 4	|	499796K	|	491472K
			| reset 5	|	501184K	|	491472K
			| reset 6	|	502568K	|	491472K
------------------------------------------------------------------
	Saving			|	  4536K	|	  7144K
------------------------------------------------------------------

Test 2) Freescale MPC8572DS (e500v2 core) board
# qemu-system-ppc -enable-kvm -m 256 -nographic -M mpc8544ds -kernel vmlinux -initrd ramdisk -append "root=/dev/ram rw loglevel=7 console=ttyS0,115200" -serial tcp::4445,server -net nic
# qemu-system-ppc -enable-kvm -m 256 -nographic -M mpc8544ds -kernel uImage -initrd ramdisk -append "root=/dev/ram rw loglevel=7 console=ttyS0,115200" -serial tcp::4445,server -net nic

------------------------------------------------------------------
			Pmap of QEMU on MPC8472DS board
------------------------------------------------------------------
					|	vmlinux	|	uImage	
------------------------------------------------------------------
			| pre boot	|	 16240K	|	 16240K
			| boot	|	322092K	|	320500K
Upstream		| reset 1	|-	322092K	|-	320500K	
			| reset 2	|	322092K	|	320500K	
------------------------------------------------------------------
			| pre boot	|	 16244K	|	 16244K
			| boot	|	291476K	|	288252K
			| reset 1	|-	292868K	|-	288252K
Loader patches	| reset 2	|	294260K	|	288252K
			| reset 3	|	295652K	|	288252K
			| reset 4	|	297044K	|	288252K
			| reset 5	|	298436K	|	288252K
			| reset 6	|	299828K	|	288252K
------------------------------------------------------------------
	Saving			|	 29224K	|	 32248K
------------------------------------------------------------------


Best Regards,
Olivia

> -----Original Message-----
> From: Yin Olivia-R63875
> Sent: Wednesday, November 14, 2012 9:29 PM
> To: qemu-devel@nongnu.org; qemu-ppc@nongnu.org
> Cc: Yin Olivia-R63875
> Subject: [RFC PATCH v4 0/3] reload kernel/initrd/elf images when reset
> 
> 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 (3):
>   use image_file_reset to reload initrd image
>   use uimage_reset to reload uimage
>   use elf_reset to reload elf image
> 
>  elf.h        |   10 ++++++
>  hw/elf_ops.h |  101
> ++++++++++++++++++++++++++++++++++++++++++++++++++++++++
>  hw/loader.c  |  103 +++++++++++++++++++++++++++++++++++++++++++++++-----
> -----
>  hw/loader.h  |    7 ++++
>  4 files changed, 203 insertions(+), 18 deletions(-)
> ---
> 
> v4:
>  uImage/vmlinux/ramdisk had been verified on PowerPC platforms.
> Issue:
>  It seemed that the patch 3/3 could not free the memory for phdr, and the
> memory map of QEMU will increase 1392K every time.
>  If free phdr, system_reset will cause segmentation fault.

Re: [Qemu-devel] [RFC PATCH v4 1/3] use image_file_reset to reload initrd image

[Stuart Yoder]

>  /* 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);

You need to remove the call to rom_add_file_fixed(), no?

Stuart

Re: [Qemu-devel] [RFC PATCH v4 2/3] use uimage_reset to reload uimage

[Stuart Yoder]

On Wed, Nov 14, 2012 at 2:28 PM, Olivia Yin <hong-hua.yin@freescale.com> wrote:
> Signed-off-by: Olivia Yin <hong-hua.yin@freescale.com>
> ---
>  hw/loader.c |   64 ++++++++++++++++++++++++++++++++++++++++++----------------
>  1 files changed, 46 insertions(+), 18 deletions(-)
>
> diff --git a/hw/loader.c b/hw/loader.c
> index a8a0a09..1a909d0 100644
> --- a/hw/loader.c
> +++ b/hw/loader.c
> @@ -55,6 +55,8 @@
>  #include <zlib.h>
>
>  static int roms_loaded;
> +static int kernel_loaded;
> +static int *is_linux;

Why do we need these 2 new variables?

>  /* return the size or -1 if error */
>  int get_image_size(const char *filename)
> @@ -472,15 +474,14 @@ 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, uint8_t **data,
> +                                  hwaddr *loadaddr, int *is_linux)
>  {
>      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);
> @@ -521,10 +522,9 @@ int load_uimage(const char *filename, hwaddr *ep,
>              *is_linux = 0;
>      }
>
> -    *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,9 @@ 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 (!kernel_loaded) {
> +        rom_add_blob_fixed(filename, *data, hdr->ih_size, hdr->ih_load);
> +    }

Why do we need to keep the rom_add_blob_fixed() call?  I thought the
point of this was to remove adding roms.

>      if (loadaddr)
>          *loadaddr = hdr->ih_load;
> @@ -555,12 +557,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;
> +    int size;
> +
> +    size = uimage_physical_loader(image->name, &data, &image->addr, is_linux);

Not sure why we need is_linux here.  I think you should just set that
parameter to NULL.
Nothing will look at is_linux.

Stuart

Re: [Qemu-devel] [RFC PATCH v4 1/3] use image_file_reset to reload initrd image

[Yin Olivia-R63875]

Hi Stuart,

I want to keep the Memory Region of rom which will be inserted to the queue roms.
And then we can use "info roms" command to check the rom information.

ROM images must be loaded at startup, so rom_add_* functions could be called only
one time before all the reset handlers.

Exactly all the memory malloced to rom->data will be free when rom_reset() no matter
it is rom (rom->isrom = 1) or ram (rom->isrom =0).
@@ -708,11 +739,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;
     }
 }

For example, to load initrd image, both reset handlers (rom_reset() and image_file_reset() 
will write the image into the same physical address. So it will not malloc more memory.
	cpu_physical_memory_write_rom(rom->addr, rom->data, rom->romsize);
	cpu_physical_memory_write(image->addr, (uint8_t *)content, size);

The uImage is another case, I'll explain in the reply of patch 2/3.

Best Regards,
Olivia

> -----Original Message-----
> From: Stuart Yoder [mailto:b08248@gmail.com]
> Sent: Friday, November 16, 2012 3:52 AM
> To: Yin Olivia-R63875
> Cc: qemu-devel@nongnu.org; qemu-ppc@nongnu.org
> Subject: Re: [Qemu-devel] [RFC PATCH v4 1/3] use image_file_reset to
> reload initrd image
> 
> >  /* 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);
> 
> You need to remove the call to rom_add_file_fixed(), no?
> 
> Stuart

Re: [Qemu-devel] [RFC PATCH v4 2/3] use uimage_reset to reload uimage

[Yin Olivia-R63875]

Hi Stuart,

load_uimage() is a public function which is called by below files:
	hw/dummy_m68k.c:            kernel_size = load_uimage(kernel_filename, &entry, NULL, NULL);
	hw/an5206.c:        kernel_size = load_uimage(kernel_filename, &entry, NULL, NULL);
	hw/ppc440_bamboo.c:        success = load_uimage(kernel_filename, &entry, &loadaddr, NULL);
	hw/openrisc_sim.c:            kernel_size = load_uimage(kernel_filename, entry, NULL, NULL);
	hw/ppc/e500.c:        kernel_size = load_uimage(params->kernel_filename, &entry, &loadaddr, NULL);
	hw/arm_boot.c:        kernel_size = load_uimage(info->kernel_filename, &entry, NULL, &is_linux);
	hw/microblaze_boot.c:            kernel_size = load_uimage(kernel_filename, &uentry, &loadaddr, 0);
	hw/mcf5208.c:        kernel_size = load_uimage(kernel_filename, &entry, NULL, NULL);

Most value of *is_linux is NULL, only arm_boot.c will check the value of is_linux.
I just define an static variable is_linux other than include it into structure ImageFile.

To define static variable kernel_loaded because function uimage_physical_loader() will 
be called twice. 
	1) load_uimage() which will run only one time when startup.
	2) uimage_reset() which will run once reset, so it maybe run many times.
Since "ROM images must be loaded at startup", it means rom_add_*() should not be called by any reset handlers.
So I use variable kernel_loaded to decide the booting is startup or reset.


Best Regards,
Olivia


> -----Original Message-----
> From: Stuart Yoder [mailto:b08248@gmail.com]
> Sent: Friday, November 16, 2012 4:46 AM
> To: Yin Olivia-R63875
> Cc: qemu-devel@nongnu.org; qemu-ppc@nongnu.org
> Subject: Re: [Qemu-devel] [RFC PATCH v4 2/3] use uimage_reset to reload
> uimage
> 
> On Wed, Nov 14, 2012 at 2:28 PM, Olivia Yin <hong-hua.yin@freescale.com>
> wrote:
> > Signed-off-by: Olivia Yin <hong-hua.yin@freescale.com>
> > ---
> >  hw/loader.c |   64 ++++++++++++++++++++++++++++++++++++++++++---------
> -------
> >  1 files changed, 46 insertions(+), 18 deletions(-)
> >
> > diff --git a/hw/loader.c b/hw/loader.c index a8a0a09..1a909d0 100644
> > --- a/hw/loader.c
> > +++ b/hw/loader.c
> > @@ -55,6 +55,8 @@
> >  #include <zlib.h>
> >
> >  static int roms_loaded;
> > +static int kernel_loaded;
> > +static int *is_linux;
> 
> Why do we need these 2 new variables?
> 
> >  /* return the size or -1 if error */
> >  int get_image_size(const char *filename) @@ -472,15 +474,14 @@ 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, uint8_t **data,
> > +                                  hwaddr *loadaddr, int *is_linux)
> >  {
> >      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); @@ -521,10 +522,9 @@
> > int load_uimage(const char *filename, hwaddr *ep,
> >              *is_linux = 0;
> >      }
> >
> > -    *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,9 @@ 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 (!kernel_loaded) {
> > +        rom_add_blob_fixed(filename, *data, hdr->ih_size, hdr-
> >ih_load);
> > +    }
> 
> Why do we need to keep the rom_add_blob_fixed() call?  I thought the
> point of this was to remove adding roms.
> 
> >      if (loadaddr)
> >          *loadaddr = hdr->ih_load;
> > @@ -555,12 +557,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;
> > +    int size;
> > +
> > +    size = uimage_physical_loader(image->name, &data, &image->addr,
> > + is_linux);
> 
> Not sure why we need is_linux here.  I think you should just set that
> parameter to NULL.
> Nothing will look at is_linux.
> 
> Stuart

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

[Blue Swirl]

On Wed, Nov 14, 2012 at 1:28 PM, Olivia Yin <hong-hua.yin@freescale.com> wrote:
> Signed-off-by: Olivia Yin <hong-hua.yin@freescale.com>
> ---
>  elf.h        |   10 ++++++
>  hw/elf_ops.h |  101 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
>  2 files changed, 111 insertions(+), 0 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..89654bb 100644
> --- a/hw/elf_ops.h
> +++ b/hw/elf_ops.h
> @@ -187,6 +187,95 @@ static int glue(load_symbols, SZ)(struct elfhdr *ehdr, int fd, int must_swab,
>      return -1;
>  }
>
> +static void glue(elf_reset, SZ)(void *opaque)
> +{
> +    ImageElf *elf = opaque;
> +    struct elfhdr ehdr;
> +    struct elf_phdr *phdr = NULL, *ph;
> +    int size, i, fd;
> +    elf_word mem_size;
> +    uint64_t addr;
> +    uint8_t *data = NULL;
> +
> +    fd = open(elf->name, O_RDONLY | O_BINARY);
> +    if (read(fd, &ehdr, sizeof(ehdr)) != sizeof(ehdr))

Please add braces and check your patches with checkpatch.pl.

> +        goto fail;
> +    if (elf->swab) {
> +        glue(bswap_ehdr, SZ)(&ehdr);
> +    }
> +
> +    switch (elf->machine) {
> +        case EM_PPC64:
> +            if (EM_PPC64 != ehdr.e_machine)
> +                if (EM_PPC != ehdr.e_machine)
> +                    goto fail;
> +            break;
> +        case EM_X86_64:
> +            if (EM_X86_64 != ehdr.e_machine)
> +                if (EM_386 != ehdr.e_machine)
> +                    goto fail;
> +            break;
> +        case EM_MICROBLAZE:
> +            if (EM_MICROBLAZE != ehdr.e_machine)
> +                if (EM_MICROBLAZE_OLD != ehdr.e_machine)
> +                    goto fail;
> +            break;
> +        default:
> +            if (elf->machine != ehdr.e_machine)
> +                goto fail;
> +    }
> +
> +    glue(load_symbols, SZ)(&ehdr, fd, elf->swab, elf->lsb);
> +
> +    size = ehdr.e_phnum * sizeof(phdr[0]);
> +    lseek(fd, ehdr.e_phoff, SEEK_SET);
> +    phdr = g_malloc0(size);
> +    if (!phdr)
> +        goto fail;
> +    if (read(fd, phdr, size) != size)
> +        goto fail;
> +    if (elf->swab) {
> +        for(i = 0; i < ehdr.e_phnum; i++) {
> +            ph = &phdr[i];
> +            glue(bswap_phdr, SZ)(ph);
> +        }
> +    }
> +
> +    for(i = 0; i < ehdr.e_phnum; i++) {
> +        ph = &phdr[i];
> +        if (ph->p_type == PT_LOAD) {
> +            mem_size = ph->p_memsz;
> +            /* XXX: avoid allocating */
> +            data = g_malloc0(mem_size);
> +            if (ph->p_filesz > 0) {
> +                if (lseek(fd, ph->p_offset, SEEK_SET) < 0)
> +                    goto fail;
> +                if (read(fd, data, ph->p_filesz) != ph->p_filesz)
> +                    goto fail;
> +            }
> +            /* address_offset is hack for kernel images that are
> +               linked at the wrong physical address.  */
> +            if (elf->fn) {
> +                addr = elf->fn(elf->opaque, ph->p_paddr);
> +            } else {
> +                addr = ph->p_paddr;
> +            }
> +
> +            cpu_physical_memory_write(addr, data, mem_size);
> +
> +            g_free(data);
> +            data = NULL;
> +        }
> +    }
> +    close(fd);
> +    g_free(phdr);
> +    phdr = NULL;
> + fail:
> +    close(fd);
> +    g_free(data);
> +    g_free(phdr);
> +}
> +
>  static int glue(load_elf, SZ)(const char *name, int fd,
>                                uint64_t (*translate_fn)(void *, uint64_t),
>                                void *translate_opaque,
> @@ -293,6 +382,18 @@ static int glue(load_elf, SZ)(const char *name, int fd,
>              data = NULL;
>          }
>      }
> +
> +    ImageElf *elf;
> +    elf = g_malloc0(sizeof(*elf));
> +    elf->name = g_strdup(name);
> +    elf->fn = translate_fn;
> +    elf->opaque = translate_opaque;
> +    elf->swab = must_swab;
> +    elf->machine = elf_machine;
> +    elf->lsb = clear_lsb;
> +
> +    qemu_register_reset(glue(elf_reset, SZ), elf);
> +
>      g_free(phdr);
>      if (lowaddr)
>          *lowaddr = (uint64_t)(elf_sword)low;
> --
> 1.7.1
>
>
>

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

[Alexander Graf]

On 16.11.2012, at 07:11, Yin Olivia-R63875 wrote:

> Hi Stuart,
> 
> load_uimage() is a public function which is called by below files:
> 	hw/dummy_m68k.c:            kernel_size = load_uimage(kernel_filename, &entry, NULL, NULL);
> 	hw/an5206.c:        kernel_size = load_uimage(kernel_filename, &entry, NULL, NULL);
> 	hw/ppc440_bamboo.c:        success = load_uimage(kernel_filename, &entry, &loadaddr, NULL);
> 	hw/openrisc_sim.c:            kernel_size = load_uimage(kernel_filename, entry, NULL, NULL);
> 	hw/ppc/e500.c:        kernel_size = load_uimage(params->kernel_filename, &entry, &loadaddr, NULL);
> 	hw/arm_boot.c:        kernel_size = load_uimage(info->kernel_filename, &entry, NULL, &is_linux);
> 	hw/microblaze_boot.c:            kernel_size = load_uimage(kernel_filename, &uentry, &loadaddr, 0);
> 	hw/mcf5208.c:        kernel_size = load_uimage(kernel_filename, &entry, NULL, NULL);
> 
> Most value of *is_linux is NULL, only arm_boot.c will check the value of is_linux.
> I just define an static variable is_linux other than include it into structure ImageFile.

Just load the uImage once on load_uimage, so that you can populate all the passed down return variables (entry, loadaddr, is_linux). No need for a global here.

> To define static variable kernel_loaded because function uimage_physical_loader() will 
> be called twice. 
> 	1) load_uimage() which will run only one time when startup.
> 	2) uimage_reset() which will run once reset, so it maybe run many times.
> Since "ROM images must be loaded at startup", it means rom_add_*() should not be called by any reset handlers.
> So I use variable kernel_loaded to decide the booting is startup or reset.

Pass it as a function argument then?


Alex

> 
> 
> Best Regards,
> Olivia
> 
> 
>> -----Original Message-----
>> From: Stuart Yoder [mailto:b08248@gmail.com]
>> Sent: Friday, November 16, 2012 4:46 AM
>> To: Yin Olivia-R63875
>> Cc: qemu-devel@nongnu.org; qemu-ppc@nongnu.org
>> Subject: Re: [Qemu-devel] [RFC PATCH v4 2/3] use uimage_reset to reload
>> uimage
>> 
>> On Wed, Nov 14, 2012 at 2:28 PM, Olivia Yin <hong-hua.yin@freescale.com>
>> wrote:
>>> Signed-off-by: Olivia Yin <hong-hua.yin@freescale.com>
>>> ---
>>> hw/loader.c |   64 ++++++++++++++++++++++++++++++++++++++++++---------
>> -------
>>> 1 files changed, 46 insertions(+), 18 deletions(-)
>>> 
>>> diff --git a/hw/loader.c b/hw/loader.c index a8a0a09..1a909d0 100644
>>> --- a/hw/loader.c
>>> +++ b/hw/loader.c
>>> @@ -55,6 +55,8 @@
>>> #include <zlib.h>
>>> 
>>> static int roms_loaded;
>>> +static int kernel_loaded;
>>> +static int *is_linux;
>> 
>> Why do we need these 2 new variables?
>> 
>>> /* return the size or -1 if error */
>>> int get_image_size(const char *filename) @@ -472,15 +474,14 @@ 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, uint8_t **data,
>>> +                                  hwaddr *loadaddr, int *is_linux)
>>> {
>>>     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); @@ -521,10 +522,9 @@
>>> int load_uimage(const char *filename, hwaddr *ep,
>>>             *is_linux = 0;
>>>     }
>>> 
>>> -    *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,9 @@ 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 (!kernel_loaded) {
>>> +        rom_add_blob_fixed(filename, *data, hdr->ih_size, hdr-
>>> ih_load);
>>> +    }
>> 
>> Why do we need to keep the rom_add_blob_fixed() call?  I thought the
>> point of this was to remove adding roms.
>> 
>>>     if (loadaddr)
>>>         *loadaddr = hdr->ih_load;
>>> @@ -555,12 +557,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;
>>> +    int size;
>>> +
>>> +    size = uimage_physical_loader(image->name, &data, &image->addr,
>>> + is_linux);
>> 
>> Not sure why we need is_linux here.  I think you should just set that
>> parameter to NULL.
>> Nothing will look at is_linux.
>> 
>> Stuart
> 
> 
>