Re: [Qemu-devel] [PATCH v8 2/5] loader: All a custom SddressSpace when loading ROMs

[Peter Maydell]

On 2 July 2016 at 02:07, 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>
> ---
> V8:
>  - Introduce an RFC version of AddressSpace loading support
>
>  hw/core/loader.c     | 18 ++++++++++++------
>  include/hw/elf_ops.h |  2 +-
>  include/hw/loader.h  | 10 ++++++----
>  3 files changed, 19 insertions(+), 11 deletions(-)
>
> diff --git a/hw/core/loader.c b/hw/core/loader.c
> index 53e0e41..fcbcfbf 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;
> @@ -833,7 +834,8 @@ 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)

We seem to add this argument to the function but never use it?

I think specifying both mr and as should be an error.

>  {
>      MachineClass *mc = MACHINE_GET_CLASS(qdev_get_machine());
>      Rom *rom;
> @@ -969,7 +971,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 +981,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 +1011,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,
>                                            rom->addr, rom->data, 
> rom->datasize);
>          }
>          if (rom->isrom) {
> @@ -1031,12 +1035,13 @@ int rom_check_and_register_reset(void)
>      hwaddr addr = 0;
>      MemoryRegionSection section;
>      Rom *rom;
> +    AddressSpace *as = NULL;
>
>      QTAILQ_FOREACH(rom, &roms, next) {
>          if (rom->fw_file) {
>              continue;
>          }
> -        if (addr > rom->addr) {
> +        if ((addr > rom->addr) && (as == rom->as)) {
>              fprintf(stderr, "rom: requested regions overlap "
>                      "(rom %s. free=0x" TARGET_FMT_plx
>                      ", addr=0x" TARGET_FMT_plx ")\n",
> @@ -1048,6 +1053,7 @@ int rom_check_and_register_reset(void)
>          section = memory_region_find(get_system_memory(), rom->addr, 1);

(An unrelated bug but I've just noticed that this code doesn't
make sense for ROMs which go into a memory region rather than
at an address in the system address space.)

>          rom->isrom = int128_nz(section.size) && 
> memory_region_is_rom(section.mr);
>          memory_region_unref(section.mr);
> +        as = rom->as;
>      }

I don't think this attempt at checking is going to actually
catch all the overlap cases if you allow multiple address
spaces. The rom_insert() code orders roms in this
list purely by load address, so you could get cases like
 [AS A, 0..0x1000], [AS B, 0..0x1000], [AS A, 0x500..0x1000]
where entries 1 and 3 overlap but won't get caught.
You probably need to order the list by AS first and then
by address, and then adjust this code accordingly.

>      qemu_register_reset(rom_reset, NULL);
>      roms_loaded = 1;
> diff --git a/include/hw/elf_ops.h b/include/hw/elf_ops.h
> index db70c11..1339677 100644
> --- a/include/hw/elf_ops.h
> +++ b/include/hw/elf_ops.h
> @@ -405,7 +405,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);
> +            rom_add_elf_program(label, data, file_size, mem_size, addr, 
> NULL);
>
>              total_size += mem_size;
>              if (addr < low)
> diff --git a/include/hw/loader.h b/include/hw/loader.h
> index 4879b63..18eb0f2 100644
> --- a/include/hw/loader.h
> +++ b/include/hw/loader.h
> @@ -118,14 +118,14 @@ extern bool rom_file_has_mr;
>
>  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);
>  MemoryRegion *rom_add_blob(const char *name, const void *blob, size_t len,
>                             size_t max_len, hwaddr addr,
>                             const char *fw_file_name,
>                             FWCfgReadCallback fw_callback,
>                             void *callback_opaque);
>  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);
>  int rom_check_and_register_reset(void);
>  void rom_set_fw(FWCfgState *f);
>  void rom_set_order_override(int order);
> @@ -135,11 +135,13 @@ void *rom_ptr(hwaddr addr);
>  void hmp_info_roms(Monitor *mon, const QDict *qdict);
>
>  #define rom_add_file_fixed(_f, _a, _i)          \
> -    rom_add_file(_f, NULL, _a, _i, false, NULL)
> +    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)
> +    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)
>
>  #define PC_ROM_MIN_VGA     0xc0000
>  #define PC_ROM_MIN_OPTION  0xc8000
> --
> 2.7.4
>

thanks
-- PMM