From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mail-wr1-f46.google.com (mail-wr1-f46.google.com [209.85.221.46]) by mx.groups.io with SMTP id smtpd.web10.8023.1615380343292059647 for ; Wed, 10 Mar 2021 04:45:43 -0800 Authentication-Results: mx.groups.io; dkim=pass header.i=@linaro.org header.s=google header.b=drmHE3PZ; spf=pass (domain: linaro.org, ip: 209.85.221.46, mailfrom: ilias.apalodimas@linaro.org) Received: by mail-wr1-f46.google.com with SMTP id v15so23185017wrx.4 for ; Wed, 10 Mar 2021 04:45:43 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=linaro.org; s=google; h=date:from:to:cc:subject:message-id:references:mime-version :content-disposition:content-transfer-encoding:in-reply-to; bh=whotZ658Ih0XFTnFjyPBU4KiFWQoIgQczhr4gBGMwU4=; b=drmHE3PZ7cBPNxjP8o/Fa8BHvGcYbl9LQbNhthRzdGuxTZACjyFpV+dQKmItshrLY9 7UOi8yqLzHI7S0vzFg2KalKIdarOp+WCdVVpxMyvy7J94hSU5Fnwd9UmQSTnVlJV79F4 mSUjXxf0ul4FDNzUcK3KEOQx4KhnId0mnGZQv1hasIrQM6tjlx4aI7Pqe6Vz3v3O3cPg jd5d7Wq4/rJLKoU+Szpb0uUBleuQ2O913KH7FmkRItG4bn4sXt5uZgm4PD9cmhsLhlXe zTtys1kp9IZ3s8xjN0osNlMu1YFGg2sqr9tuTCi4zjcM/0fFx95+/iYsjGb3+k6n7Acd zUWQ== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:date:from:to:cc:subject:message-id:references :mime-version:content-disposition:content-transfer-encoding :in-reply-to; bh=whotZ658Ih0XFTnFjyPBU4KiFWQoIgQczhr4gBGMwU4=; b=TH4R4k3zv7JoWQ7nbruKh3IRcFNzfewnQXKLs7rbmH75jOkNfQVp/U1Ky6Nw9WMDxZ wTCY+BcNrpQxOLRG0g/pSWyIrplmVtRdWITMv3lH3MjKfw8bwfTNlX5mtW9KierlBxXh p7yHPFkOhqSYRxuK67A2Rla3Mv2pP3ssFiVJckKApbQ8rOWhOr1fEn/JQxwcWX91J3Jh 6Gjajngz8sKUTukERFLYmVdO8GtfmWe7lvCKs8lMvthWGsbRfcG+F/u3oX1Oopb9VazT VU/PJFgbX/I/lDV8TZw4Vb1TO7ph+TEQ2DRswowNaE3XbdgalRYo418KkfTw/q3XwtPG qkyQ== X-Gm-Message-State: AOAM531ILXLXtdeU1f/Yczseo9MxjjTIexzEZGUWh0xEVGkFrgMOBlMD k0eLGu2E5KUewm9JzyJorLHT6Q== X-Google-Smtp-Source: ABdhPJwoYZXKXlPUdkBrmsJIu04X8dCqWpA0LjqQZp00qt9x+jUMKbZbjRhl7lMgD/d6T9OEYUBquA== X-Received: by 2002:a5d:4523:: with SMTP id j3mr3500569wra.288.1615380341738; Wed, 10 Mar 2021 04:45:41 -0800 (PST) Return-Path: Received: from apalos.home (ppp-94-64-113-158.home.otenet.gr. [94.64.113.158]) by smtp.gmail.com with ESMTPSA id j9sm9418114wmi.24.2021.03.10.04.45.40 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Wed, 10 Mar 2021 04:45:41 -0800 (PST) Date: Wed, 10 Mar 2021 14:45:38 +0200 From: "Ilias Apalodimas" To: Pierre Cc: devel@edk2.groups.io, Sami.Mujawar@arm.com, ardb+tianocore@kernel.org, sughosh.ganu@linaro.org, leif@nuviainc.com Subject: Re: [PATCH 1/3 v6] Drivers/OpTeeRpmb: Add an OP-TEE backed RPMB driver Message-ID: References: <20210309140123.2992772-1-ilias.apalodimas@linaro.org> <20210309140123.2992772-2-ilias.apalodimas@linaro.org> MIME-Version: 1.0 In-Reply-To: Content-Type: text/plain; charset=iso-8859-1 Content-Disposition: inline Content-Transfer-Encoding: 8bit Hi Pierre, On Wed, Mar 10, 2021 at 09:58:19AM +0000, Pierre wrote: > Hi Ilias, > Just minor coding style nit picking: > > * Drivers/OpTee/OpteeRpmbPkg/FixupPcd.c:FixPcdMemory() I think the > error codes are missing in the function header Ah you mean the return values of locate protocol? > * Thanks for adding the IN/OUT function parameter descriptors. Is it > possible to also add them in the function headers [1] ? Sure, I'll send a v7 anyways since I managed to mess up the maintainers patch somehow... I hope I haven't missed any of your other requests. > > About the FFA/SVC call: > > >> If this is an FFA call, is it possible to: > >>  - put a reference in the header to the spec (it should be similar to the > >> one at > >> edk2/ArmPkg/Library/StandaloneMmMmuLib/AArch64/ArmMmuStandaloneMmLib.c) > >>  - check the return status of the SVC call against the ones available at > >> edk2/ArmPkg/Include/IndustryStandard/ArmFfaSvc.h > >>  - if possible, remove the dependency to > > > > The call is technically an FFA one but at the moment OP-TEE returns the > StMM > > return code which is defined in the last header you mention. > > The relevant code is in ./core/arch/arm/kernel/stmm_sp.c function > > tee2stmm_ret_val(). > > So unless we redefine that in OP-TEE or (better imho), wait for a full FFA > > mechanism to be in place, I'd prefer leaving it as is. > > Keep in mind that adding the full FFA will also get rid of the hardcoded > IDs > > on the beginning of the file. > > The description of a FFA_MSG_SEND_DIRECT_REQ (s10.2 of [2]) doesn't seem to > return the same error codes as the ones optee returns (in > optee_os/core/arch/arm/kernel/stmm_sp.c:tee2stmm_ret_val()). I am not sure a > new FFA specification will change these error codes. > Another thing is that I think the mMemMgrId variable described in > edk2-platforms/Drivers/OpTee/OpteeRpmbPkg/OpTeeRpmbFvb.c is currently > defined as edk2/ArmPkg/Include/IndustryStandard/ArmFfaSvc.h:ARM_FFA_DESTINATION_ENDPOINT_ID > (the name seems to be misleading). > I think it would be better to: > > * align the optee error codes with what is in the FFA spec > * handle these error codes in edk2 with what is in > edk2/ArmPkg/Include/IndustryStandard/ArmFfaSvc.h and remove the > dependendy to edk2/ArmPkg/Include/IndustryStandard/ArmMmSvc.h if > possible > * rename > edk2/ArmPkg/Include/IndustryStandard/ArmFfaSvc.h:ARM_FFA_DESTINATION_ENDPOINT_ID > define to a proper name, according to what is in > optee_os/core/arch/arm/kernel/stmm_sp.c, and add one define for > 'mem_mgr_id' > * remove the mMemMgrId and mStorageId variables from > edk2-platforms/Drivers/OpTee/OpteeRpmbPkg/OpTeeRpmbFvb.c and use the > newly created defines from > edk2/ArmPkg/Include/IndustryStandard/ArmFfaSvc.h > > This would allow to be aligned with the current FFA spec and when a new one > comes, these endpoints IDs (mMemMgrId/mStorageId) can just be removed from > one location (as you said). > In the end it seems you and Sami will maintain this, so I will let you > decide what is best. I get the whole point, and for the record you are technically right. But there's something (once again) that's 'weird' here. This StandAloneMM that's compiling over here is only used by OP-TEE. In order to use that you need to call an SMC into OP-TEE (not FFA) from the non-secure world to initiate it. There's an OP-TEE PTA (pseudo-TA), that then converts the message to MM and sends it over to StandAloneMM. There are no FFA manifests yet, that's why the get/set memory attributes code is still running, to set up page permissions as well. The FFA mechanism you are seeing right now, is just the internal contract between OP-TEE and this driver. We did some of the calls depend on FFA since this was a good way to start introducing FFA code into EDK2 (which will eventually be needed), without being too intrusive. In the long run OP-TEE will be fully converted into FFA the changes you are talking about make sense. In fact there's a ./core/arch/arm/kernel/secure_partition.c in OP-TEE doing exactly that but it's not yet complete. I tried to describe the entire situation here [1]. If anyone feels really strong about this, we can go and change it. The changes aren't too big to begin with. That being said I'd prefer keeping it as is, since this will naturally evolve to a complete FFA spec, but the mechanisms are still missing from OP-TEE. Last but not least when OP-TEE gets that's FFA support you won't bundle StandAloneMM with the driver right? You'd have 2 discrete Secure partitions, one dealing with variables and one dealing with storage. [1] https://apalos.github.io/Protected%20UEFI%20variables%20with%20U-Boot.html#Protected%20UEFI%20variables%20with%20U-Boot Thanks /Ilias > > [1] https://edk2-docs.gitbook.io/edk-ii-c-coding-standards-specification/6_documenting_software/610_special_commands#6-10-4-param-in-out-parameter-description > [2] https://developer.arm.com/documentation/den0077/a > > Regards, > Pierre