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

["Laszlo Ersek" <lersek@redhat.com>]

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
>