[Qemu-devel] [PATCH v2] register reset handler to write image into memory

[Qemu-devel] [PATCH v2] register reset handler to write image 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.

v2: use g_file_get_content() to load load image file.

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 |   88 ++++++++++++++++++++++++++++++++++++++++++++---------------
 1 files changed, 66 insertions(+), 22 deletions(-)

diff --git a/hw/loader.c b/hw/loader.c
index 33acc2f..0be8bf7 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)
 {
@@ -86,6 +92,24 @@ 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;
+
+    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_rw(image->addr, (uint8_t *)content, size, 1);
+       g_free(content);
+    }
+}
+
 /* read()-like version */
 ssize_t read_targphys(const char *name,
                       int fd, target_phys_addr_t dst_addr, size_t nbytes)
@@ -112,7 +136,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;
 }
@@ -433,15 +462,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);
@@ -474,18 +502,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;
     }
@@ -495,11 +514,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");
@@ -508,7 +527,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;
@@ -516,12 +534,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 v2] register reset handler to write image into memory

[Yin Olivia-R63875]

Dear All,

I can't find MAINTAINER of hw/loader.c.
Who can help review and apply this patch?

Best Regards,
Olivia Yin

> -----Original Message-----
> From: Yin Olivia-R63875
> Sent: Friday, August 17, 2012 5:08 PM
> To: qemu-ppc@nongnu.org; qemu-devel@nongnu.org
> Cc: Yin Olivia-R63875
> Subject: [PATCH v2] register reset handler to write image into memory
> 
> 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.
> 
> v2: use g_file_get_content() to load load image file.
> 
> 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 |   88 ++++++++++++++++++++++++++++++++++++++++++++---------
> ------
>  1 files changed, 66 insertions(+), 22 deletions(-)
> 
> diff --git a/hw/loader.c b/hw/loader.c
> index 33acc2f..0be8bf7 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)  { @@ -86,6 +92,24 @@ 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;
> +
> +    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_rw(image->addr, (uint8_t *)content, size, 1);
> +       g_free(content);
> +    }
> +}
> +
>  /* read()-like version */
>  ssize_t read_targphys(const char *name,
>                        int fd, target_phys_addr_t dst_addr, size_t nbytes)
> @@ -112,7 +136,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;
>  }
> @@ -433,15 +462,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); @@ -474,18 +502,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;
>      }
> @@ -495,11 +514,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"); @@
> -508,7 +527,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;
> @@ -516,12 +534,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 v2] register reset handler to write image into memory

[Dunrong Huang]

2012/8/23 Yin Olivia-R63875 <r63875@freescale.com>:
> Dear All,
>
> I can't find MAINTAINER of hw/loader.c.
> Who can help review and apply this patch?
>
Please use the script scripts/get_maintainer.pl, like:
$ scripts/get_maintainer.pl your_patch_file.patch
or
$ scripts/get_maintainer.pl -f hw/loader.c
> Best Regards,
> Olivia Yin
>


-- 
Best Regards,

Dunrong Huang

Re: [Qemu-devel] [PATCH v2] register reset handler to write image into memory

[Andreas Färber]

Hi,

Am 23.08.2012 11:45, schrieb Yin Olivia-R63875:
> Dear All,
> 
> I can't find MAINTAINER of hw/loader.c.
> Who can help review and apply this patch?

This patch is not a small bugfix so it won't be applied during the v1.2
Hard Freeze. You based it onto ppc-next so the obvious answer is, Alex
needs to review it, whom you forgot to cc.

This patch does not answer the question why you try to avoid the ROM
blobs and what ROM blobs are still being used for after your patch. I
don't think it makes much sense to work around them for your use cases
and to leave them behind - if there's something fundamentally wrong with
them they should be ripped out completely or fixed. But maybe I'm
misunderstanding in the absence of explanations?

Regards,
Andreas

