Re: [PATCH v3 01/15] PcAtChipsetPkg: Add MMIO Support to RTC driver

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

On 6/24/20 3:34 PM, Sami Mujawar wrote:
> Some virtual machine managers like Kvmtool emulate the MC146818
> RTC controller in the MMIO space so that architectures that do
> not support I/O Mapped I/O can use the RTC. This patch adds MMIO
> support to the RTC controller driver.
> 
> The PCD PcdRtcUseMmio has been added to select I/O or MMIO support.
>    If PcdRtcUseMmio is:
>      TRUE  - Indicates the RTC port registers are in MMIO space.
>      FALSE - Indicates the RTC port registers are in I/O space.
>              Default is I/O space.
> 
> Additionally two new PCDs PcdRtcIndexRegister64 and
> PcdRtcTargetRegister64 have been introduced to provide the base
> address for the RTC registers in the MMIO space.
> 
> When MMIO support is selected (PcdRtcUseMmio == TRUE) the driver
> converts the pointers to the RTC MMIO registers so that the
> RTC registers are accessible post ExitBootServices.
> 
> Signed-off-by: Sami Mujawar <sami.mujawar@arm.com>

Can't we implement this without function pointers? What Leif suggested 
was using STATIC helper functions for read and write, where each one has 
a test for the feature PCD to decide whether to use the IO or the MMIO 
version.

Using function pointers is unneeded complexity, especially given the 
fact that we need to fix up their values when the address space is 
virtually remapped by the OS.

