[PULL,20/22] hw/arm/boot: arm_load_kernel implemented as a machine init done notifier

From: Eric Auger <eric.auger@linaro.org>

Device tree nodes for the platform bus and its children dynamic sysbus
devices are added in a machine init done notifier. To load the dtb once,
after those latter nodes are built and before ROM freeze, the actual
arm_load_kernel existing code is moved into a notifier notify function,
arm_load_kernel_notify. arm_load_kernel now only registers the
corresponding notifier.

Machine files that do not support platform bus stay unchanged. Machine
files willing to support dynamic sysbus devices must call arm_load_kernel
before sysbus-fdt arm_register_platform_bus_fdt_creator to make sure
dynamic sysbus device nodes are integrated in the dtb.

Signed-off-by: Eric Auger <eric.auger@linaro.org>
Reviewed-by: Shannon Zhao <zhaoshenglong@huawei.com>
Reviewed-by: Alexander Graf <agraf@suse.de>
Reviewed-by: Alex Bennée <alex.bennee@linaro.org>
Message-id: 1433244554-12898-3-git-send-email-eric.auger@linaro.org
Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
 hw/arm/boot.c        | 14 +++++++++++++-
 include/hw/arm/arm.h | 28 ++++++++++++++++++++++++++++
 2 files changed, 41 insertions(+), 1 deletion(-)

Hi Peter,
On 06/12/2015 04:54 AM, Peter Crosthwaite wrote:
> On Tue, Jun 2, 2015 at 9:33 AM, Peter Maydell <peter.maydell@linaro.org> wrote:
>> From: Eric Auger <eric.auger@linaro.org>
>>
>> Device tree nodes for the platform bus and its children dynamic sysbus
>> devices are added in a machine init done notifier. To load the dtb once,
>> after those latter nodes are built and before ROM freeze, the actual
>> arm_load_kernel existing code is moved into a notifier notify function,
>> arm_load_kernel_notify. arm_load_kernel now only registers the
>> corresponding notifier.
>>
> 
> Does this work? I am experiencing a regression on this patch for
> xlnx-ep108 board.

Sorry for the inconvenience. On my side I tested it on virt board.

I am currently looking at the issue ...

Best Regards

Eric
 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. So the image-to-load registration
