[Qemu-devel] [PATCH 1/2] extract file_load() function from rom_add_file() for reusing

[Qemu-devel] [PATCH 1/2] extract file_load() function from rom_add_file() for reusing

[Olivia Yin]

Sanity check in rom_add_file() could be reused by other image loaders.

Signed-off-by: Olivia Yin <hong-hua.yin@freescale.com>
---
This patch is based on branch 'ppc-next' of Alex's upstream QEMU repo:
http://repo.or.cz/r/qemu/agraf.git

 hw/loader.c |   61 +++++++++++++++++++++++++++++++---------------------------
 1 files changed, 33 insertions(+), 28 deletions(-)

diff --git a/hw/loader.c b/hw/loader.c
index 33acc2f..f2099b6 100644
--- a/hw/loader.c
+++ b/hw/loader.c
@@ -86,6 +86,36 @@ int load_image(const char *filename, uint8_t *addr)
     return size;
 }
 
+static int file_load(const char *file, uint8_t **data)
+{
+    int fd = -1;
+    ssize_t rc, size;
+
+    fd = open(file, O_RDONLY | O_BINARY);
+    if (fd == -1) {
+        fprintf(stderr, "Could not open file '%s': %s\n",
+                file, strerror(errno));
+        return -1;
+    }
+
+    size = lseek(fd, 0, SEEK_END);
+    *data = g_malloc0(size);
+    lseek(fd, 0, SEEK_SET);
+    rc = read(fd, *data, size);
+    if (rc != size) {
+        fprintf(stderr, "file %-20s: read error: rc=%zd (expected %zd)\n",
+                file, rc, size);
+        goto err;
+    }
+    close(fd);
+    return size;
+err:
+    if (fd != -1)
+        close(fd);
+    g_free(*data);
+    return -1;
+}
+
 /* read()-like version */
 ssize_t read_targphys(const char *name,
                       int fd, target_phys_addr_t dst_addr, size_t nbytes)
