Re: [edk2-devel] [PATCH v2 4/4] DynamicTablesPkg: Adds ACPI SSDT HPET Table generator

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

Hello Abdul,

On 3/4/24 16:43, Abdul Lateef Attar wrote:
> From: Abdul Lateef Attar <AbdulLateef.Attar@amd.com>
> 
> Adds generic ACPI SSDT HPET table generator library.
> Register/Deregister HPET table.
> Adds ACPI namespace object for HPET device.
> Adds Address space for HPET device.
> 
> 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 |   2 +
>   .../Acpi/AcpiSsdtHpetLib/AcpiSsdtHpetLib.inf  |  36 +++
>   .../Acpi/AcpiSsdtHpetLib/SsdtHpetGenerator.c  | 266 ++++++++++++++++++
>   4 files changed, 306 insertions(+)
>   create mode 100644 DynamicTablesPkg/Library/Acpi/AcpiSsdtHpetLib/AcpiSsdtHpetLib.inf
>   create mode 100644 DynamicTablesPkg/Library/Acpi/AcpiSsdtHpetLib/SsdtHpetGenerator.c
> 
> diff --git a/DynamicTablesPkg/DynamicTables.dsc.inc b/DynamicTablesPkg/DynamicTables.dsc.inc
> index 477dc6b6a9..fc2ac5962e 100644
> --- a/DynamicTablesPkg/DynamicTables.dsc.inc
> +++ b/DynamicTablesPkg/DynamicTables.dsc.inc
> @@ -36,6 +36,7 @@
>     DynamicTablesPkg/Library/Acpi/AcpiFadtLib/AcpiFadtLib.inf
>     DynamicTablesPkg/Library/Acpi/AcpiHpetLib/AcpiHpetLib.inf
>     DynamicTablesPkg/Library/Acpi/AcpiWsmtLib/AcpiWsmtLib.inf
> +  DynamicTablesPkg/Library/Acpi/AcpiSsdtHpetLib/AcpiSsdtHpetLib.inf

(note for later):
The HPET table seems to be intel specific actually,

