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: 37551 Return-Path: X-Original-To: linaro@patches.linaro.org Delivered-To: linaro@patches.linaro.org Received: from mail-ee0-f69.google.com (mail-ee0-f69.google.com [74.125.83.69]) by ip-10-151-82-157.ec2.internal (Postfix) with ESMTPS id 9D4782057E for ; Wed, 17 Sep 2014 22:06:28 +0000 (UTC) Received: by mail-ee0-f69.google.com with SMTP id t10sf77543eei.8 for ; Wed, 17 Sep 2014 15:06:27 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20130820; h=x-gm-message-state:delivered-to:mime-version:in-reply-to:references :date:message-id:from:to:cc:subject:precedence:list-id :list-unsubscribe:list-archive:list-post:list-help:list-subscribe :errors-to:sender:x-original-sender :x-original-authentication-results:mailing-list:content-type :content-transfer-encoding; bh=aYEM5jjhHnwdiSmanRv5+J95N3Lf5srvmRhiOXFgypg=; b=aD8dtQJQtc8r9fpKkizTLD4Oy+IJ4AWXkz0SvReVZGj+PyGTMJ3Li6wlSMSjj8Yj+j qIh0VwrnwBEf4vKPtlYlQK/sAeHLOTSqQKUhbJBsYQgxocDfBxHJKKCITMvnuwjI4nX1 C8qloNeL3ZPNWxDecJwQZzOHd3g3IHxnKUbiu00g5ODh4eBE0IGxvjPN0sa9tmhhLIPE mqORK8ZLDjgiYzJvxJ4QZLu2SZNu8GG7cO/HOHJWWQBmCWDGleczFExTGTpVquevDDN4 dxuIOMHV4bWJLqUV7bu6VyCbnS0ZvVZ0j1Ayklb2kC77ksrIkesLQ1yNgI2pGQluSrZ8 NMsw== X-Gm-Message-State: ALoCoQlHodEk8QGLzVrrJXd4Cpti/T0RwPbE66L1/41WSp9tWZrddCr7GABIqSEguodHP4K5iSvX X-Received: by 10.180.72.141 with SMTP id d13mr3801338wiv.2.1410991587809; Wed, 17 Sep 2014 15:06:27 -0700 (PDT) X-BeenThere: patchwork-forward@linaro.org Received: by 10.152.30.3 with SMTP id o3ls153318lah.14.gmail; Wed, 17 Sep 2014 15:06:27 -0700 (PDT) X-Received: by 10.112.253.165 with SMTP id ab5mr163525lbd.1.1410991587181; Wed, 17 Sep 2014 15:06:27 -0700 (PDT) Received: from mail-lb0-f182.google.com (mail-lb0-f182.google.com [209.85.217.182]) by mx.google.com with ESMTPS id gk7si7860438lbc.43.2014.09.17.15.06.27 for (version=TLSv1 cipher=ECDHE-RSA-RC4-SHA bits=128/128); Wed, 17 Sep 2014 15:06:27 -0700 (PDT) Received-SPF: pass (google.com: domain of patch+caf_=patchwork-forward=linaro.org@linaro.org designates 209.85.217.182 as permitted sender) client-ip=209.85.217.182; Received: by mail-lb0-f182.google.com with SMTP id u10so2793314lbd.27 for ; Wed, 17 Sep 2014 15:06:27 -0700 (PDT) X-Received: by 10.112.200.134 with SMTP id js6mr283897lbc.0.1410991587074; Wed, 17 Sep 2014 15:06:27 -0700 (PDT) X-Forwarded-To: patchwork-forward@linaro.org X-Forwarded-For: patch@linaro.org patchwork-forward@linaro.org Delivered-To: patch@linaro.org Received: by 10.112.130.169 with SMTP id of9csp662808lbb; Wed, 17 Sep 2014 15:06:26 -0700 (PDT) X-Received: by 10.229.229.135 with SMTP id ji7mr487715qcb.15.1410991585736; Wed, 17 Sep 2014 15:06:25 -0700 (PDT) Received: from lists.gnu.org (lists.gnu.org. [2001:4830:134:3::11]) by mx.google.com with ESMTPS id ez1si24036592qcb.18.2014.09.17.15.06.25 for (version=TLSv1 cipher=RC4-SHA bits=128/128); Wed, 17 Sep 2014 15:06:25 -0700 (PDT) Received-SPF: pass (google.com: domain of qemu-devel-bounces+patch=linaro.org@nongnu.org designates 2001:4830:134:3::11 as permitted sender) client-ip=2001:4830:134:3::11; Received: from localhost ([::1]:47477 helo=lists.gnu.org) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1XUNMm-0007u0-T9 for patch@linaro.org; Wed, 17 Sep 2014 18:06:24 -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) 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+patch=linaro.org@nongnu.org Sender: qemu-devel-bounces+patch=linaro.org@nongnu.org X-Removed-Original-Auth: Dkim didn't pass. X-Original-Sender: ard.biesheuvel@linaro.org X-Original-Authentication-Results: mx.google.com; spf=pass (google.com: domain of patch+caf_=patchwork-forward=linaro.org@linaro.org designates 209.85.217.182 as permitted sender) smtp.mail=patch+caf_=patchwork-forward=linaro.org@linaro.org Mailing-list: list patchwork-forward@linaro.org; contact patchwork-forward+owners@linaro.org X-Google-Group-Id: 836684582541 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) {