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

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

[-- Attachment #1: Type: text/plain, Size: 5440 bytes --]

(+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.
>
> [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
>
>
> 
>
>
>

[-- Attachment #2: Type: text/html, Size: 7021 bytes --]