>   
>   [Components.IA32, Components.X64]
>     #
> @@ -46,6 +47,7 @@
>         NULL|DynamicTablesPkg/Library/Acpi/AcpiFadtLib/AcpiFadtLib.inf
>         NULL|DynamicTablesPkg/Library/Acpi/AcpiHpetLib/AcpiHpetLib.inf
>         NULL|DynamicTablesPkg/Library/Acpi/AcpiWsmtLib/AcpiWsmtLib.inf
> +      NULL|DynamicTablesPkg/Library/Acpi/AcpiSsdtHpetLib/AcpiSsdtHpetLib.inf
>     }
>   
>   [Components.ARM, Components.AARCH64]
> diff --git a/DynamicTablesPkg/Include/AcpiTableGenerator.h b/DynamicTablesPkg/Include/AcpiTableGenerator.h
> index a32ef46ecb..ef651aa2aa 100644
> --- a/DynamicTablesPkg/Include/AcpiTableGenerator.h
> +++ b/DynamicTablesPkg/Include/AcpiTableGenerator.h
> @@ -1,6 +1,7 @@
>   /** @file
>   
>     Copyright (c) 2017 - 2022, Arm Limited. All rights reserved.<BR>
> +  Copyright (C) 2024 Advanced Micro Devices, Inc. All rights reserved.
>   
>     SPDX-License-Identifier: BSD-2-Clause-Patent
>   
> @@ -101,6 +102,7 @@ typedef enum StdAcpiTableId {
>     EStdAcpiTableIdPcct,                          ///< PCCT Generator
>     EStdAcpiTableIdHpet,                          ///< HPET Generator
>     EStdAcpiTableIdWsmt,                          ///< WSMT Generator
> +  EStdAcpiTableIdSsdtHpet,                      ///< SSDT HPET Generator
>     EStdAcpiTableIdMax
>   } ESTD_ACPI_TABLE_ID;
>   
> diff --git a/DynamicTablesPkg/Library/Acpi/AcpiSsdtHpetLib/AcpiSsdtHpetLib.inf b/DynamicTablesPkg/Library/Acpi/AcpiSsdtHpetLib/AcpiSsdtHpetLib.inf
> new file mode 100644
> index 0000000000..7586b31adf
> --- /dev/null
> +++ b/DynamicTablesPkg/Library/Acpi/AcpiSsdtHpetLib/AcpiSsdtHpetLib.inf
> @@ -0,0 +1,36 @@
> +## @file
> +#  SSDT 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      = AcpiSsdtHpetLib
> +  FILE_GUID      = 85262912-AD7F-4EE0-8BB1-EE177275A54E
> +  VERSION_STRING = 1.0
> +  MODULE_TYPE    = DXE_DRIVER
> +  LIBRARY_CLASS  = NULL|DXE_DRIVER
> +  CONSTRUCTOR    = AcpiSsdtHpetLibConstructor
> +  DESTRUCTOR     = AcpiSsdtHpetLibDestructor
> +
> +[Sources]
> +  SsdtHpetGenerator.c
> +
> +[Packages]
> +  DynamicTablesPkg/DynamicTablesPkg.dec
> +  MdeModulePkg/MdeModulePkg.dec
> +  MdePkg/MdePkg.dec
> +  PcAtChipsetPkg/PcAtChipsetPkg.dec
> +
> +[LibraryClasses]
> +  AcpiHelperLib
> +  AmlLib
> +  BaseLib
> +  DebugLib
> +  PcdLib
> +
> +[Pcd]
> +  gPcAtChipsetPkgTokenSpaceGuid.PcdHpetBaseAddress

Cf. [1], I think this should be removed along the dependency over the
PcAtChipsetPkg dependency.

> diff --git a/DynamicTablesPkg/Library/Acpi/AcpiSsdtHpetLib/SsdtHpetGenerator.c b/DynamicTablesPkg/Library/Acpi/AcpiSsdtHpetLib/SsdtHpetGenerator.c
> new file mode 100644
> index 0000000000..3d401204ae
> --- /dev/null
> +++ b/DynamicTablesPkg/Library/Acpi/AcpiSsdtHpetLib/SsdtHpetGenerator.c
> @@ -0,0 +1,266 @@
> +/** @file
> +  SSDT 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

Is it possible to reference the HPET spec. with a link aswell if possible ?
Same comment for the other generators with their respective spec.

> +
> +**/
> +
> +#include <AcpiTableGenerator.h>
> +#include <ConfigurationManagerHelper.h>
> +#include <ConfigurationManagerObject.h>
> +#include <Library/AcpiHelperLib.h>
> +#include <Library/AmlLib/AmlLib.h>
> +#include <Library/BaseMemoryLib.h>
> +#include <Library/DebugLib.h>
> +#include <Library/MemoryAllocationLib.h>
> +#include <Library/TableHelperLib.h>
> +
> +/** 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')
> +
> +/** This macro defines the HPET Table Generator revision.
> +*/
> +#define HPET_GENERATOR_REVISION  CREATE_REVISION (1, 0)

Is it possible to move this definition down, next to the ACPI_TABLE_GENERATOR
definition (just for consistency with other generators).
Same remark for the other generators added in this patchset.

