Re: [edk2-devel] [PATCH 2/2] ShellPkg/AcpiView: APMT Parser

["Sami Mujawar" <sami.mujawar@arm.com>]

[-- Attachment #1: Type: text/plain, Size: 8330 bytes --]

Hi Jeff,

I have two minor suggestions marked inline as [SAMI].

Other than those this patch looks good to me.

With that updated,

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

Regards,

Sami Mujawar

On 27/10/2022 03:40 pm, Jeff Brasen via groups.io wrote:
> Add a new parser for the Arm Performance Monitoring Unit Table.
>
> The APMT table describes the properties of PMU support
>
> implemented by components in an Arm-based system.
>
>
>
> Signed-off-by: Jeff Brasen<jbrasen@nvidia.com>
>
> ---
>
>   .../UefiShellAcpiViewCommandLib/AcpiParser.h  |  21 ++++
>
>   .../Parsers/Apmt/ApmtParser.c                 | 105 ++++++++++++++++++
>
>   .../UefiShellAcpiViewCommandLib.c             |   1 +
>
>   .../UefiShellAcpiViewCommandLib.inf           |   1 +
>
>   4 files changed, 128 insertions(+)
>
>   create mode 100644 ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Apmt/ApmtParser.c
>
>
>
> diff --git a/ShellPkg/Library/UefiShellAcpiViewCommandLib/AcpiParser.h b/ShellPkg/Library/UefiShellAcpiViewCommandLib/AcpiParser.h
>
> index db8c88f6df..6a1de4e12b 100644
>
> --- a/ShellPkg/Library/UefiShellAcpiViewCommandLib/AcpiParser.h
>
> +++ b/ShellPkg/Library/UefiShellAcpiViewCommandLib/AcpiParser.h
>
> @@ -531,6 +531,27 @@ ParseAcpiAest (
>
>     IN UINT8    AcpiTableRevision
>
>     );
>
>   
>
> +/**
>
> +  This function parses the ACPI APMT table.
>
> +  When trace is enabled this function parses the APMT table and
>
> +  traces the ACPI table fields.
>
> +
>
> +  This function also performs validation of the ACPI table fields.
>
> +
>
> +  @param [in] Trace              If TRUE, trace the ACPI fields.
>
> +  @param [in] Ptr                Pointer to the start of the buffer.
>
> +  @param [in] AcpiTableLength    Length of the ACPI table.
>
> +  @param [in] AcpiTableRevision  Revision of the ACPI table.
>
> +**/
>
> +VOID
>
> +EFIAPI
>
> +ParseAcpiApmt (
>
> +  IN BOOLEAN  Trace,
>
> +  IN UINT8    *Ptr,
>
> +  IN UINT32   AcpiTableLength,
>
> +  IN UINT8    AcpiTableRevision
>
> +  );
>
> +
>
>   /**
>
>     This function parses the ACPI BGRT table.
>
>     When trace is enabled this function parses the BGRT table and
>
> diff --git a/ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Apmt/ApmtParser.c b/ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Apmt/ApmtParser.c
>
> new file mode 100644
>
> index 0000000000..b036cd12d3
>
> --- /dev/null
>
> +++ b/ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Apmt/ApmtParser.c
>
> @@ -0,0 +1,105 @@
>
> +/** @file
>
> +  APMT table parser
>
> +
>
> +  Copyright (c) 2022, NVIDIA CORPORATION. All rights reserved.
>
> +  Copyright (c) 2017 - 2018, ARM Limited. All rights reserved.
>
> +  SPDX-License-Identifier: BSD-2-Clause-Patent
>
> +
>
> +  @par Reference(s):
>
> +    - ACPI 6.2 Specification - Errata A, September 2017
>
> +**/
>
> +
>
> +#include <IndustryStandard/Acpi.h>
>
> +#include <IndustryStandard/ArmPerformanceMonitoringUnitTable.h>
>
> +#include <Library/UefiLib.h>
>
> +#include "AcpiParser.h"
>
> +#include "AcpiTableParser.h"
>
> +
>
> +// Local variables
>
> +STATIC ACPI_DESCRIPTION_HEADER_INFO  AcpiHdrInfo;
>
> +STATIC CONST UINT16                  *NodeLength;
>
> +
>
> +/**
>
> +  An ACPI_PARSER array describing the ACPI APMT Table.
>
> +**/
>
> +STATIC CONST ACPI_PARSER  ApmtParser[] = {
>
> +  PARSE_ACPI_HEADER (&AcpiHdrInfo)
>
> +};
>
> +
>
> +/**
>
> +  An ACPI_PARSER array describing the ACPI Arm PMU Node.
>
> +**/
>
> +STATIC CONST ACPI_PARSER  ArmPmuNodeParser[] = {
>
> +  { L"Length",                   2, 0,  L"0x%x",  NULL, (VOID **)&NodeLength, NULL, NULL },
>
> +  { L"Node flags",               1, 2,  L"0x%x",  NULL, NULL,                 NULL, NULL },
>
> +  { L"Node type",                1, 3,  L"0x%x",  NULL, NULL,                 NULL, NULL },
>
> +  { L"Identifier",               4, 4,  L"0x%x",  NULL, NULL,                 NULL, NULL },
>
> +  { L"Node Instance primary",    8, 8,  L"0x%lx", NULL, NULL,                 NULL, NULL },
>
> +  { L"Node Instance secondary",  4, 16, L"0x%x",  NULL, NULL,                 NULL, NULL },
>
> +  { L"Base address 0",           8, 20, L"0x%lx", NULL, NULL,                 NULL, NULL },
>
> +  { L"Base address 1",           8, 28, L"0x%lx", NULL, NULL,                 NULL, NULL },
>
> +  { L"Overflow interrupt",       4, 36, L"0x%x",  NULL, NULL,                 NULL, NULL },
>
> +  { L"Reserved1",                4, 40, L"0x%x",  NULL, NULL,                 NULL, NULL },
>
> +  { L"Overflow interrupt flags", 4, 44, L"0x%x",  NULL, NULL,                 NULL, NULL },
>
> +  { L"Processor affinity",       4, 48, L"0x%x",  NULL, NULL,                 NULL, NULL },
>
> +  { L"Implementation ID",        4, 52, L"0x%x",  NULL, NULL,                 NULL, NULL }
>
> +};
>
> +
>
> +/**
>
> +  This function parses the ACPI APMT table.
>
> +  When trace is enabled this function parses the APMT table and
>
> +  traces the ACPI table fields.
>
> +
>
> +  This function also performs validation of the ACPI table fields.
>
> +
>
> +  @param [in] Trace              If TRUE, trace the ACPI fields.
>
> +  @param [in] Ptr                Pointer to the start of the buffer.
>
> +  @param [in] AcpiTableLength    Length of the ACPI table.
>
> +  @param [in] AcpiTableRevision  Revision of the ACPI table.
>
> +**/
>
> +VOID
>
> +EFIAPI
>
> +ParseAcpiApmt (
>
> +  IN BOOLEAN  Trace,
>
> +  IN UINT8    *Ptr,
>
> +  IN UINT32   AcpiTableLength,
>
> +  IN UINT8    AcpiTableRevision
>
> +  )
>
> +{
>
> +  UINT32  Offset;
>
> +
>
> +  if (!Trace) {
>
> +    return;
>
> +  }
>
> +
>
> +  ParseAcpi (
>
> +    Trace,
>
> +    0,
>
> +    "APMT",
>
> +    Ptr,
>
> +    AcpiTableLength,
>
> +    PARSER_PARAMS (ApmtParser)
>
> +    );
>
> +  Offset = sizeof (EFI_ACPI_DESCRIPTION_HEADER);
>
> +
>
> +  while (Offset < AcpiTableLength) {
>
> +    ParseAcpi (
>
> +      Trace,
>
> +      2,
>
> +      "Arm PMU node",
>
> +      Ptr + Offset,
>
> +      (AcpiTableLength - Offset),
>
> +      PARSER_PARAMS (ArmPmuNodeParser)
>
> +      );
>
> +    if (NodeLength == NULL) {
[SAMI] I think it will be good to increment the error count by 
calling'IncrementErrorCount ()'.
>
> +      Print (
>
> +        L"ERROR: Insufficient remaining table buffer length to read the " \
>
> +        L"Node structure. Length = %d.\n",
>
> +        (AcpiTableLength - Offset)
>
> +        );
>
> +      break;
>
> +    }
>
> +
>
> +    Offset += *NodeLength;
>
> +  }
>
> +}
>
> diff --git a/ShellPkg/Library/UefiShellAcpiViewCommandLib/UefiShellAcpiViewCommandLib.c b/ShellPkg/Library/UefiShellAcpiViewCommandLib/UefiShellAcpiViewCommandLib.c
>
> index 09bdddb56e..53f06fc757 100644
>
> --- a/ShellPkg/Library/UefiShellAcpiViewCommandLib/UefiShellAcpiViewCommandLib.c
>
> +++ b/ShellPkg/Library/UefiShellAcpiViewCommandLib/UefiShellAcpiViewCommandLib.c
>
> @@ -48,6 +48,7 @@ STATIC
>
>   CONST
>
>   ACPI_TABLE_PARSER  ParserList[] = {
>
>     { EFI_ACPI_6_3_ARM_ERROR_SOURCE_TABLE_SIGNATURE,                                                       ParseAcpiAest },
>
> +  { EFI_ACPI_6_4_ARM_PERFORMANCE_MONITORING_UNIT_TABLE_SIGNATURE,                                        ParseAcpiApmt },
>
>     { EFI_ACPI_6_2_BOOT_GRAPHICS_RESOURCE_TABLE_SIGNATURE,                                                 ParseAcpiBgrt },
>
>     { EFI_ACPI_6_2_DEBUG_PORT_2_TABLE_SIGNATURE,                                                           ParseAcpiDbg2 },
>
>     { EFI_ACPI_6_2_DIFFERENTIATED_SYSTEM_DESCRIPTION_TABLE_SIGNATURE,
>
> diff --git a/ShellPkg/Library/UefiShellAcpiViewCommandLib/UefiShellAcpiViewCommandLib.inf b/ShellPkg/Library/UefiShellAcpiViewCommandLib/UefiShellAcpiViewCommandLib.inf
>
> index 63fc5a1281..b03ec1a31b 100644
>
> --- a/ShellPkg/Library/UefiShellAcpiViewCommandLib/UefiShellAcpiViewCommandLib.inf
>
> +++ b/ShellPkg/Library/UefiShellAcpiViewCommandLib/UefiShellAcpiViewCommandLib.inf
>
> @@ -28,6 +28,7 @@
>
>     AcpiViewConfig.c
>
>     AcpiViewConfig.h
>
>     Parsers/Aest/AestParser.c
>
> +  Parsers/Apmt/ApmtParser.c

[SAMI] I think it would be good to update the STR_GET_HELP_ACPIVIEW text 
in 
ShellPkg\Library\UefiShellAcpiViewCommandLib\UefiShellAcpiViewCommandLib.uni 
to add APMT as a supported parser.

[/SAMI]

>
>     Parsers/Bgrt/BgrtParser.c
>
>     Parsers/Dbg2/Dbg2Parser.c
>
>     Parsers/Dsdt/DsdtParser.c
>

[-- Attachment #2: Type: text/html, Size: 9157 bytes --]