Re: [Qemu-devel] [PULL 20/22] hw/arm/boot: arm_load_kernel implemented as a machine init done notifier

[Peter Crosthwaite]

On Fri, Jun 12, 2015 at 3:04 AM, Peter Maydell <address@hidden> wrote:
> On 12 June 2015 at 09:53, Eric Auger <address@hidden> wrote:
>> On 06/12/2015 10:25 AM, Eric Auger wrote:
>>>>  I think it is because this is now delaying
>>>> arm_load_kernel_notify call until after rom_load_all. From vl.c:
>>>>
>>>>     if (rom_load_all() != 0) {
>>>>         fprintf(stderr, "rom loading failed\n");
>>>>         exit(1);
>>>>     }
>>>>
>>>>     /* TODO: once all bus devices are qdevified, this should be done
>>>>      * when bus is created by qdev.c */
>>>>     qemu_register_reset(qbus_reset_all_fn, sysbus_get_default());
>>>>     qemu_run_machine_init_done_notifiers();
>>>>
>>>> the machine_init_done_notifiers are called after the rom_load_all()
>>>> call which does the image loading.
>>
>> Isn't the actual rom loading done in a reset notifier? If confirmed the
>> problem comes from the fact the order of registration of reset notifiers
>> for rom_reset and do_cpu_reset has swapped?
>
> Yes, actual writing of rom data to ram happens in rom_reset_all().
> This does seem to be called after arm_load_kernel_notify and
> before do_cpu_reset, which is the order I would expect we require...
>

So Eric's cpu reset re-ordering patch does fix it. But I thought we
were trying to make reset ordering independent?

I think the issue I describe above wrt rom_load_all is still real
though, just with a more minor consequence. The notify patch is such
that rom_load_all is now called before arm_load_kernel_notify. Looking
at rom_load_all:

int rom_load_all(void)
{
    hwaddr addr = 0;
    MemoryRegionSection section;
    Rom *rom;

    QTAILQ_FOREACH(rom, &roms, next) {
        if (rom->fw_file) {
            continue;
        }
        if (addr > rom->addr) {
            fprintf(stderr, "rom: requested regions overlap "
                    "(rom %s. free=0x" TARGET_FMT_plx
                    ", addr=0x" TARGET_FMT_plx ")\n",
                    rom->name, addr, rom->addr);
            return -1;
        }
        addr  = rom->addr;
        addr += rom->romsize;
        section = memory_region_find(get_system_memory(), rom->addr, 1);
        rom->isrom = int128_nz(section.size) &&
memory_region_is_rom(section.mr);
        memory_region_unref(section.mr);
    }
    qemu_register_reset(rom_reset, NULL);
    return 0;
}

There is a list iterator over the ROM list doing some tweaking for the
isrom field. This iteration process is now skipped for arm_load_kernel
loading, as the rom_insert calls happen after the rom_load_all. Does
this mean the arm_load_kernel no longer works on is_rom MRs?

Regards,
Peter

> -- PMM
>