Re: [edk2-devel] [PATCH v2 3/8] ArmPkg: Add Arm Firmware TRNG library

["PierreGondois" <pierre.gondois@arm.com>]

(Replacing Leif's mail address to the @quicinc.com one)


On 3/24/22 10:46, Pierre Gondois wrote:
> 
> 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 <sami.mujawar@arm.com>
>>>> ---
>>>>
>>>> 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<Smc|Hvc> 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
>