Re: [edk2-devel] [PATCH v1 1/1] ShellPkg: acpiview: Add GT Frame Number validation to GTDT parser

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

Sorry for missing this email.

Some minor comments below.
> -----Original Message-----
> From: devel@edk2.groups.io [mailto:devel@edk2.groups.io] On Behalf Of
> Krzysztof Koch
> Sent: Wednesday, May 8, 2019 9:44 PM
> To: devel@edk2.groups.io
> Cc: Carsey, Jaben <jaben.carsey@intel.com>; Ni, Ray <ray.ni@intel.com>;
> Sami.Mujawar@arm.com; Girish.Pathak@arm.com;
> Pierre.Gondois@arm.com; Matteo.Carlini@arm.com; Stephanie.Hughes-
> Fitt@arm.com; nd@arm.com
> Subject: [edk2-devel] [PATCH v1 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/woa_528_acpiview_gt_fram
> e_validate_v1
> 
> Notes:
>     v1:
>       - Add GTDT Frame Number checks [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..572e52385c54932b14630481e
> 85a67d0b211966a 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
> +  )
> +{
> +  UINT32 FrameNumber;

Refer to ACPI spec and the code blow, seems UINT8 is enough. I know it would work fine with a larger type.
Maybe I am overwrought.

Thanks,
Zhichao

> +
> +  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)'
> 
> 
> IMPORTANT NOTICE: The contents of this email and any attachments are
> confidential and may also be privileged. If you are not the intended recipient,
> please notify the sender immediately and do not disclose the contents to any
> other person, use it for any purpose, or store or copy the information in any
> medium. Thank you.
> 
>