> ---
> 
> Notes:
>      v3:
>      - Make PcdRtcUseMmio a feature PCD.                               [Sami]
>      - Read the RTC MMIO base address from the DT.                     [Andre]
>      - Introduce PCDs for RTC Index and Target register base           [Sami]
>        address in the MMIO space.
>      - Move RTC MMIO region mapping code to a separate platform        [Sami]
>        specific library. This library also reads the base addresses
>        for the RTC from DT and configures the RTC Index and Target
>        register PCDs.
>        Ref: https://edk2.groups.io/g/devel/topic/74200905#60307
>      
>      v2:
>      - Code review comments incorporated.                              [Sami]
>      
>      v1:
>      - Add support to read/write from RTC registers using MMIO access  [Sami]
>      - Use wrapper functions for RtcRead/Write accessors               [Leif]
>        Ref: https://edk2.groups.io/g/devel/topic/30915281#30695
> 
>   PcAtChipsetPkg/PcAtChipsetPkg.dec                                          |  16 +++
>   PcAtChipsetPkg/PcatRealTimeClockRuntimeDxe/PcRtc.c                         | 119 +++++++++++++++++---
>   PcAtChipsetPkg/PcatRealTimeClockRuntimeDxe/PcRtc.h                         |  31 +++++
>   PcAtChipsetPkg/PcatRealTimeClockRuntimeDxe/PcRtcEntry.c                    |  72 +++++++++++-
>   PcAtChipsetPkg/PcatRealTimeClockRuntimeDxe/PcatRealTimeClockRuntimeDxe.inf |   8 ++
>   5 files changed, 230 insertions(+), 16 deletions(-)
> 
> diff --git a/PcAtChipsetPkg/PcAtChipsetPkg.dec b/PcAtChipsetPkg/PcAtChipsetPkg.dec
> index 88de5cceea593176c3a2425a5963b66b789f2b9e..ed2d95550b8d153995b30cdc290cf3bb905e211b 100644
> --- a/PcAtChipsetPkg/PcAtChipsetPkg.dec
> +++ b/PcAtChipsetPkg/PcAtChipsetPkg.dec
> @@ -6,6 +6,7 @@
>   #
>   # Copyright (c) 2009 - 2019, Intel Corporation. All rights reserved.<BR>
>   # Copyright (c) 2017, AMD Inc. All rights reserved.<BR>
> +# Copyright (c) 2018 - 2020, ARM Limited. All rights reserved.<BR>
>   #
>   # SPDX-License-Identifier: BSD-2-Clause-Patent
>   #
> @@ -41,6 +42,13 @@ [PcdsFeatureFlag]
>     # @Prompt Configure HPET to use MSI.
>     gPcAtChipsetPkgTokenSpaceGuid.PcdHpetMsiEnable|TRUE|BOOLEAN|0x00001000
>   
> +  ## Indicates the RTC port registers are in MMIO space, or in I/O space.
> +  #  Default is I/O space.<BR><BR>
> +  #   TRUE  - RTC port registers are in MMIO space.<BR>
> +  #   FALSE - RTC port registers are in I/O space.<BR>
> +  # @Prompt RTC port registers use MMIO.
> +  gPcAtChipsetPkgTokenSpaceGuid.PcdRtcUseMmio|FALSE|BOOLEAN|0x00000021
> +
>   [PcdsFixedAtBuild, PcdsDynamic, PcdsDynamicEx, PcdsPatchableInModule]
>     ## This PCD specifies the base address of the HPET timer.
>     # @Prompt HPET base address.
> @@ -68,6 +76,14 @@ [PcdsFixedAtBuild, PcdsDynamic, PcdsDynamicEx, PcdsPatchableInModule]
>     # @Expression 0x80000001 | gPcAtChipsetPkgTokenSpaceGuid.PcdMaximalValidYear < gPcAtChipsetPkgTokenSpaceGuid.PcdMinimalValidYear + 100
>     gPcAtChipsetPkgTokenSpaceGuid.PcdMaximalValidYear|2097|UINT16|0x0000000E
>   
> +  ## Specifies RTC Index Register address in MMIO space.
> +  # @Prompt RTC Index Register address
> +  gPcAtChipsetPkgTokenSpaceGuid.PcdRtcIndexRegister64|0x0|UINT64|0x00000022
> +
> +  ## Specifies RTC Target Register address in MMIO space.
> +  # @Prompt RTC Target Register address
> +  gPcAtChipsetPkgTokenSpaceGuid.PcdRtcTargetRegister64|0x0|UINT64|0x00000023
> +
>   [PcdsFixedAtBuild, PcdsPatchableInModule]
>     ## Defines the ACPI register set base address.
>     #  The invalid 0xFFFF is as its default value. It must be configured to the real value.
> diff --git a/PcAtChipsetPkg/PcatRealTimeClockRuntimeDxe/PcRtc.c b/PcAtChipsetPkg/PcatRealTimeClockRuntimeDxe/PcRtc.c
> index 52af17941786ef81c3911512ee64551724e67209..5ddc06549103ce9944054d3a05b73803f7037ec2 100644
> --- a/PcAtChipsetPkg/PcatRealTimeClockRuntimeDxe/PcRtc.c
> +++ b/PcAtChipsetPkg/PcatRealTimeClockRuntimeDxe/PcRtc.c
> @@ -3,6 +3,7 @@
>   
>   Copyright (c) 2006 - 2018, Intel Corporation. All rights reserved.<BR>
>   Copyright (c) 2017, AMD Inc. All rights reserved.<BR>
> +Copyright (c) 2018 - 2020, ARM Limited. All rights reserved.<BR>
>   
>   SPDX-License-Identifier: BSD-2-Clause-Patent
>   
> @@ -10,6 +11,9 @@ SPDX-License-Identifier: BSD-2-Clause-Patent
>   
>   #include "PcRtc.h"
>   
> +extern UINTN  mRtcIndexRegister;
> +extern UINTN  mRtcTargetRegister;
> +
>   //
>   // Days of month.
>   //
> @@ -21,6 +25,28 @@ UINTN mDayOfMonth[] = { 31, 29, 31, 30, 31, 30, 31, 31, 30, 31, 30, 31 };
>   CHAR16 mTimeZoneVariableName[] = L"RTC";
>   
>   /**
> +  A function pointer that evaluates to a function that reads the RTC content
> +  through its registers either using IO or MMIO access.
> +
> +  @param  Address   Address offset of RTC. It is recommended to use
> +                    macros such as RTC_ADDRESS_SECONDS.
> +
> +  @return The data of UINT8 type read from RTC.
> +**/
> +RTC_READ  RtcRead;
> +
> +/**
> +  A function pointer that evaluates to a function that reads the RTC content
> +  through its registers either using IO or MMIO access.
> +
> +  @param  Address   Address offset of RTC. It is recommended to use
> +                    macros such as RTC_ADDRESS_SECONDS.
> +
> +  @return The data of UINT8 type read from RTC.
> +**/
> +RTC_WRITE RtcWrite;
> +
> +/**
>     Compare the Hour, Minute and Second of the From time and the To time.
>   
>     Only compare H/M/S in EFI_TIME and ignore other fields here.
> @@ -54,41 +80,96 @@ IsWithinOneDay (
>     );
>   
>   /**
> -  Read RTC content through its registers.
> +  Read RTC content through its registers using IO access.
>   
> -  @param  Address  Address offset of RTC. It is recommended to use macros such as
> -                   RTC_ADDRESS_SECONDS.
> +  @param  Address   Address offset of RTC. It is recommended to use
> +                    macros such as RTC_ADDRESS_SECONDS.
>   
>     @return The data of UINT8 type read from RTC.
>   **/
> +STATIC
>   UINT8
> -RtcRead (
> -  IN  UINT8 Address
> +EFIAPI
> +IoRtcRead (
> +  IN  UINTN Address
>     )
>   {
> -  IoWrite8 (PcdGet8 (PcdRtcIndexRegister), (UINT8) (Address | (UINT8) (IoRead8 (PcdGet8 (PcdRtcIndexRegister)) & 0x80)));
> +  IoWrite8 (
> +    PcdGet8 (PcdRtcIndexRegister),
> +    (UINT8)(Address | (UINT8)(IoRead8 (PcdGet8 (PcdRtcIndexRegister)) & 0x80))
> +    );
>     return IoRead8 (PcdGet8 (PcdRtcTargetRegister));
>   }
>   
>   /**
> -  Write RTC through its registers.
> +  Write RTC through its registers  using IO access.
>   
> -  @param  Address  Address offset of RTC. It is recommended to use macros such as
> -                   RTC_ADDRESS_SECONDS.
> -  @param  Data     The content you want to write into RTC.
> +  @param  Address   Address offset of RTC. It is recommended to use
> +                    macros such as RTC_ADDRESS_SECONDS.
> +  @param  Data      The content you want to write into RTC.
>   
>   **/
> +STATIC
>   VOID
> -RtcWrite (
> -  IN  UINT8   Address,
> +EFIAPI
> +IoRtcWrite (
> +  IN  UINTN   Address,
>     IN  UINT8   Data
>     )
>   {
> -  IoWrite8 (PcdGet8 (PcdRtcIndexRegister), (UINT8) (Address | (UINT8) (IoRead8 (PcdGet8 (PcdRtcIndexRegister)) & 0x80)));
> +  IoWrite8 (
> +    PcdGet8 (PcdRtcIndexRegister),
> +    (UINT8)(Address | (UINT8)(IoRead8 (PcdGet8 (PcdRtcIndexRegister)) & 0x80))
> +    );
>     IoWrite8 (PcdGet8 (PcdRtcTargetRegister), Data);
>   }
>   
>   /**
> +  Read RTC content through its registers using MMIO access.
> +
> +  @param  Address   Address offset of RTC. It is recommended to use
> +                    macros such as RTC_ADDRESS_SECONDS.
> +
> +  @return The data of UINT8 type read from RTC.
> +**/
> +STATIC
> +UINT8
> +EFIAPI
> +MmioRtcRead (
> +  IN  UINTN Address
> +  )
> +{
> +  MmioWrite8 (
> +    mRtcIndexRegister,
> +    (UINT8)(Address | (UINT8)(MmioRead8 (mRtcIndexRegister) & 0x80))
> +    );
> +  return MmioRead8 (mRtcTargetRegister);
> +}
> +
> +/**
> +  Write RTC through its registers using MMIO access.
> +
> +  @param  Address   Address offset of RTC. It is recommended to use
> +                    macros such as RTC_ADDRESS_SECONDS.
> +  @param  Data      The content you want to write into RTC.
> +
> +**/
> +STATIC
> +VOID
> +EFIAPI
> +MmioRtcWrite (
> +  IN  UINTN   Address,
> +  IN  UINT8   Data
> +  )
> +{
> +  MmioWrite8 (
> +    mRtcIndexRegister,
> +    (UINT8)(Address | (UINT8)(MmioRead8 (mRtcIndexRegister) & 0x80))
> +    );
> +  MmioWrite8 (mRtcTargetRegister, Data);
> +}
> +
> +/**
>     Initialize RTC.
>   
>     @param  Global            For global use inside this module.
> @@ -113,6 +194,18 @@ PcRtcInit (
>     BOOLEAN         Pending;
>   
>     //
> +  // Initialize the RtcRead and RtcWrite functions
> +  // based on the chosen IO/MMIO access.
> +  //
> +  if (FeaturePcdGet (PcdRtcUseMmio)) {
> +    RtcRead = MmioRtcRead;
> +    RtcWrite = MmioRtcWrite;
> +  } else {
> +    RtcRead = IoRtcRead;
> +    RtcWrite = IoRtcWrite;
> +  }
> +
> +  //
>     // Acquire RTC Lock to make access to RTC atomic
>     //
>     if (!EfiAtRuntime ()) {
> diff --git a/PcAtChipsetPkg/PcatRealTimeClockRuntimeDxe/PcRtc.h b/PcAtChipsetPkg/PcatRealTimeClockRuntimeDxe/PcRtc.h
> index 47293ce44c5a1f4792892892f7da40d7f0a5a001..4074261c2ca5dbc7b0727ea7961154eaf41c0e38 100644
> --- a/PcAtChipsetPkg/PcatRealTimeClockRuntimeDxe/PcRtc.h
> +++ b/PcAtChipsetPkg/PcatRealTimeClockRuntimeDxe/PcRtc.h
> @@ -3,6 +3,7 @@
>   
>   Copyright (c) 2006 - 2018, Intel Corporation. All rights reserved.<BR>
>   Copyright (c) 2017, AMD Inc. All rights reserved.<BR>
> +Copyright (c) 2019 - 2020, ARM Limited. All rights reserved.<BR>
>   
>   SPDX-License-Identifier: BSD-2-Clause-Patent
>   
> @@ -371,4 +372,34 @@ PcRtcAcpiTableChangeCallback (
>     IN EFI_EVENT        Event,
>     IN VOID             *Context
>     );
> +
> +/**
> +  Function pointer to Read RTC content through its registers.
> +
> +  @param  Address   Address offset of RTC. It is recommended to use
> +                    macros such as RTC_ADDRESS_SECONDS.
> +
> +  @return The data of UINT8 type read from RTC.
> +**/
> +typedef
> +UINT8
> +(EFIAPI *RTC_READ) (
> +  IN  UINTN Address
> +  );
> +
> +/**
> +  Function pointer to Write RTC through its registers.
> +
> +  @param  Address   Address offset of RTC. It is recommended to use
> +                    macros such as RTC_ADDRESS_SECONDS.
> +  @param  Data      The content you want to write into RTC.
> +
> +**/
> +typedef
> +VOID
> +(EFIAPI *RTC_WRITE) (
> +  IN  UINTN   Address,
> +  IN  UINT8   Data
> +  );
> +
>   #endif
> diff --git a/PcAtChipsetPkg/PcatRealTimeClockRuntimeDxe/PcRtcEntry.c b/PcAtChipsetPkg/PcatRealTimeClockRuntimeDxe/PcRtcEntry.c
> index ccda6331373bfe4069b0a59495b5e5cc731c8fc8..e30883e53333aae655bbf891f86d1a6ed739715f 100644
> --- a/PcAtChipsetPkg/PcatRealTimeClockRuntimeDxe/PcRtcEntry.c
> +++ b/PcAtChipsetPkg/PcatRealTimeClockRuntimeDxe/PcRtcEntry.c
> @@ -2,16 +2,33 @@
>     Provides Set/Get time operations.
>   
>   Copyright (c) 2006 - 2018, Intel Corporation. All rights reserved.<BR>
> +Copyright (c) 2018 - 2020, ARM Limited. All rights reserved.<BR>
>   SPDX-License-Identifier: BSD-2-Clause-Patent
>   
>   **/
>   
> +#include <Library/DxeServicesTableLib.h>
>   #include "PcRtc.h"
>   
>   PC_RTC_MODULE_GLOBALS  mModuleGlobal;
>   
>   EFI_HANDLE             mHandle = NULL;
>   
> +STATIC EFI_EVENT       mVirtualAddrChangeEvent;
> +
> +UINTN                  mRtcIndexRegister;
> +UINTN                  mRtcTargetRegister;
> +
> +//
> +// Function pointer for the Rtc Read interface function
> +//
> +extern RTC_READ   RtcRead;
> +
> +//
> +// Function pointer for the Rtc Write interface function
> +//
> +extern RTC_WRITE  RtcWrite;
> +
>   /**
>     Returns the current time and date information, and the time-keeping capabilities
>     of the hardware platform.
> @@ -106,6 +123,34 @@ PcRtcEfiSetWakeupTime (
>   }
>   
>   /**
> +  Fixup internal data so that EFI can be called in virtual mode.
> +  Call the passed in Child Notify event and convert any pointers in
> +  lib to virtual mode.
> +
> +  @param[in]    Event   The Event that is being processed
> +  @param[in]    Context Event Context
> +**/
> +VOID
> +EFIAPI
> +LibRtcVirtualNotifyEvent (
> +  IN EFI_EVENT        Event,
> +  IN VOID             *Context
> +  )
> +{
> +  // Only needed if you are going to support the OS calling RTC functions in
> +  // virtual mode. You will need to call EfiConvertPointer (). To convert any
> +  // stored physical addresses to virtual address. After the OS transitions to
> +  // calling in virtual mode, all future runtime calls will be made in virtual
> +  // mode.
> +  EfiConvertPointer (0x0, (VOID**)&mRtcIndexRegister);
> +  EfiConvertPointer (0x0, (VOID**)&mRtcTargetRegister);
> +
> +  // Convert the RtcRead and RtcWrite pointers for runtime use.
> +  EfiConvertPointer (0x0, (VOID**)&RtcRead);
> +  EfiConvertPointer (0x0, (VOID**)&RtcWrite);
> +}
> +
> +/**
>     The user Entry Point for PcRTC module.
>   
>     This is the entry point for PcRTC module. It installs the UEFI runtime service
> @@ -125,12 +170,17 @@ InitializePcRtc (
>     IN EFI_SYSTEM_TABLE                      *SystemTable
>     )
>   {
> -  EFI_STATUS  Status;
> -  EFI_EVENT   Event;
> +  EFI_STATUS             Status;
> +  EFI_EVENT              Event;
>   
>     EfiInitializeLock (&mModuleGlobal.RtcLock, TPL_CALLBACK);
>     mModuleGlobal.CenturyRtcAddress = GetCenturyRtcAddress ();
>   
> +  if (FeaturePcdGet (PcdRtcUseMmio)) {
> +    mRtcIndexRegister = (UINTN)PcdGet64 (PcdRtcIndexRegister64);
> +    mRtcTargetRegister = (UINTN)PcdGet64 (PcdRtcTargetRegister64);
> +  }
> +
>     Status = PcRtcInit (&mModuleGlobal);
>     ASSERT_EFI_ERROR (Status);
>   
> @@ -165,7 +215,23 @@ InitializePcRtc (
>                     NULL,
>                     NULL
>                     );
> -  ASSERT_EFI_ERROR (Status);
> +  if (EFI_ERROR (Status)) {
> +    ASSERT_EFI_ERROR (Status);
> +    return Status;
> +  }
> +
> +  if (FeaturePcdGet (PcdRtcUseMmio)) {
> +    // Register for the virtual address change event
> +    Status = gBS->CreateEventEx (
> +                    EVT_NOTIFY_SIGNAL,
> +                    TPL_NOTIFY,
> +                    LibRtcVirtualNotifyEvent,
> +                    NULL,
> +                    &gEfiEventVirtualAddressChangeGuid,
> +                    &mVirtualAddrChangeEvent
> +                    );
> +    ASSERT_EFI_ERROR (Status);
> +  }
>   
>     return Status;
>   }
> diff --git a/PcAtChipsetPkg/PcatRealTimeClockRuntimeDxe/PcatRealTimeClockRuntimeDxe.inf b/PcAtChipsetPkg/PcatRealTimeClockRuntimeDxe/PcatRealTimeClockRuntimeDxe.inf
> index c73ee98105e510f9e4e23c1a6c1e5c505325d2c9..0d8eca28b65954b073a72fc4fe5ad6247320e79d 100644
> --- a/PcAtChipsetPkg/PcatRealTimeClockRuntimeDxe/PcatRealTimeClockRuntimeDxe.inf
> +++ b/PcAtChipsetPkg/PcatRealTimeClockRuntimeDxe/PcatRealTimeClockRuntimeDxe.inf
> @@ -6,6 +6,7 @@
>   #
>   # Copyright (c) 2006 - 2019, Intel Corporation. All rights reserved.<BR>
>   # Copyright (c) 2017, AMD Inc. All rights reserved.<BR>
> +# Copyright (c) 2018 - 2020, ARM Limited. All rights reserved.<BR>
>   #
>   # SPDX-License-Identifier: BSD-2-Clause-Patent
>   #
> @@ -61,6 +62,11 @@ [Guids]
>     ## SOMETIMES_CONSUMES ## SystemTable
>     gEfiAcpiTableGuid
>   
> +  gEfiEventVirtualAddressChangeGuid
> +
> +[FeaturePcd]
> +  gPcAtChipsetPkgTokenSpaceGuid.PcdRtcUseMmio                   ## CONSUMES
> +
>   [FixedPcd]
>     gPcAtChipsetPkgTokenSpaceGuid.PcdInitialValueRtcRegisterA     ## CONSUMES
>     gPcAtChipsetPkgTokenSpaceGuid.PcdInitialValueRtcRegisterB     ## CONSUMES
> @@ -72,6 +78,8 @@ [Pcd]
>     gPcAtChipsetPkgTokenSpaceGuid.PcdMaximalValidYear             ## CONSUMES
>     gPcAtChipsetPkgTokenSpaceGuid.PcdRtcIndexRegister             ## CONSUMES
>     gPcAtChipsetPkgTokenSpaceGuid.PcdRtcTargetRegister            ## CONSUMES
> +  gPcAtChipsetPkgTokenSpaceGuid.PcdRtcIndexRegister64           ## CONSUMES
> +  gPcAtChipsetPkgTokenSpaceGuid.PcdRtcTargetRegister64          ## CONSUMES
>   
>   [Depex]
>     gEfiVariableArchProtocolGuid AND gEfiVariableWriteArchProtocolGuid
>