Re: [PATCH v2 1/1] ShellPkg: acpiview: Add GT Frame Number validation to GTDT parser

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

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

> -----Original Message-----
> From: Krzysztof Koch [mailto:krzysztof.koch@arm.com]
> Sent: Thursday, May 16, 2019 4:44 PM
> To: devel@edk2.groups.io
> Cc: Carsey, Jaben <jaben.carsey@intel.com>; Ni, Ray <ray.ni@intel.com>;
> Gao, Zhichao <zhichao.gao@intel.com>; Sami.Mujawar@arm.com;
> nd@arm.com
> Subject: [PATCH v2 1/1] ShellPkg: acpiview: Add GT Frame Number validation
> to GTDT parser
> 
> The ACPI 6.2 specification mandates that the Generic Timer (GT) Block Timer
> Structures must have a frame number in the range 0-7.
> 
> Update the GTDT parser to warn if this condition is violated.
> 
> Signed-off-by: Krzysztof Koch <krzysztof.koch@arm.com>
> ---
> 
> The changes can be seen at:
> https://github.com/KrzysztofKoch1/edk2/tree/528_acpiview_gt_frame_vali
> date_v2
> 
> Notes:
>     v2:
>     - Change FrameNumber data type to UINT8 [Zhichao]
>     - Modify the patch based on upstream review [Krzysztof]
> 
>  ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Gtdt/GtdtParser.c
> | 47 +++++++++++++++++++-
>  1 file changed, 45 insertions(+), 2 deletions(-)
> 
> diff --git
> a/ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Gtdt/GtdtParser
> .c
> b/ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Gtdt/GtdtParser
> .c
> index
> f31c4a2761751c58d4b1d3eb75084e24ec318e7f..1b7e56486c8fb98a8fe063ae5f
> a25d86500a58a9 100644
> ---
> a/ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Gtdt/GtdtParser
> .c
> +++
> b/ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Gtdt/GtdtPars
> +++ er.c
> @@ -1,7 +1,7 @@
>  /** @file
>    GTDT table parser
> 
> -  Copyright (c) 2016 - 2018, ARM Limited. All rights reserved.
> +  Copyright (c) 2016 - 2019, ARM Limited. All rights reserved.
>    SPDX-License-Identifier: BSD-2-Clause-Patent
> 
>    @par Reference(s):
> @@ -38,6 +38,21 @@ ValidateGtBlockTimerCount (
>    IN VOID*  Context
>    );
> 
> +/**
> +  This function validates the GT Frame Number.
> +
> +  @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
> +ValidateGtFrameNumber (
> +  IN UINT8* Ptr,
> +  IN VOID*  Context
> +  );
> +
>  /**
>    An ACPI_PARSER array describing the ACPI GTDT Table.
>  **/
> @@ -92,7 +107,7 @@ STATIC CONST ACPI_PARSER GtBlockParser[] = {
>    An ACPI_PARSER array describing the GT Block timer.
>  **/
>  STATIC CONST ACPI_PARSER GtBlockTimerParser[] = {
> -  {L"Frame Number", 1, 0, L"%d", NULL, NULL, NULL, NULL},
> +  {L"Frame Number", 1, 0, L"%d", NULL, NULL, ValidateGtFrameNumber,
> + NULL},
>    {L"Reserved", 3, 1, L"%x %x %x", Dump3Chars, NULL, NULL, NULL},
>    {L"Physical address (CntBaseX)", 8, 4, L"0x%lx", NULL, NULL, NULL, NULL},
>    {L"Physical address (CntEL0BaseX)", 8, 12, L"0x%lx", NULL, NULL, NULL, @@
> -145,6 +160,34 @@ ValidateGtBlockTimerCount (
>    }
>  }
> 
> +/**
> +  This function validates the GT Frame Number.
> +
> +  @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
> +ValidateGtFrameNumber (
> +  IN UINT8* Ptr,
> +  IN VOID*  Context
> +  )
> +{
> +  UINT8 FrameNumber;
> +
> +  FrameNumber = *(UINT8*)Ptr;
> +
> +  if (FrameNumber > 7) {
> +    IncrementErrorCount ();
> +    Print (
> +      L"\nERROR: GT Frame Number = %d. GT Frame Number must be in
> range 0-7.",
> +      FrameNumber
> +      );
> +  }
> +}
> +
>  /**
>    This function parses the Platform GT Block.
> 
> --
> 'Guid(CE165669-3EF3-493F-B85D-6190EE5B9759)'
>