> 
> Best Regards,
> Olivia Yin
> 
>> -----Original Message-----
>> From: Yin Olivia-R63875
>> Sent: Friday, August 17, 2012 5:08 PM
>> To: qemu-ppc@nongnu.org; qemu-devel@nongnu.org
>> Cc: Yin Olivia-R63875
>> Subject: [PATCH v2] register reset handler to write image into memory
>>
>> 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.
>>
>> v2: use g_file_get_content() to load load image file.
>>
>> 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 |   88 ++++++++++++++++++++++++++++++++++++++++++++---------
>> ------
>>  1 files changed, 66 insertions(+), 22 deletions(-)
>>
>> diff --git a/hw/loader.c b/hw/loader.c
>> index 33acc2f..0be8bf7 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)  { @@ -86,6 +92,24 @@ 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;
>> +
>> +    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_rw(image->addr, (uint8_t *)content, size, 1);
>> +       g_free(content);
>> +    }
>> +}
>> +
>>  /* read()-like version */
>>  ssize_t read_targphys(const char *name,
>>                        int fd, target_phys_addr_t dst_addr, size_t nbytes)
>> @@ -112,7 +136,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;
>>  }
>> @@ -433,15 +462,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); @@ -474,18 +502,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;
>>      }
>> @@ -495,11 +514,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"); @@
>> -508,7 +527,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;
>> @@ -516,12 +534,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
> 
> 
> 


-- 
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg

Re: [Qemu-devel] [PATCH v2] register reset handler to write image into memory

[Yin Olivia-R63875]

Thanks to Dunrong and Andreas.

$ scripts/get_maintainer.pl -f hw/loader.c
Alexander Graf <agraf@suse.de> (commit_signer:3/6=50%)
Anthony Liguori <aliguori@us.ibm.com> (commit_signer:2/6=33%)
Stefan Weil <weil@mail.berlios.de> (commit_signer:1/6=17%)
Benjamin Herrenschmidt <benh@kernel.crashing.org> (commit_signer:1/6=17%)
Avi Kivity <avi@redhat.com> (commit_signer:1/6=17%)

Dear maintainers,
Could you please help review this patch?

So far I got feedback from Andreas and try to answer the question.

> This patch does not answer the question why you try to avoid the ROM blobs
> and what ROM blobs are still being used for after your patch. I don't 
> think it makes much sense to work around them for your use cases and to 
> leave them behind - if there's something fundamentally wrong with them 
> they should be ripped out completely or fixed. But maybe I'm misunderstanding 
> in the absence of explanations?

It's a general problem. 

For example, in my case, there're 3 different files loaded from host rootfs.
$ qemu-system-ppc -enable-kvm -m 256 -nographic -M mpc8544ds -kernel uImage.8572.agraf -initrd /media/ram/guest-8572.rootfs.ext2.gz -append "root=/dev/ram rw loglevel=7 console=ttyS0,115200" -serial tcp::4445,server -net nic

(qemu) info roms
 addr=0000000000000000 size=0x782840 mem=ram name="uImage.8572.agraf"
 addr=0000000000c00000 size=0x010000 mem=ram name="mpc8544ds.dtb"
 addr=0000000002000000 size=0x3f922f mem=ram name="/media/ram/guest-8572.rootfs.ext2.gz"

The problem is that rom_add_*() mallocs memory for the image, and then rom_reset()
copies those images into the guest's memory, but the QEMU memory does not get freed.
On a VM reset, the images get recopied from QEMU to guest.

Comparing the memory map of qemu process before and after starting up guest,
we can find that QEMU consumes much memory for those images.

$ diff -urN pmap.pre.log pmap.post.log
--- pmap.pre.log
+++ pmap.post.log
@@ -33,7 +33,14 @@
 0ffee000      8K rwx--  /lib/ld-2.13.so
 10000000   3472K r-x--  qemu-system-ppc
 10374000    112K rwx--  qemu-system-ppc
-10390000   6524K rwx--    [ anon ]
+10390000   7100K rwx--    [ anon ]
 48002000     16K rw---    [ anon ]
+48006000      4K -----    [ anon ]
+48007000   8188K rw---    [ anon ]
+48806000      8K rw-s-    [ anon ]
+48808000      4K rw---    [ anon ]
+48809000 262144K rw---    [ anon ]
+58809000   5280K rw---    [ anon ]
+5cb98000   7692K rw---    [ anon ]
 bf93e000    132K rw---    [ stack ]
