Patchwork [RFC,v7,1/4] use image_file_reset to reload initrd

login
register
mail settings
Submitter Olivia Yin
Date Nov. 29, 2012, 5:26 a.m.
Message ID <1354166782-5146-2-git-send-email-hong-hua.yin@freescale.com>
Download mbox | patch
Permalink /patch/202662/
State New
Headers show

Comments

Olivia Yin - Nov. 29, 2012, 5:26 a.m.
Signed-off-by: Olivia Yin <hong-hua.yin@freescale.com>
---
 hw/loader.c |   24 ++++++++++++++++++++++++
 hw/loader.h |    6 ++++++
 2 files changed, 30 insertions(+), 0 deletions(-)
Alexander Graf - Dec. 2, 2012, 11:20 a.m.
Missing patch description

On 29.11.2012, at 06:26, Olivia Yin wrote:

> Signed-off-by: Olivia Yin <hong-hua.yin@freescale.com>
> ---
> hw/loader.c |   24 ++++++++++++++++++++++++
> hw/loader.h |    6 ++++++
> 2 files changed, 30 insertions(+), 0 deletions(-)
> 
> diff --git a/hw/loader.c b/hw/loader.c
> index ba01ca6..f62aa7c 100644
> --- a/hw/loader.c
> +++ b/hw/loader.c
> @@ -86,6 +86,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_write(image->addr, (uint8_t *)content, size);
> +       g_free(content);
> +    }

If I compare this function to rom_add_file(), it seems like there's a lot of logic missing.

> +}
> +
> /* read()-like version */
> ssize_t read_targphys(const char *name,
>                       int fd, hwaddr dst_addr, size_t nbytes)
> @@ -113,6 +131,12 @@ int load_image_targphys(const char *filename,

Up here is:

>     int size;
> 
>     size = get_image_size(filename);
>     if (size > max_sz) {
>         return -1;

which could be easily replaced by the glib helper function. It passes size and a proper return code already.

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

You now have 2 competing reset handlers: The rom based one and the ImageFile based one.

Why bother with the rom based one? Traditionally, having the rom caller buys you 2 things:

  1) Reset restoration of the rom data

This one is obsolete with the new dynamic loader.

  2) Listing of the rom region in "info roms"

You can replace the Rom struct in loader.c with a new struct that is more clever:

struct RomImage {
    union {
        ImageFile *image;
    } u;
    QTAILQ_ENTRY(RomImage) next;
}

which means that "info roms" can loop through these RomImage types and actually show things like

  ELF image /foo/bar.uImage
    ELF .text section hwaddr=0x... size=0x...
    ELF .data section hwaddr=0x... size=0x...
  Raw image /foo/initrd hwaddr=0x... size=0x...


Alex

>     }
>     return size;
> }
> diff --git a/hw/loader.h b/hw/loader.h
> index 26480ad..9e76ebd 100644
> --- a/hw/loader.h
> +++ b/hw/loader.h
> @@ -1,6 +1,12 @@
> #ifndef LOADER_H
> #define LOADER_H
> 
> +typedef struct ImageFile ImageFile;
> +struct ImageFile {
> +    char *name;
> +    hwaddr addr;
> +};
> +
> /* loader.c */
> int get_image_size(const char *filename);
> int load_image(const char *filename, uint8_t *addr); /* deprecated */
> -- 
> 1.7.1
> 
> 
>
Yin Olivia-R63875 - Dec. 6, 2012, 4:11 a.m.
> -----Original Message-----
> From: Alexander Graf [mailto:agraf@suse.de]
> Sent: Sunday, December 02, 2012 7:20 PM
> To: Yin Olivia-R63875
> Cc: qemu-devel@nongnu.org; qemu-ppc@nongnu.org
> Subject: Re: [Qemu-ppc] [RFC PATCH v7 1/4] use image_file_reset to reload
> initrd
> 
> Missing patch description
> 
> On 29.11.2012, at 06:26, Olivia Yin wrote:
> 
> > Signed-off-by: Olivia Yin <hong-hua.yin@freescale.com>
> > ---
> > hw/loader.c |   24 ++++++++++++++++++++++++
> > hw/loader.h |    6 ++++++
> > 2 files changed, 30 insertions(+), 0 deletions(-)
> >
> > diff --git a/hw/loader.c b/hw/loader.c index ba01ca6..f62aa7c 100644
> > --- a/hw/loader.c
> > +++ b/hw/loader.c
> > @@ -86,6 +86,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_write(image->addr, (uint8_t *)content,
> size);
> > +       g_free(content);
> > +    }
> 
> If I compare this function to rom_add_file(), it seems like there's a lot
> of logic missing.
> 
> > +}
> > +
> > /* read()-like version */
> > ssize_t read_targphys(const char *name,
> >                       int fd, hwaddr dst_addr, size_t nbytes) @@
> > -113,6 +131,12 @@ int load_image_targphys(const char *filename,
> 
> Up here is:
> 
> >     int size;
> >
> >     size = get_image_size(filename);
> >     if (size > max_sz) {
> >         return -1;
> 
> which could be easily replaced by the glib helper function. It passes
> size and a proper return code already.

get_image_size() is a public function in QEMU.
	hw/palm.c:        rom_size = get_image_size(option_rom[0].name);
	hw/mips_fulong2e.c:        initrd_size = get_image_size (loaderparams.initrd_filename);
	hw/loader.c:int get_image_size(const char *filename)
	hw/loader.c:    size = get_image_size(filename);
	hw/multiboot.c:            mb_mod_length = get_image_size(initrd_filename);
	hw/pc.c:	initrd_size = get_image_size(initrd_filename);
	hw/mips_mipssim.c:        initrd_size = get_image_size (loaderparams.initrd_filename);
	hw/pc_sysfw.c:        bios_size = get_image_size(filename);
	hw/smbios.c:        int size = get_image_size(buf);
	hw/leon3.c:    bios_size = get_image_size(filename);
	hw/pci.c:    size = get_image_size(path);
	hw/ppc_prep.c:        bios_size = get_image_size(filename);
	hw/mips_r4k.c:        initrd_size = get_image_size (loaderparams.initrd_filename);
	hw/mips_r4k.c:        bios_size = get_image_size(filename);
	hw/alpha_dp264.c:            initrd_size = get_image_size(initrd_filename);
	hw/loader.h:int get_image_size(const char *filename);
	hw/mips_malta.c:        initrd_size = get_image_size (loaderparams.initrd_filename);
	hw/highbank.c:            uint32_t filesize = get_image_size(sysboot_filename);
	device_tree.c:    dt_size = get_image_size(filename_path);

Do you think I should replace get_image_fize() with g_file_get_contents()?
Or just revise get_image_size() function as below?
gsize get_image_size(const char *filename)
{
    gchar *content;
    gsize size;
    g_file_get_contents(image->name, &content, &size, &err);
    return size;
}

> >     }
> >     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);
> 
> You now have 2 competing reset handlers: The rom based one and the
> ImageFile based one.

OK. I'll remove rom_reset() since the rom->data could be written when rom_load_all().

> Why bother with the rom based one? Traditionally, having the rom caller
> buys you 2 things:
> 
>   1) Reset restoration of the rom data
> 
> This one is obsolete with the new dynamic loader.
> 
>   2) Listing of the rom region in "info roms"
> 
> You can replace the Rom struct in loader.c with a new struct that is more
> clever:
> 
> struct RomImage {
>     union {
>         ImageFile *image;
>     } u;
>     QTAILQ_ENTRY(RomImage) next;
> }
> 
> which means that "info roms" can loop through these RomImage types and
> actually show things like
> 
>   ELF image /foo/bar.uImage
>     ELF .text section hwaddr=0x... size=0x...
>     ELF .data section hwaddr=0x... size=0x...
>   Raw image /foo/initrd hwaddr=0x... size=0x...

Exactly ehdr.e_phnum = 3 and the only first one is PT_LOAD type and will be added into roms and written into memory.
    for(i = 0; i < ehdr.e_phnum; i++) {
        ph = &phdr[i];
        if (ph->p_type == PT_LOAD) {

> 
> Alex
> 
> >     }
> >     return size;
> > }
> > diff --git a/hw/loader.h b/hw/loader.h index 26480ad..9e76ebd 100644
> > --- a/hw/loader.h
> > +++ b/hw/loader.h
> > @@ -1,6 +1,12 @@
> > #ifndef LOADER_H
> > #define LOADER_H
> >
> > +typedef struct ImageFile ImageFile;
> > +struct ImageFile {
> > +    char *name;
> > +    hwaddr addr;
> > +};
> > +
> > /* loader.c */
> > int get_image_size(const char *filename); int load_image(const char
> > *filename, uint8_t *addr); /* deprecated */
> > --
> > 1.7.1
> >
> >
> >
>
Alexander Graf - Dec. 17, 2012, 3:04 p.m.
On 06.12.2012, at 05:11, Yin Olivia-R63875 wrote:

