From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:57613) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1XUNMC-0007Tv-Mz for qemu-devel@nongnu.org; Wed, 17 Sep 2014 18:05:53 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1XUNM7-0002Hr-NJ for qemu-devel@nongnu.org; Wed, 17 Sep 2014 18:05:48 -0400 Received: from mail-lb0-f175.google.com ([209.85.217.175]:34280) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1XUNM7-0002Ha-Gp for qemu-devel@nongnu.org; Wed, 17 Sep 2014 18:05:43 -0400 Received: by mail-lb0-f175.google.com with SMTP id n15so2753112lbi.34 for ; Wed, 17 Sep 2014 15:05:37 -0700 (PDT) MIME-Version: 1.0 In-Reply-To: <5419C169.5060007@suse.de> References: <1409930126-28449-1-git-send-email-ard.biesheuvel@linaro.org> <1409930126-28449-5-git-send-email-ard.biesheuvel@linaro.org> <5419AEF3.8010101@suse.de> <5419B962.7060900@suse.de> <5419C169.5060007@suse.de> Date: Wed, 17 Sep 2014 15:05:37 -0700 Message-ID: From: Ard Biesheuvel Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: quoted-printable Subject: Re: [Qemu-devel] [PATCH 4/6] hw/arm/boot: register cpu reset handlers if using -bios List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: =?UTF-8?Q?Andreas_F=C3=A4rber?= Cc: Peter Maydell , Fu Wei , QEMU Developers , Christoffer Dall On 17 September 2014 10:14, Andreas F=C3=A4rber wrote: > Am 17.09.2014 um 18:47 schrieb Peter Maydell: >> On 17 September 2014 09:40, Andreas F=C3=A4rber 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 =3D CPU_NEXT(cs)) { + cpu =3D ARM_CPU(cs); + cpu->env.boot_info =3D 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 =3D is_linux; - - for (; cs; cs =3D CPU_NEXT(cs)) { - cpu =3D ARM_CPU(cs); - cpu->env.boot_info =3D 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.= .. >