From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from us-smtp-delivery-124.mimecast.com (us-smtp-delivery-124.mimecast.com [216.205.24.124]) by mx.groups.io with SMTP id smtpd.web08.6790.1624537857611762961 for ; Thu, 24 Jun 2021 05:30:58 -0700 Authentication-Results: mx.groups.io; dkim=pass header.i=@redhat.com header.s=mimecast20190719 header.b=ZAH0GJh3; spf=pass (domain: redhat.com, ip: 216.205.24.124, mailfrom: lersek@redhat.com) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1624537856; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:cc:mime-version:mime-version:content-type:content-type: content-transfer-encoding:content-transfer-encoding: in-reply-to:in-reply-to:references:references; bh=SlkwKmJzbiORQBNNp1dl79f83Jvg8YTz2psri4fW6uE=; b=ZAH0GJh3qA59Auj0bDIxO1DSf115OY2ISpQChH/Goo3AM+Cxi5lt/ZHpHiYbZjp8aPdEXp erLRi4ryc0pVBkVo1HyHTy3lUKndPvzE/KCxkmQ0Av8Rch9TiEmv5LkR5MOnmKMMbNwyRl jc/YFvfUH7xd7+uLpxv1fWUsqaxdtd8= Received: from mimecast-mx01.redhat.com (mimecast-mx01.redhat.com [209.132.183.4]) (Using TLS) by relay.mimecast.com with ESMTP id us-mta-435-wly23-wUNk-0IlBm62UaDA-1; Thu, 24 Jun 2021 08:30:53 -0400 X-MC-Unique: wly23-wUNk-0IlBm62UaDA-1 Received: from smtp.corp.redhat.com (int-mx02.intmail.prod.int.phx2.redhat.com [10.5.11.12]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mimecast-mx01.redhat.com (Postfix) with ESMTPS id 9AA91A40C1; Thu, 24 Jun 2021 12:30:51 +0000 (UTC) Received: from lacos-laptop-7.usersys.redhat.com (ovpn-114-106.ams2.redhat.com [10.36.114.106]) by smtp.corp.redhat.com (Postfix) with ESMTPS id 7377160C05; Thu, 24 Jun 2021 12:30:49 +0000 (UTC) Subject: Re: [edk2-devel] [PATCH v1 3/5] ArmVirtPkg: Add Configuration Manager for Kvmtool firmware To: devel@edk2.groups.io, pierre.gondois@arm.com, Sami Mujawar Cc: Ard Biesheuvel , Leif Lindholm , Akanksha Jain , Alexandru Elisei References: <20210623140640.16754-1-Pierre.Gondois@arm.com> <20210623140640.16754-4-Pierre.Gondois@arm.com> From: "Laszlo Ersek" Message-ID: <14795d6e-a784-5aaf-e697-e60464f0dc5e@redhat.com> Date: Thu, 24 Jun 2021 14:30:47 +0200 MIME-Version: 1.0 In-Reply-To: <20210623140640.16754-4-Pierre.Gondois@arm.com> X-Scanned-By: MIMEDefang 2.79 on 10.5.11.12 Authentication-Results: relay.mimecast.com; auth=pass smtp.auth=CUSA124A263 smtp.mailfrom=lersek@redhat.com X-Mimecast-Spam-Score: 0 X-Mimecast-Originator: redhat.com Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 7bit On 06/23/21 16:06, PierreGondois wrote: > From: Sami Mujawar > > Add Configuration Manager to enable ACPI tables for Kvmtool > firmware. The Configuration Manager for Kvmtool uses the DT > Hardware Information Parser module (FdtHwInfoParser) to parse > the DT provided by Kvmtool. The FdtHwInfoParser parses the DT > and invokes the callback function HW_INFO_ADD_OBJECT to add > the Configuration Manager objects to the Platform Information > repository. > > The information for some Configuration Manager objects may not > be available in the DT. Such objects are initialised locally > by the Configuration Manager. > > Support for the following ACPI tables is provided: > - DBG2 > - DSDT (Empty stub) > - FADT > - GTDT > - MADT > - SPCR > - SSDT (Cpu Hierarchy) > - SSDT (Pcie bus) > > Signed-off-by: Sami Mujawar > Signed-off-by: Pierre Gondois > --- > ArmVirtPkg/ArmVirtKvmTool.dsc | 3 + > .../KvmtoolCfgMgrDxe/ConfigurationManager.c | 948 ++++++++++++++++++ > .../KvmtoolCfgMgrDxe/ConfigurationManager.h | 94 ++ > .../ConfigurationManagerDxe.inf | 58 ++ > 4 files changed, 1103 insertions(+) > create mode 100644 ArmVirtPkg/KvmtoolCfgMgrDxe/ConfigurationManager.c > create mode 100644 ArmVirtPkg/KvmtoolCfgMgrDxe/ConfigurationManager.h > create mode 100644 ArmVirtPkg/KvmtoolCfgMgrDxe/ConfigurationManagerDxe.inf > > diff --git a/ArmVirtPkg/ArmVirtKvmTool.dsc b/ArmVirtPkg/ArmVirtKvmTool.dsc > index 3bd1cc72a1eb..920880796ac2 100644 > --- a/ArmVirtPkg/ArmVirtKvmTool.dsc > +++ b/ArmVirtPkg/ArmVirtKvmTool.dsc > @@ -71,6 +71,9 @@ [LibraryClasses.common] > PlatformHookLib|ArmVirtPkg/Library/Fdt16550SerialPortHookLib/Fdt16550SerialPortHookLib.inf > SerialPortLib|MdeModulePkg/Library/BaseSerialPortLib16550/BaseSerialPortLib16550.inf > > + HwInfoParserLib|DynamicTablesPkg/Library/FdtHwInfoParserLib/FdtHwInfoParserLib.inf > + DynamicPlatRepoLib|DynamicTablesPkg/Library/Common/DynamicPlatRepoLib/DynamicPlatRepoLib.inf > + > [LibraryClasses.common.SEC, LibraryClasses.common.PEI_CORE, LibraryClasses.common.PEIM] > PciExpressLib|MdePkg/Library/BasePciExpressLib/BasePciExpressLib.inf > PlatformHookLib|ArmVirtPkg/Library/Fdt16550SerialPortHookLib/EarlyFdt16550SerialPortHookLib.inf (1) Please move at least the line ArmVirtPkg/KvmtoolCfgMgrDxe/ConfigurationManagerDxe.inf from the next patch into this patch. The reason is that without that line in the DSC file, you can't build the new driver, right after this patch is applied, even with the "-m" option of the "build" utility. And if you cannot build the driver, then adding the library class resolutions *here* is useless. ... Alternatively, please move the lib class resolutions to the next patch (and don't touch the DSC file in this patch). > diff --git a/ArmVirtPkg/KvmtoolCfgMgrDxe/ConfigurationManager.c b/ArmVirtPkg/KvmtoolCfgMgrDxe/ConfigurationManager.c > new file mode 100644 > index 000000000000..07b8b403dd4a > --- /dev/null > +++ b/ArmVirtPkg/KvmtoolCfgMgrDxe/ConfigurationManager.c > @@ -0,0 +1,948 @@ > +/** @file > + Configuration Manager Dxe > + > + Copyright (c) 2021, Arm Limited. All rights reserved.
> + > + SPDX-License-Identifier: BSD-2-Clause-Patent > + > + @par Glossary: > + - Cm or CM - Configuration Manager > + - Obj or OBJ - Object > +**/ > + > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include > + > +#include "ConfigurationManager.h" > + > +/** The platform configuration repository information. > +*/ (2) This comment style is only used on functions. For other declarations / definitions, please use // // blah // > +STATIC > +EDKII_PLATFORM_REPOSITORY_INFO KvmtoolPlatRepositoryInfo = { > + /// Configuration Manager information (3) "///" is sometimes used in edk2 indeed, but here it is inconsistent with the other "//" usages (4) please add leading and trailing empty comment lines (elsewhere too) (5) all module global variables must start with a lower-case "m". Such as "mKvmtoolPlatRepositoryInfo". > + { CONFIGURATION_MANAGER_REVISION, CFG_MGR_OEM_ID }, > + > + // ACPI Table List > + { > + // FADT Table > + { > + EFI_ACPI_6_3_FIXED_ACPI_DESCRIPTION_TABLE_SIGNATURE, > + EFI_ACPI_6_3_FIXED_ACPI_DESCRIPTION_TABLE_REVISION, > + CREATE_STD_ACPI_TABLE_GEN_ID (EStdAcpiTableIdFadt), > + NULL > + }, > + // GTDT Table > + { > + EFI_ACPI_6_3_GENERIC_TIMER_DESCRIPTION_TABLE_SIGNATURE, > + EFI_ACPI_6_3_GENERIC_TIMER_DESCRIPTION_TABLE_REVISION, > + CREATE_STD_ACPI_TABLE_GEN_ID (EStdAcpiTableIdGtdt), > + NULL > + }, > + // MADT Table > + { > + EFI_ACPI_6_3_MULTIPLE_APIC_DESCRIPTION_TABLE_SIGNATURE, > + EFI_ACPI_6_3_MULTIPLE_APIC_DESCRIPTION_TABLE_REVISION, > + CREATE_STD_ACPI_TABLE_GEN_ID (EStdAcpiTableIdMadt), > + NULL > + }, > + // SPCR Table > + { > + EFI_ACPI_6_3_SERIAL_PORT_CONSOLE_REDIRECTION_TABLE_SIGNATURE, > + EFI_ACPI_SERIAL_PORT_CONSOLE_REDIRECTION_TABLE_REVISION, > + CREATE_STD_ACPI_TABLE_GEN_ID (EStdAcpiTableIdSpcr), > + NULL > + }, > + // DSDT Table > + { > + EFI_ACPI_6_3_DIFFERENTIATED_SYSTEM_DESCRIPTION_TABLE_SIGNATURE, > + 0, // Unused > + CREATE_STD_ACPI_TABLE_GEN_ID (EStdAcpiTableIdDsdt), > + (EFI_ACPI_DESCRIPTION_HEADER*)dsdt_aml_code > + }, > + // SSDT Cpu Hierarchy Table > + { > + EFI_ACPI_6_3_SECONDARY_SYSTEM_DESCRIPTION_TABLE_SIGNATURE, > + 0, // Unused > + CREATE_STD_ACPI_TABLE_GEN_ID (EStdAcpiTableIdSsdtCpuTopology), > + NULL > + }, > + // DBG2 Table > + { > + EFI_ACPI_6_3_DEBUG_PORT_2_TABLE_SIGNATURE, > + EFI_ACPI_DBG2_DEBUG_DEVICE_INFORMATION_STRUCT_REVISION, > + CREATE_STD_ACPI_TABLE_GEN_ID (EStdAcpiTableIdDbg2), > + NULL > + }, > + // PCI MCFG Table > + { > + EFI_ACPI_6_3_PCI_EXPRESS_MEMORY_MAPPED_CONFIGURATION_SPACE_BASE_ADDRESS_DESCRIPTION_TABLE_SIGNATURE, > + EFI_ACPI_MEMORY_MAPPED_CONFIGURATION_SPACE_ACCESS_TABLE_REVISION, > + CREATE_STD_ACPI_TABLE_GEN_ID (EStdAcpiTableIdMcfg), > + NULL > + }, > + // SSDT table describing the PCI root complex > + { > + EFI_ACPI_6_3_SECONDARY_SYSTEM_DESCRIPTION_TABLE_SIGNATURE, > + 0, // Unused > + CREATE_STD_ACPI_TABLE_GEN_ID (EStdAcpiTableIdSsdtPciExpress), > + NULL > + }, > + // IORT Table > + { > + EFI_ACPI_6_3_IO_REMAPPING_TABLE_SIGNATURE, > + EFI_ACPI_IO_REMAPPING_TABLE_REVISION, > + CREATE_STD_ACPI_TABLE_GEN_ID (EStdAcpiTableIdIort), > + NULL > + }, > + }, > + > + // Power management profile information > + { EFI_ACPI_6_3_PM_PROFILE_ENTERPRISE_SERVER }, // PowerManagement Profile > + > + // ITS group node > + { > + // Reference token for this Iort node > + REFERENCE_TOKEN (ItsGroupInfo), > + // The number of ITS identifiers in the ITS node. > + 1, > + // Reference token for the ITS identifier array > + REFERENCE_TOKEN (ItsIdentifierArray) > + }, > + // ITS identifier array > + { > + { > + // The ITS Identifier > + 0 > + } > + }, > + > + // Root Complex node info > + { > + // Reference token for this Iort node > + REFERENCE_TOKEN (RootComplexInfo), > + // Number of ID mappings > + 1, > + // Reference token for the ID mapping array > + REFERENCE_TOKEN (DeviceIdMapping[0]), > + > + // Memory access properties : Cache coherent attributes > + EFI_ACPI_IORT_MEM_ACCESS_PROP_CCA, > + // Memory access properties : Allocation hints > + 0, > + // Memory access properties : Memory access flags > + 0, > + // ATS attributes > + EFI_ACPI_IORT_ROOT_COMPLEX_ATS_UNSUPPORTED, > + // PCI segment number > + 0 > + }, > + > + // Array of Device ID mappings > + { > + /* RootComplex -> ITS Group > + */ > + // Device ID mapping for Root complex node > + { > + // Input base > + 0x0, > + // Number of input IDs > + 0x0000FFFF, > + // Output Base > + 0x0, > + // Output reference > + REFERENCE_TOKEN (ItsGroupInfo), > + // Flags > + 0 > + }, > + }, > +}; > + > +/** A helper function for returning the Configuration Manager Objects. (6) I think we tend to keep the "/**" comment starter alone on the line, and the actual function description starts at the next line. I suggest updating all the other comment blocks, too. > + > + @param [in] CmObjectId The Configuration Manager Object ID. > + @param [in] Object Pointer to the Object(s). > + @param [in] ObjectSize Total size of the Object(s). > + @param [in] ObjectCount Number of Objects. > + @param [in, out] CmObjectDesc Pointer to the Configuration Manager Object > + descriptor describing the requested Object. > + > + @retval EFI_SUCCESS Success. > +**/ > +STATIC > +EFI_STATUS > +EFIAPI > +HandleCmObject ( > + IN CONST CM_OBJECT_ID CmObjectId, > + IN VOID * Object, > + IN CONST UINTN ObjectSize, > + IN CONST UINTN ObjectCount, > + IN OUT CM_OBJ_DESCRIPTOR * CONST CmObjectDesc > + ) > +{ > + CmObjectDesc->ObjectId = CmObjectId; > + CmObjectDesc->Size = ObjectSize; > + CmObjectDesc->Data = (VOID*)Object; (7) This cast is useless. Please update the rest of the code too, if necessary. > + CmObjectDesc->Count = ObjectCount; > + DEBUG (( > + DEBUG_INFO, > + "INFO: CmObjectId = %x, Ptr = 0x%p, Size = %d, Count = %d\n", > + CmObjectId, > + CmObjectDesc->Data, > + CmObjectDesc->Size, > + CmObjectDesc->Count > + )); (8) Please audit the format specifications in all of your DEBUGs. First, wherever you introduce the CM_OBJECT_ID typedef, please also #define a new format string macro for printing objects of type CM_OBJECT_ID. Then, in DEBUG format strings, please use that typedef, rather than an open-coded %x. "%x" may be proper (I can't tell), but it should be hidden behind a macro. #define FMT_CM_OBJECT_ID "%x" DEBUG ((DEBUG_INFO, "blah " FMT_CM_OBJECT_ID " blah\n", CmObjectId)); Second, *assuming* "CmObjectDesc->Size" has type UINTN, the right way to print it is not with %d, but (a) casting the value to UINT64, (b) printing the converted value with %Lu. Same for "CmObjectDesc->Count". > + return EFI_SUCCESS; > +} > + > +/** A helper function for returning the Configuration Manager Objects that > + match the token. > + > + @param [in] This Pointer to the Configuration Manager Protocol. > + @param [in] CmObjectId The Configuration Manager Object ID. > + @param [in] Object Pointer to the Object(s). > + @param [in] ObjectSize Total size of the Object(s). > + @param [in] ObjectCount Number of Objects. > + @param [in] Token A token identifying the object. > + @param [in] HandlerProc A handler function to search the object > + referenced by the token. > + @param [in, out] CmObjectDesc Pointer to the Configuration Manager Object > + descriptor describing the requested Object. > + > + @retval EFI_SUCCESS Success. > + @retval EFI_INVALID_PARAMETER A parameter is invalid. > + @retval EFI_NOT_FOUND The required object information is not found. > +**/ > +STATIC > +EFI_STATUS > +EFIAPI > +HandleCmObjectRefByToken ( > + IN CONST EDKII_CONFIGURATION_MANAGER_PROTOCOL * CONST This, > + IN CONST CM_OBJECT_ID CmObjectId, > + IN VOID * Object, > + IN CONST UINTN ObjectSize, > + IN CONST UINTN ObjectCount, > + IN CONST CM_OBJECT_TOKEN Token, > + IN CONST CM_OBJECT_HANDLER_PROC HandlerProc, > + IN OUT CM_OBJ_DESCRIPTOR * CONST CmObjectDesc > + ) > +{ > + EFI_STATUS Status; > + CmObjectDesc->ObjectId = CmObjectId; > + if (Token == CM_NULL_TOKEN) { > + CmObjectDesc->Size = ObjectSize; > + CmObjectDesc->Data = (VOID*)Object; > + CmObjectDesc->Count = ObjectCount; > + Status = EFI_SUCCESS; > + } else { > + Status = HandlerProc (This, CmObjectId, Token, CmObjectDesc); > + } > + > + DEBUG (( > + DEBUG_INFO, > + "INFO: Token = 0x%p, CmObjectId = %x, Ptr = 0x%p, Size = %d, Count = %d\n", > + (VOID*)Token, > + CmObjectId, > + CmObjectDesc->Data, > + CmObjectDesc->Size, > + CmObjectDesc->Count > + )); > + return Status; > +} > + > +/** Return an ITS identifier array. > + > + @param [in] This Pointer to the Configuration Manager Protocol. > + @param [in] CmObjectId The Configuration Manager Object ID. > + @param [in] Token A token for identifying the object > + @param [out] CmObject Pointer to the Configuration Manager Object > + descriptor describing the requested Object. > + > + @retval EFI_SUCCESS Success. > + @retval EFI_INVALID_PARAMETER A parameter is invalid. > + @retval EFI_NOT_FOUND The required object information is not found. > +**/ > +EFI_STATUS > +EFIAPI > +GetItsIdentifierArray ( > + IN CONST EDKII_CONFIGURATION_MANAGER_PROTOCOL * CONST This, > + IN CONST CM_OBJECT_ID CmObjectId, > + IN CONST CM_OBJECT_TOKEN Token, > + OUT CM_OBJ_DESCRIPTOR * CONST CmObject > + ) > +{ > + EDKII_PLATFORM_REPOSITORY_INFO * PlatformRepo; > + > + if ((This == NULL) || (CmObject == NULL)) { > + ASSERT (0); (9) should be ASSERT (FALSE) -- please update all instances. Better yet: in GetStandardNameSpaceObject(), you use ASSERT() expressions that make a lot more sense, when logged. I suggest replacing all the ASSERT (0) calls with that (more useful) pattern. > + return EFI_INVALID_PARAMETER; > + } > + > + PlatformRepo = This->PlatRepoInfo; > + > + if (Token != (CM_OBJECT_TOKEN)&PlatformRepo->ItsIdentifierArray) { > + return EFI_NOT_FOUND; > + } > + > + CmObject->ObjectId = CmObjectId; > + CmObject->Size = sizeof (PlatformRepo->ItsIdentifierArray); > + CmObject->Data = (VOID*)&PlatformRepo->ItsIdentifierArray; > + CmObject->Count = ARRAY_SIZE (PlatformRepo->ItsIdentifierArray); > + return EFI_SUCCESS; > +} > + > +/** Return a device Id mapping array. > + > + @param [in] This Pointer to the Configuration Manager Protocol. > + @param [in] CmObjectId The Configuration Manager Object ID. > + @param [in] Token A token for identifying the object > + @param [out] CmObject Pointer to the Configuration Manager Object > + descriptor describing the requested Object. > + > + @retval EFI_SUCCESS Success. > + @retval EFI_INVALID_PARAMETER A parameter is invalid. > + @retval EFI_NOT_FOUND The required object information is not found. > +**/ > +EFI_STATUS > +EFIAPI > +GetDeviceIdMappingArray ( > + IN CONST EDKII_CONFIGURATION_MANAGER_PROTOCOL * CONST This, > + IN CONST CM_OBJECT_ID CmObjectId, > + IN CONST CM_OBJECT_TOKEN Token, > + OUT CM_OBJ_DESCRIPTOR * CONST CmObject > + ) > +{ > + EDKII_PLATFORM_REPOSITORY_INFO * PlatformRepo; > + > + if ((This == NULL) || (CmObject == NULL)) { > + ASSERT (0); > + return EFI_INVALID_PARAMETER; > + } > + > + PlatformRepo = This->PlatRepoInfo; > + > + if (Token != (CM_OBJECT_TOKEN)&PlatformRepo->DeviceIdMapping[0]) { > + return EFI_NOT_FOUND; > + } > + > + CmObject->ObjectId = CmObjectId; > + CmObject->Size = sizeof (CM_ARM_ID_MAPPING); > + CmObject->Data = (VOID*)Token; > + CmObject->Count = 1; > + return EFI_SUCCESS; > +} > + > +/** Function pointer called by the parser to add information. > + > + Callback function that the parser can use to add new > + CmObj. This function must copy the CmObj data and not rely on > + the parser preserving the CmObj memory. > + This function is responsible of the Token allocation. > + > + @param [in] ParserHandle A handle to the parser instance. > + @param [in] Context A pointer to the caller's context provided in > + HwInfoParserInit (). > + @param [in] CmObjDesc CM_OBJ_DESCRIPTOR containing the CmObj(s) to add. > + @param [out] Token If provided and success, contain the token > + generated for the CmObj. > + > + @retval EFI_SUCCESS The function completed successfully. > + @retval EFI_INVALID_PARAMETER Invalid parameter. > +**/ > +STATIC > +EFI_STATUS > +EFIAPI > +HwInfoAdd ( > + IN HW_INFO_PARSER_HANDLE ParserHandle, > + IN VOID * Context, > + IN CONST CM_OBJ_DESCRIPTOR * CmObjDesc, > + OUT CM_OBJECT_TOKEN * Token OPTIONAL > + ) > +{ > + EFI_STATUS Status; > + EDKII_PLATFORM_REPOSITORY_INFO * PlatformRepo; > + > + if ((ParserHandle == NULL) || > + (Context == NULL) || > + (CmObjDesc == NULL)) { > + ASSERT (0); > + return EFI_INVALID_PARAMETER; > + } > + > + PlatformRepo = (EDKII_PLATFORM_REPOSITORY_INFO*)Context; > + > +#ifndef MDEPKG_NDEBUG > + // Print the received objects. > + ParseCmObjDesc (CmObjDesc); > +#endif (10) Please use the DEBUG_CODE() or DEBUG_CODE_BEGIN() / DEBUG_CODE_END() macros here. > + > + Status = DynPlatRepoAddObject ( > + PlatformRepo->DynamicPlatformRepo, > + CmObjDesc, > + Token > + ); > + if (EFI_ERROR (Status)) { > + ASSERT (0); > + } (11) Please use the dedicated macro ASSERT_EFI_ERROR(). (There are other instances of the same, please update those as well.) > + return Status; > +} > + > +/** Cleanup the platform configuration repository. > + > + @param [in] This Pointer to the Configuration Manager Protocol. > + > + @retval EFI_SUCCESS Success > + @retval EFI_INVALID_PARAMETER A parameter is invalid. > +**/ > +STATIC > +EFI_STATUS > +EFIAPI > +CleanupPlatformRepository ( > + IN CONST EDKII_CONFIGURATION_MANAGER_PROTOCOL * CONST This > + ) > +{ > + EFI_STATUS Status; > + EDKII_PLATFORM_REPOSITORY_INFO * PlatformRepo; > + > + if (This == NULL) { > + ASSERT (0); > + return EFI_INVALID_PARAMETER; > + } > + > + PlatformRepo = This->PlatRepoInfo; > + > + // Shutdown the dynamic repo and free all objects. > + Status = DynamicPlatRepoShutdown (PlatformRepo->DynamicPlatformRepo); > + if (EFI_ERROR (Status)) { > + ASSERT (0); > + return Status; > + } > + > + // Shutdown parser. > + Status = HwInfoParserShutdown (PlatformRepo->FdtParserHandle); > + if (EFI_ERROR (Status)) { > + ASSERT (0); > + } > + return Status; > +} > + > +/** Initialize the platform configuration repository. > + > + @param [in] This Pointer to the Configuration Manager Protocol. > + > + @retval EFI_SUCCESS Success > + @retval EFI_INVALID_PARAMETER A parameter is invalid. > + @retval EFI_OUT_OF_RESOURCES An allocation has failed. > +**/ > +STATIC > +EFI_STATUS > +EFIAPI > +InitializePlatformRepository ( > + IN CONST EDKII_CONFIGURATION_MANAGER_PROTOCOL * CONST This > + ) > +{ > + EFI_STATUS Status; > + EDKII_PLATFORM_REPOSITORY_INFO * PlatformRepo; > + VOID * Hob; > + > + if (This == NULL) { > + ASSERT (0); > + return EFI_INVALID_PARAMETER; > + } > + > + Hob = GetFirstGuidHob (&gFdtHobGuid); > + if (Hob == NULL || GET_GUID_HOB_DATA_SIZE (Hob) != sizeof (UINT64)) { > + ASSERT (0); > + return EFI_NOT_FOUND; > + } > + > + PlatformRepo = This->PlatRepoInfo; > + PlatformRepo->FdtBase = (VOID *)*(UINTN*)GET_GUID_HOB_DATA (Hob); > + > + // Initialise the dynamic platform repository. > + Status = DynamicPlatRepoInit (&PlatformRepo->DynamicPlatformRepo); > + if (EFI_ERROR (Status)) { > + ASSERT (0); > + return Status; > + } > + > + // Initialise the FDT parser > + Status = HwInfoParserInit ( > + PlatformRepo->FdtBase, > + PlatformRepo, > + HwInfoAdd, > + &PlatformRepo->FdtParserHandle > + ); > + if (EFI_ERROR (Status)) { > + ASSERT (0); > + goto error_handler; (12) This label should be named ErrorHandler or just Error. (Applies to ConfigurationManagerDxeInitialize() as well.) > + } > + > + Status = HwInfoParse (PlatformRepo->FdtParserHandle); > + if (EFI_ERROR (Status)) { > + ASSERT (0); > + goto error_handler; > + } > + > + Status = DynamicPlatRepoFinalise (PlatformRepo->DynamicPlatformRepo); > + if (EFI_ERROR (Status)) { > + ASSERT (0); > + goto error_handler; > + } > + > + return EFI_SUCCESS; > + > +error_handler: > + CleanupPlatformRepository (This); > + return Status; > +} > + > +/** Return a standard namespace object. > + > + @param [in] This Pointer to the Configuration Manager Protocol. > + @param [in] CmObjectId The Configuration Manager Object ID. > + @param [in] Token An optional token identifying the object. If > + unused this must be CM_NULL_TOKEN. > + @param [in, out] CmObject Pointer to the Configuration Manager Object > + descriptor describing the requested Object. > + > + @retval EFI_SUCCESS Success. > + @retval EFI_INVALID_PARAMETER A parameter is invalid. > + @retval EFI_NOT_FOUND The required object information is not found. > +**/ > +EFI_STATUS > +EFIAPI > +GetStandardNameSpaceObject ( > + IN CONST EDKII_CONFIGURATION_MANAGER_PROTOCOL * CONST This, > + IN CONST CM_OBJECT_ID CmObjectId, > + IN CONST CM_OBJECT_TOKEN Token OPTIONAL, > + IN OUT CM_OBJ_DESCRIPTOR * CONST CmObject > + ) > +{ > + EFI_STATUS Status; > + EDKII_PLATFORM_REPOSITORY_INFO * PlatformRepo; > + UINTN AcpiTableCount; > + CM_OBJ_DESCRIPTOR CmObjDesc; > + > + if ((This == NULL) || (CmObject == NULL)) { > + ASSERT (This != NULL); > + ASSERT (CmObject != NULL); > + return EFI_INVALID_PARAMETER; > + } > + > + Status = EFI_NOT_FOUND; > + PlatformRepo = This->PlatRepoInfo; > + > + switch (GET_CM_OBJECT_ID (CmObjectId)) { > + case EStdObjCfgMgrInfo: > + Status = HandleCmObject ( > + CmObjectId, > + &PlatformRepo->CmInfo, > + sizeof (PlatformRepo->CmInfo), > + 1, > + CmObject > + ); > + break; > + > + case EStdObjAcpiTableList: > + AcpiTableCount = ARRAY_SIZE (PlatformRepo->CmAcpiTableList); > + > + // Get Pci config space information. > + Status = DynamicPlatRepoGetObject ( > + PlatformRepo->DynamicPlatformRepo, > + CREATE_CM_ARM_OBJECT_ID (EArmObjPciConfigSpaceInfo), > + CM_NULL_TOKEN, > + &CmObjDesc > + ); > + if (Status == EFI_NOT_FOUND) { > + // The last 3 tables are for PCIe. If PCIe information is not > + // present, Kvmtool was launched without the PCIe option. > + // Therefore, reduce the table count by 3. > + AcpiTableCount -= 3; > + } else if (EFI_ERROR (Status)) { > + ASSERT (0); > + return Status; > + } > + > + // Get the Gic version. > + Status = DynamicPlatRepoGetObject ( > + PlatformRepo->DynamicPlatformRepo, > + CREATE_CM_ARM_OBJECT_ID (EArmObjGicDInfo), > + CM_NULL_TOKEN, > + &CmObjDesc > + ); > + if (EFI_ERROR (Status)) { > + ASSERT (0); > + return Status; > + } > + if (((CM_ARM_GICD_INFO*)CmObjDesc.Data)->GicVersion < 3) { > + // IORT is only required for GicV3/4 > + AcpiTableCount -= 1; > + } > + > + Status = HandleCmObject ( > + CmObjectId, > + PlatformRepo->CmAcpiTableList, > + (sizeof (PlatformRepo->CmAcpiTableList[0]) * AcpiTableCount), > + AcpiTableCount, > + CmObject > + ); > + break; > + > + default: { (13) Please drop the brace. (applies to ArmKvmtoolPlatformGetObject() as well) > + Status = EFI_NOT_FOUND; > + DEBUG (( > + DEBUG_ERROR, > + "ERROR: Object 0x%x. Status = %r\n", > + CmObjectId, > + Status > + )); > + break; > + } > + } > + > + return Status; > +} > + > +/** Return an ARM namespace object. > + > + @param [in] This Pointer to the Configuration Manager Protocol. > + @param [in] CmObjectId The Configuration Manager Object ID. > + @param [in] Token An optional token identifying the object. If > + unused this must be CM_NULL_TOKEN. > + @param [in, out] CmObject Pointer to the Configuration Manager Object > + descriptor describing the requested Object. > + > + @retval EFI_SUCCESS Success. > + @retval EFI_INVALID_PARAMETER A parameter is invalid. > + @retval EFI_NOT_FOUND The required object information is not found. > +**/ > +EFI_STATUS > +EFIAPI > +GetArmNameSpaceObject ( > + IN CONST EDKII_CONFIGURATION_MANAGER_PROTOCOL * CONST This, > + IN CONST CM_OBJECT_ID CmObjectId, > + IN CONST CM_OBJECT_TOKEN Token OPTIONAL, > + IN OUT CM_OBJ_DESCRIPTOR * CONST CmObject > + ) > +{ > + EFI_STATUS Status; > + EDKII_PLATFORM_REPOSITORY_INFO * PlatformRepo; > + > + if ((This == NULL) || (CmObject == NULL)) { > + ASSERT (This != NULL); > + ASSERT (CmObject != NULL); > + return EFI_INVALID_PARAMETER; > + } > + > + Status = EFI_NOT_FOUND; > + PlatformRepo = This->PlatRepoInfo; > + > + // First check among the static objects. > + switch (GET_CM_OBJECT_ID (CmObjectId)) { > + case EArmObjPowerManagementProfileInfo: > + Status = HandleCmObject ( > + CmObjectId, > + &PlatformRepo->PmProfileInfo, > + sizeof (PlatformRepo->PmProfileInfo), > + 1, > + CmObject > + ); > + break; > + > + case EArmObjItsGroup: > + Status = HandleCmObject ( > + CmObjectId, > + &PlatformRepo->ItsGroupInfo, > + sizeof (PlatformRepo->ItsGroupInfo), > + 1, > + CmObject > + ); > + break; > + > + case EArmObjGicItsIdentifierArray: > + Status = HandleCmObjectRefByToken ( > + This, > + CmObjectId, > + PlatformRepo->ItsIdentifierArray, > + sizeof (PlatformRepo->ItsIdentifierArray), > + ARRAY_SIZE (PlatformRepo->ItsIdentifierArray), > + Token, > + GetItsIdentifierArray, > + CmObject > + ); > + break; > + > + case EArmObjRootComplex: > + Status = HandleCmObject ( > + CmObjectId, > + &PlatformRepo->RootComplexInfo, > + sizeof (PlatformRepo->RootComplexInfo), > + 1, > + CmObject > + ); > + break; > + > + case EArmObjIdMappingArray: > + Status = HandleCmObjectRefByToken ( > + This, > + CmObjectId, > + PlatformRepo->DeviceIdMapping, > + sizeof (PlatformRepo->DeviceIdMapping), > + ARRAY_SIZE (PlatformRepo->DeviceIdMapping), > + Token, > + GetDeviceIdMappingArray, > + CmObject > + ); > + break; > + > + default: > + // No match found among the static objects. > + // Check the dynamic objects. > + Status = DynamicPlatRepoGetObject ( > + PlatformRepo->DynamicPlatformRepo, > + CmObjectId, > + Token, > + CmObject > + ); > + break; > + } // switch > + > + if (Status == EFI_NOT_FOUND) { > + DEBUG (( > + DEBUG_INFO, > + "INFO: Object 0x%x. Status = %r\n", > + CmObjectId, > + Status > + )); > + } else { > + ASSERT_EFI_ERROR (Status); > + } > + > + return Status; > +} > + > +/** Return an OEM namespace object. > + > + @param [in] This Pointer to the Configuration Manager Protocol. > + @param [in] CmObjectId The Configuration Manager Object ID. > + @param [in] Token An optional token identifying the object. If > + unused this must be CM_NULL_TOKEN. > + @param [in, out] CmObject Pointer to the Configuration Manager Object > + descriptor describing the requested Object. > + > + @retval EFI_SUCCESS Success. > + @retval EFI_INVALID_PARAMETER A parameter is invalid. > + @retval EFI_NOT_FOUND The required object information is not found. > +**/ > +EFI_STATUS > +EFIAPI > +GetOemNameSpaceObject ( > + IN CONST EDKII_CONFIGURATION_MANAGER_PROTOCOL * CONST This, > + IN CONST CM_OBJECT_ID CmObjectId, > + IN CONST CM_OBJECT_TOKEN Token OPTIONAL, > + IN OUT CM_OBJ_DESCRIPTOR * CONST CmObject > + ) > +{ > + EFI_STATUS Status; > + > + Status = EFI_SUCCESS; > + if ((This == NULL) || (CmObject == NULL)) { > + ASSERT (This != NULL); > + ASSERT (CmObject != NULL); > + return EFI_INVALID_PARAMETER; > + } > + > + switch (GET_CM_OBJECT_ID (CmObjectId)) { > + default: { > + Status = EFI_NOT_FOUND; > + DEBUG (( > + DEBUG_ERROR, > + "ERROR: Object 0x%x. Status = %r\n", > + CmObjectId, > + Status > + )); > + break; > + } > + } > + > + return Status; > +} > + > +/** The GetObject function defines the interface implemented by the > + Configuration Manager Protocol for returning the Configuration > + Manager Objects. > + > + @param [in] This Pointer to the Configuration Manager Protocol. > + @param [in] CmObjectId The Configuration Manager Object ID. > + @param [in] Token An optional token identifying the object. If > + unused this must be CM_NULL_TOKEN. > + @param [in, out] CmObject Pointer to the Configuration Manager Object > + descriptor describing the requested Object. > + > + @retval EFI_SUCCESS Success. > + @retval EFI_INVALID_PARAMETER A parameter is invalid. > + @retval EFI_NOT_FOUND The required object information is not found. > +**/ > +EFI_STATUS > +EFIAPI > +ArmKvmtoolPlatformGetObject ( > + IN CONST EDKII_CONFIGURATION_MANAGER_PROTOCOL * CONST This, > + IN CONST CM_OBJECT_ID CmObjectId, > + IN CONST CM_OBJECT_TOKEN Token OPTIONAL, > + IN OUT CM_OBJ_DESCRIPTOR * CONST CmObject > + ) > +{ > + EFI_STATUS Status; > + > + if ((This == NULL) || (CmObject == NULL)) { > + ASSERT (This != NULL); > + ASSERT (CmObject != NULL); > + return EFI_INVALID_PARAMETER; > + } > + > + switch (GET_CM_NAMESPACE_ID (CmObjectId)) { > + case EObjNameSpaceStandard: > + Status = GetStandardNameSpaceObject (This, CmObjectId, Token, CmObject); > + break; > + case EObjNameSpaceArm: > + Status = GetArmNameSpaceObject (This, CmObjectId, Token, CmObject); > + break; > + case EObjNameSpaceOem: > + Status = GetOemNameSpaceObject (This, CmObjectId, Token, CmObject); > + break; > + default: { > + Status = EFI_INVALID_PARAMETER; > + DEBUG (( > + DEBUG_ERROR, > + "ERROR: Unknown Namespace Object = 0x%x. Status = %r\n", > + CmObjectId, > + Status > + )); > + break; > + } > + } > + > + return Status; > +} > + > +/** The SetObject function defines the interface implemented by the > + Configuration Manager Protocol for updating the Configuration > + Manager Objects. > + > + @param [in] This Pointer to the Configuration Manager Protocol. > + @param [in] CmObjectId The Configuration Manager Object ID. > + @param [in] Token An optional token identifying the object. If > + unused this must be CM_NULL_TOKEN. > + @param [in] CmObject Pointer to the Configuration Manager Object > + descriptor describing the Object. > + > + @retval EFI_UNSUPPORTED This operation is not supported. > +**/ > +EFI_STATUS > +EFIAPI > +ArmKvmtoolPlatformSetObject ( > + IN CONST EDKII_CONFIGURATION_MANAGER_PROTOCOL * CONST This, > + IN CONST CM_OBJECT_ID CmObjectId, > + IN CONST CM_OBJECT_TOKEN Token OPTIONAL, > + IN CM_OBJ_DESCRIPTOR * CONST CmObject > + ) > +{ > + return EFI_UNSUPPORTED; > +} > + > +/** A structure describing the configuration manager protocol interface. > +*/ > +STATIC > +CONST > +EDKII_CONFIGURATION_MANAGER_PROTOCOL KvmtoolPlatformConfigManagerProtocol = { > + CREATE_REVISION(1,0), > + ArmKvmtoolPlatformGetObject, > + ArmKvmtoolPlatformSetObject, > + &KvmtoolPlatRepositoryInfo > +}; > + > +/** > + Entrypoint of Configuration Manager Dxe. > + > + @param ImageHandle > + @param SystemTable > + > + @return EFI_SUCCESS > + @return EFI_LOAD_ERROR > + @return EFI_OUT_OF_RESOURCES (14) These should be @retval, not @return. > +**/ > +EFI_STATUS > +EFIAPI > +ConfigurationManagerDxeInitialize ( > + IN EFI_HANDLE ImageHandle, > + IN EFI_SYSTEM_TABLE * SystemTable > + ) > +{ > + EFI_STATUS Status; > + > + if (PcdGetBool (PcdForceNoAcpi)) { > + // Use DT and not ACPI. > + return EFI_SUCCESS; > + } (15) This is wrong. KvmtoolPlatformDxe already (correctly) installs either "gEdkiiPlatformHasAcpiGuid" or "gEdkiiPlatformHasDeviceTreeGuid". "PcdForceNoAcpi" must not be used outside of that driver. In this driver, the DEPEX section of the INF file should list "gEdkiiPlatformHasAcpiGuid". If ACPI is disabled, this driver should not be dispatched at all. > + > + Status = gBS->InstallProtocolInterface ( > + &ImageHandle, > + &gEdkiiConfigurationManagerProtocolGuid, > + EFI_NATIVE_INTERFACE, > + (VOID*)&KvmtoolPlatformConfigManagerProtocol > + ); > + if (EFI_ERROR (Status)) { > + DEBUG (( > + DEBUG_ERROR, > + "ERROR: Failed to get Install Configuration Manager Protocol." \ > + " Status = %r\n", > + Status > + )); > + goto error_handler; > + } > + > + Status = InitializePlatformRepository ( > + &KvmtoolPlatformConfigManagerProtocol > + ); > + if (EFI_ERROR (Status)) { > + DEBUG (( > + DEBUG_ERROR, > + "ERROR: Failed to initialize the Platform Configuration Repository." \ > + " Status = %r\n", > + Status > + )); > + } > + > +error_handler: > + return Status; > +} (16) I've paid basically zero attention to your error handlers, but this is so obviously wrong that it catches my eye. If InitializePlatformRepository() fails and so you exit with an error, the driver will be unloaded, and your installed protocol interface will be a dangling one. > + > +/** > + Unload function for this image. > + > + @param ImageHandle Handle for the image of this driver. > + > + @retval EFI_SUCCESS Driver unloaded successfully. > + @return other Driver can not unloaded. > +**/ > +EFI_STATUS > +EFIAPI > +ConfigurationManagerDxeUnloadImage ( > + IN EFI_HANDLE ImageHandle > + ) > +{ > + return CleanupPlatformRepository (&KvmtoolPlatformConfigManagerProtocol); > +} > diff --git a/ArmVirtPkg/KvmtoolCfgMgrDxe/ConfigurationManager.h b/ArmVirtPkg/KvmtoolCfgMgrDxe/ConfigurationManager.h > new file mode 100644 > index 000000000000..94cfca3b7671 > --- /dev/null > +++ b/ArmVirtPkg/KvmtoolCfgMgrDxe/ConfigurationManager.h > @@ -0,0 +1,94 @@ > +/** @file > + > + Copyright (c) 2021, Arm Limited. All rights reserved.
> + > + SPDX-License-Identifier: BSD-2-Clause-Patent > + > + @par Glossary: > + - Cm or CM - Configuration Manager > + - Obj or OBJ - Object > +**/ > + > +#ifndef CONFIGURATION_MANAGER_H_ > +#define CONFIGURATION_MANAGER_H_ > + > +/** C array containing the compiled AML template. > + This symbol is defined in the auto generated C file > + containing the AML bytecode array. > +*/ (17) Please apply all the comment observations I made for the C file to the header file. > +extern CHAR8 dsdt_aml_code[]; > + > +/** The configuration manager version. > +*/ > +#define CONFIGURATION_MANAGER_REVISION CREATE_REVISION (1, 0) > + > +/** The OEM ID > +*/ > +#define CFG_MGR_OEM_ID { 'A', 'R', 'M', 'L', 'T', 'D' } > + > +/** A function that prepares Configuration Manager Objects for returning. > + > + @param [in] This Pointer to the Configuration Manager Protocol. > + @param [in] CmObjectId The Configuration Manager Object ID. > + @param [in] Token A token for identifying the object. > + @param [out] CmObject Pointer to the Configuration Manager Object > + descriptor describing the requested Object. > + > + @retval EFI_SUCCESS Success. > + @retval EFI_INVALID_PARAMETER A parameter is invalid. > + @retval EFI_NOT_FOUND The required object information is not found. > +**/ > +typedef EFI_STATUS (*CM_OBJECT_HANDLER_PROC) ( > + IN CONST EDKII_CONFIGURATION_MANAGER_PROTOCOL * CONST This, > + IN CONST CM_OBJECT_ID CmObjectId, > + IN CONST CM_OBJECT_TOKEN Token, > + IN OUT CM_OBJ_DESCRIPTOR * CONST CmObject > + ); > + > +/** A helper macro for mapping a reference token. > +*/ > +#define REFERENCE_TOKEN(Field) \ > + (CM_OBJECT_TOKEN)((UINT8*)&KvmtoolPlatRepositoryInfo + \ > + OFFSET_OF (EDKII_PLATFORM_REPOSITORY_INFO, Field)) > + > +/** The number of ACPI tables to install > +*/ > +#define PLAT_ACPI_TABLE_COUNT 10 > + > +/** A structure describing the platform configuration > + manager repository information > +*/ > +typedef struct PlatformRepositoryInfo { (18) the "PlatformRepositoryInfo" struct tag is not spelled idiomatically. On the other hand, it's also not used anywhere. I suggest simply dropping it. > + /// Configuration Manager Information. > + CM_STD_OBJ_CONFIGURATION_MANAGER_INFO CmInfo; > + > + /// List of ACPI tables > + CM_STD_OBJ_ACPI_TABLE_INFO CmAcpiTableList[PLAT_ACPI_TABLE_COUNT]; > + > + /// Power management profile information > + CM_ARM_POWER_MANAGEMENT_PROFILE_INFO PmProfileInfo; > + > + /// ITS Group node > + CM_ARM_ITS_GROUP_NODE ItsGroupInfo; > + > + /// ITS Identifier array > + CM_ARM_ITS_IDENTIFIER ItsIdentifierArray[1]; > + > + /// PCI Root complex node > + CM_ARM_ROOT_COMPLEX_NODE RootComplexInfo; > + > + /// Array of DeviceID mapping > + CM_ARM_ID_MAPPING DeviceIdMapping[1]; > + > + /// Dynamic platform repository. > + /// CmObj created by parsing the Kvmtool device tree are stored here. > + DYNAMIC_PLATFORM_REPOSITORY_INFO * DynamicPlatformRepo; > + > + /// Base address of the FDT. > + VOID * FdtBase; > + > + /// A handle to the FDT HwInfoParser. > + HW_INFO_PARSER_HANDLE FdtParserHandle; > +} EDKII_PLATFORM_REPOSITORY_INFO; > + > +#endif // CONFIGURATION_MANAGER_H_ > diff --git a/ArmVirtPkg/KvmtoolCfgMgrDxe/ConfigurationManagerDxe.inf b/ArmVirtPkg/KvmtoolCfgMgrDxe/ConfigurationManagerDxe.inf > new file mode 100644 > index 000000000000..9f0bf72fce2d > --- /dev/null > +++ b/ArmVirtPkg/KvmtoolCfgMgrDxe/ConfigurationManagerDxe.inf > @@ -0,0 +1,58 @@ > +## @file > +# Configuration Manager Dxe > +# > +# Copyright (c) 2021, Arm Limited. All rights reserved.
> +# > +# SPDX-License-Identifier: BSD-2-Clause-Patent > +## > + > +[Defines] > + INF_VERSION = 0x0001001B > + BASE_NAME = ConfigurationManagerDxe > + FILE_GUID = 3C80D366-510C-4154-BB3A-E12439AD337C > + MODULE_TYPE = DXE_DRIVER > + VERSION_STRING = 1.0 > + ENTRY_POINT = ConfigurationManagerDxeInitialize > + UNLOAD_IMAGE = ConfigurationManagerDxeUnloadImage > + > +# > +# The following information is for reference only and not required by the build tools. > +# > +# VALID_ARCHITECTURES = ARM AARCH64 > +# > + > +[Sources] > + AslTables/Dsdt.asl > + ConfigurationManager.c > + ConfigurationManager.h > + ConfigurationManagerDxe.inf > + > +[Packages] > + ArmVirtPkg/ArmVirtPkg.dec > + DynamicTablesPkg/DynamicTablesPkg.dec > + EmbeddedPkg/EmbeddedPkg.dec > + MdePkg/MdePkg.dec > + > +[LibraryClasses] > + DynamicPlatRepoLib > + HobLib > + HwInfoParserLib > + PrintLib > + TableHelperLib > + UefiBootServicesTableLib > + UefiDriverEntryPoint > + UefiRuntimeServicesTableLib > + > +[Protocols] > + gEdkiiConfigurationManagerProtocolGuid > + > +[Guids] > + gFdtHobGuid > + > +[FixedPcd] (19) please drop this empty section. > + > +[Pcd] > + gArmVirtTokenSpaceGuid.PcdForceNoAcpi > + > +[Depex] > + TRUE > I can't see myself reviewing this again (even at this level), so I'll trust you on all the updates. Acked-by: Laszlo Ersek Thanks Laszlo