[Qemu-devel] [PATCH] hw/arm/boot: fix rom_reset notifier registration order

[Qemu-devel] [PATCH] hw/arm/boot: fix rom_reset notifier registration order

[Eric Auger]

commit ac9d32e39664e060cd1b538ff190980d57ad69e4 had the consequence to
register the do_cpu_reset after the rom_reset one. Hence they get
executed in the wrong order. This commit restores the registration of
do_cpu_reset in arm_load_kernel.

Signed-off-by: Eric Auger <eric.auger@linaro.org>
---
 hw/arm/boot.c | 20 +++++++++++---------
 1 file changed, 11 insertions(+), 9 deletions(-)

diff --git a/hw/arm/boot.c b/hw/arm/boot.c
index d036624..1e7fd28 100644
--- a/hw/arm/boot.c
+++ b/hw/arm/boot.c
@@ -574,15 +574,6 @@ static void arm_load_kernel_notify(Notifier *notifier, void *data)
     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
-     * actually loading a kernel, the handler is also responsible for
-     * arranging that we start it correctly.
-     */
-    for (cs = CPU(cpu); cs; cs = CPU_NEXT(cs)) {
-        qemu_register_reset(do_cpu_reset, ARM_CPU(cs));
-    }
-
     /* Load the kernel.  */
     if (!info->kernel_filename || info->firmware_loaded) {
 
@@ -783,7 +774,18 @@ static void arm_load_kernel_notify(Notifier *notifier, void *data)
 
 void arm_load_kernel(ARMCPU *cpu, struct arm_boot_info *info)
 {
+    CPUState *cs;
+
     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);
+
+    /* CPU objects (unlike devices) are not automatically reset on system
+     * reset, so we must always register a handler to do so. If we're
+     * actually loading a kernel, the handler is also responsible for
+     * arranging that we start it correctly.
+     */
+    for (cs = CPU(cpu); cs; cs = CPU_NEXT(cs)) {
+        qemu_register_reset(do_cpu_reset, ARM_CPU(cs));
+    }
 }
-- 
1.8.3.2

Re: [Qemu-devel] [PATCH] hw/arm/boot: fix rom_reset notifier registration order

[Eric Auger]

Hi Peter,

Please can you test whether this patch fixes the issue on xlnx-ep108
board. I acknowledge I do not feel comfortable with that piece of code
and meanwhile I continue looking at boot.c & loader.c files.

Thank you in advance

Best Regards

Eric

On 06/12/2015 02:19 PM, Eric Auger wrote:
> commit ac9d32e39664e060cd1b538ff190980d57ad69e4 had the consequence to
> register the do_cpu_reset after the rom_reset one. Hence they get
> executed in the wrong order. This commit restores the registration of
> do_cpu_reset in arm_load_kernel.
> 
> Signed-off-by: Eric Auger <eric.auger@linaro.org>
> ---
>  hw/arm/boot.c | 20 +++++++++++---------
>  1 file changed, 11 insertions(+), 9 deletions(-)
> 
> diff --git a/hw/arm/boot.c b/hw/arm/boot.c
> index d036624..1e7fd28 100644
> --- a/hw/arm/boot.c
> +++ b/hw/arm/boot.c
> @@ -574,15 +574,6 @@ static void arm_load_kernel_notify(Notifier *notifier, void *data)
>      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
> -     * actually loading a kernel, the handler is also responsible for
> -     * arranging that we start it correctly.
> -     */
> -    for (cs = CPU(cpu); cs; cs = CPU_NEXT(cs)) {
> -        qemu_register_reset(do_cpu_reset, ARM_CPU(cs));
> -    }
> -
>      /* Load the kernel.  */
>      if (!info->kernel_filename || info->firmware_loaded) {
>  
> @@ -783,7 +774,18 @@ static void arm_load_kernel_notify(Notifier *notifier, void *data)
>  
>  void arm_load_kernel(ARMCPU *cpu, struct arm_boot_info *info)
>  {
> +    CPUState *cs;
> +
>      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);
> +
> +    /* CPU objects (unlike devices) are not automatically reset on system
> +     * reset, so we must always register a handler to do so. If we're
> +     * actually loading a kernel, the handler is also responsible for
> +     * arranging that we start it correctly.
> +     */
> +    for (cs = CPU(cpu); cs; cs = CPU_NEXT(cs)) {
> +        qemu_register_reset(do_cpu_reset, ARM_CPU(cs));
> +    }
>  }
> 

Re: [Qemu-devel] [PATCH] hw/arm/boot: fix rom_reset notifier registration order

[Peter Crosthwaite]

On Fri, Jun 12, 2015 at 5:19 AM, Eric Auger <eric.auger@linaro.org> wrote:
> commit ac9d32e39664e060cd1b538ff190980d57ad69e4 had the consequence to
> register the do_cpu_reset after the rom_reset one. Hence they get
> executed in the wrong order. This commit restores the registration of
> do_cpu_reset in arm_load_kernel.
>
> Signed-off-by: Eric Auger <eric.auger@linaro.org>

Reported-by: Peter Crosthwaite <peter.crosthwaite@xilinx.com>
Tested-by: Peter Crosthwaite <peter.crosthwaite@xilinx.com>
Reviewed-by: Peter Crosthwaite <peter.crosthwaite@xilinx.com>

I don't think this is the ideal solution but it does fix my bug and it
is straight revert of the critical part of 9d32.., so it is OK as the
near term fix. I'll make some comments on the other thread.

Regards,
Peter

> ---
>  hw/arm/boot.c | 20 +++++++++++---------
>  1 file changed, 11 insertions(+), 9 deletions(-)
>
> diff --git a/hw/arm/boot.c b/hw/arm/boot.c
> index d036624..1e7fd28 100644
> --- a/hw/arm/boot.c
> +++ b/hw/arm/boot.c
> @@ -574,15 +574,6 @@ static void arm_load_kernel_notify(Notifier *notifier, void *data)
>      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
> -     * actually loading a kernel, the handler is also responsible for
> -     * arranging that we start it correctly.
> -     */
> -    for (cs = CPU(cpu); cs; cs = CPU_NEXT(cs)) {
> -        qemu_register_reset(do_cpu_reset, ARM_CPU(cs));
> -    }
> -
>      /* Load the kernel.  */
>      if (!info->kernel_filename || info->firmware_loaded) {
>
> @@ -783,7 +774,18 @@ static void arm_load_kernel_notify(Notifier *notifier, void *data)
>
>  void arm_load_kernel(ARMCPU *cpu, struct arm_boot_info *info)
>  {
> +    CPUState *cs;
> +
>      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);
> +
> +    /* CPU objects (unlike devices) are not automatically reset on system
> +     * reset, so we must always register a handler to do so. If we're
> +     * actually loading a kernel, the handler is also responsible for
> +     * arranging that we start it correctly.
> +     */
> +    for (cs = CPU(cpu); cs; cs = CPU_NEXT(cs)) {
> +        qemu_register_reset(do_cpu_reset, ARM_CPU(cs));
> +    }
>  }
> --
> 1.8.3.2
>
>

Re: [Qemu-devel] [PATCH] hw/arm/boot: fix rom_reset notifier registration order

[Peter Maydell]

On 12 June 2015 at 18:56, Peter Crosthwaite
<peter.crosthwaite@xilinx.com> wrote:
> On Fri, Jun 12, 2015 at 5:19 AM, Eric Auger <eric.auger@linaro.org> wrote:
>> commit ac9d32e39664e060cd1b538ff190980d57ad69e4 had the consequence to
>> register the do_cpu_reset after the rom_reset one. Hence they get
>> executed in the wrong order. This commit restores the registration of
>> do_cpu_reset in arm_load_kernel.
>>
>> Signed-off-by: Eric Auger <eric.auger@linaro.org>
>
> Reported-by: Peter Crosthwaite <peter.crosthwaite@xilinx.com>
> Tested-by: Peter Crosthwaite <peter.crosthwaite@xilinx.com>
> Reviewed-by: Peter Crosthwaite <peter.crosthwaite@xilinx.com>
>
> I don't think this is the ideal solution but it does fix my bug and it
> is straight revert of the critical part of 9d32.., so it is OK as the
> near term fix. I'll make some comments on the other thread.

Applied to target-arm.next, thanks.

-- PMM