Re: [PATCH v2 01/11] PcAtChipsetPkg: Add MMIO Support to RTC driver

["André Przywara" <andre.przywara@arm.com>]

On 14/05/2020 09:40, Sami Mujawar wrote:

Hi Sami,

many thanks for your work on that!

> 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.

Is there any chance you could read the MMIO base address from the DT? I
sent a kvmtool patch to add the missing DT node[1].
The compatible string would be "motorola,mc146818", with just the usual
reg property.
Maybe you could fall back to the current 0x70/0x71, if there is no DT node?

For a start, this low address (0x70/0x71) causes issues, so we probably
need to move this permanently.
But also we want to introduce a more flexible memory layout, so devices
can move depending on command line parameters.

It would be great if EDK-2 could cover that from the beginning, so that
we don't end up with compatibility issues.

Cheers,
Andre

[1] https://lists.cs.columbia.edu/pipermail/kvmarm/2020-May/040703.html
> 
> 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.
> 
> When MMIO support is selected (PcdRtcUseMmio == TRUE) the driver
> maps the MMIO region used by the RTC as runtime memory so that the
> RTC registers are accessible post ExitBootServices.
> 
> Signed-off-by: Sami Mujawar <sami.mujawar@arm.com>
> ---
> 
> Notes:
>     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                                          |   8 ++
>  PcAtChipsetPkg/PcatRealTimeClockRuntimeDxe/PcRtc.c                         | 117 ++++++++++++++++--
>  PcAtChipsetPkg/PcatRealTimeClockRuntimeDxe/PcRtc.h                         |  31 +++++
>  PcAtChipsetPkg/PcatRealTimeClockRuntimeDxe/PcRtcEntry.c                    | 130 +++++++++++++++++++-
>  PcAtChipsetPkg/PcatRealTimeClockRuntimeDxe/PcatRealTimeClockRuntimeDxe.inf |   8 ++
>  5 files changed, 280 insertions(+), 14 deletions(-)
> 
> diff --git a/PcAtChipsetPkg/PcAtChipsetPkg.dec b/PcAtChipsetPkg/PcAtChipsetPkg.dec
> index 88de5cceea593176c3a2425a5963b66b789f2b9e..76d0c7eda69bb505914ba904e09c89de170f69ae 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, ARM Limited. All rights reserved.<BR>
>  #
>  # SPDX-License-Identifier: BSD-2-Clause-Patent
>  #
> @@ -142,5 +143,12 @@ [PcdsFixedAtBuild, PcdsPatchableInModule]
>    # @Prompt RTC Update Timeout Value.
>    gPcAtChipsetPkgTokenSpaceGuid.PcdRealTimeClockUpdateTimeout|100000|UINT32|0x00000020
>  
> +  ## 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
> +
>  [UserExtensions.TianoCore."ExtraFiles"]
>    PcAtChipsetPkgExtra.uni
> diff --git a/PcAtChipsetPkg/PcatRealTimeClockRuntimeDxe/PcRtc.c b/PcAtChipsetPkg/PcatRealTimeClockRuntimeDxe/PcRtc.c
> index 52af17941786ef81c3911512ee64551724e67209..df8dea83ab27bbba12351096d1bfd9ea31accb60 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,8 @@ SPDX-License-Identifier: BSD-2-Clause-Patent
>  
>  #include "PcRtc.h"
>  
> +extern EFI_PHYSICAL_ADDRESS   mRtcRegisterBase;
> +
>  //
>  // Days of month.
>  //
> @@ -21,6 +24,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 +79,99 @@ 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 (
> +IoRtcRead (
>    IN  UINT8 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 (
> +IoRtcWrite (
>    IN  UINT8   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
> +MmioRtcRead (
> +  IN  UINT8 Address
> +  )
> +{
> +  MmioWrite8 (
> +    mRtcRegisterBase,
> +    (UINT8)(Address | (UINT8)(MmioRead8 (mRtcRegisterBase) & 0x80))
> +    );
> +  return MmioRead8 (
> +           mRtcRegisterBase + (PcdGet8 (PcdRtcTargetRegister) -
> +             PcdGet8 (PcdRtcIndexRegister))
> +           );
> +}
> +
> +/**
> +  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
> +MmioRtcWrite (
> +  IN  UINT8   Address,
> +  IN  UINT8   Data
> +  )
> +{
> +  MmioWrite8 (
> +    mRtcRegisterBase,
> +    (UINT8)(Address | (UINT8)(MmioRead8 (mRtcRegisterBase) & 0x80))
> +    );
> +  MmioWrite8 (
> +    mRtcRegisterBase + (PcdGet8 (PcdRtcTargetRegister) -
> +      PcdGet8 (PcdRtcIndexRegister)),
> +    Data
> +    );
> +}
> +
> +/**
>    Initialize RTC.
>  
>    @param  Global            For global use inside this module.
> @@ -113,6 +196,18 @@ PcRtcInit (
>    BOOLEAN         Pending;
>  
>    //
> +  // Initialize the RtcRead and RtcWrite functions
> +  // based on the chosen IO/MMIO access.
> +  //
> +  if (FixedPcdGetBool (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..e64dbbea48f7f0d2f317c65c2e4b93e7b1888efc 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  UINT8 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  UINT8   Address,
> +  IN  UINT8   Data
> +  );
> +
>  #endif
> diff --git a/PcAtChipsetPkg/PcatRealTimeClockRuntimeDxe/PcRtcEntry.c b/PcAtChipsetPkg/PcatRealTimeClockRuntimeDxe/PcRtcEntry.c
> index ccda6331373bfe4069b0a59495b5e5cc731c8fc8..5d5dbeaf970ca8eb291c1e094fd764d201f9071e 100644
> --- a/PcAtChipsetPkg/PcatRealTimeClockRuntimeDxe/PcRtcEntry.c
> +++ b/PcAtChipsetPkg/PcatRealTimeClockRuntimeDxe/PcRtcEntry.c
> @@ -2,16 +2,32 @@
>    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;
> +
> +EFI_PHYSICAL_ADDRESS   mRtcRegisterBase;
> +
> +//
> +// 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 +122,33 @@ 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**)&mRtcRegisterBase);
> +
> +  // 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 +168,77 @@ InitializePcRtc (
>    IN EFI_SYSTEM_TABLE                      *SystemTable
>    )
>  {
> -  EFI_STATUS  Status;
> -  EFI_EVENT   Event;
> +  EFI_STATUS             Status;
> +  EFI_EVENT              Event;
> +  EFI_PHYSICAL_ADDRESS   RtcPageBase;
>  
>    EfiInitializeLock (&mModuleGlobal.RtcLock, TPL_CALLBACK);
>    mModuleGlobal.CenturyRtcAddress = GetCenturyRtcAddress ();
>  
> +  if (FixedPcdGetBool (PcdRtcUseMmio)) {
> +    mRtcRegisterBase = PcdGet8 (PcdRtcIndexRegister);
> +    RtcPageBase = mRtcRegisterBase & ~(EFI_PAGE_SIZE - 1);
> +
> +    // Declare the controller as EFI_MEMORY_RUNTIME
> +    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;
> +    }
> +
> +    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
> +        ));
> +      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
> +               );
> +      gDS->RemoveMemorySpace (
> +             RtcPageBase,
> +             EFI_PAGE_SIZE
> +             );
> +      return Status;
> +    }
> +  }
> +
>    Status = PcRtcInit (&mModuleGlobal);
>    ASSERT_EFI_ERROR (Status);
>  
> @@ -165,7 +273,23 @@ InitializePcRtc (
>                    NULL,
>                    NULL
>                    );
> -  ASSERT_EFI_ERROR (Status);
> +  if (EFI_ERROR (Status)) {
> +    ASSERT_EFI_ERROR (Status);
> +    return Status;
> +  }
> +
> +  if (FixedPcdGetBool (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..3a373d11f8bfc7df0e4d00be8b43e90bfa06b192 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, ARM Limited. All rights reserved.<BR>
>  #
>  # SPDX-License-Identifier: BSD-2-Clause-Patent
>  #
> @@ -48,6 +49,7 @@ [LibraryClasses]
>    BaseLib
>    PcdLib
>    ReportStatusCodeLib
> +  DxeServicesTableLib
>  
>  [Protocols]
>    gEfiRealTimeClockArchProtocolGuid             ## PRODUCES
> @@ -61,10 +63,13 @@ [Guids]
>    ## SOMETIMES_CONSUMES ## SystemTable
>    gEfiAcpiTableGuid
>  
> +  gEfiEventVirtualAddressChangeGuid
> +
>  [FixedPcd]
>    gPcAtChipsetPkgTokenSpaceGuid.PcdInitialValueRtcRegisterA     ## CONSUMES
>    gPcAtChipsetPkgTokenSpaceGuid.PcdInitialValueRtcRegisterB     ## CONSUMES
>    gPcAtChipsetPkgTokenSpaceGuid.PcdInitialValueRtcRegisterD     ## CONSUMES
> +  gPcAtChipsetPkgTokenSpaceGuid.PcdRtcUseMmio                   ## CONSUMES
>  
>  [Pcd]
>    gPcAtChipsetPkgTokenSpaceGuid.PcdRealTimeClockUpdateTimeout   ## CONSUMES
> @@ -76,5 +81,8 @@ [Pcd]
>  [Depex]
>    gEfiVariableArchProtocolGuid AND gEfiVariableWriteArchProtocolGuid
>  
> +[Depex.common.DXE_RUNTIME_DRIVER]
> +  gEfiCpuArchProtocolGuid
> +
>  [UserExtensions.TianoCore."ExtraFiles"]
>    PcRtcExtra.uni
>