@@ -568,38 +598,22 @@ int rom_add_file(const char *file, const char *fw_dir,
                  target_phys_addr_t addr, int32_t bootindex)
 {
     Rom *rom;
-    int rc, fd = -1;
     char devpath[100];
 
     rom = g_malloc0(sizeof(*rom));
     rom->name = g_strdup(file);
+    rom->addr = addr;
     rom->path = qemu_find_file(QEMU_FILE_TYPE_BIOS, rom->name);
     if (rom->path == NULL) {
         rom->path = g_strdup(file);
     }
 
-    fd = open(rom->path, O_RDONLY | O_BINARY);
-    if (fd == -1) {
-        fprintf(stderr, "Could not open option rom '%s': %s\n",
-                rom->path, strerror(errno));
-        goto err;
-    }
-
     if (fw_dir) {
         rom->fw_dir  = g_strdup(fw_dir);
         rom->fw_file = g_strdup(file);
     }
-    rom->addr    = addr;
-    rom->romsize = lseek(fd, 0, SEEK_END);
-    rom->data    = g_malloc0(rom->romsize);
-    lseek(fd, 0, SEEK_SET);
-    rc = read(fd, rom->data, rom->romsize);
-    if (rc != rom->romsize) {
-        fprintf(stderr, "rom: file %-20s: read error: rc=%d (expected %zd)\n",
-                rom->name, rc, rom->romsize);
-        goto err;
-    }
-    close(fd);
+
+    rom->romsize = file_load(rom->path, &rom->data);
     rom_insert(rom);
     if (rom->fw_file && fw_cfg) {
         const char *basename;
@@ -621,15 +635,6 @@ int rom_add_file(const char *file, const char *fw_dir,
 
     add_boot_device_path(bootindex, NULL, devpath);
     return 0;
-
-err:
-    if (fd != -1)
-        close(fd);
-    g_free(rom->data);
-    g_free(rom->path);
-    g_free(rom->name);
-    g_free(rom);
-    return -1;
 }
 
 int rom_add_blob(const char *name, const void *blob, size_t len,
-- 
1.7.1

[Qemu-devel] [PATCH 2/2] register reset handlers to write images into memory

[Olivia Yin]

Instead of add rom blobs, this patch just write them directly to memory.

This patch registers reset handler uimage_reset() and image_file_reset()
which load images into RAM during initial bootup and VM reset.

Signed-off-by: Olivia Yin <hong-hua.yin@freescale.com>
---
This patch is based on branch 'ppc-next' of Alex's upstream QEMU repo:
http://repo.or.cz/r/qemu/agraf.git

 hw/loader.c |   81 +++++++++++++++++++++++++++++++++++++++++++----------------
 1 files changed, 59 insertions(+), 22 deletions(-)

diff --git a/hw/loader.c b/hw/loader.c
index f2099b6..3c97724 100644
--- a/hw/loader.c
+++ b/hw/loader.c
@@ -56,6 +56,12 @@
 
 static int roms_loaded;
 
+typedef struct ImageFile ImageFile;
+struct ImageFile {
+    char *name;
+    target_phys_addr_t addr;
+};
+
 /* return the size or -1 if error */
 int get_image_size(const char *filename)
 {
@@ -115,6 +121,17 @@ err:
     g_free(*data);
     return -1;
 }
+ 
+static void image_file_reset(void *opaque)
+{
+    ImageFile *image = opaque;
+    int size;
+    uint8_t *data = NULL;
+
+    size = file_load(image->name, &data);
+    cpu_physical_memory_rw(image->addr, data, size, 1);
+    g_free(data);
+}
 
 /* read()-like version */
 ssize_t read_targphys(const char *name,
@@ -142,7 +159,12 @@ int load_image_targphys(const char *filename,
         return -1;
     }
     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;
 }
@@ -463,15 +485,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, target_phys_addr_t *ep,
-                target_phys_addr_t *loadaddr, int *is_linux)
+/* write uimage into memory */
+static int uimage_physical_loader(const char *filename, uint8_t **data, 
+                                  target_phys_addr_t *loadaddr)
 {
     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);
@@ -504,18 +525,9 @@ int load_uimage(const char *filename, target_phys_addr_t *ep,
         goto out;
     }
 
-    /* TODO: Check CPU type.  */
-    if (is_linux) {
-        if (hdr->ih_os == IH_OS_LINUX)
-            *is_linux = 1;
-        else
-            *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;
     }
@@ -525,11 +537,11 @@ int load_uimage(const char *filename, target_phys_addr_t *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");
@@ -538,7 +550,6 @@ int load_uimage(const char *filename, target_phys_addr_t *ep,
         hdr->ih_size = bytes;
     }
 
-    rom_add_blob_fixed(filename, data, hdr->ih_size, hdr->ih_load);
 
     if (loadaddr)
         *loadaddr = hdr->ih_load;
@@ -546,12 +557,38 @@ int load_uimage(const char *filename, target_phys_addr_t *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);
+    cpu_physical_memory_rw(image->addr, data, size, 1);
+    g_free(data);
+}
+
+/* Load a U-Boot image.  */
+int load_uimage(const char *filename, target_phys_addr_t *ep,
+                target_phys_addr_t *loadaddr, int *is_linux)
+{
+    int size;
+    ImageFile *image;
+    uint8_t *data = NULL;
+
+    size= uimage_physical_loader(filename, &data, loadaddr);
+    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.
-- 
1.7.1

Re: [Qemu-devel] [PATCH 1/2] extract file_load() function from rom_add_file() for reusing

[Yin Olivia-R63875]

Hi Avi & Ben,

I've got feedback from qemu-ppc list and revised this patch according to Blue's comments.
Could you please give any comments? 

Best Regards,
Olivia

> -----Original Message-----
> From: Yin Olivia-R63875
> Sent: Tuesday, August 14, 2012 3:50 PM
> To: qemu-ppc@nongnu.org; qemu-devel@nongnu.org
> Cc: Yin Olivia-R63875
> Subject: [PATCH 1/2] extract file_load() function from rom_add_file() for
> reusing
> 
> Sanity check in rom_add_file() could be reused by other image loaders.
> 
> Signed-off-by: Olivia Yin <hong-hua.yin@freescale.com>
> ---
> This patch is based on branch 'ppc-next' of Alex's upstream QEMU repo:
> http://repo.or.cz/r/qemu/agraf.git
> 
>  hw/loader.c |   61 +++++++++++++++++++++++++++++++----------------------
> -----
>  1 files changed, 33 insertions(+), 28 deletions(-)
> 
> diff --git a/hw/loader.c b/hw/loader.c
> index 33acc2f..f2099b6 100644
> --- a/hw/loader.c
> +++ b/hw/loader.c
> @@ -86,6 +86,36 @@ int load_image(const char *filename, uint8_t *addr)
>      return size;
>  }
> 
> +static int file_load(const char *file, uint8_t **data) {
> +    int fd = -1;
> +    ssize_t rc, size;
> +
> +    fd = open(file, O_RDONLY | O_BINARY);
> +    if (fd == -1) {
> +        fprintf(stderr, "Could not open file '%s': %s\n",
> +                file, strerror(errno));
> +        return -1;
> +    }
> +
> +    size = lseek(fd, 0, SEEK_END);
> +    *data = g_malloc0(size);
> +    lseek(fd, 0, SEEK_SET);
> +    rc = read(fd, *data, size);
> +    if (rc != size) {
> +        fprintf(stderr, "file %-20s: read error: rc=%zd
> (expected %zd)\n",
> +                file, rc, size);
> +        goto err;
> +    }
> +    close(fd);
> +    return size;
> +err:
> +    if (fd != -1)
> +        close(fd);
> +    g_free(*data);
> +    return -1;
> +}
> +
>  /* read()-like version */
>  ssize_t read_targphys(const char *name,
>                        int fd, target_phys_addr_t dst_addr, size_t nbytes)
> @@ -568,38 +598,22 @@ int rom_add_file(const char *file, const char
> *fw_dir,
>                   target_phys_addr_t addr, int32_t bootindex)  {
>      Rom *rom;
> -    int rc, fd = -1;
>      char devpath[100];
> 
>      rom = g_malloc0(sizeof(*rom));
>      rom->name = g_strdup(file);
> +    rom->addr = addr;
>      rom->path = qemu_find_file(QEMU_FILE_TYPE_BIOS, rom->name);
>      if (rom->path == NULL) {
>          rom->path = g_strdup(file);
>      }
> 
> -    fd = open(rom->path, O_RDONLY | O_BINARY);
> -    if (fd == -1) {
> -        fprintf(stderr, "Could not open option rom '%s': %s\n",
> -                rom->path, strerror(errno));
> -        goto err;
> -    }
> -
>      if (fw_dir) {
>          rom->fw_dir  = g_strdup(fw_dir);
>          rom->fw_file = g_strdup(file);
>      }
> -    rom->addr    = addr;
> -    rom->romsize = lseek(fd, 0, SEEK_END);
> -    rom->data    = g_malloc0(rom->romsize);
> -    lseek(fd, 0, SEEK_SET);
> -    rc = read(fd, rom->data, rom->romsize);
> -    if (rc != rom->romsize) {
> -        fprintf(stderr, "rom: file %-20s: read error: rc=%d
> (expected %zd)\n",
> -                rom->name, rc, rom->romsize);
> -        goto err;
> -    }
> -    close(fd);
> +
> +    rom->romsize = file_load(rom->path, &rom->data);
>      rom_insert(rom);
>      if (rom->fw_file && fw_cfg) {
>          const char *basename;
> @@ -621,15 +635,6 @@ int rom_add_file(const char *file, const char
> *fw_dir,
> 
>      add_boot_device_path(bootindex, NULL, devpath);
>      return 0;
> -
> -err:
> -    if (fd != -1)
> -        close(fd);
> -    g_free(rom->data);
> -    g_free(rom->path);
> -    g_free(rom->name);
> -    g_free(rom);
> -    return -1;
>  }
> 
>  int rom_add_blob(const char *name, const void *blob, size_t len,
> --
> 1.7.1

Re: [Qemu-devel] [PATCH 1/2] extract file_load() function from rom_add_file() for reusing

[Avi Kivity]

On 08/16/2012 02:13 PM, Yin Olivia-R63875 wrote:
> Hi Avi & Ben,
> 
> I've got feedback from qemu-ppc list and revised this patch according to Blue's comments.
> Could you please give any comments? 


I don't really have any comments, those files are outside my area of
expertise.  They look okay to me, but that doesn't mean much as I'm not
familiar with ppc.

-- 
error compiling committee.c: too many arguments to function

Re: [Qemu-devel] [PATCH 1/2] extract file_load() function from rom_add_file() for reusing

[Peter Maydell]

On 14 August 2012 08:49, Olivia Yin <hong-hua.yin@freescale.com> wrote:
> Sanity check in rom_add_file() could be reused by other image loaders.
>
> Signed-off-by: Olivia Yin <hong-hua.yin@freescale.com>
> ---
> This patch is based on branch 'ppc-next' of Alex's upstream QEMU repo:
> http://repo.or.cz/r/qemu/agraf.git
>
>  hw/loader.c |   61 +++++++++++++++++++++++++++++++---------------------------
>  1 files changed, 33 insertions(+), 28 deletions(-)
>
> diff --git a/hw/loader.c b/hw/loader.c
> index 33acc2f..f2099b6 100644
> --- a/hw/loader.c
> +++ b/hw/loader.c
> @@ -86,6 +86,36 @@ int load_image(const char *filename, uint8_t *addr)
>      return size;
>  }
>
> +static int file_load(const char *file, uint8_t **data)
> +{
> +    int fd = -1;
> +    ssize_t rc, size;
> +
> +    fd = open(file, O_RDONLY | O_BINARY);
> +    if (fd == -1) {
> +        fprintf(stderr, "Could not open file '%s': %s\n",
> +                file, strerror(errno));
> +        return -1;
> +    }
> +
> +    size = lseek(fd, 0, SEEK_END);
> +    *data = g_malloc0(size);
> +    lseek(fd, 0, SEEK_SET);
> +    rc = read(fd, *data, size);
> +    if (rc != size) {
> +        fprintf(stderr, "file %-20s: read error: rc=%zd (expected %zd)\n",
> +                file, rc, size);
> +        goto err;
> +    }
> +    close(fd);
> +    return size;
> +err:
> +    if (fd != -1)
> +        close(fd);
> +    g_free(*data);
> +    return -1;
> +}

Isn't this function effectively a reimplementation of the glib
g_file_get_contents() function? It would probably be better to
just make the callers use that instead.

-- PMM

Re: [Qemu-devel] [PATCH 1/2] extract file_load() function from rom_add_file() for reusing

[Yin Olivia-R63875]

Hi Peter,

Thanks for the reminder. I'll update the second patch to use g_file_get_contents().

Best Regards,
Olivia

> -----Original Message-----
> From: Peter Maydell [mailto:peter.maydell@linaro.org]
> Sent: Thursday, August 16, 2012 7:36 PM
> To: Yin Olivia-R63875
> Cc: qemu-ppc@nongnu.org; qemu-devel@nongnu.org
> Subject: Re: [Qemu-devel] [PATCH 1/2] extract file_load() function from
> rom_add_file() for reusing
> 
> On 14 August 2012 08:49, Olivia Yin <hong-hua.yin@freescale.com> wrote:
> > Sanity check in rom_add_file() could be reused by other image loaders.
> >
> > Signed-off-by: Olivia Yin <hong-hua.yin@freescale.com>
> > ---
> > This patch is based on branch 'ppc-next' of Alex's upstream QEMU repo:
> > http://repo.or.cz/r/qemu/agraf.git
> >
> >  hw/loader.c |   61 +++++++++++++++++++++++++++++++--------------------
> -------
> >  1 files changed, 33 insertions(+), 28 deletions(-)
> >
> > diff --git a/hw/loader.c b/hw/loader.c index 33acc2f..f2099b6 100644
> > --- a/hw/loader.c
> > +++ b/hw/loader.c
> > @@ -86,6 +86,36 @@ int load_image(const char *filename, uint8_t *addr)
> >      return size;
> >  }
> >
> > +static int file_load(const char *file, uint8_t **data) {
> > +    int fd = -1;
> > +    ssize_t rc, size;
> > +
> > +    fd = open(file, O_RDONLY | O_BINARY);
> > +    if (fd == -1) {
> > +        fprintf(stderr, "Could not open file '%s': %s\n",
> > +                file, strerror(errno));
> > +        return -1;
> > +    }
> > +
> > +    size = lseek(fd, 0, SEEK_END);
> > +    *data = g_malloc0(size);
> > +    lseek(fd, 0, SEEK_SET);
> > +    rc = read(fd, *data, size);
> > +    if (rc != size) {
> > +        fprintf(stderr, "file %-20s: read error: rc=%zd
> (expected %zd)\n",
> > +                file, rc, size);
> > +        goto err;
> > +    }
> > +    close(fd);
> > +    return size;
> > +err:
> > +    if (fd != -1)
> > +        close(fd);
> > +    g_free(*data);
> > +    return -1;
> > +}
> 
> Isn't this function effectively a reimplementation of the glib
> g_file_get_contents() function? It would probably be better to just make
> the callers use that instead.
> 
> -- PMM

Re: [Qemu-devel] [PATCH 1/2] extract file_load() function from rom_add_file() for reusing

[Yin Olivia-R63875]

Hi Avi,

Thanks. Exactly the second patch is more important which saves the QEMU memory.

Best Regards,
Olivia

> -----Original Message-----
> From: Avi Kivity [mailto:avi@redhat.com]
> Sent: Thursday, August 16, 2012 7:23 PM
> To: Yin Olivia-R63875
> Cc: qemu-ppc@nongnu.org; qemu-devel@nongnu.org; blauwirbel@gmail.com;
> benh@kernel.crashing.org
> Subject: Re: [PATCH 1/2] extract file_load() function from rom_add_file()
> for reusing
> 
> On 08/16/2012 02:13 PM, Yin Olivia-R63875 wrote:
> > Hi Avi & Ben,
> >
> > I've got feedback from qemu-ppc list and revised this patch according
> to Blue's comments.
> > Could you please give any comments?
> 
> 
> I don't really have any comments, those files are outside my area of
> expertise.  They look okay to me, but that doesn't mean much as I'm not
> familiar with ppc.
> 
> --
> error compiling committee.c: too many arguments to function