From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received-SPF: Pass (sender SPF authorized) identity=mailfrom; client-ip=2a00:1450:4864:20::341; helo=mail-wm1-x341.google.com; envelope-from=leif.lindholm@linaro.org; receiver=edk2-devel@lists.01.org Received: from mail-wm1-x341.google.com (mail-wm1-x341.google.com [IPv6:2a00:1450:4864:20::341]) (using TLSv1.2 with cipher ECDHE-RSA-AES128-GCM-SHA256 (128/128 bits)) (No client certificate requested) by ml01.01.org (Postfix) with ESMTPS id 7457821163711 for ; Mon, 8 Oct 2018 07:05:02 -0700 (PDT) Received: by mail-wm1-x341.google.com with SMTP id e187-v6so8469797wmf.0 for ; Mon, 08 Oct 2018 07:05:02 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=linaro.org; s=google; h=date:from:to:cc:subject:message-id:references:mime-version :content-disposition:in-reply-to:user-agent; bh=KmTNqs7pTd7ehVrpk9SDhAx/xIu2/Aeu8xwZaC3ptZE=; b=cQn48YSQHZ3Bs2XOV6rkoyd265ZWjTF9Pgqv7Gd7CFRPp41euvGRTUuEIE02hncTjo Uu1DfOvQcxkDRRM1pEH8KiUE1u2Ib6N9DBGGO2dBbVnCmRA/TaZCRiZDJ4DiLowqYCW9 Yf9S7B0PFSLhnK0LqIeroHSN5ljLFAtcOCzC8= X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:date:from:to:cc:subject:message-id:references :mime-version:content-disposition:in-reply-to:user-agent; bh=KmTNqs7pTd7ehVrpk9SDhAx/xIu2/Aeu8xwZaC3ptZE=; b=Mjk65CjqcyHiGu087vSRELT8YDmQWXTWqlsar+JvZz4wkLiRibi+nV3DAtj3+SaKxG 20lX83b/i/4ANRkjhzg+pNCJgKkZdhkbASRVb8sLHyl2QFFaXDR65NKAS8TBuGrW7hjk UwFuABQE1Lj091EzXBRiHN/5i2BojHCsd8IM6zTNQ2bTPMkDnRlNtz34FOVqB5Gex82r n+pNLnVchnrJhih1jE4iZnaw0P9cA5VGa0pWj/lMJLdwSvaJiDMnC8tosIi3XhD3fwyR VVQZVW97+QmNdiasWNvWmc2Bxtlfsz4hEthLE6rRHH5a0DdBnSfLapSPw3+RQHcY6TcT cWZw== X-Gm-Message-State: ABuFfogyPCCT7exzXj9dJPm5BcbEt0d3hsjw3JBHGdoSJPy4Y4gbceg6 E48RzmyXd9oCAy/8hsBZMITe5A== X-Google-Smtp-Source: ACcGV623yNoT67z+Gq0GLcnpFVf8m88YjLjmNclu3EUt2qcQRy5EcbIu+uHpw8BRAQ4AzWE/ub+8ig== X-Received: by 2002:a1c:1694:: with SMTP id 142-v6mr15879989wmw.113.1539007499991; Mon, 08 Oct 2018 07:04:59 -0700 (PDT) Received: from bivouac.eciton.net (bivouac.eciton.net. [2a00:1098:0:86:1000:23:0:2]) by smtp.gmail.com with ESMTPSA id j46-v6sm27785579wre.91.2018.10.08.07.04.58 (version=TLS1_2 cipher=ECDHE-RSA-CHACHA20-POLY1305 bits=256/256); Mon, 08 Oct 2018 07:04:58 -0700 (PDT) Date: Mon, 8 Oct 2018 15:04:57 +0100 From: Leif Lindholm To: Sumit Garg Cc: Ard Biesheuvel , edk2-devel@lists.01.org, Michael D Kinney , tee-dev@lists.linaro.org, Daniel Thompson , Joakim Bech , Matteo.Carlini@arm.com, Achin.Gupta@arm.com, udit.kumar@nxp.com Message-ID: <20181008140456.c6vjyz3tfdyvztig@bivouac.eciton.net> References: <1538552637-6972-1-git-send-email-sumit.garg@linaro.org> <1538552637-6972-2-git-send-email-sumit.garg@linaro.org> <20181005150303.aew4enyvo42liq2b@bivouac.eciton.net> MIME-Version: 1.0 In-Reply-To: User-Agent: NeoMutt/20170113 (1.7.2) Subject: Re: [PATCH v3 1/1] ArmPkg/OpteeLib: Add APIs to communicate with OP-TEE X-BeenThere: edk2-devel@lists.01.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: EDK II Development List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Mon, 08 Oct 2018 14:05:03 -0000 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline On Mon, Oct 08, 2018 at 03:20:27PM +0530, Sumit Garg wrote: > On Fri, 5 Oct 2018 at 20:33, Leif Lindholm wrote: > > > > On Wed, Oct 03, 2018 at 11:33:01AM +0200, Ard Biesheuvel wrote: > > > On 3 October 2018 at 09:43, Sumit Garg wrote: > > > > Add following APIs to communicate with OP-TEE pseudo/early TAs: > > > > 1. OpteeInit > > > > 2. OpteeOpenSession > > > > 3. OpteeCloseSession > > > > 4. OpteeInvokeFunc > > > > > > > > Cc: Ard Biesheuvel > > > > Cc: Leif Lindholm > > > > Cc: Michael D Kinney > > > > Contributed-under: TianoCore Contribution Agreement 1.1 > > > > Signed-off-by: Sumit Garg > > > > > > Given the outcome of the GP discussion, I'm fine with this approach. Leif? > > > > Apologies for the delay, I needed some time to think it over. > > > > I'm not super happy about this approach, but I'm happier with this > > than I would be with leaving the functionality out of the upstream > > tree. I really hope we can get that license either changed or dropped > > completely. > > > > Thanks Leif for this go ahead. > > > I do have a few comments below. > > > > > > --- > > > > ArmPkg/Include/Library/OpteeLib.h | 90 +++++++++ > > > > ArmPkg/Library/OpteeLib/Optee.c | 357 +++++++++++++++++++++++++++++++++++ > > > > ArmPkg/Library/OpteeLib/OpteeLib.inf | 2 + > > > > ArmPkg/Library/OpteeLib/OpteeSmc.h | 43 +++++ > > > > Could you follow the instructions in > > https://github.com/tianocore/tianocore.github.io/wiki/Laszlo's-unkempt-git-guide-for-edk2-contributors-and-maintainers#contrib-23 > > when generating future patches? > > The --stat* effects aren't apparent here, but the -O ones are. > > > > Sure. > > > > > 4 files changed, 492 insertions(+) > > > > create mode 100644 ArmPkg/Library/OpteeLib/OpteeSmc.h > > > > > > > > diff --git a/ArmPkg/Include/Library/OpteeLib.h b/ArmPkg/Include/Library/OpteeLib.h > > > > index f65d8674d9b8..2d1c60632dfe 100644 > > > > --- a/ArmPkg/Include/Library/OpteeLib.h > > > > +++ b/ArmPkg/Include/Library/OpteeLib.h > > > > @@ -25,10 +25,100 @@ > > > > #define OPTEE_OS_UID2 0xaf630002 > > > > #define OPTEE_OS_UID3 0xa5d5c51b > > > > > > > > +#define OPTEE_MSG_ATTR_TYPE_NONE 0x0 > > > > +#define OPTEE_MSG_ATTR_TYPE_VALUE_INPUT 0x1 > > > > +#define OPTEE_MSG_ATTR_TYPE_VALUE_OUTPUT 0x2 > > > > +#define OPTEE_MSG_ATTR_TYPE_VALUE_INOUT 0x3 > > > > +#define OPTEE_MSG_ATTR_TYPE_MEM_INPUT 0x9 > > > > +#define OPTEE_MSG_ATTR_TYPE_MEM_OUTPUT 0xa > > > > +#define OPTEE_MSG_ATTR_TYPE_MEM_INOUT 0xb > > > > + > > > > +#define OPTEE_MSG_ATTR_TYPE_MASK 0xff > > > > + > > > > +#define OPTEE_ORIGIN_COMMS 0x00000002 > > > > +#define OPTEE_ERROR_COMMS 0xFFFF000E > > > > + > > > > +typedef struct { > > > > + UINT64 BufPtr; > > > > If it's a pointer, it has a *. > > Otherwise it's an address. > > > > Will rather use BufferAddress. > > > > > + UINT64 Size; > > > > + UINT64 ShmRef; > > > > Abbreviations in function, variable or type names (other than the ones > > defined in [1] are not permitted unless they are explicitly added to a > > glossary section of the source file header. Where possible, just write > > out the name in full. > > > > [1] https://edk2-docs.gitbooks.io/edk-ii-c-coding-standards-specification/content/v/release/2.20/4_naming_conventions/#table-2-efi-supported-abbreviations > > > > BufPtr (as a name) gets a pass since it's unambiguous and we already > > have a bunch of those in the codebase. ShmRef needs to be clear. > > > > Will replace it with SharedMemoryReference. > > > (This comment also applies to a lot of things below, I won't point > > them all out unless asked to.) > > > > I will try to replace most of them with full names. But I have > confusion regarding usage of following not included in [1]: > > 1. MSG/Msg > 2. MEM/Mem > 3. INFO/Info > 4. FUNC/Func > 5. RET/Ret > 6. ATTR/Attr > 7. CMD/Cmd > > But I do see their usage in the codebase. Please help to clarify. It's a horrible secret, but not all of the codebase conforms to the coding style. We're continuously trying to improve it. > Also can I use 1-2 letter variable or struct member names like Mp, Ip, > Op, U etc.? Ideally not. I'm not a huge fan of ReallyLongCamelCase coding styles, but the benefit it does bring is that you don't need to stop and think about what a variable may be for. And correspondingly, mixing between abbreviations an non-abbreviations make for hard reading. Another completely unscientific observation I would make is that once you force yourself to write a full variable name out, it becomes a lot more clear when you've left valuable semantic information out. The codebase ends up with a bunch of fewer variables called only 'Msg', 'Mem', 'Info', Func', 'Ret', 'Attr' 'Cmd' (, or 'Data' ...). (Alternatively, the mailing list ends up with a lot fewer emails asking "what message", "what memory", "what information" ...) > > > > +} OPTEE_MSG_PARAM_MEM; > > > > + > > > > +typedef struct { > > > > + UINT64 A; > > > > + UINT64 B; > > > > + UINT64 C; > > > > +} OPTEE_MSG_PARAM_VALUE; > > > > + > > > > +typedef struct { > > > > + UINT64 Attr; > > > > + union { > > > > + OPTEE_MSG_PARAM_MEM Mem; > > > > + OPTEE_MSG_PARAM_VALUE Value; > > > > + } U; > > > > +} OPTEE_MSG_PARAM; > > > > + > > > > +#define MAX_PARAMS 4 > > > > This is a very localised macro with a very globalised name. > > Suggest adding an OPTEE_ prefix, but also something describing what it > > is the maximum parameters for. CALL_? > > > > Ok will rename it to OPTEE_CALL_MAX_PARAMS. I'd take that or OPTEE_MAX_CALL_PARAMS. > > > > + > > > > +typedef struct { > > > > + UINT32 Cmd; > > > > + UINT32 Func; > > > > + UINT32 Session; > > > > + UINT32 CancelId; > > > > + UINT32 Pad; > > > > + UINT32 Ret; > > > > + UINT32 RetOrigin; > > > > + UINT32 NumParams; > > > > + > > > > + // NumParams tells the actual number of element in Params > > > > + OPTEE_MSG_PARAM Params[MAX_PARAMS]; > > > > +} OPTEE_MSG_ARG; > > > > + > > > > +#define OPTEE_UUID_LEN 16 > > > > UUIDs are UUIDs. If optee decides on an incompatible format, we may > > have an interoperability issue. I assume this is not the case, so > > perhaps replace with sizeof (EFI_GUID) at each point of use? > > Agree will use sizeof (EFI_GUID) instead. > > > > > + > > > > +typedef struct { > > > > + UINT8 Uuid[OPTEE_UUID_LEN]; // [in] UUID of the Trusted Application > > > > Is there a strong reason for not using EFI_GUID here? > > > > EFI_GUID can be used here as UUID are also known as GUID. But here we > pass UUID as 16 octets to OP-TEE. So will use EFI_GUID instead and add > an api to convert uuid to octets. Right. That may be tedious, but I think it will be an improvement. If OPTEE uses UUIDs without the Microsoft timestamp endianness variant (as described in the UEFI spec), then ideally those functions could byte reverse the timestamp as well. > > > > + UINT32 Session; // [out] Session id > > > > + UINT32 Ret; // [out] Return value > > > > + UINT32 RetOrigin; // [out] Origin of the return value > > > > +} OPTEE_OPEN_SESSION_ARG; > > > > + > > > > +typedef struct { > > > > + UINT32 Func; // [in] Trusted App func, specific to the TA > > > > + UINT32 Session; // [in] Session id > > > > + UINT32 Ret; // [out] Return value > > > > + UINT32 RetOrigin; // [out] Origin of the return value > > > > + OPTEE_MSG_PARAM Params[MAX_PARAMS]; // Params for func to be invoked > > > > +} OPTEE_INVOKE_FUNC_ARG; > > > > + > > > > BOOLEAN > > > > EFIAPI > > > > IsOpteePresent ( > > > > VOID > > > > ); > > > > > > > > +EFI_STATUS > > > > +EFIAPI > > > > +OpteeInit ( > > > > + VOID > > > > + ); > > > > + > > > > +EFI_STATUS > > > > +EFIAPI > > > > +OpteeOpenSession ( > > > > + IN OUT OPTEE_OPEN_SESSION_ARG *OpenSessionArg > > > > + ); > > > > + > > > > +EFI_STATUS > > > > +EFIAPI > > > > +OpteeCloseSession ( > > > > + IN UINT32 Session > > > > + ); > > > > + > > > > +EFI_STATUS > > > > +EFIAPI > > > > +OpteeInvokeFunc ( > > > > + IN OUT OPTEE_INVOKE_FUNC_ARG *InvokeFuncArg > > > > + ); > > > > + > > > > #endif > > > > diff --git a/ArmPkg/Library/OpteeLib/Optee.c b/ArmPkg/Library/OpteeLib/Optee.c > > > > index 574527f8b5ea..bf7872cbbce0 100644 > > > > --- a/ArmPkg/Library/OpteeLib/Optee.c > > > > +++ b/ArmPkg/Library/OpteeLib/Optee.c > > > > @@ -14,11 +14,18 @@ > > > > > > > > **/ > > > > > > > > +#include > > > > #include > > > > +#include > > > > #include > > > > +#include > > > > #include > > > > > > > > #include > > > > +#include > > > > +#include > > > > + > > > > +STATIC OPTEE_SHARED_MEMORY_INFO OpteeShmInfo = { 0 }; > > > > > > > > /** > > > > Check for OP-TEE presence. > > > > @@ -31,6 +38,7 @@ IsOpteePresent ( > > > > { > > > > ARM_SMC_ARGS ArmSmcArgs; > > > > > > > > + ZeroMem (&ArmSmcArgs, sizeof (ARM_SMC_ARGS)); > > > > // Send a Trusted OS Calls UID command > > > > ArmSmcArgs.Arg0 = ARM_SMC_ID_TOS_UID; > > > > ArmCallSmc (&ArmSmcArgs); > > > > @@ -44,3 +52,352 @@ IsOpteePresent ( > > > > return FALSE; > > > > } > > > > } > > > > + > > > > +STATIC > > > > +EFI_STATUS > > > > +OpteeShmMemRemap ( > > > > + VOID > > > > + ) > > > > +{ > > > > + ARM_SMC_ARGS ArmSmcArgs; > > > > + EFI_PHYSICAL_ADDRESS Paddr; > > > > + EFI_PHYSICAL_ADDRESS Start; > > > > + EFI_PHYSICAL_ADDRESS End; > > > > + EFI_STATUS Status; > > > > + UINTN Size; > > > > + > > > > + ZeroMem (&ArmSmcArgs, sizeof (ARM_SMC_ARGS)); > > > > + ArmSmcArgs.Arg0 = OPTEE_SMC_GET_SHM_CONFIG; > > > > + > > > > + ArmCallSmc (&ArmSmcArgs); > > > > + if (ArmSmcArgs.Arg0 != OPTEE_SMC_RETURN_OK) { > > > > + DEBUG ((DEBUG_WARN, "OP-TEE shared memory not supported\n")); > > > > + return EFI_UNSUPPORTED; > > > > + } > > > > + > > > > + if (ArmSmcArgs.Arg3 != OPTEE_SMC_SHM_CACHED) { > > > > + DEBUG ((DEBUG_WARN, "OP-TEE: Only normal cached shared memory supported\n")); > > > > + return EFI_UNSUPPORTED; > > > > + } > > > > + > > > > + Start = (ArmSmcArgs.Arg1 + SIZE_4KB - 1) & ~(SIZE_4KB - 1); > > > > + End = (ArmSmcArgs.Arg1 + ArmSmcArgs.Arg2) & ~(SIZE_4KB - 1); > > > > + Paddr = Start; > > > > + Size = End - Start; > > > > + > > > > + if (Size < SIZE_4KB) { > > > > + DEBUG ((DEBUG_WARN, "OP-TEE shared memory too small\n")); > > > > + return EFI_BUFFER_TOO_SMALL; > > > > + } > > > > + > > > > + Status = ArmSetMemoryAttributes (Paddr, Size, EFI_MEMORY_WB); > > > > + if (EFI_ERROR (Status)) { > > > > + return Status; > > > > + } > > > > + > > > > + OpteeShmInfo.Base = (UINTN)Paddr; > > > > Would PHYSICAL_ADDRESS (always 64-bit) be more correct here, or is the > > OP-TEE end seeing this struct as always having fields of native width? > > Its a global struct not shared with OP-TEE. Used native width to have > compatibility with 32-bit system. OK, that is not a problem then. > > (This is where the ordefile comes in handy, because it means I see the > > struct definition first, not its use.) > > > > > > + OpteeShmInfo.Size = Size; > > > > + > > > > + return EFI_SUCCESS; > > > > +} > > > > + > > > > +EFI_STATUS > > > > +EFIAPI > > > > +OpteeInit ( > > > > + VOID > > > > + ) > > > > +{ > > > > + EFI_STATUS Status; > > > > + > > > > + if (!IsOpteePresent ()) { > > > > + DEBUG ((DEBUG_WARN, "OP-TEE not present\n")); > > > > + return EFI_UNSUPPORTED; > > > > + } > > > > + > > > > + Status = OpteeShmMemRemap (); > > > > + if (EFI_ERROR (Status)) { > > > > + DEBUG ((DEBUG_WARN, "OP-TEE shared memory remap failed\n")); > > > > + return Status; > > > > + } > > > > + > > > > + return EFI_SUCCESS; > > > > +} > > > > + > > > > +/** > > > > + Does Standard SMC to OP-TEE in secure world. > > > > + > > > > + @param[in] Parg Physical address of message to pass to secure world > > > > + > > > > + @return 0 on success, secure world return code otherwise > > > > + > > > > +**/ > > > > +STATIC > > > > +UINT32 > > > > +OpteeCallWithArg ( > > > > + IN EFI_PHYSICAL_ADDRESS Parg > > > > + ) > > > > +{ > > > > + ARM_SMC_ARGS ArmSmcArgs; > > > > + > > > > + ZeroMem (&ArmSmcArgs, sizeof (ARM_SMC_ARGS)); > > > > + ArmSmcArgs.Arg0 = OPTEE_SMC_CALL_WITH_ARG; > > > > + ArmSmcArgs.Arg1 = (UINT32)(Parg >> 32); > > > > + ArmSmcArgs.Arg2 = (UINT32)Parg; > > > > + > > > > + while (TRUE) { > > > > + ArmCallSmc (&ArmSmcArgs); > > > > + > > > > + if (ArmSmcArgs.Arg0 == OPTEE_SMC_RETURN_RPC_FOREIGN_INTR) { > > > > + // > > > > + // A foreign interrupt was raised while secure world was > > > > + // executing, since they are handled in UEFI a dummy RPC is > > > > + // performed to let UEFI take the interrupt through the normal > > > > + // vector. > > > > + // > > > > + ArmSmcArgs.Arg0 = OPTEE_SMC_RETURN_FROM_RPC; > > > > + } else { > > > > + break; > > > > + } > > > > + } > > > > + > > > > + return ArmSmcArgs.Arg0; > > > > +} > > > > + > > > > +EFI_STATUS > > > > +EFIAPI > > > > +OpteeOpenSession ( > > > > + IN OUT OPTEE_OPEN_SESSION_ARG *OpenSessionArg > > > > + ) > > > > +{ > > > > + OPTEE_MSG_ARG *MsgArg; > > > > + > > > > + MsgArg = NULL; > > > > + > > > > + if (OpteeShmInfo.Base == 0) { > > > > + DEBUG ((DEBUG_WARN, "OP-TEE not initialized\n")); > > > > + return EFI_NOT_STARTED; > > > > + } > > > > + > > > > + MsgArg = (OPTEE_MSG_ARG *)OpteeShmInfo.Base; > > > > + ZeroMem (MsgArg, sizeof (OPTEE_MSG_ARG)); > > > > + > > > > + MsgArg->Cmd = OPTEE_MSG_CMD_OPEN_SESSION; > > > > + > > > > + // > > > > + // Initialize and add the meta parameters needed when opening a > > > > + // session. > > > > + // > > > > + MsgArg->Params[0].Attr = OPTEE_MSG_ATTR_TYPE_VALUE_INPUT | > > > > + OPTEE_MSG_ATTR_META; > > > > + MsgArg->Params[1].Attr = OPTEE_MSG_ATTR_TYPE_VALUE_INPUT | > > > > + OPTEE_MSG_ATTR_META; > > > > + CopyMem (&MsgArg->Params[0].U.Value, OpenSessionArg->Uuid, OPTEE_UUID_LEN); > > > > + ZeroMem (&MsgArg->Params[1].U.Value, OPTEE_UUID_LEN); > > > > + MsgArg->Params[1].U.Value.C = TEE_LOGIN_PUBLIC; > > > > + > > > > + MsgArg->NumParams = 2; > > > > + > > > > + if (OpteeCallWithArg ((EFI_PHYSICAL_ADDRESS)MsgArg)) { > > > > + MsgArg->Ret = OPTEE_ERROR_COMMS; > > > > + MsgArg->RetOrigin = OPTEE_ORIGIN_COMMS; > > > > + } > > > > + > > > > + OpenSessionArg->Session = MsgArg->Session; > > > > + OpenSessionArg->Ret = MsgArg->Ret; > > > > + OpenSessionArg->RetOrigin = MsgArg->RetOrigin; > > > > + > > > > + return EFI_SUCCESS; > > > > +} > > > > + > > > > +EFI_STATUS > > > > +EFIAPI > > > > +OpteeCloseSession ( > > > > + IN UINT32 Session > > > > + ) > > > > +{ > > > > + OPTEE_MSG_ARG *MsgArg; > > > > + > > > > + MsgArg = NULL; > > > > + > > > > + if (OpteeShmInfo.Base == 0) { > > > > + DEBUG ((DEBUG_WARN, "OP-TEE not initialized\n")); > > > > + return EFI_NOT_STARTED; > > > > + } > > > > + > > > > + MsgArg = (OPTEE_MSG_ARG *)OpteeShmInfo.Base; > > > > + ZeroMem (MsgArg, sizeof (OPTEE_MSG_ARG)); > > > > + > > > > + MsgArg->Cmd = OPTEE_MSG_CMD_CLOSE_SESSION; > > > > + MsgArg->Session = Session; > > > > + > > > > + OpteeCallWithArg ((EFI_PHYSICAL_ADDRESS)MsgArg); > > > > + > > > > + return EFI_SUCCESS; > > > > +} > > > > + > > > > +STATIC > > > > +EFI_STATUS > > > > +OpteeToMsgParam ( > > > > + OUT OPTEE_MSG_PARAM *MsgParams, > > > > + IN UINT32 NumParams, > > > > + IN OPTEE_MSG_PARAM *InParams > > > > + ) > > > > +{ > > > > + UINT32 Idx; > > > > + UINTN ParamShmAddr; > > > > + UINTN ShmSize; > > > > + UINTN Size; > > > > + > > > > + Size = (sizeof (OPTEE_MSG_ARG) + sizeof (UINT64) - 1) & ~(sizeof (UINT64) - 1); > > > > + ParamShmAddr = OpteeShmInfo.Base + Size; > > > > + ShmSize = OpteeShmInfo.Size - Size; > > > > + > > > > + for (Idx = 0; Idx < NumParams; Idx++) { > > > > + CONST OPTEE_MSG_PARAM *Ip; > > > > + OPTEE_MSG_PARAM *Mp; > > > > + UINT32 Attr; > > > > + > > > > + Ip = InParams + Idx; > > > > + Mp = MsgParams + Idx; > > > > + Attr = Ip->Attr & OPTEE_MSG_ATTR_TYPE_MASK; > > > > + > > > > + switch (Attr) { > > > > + case OPTEE_MSG_ATTR_TYPE_NONE: > > > > + Mp->Attr = OPTEE_MSG_ATTR_TYPE_NONE; > > > > + ZeroMem (&Mp->U, sizeof (Mp->U)); > > > > + break; > > > > + > > > > + case OPTEE_MSG_ATTR_TYPE_VALUE_INPUT: > > > > + case OPTEE_MSG_ATTR_TYPE_VALUE_OUTPUT: > > > > + case OPTEE_MSG_ATTR_TYPE_VALUE_INOUT: > > > > + Mp->Attr = Attr; > > > > + Mp->U.Value.A = Ip->U.Value.A; > > > > + Mp->U.Value.B = Ip->U.Value.B; > > > > + Mp->U.Value.C = Ip->U.Value.C; > > > > + break; > > > > + > > > > + case OPTEE_MSG_ATTR_TYPE_MEM_INPUT: > > > > + case OPTEE_MSG_ATTR_TYPE_MEM_OUTPUT: > > > > + case OPTEE_MSG_ATTR_TYPE_MEM_INOUT: > > > > + Mp->Attr = Attr; > > > > + > > > > + if (Ip->U.Mem.Size > ShmSize) { > > > > + return EFI_OUT_OF_RESOURCES; > > > > + } > > > > + > > > > + CopyMem ((VOID *)ParamShmAddr, (VOID *)Ip->U.Mem.BufPtr, Ip->U.Mem.Size); > > > > + Mp->U.Mem.BufPtr = (UINT64)ParamShmAddr; > > > > While I don't think it's possible for this to end up doing something > > unexpected, casting pointers to a higher alignment (which this would > > do on 32-bit) is a bad habit to get into. > > This is a shared message param structure with OP-TEE. So needed to cast it. Then you need to also ensure that you are not accidentally casting a less-than-8-byte aligned pointer here. I would suggest an explicit test with an error return and an assert. Then if that thing ever does come back to bite you, you will know why, rather than needing to spend a few days debugging. Alternatively, make ParamShmAddr an UINT64 * to begin with. > > > > + Mp->U.Mem.Size = Ip->U.Mem.Size; > > > > + > > > > + Size = (Ip->U.Mem.Size + sizeof (UINT64) - 1) & ~(sizeof (UINT64) - 1); > > > > + ParamShmAddr += Size; > > > > + ShmSize -= Size; > > > > + break; > > > > + > > > > + default: > > > > + return EFI_INVALID_PARAMETER; > > > > + } > > > > + } > > > > + > > > > + return EFI_SUCCESS; > > > > +} > > > > + > > > > +STATIC > > > > +EFI_STATUS > > > > +OpteeFromMsgParam ( > > > > + OUT OPTEE_MSG_PARAM *OutParams, > > > > + IN UINT32 NumParams, > > > > + IN OPTEE_MSG_PARAM *MsgParams > > > > + ) > > > > +{ > > > > + UINT32 Idx; > > > > + > > > > + for (Idx = 0; Idx < NumParams; Idx++) { > > > > + OPTEE_MSG_PARAM *Op; > > > > + CONST OPTEE_MSG_PARAM *Mp; > > > > + UINT32 Attr; > > > > + > > > > + Op = OutParams + Idx; > > > > + Mp = MsgParams + Idx; > > > > + Attr = Mp->Attr & OPTEE_MSG_ATTR_TYPE_MASK; > > > > + > > > > + switch (Attr) { > > > > + case OPTEE_MSG_ATTR_TYPE_NONE: > > > > + Op->Attr = OPTEE_MSG_ATTR_TYPE_NONE; > > > > + ZeroMem (&Op->U, sizeof (Op->U)); > > > > + break; > > > > + > > > > + case OPTEE_MSG_ATTR_TYPE_VALUE_INPUT: > > > > + case OPTEE_MSG_ATTR_TYPE_VALUE_OUTPUT: > > > > + case OPTEE_MSG_ATTR_TYPE_VALUE_INOUT: > > > > + Op->Attr = Attr; > > > > + Op->U.Value.A = Mp->U.Value.A; > > > > + Op->U.Value.B = Mp->U.Value.B; > > > > + Op->U.Value.C = Mp->U.Value.C; > > > > + break; > > > > + > > > > + case OPTEE_MSG_ATTR_TYPE_MEM_INPUT: > > > > + case OPTEE_MSG_ATTR_TYPE_MEM_OUTPUT: > > > > + case OPTEE_MSG_ATTR_TYPE_MEM_INOUT: > > > > + Op->Attr = Attr; > > > > + > > > > + if (Mp->U.Mem.Size > Op->U.Mem.Size) { > > > > + return EFI_BAD_BUFFER_SIZE; > > > > + } > > > > + > > > > + CopyMem ((VOID *)Op->U.Mem.BufPtr, (VOID *)Mp->U.Mem.BufPtr, Mp->U.Mem.Size); > > > > + Op->U.Mem.Size = Mp->U.Mem.Size; > > > > + break; > > > > + > > > > + default: > > > > + return EFI_INVALID_PARAMETER; > > > > + } > > > > + } > > > > + > > > > + return EFI_SUCCESS; > > > > +} > > > > + > > > > +EFI_STATUS > > > > +EFIAPI > > > > +OpteeInvokeFunc ( > > > > + IN OUT OPTEE_INVOKE_FUNC_ARG *InvokeFuncArg > > > > + ) > > > > +{ > > > > + EFI_STATUS Status; > > > > + OPTEE_MSG_ARG *MsgArg; > > > > + > > > > + MsgArg = NULL; > > > > + > > > > + if (OpteeShmInfo.Base == 0) { > > > > + DEBUG ((DEBUG_WARN, "OP-TEE not initialized\n")); > > > > + return EFI_NOT_STARTED; > > > > + } > > > > + > > > > + MsgArg = (OPTEE_MSG_ARG *)OpteeShmInfo.Base; > > > > + ZeroMem (MsgArg, sizeof (OPTEE_MSG_ARG)); > > > > + > > > > + MsgArg->Cmd = OPTEE_MSG_CMD_INVOKE_COMMAND; > > > > + MsgArg->Func = InvokeFuncArg->Func; > > > > + MsgArg->Session = InvokeFuncArg->Session; > > > > + > > > > + Status = OpteeToMsgParam (MsgArg->Params, MAX_PARAMS, InvokeFuncArg->Params); > > > > + if (Status) > > > > + return Status; > > > > Always use {} (coding style requirement). > > > > Ok. > > > > > + > > > > + MsgArg->NumParams = MAX_PARAMS; > > > > + > > > > + if (OpteeCallWithArg ((EFI_PHYSICAL_ADDRESS)MsgArg)) { > > > > + MsgArg->Ret = OPTEE_ERROR_COMMS; > > > > + MsgArg->RetOrigin = OPTEE_ORIGIN_COMMS; > > > > + } > > > > + > > > > + if (OpteeFromMsgParam (InvokeFuncArg->Params, MAX_PARAMS, MsgArg->Params)) { > > > > + MsgArg->Ret = OPTEE_ERROR_COMMS; > > > > + MsgArg->RetOrigin = OPTEE_ORIGIN_COMMS; > > > > + } > > > > + > > > > + InvokeFuncArg->Ret = MsgArg->Ret; > > > > + InvokeFuncArg->RetOrigin = MsgArg->RetOrigin; > > > > + > > > > + return EFI_SUCCESS; > > > > +} > > > > diff --git a/ArmPkg/Library/OpteeLib/OpteeLib.inf b/ArmPkg/Library/OpteeLib/OpteeLib.inf > > > > index 5abd427379cc..e03054a7167d 100644 > > > > --- a/ArmPkg/Library/OpteeLib/OpteeLib.inf > > > > +++ b/ArmPkg/Library/OpteeLib/OpteeLib.inf > > > > @@ -23,11 +23,13 @@ [Defines] > > > > > > > > [Sources] > > > > Optee.c > > > > + OpteeSmc.h > > > > > > > > [Packages] > > > > ArmPkg/ArmPkg.dec > > > > MdePkg/MdePkg.dec > > > > > > > > [LibraryClasses] > > > > + ArmMmuLib > > > > ArmSmcLib > > > > BaseLib > > > > diff --git a/ArmPkg/Library/OpteeLib/OpteeSmc.h b/ArmPkg/Library/OpteeLib/OpteeSmc.h > > > > new file mode 100644 > > > > index 000000000000..e2ea35784a0a > > > > --- /dev/null > > > > +++ b/ArmPkg/Library/OpteeLib/OpteeSmc.h > > > > @@ -0,0 +1,43 @@ > > > > +/** @file > > > > + OP-TEE SMC header file. > > > > + > > > > + Copyright (c) 2018, Linaro Ltd. All rights reserved.
> > > > + > > > > + This program and the accompanying materials > > > > + are licensed and made available under the terms and conditions of the BSD License > > > > + which accompanies this distribution. The full text of the license may be found at > > > > + http://opensource.org/licenses/bsd-license.php > > > > + > > > > + THE PROGRAM IS DISTRIBUTED UNDER THE BSD LICENSE ON AN "AS IS" BASIS, > > > > + WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER EXPRESS OR IMPLIED. > > > > + > > > > +**/ > > > > + > > > > +#ifndef _OPTEE_SMC_H_ > > > > +#define _OPTEE_SMC_H_ > > > > + > > > > +/* Returned in Arg0 only from Trusted OS functions */ > > > > +#define OPTEE_SMC_RETURN_OK 0x0 > > > > + > > > > +#define OPTEE_SMC_RETURN_FROM_RPC 0x32000003 > > > > +#define OPTEE_SMC_CALL_WITH_ARG 0x32000004 > > > > +#define OPTEE_SMC_GET_SHM_CONFIG 0xb2000007 > > > > + > > > > +#define OPTEE_SMC_SHM_CACHED 1 > > > > + > > > > +#define OPTEE_SMC_RETURN_RPC_FOREIGN_INTR 0xffff0004 > > > > + > > > > +#define OPTEE_MSG_CMD_OPEN_SESSION 0 > > > > +#define OPTEE_MSG_CMD_INVOKE_COMMAND 1 > > > > +#define OPTEE_MSG_CMD_CLOSE_SESSION 2 > > > > + > > > > +#define OPTEE_MSG_ATTR_META 0x100 > > > > + > > > > +#define TEE_LOGIN_PUBLIC 0x0 > > > > I'm a bit apprehensive about taking over the whole TEE namespace in an > > implementation-specific header file. > > Can/should this be OPTEE_? > > > > Ok will change this to OPTEE_. Thanks! / Leif