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

[Thomas Huth <thuth@redhat.com>]

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