Re: [Qemu-devel] [PATCH v9 3/8] loader: Allow a custom AddressSpace when loading ROMs

[Peter Maydell]

On 14 July 2016 at 01:03, Alistair Francis <address@hidden> wrote:
> When loading ROMs allow the caller to specify an AddressSpace to use for
> the load.
>
> Signed-off-by: Alistair Francis <address@hidden>
> ---
> V9:
>  - Fixup the ROM ordering
>  - Don't allow address space and memory region to be specified
> V8:
>  - Introduce an RFC version of AddressSpace loading support
>
>  hw/core/loader.c     | 39 ++++++++++++++++++++++++++++-----------
>  include/hw/elf_ops.h |  2 +-
>  include/hw/loader.h  | 10 ++++++----
>  3 files changed, 35 insertions(+), 16 deletions(-)
>
> diff --git a/hw/core/loader.c b/hw/core/loader.c
> index 6b61f29..a024133 100644
> --- a/hw/core/loader.c
> +++ b/hw/core/loader.c
> @@ -777,6 +777,7 @@ struct Rom {
>
>      uint8_t *data;
>      MemoryRegion *mr;
> +    AddressSpace *as;
>      int isrom;
>      char *fw_dir;
>      char *fw_file;
> @@ -796,12 +797,15 @@ static void rom_insert(Rom *rom)
>          hw_error ("ROM images must be loaded at startup\n");
>      }
>
> -    /* list is ordered by load address */
> +    /* List is ordered by load address in the same address space */
>      QTAILQ_FOREACH(item, &roms, next) {
> -        if (rom->addr >= item->addr)
> -            continue;
> -        QTAILQ_INSERT_BEFORE(item, rom, next);
> -        return;
> +        if (rom->addr >= item->addr && rom->as == item->as) {
> +            QTAILQ_INSERT_AFTER(&roms, item, rom, next);
> +            return;
> +        } else if (rom->addr <= item->addr && rom->as == item->as) {
> +            QTAILQ_INSERT_BEFORE(item, rom, next);
> +            return;
> +        }
>      }
>      QTAILQ_INSERT_TAIL(&roms, rom, next);

This seems a somewhat confusing way of writing this. I think you
should define a comparison function and then just replace the
current "rom->addr >= item->addr" with "rom_order_compare(rom, item) >= 0".
Then it's clear what the comparison you're using to define the
sorted order is and that the loop will put things in in sorted
position.

>  }
> @@ -833,16 +837,25 @@ static void *rom_set_mr(Rom *rom, Object *owner, const 
> char *name)
>
>  int rom_add_file(const char *file, const char *fw_dir,
>                   hwaddr addr, int32_t bootindex,
> -                 bool option_rom, MemoryRegion *mr)
> +                 bool option_rom, MemoryRegion *mr,
> +                 AddressSpace *as)
>  {
>      MachineClass *mc = MACHINE_GET_CLASS(qdev_get_machine());
>      Rom *rom;
>      int rc, fd = -1;
>      char devpath[100];
>
> +    if (as && mr) {
> +        fprintf(stderr, "Specifying an Address Space and Memory Region is " \
> +                "not valid when loading a rom\n");

Some day we'll fix this up to use Errors, but that day need not be today.

> +        /* We haven't allocated anything so we don't need any cleanup */
> +        return -1;
> +    }
> +
>      rom = g_malloc0(sizeof(*rom));
>      rom->name = g_strdup(file);
>      rom->path = qemu_find_file(QEMU_FILE_TYPE_BIOS, rom->name);
> +    rom->as = as;
>      if (rom->path == NULL) {
>          rom->path = g_strdup(file);
>      }
> @@ -969,7 +982,7 @@ MemoryRegion *rom_add_blob(const char *name, const void 
> *blob, size_t len,
>   * memory ownership of "data", so we don't have to allocate and copy the 
> buffer.
>   */
>  int rom_add_elf_program(const char *name, void *data, size_t datasize,
> -                        size_t romsize, hwaddr addr)
> +                        size_t romsize, hwaddr addr, AddressSpace *as)
>  {
>      Rom *rom;
>
> @@ -979,18 +992,19 @@ int rom_add_elf_program(const char *name, void *data, 
> size_t datasize,
>      rom->datasize = datasize;
>      rom->romsize  = romsize;
>      rom->data     = data;
> +    rom->as       = as;
>      rom_insert(rom);
>      return 0;
>  }
>
>  int rom_add_vga(const char *file)
>  {
> -    return rom_add_file(file, "vgaroms", 0, -1, true, NULL);
> +    return rom_add_file(file, "vgaroms", 0, -1, true, NULL, NULL);
>  }
>
>  int rom_add_option(const char *file, int32_t bootindex)
>  {
> -    return rom_add_file(file, "genroms", 0, bootindex, true, NULL);
> +    return rom_add_file(file, "genroms", 0, bootindex, true, NULL, NULL);
>  }
>
>  static void rom_reset(void *unused)
> @@ -1008,7 +1022,8 @@ static void rom_reset(void *unused)
>              void *host = memory_region_get_ram_ptr(rom->mr);
>              memcpy(host, rom->data, rom->datasize);
>          } else {
> -            cpu_physical_memory_write_rom(&address_space_memory,
> +            cpu_physical_memory_write_rom(rom->as ? rom->as :
> +                                                    &address_space_memory,

Should we just make rom->as be &address_space_memory if the caller
doesn't provide one rather than having it be NULL and then special
casing what NULL means? (This will also avoid potentially odd
effects with some of the ROMs in the list having &address_space_memory
for an explicitly specified AS and some having NULL for the same
AS but implicitly specified, which is otherwise a pain to deal with
in comparisons.)

thanks
-- PMM