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

["Yao, Jiewen" <jiewen.yao@intel.com>]

Hi Pierre
I give the feedback on interface design and dependency, esp SecurityPkg and MdePkg.
1) Please don’t change package dependency policy.
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.

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
> >>>
> >>
> >>
> >> 
> >>
> >