Re: [edk2-devel] [PATCH v3 00/22] Add Raw algorithm support using Arm FW-TRNG interface

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

On 7/21/22 12:42, Yao, Jiewen wrote:
> Hi Pierre
> Sorry, I should have given the specific comment.
> 
> [PATCH v3 12/22] SecurityPkg: Update Securitypkg.ci.yaml
> 
> Why we need ArmPkg as new dependency? I don’t think this is good idea.
> Can we have a way to remove that dependency?
> 
> Thank you
> Yao, Jiewen
> 
> 
>> -----Original Message-----
>> From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Yao, Jiewen
>> Sent: Thursday, July 21, 2022 6:33 PM
>> To: Pierre Gondois <pierre.gondois@arm.com>; devel@edk2.groups.io
>> Cc: Sami Mujawar <sami.mujawar@arm.com>; Leif Lindholm
>> <quic_llindhol@quicinc.com>; Ard Biesheuvel <ardb+tianocore@kernel.org>;
>> Rebecca Cran <rebecca@bsdio.com>; Kinney, Michael D
>> <michael.d.kinney@intel.com>; Gao, Liming <gaoliming@byosoft.com.cn>;
>> Wang, Jian J <jian.j.wang@intel.com>
>> Subject: Re: [edk2-devel] [PATCH v3 00/22] Add Raw algorithm support using
>> Arm FW-TRNG interface
>>
>> Hi Pierre
>> I give the feedback on interface design and dependency, esp SecurityPkg and
>> MdePkg.
>> 1) Please don’t change package dependency policy.

Yes right, with the changes suggested below:
  - in SecurityPkg.dsc, have TrngLib|MdePkg/Library/BaseTrngLibNull/BaseTrngLibNull.inf
  - change ArmHasRngExt()
will allow to drop the following patch:
   [PATCH v3 12/22] SecurityPkg: Update Securitypkg.ci.yaml
and maybe some others, so no more dependency over the ArmPkg (from the SecurityPkg).

>> 2) For AesLib/DrbgLib/TrbgLib, I already expressed my concern on adding those
>> new APIs in MdePkg. Please try to avoid that.
>>
>> Basically, since this is ARM FW-TRNG feature, you can consider just add this to
>> ArmPkg without impacting other package. That would be a clean isolation.

I noted your concerns. The TrngLib, AesLib and DrbgLib were split as 3 patch-sets.
The TrngLib should come first in the 3 sets. I still need to work on the AesLib and DrbgLib.
I will send a new version of the TrngLib which will address your concerns and have a
clean isolation.

Thanks for the comments,
Pierre

