Re: [Qemu-devel] [PATCH v2 1/5] elf-loader: Allow late loading of elf

[Cornelia Huck <cornelia.huck@de.ibm.com>]

On Sat, 25 Feb 2017 07:05:27 +0100
Thomas Huth <thuth@redhat.com> wrote:

> On 23.02.2017 13:20, Cornelia Huck wrote:
> > From: Farhan Ali <alifm@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 <alifm@linux.vnet.ibm.com>
> > Reviewed-by: Christian Borntraeger <borntraeger@de.ibm.com>
> > Cc: Peter Maydell <peter.maydell@linaro.org>
> > Signed-off-by: Cornelia Huck <cornelia.huck@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 :)