| Message ID | 1352899732-1197-3-git-send-email-hong-hua.yin@freescale.com |
|---|---|
| State | New |
| Headers | show |
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
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
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 > > >
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; } }
Signed-off-by: Olivia Yin <hong-hua.yin@freescale.com> --- hw/loader.c | 64 ++++++++++++++++++++++++++++++++++++++++++---------------- 1 files changed, 46 insertions(+), 18 deletions(-)