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

[Eric Auger <eric.auger@linaro.org>]

Hi Peter,
On 06/12/2015 08:04 PM, Peter Crosthwaite wrote:
> On Fri, Jun 12, 2015 at 3:04 AM, Peter Maydell <peter.maydell@linaro.org> wrote:
>> On 12 June 2015 at 09:53, Eric Auger <eric.auger@linaro.org> 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?
Thanks for testing & reporting.
> 
> 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?

OK I see your point. Do you think it could be acceptable to put
rom_load_all after machine init done, combined with rom_load_done, in
vl.c?

In practice, as far as I understand, rom_load_all does check whether
register ROM regions overlap and whether their associated MR is readonly
RAM. In the positive this isrom field is set.

If isrom is not correctly set this has for consequence:
- bad info reported about memory regions in monitor
- rom->data pointer not freed on rom_reset reset notifier.
As you I don't think such regressions are acceptable.
I did not see any other usage of isrom?

If the above is not sensible, due to other side effects I do not see,
the other way around to achieve my goal of building the dt at  machine
init done notifier time would be to only postpone the dtb creation and
not the whole ARM load kernel. But dtb also uses rom_add_blob_fixed and
adds a ROM region which would not be dealt with rom_load_all code.

Best Regards

Eric



> 
> Regards,
> Peter
> 
>> -- PMM
>>