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

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

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