[v2,1/5] elf-loader: Allow late loading of elf

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(-)

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

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 :)

On Thu, 23 Feb 2017 13:20:21 +0100
Cornelia Huck <cornelia.huck@de.ibm.com> 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(-)
> 

Any further acks for this?

Or, put differently, any objections to me sending this in a pullreq for
2.9?

> 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];
> 
> @@ -473,11 +486,11 @@ int load_elf_as(const char *filename,
>      if (e_ident[EI_CLASS] == ELFCLASS64) {
>          ret = load_elf64(filename, fd, translate_fn, translate_opaque, must_swab,
>                           pentry, lowaddr, highaddr, elf_machine, clear_lsb,
> -                         data_swab, as);
> +                         data_swab, as, load_rom);
>      } else {
>          ret = load_elf32(filename, fd, translate_fn, translate_opaque, must_swab,
>                           pentry, lowaddr, highaddr, elf_machine, clear_lsb,
> -                         data_swab, as);
> +                         data_swab, as, load_rom);
>      }
> 
>   fail:
> diff --git a/include/hw/elf_ops.h b/include/hw/elf_ops.h
> index 25659b93be..a172a6068a 100644
> --- a/include/hw/elf_ops.h
> +++ b/include/hw/elf_ops.h
> @@ -264,7 +264,7 @@ static int glue(load_elf, SZ)(const char *name, int fd,
>                                int must_swab, uint64_t *pentry,
>                                uint64_t *lowaddr, uint64_t *highaddr,
>                                int elf_machine, int clear_lsb, int data_swab,
> -                              AddressSpace *as)
> +                              AddressSpace *as, bool load_rom)
>  {
>      struct elfhdr ehdr;
>      struct elf_phdr *phdr = NULL, *ph;
> @@ -403,10 +403,15 @@ static int glue(load_elf, SZ)(const char *name, int fd,
>                  *pentry = ehdr.e_entry - ph->p_vaddr + ph->p_paddr;
>              }
> 
> -            snprintf(label, sizeof(label), "phdr #%d: %s", i, name);
> +            if (load_rom) {
> +                snprintf(label, sizeof(label), "phdr #%d: %s", i, name);
> 
> -            /* rom_add_elf_program() seize the ownership of 'data' */
> -            rom_add_elf_program(label, data, file_size, mem_size, addr, as);
> +                /* rom_add_elf_program() seize the ownership of 'data' */
> +                rom_add_elf_program(label, data, file_size, mem_size, addr, as);
> +            } else {
> +                cpu_physical_memory_write(addr, data, file_size);
> +                g_free(data);
> +            }
> 
>              total_size += mem_size;
>              if (addr < low)
> diff --git a/include/hw/loader.h b/include/hw/loader.h
> index 0dbd8d6bf3..3df4d73398 100644
> --- a/include/hw/loader.h
> +++ b/include/hw/loader.h
> @@ -65,7 +65,7 @@ int load_image_gzipped(const char *filename, hwaddr addr, uint64_t max_sz);
>  #define ELF_LOAD_WRONG_ENDIAN -4
>  const char *load_elf_strerror(int error);
> 
> -/** load_elf_as:
> +/** load_elf_ram:
>   * @filename: Path of ELF file
>   * @translate_fn: optional function to translate load addresses
>   * @translate_opaque: opaque data passed to @translate_fn
> @@ -81,6 +81,7 @@ const char *load_elf_strerror(int error);
>   *             words and 3 for within doublewords.
>   * @as: The AddressSpace to load the ELF to. The value of address_space_memory
>   *      is used if nothing is supplied here.
> + * @load_rom : Load ELF binary as ROM
>   *
>   * Load an ELF file's contents to the emulated system's address space.
>   * Clients may optionally specify a callback to perform address
> @@ -93,6 +94,16 @@ const char *load_elf_strerror(int error);
>   * If @elf_machine is EM_NONE then the machine type will be read from the
>   * ELF header and no checks will be carried out against the machine type.
>   */
> +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);
> +
> +/** load_elf_as:
> + * Same as load_elf_ram(), but always loads the elf as ROM
> + */
>  int load_elf_as(const char *filename,
>                  uint64_t (*translate_fn)(void *, uint64_t),
>                  void *translate_opaque, uint64_t *pentry, uint64_t *lowaddr,