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 Received: from phobos.denx.de (phobos.denx.de [85.214.62.61]) (using TLSv1.2 with cipher ECDHE-RSA-AES128-GCM-SHA256 (128/128 bits)) (No client certificate requested) by smtp.lore.kernel.org (Postfix) with ESMTPS id 3209DC001DF for ; Wed, 2 Aug 2023 06:52:31 +0000 (UTC) Received: from h2850616.stratoserver.net (localhost [IPv6:::1]) by phobos.denx.de (Postfix) with ESMTP id 49BED86BFF; Wed, 2 Aug 2023 08:52:29 +0200 (CEST) Authentication-Results: phobos.denx.de; dmarc=pass (p=none dis=none) header.from=linaro.org Authentication-Results: phobos.denx.de; spf=pass smtp.mailfrom=u-boot-bounces@lists.denx.de Authentication-Results: phobos.denx.de; dkim=pass (2048-bit key; unprotected) header.d=linaro.org header.i=@linaro.org header.b="G8leBXm0"; dkim-atps=neutral Received: by phobos.denx.de (Postfix, from userid 109) id E471986C0F; Wed, 2 Aug 2023 08:52:27 +0200 (CEST) Received: from mail-lf1-x12d.google.com (mail-lf1-x12d.google.com [IPv6:2a00:1450:4864:20::12d]) (using TLSv1.3 with cipher TLS_AES_128_GCM_SHA256 (128/128 bits)) (No client certificate requested) by phobos.denx.de (Postfix) with ESMTPS id EE11486AB9 for ; Wed, 2 Aug 2023 08:52:24 +0200 (CEST) Authentication-Results: phobos.denx.de; dmarc=pass (p=none dis=none) header.from=linaro.org Authentication-Results: phobos.denx.de; spf=pass smtp.mailfrom=ilias.apalodimas@linaro.org Received: by mail-lf1-x12d.google.com with SMTP id 2adb3069b0e04-4fe389d6f19so4595220e87.3 for ; Tue, 01 Aug 2023 23:52:24 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=linaro.org; s=google; t=1690959144; x=1691563944; h=cc:to:subject:message-id:date:from:in-reply-to:references :mime-version:from:to:cc:subject:date:message-id:reply-to; bh=7KdNSFuZC4OK1WBTj22lUmvu1LpMWWku+TsTtm2a9Lg=; b=G8leBXm02sPamTm+RsUEdfdoPU850HYFqmx6rx7MkfiYbbHBhpFUBWntDdTNjgS7vz PP4ajbvbs6xaqcSZ5jSoilVrCVSpy+adz74+MmVMCqboYrcoMFqOjzjQX1a7dRlD+Pkk oCrWpsZiQa2l3jAp7avMCM/lzf3iHRPKTR8XiJuSLteua/376QaUBsUVV4GTSeFkjTG4 yyF6KKcS+SMxHs6ujzDrlBsbPMQ7II37ALst5HPWRv6VrX3CnR1WUG+KKGv4Jup0S4YE ns+mD190TJATeonYRcKnGLJ2a3stQddVq08zbvrkvygQCeyH5os5+W0ykzc/hM9xqdg3 161g== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20221208; t=1690959144; x=1691563944; h=cc:to:subject:message-id:date:from:in-reply-to:references :mime-version:x-gm-message-state:from:to:cc:subject:date:message-id :reply-to; bh=7KdNSFuZC4OK1WBTj22lUmvu1LpMWWku+TsTtm2a9Lg=; b=RSoIfDRa6ZXGDv9MbOQEWwV9/RTgUFzGoyL1LKxpA6Ll1cD2UUwT80p1/+87py9z2E nTOmDCUCi8g+LPAm7AnJEuB/TygCQLpWyOUHnrFXJChL8Hb06nYwPipJ/Bi3qv9FRLpz vEePZF7DdiCHxnvZUYfN6j3xjI3RJGBdGsEdhrdDQlFlfRnJDxo+s4CyhxfsRKbP164+ 7cLI+s9uxz1tX25/cNsc+lDu6p6TH8tPS8yrAlsdOuOuUpSXwY96N20xNv4AtSuonou0 mrl0UZGlo5GzUINz71hx3oY+1SW9wYMy5FdhsvupMemJqh+YBJ4OMB6GL7LXhvtADibD g/uA== X-Gm-Message-State: ABy/qLb9WpFfT1LnAjavtqrB42TwSU6drtHUQH4nL7j2qfiQ90Zys0Dd Vo8j0rgI4eN48euL0qKgfnyc8p6JUvv2WCoMCqFFkg== X-Google-Smtp-Source: APBJJlH4EHbECDDOJHfjv4WKTATZEvNcy9V9efVEWdBAlv47U7Gcm92tlt+8fwTnucyOhtCpqOOlLTQ2qzi8WjwfH88= X-Received: by 2002:a19:5f51:0:b0:4fb:fdf1:8b15 with SMTP id a17-20020a195f51000000b004fbfdf18b15mr3834215lfj.59.1690959144140; Tue, 01 Aug 2023 23:52:24 -0700 (PDT) MIME-Version: 1.0 References: <20230727160712.81477-1-abdellatif.elkhlifi@arm.com> <20230727160712.81477-10-abdellatif.elkhlifi@arm.com> <20230727164345.GH3630934@bill-the-cat> <20230728135415.GU3630934@bill-the-cat> <20230731114628.GA112180@e130802.arm.com> <20230801150057.GO3630934@bill-the-cat> <20230801161008.GA45125@e130802.arm.com> <20230801161917.GU3630934@bill-the-cat> In-Reply-To: <20230801161917.GU3630934@bill-the-cat> From: Ilias Apalodimas Date: Wed, 2 Aug 2023 09:51:47 +0300 Message-ID: Subject: Re: [PATCH v17 09/10] arm_ffa: efi: introduce FF-A MM communication To: Tom Rini Cc: Abdellatif El Khlifi , sjg@chromium.org, achin.gupta@arm.com, nd@arm.com, u-boot@lists.denx.de, jens.wiklander@linaro.org Content-Type: text/plain; charset="UTF-8" X-BeenThere: u-boot@lists.denx.de X-Mailman-Version: 2.1.39 Precedence: list List-Id: U-Boot discussion List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: u-boot-bounces@lists.denx.de Sender: "U-Boot" X-Virus-Scanned: clamav-milter 0.103.8 at phobos.denx.de X-Virus-Status: Clean On Tue, 1 Aug 2023 at 19:19, Tom Rini wrote: > > On Tue, Aug 01, 2023 at 05:10:08PM +0100, Abdellatif El Khlifi wrote: > > Hi guys, > > > > On Tue, Aug 01, 2023 at 11:00:57AM -0400, Tom Rini wrote: > > > > > > > > > > ... > > > > > > > > > > Changelog: > > > > > > > > > > =============== > > > > > > > > > > > > > > > > > > > > v17: > > > > > > > > > > > > > > > > > > > > * show a debug message rather than an error when FF-A is not detected > > > > > > > > > [snip] > > > > > > > > > > diff --git a/lib/efi_loader/Kconfig b/lib/efi_loader/Kconfig > > > > > > > > > > index c5835e6ef6..8fbadb9201 100644 > > > > > > > > > > --- a/lib/efi_loader/Kconfig > > > > > > > > > > +++ b/lib/efi_loader/Kconfig > > > > > > > > > > @@ -55,13 +55,53 @@ config EFI_VARIABLE_FILE_STORE > > > > > > > > > > stored as file /ubootefi.var on the EFI system partition. > > > > > > > > > > > > > > > > > > > > config EFI_MM_COMM_TEE > > > > > > > > > > - bool "UEFI variables storage service via OP-TEE" > > > > > > > > > > - depends on OPTEE > > > > > > > > > > + bool "UEFI variables storage service via the trusted world" > > > > > > > > > > + depends on OPTEE && ARM_FFA_TRANSPORT > > > > > > > > > > > > > > > > > > You didn't get my changes in here however. If you can do EFI_MM_COMM_TEE > > > > > > > > > without ARM_FFA_TRANSPORT (as lx2160ardb_tfa_stmm_defconfig does) then > > > > > > > > > you don't make this option depend on . If FF-A is only > > > > > > > > > for use here, you make FF-A depend on this, and the FF-A specific > > > > > > > > > variable depend on ARM_FFA_TRANSPORT. > > > > > > > > > > > > > > > > Abdellatif hinted at what's going on here. When I added this Kconfig > > > > > > > > option to lx2160 FF-A wasn't implemented yet. > > > > > > > > > > > > > > The defconfig has existed since May 2020, which is when you added > > > > > > > EFI_MM_COMM_TEE itself too. So I think it's that no one did the check I > > > > > > > did until now and saw this series was disabling what was on the other > > > > > > > platform. > > > > > > > > > > > > > > > Since FF-A isn't a new > > > > > > > > communication mechanism but builds upon the existing SMCs to build an > > > > > > > > easier API, I asked Abdellatif to hide this complexity. > > > > > > > > We had two options, either make Kconfig options for either FF-A or the > > > > > > > > traditional SMCs and remove the dependencies, or piggyback on FF-As > > > > > > > > discovery mechanism and make the choice at runtime. The latter has a > > > > > > > > small impact on code size, but imho makes developers' life a lot > > > > > > > > easier. > > > > > > > > > > > > > > I'm not sure how much you can do a run-time option here since you're > > > > > > > setting a bunch of default values for FF-A to 0 in Kconfig. If we're > > > > > > > supposed to be able to get them at run time, we shouldn't need a Kconfig > > > > > > > option at all. I'm also not sure how valid a use case it is where we > > > > > > > won't know at build time what the rest of the firmware stack supports > > > > > > > here. > > > > > > > > > > > > > > > > > > > That's a fair point. FF-A in theory has APIs to discover memory. > > > > > > Abdellatif, why do we need the Kconfigs for shared memory on FF-A? > > > > > > > > > > The statically carved out MM shared buffer address, size and offset cannot be discovered by FF-A ABIs. > > > > > The MM communication driver in U-Boot could allocate the buffer and share it with the MM SP but > > > > > we do not implement that support currently in either U-Boot or UEFI. > > > > > > > > Ok, that's a bit unfortunate, but Tom is right. Having the FF-A > > > > addresses show up is as confusing as having Kconfig options for > > > > discrete options. The whole point of my suggestion was to make users' > > > > lives easier. Apologies for the confusion but can you bring back the > > > > ifdefs? Looking at the patch this should be minimal just use > > > > ifdef ARM_FFA_TRANSPORT and ifndef ARM_FFA_TRANSPORT. > > > > > > > > Tom you prefer that as well? > > > > > > Pending an answer to Jens' feedback, yes, going back to #ifdef's is > > > fine, especially since default values of 0 are nonsense in this case > > > (and as Heinrich's patch re SYS_MALLOC_LEN shows, dangerous since 0 != > > > 0x0 once we do string comparisons). > > > > > > > I'd like to give some context why it's important for Corstone-1000 platform > > that the DT passed to the kernel matches the official kernel DT. > > Note that we've set aside the "should this be in DT or not" question. > > > There is a SystemReady IR 2.0 test checking the DT. It compares the DT > > passed by U-Boot with a reference DT (the kernel DT) . The test fails if there > > is a mismatch. So, if we add a DT node in U-Boot and the node is not upstreamed > > to the kernel DT, the DT test will fail. > > This is overall good and progress. > > > To be approved by the kernel DT maintainers, the node should have a use case > > in the kernel which is not the case. > > This is, I believe / hope wrong. It needs to be in the dt-schema > repository, not strictly "the kernel". For example, bootph-all (etc) > are in dt-schema and so can be in the upstream kernel but are not used > in the kernel itself. > > > There is a solution for this which is deleting the node we don't want to pass to > > the kernel using delete-node in the U-Boot DT. > > Something like this will likely be needed, in the end, at least for some > cases. But the goal is that everything gets in to dt-schema. We are already working on U-Boot on that. The idea is rather simple. We will have an array with nodes and node entries. Before we boot up we'll scan that array, if a node entry exists we will delete that, otherwise we will just get rid of the entire node. That should be pretty easy to maintain and extend. U-Boot will then be able to use it;s internal bidings without polluting the DT we handover to the kernel. I think you should just wait for that patch instead of hacking the boot cmd. > > [snip] > > With this we can get rid of the configs and the #defines: FFA_SHARED_MM_BUF_ADDR, > > FFA_SHARED_MM_BUF_OFFSET and FFA_SHARED_MM_BUF_SIZE. > > > > Also, we will avoid setting 0 as default values for the address, size and offset. > > We just need to not have default values offered. The symbols just need > to depend on FFA so that they aren't asked when not used. > > > 2/ the FF-A specific code in efi_variable_tee.c will try to find the mm-comms-buf > > reserved memory node. When found, it reads the buffer address, size and offset. > > > > 3/ adding #ifdef CONFIG_ARM_FFA_TRANSPORT in lib/efi_loader/efi_variable_tee.c > > for the FF-A specific code. > > > > 4/ make EFI_MM_COMM_TEE depends on OPTEE only > > > > What do you think guys ? > > Yes, we need to do 3 and 4. +1 here Thanks /Ilias > > -- > Tom