[Qemu-devel] [PATCH v2] hw/arm/boot: register cpu reset handlers if using -bios

[Qemu-devel] [PATCH v2] hw/arm/boot: register cpu reset handlers if using -bios

[Ard Biesheuvel]

Move the registering of CPU reset handlers to before the point where
we leave the function in the -bios (not -kernel) case, so CPU reset
works correctly with -bios as well.

Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
---
 hw/arm/boot.c | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/hw/arm/boot.c b/hw/arm/boot.c
index c8dc34f0865b..fc6a3ebf853d 100644
--- a/hw/arm/boot.c
+++ b/hw/arm/boot.c
@@ -488,6 +488,12 @@ void arm_load_kernel(ARMCPU *cpu, struct arm_boot_info *info)
     int big_endian;
     static const ARMInsnFixup *primary_loader;
 
+    for (; cs; cs = CPU_NEXT(cs)) {
+        cpu = ARM_CPU(cs);
+        cpu->env.boot_info = info;
+        qemu_register_reset(do_cpu_reset, cpu);
+    }
+
     /* Load the kernel.  */
     if (!info->kernel_filename) {
 
@@ -651,10 +657,4 @@ void arm_load_kernel(ARMCPU *cpu, struct arm_boot_info *info)
         }
     }
     info->is_linux = is_linux;
-
-    for (; cs; cs = CPU_NEXT(cs)) {
-        cpu = ARM_CPU(cs);
-        cpu->env.boot_info = info;
-        qemu_register_reset(do_cpu_reset, cpu);
-    }
 }
-- 
1.8.3.2

Re: [Qemu-devel] [PATCH v2] hw/arm/boot: register cpu reset handlers if using -bios

[Peter Maydell]

On 10 October 2014 12:35, Ard Biesheuvel <ard.biesheuvel@linaro.org> wrote:
> Move the registering of CPU reset handlers to before the point where
> we leave the function in the -bios (not -kernel) case, so CPU reset
> works correctly with -bios as well.
>
> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> ---
>  hw/arm/boot.c | 12 ++++++------
>  1 file changed, 6 insertions(+), 6 deletions(-)
>
> diff --git a/hw/arm/boot.c b/hw/arm/boot.c
> index c8dc34f0865b..fc6a3ebf853d 100644
> --- a/hw/arm/boot.c
> +++ b/hw/arm/boot.c
> @@ -488,6 +488,12 @@ void arm_load_kernel(ARMCPU *cpu, struct arm_boot_info *info)
>      int big_endian;
>      static const ARMInsnFixup *primary_loader;
>
> +    for (; cs; cs = CPU_NEXT(cs)) {
> +        cpu = ARM_CPU(cs);
> +        cpu->env.boot_info = info;
> +        qemu_register_reset(do_cpu_reset, cpu);
> +    }
> +
>      /* Load the kernel.  */
>      if (!info->kernel_filename) {
>
> @@ -651,10 +657,4 @@ void arm_load_kernel(ARMCPU *cpu, struct arm_boot_info *info)
>          }
>      }
>      info->is_linux = is_linux;
> -
> -    for (; cs; cs = CPU_NEXT(cs)) {
> -        cpu = ARM_CPU(cs);
> -        cpu->env.boot_info = info;
> -        qemu_register_reset(do_cpu_reset, cpu);
> -    }
>  }

I've just realised this isn't quite right -- we now call
the reset hook in the case where there's no kernel_filename,
but we pass it a non-NULL info pointer, which means the
reset hook will change the PC to info->entry (which might
not even be initialized) rather than leaving it at the
reset value set by the cpu's own reset function.

One way to fix this would be to have the loop at the
top of the function which registers the reset fn not
touch cpu->env.boot_info (it will default to NULL), and
then retain the loop at the bottom of the function just
to set it the boot_info pointer, since at that point we
know we have a valid kernel image to load.

We could probably also use a comment for the loop at the
top. Here's one:

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

thanks
-- PMM