From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-5.3 required=3.0 tests=BAYES_00, HEADER_FROM_DIFFERENT_DOMAINS,MAILING_LIST_MULTI,NICE_REPLY_A,SPF_HELO_NONE, SPF_PASS,USER_AGENT_SANE_1 autolearn=no autolearn_force=no version=3.4.0 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id 90413C5519F for ; Wed, 25 Nov 2020 09:36:29 +0000 (UTC) Received: from lists.gnu.org (lists.gnu.org [209.51.188.17]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by mail.kernel.org (Postfix) with ESMTPS id C5F19206B5 for ; Wed, 25 Nov 2020 09:36:28 +0000 (UTC) DMARC-Filter: OpenDMARC Filter v1.3.2 mail.kernel.org C5F19206B5 Authentication-Results: mail.kernel.org; dmarc=none (p=none dis=none) header.from=suse.de Authentication-Results: mail.kernel.org; spf=pass smtp.mailfrom=qemu-devel-bounces+qemu-devel=archiver.kernel.org@nongnu.org Received: from localhost ([::1]:39668 helo=lists1p.gnu.org) by lists.gnu.org with esmtp (Exim 4.90_1) (envelope-from ) id 1khrDv-0008Ge-Kv for qemu-devel@archiver.kernel.org; Wed, 25 Nov 2020 04:36:27 -0500 Received: from eggs.gnu.org ([2001:470:142:3::10]:60420) by lists.gnu.org with esmtps (TLS1.2:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.90_1) (envelope-from ) id 1khr9j-0004II-MF for qemu-devel@nongnu.org; Wed, 25 Nov 2020 04:32:09 -0500 Received: from mx2.suse.de ([195.135.220.15]:56940) by eggs.gnu.org with esmtps (TLS1.2:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.90_1) (envelope-from ) id 1khr9h-0006hC-H1 for qemu-devel@nongnu.org; Wed, 25 Nov 2020 04:32:07 -0500 X-Virus-Scanned: by amavisd-new at test-mx.suse.de Received: from relay2.suse.de (unknown [195.135.221.27]) by mx2.suse.de (Postfix) with ESMTP id BC2CDAC6A; Wed, 25 Nov 2020 09:32:02 +0000 (UTC) Subject: Re: [RFC v5 12/12] accel: centralize initialization of CpusAccelOps To: Eduardo Habkost References: <20201124162210.8796-1-cfontana@suse.de> <20201124162210.8796-13-cfontana@suse.de> <20201124174821.GT2271382@habkost.net> <4deb0de9-9556-85da-76fb-8050551d6dd6@suse.de> <20201124192756.GX2271382@habkost.net> <115546ec-1024-e515-8eba-b89937fb23ac@suse.de> <20201124203452.GZ2271382@habkost.net> From: Claudio Fontana Message-ID: <60e9ff3e-8896-c9a1-302c-c1378a48a564@suse.de> Date: Wed, 25 Nov 2020 10:32:01 +0100 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:68.0) Gecko/20100101 Thunderbird/68.12.0 MIME-Version: 1.0 In-Reply-To: <20201124203452.GZ2271382@habkost.net> Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 7bit Received-SPF: pass client-ip=195.135.220.15; envelope-from=cfontana@suse.de; helo=mx2.suse.de X-Spam_score_int: -41 X-Spam_score: -4.2 X-Spam_bar: ---- X-Spam_report: (-4.2 / 5.0 requ) BAYES_00=-1.9, NICE_REPLY_A=-0.001, RCVD_IN_DNSWL_MED=-2.3, RCVD_IN_MSPIKE_H3=0.001, RCVD_IN_MSPIKE_WL=0.001, SPF_HELO_NONE=0.001, SPF_PASS=-0.001 autolearn=ham autolearn_force=no X-Spam_action: no action X-BeenThere: qemu-devel@nongnu.org X-Mailman-Version: 2.1.23 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Cc: Paul Durrant , Jason Wang , qemu-devel@nongnu.org, Peter Xu , haxm-team@intel.com, Colin Xu , Olaf Hering , Stefano Stabellini , Bruce Rogers , "Emilio G . Cota" , Anthony Perard , =?UTF-8?Q?Philippe_Mathieu-Daud=c3=a9?= , Laurent Vivier , Thomas Huth , Richard Henderson , Cameron Esfahani , Dario Faggioli , Roman Bolshakov , Sunil Muthuswamy , Marcelo Tosatti , Wenchao Wang , Paolo Bonzini Errors-To: qemu-devel-bounces+qemu-devel=archiver.kernel.org@nongnu.org Sender: "Qemu-devel" On 11/24/20 9:34 PM, Eduardo Habkost wrote: > On Tue, Nov 24, 2020 at 08:39:33PM +0100, Claudio Fontana wrote: >> On 11/24/20 8:27 PM, Eduardo Habkost wrote: >>> On Tue, Nov 24, 2020 at 07:52:15PM +0100, Claudio Fontana wrote: >>> [...] >>>>>> + } >>>>> >>>>> Additionally, if you call arch_cpu_accel_init() here, you won't >>>>> need MODULE_INIT_ACCEL_CPU anymore. The >>>>> >>>>> module_call_init(MODULE_INIT_ACCEL_CPU) >>>>> >>>>> call with implicit dependencies on runtime state inside vl.c and >>>>> *-user/main.c becomes a trivial: >>>>> >>>>> accel_init(accel) >>>>> >>>>> call in accel_init_machine() and *-user:main(). >>>> >>>> >>>> >>>> I do need a separate thing for the arch cpu accel specialization though, >>>> without MODULE_INIT_ACCEL_CPU that link between all operations done at accel-chosen time is missing.. >>>> >>> >>> I think this is a key point we need to sort out. >>> >>> What do you mean by "link between all operations done at >>> accel-chosen time" and why that's important? >> >> >> For understanding by a reader that tries to figure this out, >> (see the gid MODULE_INIT_ACCEL_CPU comment elsewhere in the thread). > > Right, but how does the module_call_init(MODULE_INIT_ACCEL_CPU) > indirection makes this easier to figure out than just looking at > a accel_init() function that makes regular function calls? I agree, if we accomplish a single accel_init() call that does everything (accelerator initialization and arch independent ops initialization and arch dependent specialization of the x86 cpu), that would be the best outcome in my view also. > > >> >> And it could be that the high level plan to make accelerators fully dynamically loadable plugins in the future >> also conditioned me to want to have a very clear demarcation line around the choice of the accelerator. > > We have dynamically loadable modules for other QOM types, > already, and they just use type_init(). I don't see why an extra > module_init() type makes this easier. > >> >> >>> >>> accel_init_machine() has 2-3 lines of code with side effects. It >>> calls AccelClass.init_machine(), which may may have hundreds of >> >> >> could we initialize also all arch-dependent stuff in here? > > You can, if you use a wrapper + stub, like arch_cpu_accel_init(). > As mentioned elsewhere, I'll try to avoid stubs. One is too many I think in the codebase (well one is probably ok, but you get what I mean, I don't like their proliferation). > >> >> >>> lines of code. accel_setup_post() has one additional method >>> call, which is triggered at a slightly different moment. >>> >>> You are using MODULE_INIT_ACCEL for 2 additional lines of code: >>> - the cpus_register_accel() call >>> - the x86_cpu_accel_init() call >>> >>> What makes those 2 lines of code so special, to make them deserve >>> a completely new mechanism to trigger them, instead of using >>> trivial function calls inside a accel_init() function? >>> >> >> ...can we do also the x86_cpu_accel_init inside accel_init()? >> >> >> In any case I'll try also the alternative, it would be nice if I could bring everything together under the same roof, >> and easily discoverable, both the arch-specific steps that we need to do at accel-chosen time and the non-arch-specific steps. > > One way to bring everything together under the same roof is to > call everything inside a accel_init() function. Will try! > > >> >> Hope this helps clarifying where I am coming from, >> but I am open to have my mind changed, also trying the alternatives you propose here could help me see first hand how things play out. > > Thanks! > Thanks, Ciao, Claudio