(+cc Achin) On Wed, 10 Mar 2021 at 14:45, Ilias Apalodimas via groups.io <ilias.apalodimas=linaro....@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. > > [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 > > > > > > -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#72670): https://edk2.groups.io/g/devel/message/72670 Mute This Topic: https://groups.io/mt/81201374/21656 Group Owner: devel+ow...@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com] -=-=-=-=-=-=-=-=-=-=-=-
