[edk2-devel] [PATCH 1/3 v6] Drivers/OpTeeRpmb: Add an OP-TEE backed RPMB driver
Achin Gupta
achin.gupta at arm.com
Thu Mar 11 18:25:09 UTC 2021
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 at 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
>
>
>
>
>
>
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#72680): https://edk2.groups.io/g/devel/message/72680
Mute This Topic: https://groups.io/mt/81201374/1813853
Group Owner: devel+owner at edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [edk2-devel-archive at redhat.com]
-=-=-=-=-=-=-=-=-=-=-=-
More information about the edk2-devel-archive
mailing list