From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mail-yb1-f173.google.com (mail-yb1-f173.google.com [209.85.219.173]) by mx.groups.io with SMTP id smtpd.web11.8943.1615480687886126037 for ; Thu, 11 Mar 2021 08:38:08 -0800 Authentication-Results: mx.groups.io; dkim=pass header.i=@linaro.org header.s=google header.b=Ymz/V6b5; spf=pass (domain: linaro.org, ip: 209.85.219.173, mailfrom: ilias.apalodimas@linaro.org) Received: by mail-yb1-f173.google.com with SMTP id p193so22342143yba.4 for ; Thu, 11 Mar 2021 08:38:07 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=linaro.org; s=google; h=mime-version:references:in-reply-to:from:date:message-id:subject:to :cc; bh=fifs3A+YpnAP4htyo2cVL3wxnelBXq08mOSXQ+rnIgk=; b=Ymz/V6b5roXyzCKy0nWA+GBGXW2rbf6pXsHZtm1pY7Ski7fOq7mOZO49kH447U5hWe XHq9M+627EYOPRqsdR0N5Rcx80CzB7oaNpkQp6izWMwpXWUqvxztpibx9T+CgMCMLXFY xbuTqXWvklELDjat8qB8dcu41mO8oo2kyG6p1lkrR655f09PspQNyPLrqJ06Ohi0tC9Z CA9oYVkER5ekoi8nVi70Bk2XBLJKGn3cDewee4Y9nSPs6FbNfFnqzDf0/z6vPW4IEiIA vyHVp6FcL3/Nm+PS7bmdzXcjAJQyYmaGuZitfdKwgfAYUfh33oc/7PArZ2ymImZlXt9V iNIA== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:mime-version:references:in-reply-to:from:date :message-id:subject:to:cc; bh=fifs3A+YpnAP4htyo2cVL3wxnelBXq08mOSXQ+rnIgk=; b=WcMoEyvUX9eblmBQ44AwOv2arCTE9uFrPxfbBICCfJDs/Cvh1ntxI5+eb+C9OIvmZE I+V3AtBa7mCgt8Ijv9DluMi6HDt7T2PP9tj7lZNxs/RapX9LMa6nWbMQyxjvDeUEDiVp 9dlgIZMFI7aJUXsTW5TqkGwbtu13DD2s61ndSEALzUkQtpwWtVBXpXA9kbqAcQZ25Ch+ wOLwCAa7lL2Nt89xwdAbDB4A5K4qWr54NsSDrE3bc9UpJ6+pkZPUexw+ByjuXif3Ev9B EpNAsUDEhIuX8P/s0qsfPBB3WYsg8RQXOH94x5RVl1ohkjaBw44N1gb3Knjn2fErGFBM E4+A== X-Gm-Message-State: AOAM530qHUau0kJzUnrMHWybSxdLT5JUYArRgz2hgpsgQUa+CQzwn6x1 L5uK363/7NzX1sH9iBQGSbJf7MQLTYU3OkrKy4ijBn/RLcTn9Q== X-Google-Smtp-Source: ABdhPJzESz/L6Iv5vMqSQMMMGRMpVBs/RXTcU3vEq8hKomfSQl3+QsQFWtFq6FRBAIdfkGH/xsYlqUu/yMJicP87AYI= X-Received: by 2002:a25:4a84:: with SMTP id x126mr12058549yba.408.1615480686983; Thu, 11 Mar 2021 08:38:06 -0800 (PST) MIME-Version: 1.0 References: <20210309140123.2992772-1-ilias.apalodimas@linaro.org> <20210309140123.2992772-2-ilias.apalodimas@linaro.org> <166AFBDD38991A80.19014@groups.io> In-Reply-To: <166AFBDD38991A80.19014@groups.io> From: "Ilias Apalodimas" Date: Thu, 11 Mar 2021 18:37:30 +0200 Message-ID: Subject: Re: [edk2-devel] [PATCH 1/3 v6] Drivers/OpTeeRpmb: Add an OP-TEE backed RPMB driver To: devel@edk2.groups.io, Ilias Apalodimas , Achin Gupta Cc: Pierre , Sami Mujawar , Ard Biesheuvel , Sughosh Ganu , Leif Lindholm Content-Type: multipart/alternative; boundary="0000000000007d53c905bd4568b0" --0000000000007d53c905bd4568b0 Content-Type: text/plain; charset="UTF-8" (+cc Achin) On Wed, 10 Mar 2021 at 14:45, Ilias Apalodimas via 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 > > > > > > 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 > > > > > > --0000000000007d53c905bd4568b0 Content-Type: text/html; charset="UTF-8" Content-Transfer-Encoding: quoted-printable
(+cc Achin)