- total    14456K
+ total   298352K

Exactly we can re-load them from disk on a reset instead of holding onto the images in QEMU's memory.

With this patch, the two big images (uImage and especially initrd) will not be loaded into QEMU's memory
(qemu) info roms
 addr=0000000000c00000 size=0x010000 mem=ram name="mpc8544ds.dtb"

It will save much memory space according to memory map of QEMU process.
# diff -urN pmap.pre.log pmap.post.log
--- pmap.pre.log
+++ pmap.post.log
@@ -33,7 +33,14 @@
 0ffee000      8K rwx--  /lib/ld-2.13.so
 10000000   3472K r-x--  qemu-system-ppc
 10374000    112K rwx--  qemu-system-ppc
-10390000   6524K rwx--    [ anon ]
+10390000   7036K rwx--    [ anon ]
 48002000     16K rw---    [ anon ]
+48006000      4K -----    [ anon ]
+48007000   8188K rw---    [ anon ]
+48806000      8K rw-s-    [ anon ]
+48808000      4K rw---    [ anon ]
+48809000 262144K rw---    [ anon ]
+58809000      4K rw---    [ anon ]
+58c04000   1204K rw---    [ anon ]
 bfb2a000    132K rw---    [ stack ]
- total    14456K
+ total   286524K

This patch changes all the image load process called by load_uimage() and 
load_image_targphys() in platform initialization.

Best Regards,
Olivia

> -----Original Message-----
> From: Dunrong Huang [mailto:riegamaths@gmail.com]
> Sent: Thursday, August 23, 2012 6:44 PM
> To: Yin Olivia-R63875
> Cc: qemu-ppc@nongnu.org; qemu-devel@nongnu.org
> Subject: Re: [Qemu-devel] [PATCH v2] register reset handler to write
> image into memory
> 
> 2012/8/23 Yin Olivia-R63875 <r63875@freescale.com>:
> > Dear All,
> >
> > I can't find MAINTAINER of hw/loader.c.
> > Who can help review and apply this patch?
> >
> Please use the script scripts/get_maintainer.pl, like:
> $ scripts/get_maintainer.pl your_patch_file.patch or
> $ scripts/get_maintainer.pl -f hw/loader.c
> > Best Regards,
> > Olivia Yin
> >
> 
> 
> --
> Best Regards,
> 
> Dunrong Huang

Re: [Qemu-devel] [PATCH v2] register reset handler to write image into memory

[Alexander Graf]

On 26.08.2012, at 20:50, Yin Olivia-R63875 <r63875@freescale.com> wrote:

