Re: [edk2-devel] [RESEND PATCH v4 2/5] DynamicTablesPkg: Adds ACPI HPET Table generator

["PierreGondois" <pierre.gondois@arm.com>]

Hello Abdul,
some comments on the patch:

On 4/29/24 08:03, Abdul Lateef Attar wrote:
> Adds generic ACPI HPET table generator library.
> Register/Deregister HPET table.
> Update the HPET table during boot as per specification.
> 
> Cc: Sami Mujawar <Sami.Mujawar@arm.com>
> Cc: Pierre Gondois <pierre.gondois@arm.com>
> Signed-off-by: Abdul Lateef Attar <AbdulLateef.Attar@amd.com>
> ---
>   DynamicTablesPkg/DynamicTables.dsc.inc        |   2 +
>   DynamicTablesPkg/Include/AcpiTableGenerator.h |   1 +
>   .../Include/ArchNameSpaceObjects.h            |   9 +
>   .../Library/Acpi/AcpiHpetLib/AcpiHpetLib.inf  |  31 +++
>   .../Library/Acpi/AcpiHpetLib/HpetGenerator.c  | 246 ++++++++++++++++++
>   5 files changed, 289 insertions(+)
>   create mode 100644 DynamicTablesPkg/Library/Acpi/AcpiHpetLib/AcpiHpetLib.inf
>   create mode 100644 DynamicTablesPkg/Library/Acpi/AcpiHpetLib/HpetGenerator.c
> 
> diff --git a/DynamicTablesPkg/DynamicTables.dsc.inc b/DynamicTablesPkg/DynamicTables.dsc.inc
> index 92f3a138e4..b2ef36eb8a 100644
> --- a/DynamicTablesPkg/DynamicTables.dsc.inc
> +++ b/DynamicTablesPkg/DynamicTables.dsc.inc
> @@ -34,6 +34,7 @@
>     # Generators
>     #
>     DynamicTablesPkg/Library/Acpi/AcpiFadtLib/AcpiFadtLib.inf
> +  DynamicTablesPkg/Library/Acpi/AcpiHpetLib/AcpiHpetLib.inf

I think this should be moved to the following section if the table if for Intel arch.
Like this it would allow avoiding to build the generator for other archs.

[Components.IA32, Components.X64]
      #
      # Generators
      #
...

also if the table is Intel specific, maybe the generator should be placed under:
    DynamicTablesPkg/Library/Acpi/X64/
    (or a better folder name)
also I think the CmObject should be moved to:
    X64NameSpaceObjects.h

>   
>   [Components.IA32, Components.X64]
>     #
> @@ -42,6 +43,7 @@
>     DynamicTablesPkg/Drivers/DynamicTableFactoryDxe/DynamicTableFactoryDxe.inf {
>       <LibraryClasses>
>         NULL|DynamicTablesPkg/Library/Acpi/AcpiFadtLib/AcpiFadtLib.inf
> +      NULL|DynamicTablesPkg/Library/Acpi/AcpiHpetLib/AcpiHpetLib.inf
>     }
>   
>   [Components.ARM, Components.AARCH64]
> diff --git a/DynamicTablesPkg/Include/AcpiTableGenerator.h b/DynamicTablesPkg/Include/AcpiTableGenerator.h
> index d0eda011c3..18b5f99f47 100644
> --- a/DynamicTablesPkg/Include/AcpiTableGenerator.h
> +++ b/DynamicTablesPkg/Include/AcpiTableGenerator.h
> @@ -99,6 +99,7 @@ typedef enum StdAcpiTableId {
>     EStdAcpiTableIdSsdtCpuTopology,               ///< SSDT Cpu Topology
>     EStdAcpiTableIdSsdtPciExpress,                ///< SSDT Pci Express Generator
>     EStdAcpiTableIdPcct,                          ///< PCCT Generator
> +  EStdAcpiTableIdHpet,                          ///< HPET Generator
>     EStdAcpiTableIdMax
>   } ESTD_ACPI_TABLE_ID;
>   
> diff --git a/DynamicTablesPkg/Include/ArchNameSpaceObjects.h b/DynamicTablesPkg/Include/ArchNameSpaceObjects.h
> index b421c4cd29..b90e573a88 100644
> --- a/DynamicTablesPkg/Include/ArchNameSpaceObjects.h
> +++ b/DynamicTablesPkg/Include/ArchNameSpaceObjects.h
> @@ -39,6 +39,7 @@ typedef enum ArchObjectID {
>     EArchObjFadtArmBootArch,        ///< 11 - ARM boot arch information
>     EArchObjFadtHypervisorVendorId, ///< 12 - Hypervisor vendor identity information
>     EArchObjFadtMiscInfo,           ///< 13 - Legacy fields; RTC, latency, flush stride, etc
> +  EArchObjHpetBaseAddress,        ///< 14 - HPET Base Address
>     EArchObjMax
>   } E_ARCH_OBJECT_ID;
>   
> @@ -214,4 +215,12 @@ typedef struct CmArchFadtMiscInfo {
>     UINT8     Century;
>   } CM_ARCH_FADT_MISC_INFO;
>   
> +/** A structure that describes the
> +    HPET Base Address.
> +
> +    ID: EArchObjHpetBaseAddress
> +*/
> +typedef struct CmArchHpetBaseAddress {
> +  UINT64    BaseAddress;
> +} CM_ARCH_HPET_BASE_ADDRESS;

