Re: [Qemu-devel] [Qemu-ppc] [RFC PATCH v7 1/4] use image_file_reset to reload initrd

[Alexander Graf]

Missing patch description

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

> Signed-off-by: Olivia Yin <address@hidden>
> ---
> 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
> 
> 
>