Re: [PATCH v1 4/6] ShellPkg: acpiview: Make GTDT parsing logic data driven

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

> -----Original Message-----
> From: Krzysztof Koch <krzysztof.koch@arm.com>
> Sent: Tuesday, May 5, 2020 11:46 PM
> To: devel@edk2.groups.io
> Cc: Ni, Ray <ray.ni@intel.com>; Gao, Zhichao <zhichao.gao@intel.com>;
> Sami.Mujawar@arm.com; Laura.Moretta@arm.comMatteo.Carlini@arm.com;
> nd@arm.com
> Subject: [PATCH v1 4/6] ShellPkg: acpiview: Make GTDT parsing logic data driven
> 
> Replace the switch statement in the main parser loop with a table-driven
> approach. Use the ParseAcpiStruct () method to resolve how each Platform Timer
> Structure given should be parsed.
> 
> Enumerate all structures found in the Generic Timer Description Table
> (GTDT) on a per-type basis. Print the offset from the start of the table for each
> structure.
> 
> Consolidate all metadata about each Platform Timer Structure.
> Define an array of ACPI_STRUCT_INFO structures to store the name, instance
> count, architecture support and handling information. Use this array to construct
> the ACPI_STRUCT_DATABASE for GTDT.
> 
> Count the number of instances of each Platform Timer Structure type.
> Optionally report these counts after GTDT table parsing is finished.
> 
> Modify DumpGTBlock () funtion signature so that it matches that of
> ACPI_STRUCT_PARSER_FUNC. This way, the function can be used in the
> ParseAcpiStruct () call.
> 
> Remove the definition of the DumpWatchdogTimer (). Its only purpose was to call
> ParseAcpi () and now this process is streamlined.
> 
> References:
> - ACPI 6.3 Specification - January 2019, Section 5.2.24
> 
> Signed-off-by: Krzysztof Koch <krzysztof.koch@arm.com>
> ---
> 
> Notes:
>     v1:
>     - Make GTDT parsing logic data driven [Krzysztof]
> 
>  ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Gtdt/GtdtParser.c |
> 123 ++++++++++++--------
>  1 file changed, 77 insertions(+), 46 deletions(-)
> 
> diff --git
> a/ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Gtdt/GtdtParser.c
> b/ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Gtdt/GtdtParser.c
> index
> bdd30ff45c61142c071ead63a27babab8998721b..9a9f8fda442081507768b1540f0
> b9b3c6c254329 100644
> --- a/ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Gtdt/GtdtParser.c
> +++ b/ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Gtdt/GtdtPars
> +++ er.c
> @@ -9,13 +9,20 @@
>    **/
> 
>  #include <IndustryStandard/Acpi.h>
> +#include <Library/PrintLib.h>
>  #include <Library/UefiLib.h>
>  #include "AcpiParser.h"
>  #include "AcpiTableParser.h"
> +#include "AcpiView.h"
> 
>  // "The number of GT Block Timers must be less than or equal to 8"
>  #define GT_BLOCK_TIMER_COUNT_MAX 8
> 
> +/**
> +  Handler for each Platform Timer Structure type **/ STATIC
> +ACPI_STRUCT_INFO GtdtStructs[];

It is illegal for most of the compilers to declare an array without size. For VS, it would cause a build error. Two method to solve:
1. define the array without handler field initialization. Update the field at the beginning of the parser function
2. put the required function declaration before the array definition.

Same in the patch #5 & #6.

Thanks,
Zhichao

