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.web08.8828.1648115176271000915 for ; Thu, 24 Mar 2022 02:46:16 -0700 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 131531515; Thu, 24 Mar 2022 02:46:15 -0700 (PDT) Received: from [10.57.40.69] (unknown [10.57.40.69]) by usa-sjc-imap-foss1.foss.arm.com (Postfix) with ESMTPSA id 9B25E3F73B; Thu, 24 Mar 2022 02:46:11 -0700 (PDT) Message-ID: <4e395fdb-2fe3-4a90-aeca-2a65122e690b@arm.com> Date: Thu, 24 Mar 2022 10:46:30 +0100 MIME-Version: 1.0 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:91.0) Gecko/20100101 Thunderbird/91.5.0 Subject: Re: [edk2-devel] [PATCH v2 3/8] ArmPkg: Add Arm Firmware TRNG library To: devel@edk2.groups.io, sami.mujawar@arm.com, Leif Lindholm Cc: ardb+tianocore@kernel.org, rebecca@bsdio.com, kraxel@redhat.com, michael.d.kinney@intel.com, gaoliming@byosoft.com.cn, zhiguang.liu@intel.com, jiewen.yao@intel.com, jian.j.wang@intel.com, Matteo.Carlini@arm.com, Akanksha.Jain2@arm.com, Ben.Adderson@arm.com, nd@arm.com References: <20211116113301.31088-1-sami.mujawar@arm.com> <20211116113301.31088-4-sami.mujawar@arm.com> <15621f6b-8df4-a65b-9996-92b68c1ae3c1@arm.com> From: "PierreGondois" In-Reply-To: <15621f6b-8df4-a65b-9996-92b68c1ae3c1@arm.com> Content-Language: en-US Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 7bit On 11/25/21 16:23, Sami Mujawar via groups.io wrote: > Hi Leif, > > Thank you for the feedback. > > Please find my response inline marked [SAMI]. > > Regards, > > Sami Mujawar > > > On 24/11/2021 01:01 PM, Leif Lindholm wrote: >> Hi Sami, >> >> On Tue, Nov 16, 2021 at 11:32:55 +0000, Sami Mujawar wrote: >>> Bugzilla: 3668 (https://bugzilla.tianocore.org/show_bug.cgi?id=3668) >>> >>> The Arm True Random Number Generator Firmware, Interface 1.0, >>> Platform Design Document >>> (https://developer.arm.com/documentation/den0098/latest/) >>> defines an interface between an Operating System (OS) executing >>> at EL1 and Firmware (FW) exposing a conditioned entropy source >>> that is provided by a TRNG back end. >>> >>> The conditioned entropy, that is provided by the TRNG FW interface, >>> is commonly used to seed deterministic random number generators. >>> >>> This patch adds a TrngLib library that implements the Arm TRNG >>> firmware interface. >>> >>> Signed-off-by: Sami Mujawar >>> --- >>> >>> Notes: >>> v2: >>> - MdePkg\Include\Library\TrngLib.h is base type [LIMING] >>> library. It can use RETURN_STATUS instead of >>> EFI_STATUS. >>> - Replaced EFI_STATUS with RETURN_STATUS. [SAMI] >>> - MdePkg\Include\Library\TrngLib.h API parameter [LIMING] >>> doesn't require CONST. CONST means the value >>> specified by the input pointer will not be >>> changed in API implementation. >>> - Removed the use of constant pointers in the [SAMI] >>> TRNG API. >>> >>> ArmPkg/ArmPkg.dsc | 1 + >>> ArmPkg/Library/ArmFwTrngLib/ArmFwTrngDefs.h | 64 +++ >>> ArmPkg/Library/ArmFwTrngLib/ArmFwTrngLib.c | 483 ++++++++++++++++++++ >>> ArmPkg/Library/ArmFwTrngLib/ArmFwTrngLib.inf | 34 ++ >>> 4 files changed, 582 insertions(+) >>> [...] >>> + >>> +/** Invoke the monitor call using the appropriate conduit. >>> + If PcdMonitorConduitHvc is TRUE use the HVC conduit else use SMC conduit. >>> + >>> + @param [in, out] Args Arguments passed to and returned from the monitor. >>> + >>> + @return VOID >>> +**/ >>> +STATIC >>> +VOID >>> +ArmCallMonitor ( >>> + IN OUT ARM_MONITOR_ARGS *Args >>> + ) >>> +{ >>> + if (FeaturePcdGet (PcdMonitorConduitHvc)) { >>> + ArmCallHvc ((ARM_HVC_ARGS*)Args); >>> + } else { >>> + ArmCallSmc ((ARM_SMC_ARGS*)Args); >>> + } >>> +} >> Should this be in (a potentially renamed) ArmSmcLib? > [SAMI] Looking at ArmSmcLib and ArmHvcLib libraries there is not much > difference in the code other than the SMC/HVC call. Please let me know > if I should submit a patch to unify these in ArmMonitorLib? > The ArmCall APIs would still remain the same but moved to > ArmMonitorLib. Hello Leif, About your comment, I am not sure I understand it correctly. Assuming the function allowing to choose the conduit looks like: VOID EFIAPI ArmConduitCall (UINT8 Conduit) { if (Conduit == 0) { ArmCallHvc ((ARM_HVC_ARGS*)Args); } else if (Conduit == 1) { ArmCallSmc ((ARM_SMC_ARGS*)Args); } else { ASSERT (FALSE); } } Do you suggest to: 1. Make ArmSmcLib dependent on ArmHvcLib and add ArmConduitCall() in ArmSmcLib (or do the opposite with the ArmHvcLib) 2. Merge ArmSmcLib and ArmHvcLib in a new ArmConduitLibrary and add ArmConduitCall() in this new library. 3. Add an ArmConduitLibrary, relying on ArmSmcLib and ArmHvcLib, and having only one function: ArmConduitCall() 2. would make the Hvc and Smc calls really tied together. 3. would avoid creating new dependencies on existing libraries (i.e. a platform only using the ArmSmcLib would not require to have a NULL instance of ArmHvcLib). I assume you meant 3. Regards, Pierre