> is too late.
> 
> Straight revert of this patch fixes the issue for me.
> 
> Regards,
> Peter
> 
> 
>> Machine files that do not support platform bus stay unchanged. Machine
>> files willing to support dynamic sysbus devices must call arm_load_kernel
>> before sysbus-fdt arm_register_platform_bus_fdt_creator to make sure
>> dynamic sysbus device nodes are integrated in the dtb.
>>
>> Signed-off-by: Eric Auger <eric.auger@linaro.org>
>> Reviewed-by: Shannon Zhao <zhaoshenglong@huawei.com>
>> Reviewed-by: Alexander Graf <agraf@suse.de>
>> Reviewed-by: Alex Bennée <alex.bennee@linaro.org>
>> Message-id: 1433244554-12898-3-git-send-email-eric.auger@linaro.org
>> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
>> ---
>>  hw/arm/boot.c        | 14 +++++++++++++-
>>  include/hw/arm/arm.h | 28 ++++++++++++++++++++++++++++
>>  2 files changed, 41 insertions(+), 1 deletion(-)
>>
>> diff --git a/hw/arm/boot.c b/hw/arm/boot.c
>> index fa69503..d036624 100644
>> --- a/hw/arm/boot.c
>> +++ b/hw/arm/boot.c
>> @@ -557,7 +557,7 @@ static void load_image_to_fw_cfg(FWCfgState *fw_cfg, uint16_t size_key,
>>      fw_cfg_add_bytes(fw_cfg, data_key, data, size);
>>  }
>>
>> -void arm_load_kernel(ARMCPU *cpu, struct arm_boot_info *info)
>> +static void arm_load_kernel_notify(Notifier *notifier, void *data)
>>  {
>>      CPUState *cs;
>>      int kernel_size;
>> @@ -568,6 +568,11 @@ void arm_load_kernel(ARMCPU *cpu, struct arm_boot_info *info)
>>      hwaddr entry, kernel_load_offset;
>>      int big_endian;
>>      static const ARMInsnFixup *primary_loader;
>> +    ArmLoadKernelNotifier *n = DO_UPCAST(ArmLoadKernelNotifier,
>> +                                         notifier, notifier);
>> +    ARMCPU *cpu = n->cpu;
>> +    struct arm_boot_info *info =
>> +        container_of(n, struct arm_boot_info, load_kernel_notifier);
>>
>>      /* CPU objects (unlike devices) are not automatically reset on system
>>       * reset, so we must always register a handler to do so. If we're
>> @@ -775,3 +780,10 @@ void arm_load_kernel(ARMCPU *cpu, struct arm_boot_info *info)
>>          ARM_CPU(cs)->env.boot_info = info;
>>      }
>>  }
>> +
>> +void arm_load_kernel(ARMCPU *cpu, struct arm_boot_info *info)
>> +{
>> +    info->load_kernel_notifier.cpu = cpu;
>> +    info->load_kernel_notifier.notifier.notify = arm_load_kernel_notify;
>> +    qemu_add_machine_init_done_notifier(&info->load_kernel_notifier.notifier);
>> +}
>> diff --git a/include/hw/arm/arm.h b/include/hw/arm/arm.h
>> index 5c940eb..760804c 100644
>> --- a/include/hw/arm/arm.h
>> +++ b/include/hw/arm/arm.h
>> @@ -13,11 +13,21 @@
>>
>>  #include "exec/memory.h"
>>  #include "hw/irq.h"
>> +#include "qemu/notify.h"
>>
>>  /* armv7m.c */
>>  qemu_irq *armv7m_init(MemoryRegion *system_memory, int mem_size, int num_irq,
>>                        const char *kernel_filename, const char *cpu_model);
>>
>> +/*
>> + * struct used as a parameter of the arm_load_kernel machine init
>> + * done notifier
>> + */
>> +typedef struct {
>> +    Notifier notifier; /* actual notifier */
>> +    ARMCPU *cpu; /* handle to the first cpu object */
>> +} ArmLoadKernelNotifier;
>> +
>>  /* arm_boot.c */
>>  struct arm_boot_info {
>>      uint64_t ram_size;
>> @@ -64,6 +74,8 @@ struct arm_boot_info {
>>       * the user it should implement this hook.
>>       */
>>      void (*modify_dtb)(const struct arm_boot_info *info, void *fdt);
>> +    /* machine init done notifier executing arm_load_dtb */
>> +    ArmLoadKernelNotifier load_kernel_notifier;
>>      /* Used internally by arm_boot.c */
>>      int is_linux;
>>      hwaddr initrd_start;
>> @@ -75,6 +87,22 @@ struct arm_boot_info {
>>       */
>>      bool firmware_loaded;
>>  };
>> +
>> +/**
>> + * arm_load_kernel - Loads memory with everything needed to boot
>> + *
>> + * @cpu: handle to the first CPU object
>> + * @info: handle to the boot info struct
>> + * Registers a machine init done notifier that copies to memory
>> + * everything needed to boot, depending on machine and user options:
>> + * kernel image, boot loaders, initrd, dtb. Also registers the CPU
>> + * reset handler.
>> + *
>> + * In case the machine file supports the platform bus device and its
>> + * dynamically instantiable sysbus devices, this function must be called
>> + * before sysbus-fdt arm_register_platform_bus_fdt_creator. Indeed the
>> + * machine init done notifiers are called in registration reverse order.
>> + */
>>  void arm_load_kernel(ARMCPU *cpu, struct arm_boot_info *info);
>>
>>  /* Multiplication factor to convert from system clock ticks to qemu timer
>> --
>> 1.9.1
>>
>>

On 06/12/2015 10:25 AM, Eric Auger wrote:
> Hi Peter,
> On 06/12/2015 04:54 AM, Peter Crosthwaite wrote:
>> On Tue, Jun 2, 2015 at 9:33 AM, Peter Maydell <peter.maydell@linaro.org> wrote:
>>> From: Eric Auger <eric.auger@linaro.org>
>>>
>>> Device tree nodes for the platform bus and its children dynamic sysbus
>>> devices are added in a machine init done notifier. To load the dtb once,
>>> after those latter nodes are built and before ROM freeze, the actual
>>> arm_load_kernel existing code is moved into a notifier notify function,
>>> arm_load_kernel_notify. arm_load_kernel now only registers the
>>> corresponding notifier.
>>>
>>
>> Does this work? I am experiencing a regression on this patch for
>> xlnx-ep108 board.
> 
> Sorry for the inconvenience. On my side I tested it on virt board.
> 
> I am currently looking at the issue ...
> 
> Best Regards
> 
> Eric
>  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?

Best Regards

Eric
 So the image-to-load registration
>> is too late.
>>
>> Straight revert of this patch fixes the issue for me.
>>
>> Regards,
>> Peter
>>
>>
>>> Machine files that do not support platform bus stay unchanged. Machine
>>> files willing to support dynamic sysbus devices must call arm_load_kernel
>>> before sysbus-fdt arm_register_platform_bus_fdt_creator to make sure
>>> dynamic sysbus device nodes are integrated in the dtb.
>>>
>>> Signed-off-by: Eric Auger <eric.auger@linaro.org>
>>> Reviewed-by: Shannon Zhao <zhaoshenglong@huawei.com>
>>> Reviewed-by: Alexander Graf <agraf@suse.de>
>>> Reviewed-by: Alex Bennée <alex.bennee@linaro.org>
>>> Message-id: 1433244554-12898-3-git-send-email-eric.auger@linaro.org
>>> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
>>> ---
>>>  hw/arm/boot.c        | 14 +++++++++++++-
>>>  include/hw/arm/arm.h | 28 ++++++++++++++++++++++++++++
>>>  2 files changed, 41 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/hw/arm/boot.c b/hw/arm/boot.c
>>> index fa69503..d036624 100644
>>> --- a/hw/arm/boot.c
>>> +++ b/hw/arm/boot.c
>>> @@ -557,7 +557,7 @@ static void load_image_to_fw_cfg(FWCfgState *fw_cfg, uint16_t size_key,
>>>      fw_cfg_add_bytes(fw_cfg, data_key, data, size);
>>>  }
>>>
>>> -void arm_load_kernel(ARMCPU *cpu, struct arm_boot_info *info)
>>> +static void arm_load_kernel_notify(Notifier *notifier, void *data)
>>>  {
>>>      CPUState *cs;
>>>      int kernel_size;
>>> @@ -568,6 +568,11 @@ void arm_load_kernel(ARMCPU *cpu, struct arm_boot_info *info)
>>>      hwaddr entry, kernel_load_offset;
>>>      int big_endian;
>>>      static const ARMInsnFixup *primary_loader;
>>> +    ArmLoadKernelNotifier *n = DO_UPCAST(ArmLoadKernelNotifier,
>>> +                                         notifier, notifier);
>>> +    ARMCPU *cpu = n->cpu;
>>> +    struct arm_boot_info *info =
>>> +        container_of(n, struct arm_boot_info, load_kernel_notifier);
>>>
>>>      /* CPU objects (unlike devices) are not automatically reset on system
>>>       * reset, so we must always register a handler to do so. If we're
>>> @@ -775,3 +780,10 @@ void arm_load_kernel(ARMCPU *cpu, struct arm_boot_info *info)
>>>          ARM_CPU(cs)->env.boot_info = info;
>>>      }
>>>  }
>>> +
>>> +void arm_load_kernel(ARMCPU *cpu, struct arm_boot_info *info)
>>> +{
>>> +    info->load_kernel_notifier.cpu = cpu;
>>> +    info->load_kernel_notifier.notifier.notify = arm_load_kernel_notify;
>>> +    qemu_add_machine_init_done_notifier(&info->load_kernel_notifier.notifier);
>>> +}
>>> diff --git a/include/hw/arm/arm.h b/include/hw/arm/arm.h
>>> index 5c940eb..760804c 100644
>>> --- a/include/hw/arm/arm.h
>>> +++ b/include/hw/arm/arm.h
>>> @@ -13,11 +13,21 @@
>>>
>>>  #include "exec/memory.h"
>>>  #include "hw/irq.h"
>>> +#include "qemu/notify.h"
>>>
>>>  /* armv7m.c */
>>>  qemu_irq *armv7m_init(MemoryRegion *system_memory, int mem_size, int num_irq,
>>>                        const char *kernel_filename, const char *cpu_model);
>>>
>>> +/*
>>> + * struct used as a parameter of the arm_load_kernel machine init
>>> + * done notifier
>>> + */
>>> +typedef struct {
>>> +    Notifier notifier; /* actual notifier */
>>> +    ARMCPU *cpu; /* handle to the first cpu object */
>>> +} ArmLoadKernelNotifier;
>>> +
>>>  /* arm_boot.c */
>>>  struct arm_boot_info {
>>>      uint64_t ram_size;
>>> @@ -64,6 +74,8 @@ struct arm_boot_info {
>>>       * the user it should implement this hook.
>>>       */
>>>      void (*modify_dtb)(const struct arm_boot_info *info, void *fdt);
>>> +    /* machine init done notifier executing arm_load_dtb */
>>> +    ArmLoadKernelNotifier load_kernel_notifier;
>>>      /* Used internally by arm_boot.c */
>>>      int is_linux;
>>>      hwaddr initrd_start;
>>> @@ -75,6 +87,22 @@ struct arm_boot_info {
>>>       */
>>>      bool firmware_loaded;
>>>  };
>>> +
>>> +/**
>>> + * arm_load_kernel - Loads memory with everything needed to boot
>>> + *
>>> + * @cpu: handle to the first CPU object
>>> + * @info: handle to the boot info struct
>>> + * Registers a machine init done notifier that copies to memory
>>> + * everything needed to boot, depending on machine and user options:
>>> + * kernel image, boot loaders, initrd, dtb. Also registers the CPU
>>> + * reset handler.
>>> + *
>>> + * In case the machine file supports the platform bus device and its
>>> + * dynamically instantiable sysbus devices, this function must be called
>>> + * before sysbus-fdt arm_register_platform_bus_fdt_creator. Indeed the
>>> + * machine init done notifiers are called in registration reverse order.
>>> + */
>>>  void arm_load_kernel(ARMCPU *cpu, struct arm_boot_info *info);
>>>
>>>  /* Multiplication factor to convert from system clock ticks to qemu timer
>>> --
>>> 1.9.1
>>>
>>>
>

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...

-- PMM

On 12 June 2015 at 19:04, Peter Crosthwaite
<peter.crosthwaite@xilinx.com> wrote:
> So Eric's cpu reset re-ordering patch does fix it. But I thought we
> were trying to make reset ordering independent?

I think it would be more accurate to say that we hope reset
ordering is independent because we have no good way of
describing and enforcing any ordering requirements :-)

-- PMM

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
>>