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

["Ilias Apalodimas" <ilias.apalodimas@linaro.org>]

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.

[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