Re: [PATCH edk2-platforms v1 5/6] Platform/ARM: Add OEM CPU generator for FVP

[Sami Mujawar <Sami.Mujawar@arm.com>]

Hi Ard,

Please see my response inline.

Regards,

Sami Mujawar

-----Original Message-----
From: Ard Biesheuvel <ard.biesheuvel@linaro.org> 
Sent: 21 December 2018 05:08 PM
To: Sami Mujawar <Sami.Mujawar@arm.com>
Cc: edk2-devel@lists.01.org; Arvind Chauhan <Arvind.Chauhan@arm.com>; Daniil Egranov <Daniil.Egranov@arm.com>; Thomas Abraham <thomas.abraham@arm.com>; Leif Lindholm <leif.lindholm@linaro.org>; Kinney, Michael D <michael.d.kinney@intel.com>; Alexei Fedorov <Alexei.Fedorov@arm.com>; Matteo Carlini <Matteo.Carlini@arm.com>; Stephanie Hughes-Fitt <Stephanie.Hughes-Fitt@arm.com>; nd <nd@arm.com>
Subject: Re: [PATCH edk2-platforms v1 5/6] Platform/ARM: Add OEM CPU generator for FVP

Hi Sami,

On Fri, 21 Dec 2018 at 18:01, Sami Mujawar <sami.mujawar@arm.com> wrote:
>
> Add support for dynamic generation of ACPI CPU device information.
> This generator uses the compiled data from a template asl file and 
> patches it at runtime to generate the CPU information based on the 
> number of CPUs and their ACPI UID. This patched data is then installed 
> as a SSDT table.
>
> Contributed-under: TianoCore Contribution Agreement 1.1
> Signed-off-by: Sami Mujawar <sami.mujawar@arm.com>
> ---
>  Platform/ARM/VExpressPkg/ConfigurationManager/AcpiOemCpuGeneratorLib/AcpiOemCpuASLLib.inf       |  27 ++
>  Platform/ARM/VExpressPkg/ConfigurationManager/AcpiOemCpuGeneratorLib/AcpiOemCpuGeneratorLib.inf |  42 ++
>  Platform/ARM/VExpressPkg/ConfigurationManager/AcpiOemCpuGeneratorLib/OemCpuGenerator.c          | 403 ++++++++++++++++++++
>  Platform/ARM/VExpressPkg/ConfigurationManager/AcpiOemCpuGeneratorLib/OemCpuGenerator.h          |  23 ++
>  Platform/ARM/VExpressPkg/ConfigurationManager/AcpiOemCpuGeneratorLib/SsdtCpuTemplate.asl        |  25 ++
>  5 files changed, 520 insertions(+)
>
...
> diff --git 
> a/Platform/ARM/VExpressPkg/ConfigurationManager/AcpiOemCpuGeneratorLib
> /OemCpuGenerator.c 
> b/Platform/ARM/VExpressPkg/ConfigurationManager/AcpiOemCpuGeneratorLib
> /OemCpuGenerator.c
> new file mode 100644
> index 
> 0000000000000000000000000000000000000000..d544244bfbd566c128b57d80b0c8
> c2bdc0cca374
> --- /dev/null
> +++ b/Platform/ARM/VExpressPkg/ConfigurationManager/AcpiOemCpuGenerato
> +++ rLib/OemCpuGenerator.c
> @@ -0,0 +1,403 @@
> +/** @file
> +  OEM CPU Table Generator
> +
> +  Copyright (c) 2018, ARM Limited. All rights reserved.
> +  This program and the accompanying materials  are licensed and made 
> + available under the terms and conditions of the BSD License  which 
> + accompanies this distribution.  The full text of the license may be 
> + found at  http://opensource.org/licenses/bsd-license.php
> +
> +  THE PROGRAM IS DISTRIBUTED UNDER THE BSD LICENSE ON AN "AS IS" 
> +BASIS,
> +  WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER EXPRESS OR IMPLIED.
> +**/
> +
> +#include <Library/AcpiLib.h>
> +#include <Library/BaseLib.h>
> +#include <Library/BaseMemoryLib.h>
> +#include <Library/DebugLib.h>
> +#include <Library/MemoryAllocationLib.h> #include 
> +<Library/UefiBootServicesTableLib.h>
> +#include <Protocol/AcpiTable.h>
> +
> +// Module specific include files.
> +#include <AcpiTableGenerator.h>
> +#include <ConfigurationManagerObject.h> #include 
> +<ConfigurationManagerHelper.h> #include <Library/TableHelperLib.h> 
> +#include <Protocol/ConfigurationManagerProtocol.h>
> +#include <Protocol/DynamicTableFactoryProtocol.h>
> +
> +#include <OemCpuGenerator.h>
> +
> +// AML Code Include files generated by iASL Compiler #include 
> +<SsdtCpuTemplate.hex>
> +
> +// AML Code offsets file generated by iASL Compiler #include 
> +<SsdtCpuTemplate.offset.h>
> +

