| Message ID | 1354166782-5146-2-git-send-email-hong-hua.yin@freescale.com |
|---|---|
| State | New |
| Headers | show |
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 > > >
> -----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 > > > > > > >
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
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 */
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(-)