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:address@hidden
> Sent: Friday, November 16, 2012 4:46 AM
> To: Yin Olivia-R63875
> Cc: address@hidden; address@hidden
> 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 <address@hidden>
> wrote:
> > Signed-off-by: Olivia Yin <address@hidden>
> > ---
> >  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