On Fri, 21 Dec 2018 at 19:14, Sami Mujawar <sami.muja...@arm.com> wrote:
>
> Hi Ard,
>
> Please see my response inline.
>
> Regards,
>
> Sami Mujawar
>
> -----Original Message-----
> From: Ard Biesheuvel <ard.biesheu...@linaro.org>
> Sent: 21 December 2018 05:08 PM
> To: Sami Mujawar <sami.muja...@arm.com>
> Cc: edk2-devel@lists.01.org; Arvind Chauhan <arvind.chau...@arm.com>; Daniil
> Egranov <daniil.egra...@arm.com>; Thomas Abraham <thomas.abra...@arm.com>;
> Leif Lindholm <leif.lindh...@linaro.org>; Kinney, Michael D
> <michael.d.kin...@intel.com>; Alexei Fedorov <alexei.fedo...@arm.com>; Matteo
> Carlini <matteo.carl...@arm.com>; Stephanie Hughes-Fitt
> <stephanie.hughes-f...@arm.com>; nd <n...@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.muja...@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.muja...@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?
>
No worries, if they are logically separate, there is no need to resend.
> > +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)'
> >
> >
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