> +
> +#define SB_SCOPE  "\\_SB_"
> +
> +/** Construct the SSDT HPET devices Table.
> +
> +  This function invokes the Configuration Manager protocol interface
> +  to get the required hardware information for generating the ACPI
> +  table if required.
> +
> +  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
> +BuildSsdtHpetTable (
> +  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;
> +  AML_ROOT_NODE_HANDLE    RootNode;
> +  AML_OBJECT_NODE_HANDLE  ScopeNode;
> +  AML_OBJECT_NODE_HANDLE  HpetNode;
> +  AML_OBJECT_NODE_HANDLE  CrsNode;
> +  UINT32                  EisaId;
> +
> +  ASSERT (This != NULL);
> +  ASSERT (AcpiTableInfo != NULL);
> +  ASSERT (CfgMgrProtocol != NULL);
> +  ASSERT (Table != NULL);
> +  ASSERT (AcpiTableInfo->TableGeneratorId == This->GeneratorID);
> +  ASSERT (AcpiTableInfo->AcpiTableSignature == This->AcpiTableSignature);
> +
> +  Status = AddSsdtAcpiHeader (
> +             CfgMgrProtocol,
> +             This,
> +             AcpiTableInfo,
> +             &RootNode
> +             );
> +  if (EFI_ERROR (Status)) {
> +    return Status;
> +  }
> +
> +  Status = AmlCodeGenScope (SB_SCOPE, RootNode, &ScopeNode);
> +  if (EFI_ERROR (Status)) {
> +    goto exit_handler;
> +  }
> +
> +  Status = AmlCodeGenDevice ("HPET", ScopeNode, &HpetNode);
> +  if (EFI_ERROR (Status)) {
> +    ASSERT_EFI_ERROR (Status);
> +    goto exit_handler;
> +  }
> +
> +  Status = AmlGetEisaIdFromString ("PNP0103", &EisaId);
> +  if (EFI_ERROR (Status)) {
> +    ASSERT_EFI_ERROR (Status);
> +    goto exit_handler;
> +  }
> +
> +  Status = AmlCodeGenNameInteger ("_HID", EisaId, HpetNode, NULL);
> +  if (EFI_ERROR (Status)) {
> +    ASSERT_EFI_ERROR (Status);
> +    goto exit_handler;
> +  }
> +
> +  Status = AmlCodeGenNameInteger ("_UID", 0x00, HpetNode, NULL);
> +  if (EFI_ERROR (Status)) {
> +    ASSERT_EFI_ERROR (Status);
> +    goto exit_handler;
> +  }
> +
> +  Status = AmlCodeGenNameResourceTemplate ("_CRS", HpetNode, &CrsNode);
> +  if (EFI_ERROR (Status)) {
> +    ASSERT_EFI_ERROR (Status);
> +    goto exit_handler;
> +  }
> +
> +  Status = AmlCodeGenRdMemory32Fixed (FALSE, PcdGet32 (PcdHpetBaseAddress), SIZE_1KB, CrsNode, NULL);

I didn't find anything in the spec. stating the memory range should be read only or
read-write. There are examples for both in edk2-platforms. Did you see something
specific going in the way of a read only configuration ?

---

[1]

- to avoid making the DynamicTablesPkg dependent over the PcAtChipsetPkg.dec
     package (not that it is explicitly forbidden).
- to avoid making the DynamicTablesPkg fetch configuration information from
     PCDs instead of CmObjects (there is a counter example, but this should be avoided
     I think).
would it be possible to create a HPET object ? The base address would be fetched
through this object instead. The object would contain:
- UINT32 BaseAddress
- (maybe, depending on whether this is usefull) BOOLEAN IsReadOnly

This would mean that the ConfigurationManager used for your platform
will have the dependency over the PcAtChipsetPkg and populate the HPET
CmObject with:
    PcdGet32 (PcdHpetBaseAddress)


> +  if (EFI_ERROR (Status)) {
> +    ASSERT_EFI_ERROR (Status);
> +    goto exit_handler;
> +  }
> +
> +  Status = AmlSerializeDefinitionBlock (
> +             RootNode,
> +             Table
> +             );
> +  if (EFI_ERROR (Status)) {
> +    DEBUG ((
> +      DEBUG_ERROR,
> +      "ERROR: SSDT-HPET: Failed to Serialize SSDT Table Data."
> +      " Status = %r\n",
> +      Status
> +      ));
> +    goto exit_handler;
> +  }
> +
> +exit_handler:
> +  // Delete the RootNode and its attached children.
> +  return AmlDeleteTree (RootNode);
> +}
> +
> +/** Free any resources allocated for constructing the
> +    SSDT HPET ACPI table.
> +
> +  @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 [in, out] Table          Pointer to the ACPI Table.
> +
> +  @retval EFI_SUCCESS           The resources were freed successfully.
> +  @retval EFI_INVALID_PARAMETER The table pointer is NULL or invalid.
> +**/
> +STATIC
> +EFI_STATUS
> +FreeSsdtHpetTableResources (
> +  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,
> +  IN OUT        EFI_ACPI_DESCRIPTION_HEADER          **CONST  Table
> +  )
> +{
> +  ASSERT (This != NULL);
> +  ASSERT (AcpiTableInfo != NULL);
> +  ASSERT (CfgMgrProtocol != NULL);
> +  ASSERT (AcpiTableInfo->TableGeneratorId == This->GeneratorID);
> +  ASSERT (AcpiTableInfo->AcpiTableSignature == This->AcpiTableSignature);
> +
> +  if ((Table == NULL) || (*Table == NULL)) {
> +    DEBUG ((DEBUG_ERROR, "ERROR: SSDT-HPET: Invalid Table Pointer\n"));
> +    ASSERT ((Table != NULL) && (*Table != NULL));
> +    return EFI_INVALID_PARAMETER;
> +  }
> +
> +  FreePool (*Table);
> +  *Table = NULL;
> +  return EFI_SUCCESS;
> +}
> +
> +/** The interface for the SSDT HPET Table Generator.
> +*/
> +STATIC
> +CONST
> +ACPI_TABLE_GENERATOR  mSsdtPlatHpetGenerator = {
> +  // Generator ID
> +  CREATE_STD_ACPI_TABLE_GEN_ID (EStdAcpiTableIdSsdtHpet),
> +  // Generator Description
> +  L"ACPI.STD.SSDT.HPET.GENERATOR",
> +  // ACPI Table Signature
> +  EFI_ACPI_6_5_SECONDARY_SYSTEM_DESCRIPTION_TABLE_SIGNATURE,
> +  // ACPI Table Revision - Unused
> +  0,
> +  // Minimum ACPI Table Revision - Unused
> +  0,
> +  // Creator ID
> +  TABLE_GENERATOR_CREATOR_ID_GENERIC,

@Sami, shouldn't all the generators use this Creator Id instead of 'ARMH' ?
If yes the definition of:
     TABLE_GENERATOR_CREATOR_ID_GENERIC
should be placed there I think:
     DynamicTablesPkg/Include/AcpiTableGenerator.h

> +  // Creator Revision
> +  HPET_GENERATOR_REVISION,
> +  // Build Table function
> +  BuildSsdtHpetTable,
> +  // Free Resource function
> +  FreeSsdtHpetTableResources,
> +  // FreeSsdtPlatDevicesTableResources,

Should be removed I think.

> +  // 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
> +AcpiSsdtHpetLibConstructor (
> +  IN  EFI_HANDLE        ImageHandle,
> +  IN  EFI_SYSTEM_TABLE  *SystemTable
> +  )
> +{
> +  EFI_STATUS  Status;
> +
> +  Status = RegisterAcpiTableGenerator (&mSsdtPlatHpetGenerator);
> +  DEBUG ((DEBUG_INFO, "SSDT-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
> +AcpiSsdtHpetLibDestructor (
> +  IN  EFI_HANDLE        ImageHandle,
> +  IN  EFI_SYSTEM_TABLE  *SystemTable
> +  )
> +{
> +  EFI_STATUS  Status;
> +
> +  Status = DeregisterAcpiTableGenerator (&mSsdtPlatHpetGenerator);
> +  DEBUG ((DEBUG_INFO, "SSDT-HPET: Deregister Generator. Status = %r\n", Status));
> +  ASSERT_EFI_ERROR (Status);
> +  return Status;
> +}


Regards,
Pierre


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