Re: [PATCH v2] MdePkg: add null version of RngLib

["Wang, Jian J" <jian.j.wang@intel.com>]

Laszlo,

Thanks for the comments. I agree all of them. Since there's still EBC listed
in SUPPORTED_ARCHITECTURES in dsc file, I don't see any reason not to add
it. But likewise, I'll leave the final decision to package maintainers.

Regards,
Jian

> -----Original Message-----
> From: Laszlo Ersek <lersek@redhat.com>
> Sent: Wednesday, November 13, 2019 4:27 PM
> To: Wang, Jian J <jian.j.wang@intel.com>; devel@edk2.groups.io
> Cc: Kinney, Michael D <michael.d.kinney@intel.com>; Gao, Liming
> <liming.gao@intel.com>; Ard Biesheuvel <ard.biesheuvel@linaro.org>; Ni, Ray
> <ray.ni@intel.com>
> Subject: Re: [PATCH v2] MdePkg: add null version of RngLib
> 
> Hi Jian,
> 
> thanks for the update. Comments below:
> 
> On 11/13/19 06:35, Jian J Wang wrote:
> >> v2:
> >>  - Change the name from RngLibNull to BaseRngLibNull according to Laszlo's
> >>    comments
> >>  - Move the module from SecurityPkg to MdePkg according to Laszlo's
> comments
> >>  - Update commit message according to Laszlo and Ray's comments
> >
> > This is null version of RngLib which should be used with modules that
> > inherit an (indirect) dependency on the RngLib class, but never actually
> > call RngLib APIs for consuming randomness.
> >
> >
> > Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=1871
> > Cc: Michael D Kinney <michael.d.kinney@intel.com>
> > Cc: Liming Gao <liming.gao@intel.com>
> > Cc: Laszlo Ersek <lersek@redhat.com>
> > Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> > Cc: Ray Ni <ray.ni@intel.com>
> > Signed-off-by: Jian J Wang <jian.j.wang@intel.com>
> > ---
> >  .../Library/BaseRngLibNull/BaseRngLibNull.c   | 95 +++++++++++++++++++
> >  .../Library/BaseRngLibNull/BaseRngLibNull.inf | 31 ++++++
> >  .../Library/BaseRngLibNull/BaseRngLibNull.uni | 14 +++
> >  MdePkg/MdePkg.dsc                             |  1 +
> >  4 files changed, 141 insertions(+)
> >  create mode 100644 MdePkg/Library/BaseRngLibNull/BaseRngLibNull.c
> >  create mode 100644 MdePkg/Library/BaseRngLibNull/BaseRngLibNull.inf
> >  create mode 100644 MdePkg/Library/BaseRngLibNull/BaseRngLibNull.uni
> >
> > diff --git a/MdePkg/Library/BaseRngLibNull/BaseRngLibNull.c
> b/MdePkg/Library/BaseRngLibNull/BaseRngLibNull.c
> > new file mode 100644
> > index 0000000000..13677abc84
> > --- /dev/null
> > +++ b/MdePkg/Library/BaseRngLibNull/BaseRngLibNull.c
> > @@ -0,0 +1,95 @@
> > +/** @file
> > +  Null version of Random number generator services.
> > +
> > +Copyright (c) 2019, Intel Corporation. All rights reserved.<BR>
> > +SPDX-License-Identifier: BSD-2-Clause-Patent
> > +
> > +**/
> > +
> > +#include <Library/BaseLib.h>
> 
> (1) I think we can drop BaseLib.
> 
> > +#include <Library/DebugLib.h>
> > +#include <Library/RngLib.h>
> > +
> > +/**
> > +  Generates a 16-bit random number.
> > +
> > +  if Rand is NULL, then ASSERT().
> > +
> > +  @param[out] Rand     Buffer pointer to store the 16-bit random value.
> > +
> > +  @retval TRUE         Random number generated successfully.
> > +  @retval FALSE        Failed to generate the random number.
> > +
> > +**/
> > +BOOLEAN
> > +EFIAPI
> > +GetRandomNumber16 (
> > +  OUT     UINT16                    *Rand
> > +  )
> > +{
> > +  ASSERT (FALSE);
> > +  return FALSE;
> > +}
> > +
> > +/**
> > +  Generates a 32-bit random number.
> > +
> > +  if Rand is NULL, then ASSERT().
> > +
> > +  @param[out] Rand     Buffer pointer to store the 32-bit random value.
> > +
> > +  @retval TRUE         Random number generated successfully.
> > +  @retval FALSE        Failed to generate the random number.
> > +
> > +**/
> > +BOOLEAN
> > +EFIAPI
> > +GetRandomNumber32 (
> > +  OUT     UINT32                    *Rand
> > +  )
> > +{
> > +  ASSERT (FALSE);
> > +  return FALSE;
> > +}
> > +
> > +/**
> > +  Generates a 64-bit random number.
> > +
> > +  if Rand is NULL, then ASSERT().
> > +
> > +  @param[out] Rand     Buffer pointer to store the 64-bit random value.
> > +
> > +  @retval TRUE         Random number generated successfully.
> > +  @retval FALSE        Failed to generate the random number.
> > +
> > +**/
> > +BOOLEAN
> > +EFIAPI
> > +GetRandomNumber64 (
> > +  OUT     UINT64                    *Rand
> > +  )
> > +{
> > +  ASSERT (FALSE);
> > +  return FALSE;
> > +}
> > +
> > +/**
> > +  Generates a 128-bit random number.
> > +
> > +  if Rand is NULL, then ASSERT().
> > +
> > +  @param[out] Rand     Buffer pointer to store the 128-bit random value.
> > +
> > +  @retval TRUE         Random number generated successfully.
> > +  @retval FALSE        Failed to generate the random number.
> > +
> > +**/
> > +BOOLEAN
> > +EFIAPI
> > +GetRandomNumber128 (
> > +  OUT     UINT64                    *Rand
> > +  )
> > +{
> > +  ASSERT (FALSE);
> > +  return FALSE;
> > +}
> > diff --git a/MdePkg/Library/BaseRngLibNull/BaseRngLibNull.inf
> b/MdePkg/Library/BaseRngLibNull/BaseRngLibNull.inf
> > new file mode 100644
> > index 0000000000..f456df1dae
> > --- /dev/null
> > +++ b/MdePkg/Library/BaseRngLibNull/BaseRngLibNull.inf
> > @@ -0,0 +1,31 @@
> > +## @file
> > +#  Null instance of RNG (Random Number Generator) Library.
> > +#
> > +#  Copyright (c) 2019, Intel Corporation. All rights reserved.<BR>
> > +#
> > +#  SPDX-License-Identifier: BSD-2-Clause-Patent
> > +#
> > +##
> > +
> > +[Defines]
> > +  INF_VERSION                    = 0x00010005
> 
> (2) When adding a new library instance, I seem to remember that we
> recommend referring to the latest published INF spec version (and using
> a decimal version reference). That would be "1.29" at this point.
> 
> https://edk2-docs.gitbooks.io/edk-ii-inf-specification/content/#edk-ii-module-
> information-inf-file-specification
> 
> I defer to the MdePkg maintainers on this, I just wanted to raise this.
> 
> 
> > +  BASE_NAME                      = BaseRngLibNull
> > +  MODULE_UNI_FILE                = BaseRngLibNull.uni
> > +  FILE_GUID                      = CD8991F8-2061-4084-8C9E-9C6F352DC58D
> > +  MODULE_TYPE                    = BASE
> > +  VERSION_STRING                 = 1.0
> > +  LIBRARY_CLASS                  = RngLib
> > +
> > +#
> > +#  VALID_ARCHITECTURES           = IA32 X64 ARM AARCH64
> > +#
> 
> (3) Again, just another idea that I'd like the MdePkg reviewers to
> comment on:
> 
> I don't see why this library instance would not work with EBC.
> 
> However, I can also imagine we should not add EBC unless we at least
> build-test the lib instance for EBC.
> 
> What is the general approach here?
> 
> (Note that I would never consider adding EBC in OvmfPkg or ArmVirtPkg
> INF files, simply because I don't even know what EBC compilers exist.
> But this is MdePkg, and a Null instance, so EBC *could* make sense.)
> 
> 
> > +
> > +[Sources]
> > +  BaseRngLibNull.c
> > +
> > +[Packages]
> > +  MdePkg/MdePkg.dec
> > +
> > +[LibraryClasses]
> > +  BaseLib
> 
> (4) Same as (1).
> 
> > +  DebugLib
> > diff --git a/MdePkg/Library/BaseRngLibNull/BaseRngLibNull.uni
> b/MdePkg/Library/BaseRngLibNull/BaseRngLibNull.uni
> > new file mode 100644
> > index 0000000000..f32be6a617
> > --- /dev/null
> > +++ b/MdePkg/Library/BaseRngLibNull/BaseRngLibNull.uni
> > @@ -0,0 +1,14 @@
> > +// /** @file
> > +// Null Instance of RNG (Random Number Generator) Library.
> > +//
> > +// Copyright (c) 2019, Intel Corporation. All rights reserved.<BR>
> > +//
> > +// SPDX-License-Identifier: BSD-2-Clause-Patent
> > +//
> > +// **/
> > +
> > +
> > +#string STR_MODULE_ABSTRACT             #language en-US "Null Instance of
> RNG Library"
> > +
> > +#string STR_MODULE_DESCRIPTION          #language en-US "This library
> instance should be used with modules that inherit an (indirect) dependency on
> the RngLib class, but never actually call RngLib APIs for consuming randomness."
> > +
> > diff --git a/MdePkg/MdePkg.dsc b/MdePkg/MdePkg.dsc
> > index c1278e7907..0aeafaaacc 100644
> > --- a/MdePkg/MdePkg.dsc
> > +++ b/MdePkg/MdePkg.dsc
> > @@ -61,6 +61,7 @@
> >
> MdePkg/Library/BaseUefiDecompressLib/BaseUefiTianoCustomDecompressLib.
> inf
> >    MdePkg/Library/BaseSmbusLibNull/BaseSmbusLibNull.inf
> >    MdePkg/Library/BaseSafeIntLib/BaseSafeIntLib.inf
> > +  MdePkg/Library/BaseRngLibNull/BaseRngLibNull.inf
> 
> OK, so this actually seems to confirm my question (3). This hunk
> modifies the [Components] section in the DSC file, and not the
> - [Components.IA32, Components.X64],
> - [Components.ARM, Components.AARCH64]
> sections.
> 
> Furthermore, there is even a [Components.EBC] section in the DSC file.
> 
> So I think "EBC" is still considered a viable ARCH in MdePkg, and we're
> actually enabling this lib instance for the EBC arch. Therefore, EBC
> should be listed in VALID_ARCHITECTURES, in the INF File -- as long as
> we care to add a VALID_ARCHITECTURES comment at all, that is.
> 
> If you agree to fix (1) through (4), you can immediately add:
> 
> Reviewed-by: Laszlo Ersek <lersek@redhat.com>
> 
> Thanks!
> Laszlo
> 
> 
> >
> >    MdePkg/Library/DxeCoreEntryPoint/DxeCoreEntryPoint.inf
> >    MdePkg/Library/DxeCoreHobLib/DxeCoreHobLib.inf
> >