diff mbox

[v10,3/8] loader: Allow a custom AddressSpace when loading ROMs

Message ID f7ee2b23e3fab86df69f14df0e4233aea4b22732.1470253246.git.alistair.francis@xilinx.com
State New
Headers show

Commit Message

Alistair Francis Aug. 3, 2016, 8:06 p.m. UTC 
When loading ROMs allow the caller to specify an AddressSpace to use for
the load.

Signed-off-by: Alistair Francis <alistair.francis@xilinx.com>
---
V10:
 - Set the rom address space instead of leaving it NULL
 - Cleanup ordering logic
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     | 52 ++++++++++++++++++++++++++++++++++++++++------------
 include/hw/elf_ops.h |  2 +-
 include/hw/loader.h  | 10 ++++++----
 3 files changed, 47 insertions(+), 17 deletions(-)

Comments

Peter Maydell Aug. 9, 2016, 6:44 p.m. UTC | #1 
On 3 August 2016 at 21:06, Alistair Francis <alistair.francis@xilinx.com> wrote:
> When loading ROMs allow the caller to specify an AddressSpace to use for
> the load.
>
> Signed-off-by: Alistair Francis <alistair.francis@xilinx.com>
> ---
> V10:
>  - Set the rom address space instead of leaving it NULL
>  - Cleanup ordering logic
> 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     | 52 ++++++++++++++++++++++++++++++++++++++++------------
>  include/hw/elf_ops.h |  2 +-
>  include/hw/loader.h  | 10 ++++++----
>  3 files changed, 47 insertions(+), 17 deletions(-)
>
> diff --git a/hw/core/loader.c b/hw/core/loader.c
> index 6b61f29..aef7bc9 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;
> @@ -788,6 +789,11 @@ struct Rom {
>  static FWCfgState *fw_cfg;
>  static QTAILQ_HEAD(, Rom) roms = QTAILQ_HEAD_INITIALIZER(roms);
>
> +static inline bool rom_order_compare(Rom *rom, Rom *item)
> +{
> +    return rom->addr >= item->addr;
> +}
> +
>  static void rom_insert(Rom *rom)
>  {
>      Rom *item;
> @@ -796,12 +802,22 @@ static void rom_insert(Rom *rom)
>          hw_error ("ROM images must be loaded at startup\n");
>      }
>
> -    /* list is ordered by load address */
> +    /* The user didn't specify an address space, this is the default */
> +    if (!rom->as) {
> +        rom->as = &address_space_memory;
> +    }
> +
> +    /* 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->as == item->as) {
> +            if (rom_order_compare(rom, item)) {
> +                QTAILQ_INSERT_AFTER(&roms, item, rom, next);
> +                return;
> +            } else {
> +                QTAILQ_INSERT_BEFORE(item, rom, next);
> +                return;
> +            }
> +        }

This isn't quite what I meant, and if we're doing it like this
we might as well have "rom->addr >= item->addr" opencoded here.

What I had in mind when I suggested a separate rom_order_compare()
was that it should be doing the full ordering operation:
    return (rom->as > item->as) ||
        (rom->as == item->as && rom->addr >= item->addr);

(ie it defines a total order on ROMs which for any two ROMs
tells you which way round they go in the list).

and then the change in this function would just be to change the old
   if (rom->addr >= item->addr)
to
   if (rom_order_compare(rom, item))

This has the advantage that it's really obvious that we're
maintaining a sorted array and all we've done is change the
function defining our total order on ROMs.

The problem which I've realised in the course of thinking about
this this evening is that the compare function I suggest above:
 (a) requires a total ordering on pointers, which is not
  guaranteed by the C standard
 (b) even if you cast the rom->as pointers all to uintptr_t
  (getting you defined behaviour) still gives you an ordering
  which is variable from run to run of QEMU, which isn't great.
Unfortunately there's no other obvious ordering on AddressSpaces
(I guess you could look at their position in the address_spaces
list, but that's getting pretty heavyweight.)

That said, the code as you have it in this patch doesn't
sort things properly I think. Consider three ROMs with
addresses 0, 100, 200, all in the same AS, inserted in that order:
 * ROM-0 is added as the first and only item in the list
 * ROM-100 is added in the list after ROM-0 (same AS, and
   rom_order_compare is true, so we do INSERT_AFTER)
 * when we insert ROM-200 the first item in the list is ROM-0.
   This has the same AS, and rom_order_compare(ROM-200, ROM-0)
   is true, so we do an INSERT_AFTER.
Now the list is (ROM-0, ROM-200, ROM-100), which isn't sorted.


So our options are:
 (1) fix up the open-coded code to sort properly
 (2) figure out a rom_order_compare() that does give a total
     ordering on ROMs which isn't variable from run to run of QEMU
 (3) decide that it's OK for the ROM list ordering not to be
     the same from run to run (or on two sides of a migration)
     so we can use the cast-to-uintptr_t version

I don't currently have an opinion (well, I would like 2 except
I can't think of a workable definition for the comparison function).
Anybody?

thanks
-- PMM
Alistair Francis Aug. 9, 2016, 11:26 p.m. UTC | #2 
On Tue, Aug 9, 2016 at 11:44 AM, Peter Maydell <peter.maydell@linaro.org> wrote:
> On 3 August 2016 at 21:06, Alistair Francis <alistair.francis@xilinx.com> wrote:
>> When loading ROMs allow the caller to specify an AddressSpace to use for
>> the load.
>>
>> Signed-off-by: Alistair Francis <alistair.francis@xilinx.com>
>> ---
>> V10:
>>  - Set the rom address space instead of leaving it NULL
>>  - Cleanup ordering logic
>> 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     | 52 ++++++++++++++++++++++++++++++++++++++++------------
>>  include/hw/elf_ops.h |  2 +-
>>  include/hw/loader.h  | 10 ++++++----
>>  3 files changed, 47 insertions(+), 17 deletions(-)
>>
>> diff --git a/hw/core/loader.c b/hw/core/loader.c
>> index 6b61f29..aef7bc9 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;
>> @@ -788,6 +789,11 @@ struct Rom {
>>  static FWCfgState *fw_cfg;
>>  static QTAILQ_HEAD(, Rom) roms = QTAILQ_HEAD_INITIALIZER(roms);
>>
>> +static inline bool rom_order_compare(Rom *rom, Rom *item)
>> +{
>> +    return rom->addr >= item->addr;
>> +}
>> +
>>  static void rom_insert(Rom *rom)
>>  {
>>      Rom *item;
>> @@ -796,12 +802,22 @@ static void rom_insert(Rom *rom)
>>          hw_error ("ROM images must be loaded at startup\n");
>>      }
>>
>> -    /* list is ordered by load address */
>> +    /* The user didn't specify an address space, this is the default */
>> +    if (!rom->as) {
>> +        rom->as = &address_space_memory;
>> +    }
>> +
>> +    /* 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->as == item->as) {
>> +            if (rom_order_compare(rom, item)) {
>> +                QTAILQ_INSERT_AFTER(&roms, item, rom, next);
>> +                return;
>> +            } else {
>> +                QTAILQ_INSERT_BEFORE(item, rom, next);
>> +                return;
>> +            }
>> +        }
>
> This isn't quite what I meant, and if we're doing it like this
> we might as well have "rom->addr >= item->addr" opencoded here.
>
> What I had in mind when I suggested a separate rom_order_compare()
> was that it should be doing the full ordering operation:
>     return (rom->as > item->as) ||
>         (rom->as == item->as && rom->addr >= item->addr);
>
> (ie it defines a total order on ROMs which for any two ROMs
> tells you which way round they go in the list).
>
> and then the change in this function would just be to change the old
>    if (rom->addr >= item->addr)
> to
>    if (rom_order_compare(rom, item))

Aw, now I understand. I was a little confused why you wanted the extra function.

>
> This has the advantage that it's really obvious that we're
> maintaining a sorted array and all we've done is change the
> function defining our total order on ROMs.
>
> The problem which I've realised in the course of thinking about
> this this evening is that the compare function I suggest above:
>  (a) requires a total ordering on pointers, which is not
>   guaranteed by the C standard
>  (b) even if you cast the rom->as pointers all to uintptr_t
>   (getting you defined behaviour) still gives you an ordering
>   which is variable from run to run of QEMU, which isn't great.
> Unfortunately there's no other obvious ordering on AddressSpaces
> (I guess you could look at their position in the address_spaces
> list, but that's getting pretty heavyweight.)
>
> That said, the code as you have it in this patch doesn't
> sort things properly I think. Consider three ROMs with
> addresses 0, 100, 200, all in the same AS, inserted in that order:
>  * ROM-0 is added as the first and only item in the list
>  * ROM-100 is added in the list after ROM-0 (same AS, and
>    rom_order_compare is true, so we do INSERT_AFTER)
>  * when we insert ROM-200 the first item in the list is ROM-0.
>    This has the same AS, and rom_order_compare(ROM-200, ROM-0)
>    is true, so we do an INSERT_AFTER.
> Now the list is (ROM-0, ROM-200, ROM-100), which isn't sorted.

I missed that case. Good catch.

>
>
> So our options are:
>  (1) fix up the open-coded code to sort properly
>  (2) figure out a rom_order_compare() that does give a total
>      ordering on ROMs which isn't variable from run to run of QEMU
>  (3) decide that it's OK for the ROM list ordering not to be
>      the same from run to run (or on two sides of a migration)
>      so we can use the cast-to-uintptr_t version

Remember that the original reason to order these ROMs was to ensure
that there were no overlaps. So I don't think that it matters too much
if the address space ordering changes.

One solution though to sort them is to sort the address spaces by
name. Are they guaranteed to have a unique names?

Thanks,

Alistair

>
> I don't currently have an opinion (well, I would like 2 except
> I can't think of a workable definition for the comparison function).
> Anybody?
>
> thanks
> -- PMM
>
diff mbox

Patch

diff --git a/hw/core/loader.c b/hw/core/loader.c
index 6b61f29..aef7bc9 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;
@@ -788,6 +789,11 @@  struct Rom {
 static FWCfgState *fw_cfg;
 static QTAILQ_HEAD(, Rom) roms = QTAILQ_HEAD_INITIALIZER(roms);
 
+static inline bool rom_order_compare(Rom *rom, Rom *item)
+{
+    return rom->addr >= item->addr;
+}
+
 static void rom_insert(Rom *rom)
 {
     Rom *item;
@@ -796,12 +802,22 @@  static void rom_insert(Rom *rom)
         hw_error ("ROM images must be loaded at startup\n");
     }
 
-    /* list is ordered by load address */
+    /* The user didn't specify an address space, this is the default */
+    if (!rom->as) {
+        rom->as = &address_space_memory;
+    }
+
+    /* 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->as == item->as) {
+            if (rom_order_compare(rom, item)) {
+                QTAILQ_INSERT_AFTER(&roms, item, rom, next);
+                return;
+            } else {
+                QTAILQ_INSERT_BEFORE(item, rom, next);
+                return;
+            }
+        }
     }
     QTAILQ_INSERT_TAIL(&roms, rom, next);
 }
@@ -833,16 +849,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");
+        /* 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 +994,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 +1004,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,8 +1034,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,
-                                          rom->addr, rom->data, rom->datasize);
+            cpu_physical_memory_write_rom(rom->as, rom->addr, rom->data,
+                                          rom->datasize);
         }
         if (rom->isrom) {
             /* rom needs to be written only once */
@@ -1031,12 +1057,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",
@@ -1049,6 +1076,7 @@  int rom_check_and_register_reset(void)
                                      rom->addr, 1);
         rom->isrom = int128_nz(section.size) && memory_region_is_rom(section.mr);
         memory_region_unref(section.mr);
+        as = rom->as;
     }
     qemu_register_reset(rom_reset, NULL);
     roms_loaded = 1;
diff --git a/include/hw/elf_ops.h b/include/hw/elf_ops.h
index 5038c7f..4744d11 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 c59673d..815a8d6 100644
--- a/include/hw/loader.h
+++ b/include/hw/loader.h
@@ -120,14 +120,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);
@@ -137,11 +137,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