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

[Alexander Graf]

On 06.12.2012, at 05:11, Yin Olivia-R63875 wrote:

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

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