> Thanks to Dunrong and Andreas.
> 
> $ scripts/get_maintainer.pl -f hw/loader.c
> Alexander Graf <agraf@suse.de> (commit_signer:3/6=50%)
> Anthony Liguori <aliguori@us.ibm.com> (commit_signer:2/6=33%)
> Stefan Weil <weil@mail.berlios.de> (commit_signer:1/6=17%)
> Benjamin Herrenschmidt <benh@kernel.crashing.org> (commit_signer:1/6=17%)
> Avi Kivity <avi@redhat.com> (commit_signer:1/6=17%)
> 
> Dear maintainers,
> Could you please help review this patch?
> 
> So far I got feedback from Andreas and try to answer the question.
> 
>> This patch does not answer the question why you try to avoid the ROM blobs
>> and what ROM blobs are still being used for after your patch. I don't 
>> think it makes much sense to work around them for your use cases and to 
>> leave them behind - if there's something fundamentally wrong with them 
>> they should be ripped out completely or fixed. But maybe I'm misunderstanding 
>> in the absence of explanations?
> 
> It's a general problem. 
> 
> For example, in my case, there're 3 different files loaded from host rootfs.
> $ qemu-system-ppc -enable-kvm -m 256 -nographic -M mpc8544ds -kernel uImage.8572.agraf -initrd /media/ram/guest-8572.rootfs.ext2.gz -append "root=/dev/ram rw loglevel=7 console=ttyS0,115200" -serial tcp::4445,server -net nic
> 
> (qemu) info roms
> addr=0000000000000000 size=0x782840 mem=ram name="uImage.8572.agraf"
> addr=0000000000c00000 size=0x010000 mem=ram name="mpc8544ds.dtb"
> addr=0000000002000000 size=0x3f922f mem=ram name="/media/ram/guest-8572.rootfs.ext2.gz"
> 
> The problem is that rom_add_*() mallocs memory for the image, and then rom_reset()
> copies those images into the guest's memory, but the QEMU memory does not get freed.
> On a VM reset, the images get recopied from QEMU to guest.
> 
> Comparing the memory map of qemu process before and after starting up guest,
> we can find that QEMU consumes much memory for those images.
> 
> $ diff -urN pmap.pre.log pmap.post.log
> --- pmap.pre.log
> +++ pmap.post.log
> @@ -33,7 +33,14 @@
> 0ffee000      8K rwx--  /lib/ld-2.13.so
> 10000000   3472K r-x--  qemu-system-ppc
> 10374000    112K rwx--  qemu-system-ppc
> -10390000   6524K rwx--    [ anon ]
> +10390000   7100K rwx--    [ anon ]
> 48002000     16K rw---    [ anon ]
> +48006000      4K -----    [ anon ]
> +48007000   8188K rw---    [ anon ]
> +48806000      8K rw-s-    [ anon ]
> +48808000      4K rw---    [ anon ]
> +48809000 262144K rw---    [ anon ]
> +58809000   5280K rw---    [ anon ]
> +5cb98000   7692K rw---    [ anon ]
> bf93e000    132K rw---    [ stack ]
> - total    14456K
> + total   298352K
> 
> Exactly we can re-load them from disk on a reset instead of holding onto the images in QEMU's memory.
> 
> With this patch, the two big images (uImage and especially initrd) will not be loaded into QEMU's memory
> (qemu) info roms
> addr=0000000000c00000 size=0x010000 mem=ram name="mpc8544ds.dtb"
> 
> It will save much memory space according to memory map of QEMU process.
> # diff -urN pmap.pre.log pmap.post.log
> --- pmap.pre.log
> +++ pmap.post.log
> @@ -33,7 +33,14 @@
> 0ffee000      8K rwx--  /lib/ld-2.13.so
> 10000000   3472K r-x--  qemu-system-ppc
> 10374000    112K rwx--  qemu-system-ppc
> -10390000   6524K rwx--    [ anon ]
> +10390000   7036K rwx--    [ anon ]
> 48002000     16K rw---    [ anon ]
> +48006000      4K -----    [ anon ]
> +48007000   8188K rw---    [ anon ]
> +48806000      8K rw-s-    [ anon ]
> +48808000      4K rw---    [ anon ]
> +48809000 262144K rw---    [ anon ]
> +58809000      4K rw---    [ anon ]
> +58c04000   1204K rw---    [ anon ]
> bfb2a000    132K rw---    [ stack ]
> - total    14456K
> + total   286524K
> 
> This patch changes all the image load process called by load_uimage() and 
> load_image_targphys() in platform initialization.

This doesn't explain why you leave the old in-RAM code alive though. The only reason I can imagine would be to allow for reset to not reload new roms after an update.

Anthony, any opinion here? Do we need the keep-in-RAM rom code? Or could we just always load rom blobs on demand for everything?


Alex

> 
> Best Regards,
> Olivia
> 
>> -----Original Message-----
>> From: Dunrong Huang [mailto:riegamaths@gmail.com]
>> Sent: Thursday, August 23, 2012 6:44 PM
>> To: Yin Olivia-R63875
>> Cc: qemu-ppc@nongnu.org; qemu-devel@nongnu.org
>> Subject: Re: [Qemu-devel] [PATCH v2] register reset handler to write
>> image into memory
>> 
>> 2012/8/23 Yin Olivia-R63875 <r63875@freescale.com>:
>>> Dear All,
>>> 
>>> I can't find MAINTAINER of hw/loader.c.
>>> Who can help review and apply this patch?
>>> 
>> Please use the script scripts/get_maintainer.pl, like:
>> $ scripts/get_maintainer.pl your_patch_file.patch or
>> $ scripts/get_maintainer.pl -f hw/loader.c
>>> Best Regards,
>>> Olivia Yin
>>> 
>> 
>> 
>> --
>> Best Regards,
>> 
>> Dunrong Huang
> 
> 

