Hi Sami, The optimisation looks good to me and I would be glad if you added it to the patch. Thanks, Chris ________________________________ From: Sami Mujawar Sent: Thursday, December 9, 2021 1:19 PM To: Christopher Jones ; devel@edk2.groups.io Cc: michael.d.kinney@intel.com ; gaoliming@byosoft.com.cn ; zhiguang.liu@intel.com ; ray.ni@intel.com ; zhichao.gao@intel.com ; Alexei Fedorov ; nd Subject: Re: [PATCH v3 7/7] DynamicTablesPkg: Add CacheId to PPTT generator Hi Chris, Thank you for this patch. Please see my feedback below inline marked [SAMI]. Regards, Sami Mujawar On 08/12/2021 04:06 PM, Chris Jones wrote: > Bugzilla: 3697 (https://bugzilla.tianocore.org/show_bug.cgi?id=3697) > > Update the PPTT generator with the CacheId field as defined in table > 5.140 of the ACPI 6.4 specification. > > Also add validations to ensure that the cache id generated is unique. > > Signed-off-by: Chris Jones > --- > DynamicTablesPkg/Include/ArmNameSpaceObjects.h | 4 +- > DynamicTablesPkg/Library/Acpi/Arm/AcpiPpttLibArm/PpttGenerator.c | 102 ++++++++++++++++++-- > 2 files changed, 96 insertions(+), 10 deletions(-) > > diff --git a/DynamicTablesPkg/Include/ArmNameSpaceObjects.h b/DynamicTablesPkg/Include/ArmNameSpaceObjects.h > index 3246e8884723ac85340bf880a3859800726be3c1..6ea03fca487b96577b8fd8105bc3d22047ff5697 100644 > --- a/DynamicTablesPkg/Include/ArmNameSpaceObjects.h > +++ b/DynamicTablesPkg/Include/ArmNameSpaceObjects.h > @@ -741,10 +741,12 @@ typedef struct CmArmCacheInfo { > /// PPTT_ARM_CCIDX_CACHE_ASSOCIATIVITY_MAX. Therfore this field > /// is 32-bit wide. > UINT32 Associativity; > - /// Cache attributes (ACPI 6.3 - January 2019, PPTT, Table 5-156) > + /// Cache attributes (ACPI 6.4 - January 2021, PPTT, Table 5.140) > UINT8 Attributes; > /// Line size in bytes > UINT16 LineSize; > + /// Unique ID for the cache > + UINT32 CacheId; > } CM_ARM_CACHE_INFO; > > /** A structure that describes a reference to another Configuration Manager > diff --git a/DynamicTablesPkg/Library/Acpi/Arm/AcpiPpttLibArm/PpttGenerator.c b/DynamicTablesPkg/Library/Acpi/Arm/AcpiPpttLibArm/PpttGenerator.c > index 3d416ca78ec16a1929ede87abbe4f8f4464ef0cf..6b74572ea2dd8478f14d013e6cb7394216e45d8d 100644 > --- a/DynamicTablesPkg/Library/Acpi/Arm/AcpiPpttLibArm/PpttGenerator.c > +++ b/DynamicTablesPkg/Library/Acpi/Arm/AcpiPpttLibArm/PpttGenerator.c > @@ -726,6 +726,35 @@ AddProcHierarchyNodes ( > return Status; > } > > +/** > + Test whether CacheId is unique among the CacheIdList. > + > + @param [in] CacheId Cache ID to check. > + @param [in] CacheIdList List of already existing cache IDs. > + @param [in] CacheIdListSize Size of CacheIdList. > + > + @retval TRUE CacheId does not exist in CacheIdList. > + @retval FALSE CacheId already exists in CacheIdList. > +**/ > +STATIC > +BOOLEAN > +IsCacheIdUnique ( > + IN CONST UINT32 CacheId, > + IN CONST UINT32 *CacheIdList, > + IN CONST UINT32 CacheIdListSize > + ) > +{ > + UINT32 Index; > + > + for (Index = 0; Index < CacheIdListSize; Index++) { > + if (CacheIdList[Index] == CacheId) { > + return FALSE; > + } > + } > + > + return TRUE; > +} > + > /** > Update the Cache Type Structure (Type 1) information. > > @@ -738,10 +767,12 @@ AddProcHierarchyNodes ( > @param [in] Pptt Pointer to PPTT table structure. > @param [in] NodesStartOffset Offset from the start of PPTT table to the > start of Cache Type Structures. > + @param [in] Revision Revision of the PPTT table being requested. > > @retval EFI_SUCCESS Structures updated successfully. > @retval EFI_INVALID_PARAMETER A parameter is invalid. > @retval EFI_NOT_FOUND A required object was not found. > + @retval EFI_OUT_OF_RESOURCES Out of resources. > **/ > STATIC > EFI_STATUS > @@ -749,7 +780,8 @@ AddCacheTypeStructures ( > IN CONST ACPI_PPTT_GENERATOR *CONST Generator, > IN CONST EDKII_CONFIGURATION_MANAGER_PROTOCOL *CONST CfgMgrProtocol, > IN CONST EFI_ACPI_6_4_PROCESSOR_PROPERTIES_TOPOLOGY_TABLE_HEADER *Pptt, > - IN CONST UINT32 NodesStartOffset > + IN CONST UINT32 NodesStartOffset, > + IN CONST UINT32 Revision > ) > { > EFI_STATUS Status; > @@ -758,6 +790,9 @@ AddCacheTypeStructures ( > CM_ARM_CACHE_INFO *CacheInfoNode; > PPTT_NODE_INDEXER *CacheNodeIterator; > UINT32 NodeCount; > + BOOLEAN CacheIdUnique; > + UINT32 TotalNodeCount; [SAMI] I think we could do slight optimisation by doing the following below: (a) change TotalNode Count to NodeIndex. > + UINT32 *FoundCacheIds; > > ASSERT ( > (Generator != NULL) && > @@ -770,6 +805,13 @@ AddCacheTypeStructures ( > > CacheNodeIterator = Generator->CacheStructIndexedList; > NodeCount = Generator->CacheStructCount; > + TotalNodeCount = NodeCount; > + > + FoundCacheIds = AllocateZeroPool (TotalNodeCount * sizeof (*FoundCacheIds)); [SAMI] (b) Change TotalNodeCount to NodeCount. > + if (FoundCacheIds == NULL) { > + DEBUG ((DEBUG_ERROR, "ERROR: PPTT: Failed to allocate resources.\n")); > + return EFI_OUT_OF_RESOURCES; > + } > > while (NodeCount-- != 0) { [SAMI] (c) Replace while loop to a for loop where NodeIndes = 0 to NodeCount. > CacheInfoNode = (CM_ARM_CACHE_INFO *)CacheNodeIterator->Object; > @@ -789,6 +831,7 @@ AddCacheTypeStructures ( > CacheStruct->Flags.CacheTypeValid = 1; > CacheStruct->Flags.WritePolicyValid = 1; > CacheStruct->Flags.LineSizeValid = 1; > + CacheStruct->Flags.CacheIdValid = 1; > CacheStruct->Flags.Reserved = 0; > > // Populate the reference to the next level of cache > @@ -811,7 +854,7 @@ AddCacheTypeStructures ( > CacheInfoNode->Token, > Status > )); > - return Status; > + goto cleanup; > } > > // Update Cache Structure with the offset for the next level of cache > @@ -835,7 +878,7 @@ AddCacheTypeStructures ( > CacheInfoNode->NumberOfSets, > Status > )); > - return Status; > + goto cleanup; > } > > if (CacheInfoNode->NumberOfSets > PPTT_ARM_CACHE_NUMBER_OF_SETS_MAX) { > @@ -862,7 +905,7 @@ AddCacheTypeStructures ( > CacheInfoNode->Associativity, > Status > )); > - return Status; > + goto cleanup; > } > > // Validate the Associativity field based on the architecture specification > @@ -881,7 +924,7 @@ AddCacheTypeStructures ( > CacheInfoNode->Associativity, > Status > )); > - return Status; > + goto cleanup; > } > > if (CacheInfoNode->Associativity > PPTT_ARM_CACHE_ASSOCIATIVITY_MAX) { > @@ -923,7 +966,7 @@ AddCacheTypeStructures ( > CacheInfoNode->LineSize, > Status > )); > - return Status; > + goto cleanup; > } > > if ((CacheInfoNode->LineSize & (CacheInfoNode->LineSize - 1)) != 0) { > @@ -935,18 +978,58 @@ AddCacheTypeStructures ( > CacheInfoNode->LineSize, > Status > )); > - return Status; > + goto cleanup; > } > > CacheStruct->LineSize = CacheInfoNode->LineSize; > > + if (Revision >= 3) { > + // Validate and populate cache id > + if (CacheInfoNode->CacheId == 0) { > + Status = EFI_INVALID_PARAMETER; > + DEBUG (( > + DEBUG_ERROR, > + "ERROR: PPTT: The cache id cannot be zero. Status = %r\n", > + Status > + )); > + goto cleanup; > + } > + > + CacheIdUnique = IsCacheIdUnique ( > + CacheInfoNode->CacheId, > + FoundCacheIds, > + TotalNodeCount [SAMI] (d) Replace TotalNodeCount with NodeIndex in call above. By doing (a) to (d) and (e) below we can reduce the number of iterations in IsCacheIdUnique (). If you agree I will make the adjustments before pushing this patch series. [/SAMI] > + ); > + if (!CacheIdUnique) { > + Status = EFI_INVALID_PARAMETER; > + DEBUG (( > + DEBUG_ERROR, > + "ERROR: PPTT: The cache id is not unique. " \ > + "CacheId = %d. Status = %r\n", > + CacheInfoNode->CacheId, > + Status > + )); > + goto cleanup; > + } > + > + // Store the cache id so we can check future cache ids for uniqueness > + FoundCacheIds[NodeCount] = CacheInfoNode->CacheId; [SAMI] (e) Replace NodeCount with NodeIndex above. > + > + CacheStruct->CacheId = CacheInfoNode->CacheId; > + } > + > // Next Cache Type Structure > CacheStruct = (EFI_ACPI_6_4_PPTT_STRUCTURE_CACHE *)((UINT8 *)CacheStruct + > CacheStruct->Length); > CacheNodeIterator++; > } // Cache Type Structure > > - return EFI_SUCCESS; > + Status = EFI_SUCCESS; > + > +cleanup: > + FreePool (FoundCacheIds); > + > + return Status; > } > > /** > @@ -1205,7 +1288,8 @@ BuildPpttTable ( > Generator, > CfgMgrProtocol, > Pptt, > - CacheStructOffset > + CacheStructOffset, > + AcpiTableInfo->AcpiTableRevision > ); > if (EFI_ERROR (Status)) { > DEBUG ((