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

[Ard Biesheuvel]

On 17 September 2014 10:14, Andreas Färber <address@hidden> wrote:
> Am 17.09.2014 um 18:47 schrieb Peter Maydell:
>> On 17 September 2014 09:40, Andreas Färber <address@hidden> wrote:
>>> We avoided that by not using DeviceClass::reset but CPUClass::reset.
>>> It's a question of assuring appropriate reset ordering between CPU and
>>> devices. PowerPC needed a special reset order via hook in (what is now)
>>> MachineClass.
>>
>>> So while I agree that CPU reset registration is not ideal and needs
>>> changing, I am not convinced that we can generally make the change and
>>> hope for the best. I wouldn't mind an incremental transition though,
>>> with arm taking the first step - still leaves the question of exact
>>> direction. If you look at x86, you will find that despite my protest
>>> against this inconsistency, the reset hook registration was moved into
>>> CPU code but none of the other targets changed alongside.
>>
>> I don't object to taking a pragmatic approach in the ARM code
>> (eg this patch). I just wanted to know if you had a preferred
>> direction we should be taking instead (which as you say we
>> kind of have to do in an incremental way). It sounds like you
>> don't have anything concrete in mind so maybe we should just
>> apply this patch.
>
> Ack.
>
> One other concern I have with this patch is the loop assuming that all
> following CPUs will be of type ARMCPU, but I suspect there will be other
> code making the same assumption - in that case Reviewed-by.
>

Thanks.

So if this is the correct approach, it probably makes sense to modify
the patch and just move the existing code to the top of the function,
rather than having the duplicated for loop.
I.e.,

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);
-    }
 }

@Peter: would you like a new patch or are you happy to take it as is?

Cheers,
Ard.



>> In general I suspect there are a lot of unresolved issues in
>> our handling of reset -- it's a complicated area which we
>> attempt to address in an over-simplistic way at the moment :-(
>
> Yes, having test cases for all machines would help refactor these things...
>