On 06.12.2012, at 05:11, Yin Olivia-R63875 wrote:
>
>
>> -----Original Message-----
>> From: Alexander Graf [mailto:ag...@suse.de]
>> Sent: Sunday, December 02, 2012 7:20 PM
>> To: Yin Olivia-R63875
>> Cc: qemu-devel@nongnu.org; qemu-...@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....@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