On Wed, 10 Mar 2021 at 14:45, Ilias Apalodim= as via groups.io <ilias.apalodimas=3Dlinaro.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:
>
>=C2=A0 * Drivers/OpTee/OpteeRpmbPkg/FixupPcd.c:FixPcdMemory() I think = the
>=C2=A0 =C2=A0 error codes are missing in the function header

Ah you mean the return values of locate protocol?

>=C2=A0 * Thanks for adding the IN/OUT function parameter descriptors. = Is it
>=C2=A0 =C2=A0 possible to also add them in the function headers [1] ?<= br>
Sure, I'll send a v7 anyways since I managed to mess up the maintainer= s 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:
> >>=C2=A0 - put a reference in the header to the spec (it should= be similar to the
> >> one at
> >> edk2/ArmPkg/Library/StandaloneMmMmuLib/AArch64/ArmMmuStandal= oneMmLib.c)
> >>=C2=A0 - check the return status of the SVC call against the = ones available at
> >> edk2/ArmPkg/Include/IndustryStandard/ArmFfaSvc.h
> >>=C2=A0 - if possible, remove the dependency to <IndustrySt= andard/ArmMmSvc.h>
> >
> > The call is technically an FFA one but at the moment OP-TEE retu= rns 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 functio= n
> > 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 h= ardcoded
> 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_D= ESTINATION_ENDPOINT_ID
> (the name seems to be misleading).
> I think it would be better to:
>
>=C2=A0 * align the optee error codes with what is in the FFA spec
>=C2=A0 * handle these error codes in edk2 with what is in
>=C2=A0 =C2=A0 edk2/ArmPkg/Include/IndustryStandard/ArmFfaSvc.h and rem= ove the
>=C2=A0 =C2=A0 dependendy to edk2/ArmPkg/Include/IndustryStandard/ArmMm= Svc.h if
>=C2=A0 =C2=A0 possible
>=C2=A0 * rename
>=C2=A0 =C2=A0 edk2/ArmPkg/Include/IndustryStandard/ArmFfaSvc.h:ARM_FFA= _DESTINATION_ENDPOINT_ID
>=C2=A0 =C2=A0 define to a proper name, according to what is in
>=C2=A0 =C2=A0 optee_os/core/arch/arm/kernel/stmm_sp.c, and add one def= ine for
>=C2=A0 =C2=A0 'mem_mgr_id'
>=C2=A0 * remove the mMemMgrId and mStorageId variables from
>=C2=A0 =C2=A0 edk2-platforms/Drivers/OpTee/OpteeRpmbPkg/OpTeeRpmbFvb.c= and use the
>=C2=A0 =C2=A0 newly created defines from
>=C2=A0 =C2=A0 edk2/ArmPkg/Include/IndustryStandard/ArmFfaSvc.h
>
> This would allow to be aligned with the current FFA spec and when a n= ew 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 yo= u
> 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. <= br> 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 M= M and
sends it over to StandAloneMM.=C2=A0 There are no FFA manifests yet, that&= #39;s why the
get/set memory attributes code is still running,=C2=A0 to set up page perm= issions
as well.

The FFA mechanism you are seeing right now, is just the internal contract<= br> between OP-TEE and this driver. We did some of the calls depend on FFA sin= ce
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 ar= e
talking about make sense. In fact there's a ./core/arch/arm/kernel/sec= ure_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 cha= nges
aren't too big to begin with. That being said I'd prefer keeping i= t as is,
since this will naturally evolve to a complete FFA spec, but the mechanism= s
are still missing from OP-TEE. Last but not least when OP-TEE gets that= 9;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 wit= h
storage.

[1] https://apalos.github.io/Protected%20UEFI%20variabl= es%20with%20U-Boot.html#Protected%20UEFI%20variables%20with%20U-Boot

Thanks
/Ilias
>
> [1] https://edk= 2-docs.gitbook.io/edk-ii-c-coding-standards-specification/6_documenting_sof= tware/610_special_commands#6-10-4-param-in-out-parameter-description > [2] https://developer.arm.com/documentation/= den0077/a
>
> Regards,
> Pierre





--0000000000007d53c905bd4568b0--