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

["PierreGondois" <pierre.gondois@arm.com>]

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
  * Thanks for adding the IN/OUT function parameter descriptors. Is it
    possible to also add them in the function headers [1] ?

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.

[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