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

[Eric Auger]

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 <address@hidden> wrote:
>>> From: Eric Auger <address@hidden>
>>>
>>> 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 <address@hidden>
>>> Reviewed-by: Shannon Zhao <address@hidden>
>>> Reviewed-by: Alexander Graf <address@hidden>
>>> Reviewed-by: Alex Bennée <address@hidden>
>>> Message-id: address@hidden
>>> Signed-off-by: Peter Maydell <address@hidden>
>>> ---
>>>  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
>>>
>>>
>