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

[Peter Maydell]

On 3 August 2016 at 21:06, 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>
> ---
> 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