Re: [PATCH v3 02/15] ArmVirtPkg: Add Kvmtool RTC Fdt Client Library

["Ard Biesheuvel" <ard.biesheuvel@arm.com>]

On 6/24/20 3:34 PM, Sami Mujawar wrote:
> Add library that parses the Kvmtool device tree and updates
> the dynamic PCDs describing the RTC Memory map.
> 
> It also maps the MMIO region used by the RTC as runtime memory
> so that the RTC registers are accessible post ExitBootServices.
> 
> Since UEFI takes ownership of the RTC hardware disable the RTC
> node in the DT to prevent the OS from attaching its device
> driver as well.
> 
> Signed-off-by: Sami Mujawar <sami.mujawar@arm.com>
> ---
> 
> Notes:
>      v3:
>        - Introduce library to read and map the MMIO base addresses     [Sami]
>          for the RTC registers from the DT and configure the Index
>          and Target register PCDs.
> 
>   ArmVirtPkg/Library/KvmtoolRtcFdtClientLib/KvmtoolRtcFdtClientLib.c   | 255 ++++++++++++++++++++
>   ArmVirtPkg/Library/KvmtoolRtcFdtClientLib/KvmtoolRtcFdtClientLib.inf |  42 ++++
>   2 files changed, 297 insertions(+)
> 
> diff --git a/ArmVirtPkg/Library/KvmtoolRtcFdtClientLib/KvmtoolRtcFdtClientLib.c b/ArmVirtPkg/Library/KvmtoolRtcFdtClientLib/KvmtoolRtcFdtClientLib.c
> new file mode 100644
> index 0000000000000000000000000000000000000000..2f38923187c1a621578413be9c39fa425bde2ed1
> --- /dev/null
> +++ b/ArmVirtPkg/Library/KvmtoolRtcFdtClientLib/KvmtoolRtcFdtClientLib.c
> @@ -0,0 +1,255 @@
> +/** @file
> +  FDT client library for motorola,mc146818 RTC driver
> +
> +  Copyright (c) 2020, ARM Limited. All rights reserved.<BR>
> +
> +  SPDX-License-Identifier: BSD-2-Clause-Patent
> +
> +**/
> +
> +#include <PiDxe.h>
> +#include <Library/BaseLib.h>
> +#include <Library/BaseMemoryLib.h>
> +#include <Library/DebugLib.h>
> +#include <Library/DxeServicesTableLib.h>
> +#include <Library/PcdLib.h>
> +#include <Library/UefiBootServicesTableLib.h>
> +#include <Library/UefiDriverEntryPoint.h>
> +#include <Library/UefiLib.h>
> +#include <Protocol/FdtClient.h>
> +#include <Uefi.h>
> +

This list of #includes seems a bit long and is out of sync with your 
[LibraryClasses] INF section. Could you clean that up please?

> +/** RTC Index register is at offset 0x0
> +*/
> +#define RTC_INDEX_REG_OFFSET    0x0ULL
> +
> +/** RTC Target register is at offset 0x1
> +*/
> +#define RTC_TARGET_REG_OFFSET   0x1ULL
> +
> +/** Add the RTC controller address range to the memory map.
> +
> +  @param [in]  ImageHandle  The handle to the image.
> +  @param [in]  RtcPageBase  Base address of the RTC controller.
> +
> +  @retval EFI_SUCCESS             Success.
> +  @retval EFI_INVALID_PARAMETER   A parameter is invalid.
> +  @retval EFI_NOT_FOUND           Flash device not found.
> +**/
> +EFI_STATUS

STATIC?

> +EFIAPI

You can drop EFIAPI then as well

