From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from foss.arm.com (foss.arm.com [217.140.110.172]) by mx.groups.io with SMTP id smtpd.web09.6632.1615370307046490143 for ; Wed, 10 Mar 2021 01:58:27 -0800 Authentication-Results: mx.groups.io; dkim=missing; spf=pass (domain: arm.com, ip: 217.140.110.172, mailfrom: pierre.gondois@arm.com) Received: from usa-sjc-imap-foss1.foss.arm.com (unknown [10.121.207.14]) by usa-sjc-mx-foss1.foss.arm.com (Postfix) with ESMTP id 682611FB; Wed, 10 Mar 2021 01:58:25 -0800 (PST) Received: from [192.168.1.169] (unknown [172.31.20.19]) by usa-sjc-imap-foss1.foss.arm.com (Postfix) with ESMTPA id 78DB13F85F; Wed, 10 Mar 2021 01:58:24 -0800 (PST) From: "PierreGondois" Subject: Re: [PATCH 1/3 v6] Drivers/OpTeeRpmb: Add an OP-TEE backed RPMB driver References: <20210309140123.2992772-1-ilias.apalodimas@linaro.org> <20210309140123.2992772-2-ilias.apalodimas@linaro.org> To: devel@edk2.groups.io, Ilias Apalodimas , Sami.Mujawar@arm.com Cc: ardb+tianocore@kernel.org, sughosh.ganu@linaro.org, leif@nuviainc.com Message-ID: Date: Wed, 10 Mar 2021 09:58:19 +0000 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:78.0) Gecko/20100101 Thunderbird/78.8.0 MIME-Version: 1.0 In-Reply-To: <20210309140123.2992772-2-ilias.apalodimas@linaro.org> Content-Type: text/plain; charset=windows-1252; format=flowed Content-Language: en-US Content-Transfer-Encoding: quoted-printable 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: >>=A0 - put a reference in the header to the spec (it should be similar=20 to the >> one at >> edk2/ArmPkg/Library/StandaloneMmMmuLib/AArch64/ArmMmuStandaloneMmLib.= c) >>=A0 - check the return status of the SVC call against the ones availab= le at >> edk2/ArmPkg/Include/IndustryStandard/ArmFfaSvc.h >>=A0 - if possible, remove the dependency to > > The call is technically an FFA one but at the moment OP-TEE returns=20 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=20 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=20 hardcoded IDs > on the beginning of the file. The description of a FFA_MSG_SEND_DIRECT_REQ (s10.2 of [2]) doesn't seem=20 to return the same error codes as the ones optee returns (in=20 optee_os/core/arch/arm/kernel/stmm_sp.c:tee2stmm_ret_val()). I am not=20 sure a new FFA specification will change these error codes. Another thing is that I think the mMemMgrId variable described in=20 edk2-platforms/Drivers/OpTee/OpteeRpmbPkg/OpTeeRpmbFvb.c is currently=20 defined as=20 edk2/ArmPkg/Include/IndustryStandard/ArmFfaSvc.h:ARM_FFA_DESTINATION_ENDP= OINT_ID=20 (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=20 one comes, these endpoints IDs (mMemMgrId/mStorageId) can just be=20 removed from one location (as you said). In the end it seems you and Sami will maintain this, so I will let you=20 decide what is best. [1]=20 https://edk2-docs.gitbook.io/edk-ii-c-coding-standards-specification/6_do= cumenting_software/610_special_commands#6-10-4-param-in-out-parameter-des= cription [2] https://developer.arm.com/documentation/den0077/a Regards, Pierre