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

[Alistair Francis]

On Tue, Aug 9, 2016 at 11:44 AM, Peter Maydell <address@hidden> wrote:
> 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))

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
>