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

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

[Krzysztof Koch]

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_frame_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..572e52385c54932b14630481e85a67d0b211966a 100644
--- a/ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Gtdt/GtdtParser.c
+++ b/ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Gtdt/GtdtParser.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;
+
+  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.

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

[Sami Mujawar]

Reviewed-by: Sami Mujawar <sami.mujawar@arm.com>

Regards,

Sami Mujawar

-----Original Message-----
From: Krzysztof Koch <krzysztof.koch@arm.com> 
Sent: 08 May 2019 02:44 PM
To: devel@edk2.groups.io
Cc: jaben.carsey@intel.com; ray.ni@intel.com; Sami Mujawar <Sami.Mujawar@arm.com>; Girish Pathak <Girish.Pathak@arm.com>; Pierre Gondois <Pierre.Gondois@arm.com>; Matteo Carlini <Matteo.Carlini@arm.com>; Stephanie Hughes-Fitt <Stephanie.Hughes-Fitt@arm.com>; nd <nd@arm.com>
Subject: [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_frame_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..572e52385c54932b14630481e85a67d0b211966a 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;
+
+  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)'

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

[Krzysztof Koch]

Hi Jaben and Ray,

Is there anything I can do to help get this patch merged?

Kind regards,

Krzysztof Koch

-----Original Message-----
From: Sami Mujawar <Sami.Mujawar@arm.com> 
Sent: Thursday, May 9, 2019 14:37
To: devel@edk2.groups.io
Cc: jaben.carsey@intel.com; ray.ni@intel.com; Krzysztof Koch <Krzysztof.Koch@arm.com>; nd <nd@arm.com>
Subject: RE: [PATCH v1 1/1] ShellPkg: acpiview: Add GT Frame Number validation to GTDT parser

Reviewed-by: Sami Mujawar <sami.mujawar@arm.com>

Regards,

Sami Mujawar

-----Original Message-----
From: Krzysztof Koch <krzysztof.koch@arm.com> 
Sent: 08 May 2019 02:44 PM
To: devel@edk2.groups.io
Cc: jaben.carsey@intel.com; ray.ni@intel.com; Sami Mujawar <Sami.Mujawar@arm.com>; Girish Pathak <Girish.Pathak@arm.com>; Pierre Gondois <Pierre.Gondois@arm.com>; Matteo Carlini <Matteo.Carlini@arm.com>; Stephanie Hughes-Fitt <Stephanie.Hughes-Fitt@arm.com>; nd <nd@arm.com>
Subject: [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_frame_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..572e52385c54932b14630481e85a67d0b211966a 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;
+
+  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)'

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

[Gao, Zhichao]

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.
> 
>