Re: [platforms: PATCH 1/8] Marvell/Armada: Implement EFI_RNG_PROTOCOL driver for EIP76 TRNG

[Marcin Wojtas <mw@semihalf.com>]

2017-10-11 20:15 GMT+02:00 Ard Biesheuvel <ard.biesheuvel@linaro.org>:
> On 11 October 2017 at 17:47, Leif Lindholm <leif.lindholm@linaro.org> wrote:
>> On Wed, Oct 11, 2017 at 05:40:42PM +0200, Marcin Wojtas wrote:
>>> From: Ard Biesheuvel <ard.biesheuvel@linaro.org>
>>>
>>> Add an implementation of EFI_RNG_PROTOCOL so that the OS loader has
>>> access to entropy for KASLR and other purposes (i.e., seeding the OS's
>>> entropy pool very early on).
>>>
>>> Contributed-under: TianoCore Contribution Agreement 1.1
>>> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
>>> Signed-off-by: Marcin Wojtas <mw@semihalf.com>
>>> ---
>>>  Platform/Marvell/Armada/Armada.dsc.inc                                |   4 +
>>>  Platform/Marvell/Armada/Armada70x0.fdf                                |   1 +
>>>  Platform/Marvell/Armada/Drivers/Armada70x0RngDxe/Armada70x0RngDxe.c   | 254 ++++++++++++++++++++
>>>  Platform/Marvell/Armada/Drivers/Armada70x0RngDxe/Armada70x0RngDxe.inf |  47 ++++
>>>  Platform/Marvell/Marvell.dec                                          |   3 +
>>>  5 files changed, 309 insertions(+)
>>>
>>> diff --git a/Platform/Marvell/Armada/Armada.dsc.inc b/Platform/Marvell/Armada/Armada.dsc.inc
>>> index 1aa485c..ec24d76 100644
>>> --- a/Platform/Marvell/Armada/Armada.dsc.inc
>>> +++ b/Platform/Marvell/Armada/Armada.dsc.inc
>>> @@ -364,6 +364,9 @@
>>>    gArmTokenSpaceGuid.PcdSystemMemorySize|0x40000000
>>>    gArmTokenSpaceGuid.PcdArmScr|0x531
>>>
>>> +  # TRNG
>>> +  gMarvellTokenSpaceGuid.PcdEip76TrngBaseAddress|0xF2760000
>>> +
>>>  ################################################################################
>>>  #
>>>  # Components Section - list of all EDK II Modules needed by this Platform
>>> @@ -400,6 +403,7 @@
>>>    Platform/Marvell/Drivers/I2c/Devices/MvEeprom/MvEeprom.inf
>>>    Platform/Marvell/Drivers/Spi/MvSpiDxe.inf
>>>    Platform/Marvell/Drivers/Spi/Devices/MvSpiFlash.inf
>>> +  Platform/Marvell/Armada/Drivers/Armada70x0RngDxe/Armada70x0RngDxe.inf
>>>
>>>    # Network support
>>>    MdeModulePkg/Universal/Network/SnpDxe/SnpDxe.inf
>>> diff --git a/Platform/Marvell/Armada/Armada70x0.fdf b/Platform/Marvell/Armada/Armada70x0.fdf
>>> index 933c3ed..a94a9ff 100644
>>> --- a/Platform/Marvell/Armada/Armada70x0.fdf
>>> +++ b/Platform/Marvell/Armada/Armada70x0.fdf
>>> @@ -113,6 +113,7 @@ FvNameGuid         = 5eda4200-2c5f-43cb-9da3-0baf74b1b30c
>>>    INF Platform/Marvell/Drivers/I2c/Devices/MvEeprom/MvEeprom.inf
>>>    INF Platform/Marvell/Drivers/Spi/MvSpiDxe.inf
>>>    INF Platform/Marvell/Drivers/Spi/Devices/MvSpiFlash.inf
>>> +  INF Platform/Marvell/Armada/Drivers/Armada70x0RngDxe/Armada70x0RngDxe.inf
>>>
>>>    # Network support
>>>    INF MdeModulePkg/Universal/Network/SnpDxe/SnpDxe.inf
>>> diff --git a/Platform/Marvell/Armada/Drivers/Armada70x0RngDxe/Armada70x0RngDxe.c b/Platform/Marvell/Armada/Drivers/Armada70x0RngDxe/Armada70x0RngDxe.c
>>> new file mode 100644
>>> index 0000000..dca6dcc
>>> --- /dev/null
>>> +++ b/Platform/Marvell/Armada/Drivers/Armada70x0RngDxe/Armada70x0RngDxe.c
>>> @@ -0,0 +1,254 @@
>>> +/** @file
>>> +
>>> +  This driver produces an EFI_RNG_PROTOCOL instance for the Armada 70x0 TRNG
>>> +
>>> +  Copyright (C) 2017, Linaro Ltd. All rights reserved.<BR>
>>> +
>>> +  This program and the accompanying materials are licensed and made available
>>> +  under the terms and conditions of the BSD License which accompanies this
>>> +  distribution. The full text of the license may be found at
>>> +  http://opensource.org/licenses/bsd-license.php
>>> +
>>> +  THE PROGRAM IS DISTRIBUTED UNDER THE BSD LICENSE ON AN "AS IS" BASIS, WITHOUT
>>> +  WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER EXPRESS OR IMPLIED.
>>> +
>>> +**/
>>> +
>>> +#include <Library/BaseMemoryLib.h>
>>> +#include <Library/IoLib.h>
>>> +#include <Library/PcdLib.h>
>>> +#include <Library/UefiBootServicesTableLib.h>
>>> +
>>> +#include <Protocol/Rng.h>
>>> +
>>> +#define TRNG_OUTPUT_REG                         mTrngBaseAddress
>>> +#define TRNG_OUTPUT_SIZE                        0x10
>>> +
>>> +#define TRNG_STATUS_REG                         (mTrngBaseAddress + 0x10)
>>> +#define TRNG_STATUS_READY                       BIT0
>>> +
>>> +#define TRNG_INTACK_REG                         (mTrngBaseAddress + 0x10)
>>> +#define TRNG_INTACK_READY                       BIT0
>>> +
>>> +#define TRNG_CONTROL_REG                        (mTrngBaseAddress + 0x14)
>>> +#define TRNG_CONTROL_REG_ENABLE                 BIT10
>>> +
>>> +#define TRNG_CONFIG_REG                         (mTrngBaseAddress + 0x18)
>>> +#define __MIN_REFILL_SHIFT                      0
>>> +#define __MAX_REFILL_SHIFT                      16
>>> +#define TRNG_CONFIG_MIN_REFILL_CYCLES           (0x05 << __MIN_REFILL_SHIFT)
>>> +#define TRNG_CONFIG_MAX_REFILL_CYCLES           (0x22 << __MAX_REFILL_SHIFT)
>>> +
>>> +#define TRNG_FRODETUNE_REG                      (mTrngBaseAddress + 0x24)
>>> +#define TRNG_FRODETUNE_MASK                     0x0
>>> +
>>> +#define TRNG_FROENABLE_REG                      (mTrngBaseAddress + 0x20)
>>> +#define TRNG_FROENABLE_MASK                     0xffffff
>>> +
>>> +#define TRNG_MAX_RETRIES                        20
>>> +
>>> +STATIC EFI_PHYSICAL_ADDRESS                     mTrngBaseAddress;
>>> +
>>> +/**
>>> +  Returns information about the random number generation implementation.
>>> +
>>> +  @param[in]     This                 A pointer to the EFI_RNG_PROTOCOL
>>> +                                      instance.
>>> +  @param[in,out] RNGAlgorithmListSize On input, the size in bytes of
>>> +                                      RNGAlgorithmList.
>>> +                                      On output with a return code of
>>> +                                      EFI_SUCCESS, the size in bytes of the
>>> +                                      data returned in RNGAlgorithmList. On
>>> +                                      output with a return code of
>>> +                                      EFI_BUFFER_TOO_SMALL, the size of
>>> +                                      RNGAlgorithmList required to obtain the
>>> +                                      list.
>>> +  @param[out] RNGAlgorithmList        A caller-allocated memory buffer filled
>>> +                                      by the driver with one EFI_RNG_ALGORITHM
>>> +                                      element for each supported RNG algorithm.
>>> +                                      The list must not change across multiple
>>> +                                      calls to the same driver. The first
>>> +                                      algorithm in the list is the default
>>> +                                      algorithm for the driver.
>>> +
>>> +  @retval EFI_SUCCESS                 The RNG algorithm list was returned
>>> +                                      successfully.
>>> +  @retval EFI_UNSUPPORTED             The services is not supported by this
>>> +                                      driver.
>>> +  @retval EFI_DEVICE_ERROR            The list of algorithms could not be
>>> +                                      retrieved due to a hardware or firmware
>>> +                                      error.
>>> +  @retval EFI_INVALID_PARAMETER       One or more of the parameters are
>>> +                                      incorrect.
>>> +  @retval EFI_BUFFER_TOO_SMALL        The buffer RNGAlgorithmList is too small
>>> +                                      to hold the result.
>>> +
>>> +**/
>>> +STATIC
>>> +EFI_STATUS
>>> +EFIAPI
>>> +Armada70x0RngGetInfo (
>>> +  IN      EFI_RNG_PROTOCOL        *This,
>>> +  IN OUT  UINTN                   *RNGAlgorithmListSize,
>>> +  OUT     EFI_RNG_ALGORITHM       *RNGAlgorithmList
>>> +  )
>>> +{
>>> +  if (This == NULL || RNGAlgorithmListSize == NULL) {
>>> +    return EFI_INVALID_PARAMETER;
>>> +  }
>>> +
>>> +  if (*RNGAlgorithmListSize < sizeof (EFI_RNG_ALGORITHM)) {
>>> +    *RNGAlgorithmListSize = sizeof (EFI_RNG_ALGORITHM);
>>> +    return EFI_BUFFER_TOO_SMALL;
>>> +  }
>>> +
>>> +  if (RNGAlgorithmList == NULL) {
>>> +    return EFI_INVALID_PARAMETER;
>>> +  }
>>> +
>>> +  *RNGAlgorithmListSize = sizeof (EFI_RNG_ALGORITHM);
>>> +  CopyGuid (RNGAlgorithmList, &gEfiRngAlgorithmRaw);
>>> +
>>> +  return EFI_SUCCESS;
>>> +}
>>> +
>>> +STATIC
>>> +EFI_STATUS
>>> +GetTrngData (
>>> +  IN    UINTN   Length,
>>> +  OUT   UINT8   *Bits
>>> +  )
>>> +{
>>> +  UINTN   Tries;
>>> +  UINT32  Buf[TRNG_OUTPUT_SIZE / sizeof (UINT32)];
>>> +  UINTN   Index;
>>> +
>>> +  for (Tries = 0; Tries < TRNG_MAX_RETRIES; Tries++) {
>>> +    if (MmioRead32 (TRNG_STATUS_REG) & TRNG_STATUS_READY) {
>>> +      for (Index = 0; Index < ARRAY_SIZE (Buf); Index++) {
>>> +        Buf[Index] = MmioRead32 (TRNG_OUTPUT_REG + Index * sizeof (UINT32));
>>> +      }
>>> +      CopyMem (Bits, Buf, Length);
>>> +      MmioWrite32 (TRNG_INTACK_REG, TRNG_INTACK_READY);
>>> +
>>> +      return EFI_SUCCESS;
>>> +    }
>>> +    gBS->Stall (10);
>>
>> Why? Why 10? Please add a comment.
>>
>> No other comments on this patch.
>>
>
> I don't remember the details, and I think the 1 ms delay was chosen
> arbitrarily, but the purpose of the delay is to allow the hardware to
> assume the 'ready' state.
>

I don't have EIP150 docs, but how about a comment:

// Polling interval for obtaining TRNG data is 10us
gBS->Stall (10);

I know it's not perfect, but would you accept it?:)

Best regards,
Marcin