> +KvmtoolRtcMapMemory (
> +  IN EFI_HANDLE               ImageHandle,
> +  IN EFI_PHYSICAL_ADDRESS     RtcPageBase
> +  )
> +{
> +  EFI_STATUS                        Status;
> +  EFI_GCD_MEMORY_SPACE_DESCRIPTOR   Descriptor;
> +  BOOLEAN                           MemorySpaceAdded;
> +
> +  MemorySpaceAdded = FALSE;
> +
> +  Status = gDS->GetMemorySpaceDescriptor (RtcPageBase, &Descriptor);
> +  if (EFI_ERROR (Status)) {
> +    DEBUG ((
> +      DEBUG_ERROR, "Failed to get memory space descriptor. Status = %r\n",
> +      Status
> +      ));
> +    return Status;
> +  }
> +
> +  if (Descriptor.GcdMemoryType == EfiGcdMemoryTypeNonExistent) {

Why you need this test? Are there cases where the GCD region already exists?

> +    Status = gDS->AddMemorySpace (
> +                    EfiGcdMemoryTypeMemoryMappedIo,
> +                    RtcPageBase,
> +                    EFI_PAGE_SIZE,
> +                    EFI_MEMORY_UC | EFI_MEMORY_RUNTIME
> +                    );
> +    if (EFI_ERROR (Status)) {
> +      DEBUG ((
> +        DEBUG_ERROR, "Failed to add memory space. Status = %r\n",
> +        Status
> +        ));
> +      return Status;
> +    }
> +    MemorySpaceAdded = TRUE;
> +  }
> +
> +  Status = gDS->AllocateMemorySpace (
> +                  EfiGcdAllocateAddress,
> +                  EfiGcdMemoryTypeMemoryMappedIo,
> +                  0,
> +                  EFI_PAGE_SIZE,
> +                  &RtcPageBase,
> +                  ImageHandle,
> +                  NULL
> +                  );
> +  if (EFI_ERROR (Status)) {
> +    DEBUG ((
> +      DEBUG_ERROR,
> +      "Failed to allocate memory space. Status = %r\n",
> +      Status
> +      ));
> +    if (MemorySpaceAdded) {
> +      gDS->RemoveMemorySpace (
> +             RtcPageBase,
> +             EFI_PAGE_SIZE
> +             );
> +    }
> +    return Status;
> +  }
> +
> +  Status = gDS->SetMemorySpaceAttributes (
> +                  RtcPageBase,
> +                  EFI_PAGE_SIZE,
> +                  EFI_MEMORY_UC | EFI_MEMORY_RUNTIME
> +                  );
> +  if (EFI_ERROR (Status)) {
> +    DEBUG ((
> +      DEBUG_ERROR,
> +      "Failed to set memory attributes. Status = %r\n",
> +      Status
> +      ));
> +    gDS->FreeMemorySpace (
> +           RtcPageBase,
> +           EFI_PAGE_SIZE
> +           );
> +    if (MemorySpaceAdded) {
> +      gDS->RemoveMemorySpace (
> +             RtcPageBase,
> +             EFI_PAGE_SIZE
> +             );
> +    }
> +  }
> +
> +  return Status;
> +}
> +
> +/** Entrypoint for KvmtoolRtcFdtClientLib.
> +
> +  Locate the RTC node in the DT and update the Index and
> +  Target register base addresses in the respective PCDs.
> +  Add the RTC memory region to the memory map.
> +  Disable the RTC node as the RTC is owned by UEFI.
> +
> +  @param [in]  ImageHandle  The handle to the image.
> +  @param [in]  SystemTable  Pointer to the System Table.
> +
> +  @retval EFI_SUCCESS             Success.
> +  @retval EFI_INVALID_PARAMETER   A parameter is invalid.
> +  @retval EFI_NOT_FOUND           Flash device not found.
> +**/
> +EFI_STATUS
> +EFIAPI
> +KvmtoolRtcFdtClientLibConstructor (
> +  IN EFI_HANDLE         ImageHandle,
> +  IN EFI_SYSTEM_TABLE   *SystemTable
> +  )
> +{
> +  EFI_STATUS                        Status;
> +  FDT_CLIENT_PROTOCOL               *FdtClient;
> +  INT32                             Node;
> +  CONST UINT64                      *Reg;
> +  UINT32                            RegSize;
> +  UINT64                            RegBase;
> +  UINT64                            Range;
> +  RETURN_STATUS                     PcdStatus;
> +
> +  Status = gBS->LocateProtocol (
> +                  &gFdtClientProtocolGuid,
> +                  NULL,
> +                  (VOID **)&FdtClient
> +                  );
> +  ASSERT_EFI_ERROR (Status);
> +
> +  Status = FdtClient->FindCompatibleNode (
> +                        FdtClient,
> +                        "motorola,mc146818",
> +                        &Node
> +                        );
> +  if (EFI_ERROR (Status)) {
> +    DEBUG ((
> +      DEBUG_ERROR,
> +      "%a: No 'motorola,mc146818' compatible DT node found\n",
> +      __FUNCTION__
> +      ));
> +    return Status;
> +  }
> +
> +  Status = FdtClient->GetNodeProperty (
> +                        FdtClient,
> +                        Node,
> +                        "reg",
> +                        (CONST VOID **)&Reg,
> +                        &RegSize
> +                        );
> +  if (EFI_ERROR (Status)) {
> +    DEBUG ((
> +      DEBUG_ERROR,
> +      "%a: No 'reg' property found in 'motorola,mc146818' compatible DT node\n",
> +      __FUNCTION__
> +      ));
> +    return Status;
> +  }
> +
> +  ASSERT (RegSize == 16);
> +
> +  RegBase = SwapBytes64 (Reg[0]);
> +  Range = SwapBytes64 (Reg[1]);

Better to use ReadUnaligned64 here, and make Reg a UINT32[] or a VOID *, 
as it may not be 64-bit aligned.

> +  DEBUG ((
> +    DEBUG_INFO,
> +    "Found motorola,mc146818 RTC @ 0x%Lx Range = 0x%x\n",
> +    RegBase,
> +    Range
> +    ));
> +
> +  // The address range must cover the RTC Index and the Target registers.
> +  ASSERT (Range >= 0x2);
> +
> +  // RTC Index register is at offset 0x0
> +  PcdStatus = PcdSet64S (
> +                PcdRtcIndexRegister64,
> +                (RegBase + RTC_INDEX_REG_OFFSET)
> +                );
> +  ASSERT_RETURN_ERROR (PcdStatus);
> +
> +  // RTC Target register is at offset 0x1
> +  PcdStatus = PcdSet64S (
> +                PcdRtcTargetRegister64,
> +                (RegBase + RTC_TARGET_REG_OFFSET)
> +                );
> +  ASSERT_RETURN_ERROR (PcdStatus);
> +
> +  Status = KvmtoolRtcMapMemory (ImageHandle, (RegBase & ~(EFI_PAGE_SIZE - 1)));

Can you use EFI_PAGE_MASK here?

> +  if (EFI_ERROR (Status)) {
> +    DEBUG ((
> +      DEBUG_ERROR,
> +      "Failed to map memory for motorola,mc146818. Status = %r\n",
> +      Status
> +      ));
> +    return Status;
> +  }
> +
> +  //
> +  // UEFI takes ownership of the RTC hardware, and exposes its functionality
> +  // through the UEFI Runtime Services GetTime, SetTime, etc. This means we
> +  // need to disable it in the device tree to prevent the OS from attaching
> +  // its device driver as well.
> +  //
> +  Status = FdtClient->SetNodeProperty (
> +                        FdtClient,
> +                        Node,
> +                        "status",
> +                        "disabled",
> +                        sizeof ("disabled")
> +                        );
> +  if (EFI_ERROR (Status)) {
> +    DEBUG ((
> +      DEBUG_WARN,
> +      "Failed to set motorola,mc146818 status to 'disabled', Status = %r\n",
> +      Status
> +      ));
> +  }
> +
> +  return EFI_SUCCESS;
> +}
> diff --git a/ArmVirtPkg/Library/KvmtoolRtcFdtClientLib/KvmtoolRtcFdtClientLib.inf b/ArmVirtPkg/Library/KvmtoolRtcFdtClientLib/KvmtoolRtcFdtClientLib.inf
> new file mode 100644
> index 0000000000000000000000000000000000000000..0d1b27997ed00d7c1be3578cab4d1d2eba663a90
> --- /dev/null
> +++ b/ArmVirtPkg/Library/KvmtoolRtcFdtClientLib/KvmtoolRtcFdtClientLib.inf
> @@ -0,0 +1,42 @@
> +#/** @file
> +#  FDT client library for motorola,mc146818 RTC driver
> +#
> +#  Copyright (c) 2020, ARM Limited. All rights reserved.<BR>
> +#
> +#  SPDX-License-Identifier: BSD-2-Clause-Patent
> +#
> +#**/
> +
> +[Defines]
> +  INF_VERSION                    = 0x0001001B
> +  BASE_NAME                      = KvmtoolRtcFdtClientLib
> +  FILE_GUID                      = 3254B4F7-30B5-48C6-B06A-D8FF97F3EF95
> +  MODULE_TYPE                    = DXE_DRIVER
> +  VERSION_STRING                 = 1.0
> +  LIBRARY_CLASS                  = KvmtoolRtcFdtClientLib|DXE_DRIVER DXE_RUNTIME_DRIVER
> +  CONSTRUCTOR                    = KvmtoolRtcFdtClientLibConstructor
> +
> +[Sources]
> +  KvmtoolRtcFdtClientLib.c
> +
> +[Packages]
> +  ArmVirtPkg/ArmVirtPkg.dec
> +  MdePkg/MdePkg.dec
> +  PcAtChipsetPkg/PcAtChipsetPkg.dec
> +
> +[LibraryClasses]
> +  BaseLib
> +  DebugLib
> +  PcdLib
> +  UefiBootServicesTableLib
> +  DxeServicesTableLib
> +
> +[Protocols]
> +  gFdtClientProtocolGuid                                ## CONSUMES
> +
> +[Pcd]
> +  gPcAtChipsetPkgTokenSpaceGuid.PcdRtcIndexRegister64
> +  gPcAtChipsetPkgTokenSpaceGuid.PcdRtcTargetRegister64
> +
> +[Depex]
> +  gFdtClientProtocolGuid
>