qemu-ppc
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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


From: Yin Olivia-R63875
Subject: Re: [Qemu-ppc] [RFC PATCH v7 1/4] use image_file_reset to reload initrd
Date: Thu, 6 Dec 2012 04:11:48 +0000


> -----Original Message-----
> From: Alexander Graf [mailto:address@hidden
> Sent: Sunday, December 02, 2012 7:20 PM
> To: Yin Olivia-R63875
> Cc: address@hidden; address@hidden
> 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 <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.

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
> >
> >
> >
> 





reply via email to

[Prev in Thread] Current Thread [Next in Thread]