On Sat, 25 Feb 2017 07:05:27 +0100
Thomas Huth <th...@redhat.com> wrote:
> On 23.02.2017 13:20, Cornelia Huck wrote:
> > From: Farhan Ali <al...@linux.vnet.ibm.com>
> >
> > The current QEMU ROM infrastructure rejects late loading of ROMs.
> > And ELFs are currently loaded as ROM, this prevents delayed loading
> > of ELFs. So when loading ELF, allow the user to specify if ELF should
> > be loaded as ROM or not.
> >
> > If an ELF is not loaded as ROM, then they are not restored on a
> > guest reboot/reset and so its upto the user to handle the reloading.
> >
> > Signed-off-by: Farhan Ali <al...@linux.vnet.ibm.com>
> > Reviewed-by: Christian Borntraeger <borntrae...@de.ibm.com>
> > Cc: Peter Maydell <peter.mayd...@linaro.org>
> > Signed-off-by: Cornelia Huck <cornelia.h...@de.ibm.com>
> > ---
> > hw/core/loader.c | 17 +++++++++++++++--
> > include/hw/elf_ops.h | 13 +++++++++----
> > include/hw/loader.h | 13 ++++++++++++-
> > 3 files changed, 36 insertions(+), 7 deletions(-)
> >
> > diff --git a/hw/core/loader.c b/hw/core/loader.c
> > index ee5abd6eb7..9d1af1f6f3 100644
> > --- a/hw/core/loader.c
> > +++ b/hw/core/loader.c
> > @@ -435,6 +435,19 @@ int load_elf_as(const char *filename,
> > uint64_t *highaddr, int big_endian, int elf_machine,
> > int clear_lsb, int data_swab, AddressSpace *as)
> > {
> > + return load_elf_ram(filename, translate_fn, translate_opaque,
> > + pentry, lowaddr, highaddr, big_endian, elf_machine,
> > + clear_lsb, data_swab, as, true);
> > +}
> > +
> > +/* return < 0 if error, otherwise the number of bytes loaded in memory */
> > +int load_elf_ram(const char *filename,
> > + uint64_t (*translate_fn)(void *, uint64_t),
> > + void *translate_opaque, uint64_t *pentry, uint64_t
> > *lowaddr,
> > + uint64_t *highaddr, int big_endian, int elf_machine,
> > + int clear_lsb, int data_swab, AddressSpace *as,
> > + bool load_rom)
> > +{
> > int fd, data_order, target_data_order, must_swab, ret =
> > ELF_LOAD_FAILED;
> > uint8_t e_ident[EI_NIDENT];
>
> <bikeshedpaintingsofeelfreetoignoreme>
>
> The patch looks basically fine to me, but I think it's a little bit
> confusing to have a function called load_elf_ram() which can also be
> used to load ROMs with a load_rom=1 parameter. If I read
> "load_elf_ram()", I'd expect a function that can only read ELFs to RAM.
> So what about adding the "load_rom" parameter to load_elf_as() instead
> and then making load_elf_ram() a wrapper function to that one with
> load_rom=0 ? AFAICS there is only one additional caller to load_elf_as
> (in the generic-loader), so the additional effort here should be OK, I
> think.
>
> </bikeshedpaintingsofeelfreetoignoreme>
>
> Thomas
>
I think both approaches are fine, but I'd prefer to keep the bikeshed
colour to avoid a re-spin :)
