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

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

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