From patchwork Wed Sep 17 22:05:37 2014 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 8bit X-Patchwork-Submitter: Ard Biesheuvel X-Patchwork-Id: 390565 Return-Path: X-Original-To: incoming@patchwork.ozlabs.org Delivered-To: patchwork-incoming@bilbo.ozlabs.org Received: from lists.gnu.org (lists.gnu.org [IPv6:2001:4830:134:3::11]) (using TLSv1 with cipher AES256-SHA (256/256 bits)) (No client certificate requested) by ozlabs.org (Postfix) with ESMTPS id 266CA1400D5 for ; Thu, 18 Sep 2014 08:06:15 +1000 (EST) Received: from localhost ([::1]:47477 helo=lists.gnu.org) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1XUNMb-0007u0-Mf for incoming@patchwork.ozlabs.org; Wed, 17 Sep 2014 18:06:13 -0400 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) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20130820; h=x-gm-message-state:mime-version:in-reply-to:references:date :message-id:subject:from:to:cc:content-type :content-transfer-encoding; bh=aYEM5jjhHnwdiSmanRv5+J95N3Lf5srvmRhiOXFgypg=; b=L16UVSmeW28SXhNlDKWkMbShkBQPThYEPGb2UZv+Z2IDtkSxoLg3sRAg4RFcQgxsHk O5n/kudzGpBfY5b9YQHPdP6dkNVOFYphnI3TjDvxtBT8vUfj5DbCl1dvp3AlGXIG6lMC IKjBLD7awko0pPWdt2nnMxmN9nIJeiAhgDvkyyVnVesEcEcGoflYX0kzfOSNp/qNXL7/ 5ramIB8IG0jgsUX/14PRUce8tGnXId8HP24Dao9MK0/HBn2nQENTjOAK8UPFz9cRu2Oa MwyzwX8UimlL5eDhdFgVt5FinkzNxNPHPuzfxhT8yxfHYvQyVQz79gotHYSatXLTXa5T tfZA== X-Gm-Message-State: ALoCoQkLrwKbbbtDvwY0qaf7a2GeH4hSLDVwU3ulc2v+R9jtiW8DtZ4Bqu6dsECGrTwda2b3uy0M MIME-Version: 1.0 X-Received: by 10.112.134.169 with SMTP id pl9mr82556lbb.75.1410991537546; Wed, 17 Sep 2014 15:05:37 -0700 (PDT) Received: by 10.112.84.67 with HTTP; Wed, 17 Sep 2014 15:05:37 -0700 (PDT) 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 To: =?UTF-8?Q?Andreas_F=C3=A4rber?= X-detected-operating-system: by eggs.gnu.org: GNU/Linux 2.2.x-3.x [generic] [fuzzy] X-Received-From: 209.85.217.175 Cc: Peter Maydell , Fu Wei , QEMU Developers , Christoffer Dall Subject: Re: [Qemu-devel] [PATCH 4/6] hw/arm/boot: register cpu reset handlers if using -bios X-BeenThere: qemu-devel@nongnu.org X-Mailman-Version: 2.1.14 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: qemu-devel-bounces+incoming=patchwork.ozlabs.org@nongnu.org Sender: qemu-devel-bounces+incoming=patchwork.ozlabs.org@nongnu.org On 17 September 2014 10:14, Andreas Färber wrote: > Am 17.09.2014 um 18:47 schrieb Peter Maydell: >> On 17 September 2014 09:40, Andreas Färber 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., @@ -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... > 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) {