Re: [Qemu-devel] [PATCH v9 4/8] loader: Add AddressSpace loading support to ELFs

[Peter Maydell <peter.maydell@linaro.org>]

On 14 July 2016 at 01:03, Alistair Francis <alistair.francis@xilinx.com> wrote:
> Add a new function load_elf_as() that allows the caller to specify an
> AddressSpace to use when loading the ELF. The original load_elf()
> function doesn't have any change in functionality.
>
> Signed-off-by: Alistair Francis <alistair.francis@xilinx.com>
> ---
> V8:
>  - Introduce an RFC version of AddressSpace support
>
>  hw/core/loader.c     | 16 ++++++++++++++--
>  include/hw/elf_ops.h |  5 +++--
>  include/hw/loader.h  | 16 +++++++++++++++-
>  3 files changed, 32 insertions(+), 5 deletions(-)
>
> diff --git a/hw/core/loader.c b/hw/core/loader.c
> index a024133..0f69894 100644
> --- a/hw/core/loader.c
> +++ b/hw/core/loader.c
> @@ -417,6 +417,18 @@ int load_elf(const char *filename, uint64_t (*translate_fn)(void *, uint64_t),
>               uint64_t *highaddr, int big_endian, int elf_machine,
>               int clear_lsb, int data_swab)
>  {
> +    return load_elf_as(filename, translate_fn, translate_opaque, pentry,
> +                       lowaddr, highaddr, big_endian, elf_machine, clear_lsb,
> +                       data_swab, NULL);
> +}
> +
> +/* return < 0 if error, otherwise the number of bytes loaded in memory */
> +int load_elf_as(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)
> +{
>      int fd, data_order, target_data_order, must_swab, ret = ELF_LOAD_FAILED;
>      uint8_t e_ident[EI_NIDENT];
>
> @@ -455,11 +467,11 @@ int load_elf(const char *filename, uint64_t (*translate_fn)(void *, uint64_t),
>      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);
> +                         data_swab, as);
>      } else {
>          ret = load_elf32(filename, fd, translate_fn, translate_opaque, must_swab,
>                           pentry, lowaddr, highaddr, elf_machine, clear_lsb,
> -                         data_swab);
> +                         data_swab, as);
>      }
>
>   fail:
> diff --git a/include/hw/elf_ops.h b/include/hw/elf_ops.h
> index 1339677..3b8c9e9 100644
> --- a/include/hw/elf_ops.h
> +++ b/include/hw/elf_ops.h
> @@ -263,7 +263,8 @@ static int glue(load_elf, SZ)(const char *name, int fd,
>                                void *translate_opaque,
>                                int must_swab, uint64_t *pentry,
>                                uint64_t *lowaddr, uint64_t *highaddr,
> -                              int elf_machine, int clear_lsb, int data_swab)
> +                              int elf_machine, int clear_lsb, int data_swab,
> +                              AddressSpace *as)
>  {
>      struct elfhdr ehdr;
>      struct elf_phdr *phdr = NULL, *ph;
> @@ -405,7 +406,7 @@ static int glue(load_elf, SZ)(const char *name, int fd,
>              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, NULL);
> +            rom_add_elf_program(label, data, file_size, mem_size, addr, as);
>
>              total_size += mem_size;
>              if (addr < low)
> diff --git a/include/hw/loader.h b/include/hw/loader.h
> index a701423..36a16cc 100644
> --- a/include/hw/loader.h
> +++ b/include/hw/loader.h
> @@ -45,7 +45,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:
> +/** load_elf_as:
>   * @filename: Path of ELF file
>   * @translate_fn: optional function to translate load addresses
>   * @translate_opaque: opaque data passed to @translate_fn
> @@ -59,6 +59,8 @@ const char *load_elf_strerror(int error);
>   * @data_swab: Set to order of byte swapping for data. 0 for no swap, 1
>   *             for swapping bytes within halfwords, 2 for bytes within
>   *             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.

"if @as is NULL".

>   *
>   * Load an ELF file's contents to the emulated system's address space.
>   * Clients may optionally specify a callback to perform address
> @@ -73,6 +75,16 @@ const char *load_elf_strerror(int error);
>   * machine type.
>   */
>
> +int load_elf_as(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);
> +
> +/** load_elf:
> + * Same as above, but doesn't allow the caller to specify an AddressSpace

"Same as load_elf_as()", just in case things get shuffled around
in the file later.

> + */
> +
>  int load_elf(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,
> @@ -141,8 +153,10 @@ void hmp_info_roms(Monitor *mon, const QDict *qdict);
>      rom_add_file(_f, NULL, _a, _i, false, NULL, NULL)
>  #define rom_add_blob_fixed(_f, _b, _l, _a)      \
>      rom_add_blob(_f, _b, _l, _l, _a, NULL, NULL, NULL)
> +
>  #define rom_add_file_mr(_f, _mr, _i)            \
>      rom_add_file(_f, NULL, 0, _i, false, _mr, NULL)
> +
>  #define rom_add_file_as(_f, _as, _i)            \
>      rom_add_file(_f, NULL, 0, _i, false, NULL, _as)

Random whitespace changes.

Otherwise
Reviewed-by: Peter Maydell <peter.maydell@linaro.org>

thanks
-- PMM