Re: [PATCH v3 7/7] DynamicTablesPkg: Add CacheId to PPTT generator

["Sami Mujawar" <sami.mujawar@arm.com>]

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@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 ((