Re: [edk2-devel] [PATCH 1/3 v6] Drivers/OpTeeRpmb: Add an OP-TEE backed RPMB driver

[Achin Gupta <achin.gupta@arm.com>]

Hi,

CIL...

On Thu, Mar 11, 2021 at 06:37:30PM +0200, Ilias Apalodimas wrote:
> (+cc Achin)
>
> On Wed, 10 Mar 2021 at 14:45, Ilias Apalodimas via groups.io <ilias.apalodimas=
> linaro.org@groups.io> wrote:
>
>     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 <IndustryStandard/ArmMmSvc.h>
>     > >
>     > > 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.

Just to second Ilias's explanation above...

The plan is to incorporate the ABIs to get and set memory attributes into the
FF-A v1.1 specification. This way, the memory manager service will not be a
protocol that uses FF-A DIRECT_REQUEST and DIRECT_RESP as the
transport. Instead, it will be natively implemented by OP-TEE. The error codes
etc will align at this point.

I hope this helps clarify. Do shout if you need more information.

cheers,
Achin

>
>     [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
>
>
>     
>
>
>