From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from foss.arm.com (foss.arm.com [217.140.110.172]) by mx.groups.io with SMTP id smtpd.web11.8332.1593084314197462852 for ; Thu, 25 Jun 2020 04:25:14 -0700 Authentication-Results: mx.groups.io; dkim=missing; spf=pass (domain: arm.com, ip: 217.140.110.172, mailfrom: ard.biesheuvel@arm.com) Received: from usa-sjc-imap-foss1.foss.arm.com (unknown [10.121.207.14]) by usa-sjc-mx-foss1.foss.arm.com (Postfix) with ESMTP id C3DA41FB; Thu, 25 Jun 2020 04:25:13 -0700 (PDT) Received: from [192.168.43.142] (unknown [172.31.20.19]) by usa-sjc-imap-foss1.foss.arm.com (Postfix) with ESMTPSA id 27E713F73C; Thu, 25 Jun 2020 04:25:12 -0700 (PDT) Subject: Re: [PATCH v3 01/15] PcAtChipsetPkg: Add MMIO Support to RTC driver To: Sami Mujawar , devel@edk2.groups.io Cc: leif@nuviainc.com, ray.ni@intel.com, Alexandru.Elisei@arm.com, Andre.Przywara@arm.com, Matteo.Carlini@arm.com, Laura.Moretta@arm.com, nd@arm.com References: <20200624133458.61920-1-sami.mujawar@arm.com> <20200624133458.61920-2-sami.mujawar@arm.com> From: "Ard Biesheuvel" Message-ID: Date: Thu, 25 Jun 2020 13:24:52 +0200 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:68.0) Gecko/20100101 Thunderbird/68.7.0 MIME-Version: 1.0 In-Reply-To: <20200624133458.61920-2-sami.mujawar@arm.com> Content-Type: text/plain; charset=utf-8; format=flowed Content-Language: en-US Content-Transfer-Encoding: 7bit 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 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.
> # Copyright (c) 2017, AMD Inc. All rights reserved.
> +# Copyright (c) 2018 - 2020, ARM Limited. All rights reserved.
> # > # 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.

> + # TRUE - RTC port registers are in MMIO space.
> + # FALSE - RTC port registers are in I/O space.
> + # @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.
> Copyright (c) 2017, AMD Inc. All rights reserved.
> +Copyright (c) 2018 - 2020, ARM Limited. All rights reserved.
> > 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.
> Copyright (c) 2017, AMD Inc. All rights reserved.
> +Copyright (c) 2019 - 2020, ARM Limited. All rights reserved.
> > 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.
> +Copyright (c) 2018 - 2020, ARM Limited. All rights reserved.
> SPDX-License-Identifier: BSD-2-Clause-Patent > > **/ > > +#include > #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.
> # Copyright (c) 2017, AMD Inc. All rights reserved.
> +# Copyright (c) 2018 - 2020, ARM Limited. All rights reserved.
> # > # 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 >