> +
>  // Local variables
>  STATIC CONST UINT32* GtdtPlatformTimerCount;  STATIC CONST UINT32*
> GtdtPlatformTimerOffset; @@ -167,23 +174,35 @@ STATIC CONST
> ACPI_PARSER SBSAGenericWatchdogParser[] = {
>  /**
>    This function parses the Platform GT Block.
> 
> -  @param [in] Ptr       Pointer to the start of the GT Block data.
> -  @param [in] Length    Length of the GT Block structure.
> +  @param [in] Ptr         Pointer to the start of structure's buffer.
> +  @param [in] Length      Length of the buffer.
> +  @param [in] OptArg0     First optional argument (Not used).
> +  @param [in] OptArg1     Second optional argument (Not used).
>  **/
>  STATIC
>  VOID
>  DumpGTBlock (
> -  IN UINT8* Ptr,
> -  IN UINT16 Length
> +  IN       UINT8* Ptr,
> +  IN       UINT32 Length,
> +  IN CONST VOID*  OptArg0 OPTIONAL,
> +  IN CONST VOID*  OptArg1 OPTIONAL
>    )
>  {
>    UINT32 Index;
>    UINT32 Offset;
> +  CHAR8  Buffer[80];
> +
> +  PrintAcpiStructName (
> +    GtdtStructs[EFI_ACPI_6_3_GTDT_GT_BLOCK].Name,
> +    GtdtStructs[EFI_ACPI_6_3_GTDT_GT_BLOCK].Count,
> +    sizeof (Buffer),
> +    Buffer
> +    );
> 
>    ParseAcpi (
>      TRUE,
>      2,
> -    "GT Block",
> +    Buffer,
>      Ptr,
>      Length,
>      PARSER_PARAMS (GtBlockParser)
> @@ -195,7 +214,8 @@ DumpGTBlock (
>        (GtBlockTimerOffset == NULL)) {
>      IncrementErrorCount ();
>      Print (
> -      L"ERROR: Insufficient GT Block Structure length. Length = %d.\n",
> +      L"ERROR: Insufficient %a Structure length. Length = %d.\n",
> +      GtdtStructs[EFI_ACPI_6_3_GTDT_GT_BLOCK].Name,
>        Length
>        );
>      return;
> @@ -206,41 +226,47 @@ DumpGTBlock (
> 
>    // Parse the specified number of GT Block Timer Structures or the GT Block
>    // Structure buffer length. Whichever is minimum.
> -  while ((Index++ < *GtBlockTimerCount) &&
> +  while ((Index < *GtBlockTimerCount) &&
>           (Offset < Length)) {
> +    PrintAcpiStructName ("GT Block Timer", Index, sizeof (Buffer),
> + Buffer);
>      Offset += ParseAcpi (
>                  TRUE,
> -                2,
> -                "GT Block Timer",
> +                4,
> +                Buffer,
>                  Ptr + Offset,
>                  Length - Offset,
>                  PARSER_PARAMS (GtBlockTimerParser)
>                  );
> +    Index++;
>    }
>  }
> 
>  /**
> -  This function parses the Platform Watchdog timer.
> -
> -  @param [in] Ptr     Pointer to the start of the watchdog timer data.
> -  @param [in] Length  Length of the watchdog timer structure.
> +  Information about each Platform Timer Structure type.
>  **/
> -STATIC
> -VOID
> -DumpWatchdogTimer (
> -  IN UINT8* Ptr,
> -  IN UINT16 Length
> -  )
> -{
> -  ParseAcpi (
> -    TRUE,
> -    2,
> +STATIC ACPI_STRUCT_INFO GtdtStructs[] = {
> +  ADD_ACPI_STRUCT_INFO_FUNC (
> +    "GT Block",
> +    EFI_ACPI_6_3_GTDT_GT_BLOCK,
> +    ARCH_COMPAT_ARM | ARCH_COMPAT_AARCH64,
> +    DumpGTBlock
> +    ),
> +  ADD_ACPI_STRUCT_INFO_ARRAY (
>      "SBSA Generic Watchdog",
> -    Ptr,
> -    Length,
> -    PARSER_PARAMS (SBSAGenericWatchdogParser)
> -    );
> -}
> +    EFI_ACPI_6_3_GTDT_SBSA_GENERIC_WATCHDOG,
> +    ARCH_COMPAT_ARM | ARCH_COMPAT_AARCH64,
> +    SBSAGenericWatchdogParser
> +    )
> +};
> +
> +/**
> +  GTDT structure database
> +**/
> +STATIC ACPI_STRUCT_DATABASE GtdtDatabase = {
> +  "Platform Timer Structure",
> +  GtdtStructs,
> +  ARRAY_SIZE (GtdtStructs)
> +};
> 
>  /**
>    This function parses the ACPI GTDT table.
> @@ -275,6 +301,8 @@ ParseAcpiGtdt (
>      return;
>    }
> 
> +  ResetAcpiStructCounts (&GtdtDatabase);
> +
>    ParseAcpi (
>      TRUE,
>      0,
> @@ -321,7 +349,8 @@ ParseAcpiGtdt (
>        IncrementErrorCount ();
>        Print (
>          L"ERROR: Insufficient remaining table buffer length to read the " \
> -          L"Platform Timer Structure header. Length = %d.\n",
> +          L"%a header. Length = %d.\n",
> +        GtdtDatabase.Name,
>          AcpiTableLength - Offset
>          );
>        return;
> @@ -332,8 +361,9 @@ ParseAcpiGtdt (
>          ((Offset + (*PlatformTimerLength)) > AcpiTableLength)) {
>        IncrementErrorCount ();
>        Print (
> -        L"ERROR: Invalid Platform Timer Structure length. " \
> -          L"Length = %d. Offset = %d. AcpiTableLength = %d.\n",
> +        L"ERROR: Invalid %a length. Length = %d. Offset = %d. " \
> +          L"AcpiTableLength = %d.\n",
> +        GtdtDatabase.Name,
>          *PlatformTimerLength,
>          Offset,
>          AcpiTableLength
> @@ -341,23 +371,24 @@ ParseAcpiGtdt (
>        return;
>      }
> 
> -    switch (*PlatformTimerType) {
> -      case EFI_ACPI_6_3_GTDT_GT_BLOCK:
> -        DumpGTBlock (TimerPtr, *PlatformTimerLength);
> -        break;
> -      case EFI_ACPI_6_3_GTDT_SBSA_GENERIC_WATCHDOG:
> -        DumpWatchdogTimer (TimerPtr, *PlatformTimerLength);
> -        break;
> -      default:
> -        IncrementErrorCount ();
> -        Print (
> -          L"ERROR: Invalid Platform Timer Type = %d\n",
> -          *PlatformTimerType
> -          );
> -        break;
> -    } // switch
> +    // Parse the Platform Timer Structure
> +    ParseAcpiStruct (
> +      2,
> +      TimerPtr,
> +      &GtdtDatabase,
> +      Offset,
> +      *PlatformTimerType,
> +      *PlatformTimerLength,
> +      NULL,
> +      NULL
> +      );
> 
>      TimerPtr += *PlatformTimerLength;
>      Offset += *PlatformTimerLength;
>    } // while
> +
> +  // Report and validate Platform Timer Type Structure counts  if
> + (GetConsistencyChecking ()) {
> +    ValidateAcpiStructCounts (&GtdtDatabase);  }
>  }
> --
> 'Guid(CE165669-3EF3-493F-B85D-6190EE5B9759)'