Re: [edk2-devel] [PATCH v1 1/2] ShellPkg/Acpiview: Adds HPET parser

["Abdul Lateef Attar via groups.io" <AbdulLateef.Attar=amd.com@groups.io>]

Thanks Pierre for review, I'll address the review comment and submit the 
V2 patch.


On 07-03-2024 16:07, Pierre Gondois wrote:
> Caution: This message originated from an External Source. Use proper 
> caution when opening attachments, clicking links, or responding.
>
>
> Hello Abdul,
>
> With the comments resolved:
> Reviewed-by: Pierre Gondois <pierre.gondois@arm.com>
>
> On 3/5/24 12:12, Abdul Lateef Attar wrote:
>> From: Abdul Lateef Attar <AbdulLateef.Attar@amd.com>
>>
>> Adds HPET parse to the UefiShellAcpiViewCommandLib library.
>>
>> Cc: Zhichao Gao <zhichao.gao@intel.com>
>> Cc: Pierre Gondois  <pierre.gondois@arm.com>
>> Signed-off-by: Abdul Lateef Attar <AbdulLateef.Attar@amd.com>
>> ---
>>   .../UefiShellAcpiViewCommandLib/AcpiParser.h  |  17 ++
>>   .../Parsers/Hpet/HpetParser.c                 | 221 ++++++++++++++++++
>>   .../UefiShellAcpiViewCommandLib.c             |   1 +
>>   .../UefiShellAcpiViewCommandLib.inf           |   1 +
>>   4 files changed, 240 insertions(+)
>>   create mode 100644 
>> ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Hpet/HpetParser.c
>>
>> diff --git 
>> a/ShellPkg/Library/UefiShellAcpiViewCommandLib/AcpiParser.h 
>> b/ShellPkg/Library/UefiShellAcpiViewCommandLib/AcpiParser.h
>> index 4b4397961b..ba3364f2c2 100644
>> --- a/ShellPkg/Library/UefiShellAcpiViewCommandLib/AcpiParser.h
>> +++ b/ShellPkg/Library/UefiShellAcpiViewCommandLib/AcpiParser.h
>> @@ -730,6 +730,23 @@ ParseAcpiHmat (
>>     IN UINT8    AcpiTableRevision
>>     );
>>
>> +/**
>> +  This function parses the ACPI HPET table.
>> +
>> +  @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
>> +ParseAcpiHpet (
>> +  IN BOOLEAN  Trace,
>> +  IN UINT8    *Ptr,
>> +  IN UINT32   AcpiTableLength,
>> +  IN UINT8    AcpiTableRevision
>> +  );
>> +
>>   /**
>>     This function parses the ACPI IORT table.
>>     When trace is enabled this function parses the IORT table and
>> diff --git 
>> a/ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Hpet/HpetParser.c 
>> b/ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Hpet/HpetParser.c
>> new file mode 100644
>> index 0000000000..d1866f91c1
>> --- /dev/null
>> +++ 
>> b/ShellPkg/Library/UefiShellAcpiViewCommandLib/Parsers/Hpet/HpetParser.c
>> @@ -0,0 +1,221 @@
>> +/** @file
>> +  HPET table parser
>> +
>> +  Copyright (C) 2023 Advanced Micro Devices, Inc. All rights reserved.
>> +  SPDX-License-Identifier: BSD-2-Clause-Patent
>> +
>> +  @par Reference(s):
>> +    - HPET spec, version 1.0a
>> +**/
>> +
>> +#include <Library/UefiLib.h>
>> +#include "AcpiParser.h"
>> +
>> +STATIC ACPI_DESCRIPTION_HEADER_INFO  AcpiHdrInfo;
>> +
>> +/**
>> +  This function prints HPET page protection flags.
>> +  If no format string is specified the Format must be NULL.
>> +
>> +  @param [in] Format  Optional format string for tracing the data.
>> +  @param [in] Ptr     Pointer to the start of the buffer.
>> +**/
>> +VOID
>> +EFIAPI
>> +DumpHpetPageProtectionFlag (
>> +  IN CONST CHAR16  *Format OPTIONAL,
>> +  IN UINT8         *Ptr
>> +  )
>> +{
>> +  if (Format != NULL) {
>> +    Print (Format, *(UINT8 *)Ptr);
>> +    return;
>> +  }
>> +
>> +  Print (L"0x%X ", *(UINT8 *)Ptr);
>> +  switch (*Ptr) {
>> +    case 0:
>> +      Print (L"(no guarantee for page protection)");
>> +      break;
>> +    case 1:
>> +      Print (L"(4K page protection)");
>> +      break;
>> +    case 2:
>> +      Print (L"(64K page protection)");
>> +      break;
>> +    default:
>> +      Print (L"(OEM Reserved)");
>
> Maybe IncrementErrorCount() + Print() here.
>
>> +      break;
>> +  }
>> +
>> +  return;
>> +}
>> +
>> +/**
>> +  An ACPI_PARSER array describing the ACPI HPET flags.
>> +**/
>> +STATIC CONST ACPI_PARSER  DumpHpetFlagParser[] = {
>> +  { L"Page Protection Flag", 4, 0, NULL, DumpHpetPageProtectionFlag, 
>> NULL, NULL, NULL },
>> +  { L"OEM Attributes",       4, 4, L"0x%x", 
>> NULL,                       NULL, NULL, NULL }
>> +};
>> +
>> +/**
>> +  This function prints HPET Flags fields.
>> +  If no format string is specified the Format must be NULL.
>> +
>> +  @param [in] Format  Optional format string for tracing the data.
>> +  @param [in] Ptr     Pointer to the start of the buffer.
>> +**/
>> +VOID
>> +EFIAPI
>> +DumpHpetFlag (
>> +  IN CONST CHAR16  *Format OPTIONAL,
>> +  IN UINT8         *Ptr
>> +  )
>> +{
>> +  if (Format != NULL) {
>> +    Print (Format, *(UINT8 *)Ptr);
>> +    return;
>> +  }
>> +
>> +  Print (L"0x%X\n", *(UINT8 *)Ptr);
>> +  ParseAcpiBitFields (
>> +    TRUE,
>> +    2,
>> +    NULL,
>> +    Ptr,
>> +    4,
>> +    PARSER_PARAMS (DumpHpetFlagParser)
>> +    );
>> +}
>> +
>> +/**
>> +  This function prints HPET Counter size fields.
>> +  If no format string is specified the Format must be NULL.
>> +
>> +  @param [in] Format  Optional format string for tracing the data.
>> +  @param [in] Ptr     Pointer to the start of the buffer.
>> +**/
>> +VOID
>> +EFIAPI
>> +DumpCounterSize (
>> +  IN CONST CHAR16  *Format OPTIONAL,
>> +  IN UINT8         *Ptr
>> +  )
>> +{
>> +  if (Format != NULL) {
>> +    Print (Format, *(UINT32 *)Ptr);
>> +    return;
>> +  }
>> +
>> +  Print (L"0x%X ", *(UINT32 *)Ptr);
>> +  if (*Ptr == 0) {
>> +    Print (L"(Max 32-bit counter size)");
>> +  } else {
>> +    Print (L"(Max 64-bit counter size)");
>> +  }
>> +}
>> +
>> +/**
>> +  This function validates the flags.
>> +
>> +  @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
>> +ValidateHpetRevId (
>> +  IN UINT8  *Ptr,
>> +  IN VOID   *Context
>> +  )
>> +{
>> +  if ((*(UINT8 *)Ptr) == 0) {
>> +    IncrementErrorCount ();
>> +    Print (
>> +      L"\nERROR: HPET Hardware Rev ID must be set."
>> +      );
>> +  }
>> +}
>> +
>> +/**
>> +  An ACPI_PARSER array describing the ACPI HPET Event Timer Block ID.
>> +**/
>> +STATIC CONST ACPI_PARSER  HpetEventTimerBlockIdFlagParser[] = {
>> +  { L"Hardware Rev ID",                  8,  0,  L"0x%x", 
>> NULL,            NULL, ValidateHpetRevId, NULL },
>> +  { L"Timer count (minus one)",          5,  8,  L"0x%x", 
>> NULL,            NULL, NULL,              NULL },
>
> The description in the spec is:
> 'Number of Comparators in 1st Timer Bloc'
> (maybe the name should be shorted)
>
>> +  { L"Counter max size", 1,  13, NULL,    DumpCounterSize, NULL, 
>> NULL,              NULL },
>> +  { L"Reserved",                         1,  14, L"%d", 
>> NULL,            NULL, NULL,              NULL },
>> +  { L"LegacyReplacement IRQ Routing",    1,  15, L"%d", 
>> NULL,            NULL, NULL,              NULL },
>> +  { L"PCI Vendor ID of 1st Timer Block", 16, 16, L"0x%x", 
>> NULL,            NULL, NULL,              NULL }
>> +};
>> +
>> +/**
>> +  This function prints Hardware ID of HPET Event timer block.
>> +  If no format string is specified the Format must be NULL.
>> +
>> +  @param [in] Format  Optional format string for tracing the data.
>> +  @param [in] Ptr     Pointer to the start of the buffer.
>> +**/
>> +VOID
>> +EFIAPI
>> +DumpHpetEventTimerBlockId (
>> +  IN CONST CHAR16  *Format OPTIONAL,
>> +  IN UINT8         *Ptr
>> +  )
>> +{
>> +  if (Format != NULL) {
>> +    Print (Format, *(UINT32 *)Ptr);
>> +    return;
>> +  }
>> +
>> +  Print (L"0x%X\n", *(UINT32 *)Ptr);
>> +  ParseAcpiBitFields (
>> +    TRUE,
>> +    2,
>> +    NULL,
>> +    Ptr,
>> +    4,
>> +    PARSER_PARAMS (HpetEventTimerBlockIdFlagParser)
>> +    );
>> +}
>> +
>> +/**
>> +  An ACPI_PARSER array describing the ACPI HPET Table.
>> +**/
>> +STATIC CONST ACPI_PARSER  HpetParser[] = {
>> +  PARSE_ACPI_HEADER (&AcpiHdrInfo),
>> +  { L"Event Timer Block ID",       4,   36, NULL, 
>> DumpHpetEventTimerBlockId, NULL, NULL, NULL },
>> +  { L"Event Timer Block Register", 12,  40, NULL, 
>> DumpGas,                   NULL, NULL, NULL },
>
> Even though this is close, maybe the spec definition should be used 
> instead:
> 'Base address'
>
>> +  { L"Sequence number",            1, 52, L"0x%x", 
>> NULL,                      NULL, NULL, NULL },
>
> 'HPET number'
>
>> +  { L"Minimum Clock Ticks",        2, 53, L"0x%x", 
>> NULL,                      NULL, NULL, NULL },
>> +  { L"Flag",                       1,   55, NULL, 
>> DumpHpetFlag,              NULL, NULL, NULL }
>> +};
>> +
>> +/**
>> +  This function parses the ACPI HPET table.
>> +
>> +  @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
>> +ParseAcpiHpet (
>> +  IN BOOLEAN  Trace,
>> +  IN UINT8    *Ptr,
>> +  IN UINT32   AcpiTableLength,
>> +  IN UINT8    AcpiTableRevision
>> +  )
>> +{
>> +  ParseAcpi (
>> +    Trace,
>> +    0,
>> +    "HPET",
>> +    Ptr,
>> +    AcpiTableLength,
>> +    PARSER_PARAMS (HpetParser)
>> +    );
>> +}
>> diff --git 
>> a/ShellPkg/Library/UefiShellAcpiViewCommandLib/UefiShellAcpiViewCommandLib.c 
>> b/ShellPkg/Library/UefiShellAcpiViewCommandLib/UefiShellAcpiViewCommandLib.c 
>>
>> index bbac125bb9..9e15979ea2 100644
>> --- 
>> a/ShellPkg/Library/UefiShellAcpiViewCommandLib/UefiShellAcpiViewCommandLib.c
>> +++ 
>> b/ShellPkg/Library/UefiShellAcpiViewCommandLib/UefiShellAcpiViewCommandLib.c
>> @@ -59,6 +59,7 @@ ACPI_TABLE_PARSER  ParserList[] = {
>>     { EFI_ACPI_6_2_FIXED_ACPI_DESCRIPTION_TABLE_SIGNATURE, 
>> ParseAcpiFadt },
>>     { EFI_ACPI_6_4_GENERIC_TIMER_DESCRIPTION_TABLE_SIGNATURE, 
>> ParseAcpiGtdt },
>>     { EFI_ACPI_6_4_HETEROGENEOUS_MEMORY_ATTRIBUTE_TABLE_SIGNATURE, 
>> ParseAcpiHmat },
>> +  { EFI_ACPI_6_5_HIGH_PRECISION_EVENT_TIMER_TABLE_SIGNATURE, 
>> ParseAcpiHpet },
>>     { EFI_ACPI_6_2_IO_REMAPPING_TABLE_SIGNATURE, ParseAcpiIort },
>>     { EFI_ACPI_6_2_MULTIPLE_APIC_DESCRIPTION_TABLE_SIGNATURE, 
>> ParseAcpiMadt },
>>     { 
>> EFI_ACPI_6_2_PCI_EXPRESS_MEMORY_MAPPED_CONFIGURATION_SPACE_BASE_ADDRESS_DESCRIPTION_TABLE_SIGNATURE,
>> diff --git 
>> a/ShellPkg/Library/UefiShellAcpiViewCommandLib/UefiShellAcpiViewCommandLib.inf 
>> b/ShellPkg/Library/UefiShellAcpiViewCommandLib/UefiShellAcpiViewCommandLib.inf 
>>
>> index a38965a4bf..3338e04379 100644
>> --- 
>> a/ShellPkg/Library/UefiShellAcpiViewCommandLib/UefiShellAcpiViewCommandLib.inf
>> +++ 
>> b/ShellPkg/Library/UefiShellAcpiViewCommandLib/UefiShellAcpiViewCommandLib.inf
>> @@ -38,6 +38,7 @@
>>     Parsers/Fadt/FadtParser.c
>>     Parsers/Gtdt/GtdtParser.c
>>     Parsers/Hmat/HmatParser.c
>> +  Parsers/Hpet/HpetParser.c
>>     Parsers/Iort/IortParser.c
>>     Parsers/Madt/MadtParser.c
>>     Parsers/Madt/MadtParser.h


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#116487): https://edk2.groups.io/g/devel/message/116487
Mute This Topic: https://groups.io/mt/104742259/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-