> 
> 
>> -----Original Message-----
>> From: Alexander Graf [mailto:agraf@suse.de]
>> Sent: Sunday, December 02, 2012 7:20 PM
>> To: Yin Olivia-R63875
>> Cc: qemu-devel@nongnu.org; qemu-ppc@nongnu.org
>> Subject: Re: [Qemu-ppc] [RFC PATCH v7 1/4] use image_file_reset to reload
>> initrd
>> 
>> Missing patch description
>> 
>> On 29.11.2012, at 06:26, Olivia Yin wrote:
>> 
>>> Signed-off-by: Olivia Yin <hong-hua.yin@freescale.com>
>>> ---
>>> hw/loader.c |   24 ++++++++++++++++++++++++
>>> hw/loader.h |    6 ++++++
>>> 2 files changed, 30 insertions(+), 0 deletions(-)
>>> 
>>> diff --git a/hw/loader.c b/hw/loader.c index ba01ca6..f62aa7c 100644
>>> --- a/hw/loader.c
>>> +++ b/hw/loader.c
>>> @@ -86,6 +86,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_write(image->addr, (uint8_t *)content,
>> size);
>>> +       g_free(content);
>>> +    }
>> 
>> If I compare this function to rom_add_file(), it seems like there's a lot
>> of logic missing.
>> 
>>> +}
>>> +
>>> /* read()-like version */
>>> ssize_t read_targphys(const char *name,
>>>                      int fd, hwaddr dst_addr, size_t nbytes) @@
>>> -113,6 +131,12 @@ int load_image_targphys(const char *filename,
>> 
>> Up here is:
>> 
>>>    int size;
>>> 
>>>    size = get_image_size(filename);
>>>    if (size > max_sz) {
>>>        return -1;
>> 
>> which could be easily replaced by the glib helper function. It passes
>> size and a proper return code already.
> 
> get_image_size() is a public function in QEMU.
> 	hw/palm.c:        rom_size = get_image_size(option_rom[0].name);
> 	hw/mips_fulong2e.c:        initrd_size = get_image_size (loaderparams.initrd_filename);
> 	hw/loader.c:int get_image_size(const char *filename)
> 	hw/loader.c:    size = get_image_size(filename);
> 	hw/multiboot.c:            mb_mod_length = get_image_size(initrd_filename);
> 	hw/pc.c:	initrd_size = get_image_size(initrd_filename);
> 	hw/mips_mipssim.c:        initrd_size = get_image_size (loaderparams.initrd_filename);
> 	hw/pc_sysfw.c:        bios_size = get_image_size(filename);
> 	hw/smbios.c:        int size = get_image_size(buf);
> 	hw/leon3.c:    bios_size = get_image_size(filename);
> 	hw/pci.c:    size = get_image_size(path);
> 	hw/ppc_prep.c:        bios_size = get_image_size(filename);
> 	hw/mips_r4k.c:        initrd_size = get_image_size (loaderparams.initrd_filename);
> 	hw/mips_r4k.c:        bios_size = get_image_size(filename);
> 	hw/alpha_dp264.c:            initrd_size = get_image_size(initrd_filename);
> 	hw/loader.h:int get_image_size(const char *filename);
> 	hw/mips_malta.c:        initrd_size = get_image_size (loaderparams.initrd_filename);
> 	hw/highbank.c:            uint32_t filesize = get_image_size(sysboot_filename);
> 	device_tree.c:    dt_size = get_image_size(filename_path);
> 
> Do you think I should replace get_image_fize() with g_file_get_contents()?
> Or just revise get_image_size() function as below?
> gsize get_image_size(const char *filename)
> {
>    gchar *content;
>    gsize size;
>    g_file_get_contents(image->name, &content, &size, &err);
>    return size;
> }

That looks good, yes. Make sure to free it again ;).

> 
>>>    }
>>>    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);
>> 
>> You now have 2 competing reset handlers: The rom based one and the
>> ImageFile based one.
> 
> OK. I'll remove rom_reset() since the rom->data could be written when rom_load_all().
> 
>> Why bother with the rom based one? Traditionally, having the rom caller
>> buys you 2 things:
>> 
>>  1) Reset restoration of the rom data
>> 
>> This one is obsolete with the new dynamic loader.
>> 
>>  2) Listing of the rom region in "info roms"
>> 
>> You can replace the Rom struct in loader.c with a new struct that is more
>> clever:
>> 
>> struct RomImage {
>>    union {
>>        ImageFile *image;
>>    } u;
>>    QTAILQ_ENTRY(RomImage) next;
>> }
>> 
>> which means that "info roms" can loop through these RomImage types and
>> actually show things like
>> 
>>  ELF image /foo/bar.uImage
>>    ELF .text section hwaddr=0x... size=0x...
>>    ELF .data section hwaddr=0x... size=0x...
>>  Raw image /foo/initrd hwaddr=0x... size=0x...
> 
> Exactly ehdr.e_phnum = 3 and the only first one is PT_LOAD type and will be added into roms and written into memory.
>    for(i = 0; i < ehdr.e_phnum; i++) {
>        ph = &phdr[i];
>        if (ph->p_type == PT_LOAD) {

That depends on the image, no?


Alex

Patch

diff --git a/hw/loader.c b/hw/loader.c
index ba01ca6..f62aa7c 100644
--- a/hw/loader.c
+++ b/hw/loader.c
@@ -86,6 +86,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_write(image->addr, (uint8_t *)content, size);
+       g_free(content);
+    }
+}
+
 /* read()-like version */
 ssize_t read_targphys(const char *name,
                       int fd, hwaddr dst_addr, size_t nbytes)
@@ -113,6 +131,12 @@  int load_image_targphys(const char *filename,
     }
     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;
 }
diff --git a/hw/loader.h b/hw/loader.h
index 26480ad..9e76ebd 100644
--- a/hw/loader.h
+++ b/hw/loader.h
@@ -1,6 +1,12 @@ 
 #ifndef LOADER_H
 #define LOADER_H
 
+typedef struct ImageFile ImageFile;
+struct ImageFile {
+    char *name;
+    hwaddr addr;
+};
+
 /* loader.c */
 int get_image_size(const char *filename);
 int load_image(const char *filename, uint8_t *addr); /* deprecated */