Apologies if I should have spotted this before, but this is a no-go.
We are relying on intermediate output of some version of the IASL compiler here, which [AFAIK] is not formally specified or documented.
We cannot base an elaborate framework like DynamicTables on this.

I guess this only affects DSDT/SSDT generation, right?
[SAMI] Yes. The last 2 patches in this series add this feature. We probably need more discussion on this topic.
Until then can we proceed with review of the remaining patches, or should I submit a new patch series that drops the last 2 patches?

> +STATIC EDKII_DYNAMIC_TABLE_FACTORY_PROTOCOL  * mTableFactoryProtocol 
> += NULL;
> +
> +/** OEM CPU Generator
> +
> +Requirements:
> +  The following Configuration Manager Object(s) are required by
> +  this Generator:
> +  - EArmObjGicCInfo
> +*/
> +
> +/** This macro expands to a function that retrieves the GIC
> +    CPU interface Information from the Configuration Manager.
> +*/
> +GET_OBJECT_LIST (
> +  EObjNameSpaceArm,
> +  EArmObjGicCInfo,
> +  CM_ARM_GICC_INFO
> +  );
> +
> +STATIC
> +CHAR8
> +AsciiFromHex (
> +  UINTN x
> +  )
> +{
> +  if (x < 10) {
> +    return x + '0';
> +  }
> +
> +  if (x < 16) {
> +    return x - 10 + 'A';
> +  }
> +
> +  ASSERT (FALSE);
> +  return -1;
> +}
> +
> +/** Free any resources allocated for constructing the tables.
> +
> +  @param [in]      This           Pointer to the ACPI 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 list of ACPI Table(s).
> +  @param [in]      TableCount     Number of ACPI table(s).
> +
> +  @retval EFI_SUCCESS           The resources were freed successfully.
> +  @retval EFI_INVALID_PARAMETER The table pointer is NULL or invalid.
> +**/
> +STATIC
> +EFI_STATUS
> +FreeOemCpuTableResourcesEx (
> +  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,
> +  IN      CONST UINTN                                          TableCount
> +  )
> +{
> +  EFI_ACPI_DESCRIPTION_HEADER    ** TableList = NULL;
> +  UINTN                             Index;
> +
> +  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: OEM-CPU: Invalid Table Pointer\n"));
> +    ASSERT ((Table != NULL) && (*Table != NULL));
> +    return EFI_INVALID_PARAMETER;
> +  }
> +
> +  TableList = *Table;
> +
> +  for (Index = 0; Index < TableCount; Index++) {
> +    if (TableList[Index] != NULL) {
> +      // Free the table data
> +      FreePool (TableList[Index]);
> +      TableList[Index] = NULL;
> +    }
> +  }
> +
> +  // Free the table list
> +  FreePool (*Table);
> +  *Table = NULL;
> +  return EFI_SUCCESS;
> +}
> +
> +/** Construct the ACPI table using the ACPI table data provided.
> +
> +  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 FreeXXXXTableResourcesEx 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 a list of generated ACPI table(s).
> +  @param [out] TableCount     Number of generated ACPI table(s).
> +
> +  @retval EFI_SUCCESS           Table generated successfully.
> +  @retval EFI_INVALID_PARAMETER A parameter is invalid.
> +**/
> +STATIC
> +EFI_STATUS
> +EFIAPI
> +BuildOemCpuTableEx (
> +  IN  CONST ACPI_TABLE_GENERATOR                   *       This,
> +  IN  CONST CM_STD_OBJ_ACPI_TABLE_INFO             * CONST AcpiTableInfo,
> +  IN  CONST EDKII_CONFIGURATION_MANAGER_PROTOCOL   * CONST CfgMgrProtocol,
> +  OUT       EFI_ACPI_DESCRIPTION_HEADER          ***       Table,
> +  OUT       UINTN                                  * CONST TableCount
> +  )
> +{
> +  EFI_STATUS                        Status;
> +  UINT32                            GicCCount;
> +  CM_ARM_GICC_INFO                * GicCInfo;
> +  EFI_ACPI_DESCRIPTION_HEADER    ** TableList = NULL;
> +  CHAR8                           * TableData;
> +  CHAR8                           * PatchData;
> +  UINTN                             Index;
> +
> +  ASSERT (This != NULL);
> +  ASSERT (AcpiTableInfo != NULL);
> +  ASSERT (CfgMgrProtocol != NULL);
> +  ASSERT (Table != NULL);
> +  ASSERT (AcpiTableInfo->TableGeneratorId == This->GeneratorID);
> +
> +  *Table = NULL;
> +
> +  Status = GetEArmObjGicCInfo (
> +             CfgMgrProtocol,
> +             CM_NULL_TOKEN,
> +             &GicCInfo,
> +             &GicCCount
> +             );
> +  if (EFI_ERROR (Status)) {
> +    DEBUG ((
> +      DEBUG_ERROR,
> +      "ERROR: OEM-CPU: Failed to get GICC Info. Status = %r\n",
> +      Status
> +      ));
> +    goto error_handler;
> +  }
> +
> +  if (GicCCount == 0) {
> +    DEBUG ((
> +      DEBUG_ERROR,
> +      "ERROR: OEM-CPU: GIC CPU Interface information not provided.\n"
> +      ));
> +    ASSERT (GicCCount != 0);
> +    Status = EFI_INVALID_PARAMETER;
> +    goto error_handler;
> +  }
> +
> +  // Update the table count
> +  *TableCount = GicCCount;
> +
> +  // Allocate storage for the Table list  TableList = 
> + (EFI_ACPI_DESCRIPTION_HEADER**)
> +              AllocateZeroPool (
> +                (GicCCount * sizeof (EFI_ACPI_DESCRIPTION_HEADER*))
> +                );
> +  if (TableList == NULL) {
> +    Status = EFI_OUT_OF_RESOURCES;
> +    DEBUG ((
> +      DEBUG_ERROR,
> +      "ERROR: OEM-CPU: Failed to allocate memory for Table List," \
> +      " Status = %r\n",
> +      Status
> +      ));
> +    goto error_handler;
> +  }
> +
> +  // Update the table list pointer
> +  *Table = TableList;
> +
> +  for (Index = 0; Index < GicCCount; Index++) {
> +    UINT32 AcpiProcessorUid = GicCInfo[Index].AcpiProcessorUid;
> +
> +    // This generator implementation supports maximum 256 CPUs (IDS 0 - 255)
> +    if (AcpiProcessorUid > 0xFF) {
> +      Status = EFI_INVALID_PARAMETER;
> +      goto error_handler;
> +    }
> +
> +    TableData = (CHAR8*)AllocateZeroPool (sizeof (ssdtcputemplate_aml_code));
> +    if (TableData == NULL) {
> +      Status = EFI_OUT_OF_RESOURCES;
> +      DEBUG ((
> +        DEBUG_ERROR,
> +        "ERROR: OEM-CPU: Failed to allocate memory for Table," \
> +        " Status = %r\n",
> +        Status
> +        ));
> +      goto error_handler;
> +    }
> +
> +    // Store the table data pointer in the list
> +    TableList[Index] = (EFI_ACPI_DESCRIPTION_HEADER*)TableData;
> +    // Copy from template
> +    CopyMem (
> +      (VOID *)(UINTN)TableData,
> +      (VOID *)(UINTN)ssdtcputemplate_aml_code,
> +      sizeof (ssdtcputemplate_aml_code)
> +      );
> +
> +    // Update relevent sections in the table
> +    // 1. Update DEVICE name
> +    // {"_SB_.CP5A",
> +    //  0x5B82, 0x0000002D, 0x00, 0x00000000, 0x0000000000000000}
> +    DEBUG ((
> +      DEBUG_INFO,
> +      "Pathname = %a, NamesegOffset = 0x%x, Offset = 0x%x, Value = 0x%lx\n",
> +      SSDT_ARM_VEXP_OffsetTable[OFFSET_TABLE_DEVICE_INDEX].Pathname,
> +      SSDT_ARM_VEXP_OffsetTable[OFFSET_TABLE_DEVICE_INDEX].NamesegOffset,
> +      SSDT_ARM_VEXP_OffsetTable[OFFSET_TABLE_DEVICE_INDEX].Offset,
> +      SSDT_ARM_VEXP_OffsetTable[OFFSET_TABLE_DEVICE_INDEX].Value
> +      ));
> +
> +    PatchData = TableData +
> +      SSDT_ARM_VEXP_OffsetTable[OFFSET_TABLE_DEVICE_INDEX].NamesegOffset;
> +    // PatchData[0] - 'C'
> +    // PatchData[1] - 'P'
> +    // PatchData[2] - Tens Digit
> +    // PatchData[3] - Units Digit
> +    PatchData[2] = AsciiFromHex ((AcpiProcessorUid >> 4) & 0xF);
> +    PatchData[3] = AsciiFromHex (AcpiProcessorUid & 0xF);
> +
> +    // 2. Update UID value
> +    // {"_SB_.CP5A._UID",
> +    //  0x0008, 0x00000041, 0x0A, 0x00000046, 0x00000000000000A5}
> +    DEBUG ((
> +      DEBUG_INFO,
> +      "Pathname = %a, NamesegOffset = 0x%x, Offset = 0x%x, Value = 0x%lx\n",
> +      SSDT_ARM_VEXP_OffsetTable[OFFSET_TABLE_UID_INDEX].Pathname,
> +      SSDT_ARM_VEXP_OffsetTable[OFFSET_TABLE_UID_INDEX].NamesegOffset,
> +      SSDT_ARM_VEXP_OffsetTable[OFFSET_TABLE_UID_INDEX].Offset,
> +      SSDT_ARM_VEXP_OffsetTable[OFFSET_TABLE_UID_INDEX].Value
> +      ));
> +
> +    PatchData = TableData +
> +      SSDT_ARM_VEXP_OffsetTable[OFFSET_TABLE_UID_INDEX].Offset;
> +    PatchData[0] = AcpiProcessorUid;
> +  } // for
> +
> +  return Status;
> +
> +error_handler:
> +  // Free up the allocated resources in case of an error
> +  FreeOemCpuTableResourcesEx (
> +    This,
> +    AcpiTableInfo,
> +    CfgMgrProtocol,
> +    Table,
> +    *TableCount
> +    );
> +  return Status;
> +}
> +
> +/** This macro defines the Raw Generator revision.
> +*/
> +#define OEM_CPU_GENERATOR_REVISION CREATE_REVISION (1, 0)
> +
> +/** The interface for the Raw Table Generator.
> +*/
> +STATIC
> +CONST
> +ACPI_TABLE_GENERATOR OemCpuGenerator = {
> +  // Generator ID
> +  CREATE_OEM_ACPI_TABLE_GEN_ID (OEM_ACPI_TABLE_ID_CPU),
> +  // Generator Description
> +  L"ACPI.OEM.CPU.GENERATOR",
> +  // ACPI Table Signature - Unused
> +  0,
> +  // ACPI Table Revision - Unused
> +  0,
> +  // Creator ID
> +  TABLE_GENERATOR_CREATOR_ID_ARM,
> +  // Creator Revision
> +  OEM_CPU_GENERATOR_REVISION,
> +  // Build Table function not implemented
> +  // as this generator implements the extended
> +  // version
> +  NULL,
> +  NULL,
> +  // Extended build table function
> +  BuildOemCpuTableEx,
> +  // Free resources allocated by extended build table interface
> +  FreeOemCpuTableResourcesEx
> +};
> +
> +/** 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
> +AcpiOemCpuGeneratorLibConstructor (
> +  IN CONST EFI_HANDLE                ImageHandle,
> +  IN       EFI_SYSTEM_TABLE  * CONST SystemTable
> +  )
> +{
> +  EFI_STATUS  Status;
> +  // Locate the Dynamic Table Factory
> +  Status = gBS->LocateProtocol (
> +                  &gEdkiiDynamicTableFactoryProtocolGuid,
> +                  NULL,
> +                  (VOID**)&mTableFactoryProtocol
> +                  );
> +  if (EFI_ERROR (Status)) {
> +    DEBUG ((
> +      DEBUG_ERROR,
> +      "ERROR: Failed to find Dynamic Table Factory protocol." \
> +      " Status = %r\n",
> +      Status
> +      ));
> +    return Status;
> +  }
> +
> +  Status = mTableFactoryProtocol->RegisterAcpiTableGenerator 
> +(&OemCpuGenerator);
> +  DEBUG ((DEBUG_INFO, "OEM-CPU: 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
> +AcpiOemCpuGeneratorLibDestructor (
> +  IN CONST EFI_HANDLE                ImageHandle,
> +  IN       EFI_SYSTEM_TABLE  * CONST SystemTable
> +  )
> +{
> +  EFI_STATUS  Status;
> +  if (mTableFactoryProtocol != NULL) {
> +    Status = mTableFactoryProtocol->DeregisterAcpiTableGenerator (
> +                                      &OemCpuGenerator
> +                                      );
> +  } else {
> +    Status = EFI_NOT_FOUND;
> +  }
> +  DEBUG ((DEBUG_INFO, "OEM-CPU: Deregister Generator. Status = %r\n", 
> +Status));
> +  ASSERT_EFI_ERROR (Status);
> +  return Status;
> +}
> diff --git 
> a/Platform/ARM/VExpressPkg/ConfigurationManager/AcpiOemCpuGeneratorLib
> /OemCpuGenerator.h 
> b/Platform/ARM/VExpressPkg/ConfigurationManager/AcpiOemCpuGeneratorLib
> /OemCpuGenerator.h
> new file mode 100644
> index 
> 0000000000000000000000000000000000000000..49f75202840af3e37d3ac4b31a6d
> 9b1d9673072f
> --- /dev/null
> +++ b/Platform/ARM/VExpressPkg/ConfigurationManager/AcpiOemCpuGenerato
> +++ rLib/OemCpuGenerator.h
> @@ -0,0 +1,23 @@
> +/** @file
> +
> +  Copyright (c) 2018, ARM Limited. All rights reserved.
> +
> +  This program and the accompanying materials  are licensed and made 
> + available under the terms and conditions of the BSD License  which 
> + accompanies this distribution.  The full text of the license may be 
> + found at  http://opensource.org/licenses/bsd-license.php
> +
> +  THE PROGRAM IS DISTRIBUTED UNDER THE BSD LICENSE ON AN "AS IS" 
> + BASIS,  WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER EXPRESS OR IMPLIED.
> +
> +**/
> +
> +#ifndef OEMCPUGENERATOR_H__
> +#define OEMCPUGENERATOR_H__
> +
> +#define OEM_ACPI_TABLE_ID_CPU       1
> +
> +#define OFFSET_TABLE_DEVICE_INDEX   1
> +#define OFFSET_TABLE_UID_INDEX      2
> +
> +#endif // OEMCPUGENERATOR_H__
> diff --git 
> a/Platform/ARM/VExpressPkg/ConfigurationManager/AcpiOemCpuGeneratorLib
> /SsdtCpuTemplate.asl 
> b/Platform/ARM/VExpressPkg/ConfigurationManager/AcpiOemCpuGeneratorLib
> /SsdtCpuTemplate.asl
> new file mode 100644
> index 
> 0000000000000000000000000000000000000000..11d9025044e0b853af12e6cc3dd7
> f3cfe87a1ecb
> --- /dev/null
> +++ b/Platform/ARM/VExpressPkg/ConfigurationManager/AcpiOemCpuGenerato
> +++ rLib/SsdtCpuTemplate.asl
> @@ -0,0 +1,25 @@
> +/** @file
> +  SSDT Template for CPU
> +
> +  Copyright (c) 2018, ARM Ltd. All rights reserved.<BR>
> +    This program and the accompanying materials  are licensed and 
> + made available under the terms and conditions of the BSD License  
> + which accompanies this distribution.  The full text of the license 
> + may be found at  http://opensource.org/licenses/bsd-license.php
> +
> +  THE PROGRAM IS DISTRIBUTED UNDER THE BSD LICENSE ON AN "AS IS" 
> + BASIS,  WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER EXPRESS OR IMPLIED.
> +
> +**/
> +
> +DefinitionBlock("SsdtCpu.aml", "SSDT", 1, "ARMLTD", "ARM-VEXP", 1) {
> +  Scope(_SB) {
> +    //
> +    // Processor
> +    //
> +    Device(CP5A) {
> +      Name(_HID, "ACPI0007")
> +      Name(_UID, 0xA5)
> +    }
> +  }
> +}
> --
> 'Guid(CE165669-3EF3-493F-B85D-6190EE5B9759)'
>
>