Re: [Qemu-devel] [PATCH v2] register reset handler to write image into memory

[Alexander Graf]

On 23.08.2012, at 13:38, Andreas Färber wrote:

> Hi,
> 
> Am 23.08.2012 11:45, schrieb Yin Olivia-R63875:
>> Dear All,
>> 
>> I can't find MAINTAINER of hw/loader.c.
>> Who can help review and apply this patch?
> 
> This patch is not a small bugfix so it won't be applied during the v1.2
> Hard Freeze. You based it onto ppc-next so the obvious answer is, Alex
> needs to review it, whom you forgot to cc.
> 
> This patch does not answer the question why you try to avoid the ROM
> blobs and what ROM blobs are still being used for after your patch. I
> don't think it makes much sense to work around them for your use cases
> and to leave them behind - if there's something fundamentally wrong with
> them they should be ripped out completely or fixed. But maybe I'm
> misunderstanding in the absence of explanations?

The fundamental problem is the memory footprint. We only need ROM blobs on reset, where they get copied into guest RAM. That means during the lifetime of the VM, we waste host memory for no good reason. Imagine a guest that runs with -kernel and -initrd, each 10MB in size. Then that VM wastes 20MB of precious host memory.

Eventually, I don't think we will need the full-blown rom interface with in-memory rom blobs anymore. Everything should be constructed on demand during reset.

However, if you look at code like the s390 initialization, some users of the rom interface expect changes done once to be persistent. These will have to be rewritten to redo their changes on reset.

So Olivia, please do the following:

  - Make sure that _no_ persistent rom code is left over eventually. This also means that you need to convert ELF.
  - Go through every user of rom_ptr and write the code differently. For now, probably by just registering a reset handler that overwrites the respective memory location on every reset, rather than modify the rom blob.


Alex

Re: [Qemu-devel] [PATCH v2] register reset handler to write image into memory

[Yin Olivia-R63875]

Hi Alex,

I checked all the rom_add_*() functions.
Multiple platforms of different architectures use rom_add_* to save images.
	hw/arm_boot.c
	hw/exynos4210.
	hw/highbank.
	hw/mips_fulong2e.c
	hw/mips_malta.c
	hw/mips_r4k.c
	hw/r2d.c

Even for PowerPC, it also use rom_add_blob() to write dtb in memory.
	hw/ppc/e500.c:	rom_add_blob_fixed(BINARY_DEVICE_TREE_FILE, fdt, fdt_size, addr);
	hw/ppc440_bamboo.c

You also minder that ELF file.
	hw/elf_ops.h: 	rom_add_blob_fixed(label, data, mem_size, addr);

pstrcpy_targphys() does also call rom_add_blob_fixed() function, so we need also verify
	hw/alpha_dp264.c
	hw/cris-boot.c
	hw/lm32_boards.c
	hw/microblaze_boot.c
	hw/milkymist.c
	hw/ppc.c
	hw/ppc_newworld.c
	hw/ppc_oldworld.c
	hw/sun4m.c
	hw/sun4m.c

Should we register reset handler for each above user?

The callers of rom_ptr() function:
	hw/s390-virtio.c
	hw/sun4m.c
	hw/sun4u.c
	target-arm/cpu.c
But I don't understand why rom_ptr should be changed.

Best Regards,
Olivia


