Patchwork [RFC,v4,2/3] use uimage_reset to reload uimage

login
register
mail settings
Submitter Olivia Yin
Date Nov. 14, 2012, 1:28 p.m.
Message ID <1352899732-1197-3-git-send-email-hong-hua.yin@freescale.com>
Download mbox | patch
Permalink /patch/198924/
State New
Headers show

Comments

Olivia Yin - Nov. 14, 2012, 1:28 p.m.
Signed-off-by: Olivia Yin <hong-hua.yin@freescale.com>
---
 hw/loader.c |   64 ++++++++++++++++++++++++++++++++++++++++++----------------
 1 files changed, 46 insertions(+), 18 deletions(-)
Stuart Yoder - Nov. 15, 2012, 8:46 p.m.
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
Yin Olivia-R63875 - Nov. 16, 2012, 6:11 a.m.
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
Alexander Graf - Nov. 19, 2012, 11:26 a.m.
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
> 
> 
>

Patch

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