>>
>> Thank you
>> Yao, Jiewen
>>
>>
>>> -----Original Message-----
>>> From: Pierre Gondois <pierre.gondois@arm.com>
>>> Sent: Thursday, July 21, 2022 4:55 PM
>>> To: Yao, Jiewen <jiewen.yao@intel.com>; devel@edk2.groups.io
>>> Cc: Sami Mujawar <sami.mujawar@arm.com>; Leif Lindholm
>>> <quic_llindhol@quicinc.com>; Ard Biesheuvel <ardb+tianocore@kernel.org>;
>>> Rebecca Cran <rebecca@bsdio.com>; Kinney, Michael D
>>> <michael.d.kinney@intel.com>; Gao, Liming <gaoliming@byosoft.com.cn>;
>>> Wang, Jian J <jian.j.wang@intel.com>
>>> Subject: Re: [edk2-devel] [PATCH v3 00/22] Add Raw algorithm support using
>>> Arm FW-TRNG interface
>>>
>>> Hi Jiewen,
>>>
>>> On 7/20/22 18:44, Yao, Jiewen wrote:
>>>> Hey
>>>> This patch add dependency that SecurityPkg will depend on ArmPkg.
>>>>
>>>> I am not sure it is good idea. As alternative, why not put all those to ArmPkg?
>>>> Then you don’t need change the package dependency.
>>>
>>> I am not sure I understood what you want to move to the ArmPkg.
>>> Nonetheless, doing the following removes the dependency over the ArmPkg
>>> (and it seems to be the normal way way of removing dependencies):
>>>    + TrngLib|MdePkg/Library/BaseTrngLibNull/BaseTrngLibNull.inf
>>>    - TrngLib|ArmPkg/Library/ArmFwTrngLib/ArmFwTrngLib.inf
>>>    - ArmMonitorLib|ArmPkg/Library/ArmMonitorLib/ArmMonitorLib.inf
>>>    - ArmSmcLib|ArmPkg/Library/ArmSmcLib/ArmSmcLib.inf
>>>    - ArmHvcLib|ArmPkg/Library/ArmHvcLib/ArmHvcLib.inf
>>>
>>> The only other dependecy over the ArmPkg is the ArmHasRngExt() call in
>>>     [PATCH v3 20/21] SecurityPkg/RngDxe: Add Arm support of RngDxe
>>> but this can be easily replaced.
>>>
>>> Would it be ok ?
>>>
>>> Regards,
>>> Pierre
>>>
>>>
>>>>
>>>> Thank you
>>>> Yao Jiewen
>>>>
>>>>> -----Original Message-----
>>>>> From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of
>>>>> PierreGondois
>>>>> Sent: Wednesday, July 20, 2022 10:59 PM
>>>>> To: devel@edk2.groups.io
>>>>> Cc: Sami Mujawar <sami.mujawar@arm.com>; Leif Lindholm
>>>>> <quic_llindhol@quicinc.com>; Ard Biesheuvel
>> <ardb+tianocore@kernel.org>;
>>>>> Rebecca Cran <rebecca@bsdio.com>; Kinney, Michael D
>>>>> <michael.d.kinney@intel.com>; Gao, Liming <gaoliming@byosoft.com.cn>;
>>> Yao,
>>>>> Jiewen <jiewen.yao@intel.com>; Wang, Jian J <jian.j.wang@intel.com>
>>>>> Subject: Re: [edk2-devel] [PATCH v3 00/22] Add Raw algorithm support
>> using
>>>>> Arm FW-TRNG interface
>>>>>
>>>>> Hi,
>>>>> I know this patch-set might be long to review, but I wanted to ask
>>>>> if somebody already had some comments,
>>>>>
>>>>> Note the following patch-sets are depending on this patch-set:
>>>>> - [PATCH v1 0/7] Add AesLib and ArmAesLib
>>>>>       https://edk2.groups.io/g/devel/message/90878
>>>>> - [PATCH RESEND v1 0/9] Add DrbgLib
>>>>>       https://edk2.groups.io/g/devel/message/90898
>>>>>
>>>>> Regards,
>>>>> Pierre
>>>>>
>>>>> On 6/29/22 17:02, PierreGondois via groups.io wrote:
>>>>>> From: Pierre Gondois <pierre.gondois@arm.com>
>>>>>>
>>>>>> Bugzilla: Bug 3668 (https://bugzilla.tianocore.org/show_bug.cgi?id=3668)
>>>>>>
>>>>>> The Arm True Random Number Generator Firmware, Interface 1.0,
>>>>> specification
>>>>>> 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.
>>>>>> This patch-set:
>>>>>> - defines a TRNG library class that provides an interface to access the
>>>>>>      entropy source on a platform.
>>>>>> - implements a TRNG library instance that uses the Arm FW-TRNG
>> interface.
>>>>>> - Adds RawAlgorithm support to RngDxe for Arm architecture using the
>> Arm
>>>>>>      FW-TRNG interface.
>>>>>> - Enables RNG support using FW-TRNG interface for Kvmtool Guest/Virtual
>>>>>>      firmware.
>>>>>>
>>>>>> This patch-set is based on the v2 from Sami Mujawar:
>>>>>> [PATCH v2 0/8] Add Raw algorithm support using Arm FW-TRNG interface
>>>>>> https://edk2.groups.io/g/devel/message/83775
>>>>>>
>>>>>> This patch-set can seen at:
>>>>>> https://github.com/PierreARM/edk2/tree/Arm_Trng_v3
>>>>>>
>>>>>> V3:
>>>>>>     - Address Leif's comment (moving definitions, optimizations, ...)
>>>>>>     - Add ArmMonitorLib to choose Hvc/Smc conduit depending on a Pcd.
>>>>>>     - Re-factor some parts of SecurityPkg/RngDxe/ to ease the addition
>>>>>>       of new algorithms.
>>>>>>     - Add ArmHasRngExt() function to check Arm's FEAT_RNG extension.
>>>>>> V2:
>>>>>>     - Updates TrngLib definitions to use RETURN_STATUS as the return type
>>>>>>       from the interface functions as TrngLib is base type library.
>>>>>>     - Drops the patch "MdePkg: Add definition for NULL GUID" as there is
>>>>>>       already an equivalent definition provided by gZeroGuid. Thus, the
>>>>>>       use of gNullGuid has been replaced with gZeroGuid.
>>>>>>
>>>>>> Pierre Gondois (14):
>>>>>>      ArmPkg/ArmMonitorLib: Definition for ArmMonitorLib library class
>>>>>>      ArmPkg/ArmMonitorLib: Add ArmMonitorLib
>>>>>>      ArmPkg/ArmHvcNullLib: Add NULL instance of ArmHvcLib
>>>>>>      MdePkg/BaseRngLib: Rename ArmReadIdIsar0() to ArmGetFeatRng()
>>>>>>      ArmPkg/ArmLib: Add ArmReadIdIsar0() helper
>>>>>>      ArmPkg/ArmLib: Add ArmHasRngExt()
>>>>>>      SecurityPkg: Update Securitypkg.ci.yaml
>>>>>>      SecurityPkg/RngDxe: Replace Pcd with Sp80090Ctr256Guid
>>>>>>      SecurityPkg/RngDxe: Remove ArchGetSupportedRngAlgorithms()
>>>>>>      SecurityPkg/RngDxe: Documentation/include/parameter cleanup
>>>>>>      SecurityPkg/RngDxe: Check before advertising Cpu Rng algo
>>>>>>      SecurityPkg/RngDxe: Add debug warning for NULL
>>>>>>        PcdCpuRngSupportedAlgorithm
>>>>>>      SecurityPkg/RngDxe: Rename AArch64/RngDxe.c
>>>>>>      SecurityPkg/RngDxe: Add Arm support of RngDxe
>>>>>>
>>>>>> Sami Mujawar (8):
>>>>>>      ArmPkg: PCD to select conduit for monitor calls
>>>>>>      MdePkg/TrngLib: Definition for TRNG library class interface
>>>>>>      MdePkg/TrngLib: Add NULL instance of TRNG Library
>>>>>>      ArmPkg: Add FID definitions for Firmware TRNG
>>>>>>      ArmPkg/TrngLib: Add Arm Firmware TRNG library
>>>>>>      SecurityPkg/RngDxe: Rename RdRandGenerateEntropy to generic name
>>>>>>      SecurityPkg/RngDxe: Add AArch64 RawAlgorithm support through
>> TrngLib
>>>>>>      ArmVirtPkg: Kvmtool: Add RNG support using FW-TRNG interface
>>>>>>
>>>>>>     ArmPkg/ArmPkg.dec                             |  12 +-
>>>>>>     ArmPkg/ArmPkg.dsc                             |   3 +
>>>>>>     ArmPkg/Include/IndustryStandard/ArmStdSmc.h   | 109 ++++-
>>>>>>     ArmPkg/Include/Library/ArmLib.h               |  12 +-
>>>>>>     ArmPkg/Include/Library/ArmMonitorLib.h        |  42 ++
>>>>>>     ArmPkg/Library/ArmFwTrngLib/ArmFwTrngDefs.h   |  50 +++
>>>>>>     ArmPkg/Library/ArmFwTrngLib/ArmFwTrngLib.c    | 403
>>>>> ++++++++++++++++++
>>>>>>     ArmPkg/Library/ArmFwTrngLib/ArmFwTrngLib.inf  |  29 ++
>>>>>>     ArmPkg/Library/ArmHvcNullLib/ArmHvcNullLib.c  |  29 ++
>>>>>>     .../Library/ArmHvcNullLib/ArmHvcNullLib.inf   |  22 +
>>>>>>     ArmPkg/Library/ArmLib/AArch64/AArch64Lib.c    |  15 +-
>>>>>>     ArmPkg/Library/ArmLib/AArch64/AArch64Lib.h    |  14 +-
>>>>>>     .../Library/ArmLib/AArch64/AArch64Support.S   |   7 +-
>>>>>>     ArmPkg/Library/ArmLib/Arm/ArmV7Lib.c          |  16 +-
>>>>>>     ArmPkg/Library/ArmMonitorLib/ArmMonitorLib.c  |  34 ++
>>>>>>     .../Library/ArmMonitorLib/ArmMonitorLib.inf   |  29 ++
>>>>>>     ArmVirtPkg/ArmVirtKvmTool.dsc                 |  10 +
>>>>>>     ArmVirtPkg/ArmVirtKvmTool.fdf                 |   5 +
>>>>>>     MdePkg/Include/Library/TrngLib.h              | 121 ++++++
>>>>>>     .../{ArmReadIdIsar0.S => ArmGetFeatRng.S}     |   8 +-
>>>>>>     .../{ArmReadIdIsar0.asm => ArmGetFeatRng.asm} |   8 +-
>>>>>>     MdePkg/Library/BaseRngLib/AArch64/ArmRng.h    |   2 +-
>>>>>>     MdePkg/Library/BaseRngLib/AArch64/Rndr.c      |   2 +-
>>>>>>     MdePkg/Library/BaseRngLib/BaseRngLib.inf      |   4 +-
>>>>>>     .../Library/BaseTrngLibNull/BaseTrngLibNull.c | 135 ++++++
>>>>>>     .../BaseTrngLibNull/BaseTrngLibNull.inf       |  30 ++
>>>>>>     .../BaseTrngLibNull/BaseTrngLibNull.uni       |  12 +
>>>>>>     MdePkg/MdePkg.dec                             |   5 +
>>>>>>     MdePkg/MdePkg.dsc                             |   1 +
>>>>>>     .../RngDxe/{AArch64/RngDxe.c => ArmRngDxe.c}  | 137 +++++-
>>>>>>     .../RandomNumberGenerator/RngDxe/ArmTrng.c    |  71 +++
>>>>>>     .../RngDxe/Rand/RdRand.c                      |  14 +-
>>>>>>     .../RngDxe/Rand/RdRand.h                      |  43 --
>>>>>>     .../RngDxe/Rand/RngDxe.c                      |  36 +-
>>>>>>     .../RandomNumberGenerator/RngDxe/RngDxe.c     |  52 +--
>>>>>>     .../RandomNumberGenerator/RngDxe/RngDxe.inf   |  17 +-
>>>>>>     .../RngDxe/RngDxeInternals.h                  |  44 +-
>>>>>>     SecurityPkg/SecurityPkg.ci.yaml               |   1 +
>>>>>>     SecurityPkg/SecurityPkg.dsc                   |  12 +-
>>>>>>     39 files changed, 1423 insertions(+), 173 deletions(-)
>>>>>>     create mode 100644 ArmPkg/Include/Library/ArmMonitorLib.h
>>>>>>     create mode 100644 ArmPkg/Library/ArmFwTrngLib/ArmFwTrngDefs.h
>>>>>>     create mode 100644 ArmPkg/Library/ArmFwTrngLib/ArmFwTrngLib.c
>>>>>>     create mode 100644 ArmPkg/Library/ArmFwTrngLib/ArmFwTrngLib.inf
>>>>>>     create mode 100644 ArmPkg/Library/ArmHvcNullLib/ArmHvcNullLib.c
>>>>>>     create mode 100644 ArmPkg/Library/ArmHvcNullLib/ArmHvcNullLib.inf
>>>>>>     create mode 100644 ArmPkg/Library/ArmMonitorLib/ArmMonitorLib.c
>>>>>>     create mode 100644 ArmPkg/Library/ArmMonitorLib/ArmMonitorLib.inf
>>>>>>     create mode 100644 MdePkg/Include/Library/TrngLib.h
>>>>>>     rename MdePkg/Library/BaseRngLib/AArch64/{ArmReadIdIsar0.S =>
>>>>> ArmGetFeatRng.S} (78%)
>>>>>>     rename MdePkg/Library/BaseRngLib/AArch64/{ArmReadIdIsar0.asm =>
>>>>> ArmGetFeatRng.asm} (81%)
>>>>>>     create mode 100644
>> MdePkg/Library/BaseTrngLibNull/BaseTrngLibNull.c
>>>>>>     create mode 100644
>> MdePkg/Library/BaseTrngLibNull/BaseTrngLibNull.inf
>>>>>>     create mode 100644
>>> MdePkg/Library/BaseTrngLibNull/BaseTrngLibNull.uni
>>>>>>     rename
>>> SecurityPkg/RandomNumberGenerator/RngDxe/{AArch64/RngDxe.c
>>>>> => ArmRngDxe.c} (51%)
>>>>>>     create mode 100644
>>>>> SecurityPkg/RandomNumberGenerator/RngDxe/ArmTrng.c
>>>>>>     delete mode 100644
>>>>> SecurityPkg/RandomNumberGenerator/RngDxe/Rand/RdRand.h
>>>>>>
>>>>>
>>>>>
>>>>>
>>>>>
>>>>
>>
>>
>> 
>>
>