From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:56235) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Z3KHu-0005Jx-KE for qemu-devel@nongnu.org; Fri, 12 Jun 2015 04:26:07 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1Z3KHr-0002pe-AG for qemu-devel@nongnu.org; Fri, 12 Jun 2015 04:26:06 -0400 Received: from mail-wg0-f53.google.com ([74.125.82.53]:36487) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Z3KHr-0002nT-22 for qemu-devel@nongnu.org; Fri, 12 Jun 2015 04:26:03 -0400 Received: by wgbgq6 with SMTP id gq6so19180421wgb.3 for ; Fri, 12 Jun 2015 01:26:02 -0700 (PDT) Message-ID: <557A978B.1090701@linaro.org> Date: Fri, 12 Jun 2015 10:25:47 +0200 From: Eric Auger MIME-Version: 1.0 References: <1433262832-11527-1-git-send-email-peter.maydell@linaro.org> <1433262832-11527-21-git-send-email-peter.maydell@linaro.org> In-Reply-To: Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 8bit Subject: Re: [Qemu-devel] [PULL 20/22] hw/arm/boot: arm_load_kernel implemented as a machine init done notifier List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Peter Crosthwaite , Peter Maydell , Shannon Zhao , Alexander Graf , =?UTF-8?B?QWxleCBCZW5uw6ll?= Cc: "qemu-devel@nongnu.org Developers" Hi Peter, On 06/12/2015 04:54 AM, Peter Crosthwaite wrote: > On Tue, Jun 2, 2015 at 9:33 AM, Peter Maydell wrote: >> From: Eric Auger >> >> 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 >> Reviewed-by: Shannon Zhao >> Reviewed-by: Alexander Graf >> Reviewed-by: Alex Bennée >> Message-id: 1433244554-12898-3-git-send-email-eric.auger@linaro.org >> Signed-off-by: Peter Maydell >> --- >> 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 >> >>