[PATCH v3 2/3] efi: qemu: arm64: Add efi_rng_protocol implementation for the platform

[Heinrich Schuchardt]

On 12/28/19 4:03 PM, Sughosh Ganu wrote:
>
> On Sat, 28 Dec 2019 at 20:01, Heinrich Schuchardt <xypron.glpk at gmx.de
> <mailto:xypron.glpk at gmx.de>> wrote:
>
>     On 12/27/19 3:26 PM, Sughosh Ganu wrote:
>      > Add support for the EFI_RNG_PROTOCOL routines for the qemu arm64
>      > platform. EFI_RNG_PROTOCOL is an uefi boottime service which is
>      > invoked by the efi stub in the kernel for getting random seed for
>      > kaslr.
>      >
>      > The routines are platform specific, and use the virtio-rng device on
>      > the platform to get random data.
>      >
>      > The feature can be enabled through the following config
>      > CONFIG_EFI_RNG_PROTOCOL
>      >
>      > Signed-off-by: Sughosh Ganu <sughosh.ganu at linaro.org
>     <mailto:sughosh.ganu at linaro.org>>
>      > ---
>      > Changes since V2:
>      > * Based on review comments from Heinrich Schuchardt, the rng drivers
>      >    read all the bytes requested in the individual
>      >    drivers. Corresponding changes made in getrng routine to
>     remove the
>      >    loop to read the bytes requested, since that would be handled
>     in the
>      >    drivers.
>      >
>      >   board/emulation/qemu-arm/qemu-arm.c | 41 +++++++++++++++++++
>      >   include/efi_rng.h                   | 32 +++++++++++++++
>      >   lib/efi_loader/Kconfig              |  8 ++++
>      >   lib/efi_loader/Makefile             |  1 +
>      >   lib/efi_loader/efi_rng.c            | 80
>     +++++++++++++++++++++++++++++++++++++
>      >   5 files changed, 162 insertions(+)
>      >   create mode 100644 include/efi_rng.h
>      >   create mode 100644 lib/efi_loader/efi_rng.c
>      >
>
>
> <snip>
>
>      > diff --git a/lib/efi_loader/efi_rng.c b/lib/efi_loader/efi_rng.c
>      > new file mode 100644
>      > index 0000000..eb91aa7
>      > --- /dev/null
>      > +++ b/lib/efi_loader/efi_rng.c
>      > @@ -0,0 +1,80 @@
>      > +// SPDX-License-Identifier: GPL-2.0+
>      > +/*
>      > + * Copyright (c) 2019, Linaro Limited
>      > + */
>      > +
>      > +#include <common.h>
>      > +#include <dm.h>
>      > +#include <efi_loader.h>
>      > +#include <efi_rng.h>
>      > +#include <rng.h>
>      > +
>      > +DECLARE_GLOBAL_DATA_PTR;
>      > +
>      > +static efi_status_t EFIAPI rng_getinfo(struct efi_rng_protocol
>     *this,
>      > +                                    efi_uintn_t
>     *rng_algorithm_list_size,
>      > +                                    efi_guid_t *rng_algorithm_list)
>      > +{
>      > +     efi_guid_t rng_algo_guid = EFI_RNG_ALGORITHM_RAW;
>      > +
>      > +     EFI_ENTRY("%p, %p, %p", this, rng_algorithm_list_size,
>      > +               rng_algorithm_list);
>      > +
>      > +     if (!this || !rng_algorithm_list_size)
>      > +             return EFI_INVALID_PARAMETER;
>      > +
>      > +     if (!rng_algorithm_list ||
>      > +         *rng_algorithm_list_size < sizeof(*rng_algorithm_list)) {
>      > +             *rng_algorithm_list_size = sizeof(*rng_algorithm_list);
>      > +             return EFI_BUFFER_TOO_SMALL;
>      > +     }
>      > +
>      > +     /*
>      > +      * For now, use EFI_RNG_ALGORITHM_RAW as the default
>      > +      * algorithm. If a new algorithm gets added in the
>      > +      * future through a Kconfig, rng_algo_guid will be set
>      > +      * based on that Kconfig option
>      > +      */
>      > +     *rng_algorithm_list_size = sizeof(*rng_algorithm_list);
>      > +     guidcpy(rng_algorithm_list, &rng_algo_guid);
>      > +
>      > +     return EFI_EXIT(EFI_SUCCESS);
>      > +}
>      > +
>      > +static efi_status_t EFIAPI getrng(struct efi_rng_protocol *this,
>      > +                               efi_guid_t *rng_algorithm,
>      > +                               efi_uintn_t rng_value_length,
>      > +                               uint8_t *rng_value)
>      > +{
>      > +     int ret;
>      > +     struct udevice *dev;
>      > +     const efi_guid_t rng_raw_guid = EFI_RNG_ALGORITHM_RAW;
>      > +
>      > +     EFI_ENTRY("%p, %p, %zu, %p", this, rng_algorithm,
>     rng_value_length,
>      > +               rng_value);
>      > +
>      > +     if (!this || !rng_value || !rng_value_length)
>      > +             return EFI_INVALID_PARAMETER;
>      > +
>      > +     if (rng_algorithm) {
>      > +             if (guidcmp(rng_algorithm, &rng_raw_guid))
>      > +                     return EFI_UNSUPPORTED;
>      > +     }
>      > +
>      > +     ret = platform_get_rng_device(&dev);
>
>     This does not compile for sandbox_defconfig.
>
>     You could replace this by:
>
>     ret = uclass_get_device(UCLASS_RNG, 0, &dev);
>
>
> Like I had stated in one of my earlier mail, I would prefer having a
> platform specific routine for fetching the rng device. For example, on
> qemu, where the rng device is the child of a virtio-pci device, the
> above logic would not get the rng device without having previously
> scanned the virtio devices. Instead, i will add a weak function
> platform_get_rng_device, which uses uclass_get_device. Any platform
> which has a different topology, can then define it's own
> platform_get_rng_device function.
>
> -sughosh

For patches series the expectation is that the code compiles when
stopping after any patch in the series.

Adding the uclass_get_device() solution in a weak function is ok for me.

When testing I already experienced that I had to issue the `virtio scan`
command.

Best regards

Heinrich