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.web08.8021.1666342977728770913 for ; Fri, 21 Oct 2022 02:02:58 -0700 Authentication-Results: mx.groups.io; dkim=missing; spf=pass (domain: arm.com, ip: 217.140.110.172, mailfrom: pierre.gondois@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 40FA91FB; Fri, 21 Oct 2022 02:03:03 -0700 (PDT) Received: from [10.57.5.74] (unknown [10.57.5.74]) by usa-sjc-imap-foss1.foss.arm.com (Postfix) with ESMTPSA id 8D8523F67D; Fri, 21 Oct 2022 02:02:55 -0700 (PDT) Message-ID: <195f6a27-a09c-ac05-c9d3-99a441567336@arm.com> Date: Fri, 21 Oct 2022 11:02:48 +0200 MIME-Version: 1.0 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101 Thunderbird/102.2.2 Subject: Re: [edk2-devel] [PATCH] DynamicTablesPkg:ACPI: SLIT Generator files To: devel@edk2.groups.io, username@nvidia.com, Sami.Mujawar@arm.com, Alexei.Fedorov@arm.com Cc: Swatisri Kantamsetti References: <9ac0f76bcacc7610b8b22f7a024fe3818bbdd235.1664475116.git.swatisrik@nvidia.com> From: "PierreGondois" In-Reply-To: <9ac0f76bcacc7610b8b22f7a024fe3818bbdd235.1664475116.git.swatisrik@nvidia.com> Content-Language: en-US Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 7bit Hello Swatisri, On 9/29/22 20:18, Name via groups.io wrote: > From: Swatisri Kantamsetti > > Added SLIT Generator and supporting files to > generate SLIT ACPI Table > > Signed-off-by: Swatisri Kantamsetti > --- > DynamicTablesPkg/DynamicTables.dsc.inc | 2 + > DynamicTablesPkg/Include/AcpiTableGenerator.h | 1 + > .../Include/ArmNameSpaceObjects.h | 12 + > .../Arm/AcpiSlitLibArm/AcpiSlitLibArm.inf | 29 ++ > .../Acpi/Arm/AcpiSlitLibArm/SlitGenerator.c | 326 ++++++++++++++++++ > MdePkg/Include/IndustryStandard/Slit.h | 39 +++ > 6 files changed, 409 insertions(+) > create mode 100644 DynamicTablesPkg/Library/Acpi/Arm/AcpiSlitLibArm/AcpiSlitLibArm.inf > create mode 100644 DynamicTablesPkg/Library/Acpi/Arm/AcpiSlitLibArm/SlitGenerator.c > create mode 100644 MdePkg/Include/IndustryStandard/Slit.h > > diff --git a/DynamicTablesPkg/DynamicTables.dsc.inc b/DynamicTablesPkg/DynamicTables.dsc.inc > index 3d4fa0c4c4..c24fc1d719 100644 > --- a/DynamicTablesPkg/DynamicTables.dsc.inc > +++ b/DynamicTablesPkg/DynamicTables.dsc.inc > @@ -31,6 +31,7 @@ > DynamicTablesPkg/Library/Acpi/Arm/AcpiMcfgLibArm/AcpiMcfgLibArm.inf > DynamicTablesPkg/Library/Acpi/Arm/AcpiPpttLibArm/AcpiPpttLibArm.inf > DynamicTablesPkg/Library/Acpi/Arm/AcpiRawLibArm/AcpiRawLibArm.inf > + DynamicTablesPkg/Library/Acpi/Arm/AcpiSlitLibArm/AcpiSlitLibArm.inf > DynamicTablesPkg/Library/Acpi/Arm/AcpiSpcrLibArm/AcpiSpcrLibArm.inf > DynamicTablesPkg/Library/Acpi/Arm/AcpiSratLibArm/AcpiSratLibArm.inf > > @@ -56,6 +57,7 @@ > NULL|DynamicTablesPkg/Library/Acpi/Arm/AcpiPpttLibArm/AcpiPpttLibArm.inf > NULL|DynamicTablesPkg/Library/Acpi/Arm/AcpiRawLibArm/AcpiRawLibArm.inf > NULL|DynamicTablesPkg/Library/Acpi/Arm/AcpiSpcrLibArm/AcpiSpcrLibArm.inf > + NULL|DynamicTablesPkg/Library/Acpi/Arm/AcpiSlitLibArm/AcpiSlitLibArm.inf > NULL|DynamicTablesPkg/Library/Acpi/Arm/AcpiSratLibArm/AcpiSratLibArm.inf > > # AML Fixup > diff --git a/DynamicTablesPkg/Include/AcpiTableGenerator.h b/DynamicTablesPkg/Include/AcpiTableGenerator.h > index f962dbff57..82d0272031 100644 > --- a/DynamicTablesPkg/Include/AcpiTableGenerator.h > +++ b/DynamicTablesPkg/Include/AcpiTableGenerator.h > @@ -93,6 +93,7 @@ typedef enum StdAcpiTableId { > EStdAcpiTableIdMcfg, ///< MCFG Generator > EStdAcpiTableIdIort, ///< IORT Generator > EStdAcpiTableIdPptt, ///< PPTT Generator > + EStdAcpiTableIdSlit, ///< SLIT Generator > EStdAcpiTableIdSrat, ///< SRAT Generator > EStdAcpiTableIdSsdtSerialPort, ///< SSDT Serial-Port Generator > EStdAcpiTableIdSsdtCmn600, ///< SSDT Cmn-600 Generator > diff --git a/DynamicTablesPkg/Include/ArmNameSpaceObjects.h b/DynamicTablesPkg/Include/ArmNameSpaceObjects.h > index c66b441d53..aa3f464132 100644 > --- a/DynamicTablesPkg/Include/ArmNameSpaceObjects.h > +++ b/DynamicTablesPkg/Include/ArmNameSpaceObjects.h > @@ -65,6 +65,7 @@ typedef enum ArmObjectID { > EArmObjRmr, ///< 40 - Reserved Memory Range Node > EArmObjMemoryRangeDescriptor, ///< 41 - Memory Range Descriptor > EArmObjCpcInfo, ///< 42 - Continuous Performance Control Info > + EArmObjSystemLocalityInfo, ///< 43 - Relative Distance Info > EArmObjMax > } EARM_OBJECT_ID; > > @@ -1095,6 +1096,17 @@ typedef struct CmArmRmrDescriptor { > */ > typedef AML_CPC_INFO CM_ARM_CPC_INFO; > > +/** A structure that describes the System Locality Information > + > + This information is used to optimize the system performance This sentence could be replaced by something more desciptive. > + > + ID: EArmObjSystemLocalityInfo > +*/ > +typedef struct CmArmSystemLocalityInfo { > + UINT64 NumSystemLocalities; > + UINT8 *Distance; The fields need comments I think. > +} CM_ARM_SYSTEM_LOCALITY_INFO; > + > #pragma pack() > > #endif // ARM_NAMESPACE_OBJECTS_H_ > diff --git a/DynamicTablesPkg/Library/Acpi/Arm/AcpiSlitLibArm/AcpiSlitLibArm.inf b/DynamicTablesPkg/Library/Acpi/Arm/AcpiSlitLibArm/AcpiSlitLibArm.inf > new file mode 100644 > index 0000000000..ab0ffc9a39 > --- /dev/null > +++ b/DynamicTablesPkg/Library/Acpi/Arm/AcpiSlitLibArm/AcpiSlitLibArm.inf > @@ -0,0 +1,29 @@ > +## @file > +# SRAT Table Generator > +# > +# Copyright (c) 2022, ARM Limited. All rights reserved. > +# > +# SPDX-License-Identifier: BSD-2-Clause-Patent > +## > + > +[Defines] > + INF_VERSION = 0x0001001B > + BASE_NAME = AcpiSlitLibArm > + FILE_GUID = 28e3f485-0c24-4376-8982-8e1febd791bc > + VERSION_STRING = 1.0 > + MODULE_TYPE = DXE_DRIVER > + LIBRARY_CLASS = NULL|DXE_DRIVER > + CONSTRUCTOR = AcpiSlitLibConstructor > + DESTRUCTOR = AcpiSlitLibDestructor > + > +[Sources] > + SlitGenerator.c > + > +[Packages] > + EmbeddedPkg/EmbeddedPkg.dec > + DynamicTablesPkg/DynamicTablesPkg.dec > + MdeModulePkg/MdeModulePkg.dec > + MdePkg/MdePkg.dec This could be sorted in alphabetical order. > + > +[LibraryClasses] > + BaseLib > diff --git a/DynamicTablesPkg/Library/Acpi/Arm/AcpiSlitLibArm/SlitGenerator.c b/DynamicTablesPkg/Library/Acpi/Arm/AcpiSlitLibArm/SlitGenerator.c > new file mode 100644 > index 0000000000..1a232ce95c > --- /dev/null > +++ b/DynamicTablesPkg/Library/Acpi/Arm/AcpiSlitLibArm/SlitGenerator.c > @@ -0,0 +1,326 @@ > +/** @file > + SLIT Table Generator > + > + Copyright (c) 2019 - 2020, Arm Limited. All rights reserved. > + SPDX-License-Identifier: BSD-2-Clause-Patent > + > + @par Reference(s): > + - ACPI 6.3 Specification, January 2019 > + > + @par Glossary: > + - Cm or CM - Configuration Manager > + - Obj or OBJ - Object > +**/ > + > + NIT: one newline should be enough. > +#include > +#include > +#include > +#include > +#include > +#include > + > +// Module specific include files. > +#include > +#include > +#include > +#include > +#include > + > +/** > + ARM standard SLIT Generator > + > + Requirements: > + The following Configuration Manager Object(s) are used by this Generator: > + - EArmObjSystemLocalityInfo (Required) > +*/ > + > +/** > + This macro expands to a function that retrieves the Relative Distance I think it should be 'System Locality' instead. It seems that there are other places where 'Relative Distance' is used, maybe there has been a renaming ? > + information from the Configuration Manager. > +*/ > +GET_OBJECT_LIST ( > + EObjNameSpaceArm, > + EArmObjSystemLocalityInfo, > + CM_ARM_SYSTEM_LOCALITY_INFO > + ); > + > +/** Construct the SLIT ACPI table. > + > + Called by the Dynamic Table Manager, this function invokes the > + Configuration Manager protocol interface to get the required hardware > + information for generating the ACPI table. > + > + If this function allocates any resources then they must be freed > + in the FreeXXXXTableResources function. > + > + @param [in] This Pointer to the table generator. > + @param [in] AcpiTableInfo Pointer to the ACPI Table Info. > + @param [in] CfgMgrProtocol Pointer to the Configuration Manager > + Protocol Interface. > + @param [out] Table Pointer to the constructed ACPI Table. > + > + @retval EFI_SUCCESS Table generated successfully. > + @retval EFI_INVALID_PARAMETER A parameter is invalid. > + @retval EFI_NOT_FOUND The required object was not found. > + @retval EFI_BAD_BUFFER_SIZE The size returned by the Configuration > + Manager is less than the Object size for the > + requested object. > + @retval EFI_OUT_OF_RESOURCES Memory allocation failed. > +**/ > +STATIC > +EFI_STATUS > +EFIAPI > +BuildSlitTable ( > + IN CONST ACPI_TABLE_GENERATOR *CONST This, > + IN CONST CM_STD_OBJ_ACPI_TABLE_INFO *CONST AcpiTableInfo, > + IN CONST EDKII_CONFIGURATION_MANAGER_PROTOCOL *CONST CfgMgrProtocol, > + OUT EFI_ACPI_DESCRIPTION_HEADER **CONST Table > + ) > +{ > + EFI_STATUS Status; > + UINT32 TableSize; > + UINT32 SlitObjectCount; > + UINT32 SlitObjectOffset; > + UINT64 NumSysLocalities; > + UINT8 *RelDistInfo; > + > + CM_ARM_SYSTEM_LOCALITY_INFO *SysLocalityInfo; > + EFI_ACPI_6_4_SYSTEM_LOCALITY_INFORMATION_TABLE *Slit; > + > + ASSERT ( > + (This != NULL) && > + (AcpiTableInfo != NULL) && > + (CfgMgrProtocol != NULL) && > + (Table != NULL) && > + (AcpiTableInfo->TableGeneratorId == This->GeneratorID) && > + (AcpiTableInfo->AcpiTableSignature == This->AcpiTableSignature) > + ); I think it would be better to have multiple ASSERT to identify from where the error came, as: ASSERT (This != NULL); ASSERT (AcpiTableInfo != NULL); ... > + > + if ((AcpiTableInfo->AcpiTableRevision < This->MinAcpiTableRevision) || > + (AcpiTableInfo->AcpiTableRevision > This->AcpiTableRevision)) > + { > + DEBUG (( > + DEBUG_ERROR, > + "ERROR: SLIT: Requested table revision = %d is not supported. " > + "Supported table revisions: Minimum = %d. Maximum = %d\n", > + AcpiTableInfo->AcpiTableRevision, > + This->MinAcpiTableRevision, > + This->AcpiTableRevision > + )); > + return EFI_INVALID_PARAMETER; > + } > + > + *Table = NULL; > + > + Status = GetEArmObjSystemLocalityInfo ( > + CfgMgrProtocol, > + CM_NULL_TOKEN, > + &SysLocalityInfo, > + &SlitObjectCount > + ); > + if (EFI_ERROR (Status) && (Status != EFI_NOT_FOUND)) { > + DEBUG (( > + DEBUG_ERROR, > + "ERROR: SLIT: Failed to get Relative Distance Info. Status = %r\n", > + Status > + )); > + goto error_handler; > + } I think we don't need to free memory here, so no need to go to the error_handler, we can just: return Status; > + > + // Calculate the size of the SLIT table > + TableSize = sizeof (EFI_ACPI_6_4_SYSTEM_LOCALITY_INFORMATION_TABLE); > + > + // Number of System Localities > + NumSysLocalities = SysLocalityInfo->NumSystemLocalities; I think that if (SlitObjectCount == 0), then SysLocalityInfo is pointing to random memory. However NumSysLocalities is used here and later in the function. > + > + if (SlitObjectCount != 0) { > + SlitObjectOffset = TableSize; > + TableSize += (sizeof (UINT8) * NumSysLocalities * NumSysLocalities); > + } > + > + // Allocate the Buffer for SLIT table > + *Table = (EFI_ACPI_DESCRIPTION_HEADER *)AllocateZeroPool (TableSize); > + if (*Table == NULL) { > + Status = EFI_OUT_OF_RESOURCES; > + DEBUG (( > + DEBUG_ERROR, > + "ERROR: SLIT: Failed to allocate memory for SLIT Table, Size = %d," \ > + " Status = %r\n", > + TableSize, > + Status > + )); > + goto error_handler; > + } > + > + Slit = (EFI_ACPI_6_4_SYSTEM_LOCALITY_INFORMATION_TABLE *)*Table; > + > + DEBUG (( > + DEBUG_INFO, > + "SLIT: Slit = 0x%p TableSize = 0x%x\n", > + Slit, > + TableSize > + )); > + > + Status = AddAcpiHeader ( > + CfgMgrProtocol, > + This, > + &Slit->Header, > + AcpiTableInfo, > + TableSize > + ); > + if (EFI_ERROR (Status)) { > + DEBUG (( > + DEBUG_ERROR, > + "ERROR: SLIT: Failed to add ACPI header. Status = %r\n", > + Status > + )); > + goto error_handler; > + } > + > + // Number of System Localities > + Slit->NumLocalities = NumSysLocalities; > + > + // Offset to place the Relative Distance Info > + RelDistInfo = (UINT8*)((UINT8*)Slit + SlitObjectOffset); > + > + // Adding Relative distance in SLIT Table > + for (UINT64 Index = 0; Index < Slit->NumLocalities * Slit->NumLocalities; Index++) { I think Slit->NumLocalitie should not be used in this for loop to avoid dereferencing the pointer and make the multiplication for each loop. I think a simpler solution would be to use CopyMem. > + *RelDistInfo = SysLocalityInfo->Distance[Index]; > + // Next > + RelDistInfo++; > + }// while > + > + return Status; > + > +error_handler: > + The newline could be removed. > + if (*Table != NULL) { > + FreePool (*Table); > + *Table = NULL; > + } > + > + return Status; > +} > + > +/** Free any resources allocated for constructing the SLIT. > + > + @param [in] This Pointer to the table generator. > + @param [in] AcpiTableInfo Pointer to the ACPI Table Info. > + @param [in] CfgMgrProtocol Pointer to the Configuration Manager > + Protocol Interface. > + @param [in, out] Table Pointer to the ACPI Table. > + > + @retval EFI_SUCCESS The resources were freed successfully. > + @retval EFI_INVALID_PARAMETER The table pointer is NULL or invalid. > +**/ > +STATIC > +EFI_STATUS > +FreeSlitTableResources ( > + IN CONST ACPI_TABLE_GENERATOR *CONST This, > + IN CONST CM_STD_OBJ_ACPI_TABLE_INFO *CONST AcpiTableInfo, > + IN CONST EDKII_CONFIGURATION_MANAGER_PROTOCOL *CONST CfgMgrProtocol, > + IN OUT EFI_ACPI_DESCRIPTION_HEADER **CONST Table > + ) > +{ > + ASSERT ( > + (This != NULL) && > + (AcpiTableInfo != NULL) && > + (CfgMgrProtocol != NULL) && > + (AcpiTableInfo->TableGeneratorId == This->GeneratorID) && > + (AcpiTableInfo->AcpiTableSignature == This->AcpiTableSignature) > + ); I think it would be better to have multiple ASSERT to identify from where the error came, as: ASSERT (This != NULL); ASSERT (AcpiTableInfo != NULL); ... > + > + if ((Table == NULL) || (*Table == NULL)) { > + DEBUG ((DEBUG_ERROR, "ERROR: SLIT: Invalid Table Pointer\n")); > + ASSERT (0); > + return EFI_INVALID_PARAMETER; > + } > + > + FreePool (*Table); > + *Table = NULL; > + return EFI_SUCCESS; > +} > + > +/** The SLIT Table Generator revision. > +*/ > +#define SLIT_GENERATOR_REVISION CREATE_REVISION (1, 0) > + > +/** The interface for the SLIT Table Generator. > +*/ > +STATIC > +CONST > +ACPI_TABLE_GENERATOR SlitGenerator = { > + // Generator ID > + CREATE_STD_ACPI_TABLE_GEN_ID (EStdAcpiTableIdSlit), > + // Generator Description > + L"ACPI.STD.SLIT.GENERATOR", > + // ACPI Table Signature > + EFI_ACPI_6_4_SYSTEM_LOCALITY_INFORMATION_TABLE_SIGNATURE, > + // ACPI Table Revision supported by this Generator > + EFI_ACPI_6_4_SYSTEM_LOCALITY_INFORMATION_TABLE_REVISION, > + // Minimum supported ACPI Table Revision > + EFI_ACPI_6_4_SYSTEM_LOCALITY_INFORMATION_TABLE_REVISION, > + // Creator ID > + TABLE_GENERATOR_CREATOR_ID_ARM, > + // Creator Revision > + SLIT_GENERATOR_REVISION, > + // Build Table function > + BuildSlitTable, > + // Free Resource function > + FreeSlitTableResources, > + // Extended build function not needed > + NULL, > + // Extended build function not implemented by the generator. > + // Hence extended free resource function is not required. > + NULL > +}; > + > +/** Register the Generator with the ACPI Table Factory. > + > + @param [in] ImageHandle The handle to the image. > + @param [in] SystemTable Pointer to the System Table. > + > + @retval EFI_SUCCESS The Generator is registered. > + @retval EFI_INVALID_PARAMETER A parameter is invalid. > + @retval EFI_ALREADY_STARTED The Generator for the Table ID > + is already registered. > +**/ > +EFI_STATUS > +EFIAPI > +AcpiSlitLibConstructor ( > + IN EFI_HANDLE ImageHandle, > + IN EFI_SYSTEM_TABLE *SystemTable > + ) > +{ > + EFI_STATUS Status; > + > + Status = RegisterAcpiTableGenerator (&SlitGenerator); > + DEBUG ((DEBUG_INFO, "SLIT: Register Generator. Status = %r\n", Status)); > + ASSERT_EFI_ERROR (Status); > + return Status; > +} > + > +/** Deregister the Generator from the ACPI Table Factory. > + > + @param [in] ImageHandle The handle to the image. > + @param [in] SystemTable Pointer to the System Table. > + > + @retval EFI_SUCCESS The Generator is deregistered. > + @retval EFI_INVALID_PARAMETER A parameter is invalid. > + @retval EFI_NOT_FOUND The Generator is not registered. > +**/ > +EFI_STATUS > +EFIAPI > +AcpiSlitLibDestructor ( > + IN EFI_HANDLE ImageHandle, > + IN EFI_SYSTEM_TABLE *SystemTable > + ) > +{ > + EFI_STATUS Status; > + > + Status = DeregisterAcpiTableGenerator (&SlitGenerator); > + DEBUG ((DEBUG_INFO, "SLIT: Deregister Generator. Status = %r\n", Status)); > + ASSERT_EFI_ERROR (Status); > + return Status; > +} > diff --git a/MdePkg/Include/IndustryStandard/Slit.h b/MdePkg/Include/IndustryStandard/Slit.h > new file mode 100644 > index 0000000000..3413b4046b > --- /dev/null > +++ b/MdePkg/Include/IndustryStandard/Slit.h There is already definitions/macros for the SLIT in: MdePkg/Include/IndustryStandard/Acpi64.h Cf: EFI_ACPI_6_4_SYSTEM_LOCALITY_DISTANCE_INFORMATION_TABLE_HEADER I think this new file can be removed. > @@ -0,0 +1,39 @@ > +/** @file > + ACPI System Locality Information Table (SLIT) > + > + https://developer.arm.com/documentation/ddi0598/latest > + > + Copyright (c) 2022 NVIDIA ???????? > + > + SPDX-License-Identifier: BSD-2-Clause-Patent > +**/ > + > +#ifndef __SLIT_H__ > +#define __SLIT_H__ > + > +#pragma pack(1) > + > +/// > +/// System Locality Information Table (SLIT) > +/// > +typedef struct { > + EFI_ACPI_DESCRIPTION_HEADER Header; > + UINT64 NumLocalities; > +} EFI_ACPI_6_4_SYSTEM_LOCALITY_INFORMATION_TABLE; > + > +/// > +/// SLIT Revision (as defined in ACPI 6.4 spec.) > +/// > +#define EFI_ACPI_6_4_SYSTEM_LOCALITY_INFORMATION_TABLE_REVISION 0x01 > + > +/// > +/// Relative Distance Info Node Structure > +/// > +/* > +typedef struct { > + UINT8 *Distance; > +} EFI_ACPI_6_4_REL_DISTANCE_INFO_STRUCTURE; > +*/ > +#pragma pack() > + > +#endif