Would it make sense to have these fields for the CmObj ?
typedef struct CmArchHpetBaseAddress {
      UINT32    EventTimerBlockId;
      UINT32    BaseAddressLower32Bit;
      UINT8     HpetNumber;
      UINT16    MainCounterMinimumClockTickInPeriodicMode;
      UINT8     PageProtectionAndOemAttribute;
} CM_ARCH_HPET_BASE_ADDRESS;

The reason being that:
- Ideally the DynamicTables generators should just generate ACPI tables
      from the CmObject they are given. The ConfigurationManager being a
      database providing information.
      Doing Mmio accesses to populate the ACPI tables breaks a bit the design:
      the generators would also become a source of information.
- If the Mmio accesses are possible from the generator, they should also
      be possible from the ConfigurationManager.
- Imagining a platform with an invalid value for the MainCounterMinimumClockTickInPeriodicMode,
      as the generator is generic to all platforms, it becomes hard to patch this.
      The ConfigurationManager is supposed to be platform specific, so if
      the MainCounterMinimumClockTickInPeriodicMode register is invalid,
      it is easy to patch this specific ConfigurationManager and keep a generic
      HPET generator.

(I am refering to MmioRead32 accesses from [1])

>   #endif
> diff --git a/DynamicTablesPkg/Library/Acpi/AcpiHpetLib/AcpiHpetLib.inf b/DynamicTablesPkg/Library/Acpi/AcpiHpetLib/AcpiHpetLib.inf
> new file mode 100644
> index 0000000000..f0441107fc
> --- /dev/null
> +++ b/DynamicTablesPkg/Library/Acpi/AcpiHpetLib/AcpiHpetLib.inf
> @@ -0,0 +1,31 @@
> +## @file
> +#  HPET Table Generator
> +#
> +#  Copyright (C) 2024 Advanced Micro Devices, Inc. All rights reserved.
> +#
> +#  SPDX-License-Identifier: BSD-2-Clause-Patent
> +##
> +
> +[Defines]
> +  INF_VERSION    = 1.27
> +  BASE_NAME      = AcpiHpetLib
> +  FILE_GUID      = 4E75F653-C356-48B3-B32C-D1B901ECF90A
> +  VERSION_STRING = 1.0
> +  MODULE_TYPE    = DXE_DRIVER
> +  LIBRARY_CLASS  = NULL|DXE_DRIVER
> +  CONSTRUCTOR    = AcpiHpetLibConstructor
> +  DESTRUCTOR     = AcpiHpetLibDestructor
> +
> +[Sources]
> +  HpetGenerator.c
> +
> +[Packages]
> +  DynamicTablesPkg/DynamicTablesPkg.dec
> +  MdeModulePkg/MdeModulePkg.dec
> +  MdePkg/MdePkg.dec
> +
> +[LibraryClasses]
> +  BaseLib
> +  DebugLib
> +  IoLib
> +
> diff --git a/DynamicTablesPkg/Library/Acpi/AcpiHpetLib/HpetGenerator.c b/DynamicTablesPkg/Library/Acpi/AcpiHpetLib/HpetGenerator.c
> new file mode 100644
> index 0000000000..937879b7b3
> --- /dev/null
> +++ b/DynamicTablesPkg/Library/Acpi/AcpiHpetLib/HpetGenerator.c
> @@ -0,0 +1,246 @@
> +/** @file
> +  HPET Table Generator
> +
> +  Copyright (c) 2017 - 2023, Arm Limited. All rights reserved.
> +  Copyright (C) 2024 Advanced Micro Devices, Inc. All rights reserved.
> +
> +  SPDX-License-Identifier: BSD-2-Clause-Patent
> +
> +  @par Reference(s):
> +  - ACPI 6.5 Specification, Aug 29, 2022
> +  - HPET spec, version 1.0a
> +    http://www.intel.com/content/dam/www/public/us/en/documents/technical-specifications/software-developers-hpet-spec-1-0a.pdf
> +
> +**/
> +
> +#include <AcpiTableGenerator.h>
> +#include <ConfigurationManagerHelper.h>
> +#include <ConfigurationManagerObject.h>
> +#include <IndustryStandard/HighPrecisionEventTimerTable.h>
> +#include <Library/DebugLib.h>
> +#include <Library/IoLib.h>
> +#include <Library/TableHelperLib.h>
> +#include <Protocol/AcpiTable.h>
> +#include <Protocol/ConfigurationManagerProtocol.h>
> +
> +///
> +/// HPET General Register Offsets
> +///
> +#define HPET_GENERAL_CAPABILITIES_ID_OFFSET  0x000
> +
> +/** The Creator ID for the ACPI tables generated using
> +  the standard ACPI table generators.
> +*/
> +#define TABLE_GENERATOR_CREATOR_ID_GENERIC  SIGNATURE_32('D', 'Y', 'N', 'T')
> +
> +/** The AcpiHpet is a template EFI_ACPI_HIGH_PRECISION_EVENT_TIMER_TABLE_HEADER
> +    structure used for generating the HPET Table.
> +*/
> +STATIC
> +EFI_ACPI_HIGH_PRECISION_EVENT_TIMER_TABLE_HEADER  mAcpiHpet = {
> +  ACPI_HEADER (
> +    EFI_ACPI_6_5_HIGH_PRECISION_EVENT_TIMER_TABLE_SIGNATURE,
> +    EFI_ACPI_HIGH_PRECISION_EVENT_TIMER_TABLE_HEADER,
> +    EFI_ACPI_HIGH_PRECISION_EVENT_TIMER_TABLE_REVISION
> +    ),
> +  // EventTimerBlockId
> +  0,
> +  // BaseAddressLower32Bit
> +  { EFI_ACPI_6_5_SYSTEM_MEMORY,                             0,0, EFI_ACPI_RESERVED_BYTE, 0 },
> +  // HpetNumber
> +  0,
> +  // MainCounterMinimumClockTickInPeriodicMode
> +  0,
> +  // PageProtectionAndOemAttribute
> +  EFI_ACPI_NO_PAGE_PROTECTION
> +};
> +
> +/** This macro expands to a function that retrieves the
> +    HPET base address from the Configuration Manager.
> +*/
> +GET_OBJECT_LIST (
> +  EObjNameSpaceArch,
> +  EArchObjHpetBaseAddress,
> +  CM_ARCH_HPET_BASE_ADDRESS
> +  );
> +
> +/** Construct the HPET table.
> +
> +  This function invokes the Configuration Manager protocol interface
> +  to get the required hardware information for generating the ACPI
> +  table.
> +
> +  If this function allocates any resources then they must be freed
> +  in the FreeXXXXTableResources function.
> +
> +  @param [in]  This           Pointer to the table generator.
> +  @param [in]  AcpiTableInfo  Pointer to the ACPI Table Info.
> +  @param [in]  CfgMgrProtocol Pointer to the Configuration Manager
> +                              Protocol Interface.
> +  @param [out] Table          Pointer to the constructed ACPI Table.
> +
> +  @retval EFI_SUCCESS           Table generated successfully.
> +  @retval EFI_INVALID_PARAMETER A parameter is invalid.
> +  @retval EFI_NOT_FOUND         The required object was not found.
> +  @retval EFI_BAD_BUFFER_SIZE   The size returned by the Configuration
> +                                Manager is less than the Object size for the
> +                                requested object.
> +**/
> +STATIC
> +EFI_STATUS
> +BuildHpetTable (
> +  IN  CONST ACPI_TABLE_GENERATOR                  *CONST  This,
> +  IN  CONST CM_STD_OBJ_ACPI_TABLE_INFO            *CONST  AcpiTableInfo,
> +  IN  CONST EDKII_CONFIGURATION_MANAGER_PROTOCOL  *CONST  CfgMgrProtocol,
> +  OUT       EFI_ACPI_DESCRIPTION_HEADER          **CONST  Table
> +  )
> +{
> +  EFI_STATUS                 Status;
> +  CM_ARCH_HPET_BASE_ADDRESS  *HpetBaseAddress;
> +
> +  ASSERT (This != NULL);
> +  ASSERT (AcpiTableInfo != NULL);
> +  ASSERT (CfgMgrProtocol != NULL);
> +  ASSERT (Table != NULL);
> +  ASSERT (AcpiTableInfo->TableGeneratorId == This->GeneratorID);
> +  ASSERT (AcpiTableInfo->AcpiTableSignature == This->AcpiTableSignature);
> +
> +  if ((AcpiTableInfo->AcpiTableRevision < This->MinAcpiTableRevision) ||
> +      (AcpiTableInfo->AcpiTableRevision > This->AcpiTableRevision))
> +  {
> +    DEBUG ((
> +      DEBUG_ERROR,
> +      "ERROR: HPET: Requested table revision = %d, is not supported."
> +      "Supported table revision: Minimum = %d, Maximum = %d\n",
> +      AcpiTableInfo->AcpiTableRevision,
> +      This->MinAcpiTableRevision,
> +      This->AcpiTableRevision
> +      ));
> +    return EFI_INVALID_PARAMETER;
> +  }
> +
> +  *Table = NULL;
> +
> +  Status = AddAcpiHeader (
> +             CfgMgrProtocol,
> +             This,
> +             (EFI_ACPI_DESCRIPTION_HEADER *)&mAcpiHpet,
> +             AcpiTableInfo,
> +             sizeof (EFI_ACPI_HIGH_PRECISION_EVENT_TIMER_TABLE_HEADER)
> +             );
> +  if (EFI_ERROR (Status)) {
> +    DEBUG ((
> +      DEBUG_ERROR,
> +      "ERROR: HPET: Failed to add ACPI header. Status = %r\n",
> +      Status
> +      ));
> +    return Status;
> +  }
> +
> +  // Get the HPET Base Address from the Platform Configuration Manager
> +  Status = GetEArchObjHpetBaseAddress (
> +             CfgMgrProtocol,
> +             CM_NULL_TOKEN,
> +             &HpetBaseAddress,
> +             NULL
> +             );
> +  if (EFI_ERROR (Status)) {
> +    DEBUG ((
> +      DEBUG_ERROR,
> +      "ERROR: HPET: Failed to get HPET Base Address. Status = %r\n",
> +      Status
> +      ));
> +    return Status;
> +  }
> +
> +  mAcpiHpet.BaseAddressLower32Bit.Address             = HpetBaseAddress->BaseAddress;
> +  mAcpiHpet.EventTimerBlockId                         = MmioRead32 ((UINTN)(HpetBaseAddress->BaseAddress + HPET_GENERAL_CAPABILITIES_ID_OFFSET));
> +  mAcpiHpet.MainCounterMinimumClockTickInPeriodicMode = (UINT16)MmioRead32 ((UINTN)(HpetBaseAddress->BaseAddress + HPET_GENERAL_CAPABILITIES_ID_OFFSET + 4));

[1]

> +  *Table                                              = (EFI_ACPI_DESCRIPTION_HEADER *)&mAcpiHpet;
> +
> +  return Status;
> +}
> +
> +/** This macro defines the HPET Table Generator revision.
> +*/
> +#define HPET_GENERATOR_REVISION  CREATE_REVISION (1, 0)
> +
> +/** The interface for the HPET Table Generator.
> +*/
> +STATIC
> +CONST
> +ACPI_TABLE_GENERATOR  mHpetGenerator = {
> +  // Generator ID
> +  CREATE_STD_ACPI_TABLE_GEN_ID (EStdAcpiTableIdHpet),
> +  // Generator Description
> +  L"ACPI.STD.HPET.GENERATOR",
> +  // ACPI Table Signature
> +  EFI_ACPI_6_5_HIGH_PRECISION_EVENT_TIMER_TABLE_SIGNATURE,
> +  // ACPI Table Revision supported by this Generator
> +  EFI_ACPI_HIGH_PRECISION_EVENT_TIMER_TABLE_REVISION,
> +  // Minimum supported ACPI Table Revision
> +  EFI_ACPI_HIGH_PRECISION_EVENT_TIMER_TABLE_REVISION,
> +  // Creator ID
> +  TABLE_GENERATOR_CREATOR_ID_GENERIC,
> +  // Creator Revision
> +  HPET_GENERATOR_REVISION,
> +  // Build Table function
> +  BuildHpetTable,
> +  // No additional resources are allocated by the generator.
> +  // Hence the Free Resource function is not required.
> +  NULL,
> +  // Extended build function not needed
> +  NULL,
> +  // Extended build function not implemented by the generator.
> +  // Hence extended free resource function is not required.
> +  NULL
> +};
> +
> +/** Register the Generator with the ACPI Table Factory.
> +
> +  @param [in]  ImageHandle  The handle to the image.
> +  @param [in]  SystemTable  Pointer to the System Table.
> +
> +  @retval EFI_SUCCESS           The Generator is registered.
> +  @retval EFI_INVALID_PARAMETER A parameter is invalid.
> +  @retval EFI_ALREADY_STARTED   The Generator for the Table ID
> +                                is already registered.
> +**/
> +EFI_STATUS
> +EFIAPI
> +AcpiHpetLibConstructor (
> +  IN  EFI_HANDLE        ImageHandle,
> +  IN  EFI_SYSTEM_TABLE  *SystemTable
> +  )
> +{
> +  EFI_STATUS  Status;
> +
> +  Status = RegisterAcpiTableGenerator (&mHpetGenerator);
> +  DEBUG ((DEBUG_INFO, "HPET: Register Generator. Status = %r\n", Status));
> +  ASSERT_EFI_ERROR (Status);
> +  return Status;
> +}
> +
> +/** Deregister the Generator from the ACPI Table Factory.
> +
> +  @param [in]  ImageHandle  The handle to the image.
> +  @param [in]  SystemTable  Pointer to the System Table.
> +
> +  @retval EFI_SUCCESS           The Generator is deregistered.
> +  @retval EFI_INVALID_PARAMETER A parameter is invalid.
> +  @retval EFI_NOT_FOUND         The Generator is not registered.
> +**/
> +EFI_STATUS
> +EFIAPI
> +AcpiHpetLibDestructor (
> +  IN  EFI_HANDLE        ImageHandle,
> +  IN  EFI_SYSTEM_TABLE  *SystemTable
> +  )
> +{
> +  EFI_STATUS  Status;
> +
> +  Status = DeregisterAcpiTableGenerator (&mHpetGenerator);
> +  DEBUG ((DEBUG_INFO, "HPET: Deregister Generator. Status = %r\n", Status));
> +  ASSERT_EFI_ERROR (Status);
> +  return Status;
> +}


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