> -----Original Message-----
> From: Alexander Graf [mailto:agraf@suse.de]
> Sent: Thursday, September 20, 2012 8:10 PM
> To: Andreas Färber
> Cc: Yin Olivia-R63875; qemu-ppc@nongnu.org; qemu-devel@nongnu.org
> Subject: Re: [Qemu-devel] [PATCH v2] register reset handler to write
> image into memory
> 
> 
> On 23.08.2012, at 13:38, Andreas Färber wrote:
> 
> > Hi,
> >
> > Am 23.08.2012 11:45, schrieb Yin Olivia-R63875:
> >> Dear All,
> >>
> >> I can't find MAINTAINER of hw/loader.c.
> >> Who can help review and apply this patch?
> >
> > This patch is not a small bugfix so it won't be applied during the
> > v1.2 Hard Freeze. You based it onto ppc-next so the obvious answer is,
> > Alex needs to review it, whom you forgot to cc.
> >
> > This patch does not answer the question why you try to avoid the ROM
> > blobs and what ROM blobs are still being used for after your patch. I
> > don't think it makes much sense to work around them for your use cases
> > and to leave them behind - if there's something fundamentally wrong
> > with them they should be ripped out completely or fixed. But maybe I'm
> > misunderstanding in the absence of explanations?
> 
> The fundamental problem is the memory footprint. We only need ROM blobs
> on reset, where they get copied into guest RAM. That means during the
> lifetime of the VM, we waste host memory for no good reason. Imagine a
> guest that runs with -kernel and -initrd, each 10MB in size. Then that VM
> wastes 20MB of precious host memory.
> 
> Eventually, I don't think we will need the full-blown rom interface with
> in-memory rom blobs anymore. Everything should be constructed on demand
> during reset.
> 
> However, if you look at code like the s390 initialization, some users of
> the rom interface expect changes done once to be persistent. These will
> have to be rewritten to redo their changes on reset.
> 
> So Olivia, please do the following:
> 
>   - Make sure that _no_ persistent rom code is left over eventually. This
> also means that you need to convert ELF.
>   - Go through every user of rom_ptr and write the code differently. For
> now, probably by just registering a reset handler that overwrites the
> respective memory location on every reset, rather than modify the rom
> blob.
> 
> 
> Alex
> 

Re: [Qemu-devel] [PATCH v2] register reset handler to write image into memory

[Alexander Graf]

On 26.09.2012, at 10:13, Yin Olivia-R63875 wrote:

> Hi Alex,
> 
> I checked all the rom_add_*() functions.
> Multiple platforms of different architectures use rom_add_* to save images.
> 	hw/arm_boot.c
> 	hw/exynos4210.
> 	hw/highbank.
> 	hw/mips_fulong2e.c
> 	hw/mips_malta.c
> 	hw/mips_r4k.c
> 	hw/r2d.c
> 
> Even for PowerPC, it also use rom_add_blob() to write dtb in memory.
> 	hw/ppc/e500.c:	rom_add_blob_fixed(BINARY_DEVICE_TREE_FILE, fdt, fdt_size, addr);
> 	hw/ppc440_bamboo.c
> 
> You also minder that ELF file.
> 	hw/elf_ops.h: 	rom_add_blob_fixed(label, data, mem_size, addr);
> 
> pstrcpy_targphys() does also call rom_add_blob_fixed() function, so we need also verify
> 	hw/alpha_dp264.c
> 	hw/cris-boot.c
> 	hw/lm32_boards.c
> 	hw/microblaze_boot.c
> 	hw/milkymist.c
> 	hw/ppc.c
> 	hw/ppc_newworld.c
> 	hw/ppc_oldworld.c
> 	hw/sun4m.c
> 	hw/sun4m.c
> 
> Should we register reset handler for each above user?

I suppose we should decide on a case-by-case basis. If it's easy to reconstruct the binary, convert it to a reset handler. It it's too hard, leave it to whoever cares about the board.

> The callers of rom_ptr() function:
> 	hw/s390-virtio.c
> 	hw/sun4m.c
> 	hw/sun4u.c
> 	target-arm/cpu.c
> But I don't understand why rom_ptr should be changed.

Because rom_ptr works on roms. When we get rid of roms, rom_ptr won't work anymore, because there is no in-memory representation of the rom to work on.

So instead of calling rom_ptr, the respective code should have a reset handler that gets invoked after the rom restore handler (or a callback function?) which can restore the change to the rom that code wants to do.


Alex