Re: [edk2-devel] [PATCH v2 4/7] ShellPkg: Add Cache ID to PPTT parser

["Gao, Zhichao" <zhichao.gao@intel.com>]

Reviewed-by: Zhichao Gao <zhichao.gao@intel.com>

Thanks,
Zhichao

> -----Original Message-----
> From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Chris
> Jones
> Sent: Wednesday, November 3, 2021 11:41 PM
> To: devel@edk2.groups.io
> Cc: Kinney, Michael D <michael.d.kinney@intel.com>;
> gaoliming@byosoft.com.cn; Liu, Zhiguang <zhiguang.liu@intel.com>; Ni, Ray
> <ray.ni@intel.com>; Gao, Zhichao <zhichao.gao@intel.com>;
> Alexei.Fedorov@arm.com; Sami.Mujawar@arm.com; nd@arm.com
> Subject: [edk2-devel] [PATCH v2 4/7] ShellPkg: Add Cache ID to PPTT parser
> 
> Bugzilla: 3697 (https://bugzilla.tianocore.org/show_bug.cgi?id=3697)
> 
> Update the Acpiview PPTT parser with the Cache ID field and relevant
> validations as defined in tables 5.140 and 5.141 of the ACPI 6.4 specification.
> 
> Signed-off-by: Chris Jones <christopher.jones@arm.com>
> ---
> 
> Notes:
>     v2:
>     - Fixed a bug where 'CacheFlags' and 'CacheId' were only set after the
>       validation function had finished. Instead set them inside the
>       validation function using the first 'Ptr' parameter.
> 
>  ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Pptt/PpttParser.c
> | 120 +++++++++++++++++++-
>  1 file changed, 118 insertions(+), 2 deletions(-)
> 
> diff --git
> a/ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Pptt/PpttParser.
> c
> b/ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Pptt/PpttParser.
> c
> index
> bb840a0dbab240d371aa58d323e61f47fa8d1587..3f93038ce1d83c005ae3d6a43
> e11f309440ad6fa 100644
> ---
> a/ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Pptt/PpttParser.
> c
> +++
> b/ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Pptt/PpttPars
> +++ er.c
> @@ -20,8 +20,84 @@
>  STATIC CONST UINT8*  ProcessorTopologyStructureType;  STATIC CONST
> UINT8*  ProcessorTopologyStructureLength;  STATIC CONST UINT32*
> NumberOfPrivateResources;
> +STATIC CONST EFI_ACPI_6_4_PPTT_STRUCTURE_CACHE_FLAGS*
> CacheFlags;
>  STATIC ACPI_DESCRIPTION_HEADER_INFO AcpiHdrInfo;
> 
> +/**
> +  Increment the error count and print an error that a required flag is missing.
> +
> +  @param [in] FlagName  Name of the missing flag.
> +**/
> +STATIC
> +VOID
> +EFIAPI
> +LogCacheFlagError (
> +  IN CONST CHAR16* FlagName
> +  )
> +{
> +  IncrementErrorCount ();
> +  Print (
> +      L"\nERROR: On Arm based systems, all cache properties must be"
> +        L"provided in the cache type structure."
> +        L"Missing '%s' flag.",
> +      *FlagName
> +      );
> +}
> +
> +/**
> +  This function validates the Cache Type Structure (Type 1) Cache Flags field.
> +
> +  @param [in] Ptr     Pointer to the start of the field data.
> +  @param [in] Context Pointer to context specific information e.g. this
> +                      could be a pointer to the ACPI table header.
> +**/
> +STATIC
> +VOID
> +EFIAPI
> +ValidateCacheFlags (
> +  IN UINT8* Ptr,
> +  IN VOID*  Context
> +  )
> +{
> +#if defined(MDE_CPU_ARM) || defined(MDE_CPU_AARCH64)
> +  CacheFlags = (EFI_ACPI_6_4_PPTT_STRUCTURE_CACHE_FLAGS*)Ptr;
> +
> +  if (CacheFlags == NULL) {
> +    IncrementErrorCount ();
> +    Print (L"\nERROR: Cache Structure Flags were not successfully read.");
> +    return;
> +  }
> +
> +  if (CacheFlags->SizePropertyValid ==
> EFI_ACPI_6_4_PPTT_CACHE_SIZE_INVALID) {
> +    LogCacheFlagError (L"Size Property Valid");
> +  }
> +  if (CacheFlags->NumberOfSetsValid ==
> EFI_ACPI_6_4_PPTT_NUMBER_OF_SETS_INVALID) {
> +    LogCacheFlagError (L"Number Of Sets Valid");
> +  }
> +  if (CacheFlags->AssociativityValid ==
> EFI_ACPI_6_4_PPTT_ASSOCIATIVITY_INVALID) {
> +    LogCacheFlagError (L"Associativity Valid");
> +  }
> +  if (CacheFlags->AllocationTypeValid ==
> EFI_ACPI_6_4_PPTT_ALLOCATION_TYPE_INVALID) {
> +    LogCacheFlagError (L"Allocation Type Valid");
> +  }
> +  if (CacheFlags->CacheTypeValid ==
> EFI_ACPI_6_4_PPTT_CACHE_TYPE_INVALID) {
> +    LogCacheFlagError (L"Cache Type Valid");
> +  }
> +  if (CacheFlags->WritePolicyValid ==
> EFI_ACPI_6_4_PPTT_WRITE_POLICY_INVALID) {
> +    LogCacheFlagError (L"Write Policy Valid");
> +  }
> +  if (CacheFlags->LineSizeValid == EFI_ACPI_6_4_PPTT_LINE_SIZE_INVALID)
> {
> +    LogCacheFlagError (L"Line Size Valid");
> +  }
> +  // Cache ID was only introduced in revision 3
> +  if (*(AcpiHdrInfo.Revision) >= 3) {
> +    if (CacheFlags->CacheIdValid == EFI_ACPI_6_4_PPTT_CACHE_ID_INVALID)
> {
> +      LogCacheFlagError (L"Cache Id Valid");
> +    }
> +  }
> +#endif
> +}
> +
>  /**
>    This function validates the Cache Type Structure (Type 1) 'Number of sets'
>    field.
> @@ -141,6 +217,44 @@ ValidateCacheLineSize (  #endif  }
> 
> +/**
> +  This function validates the Cache Type Structure (Type 1) Cache ID field.
> +
> +  @param [in] Ptr     Pointer to the start of the field data.
> +  @param [in] Context Pointer to context specific information e.g. this
> +                      could be a pointer to the ACPI table header.
> +**/
> +STATIC
> +VOID
> +EFIAPI
> +ValidateCacheId (
> +  IN UINT8* Ptr,
> +  IN VOID*  Context
> +  )
> +{
> +  UINT32 CacheId;
> +  CacheId = *(UINT32*)Ptr;
> +
> +  // Cache ID was only introduced in revision 3  if
> + (*(AcpiHdrInfo.Revision) < 3) {
> +    return;
> +  }
> +
> +  if (CacheFlags == NULL) {
> +    IncrementErrorCount ();
> +    Print (L"\nERROR: Cache Structure Flags were not successfully read.");
> +    return;
> +  }
> +
> +  if (CacheFlags->CacheIdValid == EFI_ACPI_6_4_PPTT_CACHE_ID_VALID) {
> +    if (CacheId == 0) {
> +      IncrementErrorCount ();
> +      Print (L"\nERROR: 0 is not a valid Cache ID.");
> +      return;
> +    }
> +  }
> +}
> +
>  /**
>    This function validates the Cache Type Structure (Type 1) Attributes field.
> 
> @@ -213,13 +327,15 @@ STATIC CONST ACPI_PARSER
> CacheTypeStructureParser[] = {
>    {L"Length", 1, 1, L"%d", NULL, NULL, NULL, NULL},
>    {L"Reserved", 2, 2, L"0x%x", NULL, NULL, NULL, NULL},
> 
> -  {L"Flags", 4, 4, L"0x%x", NULL, NULL, NULL, NULL},
> +  {L"Flags", 4, 4, L"0x%x", NULL, (VOID**)&CacheFlags, ValidateCacheFlags,
> +   NULL},
>    {L"Next Level of Cache", 4, 8, L"0x%x", NULL, NULL, NULL, NULL},
>    {L"Size", 4, 12, L"0x%x", NULL, NULL, NULL, NULL},
>    {L"Number of sets", 4, 16, L"%d", NULL, NULL, ValidateCacheNumberOfSets,
> NULL},
>    {L"Associativity", 1, 20, L"%d", NULL, NULL, ValidateCacheAssociativity,
> NULL},
>    {L"Attributes", 1, 21, L"0x%x", NULL, NULL, ValidateCacheAttributes, NULL},
> -  {L"Line size", 2, 22, L"%d", NULL, NULL, ValidateCacheLineSize, NULL}
> +  {L"Line size", 2, 22, L"%d", NULL, NULL, ValidateCacheLineSize,
> + NULL},  {L"Cache ID", 4, 24, L"%d", NULL, NULL, ValidateCacheId, NULL}
>  };
> 
>  /**
> --
> Guid("CE165669-3EF3-493F-B85D-6190EE5B9759")
> 
> 
> 
> 
>