[edk2-devel] [PATCH v3 7/7] DynamicTablesPkg: Add CacheId to PPTT generator
Chris Jones
christopher.jones at arm.com
Thu Dec 9 13:42:32 UTC 2021
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 <Sami.Mujawar at arm.com>
Sent: Thursday, December 9, 2021 1:19 PM
To: Christopher Jones <Christopher.Jones at arm.com>; devel at edk2.groups.io <devel at edk2.groups.io>
Cc: michael.d.kinney at intel.com <michael.d.kinney at intel.com>; gaoliming at byosoft.com.cn <gaoliming at byosoft.com.cn>; zhiguang.liu at intel.com <zhiguang.liu at intel.com>; ray.ni at intel.com <ray.ni at intel.com>; zhichao.gao at intel.com <zhichao.gao at intel.com>; Alexei Fedorov <Alexei.Fedorov at arm.com>; nd <nd at arm.com>
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 <christopher.jones at arm.com>
> ---
> 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 ((
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#84609): https://edk2.groups.io/g/devel/message/84609
Mute This Topic: https://groups.io/mt/87591331/1813853
Group Owner: devel+owner at edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [edk2-devel-archive at redhat.com]
-=-=-=-=-=-=-=-=-=-=-=-
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://listman.redhat.com/archives/edk2-devel-archive/attachments/20211209/7d9abb99/attachment.htm>
More information about the edk2-devel-archive
mailing list