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=-4.0 required=3.0 tests=BAYES_00,DKIMWL_WL_HIGH, DKIM_SIGNED,DKIM_VALID,HEADER_FROM_DIFFERENT_DOMAINS,MAILING_LIST_MULTI, SPF_HELO_NONE,SPF_PASS 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 D83D3C433DF for ; Fri, 17 Jul 2020 15:01:20 +0000 (UTC) Received: from merlin.infradead.org (merlin.infradead.org [205.233.59.134]) (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 A66B22076A for ; Fri, 17 Jul 2020 15:01:20 +0000 (UTC) Authentication-Results: mail.kernel.org; dkim=pass (2048-bit key) header.d=lists.infradead.org header.i=@lists.infradead.org header.b="gCPCAaXx" DMARC-Filter: OpenDMARC Filter v1.3.2 mail.kernel.org A66B22076A Authentication-Results: mail.kernel.org; dmarc=fail (p=none dis=none) header.from=linux.intel.com Authentication-Results: mail.kernel.org; spf=none smtp.mailfrom=linux-arm-kernel-bounces+linux-arm-kernel=archiver.kernel.org@lists.infradead.org DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; d=lists.infradead.org; s=merlin.20170209; h=Sender:Content-Transfer-Encoding: Content-Type:Cc:List-Subscribe:List-Help:List-Post:List-Archive: List-Unsubscribe:List-Id:MIME-Version:References:In-Reply-To:Date:To:From: Subject:Message-ID:Reply-To:Content-ID:Content-Description:Resent-Date: Resent-From:Resent-Sender:Resent-To:Resent-Cc:Resent-Message-ID:List-Owner; bh=iHtRna3yJ857Vl9UQd5jYNCuRuxk6FzzcsJJFHQVh84=; b=gCPCAaXx32c3usgx24SVI27Pi iD367f2mmG8awnPslheZGbWa1e/Wkx5jew0yej7tjDkrn3sVn91OKomFsY5v7TuT9Q6Fp44rZaEEV ICXL2L3zeywptKxKHXcYYEuLC8cUC0kg9gEVGf5eo3lYmFyFqkLm4z5NlK+gVep8segRW5Edc88R6 Z17x8O8VpWiVwrEW+pG6OdThrpBB25KfIGRm8+p8HKpM/2kuJsyQHZZlegJR575NoDn6ZJBuwuWUE pSViwCkZvjvwh7M+cSoEt4IdRFGEBI1ZOYnfXrjs+xdHAWFEqreY/EZF4XmpsbJ8QXx+23Ug16xTn oupOsMmTw==; Received: from localhost ([::1] helo=merlin.infradead.org) by merlin.infradead.org with esmtp (Exim 4.92.3 #3 (Red Hat Linux)) id 1jwRpc-0006Yc-9H; Fri, 17 Jul 2020 14:59:24 +0000 Received: from mga06.intel.com ([134.134.136.31]) by merlin.infradead.org with esmtps (Exim 4.92.3 #3 (Red Hat Linux)) id 1jwRpZ-0006Y7-Bg for linux-arm-kernel@lists.infradead.org; Fri, 17 Jul 2020 14:59:22 +0000 IronPort-SDR: xsNe9M2IW3uZMVLQQlvIC/Wqxvv/wDnbWpE85aizTLFgaKmwp+750MkgtsPED7sZNYs56p3HBK lXdUEnweWo7A== X-IronPort-AV: E=McAfee;i="6000,8403,9685"; a="211143974" X-IronPort-AV: E=Sophos;i="5.75,362,1589266800"; d="scan'208";a="211143974" X-Amp-Result: SKIPPED(no attachment in message) X-Amp-File-Uploaded: False Received: from orsmga005.jf.intel.com ([10.7.209.41]) by orsmga104.jf.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 17 Jul 2020 07:59:17 -0700 IronPort-SDR: Wm0+cJlTU1BiR9UmPbBVNJ+2ELECDjX8RZL5SSrzqXQlX/w5p7Iz1zPPytKBIA+w1xsxZmEC9K r/YaOXEWC7qw== X-IronPort-AV: E=Sophos;i="5.75,362,1589266800"; d="scan'208";a="460870755" Received: from enaessen-mobl1.ger.corp.intel.com ([10.251.86.9]) by orsmga005-auth.jf.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 17 Jul 2020 07:59:14 -0700 Message-ID: Subject: Re: [PATCH] firmware: arm_scmi: Pass shmem address to SMCCC call From: Daniele Alessandrelli To: Sudeep Holla , Florian Fainelli Date: Fri, 17 Jul 2020 15:59:10 +0100 In-Reply-To: <20200717103120.GC24501@bogus> References: <20200715165518.57558-1-daniele.alessandrelli@linux.intel.com> <5f74221b-aec7-7715-19d1-5cbb406f1bdc@gmail.com> <20200717103120.GC24501@bogus> User-Agent: Evolution 3.36.3 (3.36.3-1.fc32) MIME-Version: 1.0 X-CRM114-Version: 20100106-BlameMichelson ( TRE 0.8.0 (BSD) ) MR-646709E3 X-CRM114-CacheID: sfid-20200717_105921_687810_0C23CB8C X-CRM114-Status: GOOD ( 63.24 ) X-BeenThere: linux-arm-kernel@lists.infradead.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Cc: Peng Fan , linux-kernel@vger.kernel.org, "Paul J. Murphy" , "Paul J. Murphy" , Daniele Alessandrelli , linux-arm-kernel@lists.infradead.org Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Sender: "linux-arm-kernel" Errors-To: linux-arm-kernel-bounces+linux-arm-kernel=archiver.kernel.org@lists.infradead.org On Fri, 2020-07-17 at 11:31 +0100, Sudeep Holla wrote: > On Thu, Jul 16, 2020 at 12:57:23PM -0700, Florian Fainelli wrote: > > > > On 7/16/2020 7:13 AM, Daniele Alessandrelli wrote: > > > Hi Florian, > > > > > > Thanks for you feedback. > > > > > > On Wed, 2020-07-15 at 15:43 -0700, Florian Fainelli wrote: > > > > On 7/15/2020 9:55 AM, Daniele Alessandrelli wrote: > > > > > From: Daniele Alessandrelli > > > > > > > > > > Currently, when SMC/HVC is used as transport, the base > > > > > address of > > > > > the > > > > > shared memory used for communication is not passed to the > > > > > SMCCC > > > > > call. > > > > > This means that such an address must be hard-coded into the > > > > > bootloader. > > > > > > > > > > In order to increase flexibility and allow the memory layout > > > > > to be > > > > > changed without modifying the bootloader, this patch adds the > > > > > shared > > > > > memory base address to the a1 argument of the SMCCC call. > > > > > > > > > > On the Secure Monitor side, the service call implementation > > > > > can > > > > > therefore read the a1 argument in order to know the location > > > > > of the > > > > > shared memory to use. This change is backward compatible to > > > > > existing > > > > > service call implementations as long as they don't check for > > > > > a1 to > > > > > be > > > > > zero. > > > > > > > > resource_size_t being defined after phys_addr_t, its size is > > > > different > > > > between 32-bit, 32-bit with PAE and 64-bit so it would probably > > > > make > > > > more sense to define an physical address alignment, or maybe an > > > > address > > > > that is in multiple of 4KBytes so you can address up to 36-bits > > > > of > > > > physical address even on a 32-bit only system? > > > > > > I see your point. After a quick look, I think that, practically, > > > the > > > issue is with ARM32 LPAE addresses, for which phys_addr_t is a > > > u64. So, > > > basically, for AArch32 systems with LPAE the 64-bit shmem_paddr > > > gets > > > truncated to 32-bit when it's passed to the SMC32/HVC32 call. > > > > > > To solve that, I would prefer splitting the address between two > > > SMC > > > parameters (a1 = addr_lo, a2 = addr_hi), instead of imposing an > > > arbitrary alignment. Would that be reasonable? > > > > The low/high part would only be relevant on a 32-bit LPAE platform > > which > > is probably a corner case, I would just pass the shmem_paddr / 4096 > > since that is the smallest granule size and alignment possible and > > it > > still allows you to map up to 36-bits of physical address, which is > > the > > maximum that the long descriptor in LPAE can support. For 64-bit we > > have > > no such problems since we have the full register width. > > > > OK, I will check if 32-bit identifier can be relaxed in the spec in > which > case we can avoid having DT binding for the identifier. If that is > possible > we could use addr/4k page size. > > > > > What discovery mechanism does the OS have that the specified > > > > address > > > > within the SMCCC call has been accepted by the firmware given > > > > the > > > > return value of that SMCCC call does not appear to be used or > > > > checked? Do > > > > we just expect a timeout initializing the SCMI subsystem? > > > > > > The return code is actually checked at the end of the function: > > > https://elixir.bootlin.com/linux/v5.8-rc4/source/drivers/firmware/arm_scmi/smc.c#L118 > > > > > > But in the meantime scmi_rx_callback() has already been called. > > > Not > > > sure if that's intentional or a possible bug. > > > > > > > Given that the kernel must somehow reserve this memory as a > > > > shared > > > > memory area for obvious reasons, and the trusted firmware must > > > > also > > > > ensure it treats this memory region with specific permissions > > > > in its > > > > translation regime, does it really make sense to give that much > > > > flexibility? > > I share same opinion as Florian here. > > [...] > > > > > If your boot loader has FDT patching capability, maybe it can > > > > also do > > > > a SMC call to provide the address to your trusted firmware, > > > > prior to > > > > loading the Linux kernel, and then they both agree, prior to > > > > boot > > > > about the shared memory address? > > > > > > Yes, that's a possible solution, but it looks more complicated to > > > me, > > > since it adds an additional component (the boot loader) to the > > > equation, while the goal of this patch was to reduce the coupling > > > between components (namely the DT/kernel and the trusted > > > firmware). > > > > > > I guess my question is: if we fix the handling of LPAE addresses > > > and > > > the SMC return code, what is the drawback of having the shmem > > > address > > > passed to the SMC? > > > > My only concern is that if somehow Linux gets assigned a shared > > memory > > range that is completely outside of what the trusted firmware has > > already mapped, or is capable of addressing, or any combination > > thereof, > > it could be challenging to debug what is going on, especially if > > INVALID > > PARAMETER must not be returned (assuming this is to avoid Linux > > discovering where other shared memory areas pertaining to the > > firmware > > reside?). > > > > Valid point. Again, I was planning to use this as identifier and > didn't > think of the usecase Daniele mentions here. > > > The other concern I have is that we are not documenting the various > > SMCCC calling conventions, soon enough it will be come out of > > control, > > and we are already allowing people to define their own function IDs > > and > > parameters to call into the trusted firmware. This sounds like > > something > > that is so basic that it should be standardized from the top, by > > ARM. > > > > Completely agreed. I have failed to achieve that myself. Unless there > is > demand from partners/vendors, it will be dismissed by architects. All > I managed to get in was the identifier as the argument for SMC/HVC to > support multiple channels. However 32-bit id will be issue to be used > with 64-bit address. I will see if we can drop that 32-bit > requirement > from the spec. It seems that there is some demand already, but perhaps it needs to be requested in a more official way? What is the recommended way to do so? > > > > Anyway, I should have mentioned this in the commit message (sorry > > > for > > > not doing so), but I submitted this patch because initial > > > feedback from > > > Sudeep was positive [1]; but if there is no consensus around it > > > I'm > > > fine with dropping it. > > > > > > [1] https://lore.kernel.org/lkml/20200710075931.GB1189@bogus/ > > > > My review is by no means authoritative however in deploying SCMI on > > our > > Broadcom STB platforms some experience was gained in the process > > which > > is how it piqued my interest. Thanks for providing more background > > to > > this patch, this does help. > > > > Your knowledge is important and please always share the same and > happy > to take all your review into consideration always. End goal is to > help > all or most of the users of the driver if possible. > > > We have opted for a solution where the boot loader knows about all > > possible reserved regions prior to booting/loading the trusted > > firmware > > as well as the kernel, therefore it can pass that information to > > both > > and we never really had a situation where the two need to evolve in > > an > > uncoordinated way. > > I agree and uncoordinated evolution may not be as simple as it is > presented by Daniele earlier and I am not confident to support that. > The main reason for giving positive response for this initially is > for the addition of identifier which I always thought is required for > supporting multiple channels. E.g. DVFS/Perf has dedicated channel. I can see now that in the previous discussion there was some misunderstanding on the intended use case. That's fine, I will drop the patch for now, since hard-coding the shmem address in our trusted firmware is fine with us (and so we don't really need this patch). > > -- > Regards, > Sudeep _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel