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.8432.1593084732704640940 for ; Thu, 25 Jun 2020 04:32:12 -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 57B841042; Thu, 25 Jun 2020 04:32:12 -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 CC50F3F73C; Thu, 25 Jun 2020 04:32:10 -0700 (PDT) Subject: Re: [PATCH v3 02/15] ArmVirtPkg: Add Kvmtool RTC Fdt Client Library To: Sami Mujawar , devel@edk2.groups.io Cc: leif@nuviainc.com, lersek@redhat.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-3-sami.mujawar@arm.com> From: "Ard Biesheuvel" Message-ID: <6bd5d2f4-0113-e159-110c-2a3451907162@arm.com> Date: Thu, 25 Jun 2020 13:31:54 +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-3-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: > 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 > --- > > 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.
> + > + SPDX-License-Identifier: BSD-2-Clause-Patent > + > +**/ > + > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include > + 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.
> +# > +# 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 >