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

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

Thanks Pierre for the review, I'll address the review comments.

Please see inline for my reply

On 11-03-2024 19:46, Pierre Gondois wrote:
> Caution: This message originated from an External Source. Use proper 
> caution when opening attachments, clicking links, or responding.
>
>
> 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 ?
>
[Abdul], Yes I also didnt find any information. I think we can go with 
read only configuration which is safe.

> ---
>
> [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 (#116712): https://edk2.groups.io/g/devel/message/116712
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]
-=-=-=-=-=-=-=-=-=-=-=-