[PATCH v1 0/5] Add ACPI support for Kvmtool

[PATCH v1 0/5] Add ACPI support for Kvmtool

[PierreGondois]

From: Pierre Gondois <Pierre.Gondois@arm.com>

Kvmtool dynamically generates a device tree describing the platform
to boot on. Using the patch-sets listed below, the DynamicTables
framework generates ACPI tables describing a similar platform.

This patch-set:
 - adds a ConfigurationManager allowing to generate ACPI tables
   for Kvmtool
 - adds the acpiview command line utility to the ArmVirtPkg
 - update ArmVirtPkg.ci.yaml to add new words and use the
   DynamicTablesPkg

This patch sets also set the default platform description format
to ACPI instead of the device tree (c.f.: PcdForceNoAcpi is set
to FALSE).

The changes can be seen at: https://github.com/PierreARM/edk2/tree/1456_Add_ACPI_support_for_Kvmtool_v1
The results of the CI can be seen at: https://github.com/tianocore/edk2/pull/1753

This patch-set is dependent over the following patch-sets:
  [PATCH v1 00/10] Various DynamicTablesPkg modifications 
  https://edk2.groups.io/g/devel/message/76929
and:
  [PATCH v1 00/13] Create a SSDT CPU topology generator 
  https://edk2.groups.io/g/devel/message/76941
and:
  [PATCH v1 0/7] Create a SSDT PCIe generator 
  https://edk2.groups.io/g/devel/message/76958
and:
  [PATCH v1 00/14] Implement a FdtHwInfoParserLib
  https://edk2.groups.io/g/devel/message/76967
and:
  [PATCH v1 0/5] Add DynamicPlatRepoLib
  https://edk2.groups.io/g/devel/message/76984

Pierre Gondois (1):
  ArmVirtPkg: Add cspell exceptions

Sami Mujawar (4):
  ArmVirtPkg: Add DSDT ACPI table for Kvmtool firmware
  ArmVirtPkg: Add Configuration Manager for Kvmtool firmware
  ArmVirtPkg: Enable ACPI support for Kvmtool
  ArmVirtPkg: Enable Acpiview for ArmVirtPkg

 ArmVirtPkg/ArmVirt.dsc.inc                    |   3 +-
 ArmVirtPkg/ArmVirtKvmTool.dsc                 |  18 +-
 ArmVirtPkg/ArmVirtKvmTool.fdf                 |  11 +
 ArmVirtPkg/ArmVirtPkg.ci.yaml                 |   3 +
 .../KvmtoolCfgMgrDxe/AslTables/Dsdt.asl       |  19 +
 .../KvmtoolCfgMgrDxe/ConfigurationManager.c   | 948 ++++++++++++++++++
 .../KvmtoolCfgMgrDxe/ConfigurationManager.h   |  94 ++
 .../ConfigurationManagerDxe.inf               |  58 ++
 8 files changed, 1151 insertions(+), 3 deletions(-)
 create mode 100644 ArmVirtPkg/KvmtoolCfgMgrDxe/AslTables/Dsdt.asl
 create mode 100644 ArmVirtPkg/KvmtoolCfgMgrDxe/ConfigurationManager.c
 create mode 100644 ArmVirtPkg/KvmtoolCfgMgrDxe/ConfigurationManager.h
 create mode 100644 ArmVirtPkg/KvmtoolCfgMgrDxe/ConfigurationManagerDxe.inf

-- 
2.17.1

[PATCH v1 1/5] ArmVirtPkg: Add cspell exceptions

[PierreGondois]

From: Pierre Gondois <Pierre.Gondois@arm.com>

The cpsell tool checks for unknown words in the upstream CI.
Add some new words to the list of exceptions.

Signed-off-by: Pierre Gondois <Pierre.Gondois@arm.com>
---
 ArmVirtPkg/ArmVirtPkg.ci.yaml | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/ArmVirtPkg/ArmVirtPkg.ci.yaml b/ArmVirtPkg/ArmVirtPkg.ci.yaml
index 5f427e57233e..e3f30d69e89a 100644
--- a/ArmVirtPkg/ArmVirtPkg.ci.yaml
+++ b/ArmVirtPkg/ArmVirtPkg.ci.yaml
@@ -47,6 +47,7 @@
             "MdePkg/MdePkg.dec",
             "MdeModulePkg/MdeModulePkg.dec",
             "ArmVirtPkg/ArmVirtPkg.dec",
+            "DynamicTablesPkg/DynamicTablesPkg.dec",
             "NetworkPkg/NetworkPkg.dec",
             "ArmPkg/ArmPkg.dec",
             "OvmfPkg/OvmfPkg.dec",
@@ -97,6 +98,8 @@
         "AuditOnly": False,           # Fails right now with over 270 errors
         "IgnoreFiles": [],           # use gitignore syntax to ignore errors in matching files
         "ExtendWords": [
+            "armltd",
+            "ssdts",
             "setjump",
             "plong",
             "lparam",
-- 
2.17.1

Re: [edk2-devel] [PATCH v1 1/5] ArmVirtPkg: Add cspell exceptions

[Laszlo Ersek]

On 06/23/21 16:06, PierreGondois wrote:
> From: Pierre Gondois <Pierre.Gondois@arm.com>
> 
> The cpsell tool checks for unknown words in the upstream CI.
> Add some new words to the list of exceptions.
> 
> Signed-off-by: Pierre Gondois <Pierre.Gondois@arm.com>
> ---
>  ArmVirtPkg/ArmVirtPkg.ci.yaml | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/ArmVirtPkg/ArmVirtPkg.ci.yaml b/ArmVirtPkg/ArmVirtPkg.ci.yaml
> index 5f427e57233e..e3f30d69e89a 100644
> --- a/ArmVirtPkg/ArmVirtPkg.ci.yaml
> +++ b/ArmVirtPkg/ArmVirtPkg.ci.yaml
> @@ -47,6 +47,7 @@
>              "MdePkg/MdePkg.dec",
>              "MdeModulePkg/MdeModulePkg.dec",
>              "ArmVirtPkg/ArmVirtPkg.dec",
> +            "DynamicTablesPkg/DynamicTablesPkg.dec",
>              "NetworkPkg/NetworkPkg.dec",
>              "ArmPkg/ArmPkg.dec",
>              "OvmfPkg/OvmfPkg.dec",
> @@ -97,6 +98,8 @@
>          "AuditOnly": False,           # Fails right now with over 270 errors
>          "IgnoreFiles": [],           # use gitignore syntax to ignore errors in matching files
>          "ExtendWords": [
> +            "armltd",
> +            "ssdts",
>              "setjump",
>              "plong",
>              "lparam",
> 

Reviewed-by: Laszlo Ersek <lersek@redhat.com>

[PATCH v1 2/5] ArmVirtPkg: Add DSDT ACPI table for Kvmtool firmware

[PierreGondois]

From: Sami Mujawar <sami.mujawar@arm.com>

Most ACPI tables for Kvmtool firmware are dynamically
generated. The AML code is also generated at runtime
for most components in appropriate SSDTs.

Although there may not be much to describe in the DSDT,
the DSDT table is mandatory.

Therefore, add an empty stub for DSDT.

Signed-off-by: Sami Mujawar <sami.mujawar@arm.com>
Signed-off-by: Pierre Gondois <Pierre.Gondois@arm.com>
---
 .../KvmtoolCfgMgrDxe/AslTables/Dsdt.asl       | 19 +++++++++++++++++++
 1 file changed, 19 insertions(+)
 create mode 100644 ArmVirtPkg/KvmtoolCfgMgrDxe/AslTables/Dsdt.asl

diff --git a/ArmVirtPkg/KvmtoolCfgMgrDxe/AslTables/Dsdt.asl b/ArmVirtPkg/KvmtoolCfgMgrDxe/AslTables/Dsdt.asl
new file mode 100644
index 000000000000..8467d1ede4ec
--- /dev/null
+++ b/ArmVirtPkg/KvmtoolCfgMgrDxe/AslTables/Dsdt.asl
@@ -0,0 +1,19 @@
+/** @file
+  Differentiated System Description Table Fields (DSDT)
+
+  Copyright (c) 2021, ARM Ltd. All rights reserved.<BR>
+    SPDX-License-Identifier: BSD-2-Clause-Patent
+
+**/
+
+DefinitionBlock ("DsdtTable.aml", "DSDT", 1, "ARMLTD", "ARM-KVMT", 1) {
+  Scope (_SB) {
+    // Most ACPI tables for Kvmtool firmware are
+    // dynamically generated. The AML code is also
+    // generated at runtime for most components in
+    // appropriate SSDTs.
+    // Although there may not be much to describe
+    // in the DSDT, the DSDT table is mandatory.
+    // Therefore, add an empty stub for DSDT.
+  } // Scope (_SB)
+}
-- 
2.17.1

Re: [edk2-devel] [PATCH v1 2/5] ArmVirtPkg: Add DSDT ACPI table for Kvmtool firmware

[Laszlo Ersek]

On 06/23/21 16:06, PierreGondois wrote:
> From: Sami Mujawar <sami.mujawar@arm.com>
> 
> Most ACPI tables for Kvmtool firmware are dynamically
> generated. The AML code is also generated at runtime
> for most components in appropriate SSDTs.
> 
> Although there may not be much to describe in the DSDT,
> the DSDT table is mandatory.
> 
> Therefore, add an empty stub for DSDT.
> 
> Signed-off-by: Sami Mujawar <sami.mujawar@arm.com>
> Signed-off-by: Pierre Gondois <Pierre.Gondois@arm.com>
> ---
>  .../KvmtoolCfgMgrDxe/AslTables/Dsdt.asl       | 19 +++++++++++++++++++
>  1 file changed, 19 insertions(+)
>  create mode 100644 ArmVirtPkg/KvmtoolCfgMgrDxe/AslTables/Dsdt.asl
> 
> diff --git a/ArmVirtPkg/KvmtoolCfgMgrDxe/AslTables/Dsdt.asl b/ArmVirtPkg/KvmtoolCfgMgrDxe/AslTables/Dsdt.asl
> new file mode 100644
> index 000000000000..8467d1ede4ec
> --- /dev/null
> +++ b/ArmVirtPkg/KvmtoolCfgMgrDxe/AslTables/Dsdt.asl
> @@ -0,0 +1,19 @@
> +/** @file
> +  Differentiated System Description Table Fields (DSDT)
> +
> +  Copyright (c) 2021, ARM Ltd. All rights reserved.<BR>
> +    SPDX-License-Identifier: BSD-2-Clause-Patent
> +
> +**/
> +
> +DefinitionBlock ("DsdtTable.aml", "DSDT", 1, "ARMLTD", "ARM-KVMT", 1) {
> +  Scope (_SB) {
> +    // Most ACPI tables for Kvmtool firmware are
> +    // dynamically generated. The AML code is also
> +    // generated at runtime for most components in
> +    // appropriate SSDTs.
> +    // Although there may not be much to describe
> +    // in the DSDT, the DSDT table is mandatory.
> +    // Therefore, add an empty stub for DSDT.
> +  } // Scope (_SB)
> +}
> 

Please insert empty // lines at the top and bottom of the comment block,
to stick more closely with the edk2 coding style.

Reviewed-by: Laszlo Ersek <lersek@redhat.com>

[PATCH v1 3/5] ArmVirtPkg: Add Configuration Manager for Kvmtool firmware

[PierreGondois]

From: Sami Mujawar <sami.mujawar@arm.com>

Add Configuration Manager to enable ACPI tables for Kvmtool
firmware. The Configuration Manager for Kvmtool uses the DT
Hardware Information Parser module (FdtHwInfoParser) to parse
the DT provided by Kvmtool. The FdtHwInfoParser parses the DT
and invokes the callback function HW_INFO_ADD_OBJECT to add
the Configuration Manager objects to the Platform Information
repository.

The information for some Configuration Manager objects may not
be available in the DT. Such objects are initialised locally
by the Configuration Manager.

Support for the following ACPI tables is provided:
 - DBG2
 - DSDT (Empty stub)
 - FADT
 - GTDT
 - MADT
 - SPCR
 - SSDT (Cpu Hierarchy)
 - SSDT (Pcie bus)

Signed-off-by: Sami Mujawar <sami.mujawar@arm.com>
Signed-off-by: Pierre Gondois <Pierre.Gondois@arm.com>
---
 ArmVirtPkg/ArmVirtKvmTool.dsc                 |   3 +
 .../KvmtoolCfgMgrDxe/ConfigurationManager.c   | 948 ++++++++++++++++++
 .../KvmtoolCfgMgrDxe/ConfigurationManager.h   |  94 ++
 .../ConfigurationManagerDxe.inf               |  58 ++
 4 files changed, 1103 insertions(+)
 create mode 100644 ArmVirtPkg/KvmtoolCfgMgrDxe/ConfigurationManager.c
 create mode 100644 ArmVirtPkg/KvmtoolCfgMgrDxe/ConfigurationManager.h
 create mode 100644 ArmVirtPkg/KvmtoolCfgMgrDxe/ConfigurationManagerDxe.inf

diff --git a/ArmVirtPkg/ArmVirtKvmTool.dsc b/ArmVirtPkg/ArmVirtKvmTool.dsc
index 3bd1cc72a1eb..920880796ac2 100644
--- a/ArmVirtPkg/ArmVirtKvmTool.dsc
+++ b/ArmVirtPkg/ArmVirtKvmTool.dsc
@@ -71,6 +71,9 @@ [LibraryClasses.common]
   PlatformHookLib|ArmVirtPkg/Library/Fdt16550SerialPortHookLib/Fdt16550SerialPortHookLib.inf
   SerialPortLib|MdeModulePkg/Library/BaseSerialPortLib16550/BaseSerialPortLib16550.inf
 
+  HwInfoParserLib|DynamicTablesPkg/Library/FdtHwInfoParserLib/FdtHwInfoParserLib.inf
+  DynamicPlatRepoLib|DynamicTablesPkg/Library/Common/DynamicPlatRepoLib/DynamicPlatRepoLib.inf
+
 [LibraryClasses.common.SEC, LibraryClasses.common.PEI_CORE, LibraryClasses.common.PEIM]
   PciExpressLib|MdePkg/Library/BasePciExpressLib/BasePciExpressLib.inf
   PlatformHookLib|ArmVirtPkg/Library/Fdt16550SerialPortHookLib/EarlyFdt16550SerialPortHookLib.inf
diff --git a/ArmVirtPkg/KvmtoolCfgMgrDxe/ConfigurationManager.c b/ArmVirtPkg/KvmtoolCfgMgrDxe/ConfigurationManager.c
new file mode 100644
index 000000000000..07b8b403dd4a
--- /dev/null
+++ b/ArmVirtPkg/KvmtoolCfgMgrDxe/ConfigurationManager.c
@@ -0,0 +1,948 @@
+/** @file
+  Configuration Manager Dxe
+
+  Copyright (c) 2021, Arm Limited. All rights reserved.<BR>
+
+  SPDX-License-Identifier: BSD-2-Clause-Patent
+
+  @par Glossary:
+    - Cm or CM   - Configuration Manager
+    - Obj or OBJ - Object
+**/
+
+#include <IndustryStandard/DebugPort2Table.h>
+#include <IndustryStandard/IoRemappingTable.h>
+#include <IndustryStandard/MemoryMappedConfigurationSpaceAccessTable.h>
+#include <IndustryStandard/SerialPortConsoleRedirectionTable.h>
+#include <Library/BaseMemoryLib.h>
+#include <Library/DebugLib.h>
+#include <Library/DynamicPlatRepoLib.h>
+#include <Library/HobLib.h>
+#include <Library/HwInfoParserLib.h>
+#include <Library/IoLib.h>
+#include <Library/PcdLib.h>
+#include <Library/TableHelperLib.h>
+#include <Library/UefiBootServicesTableLib.h>
+#include <Protocol/AcpiTable.h>
+#include <Protocol/ConfigurationManagerProtocol.h>
+
+#include "ConfigurationManager.h"
+
+/** The platform configuration repository information.
+*/
+STATIC
+EDKII_PLATFORM_REPOSITORY_INFO KvmtoolPlatRepositoryInfo = {
+  /// Configuration Manager information
+  { CONFIGURATION_MANAGER_REVISION, CFG_MGR_OEM_ID },
+
+  // ACPI Table List
+  {
+    // FADT Table
+    {
+      EFI_ACPI_6_3_FIXED_ACPI_DESCRIPTION_TABLE_SIGNATURE,
+      EFI_ACPI_6_3_FIXED_ACPI_DESCRIPTION_TABLE_REVISION,
+      CREATE_STD_ACPI_TABLE_GEN_ID (EStdAcpiTableIdFadt),
+      NULL
+    },
+    // GTDT Table
+    {
+      EFI_ACPI_6_3_GENERIC_TIMER_DESCRIPTION_TABLE_SIGNATURE,
+      EFI_ACPI_6_3_GENERIC_TIMER_DESCRIPTION_TABLE_REVISION,
+      CREATE_STD_ACPI_TABLE_GEN_ID (EStdAcpiTableIdGtdt),
+      NULL
+    },
+    // MADT Table
+    {
+      EFI_ACPI_6_3_MULTIPLE_APIC_DESCRIPTION_TABLE_SIGNATURE,
+      EFI_ACPI_6_3_MULTIPLE_APIC_DESCRIPTION_TABLE_REVISION,
+      CREATE_STD_ACPI_TABLE_GEN_ID (EStdAcpiTableIdMadt),
+      NULL
+    },
+    // SPCR Table
+    {
+      EFI_ACPI_6_3_SERIAL_PORT_CONSOLE_REDIRECTION_TABLE_SIGNATURE,
+      EFI_ACPI_SERIAL_PORT_CONSOLE_REDIRECTION_TABLE_REVISION,
+      CREATE_STD_ACPI_TABLE_GEN_ID (EStdAcpiTableIdSpcr),
+      NULL
+    },
+    // DSDT Table
+    {
+      EFI_ACPI_6_3_DIFFERENTIATED_SYSTEM_DESCRIPTION_TABLE_SIGNATURE,
+      0, // Unused
+      CREATE_STD_ACPI_TABLE_GEN_ID (EStdAcpiTableIdDsdt),
+      (EFI_ACPI_DESCRIPTION_HEADER*)dsdt_aml_code
+    },
+    // SSDT Cpu Hierarchy Table
+    {
+      EFI_ACPI_6_3_SECONDARY_SYSTEM_DESCRIPTION_TABLE_SIGNATURE,
+      0, // Unused
+      CREATE_STD_ACPI_TABLE_GEN_ID (EStdAcpiTableIdSsdtCpuTopology),
+      NULL
+    },
+    // DBG2 Table
+    {
+      EFI_ACPI_6_3_DEBUG_PORT_2_TABLE_SIGNATURE,
+      EFI_ACPI_DBG2_DEBUG_DEVICE_INFORMATION_STRUCT_REVISION,
+      CREATE_STD_ACPI_TABLE_GEN_ID (EStdAcpiTableIdDbg2),
+      NULL
+    },
+    // PCI MCFG Table
+    {
+      EFI_ACPI_6_3_PCI_EXPRESS_MEMORY_MAPPED_CONFIGURATION_SPACE_BASE_ADDRESS_DESCRIPTION_TABLE_SIGNATURE,
+      EFI_ACPI_MEMORY_MAPPED_CONFIGURATION_SPACE_ACCESS_TABLE_REVISION,
+      CREATE_STD_ACPI_TABLE_GEN_ID (EStdAcpiTableIdMcfg),
+      NULL
+    },
+    // SSDT table describing the PCI root complex
+    {
+      EFI_ACPI_6_3_SECONDARY_SYSTEM_DESCRIPTION_TABLE_SIGNATURE,
+      0, // Unused
+      CREATE_STD_ACPI_TABLE_GEN_ID (EStdAcpiTableIdSsdtPciExpress),
+      NULL
+    },
+    // IORT Table
+    {
+      EFI_ACPI_6_3_IO_REMAPPING_TABLE_SIGNATURE,
+      EFI_ACPI_IO_REMAPPING_TABLE_REVISION,
+      CREATE_STD_ACPI_TABLE_GEN_ID (EStdAcpiTableIdIort),
+      NULL
+    },
+  },
+
+  // Power management profile information
+  { EFI_ACPI_6_3_PM_PROFILE_ENTERPRISE_SERVER },    // PowerManagement Profile
+
+  // ITS group node
+  {
+    // Reference token for this Iort node
+    REFERENCE_TOKEN (ItsGroupInfo),
+    // The number of ITS identifiers in the ITS node.
+    1,
+    // Reference token for the ITS identifier array
+    REFERENCE_TOKEN (ItsIdentifierArray)
+  },
+  // ITS identifier array
+  {
+    {
+      // The ITS Identifier
+      0
+    }
+  },
+
+  // Root Complex node info
+  {
+    // Reference token for this Iort node
+    REFERENCE_TOKEN (RootComplexInfo),
+    // Number of ID mappings
+    1,
+    // Reference token for the ID mapping array
+    REFERENCE_TOKEN (DeviceIdMapping[0]),
+
+    // Memory access properties : Cache coherent attributes
+    EFI_ACPI_IORT_MEM_ACCESS_PROP_CCA,
+    // Memory access properties : Allocation hints
+    0,
+    // Memory access properties : Memory access flags
+    0,
+    // ATS attributes
+    EFI_ACPI_IORT_ROOT_COMPLEX_ATS_UNSUPPORTED,
+    // PCI segment number
+    0
+  },
+
+  // Array of Device ID mappings
+  {
+    /* RootComplex -> ITS Group
+    */
+    // Device ID mapping for Root complex node
+    {
+      // Input base
+      0x0,
+      // Number of input IDs
+      0x0000FFFF,
+      // Output Base
+      0x0,
+      // Output reference
+      REFERENCE_TOKEN (ItsGroupInfo),
+      // Flags
+      0
+    },
+  },
+};
+
+/** A helper function for returning the Configuration Manager Objects.
+
+  @param [in]       CmObjectId     The Configuration Manager Object ID.
+  @param [in]       Object         Pointer to the Object(s).
+  @param [in]       ObjectSize     Total size of the Object(s).
+  @param [in]       ObjectCount    Number of Objects.
+  @param [in, out]  CmObjectDesc   Pointer to the Configuration Manager Object
+                                   descriptor describing the requested Object.
+
+  @retval EFI_SUCCESS           Success.
+**/
+STATIC
+EFI_STATUS
+EFIAPI
+HandleCmObject (
+  IN  CONST CM_OBJECT_ID                CmObjectId,
+  IN        VOID                *       Object,
+  IN  CONST UINTN                       ObjectSize,
+  IN  CONST UINTN                       ObjectCount,
+  IN  OUT   CM_OBJ_DESCRIPTOR   * CONST CmObjectDesc
+  )
+{
+  CmObjectDesc->ObjectId = CmObjectId;
+  CmObjectDesc->Size = ObjectSize;
+  CmObjectDesc->Data = (VOID*)Object;
+  CmObjectDesc->Count = ObjectCount;
+  DEBUG ((
+    DEBUG_INFO,
+    "INFO: CmObjectId = %x, Ptr = 0x%p, Size = %d, Count = %d\n",
+    CmObjectId,
+    CmObjectDesc->Data,
+    CmObjectDesc->Size,
+    CmObjectDesc->Count
+    ));
+  return EFI_SUCCESS;
+}
+
+/** A helper function for returning the Configuration Manager Objects that
+    match the token.
+
+  @param [in]  This               Pointer to the Configuration Manager Protocol.
+  @param [in]  CmObjectId         The Configuration Manager Object ID.
+  @param [in]  Object             Pointer to the Object(s).
+  @param [in]  ObjectSize         Total size of the Object(s).
+  @param [in]  ObjectCount        Number of Objects.
+  @param [in]  Token              A token identifying the object.
+  @param [in]  HandlerProc        A handler function to search the object
+                                  referenced by the token.
+  @param [in, out]  CmObjectDesc  Pointer to the Configuration Manager Object
+                                  descriptor describing the requested Object.
+
+  @retval EFI_SUCCESS           Success.
+  @retval EFI_INVALID_PARAMETER A parameter is invalid.
+  @retval EFI_NOT_FOUND         The required object information is not found.
+**/
+STATIC
+EFI_STATUS
+EFIAPI
+HandleCmObjectRefByToken (
+  IN  CONST EDKII_CONFIGURATION_MANAGER_PROTOCOL  * CONST This,
+  IN  CONST CM_OBJECT_ID                                  CmObjectId,
+  IN        VOID                                  *       Object,
+  IN  CONST UINTN                                         ObjectSize,
+  IN  CONST UINTN                                         ObjectCount,
+  IN  CONST CM_OBJECT_TOKEN                               Token,
+  IN  CONST CM_OBJECT_HANDLER_PROC                        HandlerProc,
+  IN  OUT   CM_OBJ_DESCRIPTOR                     * CONST CmObjectDesc
+  )
+{
+  EFI_STATUS  Status;
+  CmObjectDesc->ObjectId = CmObjectId;
+  if (Token == CM_NULL_TOKEN) {
+    CmObjectDesc->Size = ObjectSize;
+    CmObjectDesc->Data = (VOID*)Object;
+    CmObjectDesc->Count = ObjectCount;
+    Status = EFI_SUCCESS;
+  } else {
+    Status = HandlerProc (This, CmObjectId, Token, CmObjectDesc);
+  }
+
+  DEBUG ((
+    DEBUG_INFO,
+    "INFO: Token = 0x%p, CmObjectId = %x, Ptr = 0x%p, Size = %d, Count = %d\n",
+    (VOID*)Token,
+    CmObjectId,
+    CmObjectDesc->Data,
+    CmObjectDesc->Size,
+    CmObjectDesc->Count
+    ));
+  return Status;
+}
+
+/** Return an ITS identifier array.
+
+  @param [in]  This        Pointer to the Configuration Manager Protocol.
+  @param [in]  CmObjectId  The Configuration Manager Object ID.
+  @param [in]  Token       A token for identifying the object
+  @param [out] CmObject    Pointer to the Configuration Manager Object
+                           descriptor describing the requested Object.
+
+  @retval EFI_SUCCESS           Success.
+  @retval EFI_INVALID_PARAMETER A parameter is invalid.
+  @retval EFI_NOT_FOUND         The required object information is not found.
+**/
+EFI_STATUS
+EFIAPI
+GetItsIdentifierArray (
+  IN  CONST EDKII_CONFIGURATION_MANAGER_PROTOCOL  * CONST This,
+  IN  CONST CM_OBJECT_ID                                  CmObjectId,
+  IN  CONST CM_OBJECT_TOKEN                               Token,
+  OUT       CM_OBJ_DESCRIPTOR                     * CONST CmObject
+  )
+{
+  EDKII_PLATFORM_REPOSITORY_INFO  * PlatformRepo;
+
+  if ((This == NULL) || (CmObject == NULL)) {
+    ASSERT (0);
+    return EFI_INVALID_PARAMETER;
+  }
+
+  PlatformRepo = This->PlatRepoInfo;
+
+  if (Token != (CM_OBJECT_TOKEN)&PlatformRepo->ItsIdentifierArray) {
+    return EFI_NOT_FOUND;
+  }
+
+  CmObject->ObjectId = CmObjectId;
+  CmObject->Size = sizeof (PlatformRepo->ItsIdentifierArray);
+  CmObject->Data = (VOID*)&PlatformRepo->ItsIdentifierArray;
+  CmObject->Count = ARRAY_SIZE (PlatformRepo->ItsIdentifierArray);
+  return EFI_SUCCESS;
+}
+
+/** Return a device Id mapping array.
+
+  @param [in]  This        Pointer to the Configuration Manager Protocol.
+  @param [in]  CmObjectId  The Configuration Manager Object ID.
+  @param [in]  Token       A token for identifying the object
+  @param [out] CmObject    Pointer to the Configuration Manager Object
+                           descriptor describing the requested Object.
+
+  @retval EFI_SUCCESS           Success.
+  @retval EFI_INVALID_PARAMETER A parameter is invalid.
+  @retval EFI_NOT_FOUND         The required object information is not found.
+**/
+EFI_STATUS
+EFIAPI
+GetDeviceIdMappingArray (
+  IN  CONST EDKII_CONFIGURATION_MANAGER_PROTOCOL  * CONST This,
+  IN  CONST CM_OBJECT_ID                                  CmObjectId,
+  IN  CONST CM_OBJECT_TOKEN                               Token,
+  OUT       CM_OBJ_DESCRIPTOR                     * CONST CmObject
+  )
+{
+  EDKII_PLATFORM_REPOSITORY_INFO  * PlatformRepo;
+
+  if ((This == NULL) || (CmObject == NULL)) {
+    ASSERT (0);
+    return EFI_INVALID_PARAMETER;
+  }
+
+  PlatformRepo = This->PlatRepoInfo;
+
+  if (Token != (CM_OBJECT_TOKEN)&PlatformRepo->DeviceIdMapping[0]) {
+    return EFI_NOT_FOUND;
+  }
+
+  CmObject->ObjectId = CmObjectId;
+  CmObject->Size = sizeof (CM_ARM_ID_MAPPING);
+  CmObject->Data = (VOID*)Token;
+  CmObject->Count = 1;
+  return EFI_SUCCESS;
+}
+
+/** Function pointer called by the parser to add information.
+
+  Callback function that the parser can use to add new
+  CmObj. This function must copy the CmObj data and not rely on
+  the parser preserving the CmObj memory.
+  This function is responsible of the Token allocation.
+
+  @param  [in]  ParserHandle  A handle to the parser instance.
+  @param  [in]  Context       A pointer to the caller's context provided in
+                              HwInfoParserInit ().
+  @param  [in]  CmObjDesc     CM_OBJ_DESCRIPTOR containing the CmObj(s) to add.
+  @param  [out] Token         If provided and success, contain the token
+                              generated for the CmObj.
+
+  @retval EFI_SUCCESS             The function completed successfully.
+  @retval EFI_INVALID_PARAMETER   Invalid parameter.
+**/
+STATIC
+EFI_STATUS
+EFIAPI
+HwInfoAdd (
+  IN        HW_INFO_PARSER_HANDLE   ParserHandle,
+  IN        VOID                  * Context,
+  IN  CONST CM_OBJ_DESCRIPTOR     * CmObjDesc,
+  OUT       CM_OBJECT_TOKEN       * Token OPTIONAL
+  )
+{
+  EFI_STATUS                        Status;
+  EDKII_PLATFORM_REPOSITORY_INFO  * PlatformRepo;
+
+  if ((ParserHandle == NULL)  ||
+      (Context == NULL)       ||
+      (CmObjDesc == NULL)) {
+    ASSERT (0);
+    return EFI_INVALID_PARAMETER;
+  }
+
+  PlatformRepo = (EDKII_PLATFORM_REPOSITORY_INFO*)Context;
+
+#ifndef MDEPKG_NDEBUG
+  // Print the received objects.
+  ParseCmObjDesc (CmObjDesc);
+#endif
+
+  Status = DynPlatRepoAddObject (
+             PlatformRepo->DynamicPlatformRepo,
+             CmObjDesc,
+             Token
+             );
+  if (EFI_ERROR (Status)) {
+    ASSERT (0);
+  }
+  return Status;
+}
+
+/** Cleanup the platform configuration repository.
+
+  @param [in]  This        Pointer to the Configuration Manager Protocol.
+
+  @retval EFI_SUCCESS             Success
+  @retval EFI_INVALID_PARAMETER   A parameter is invalid.
+**/
+STATIC
+EFI_STATUS
+EFIAPI
+CleanupPlatformRepository (
+  IN  CONST EDKII_CONFIGURATION_MANAGER_PROTOCOL  * CONST This
+  )
+{
+  EFI_STATUS                        Status;
+  EDKII_PLATFORM_REPOSITORY_INFO  * PlatformRepo;
+
+  if (This == NULL) {
+    ASSERT (0);
+    return EFI_INVALID_PARAMETER;
+  }
+
+  PlatformRepo = This->PlatRepoInfo;
+
+  // Shutdown the dynamic repo and free all objects.
+  Status = DynamicPlatRepoShutdown (PlatformRepo->DynamicPlatformRepo);
+  if (EFI_ERROR (Status)) {
+    ASSERT (0);
+    return Status;
+  }
+
+  // Shutdown parser.
+  Status = HwInfoParserShutdown (PlatformRepo->FdtParserHandle);
+  if (EFI_ERROR (Status)) {
+    ASSERT (0);
+  }
+  return Status;
+}
+
+/** Initialize the platform configuration repository.
+
+  @param [in]  This        Pointer to the Configuration Manager Protocol.
+
+  @retval EFI_SUCCESS             Success
+  @retval EFI_INVALID_PARAMETER   A parameter is invalid.
+  @retval EFI_OUT_OF_RESOURCES    An allocation has failed.
+**/
+STATIC
+EFI_STATUS
+EFIAPI
+InitializePlatformRepository (
+  IN  CONST EDKII_CONFIGURATION_MANAGER_PROTOCOL  * CONST This
+  )
+{
+  EFI_STATUS                        Status;
+  EDKII_PLATFORM_REPOSITORY_INFO  * PlatformRepo;
+  VOID                            * Hob;
+
+  if (This == NULL) {
+    ASSERT (0);
+    return EFI_INVALID_PARAMETER;
+  }
+
+  Hob = GetFirstGuidHob (&gFdtHobGuid);
+  if (Hob == NULL || GET_GUID_HOB_DATA_SIZE (Hob) != sizeof (UINT64)) {
+    ASSERT (0);
+    return EFI_NOT_FOUND;
+  }
+
+  PlatformRepo = This->PlatRepoInfo;
+  PlatformRepo->FdtBase = (VOID *)*(UINTN*)GET_GUID_HOB_DATA (Hob);
+
+  // Initialise the dynamic platform repository.
+  Status = DynamicPlatRepoInit (&PlatformRepo->DynamicPlatformRepo);
+  if (EFI_ERROR (Status)) {
+    ASSERT (0);
+    return Status;
+  }
+
+  // Initialise the FDT parser
+  Status = HwInfoParserInit (
+             PlatformRepo->FdtBase,
+             PlatformRepo,
+             HwInfoAdd,
+             &PlatformRepo->FdtParserHandle
+             );
+  if (EFI_ERROR (Status)) {
+    ASSERT (0);
+    goto error_handler;
+  }
+
+  Status = HwInfoParse (PlatformRepo->FdtParserHandle);
+  if (EFI_ERROR (Status)) {
+    ASSERT (0);
+    goto error_handler;
+  }
+
+  Status = DynamicPlatRepoFinalise (PlatformRepo->DynamicPlatformRepo);
+  if (EFI_ERROR (Status)) {
+    ASSERT (0);
+    goto error_handler;
+  }
+
+  return EFI_SUCCESS;
+
+error_handler:
+  CleanupPlatformRepository (This);
+  return Status;
+}
+
+/** Return a standard namespace object.
+
+  @param [in]      This        Pointer to the Configuration Manager Protocol.
+  @param [in]      CmObjectId  The Configuration Manager Object ID.
+  @param [in]      Token       An optional token identifying the object. If
+                               unused this must be CM_NULL_TOKEN.
+  @param [in, out] CmObject    Pointer to the Configuration Manager Object
+                               descriptor describing the requested Object.
+
+  @retval EFI_SUCCESS           Success.
+  @retval EFI_INVALID_PARAMETER A parameter is invalid.
+  @retval EFI_NOT_FOUND         The required object information is not found.
+**/
+EFI_STATUS
+EFIAPI
+GetStandardNameSpaceObject (
+  IN  CONST EDKII_CONFIGURATION_MANAGER_PROTOCOL  * CONST This,
+  IN  CONST CM_OBJECT_ID                                  CmObjectId,
+  IN  CONST CM_OBJECT_TOKEN                               Token OPTIONAL,
+  IN  OUT   CM_OBJ_DESCRIPTOR                     * CONST CmObject
+  )
+{
+  EFI_STATUS                        Status;
+  EDKII_PLATFORM_REPOSITORY_INFO  * PlatformRepo;
+  UINTN                             AcpiTableCount;
+  CM_OBJ_DESCRIPTOR                 CmObjDesc;
+
+  if ((This == NULL) || (CmObject == NULL)) {
+    ASSERT (This != NULL);
+    ASSERT (CmObject != NULL);
+    return EFI_INVALID_PARAMETER;
+  }
+
+  Status = EFI_NOT_FOUND;
+  PlatformRepo = This->PlatRepoInfo;
+
+  switch (GET_CM_OBJECT_ID (CmObjectId)) {
+    case EStdObjCfgMgrInfo:
+      Status = HandleCmObject (
+                 CmObjectId,
+                 &PlatformRepo->CmInfo,
+                 sizeof (PlatformRepo->CmInfo),
+                 1,
+                 CmObject
+                 );
+      break;
+
+    case EStdObjAcpiTableList:
+      AcpiTableCount = ARRAY_SIZE (PlatformRepo->CmAcpiTableList);
+
+      // Get Pci config space information.
+      Status = DynamicPlatRepoGetObject (
+                 PlatformRepo->DynamicPlatformRepo,
+                 CREATE_CM_ARM_OBJECT_ID (EArmObjPciConfigSpaceInfo),
+                 CM_NULL_TOKEN,
+                 &CmObjDesc
+                 );
+      if (Status == EFI_NOT_FOUND) {
+        // The last 3 tables are for PCIe. If PCIe information is not
+        // present, Kvmtool was launched without the PCIe option.
+        // Therefore, reduce the table count by 3.
+        AcpiTableCount -= 3;
+      } else if (EFI_ERROR (Status)) {
+        ASSERT (0);
+        return Status;
+      }
+
+      // Get the Gic version.
+      Status = DynamicPlatRepoGetObject (
+                 PlatformRepo->DynamicPlatformRepo,
+                 CREATE_CM_ARM_OBJECT_ID (EArmObjGicDInfo),
+                 CM_NULL_TOKEN,
+                 &CmObjDesc
+                 );
+      if (EFI_ERROR (Status)) {
+        ASSERT (0);
+        return Status;
+      }
+      if (((CM_ARM_GICD_INFO*)CmObjDesc.Data)->GicVersion < 3) {
+        // IORT is only required for GicV3/4
+        AcpiTableCount -= 1;
+      }
+
+      Status = HandleCmObject (
+                 CmObjectId,
+                 PlatformRepo->CmAcpiTableList,
+                 (sizeof (PlatformRepo->CmAcpiTableList[0]) * AcpiTableCount),
+                 AcpiTableCount,
+                 CmObject
+                 );
+      break;
+
+    default: {
+      Status = EFI_NOT_FOUND;
+      DEBUG ((
+        DEBUG_ERROR,
+        "ERROR: Object 0x%x. Status = %r\n",
+        CmObjectId,
+        Status
+        ));
+      break;
+    }
+  }
+
+  return Status;
+}
+
+/** Return an ARM namespace object.
+
+  @param [in]      This        Pointer to the Configuration Manager Protocol.
+  @param [in]      CmObjectId  The Configuration Manager Object ID.
+  @param [in]      Token       An optional token identifying the object. If
+                               unused this must be CM_NULL_TOKEN.
+  @param [in, out] CmObject    Pointer to the Configuration Manager Object
+                               descriptor describing the requested Object.
+
+  @retval EFI_SUCCESS           Success.
+  @retval EFI_INVALID_PARAMETER A parameter is invalid.
+  @retval EFI_NOT_FOUND         The required object information is not found.
+**/
+EFI_STATUS
+EFIAPI
+GetArmNameSpaceObject (
+  IN  CONST EDKII_CONFIGURATION_MANAGER_PROTOCOL  * CONST This,
+  IN  CONST CM_OBJECT_ID                                  CmObjectId,
+  IN  CONST CM_OBJECT_TOKEN                               Token OPTIONAL,
+  IN  OUT   CM_OBJ_DESCRIPTOR                     * CONST CmObject
+  )
+{
+  EFI_STATUS                        Status;
+  EDKII_PLATFORM_REPOSITORY_INFO  * PlatformRepo;
+
+  if ((This == NULL) || (CmObject == NULL)) {
+    ASSERT (This != NULL);
+    ASSERT (CmObject != NULL);
+    return EFI_INVALID_PARAMETER;
+  }
+
+  Status = EFI_NOT_FOUND;
+  PlatformRepo = This->PlatRepoInfo;
+
+  // First check among the static objects.
+  switch (GET_CM_OBJECT_ID (CmObjectId)) {
+    case EArmObjPowerManagementProfileInfo:
+      Status = HandleCmObject (
+                 CmObjectId,
+                 &PlatformRepo->PmProfileInfo,
+                 sizeof (PlatformRepo->PmProfileInfo),
+                 1,
+                 CmObject
+                 );
+      break;
+
+    case EArmObjItsGroup:
+      Status = HandleCmObject (
+                 CmObjectId,
+                 &PlatformRepo->ItsGroupInfo,
+                 sizeof (PlatformRepo->ItsGroupInfo),
+                 1,
+                 CmObject
+                 );
+      break;
+
+    case EArmObjGicItsIdentifierArray:
+      Status = HandleCmObjectRefByToken (
+                 This,
+                 CmObjectId,
+                 PlatformRepo->ItsIdentifierArray,
+                 sizeof (PlatformRepo->ItsIdentifierArray),
+                 ARRAY_SIZE (PlatformRepo->ItsIdentifierArray),
+                 Token,
+                 GetItsIdentifierArray,
+                 CmObject
+                 );
+      break;
+
+    case EArmObjRootComplex:
+      Status = HandleCmObject (
+                 CmObjectId,
+                 &PlatformRepo->RootComplexInfo,
+                 sizeof (PlatformRepo->RootComplexInfo),
+                 1,
+                 CmObject
+                 );
+      break;
+
+    case EArmObjIdMappingArray:
+      Status = HandleCmObjectRefByToken (
+                 This,
+                 CmObjectId,
+                 PlatformRepo->DeviceIdMapping,
+                 sizeof (PlatformRepo->DeviceIdMapping),
+                 ARRAY_SIZE (PlatformRepo->DeviceIdMapping),
+                 Token,
+                 GetDeviceIdMappingArray,
+                 CmObject
+                 );
+      break;
+
+    default:
+      // No match found among the static objects.
+      // Check the dynamic objects.
+      Status = DynamicPlatRepoGetObject (
+                 PlatformRepo->DynamicPlatformRepo,
+                 CmObjectId,
+                 Token,
+                 CmObject
+                 );
+      break;
+  } // switch
+
+  if (Status == EFI_NOT_FOUND) {
+    DEBUG ((
+      DEBUG_INFO,
+      "INFO: Object 0x%x. Status = %r\n",
+      CmObjectId,
+      Status
+      ));
+  } else {
+    ASSERT_EFI_ERROR (Status);
+  }
+
+  return Status;
+}
+
+/** Return an OEM namespace object.
+
+  @param [in]      This        Pointer to the Configuration Manager Protocol.
+  @param [in]      CmObjectId  The Configuration Manager Object ID.
+  @param [in]      Token       An optional token identifying the object. If
+                               unused this must be CM_NULL_TOKEN.
+  @param [in, out] CmObject    Pointer to the Configuration Manager Object
+                               descriptor describing the requested Object.
+
+  @retval EFI_SUCCESS           Success.
+  @retval EFI_INVALID_PARAMETER A parameter is invalid.
+  @retval EFI_NOT_FOUND         The required object information is not found.
+**/
+EFI_STATUS
+EFIAPI
+GetOemNameSpaceObject (
+  IN  CONST EDKII_CONFIGURATION_MANAGER_PROTOCOL  * CONST This,
+  IN  CONST CM_OBJECT_ID                                  CmObjectId,
+  IN  CONST CM_OBJECT_TOKEN                               Token OPTIONAL,
+  IN  OUT   CM_OBJ_DESCRIPTOR                     * CONST CmObject
+  )
+{
+  EFI_STATUS  Status;
+
+  Status = EFI_SUCCESS;
+  if ((This == NULL) || (CmObject == NULL)) {
+    ASSERT (This != NULL);
+    ASSERT (CmObject != NULL);
+    return EFI_INVALID_PARAMETER;
+  }
+
+  switch (GET_CM_OBJECT_ID (CmObjectId)) {
+    default: {
+      Status = EFI_NOT_FOUND;
+      DEBUG ((
+        DEBUG_ERROR,
+        "ERROR: Object 0x%x. Status = %r\n",
+        CmObjectId,
+        Status
+        ));
+      break;
+    }
+  }
+
+  return Status;
+}
+
+/** The GetObject function defines the interface implemented by the
+    Configuration Manager Protocol for returning the Configuration
+    Manager Objects.
+
+  @param [in]      This        Pointer to the Configuration Manager Protocol.
+  @param [in]      CmObjectId  The Configuration Manager Object ID.
+  @param [in]      Token       An optional token identifying the object. If
+                               unused this must be CM_NULL_TOKEN.
+  @param [in, out] CmObject    Pointer to the Configuration Manager Object
+                               descriptor describing the requested Object.
+
+  @retval EFI_SUCCESS           Success.
+  @retval EFI_INVALID_PARAMETER A parameter is invalid.
+  @retval EFI_NOT_FOUND         The required object information is not found.
+**/
+EFI_STATUS
+EFIAPI
+ArmKvmtoolPlatformGetObject (
+  IN  CONST EDKII_CONFIGURATION_MANAGER_PROTOCOL  * CONST This,
+  IN  CONST CM_OBJECT_ID                                  CmObjectId,
+  IN  CONST CM_OBJECT_TOKEN                               Token OPTIONAL,
+  IN  OUT   CM_OBJ_DESCRIPTOR                     * CONST CmObject
+  )
+{
+  EFI_STATUS  Status;
+
+  if ((This == NULL) || (CmObject == NULL)) {
+    ASSERT (This != NULL);
+    ASSERT (CmObject != NULL);
+    return EFI_INVALID_PARAMETER;
+  }
+
+  switch (GET_CM_NAMESPACE_ID (CmObjectId)) {
+    case EObjNameSpaceStandard:
+      Status = GetStandardNameSpaceObject (This, CmObjectId, Token, CmObject);
+      break;
+    case EObjNameSpaceArm:
+      Status = GetArmNameSpaceObject (This, CmObjectId, Token, CmObject);
+      break;
+    case EObjNameSpaceOem:
+      Status = GetOemNameSpaceObject (This, CmObjectId, Token, CmObject);
+      break;
+    default: {
+      Status = EFI_INVALID_PARAMETER;
+      DEBUG ((
+        DEBUG_ERROR,
+        "ERROR: Unknown Namespace Object = 0x%x. Status = %r\n",
+        CmObjectId,
+        Status
+        ));
+      break;
+    }
+  }
+
+  return Status;
+}
+
+/** The SetObject function defines the interface implemented by the
+    Configuration Manager Protocol for updating the Configuration
+    Manager Objects.
+
+  @param [in]      This        Pointer to the Configuration Manager Protocol.
+  @param [in]      CmObjectId  The Configuration Manager Object ID.
+  @param [in]      Token       An optional token identifying the object. If
+                               unused this must be CM_NULL_TOKEN.
+  @param [in]      CmObject    Pointer to the Configuration Manager Object
+                               descriptor describing the Object.
+
+  @retval EFI_UNSUPPORTED  This operation is not supported.
+**/
+EFI_STATUS
+EFIAPI
+ArmKvmtoolPlatformSetObject (
+  IN  CONST EDKII_CONFIGURATION_MANAGER_PROTOCOL  * CONST This,
+  IN  CONST CM_OBJECT_ID                                  CmObjectId,
+  IN  CONST CM_OBJECT_TOKEN                               Token OPTIONAL,
+  IN        CM_OBJ_DESCRIPTOR                     * CONST CmObject
+  )
+{
+  return EFI_UNSUPPORTED;
+}
+
+/** A structure describing the configuration manager protocol interface.
+*/
+STATIC
+CONST
+EDKII_CONFIGURATION_MANAGER_PROTOCOL KvmtoolPlatformConfigManagerProtocol = {
+  CREATE_REVISION(1,0),
+  ArmKvmtoolPlatformGetObject,
+  ArmKvmtoolPlatformSetObject,
+  &KvmtoolPlatRepositoryInfo
+};
+
+/**
+  Entrypoint of Configuration Manager Dxe.
+
+  @param  ImageHandle
+  @param  SystemTable
+
+  @return EFI_SUCCESS
+  @return EFI_LOAD_ERROR
+  @return EFI_OUT_OF_RESOURCES
+**/
+EFI_STATUS
+EFIAPI
+ConfigurationManagerDxeInitialize (
+  IN EFI_HANDLE          ImageHandle,
+  IN EFI_SYSTEM_TABLE  * SystemTable
+  )
+{
+  EFI_STATUS  Status;
+
+  if (PcdGetBool (PcdForceNoAcpi)) {
+    // Use DT and not ACPI.
+    return EFI_SUCCESS;
+  }
+
+  Status = gBS->InstallProtocolInterface (
+                  &ImageHandle,
+                  &gEdkiiConfigurationManagerProtocolGuid,
+                  EFI_NATIVE_INTERFACE,
+                  (VOID*)&KvmtoolPlatformConfigManagerProtocol
+                  );
+  if (EFI_ERROR (Status)) {
+    DEBUG ((
+      DEBUG_ERROR,
+      "ERROR: Failed to get Install Configuration Manager Protocol." \
+      " Status = %r\n",
+      Status
+      ));
+    goto error_handler;
+  }
+
+  Status = InitializePlatformRepository (
+             &KvmtoolPlatformConfigManagerProtocol
+             );
+  if (EFI_ERROR (Status)) {
+    DEBUG ((
+      DEBUG_ERROR,
+      "ERROR: Failed to initialize the Platform Configuration Repository." \
+      " Status = %r\n",
+      Status
+      ));
+  }
+
+error_handler:
+  return Status;
+}
+
+/**
+  Unload function for this image.
+
+  @param ImageHandle   Handle for the image of this driver.
+
+  @retval EFI_SUCCESS  Driver unloaded successfully.
+  @return other        Driver can not unloaded.
+**/
+EFI_STATUS
+EFIAPI
+ConfigurationManagerDxeUnloadImage (
+  IN EFI_HANDLE         ImageHandle
+  )
+{
+  return CleanupPlatformRepository (&KvmtoolPlatformConfigManagerProtocol);
+}
diff --git a/ArmVirtPkg/KvmtoolCfgMgrDxe/ConfigurationManager.h b/ArmVirtPkg/KvmtoolCfgMgrDxe/ConfigurationManager.h
new file mode 100644
index 000000000000..94cfca3b7671
--- /dev/null
+++ b/ArmVirtPkg/KvmtoolCfgMgrDxe/ConfigurationManager.h
@@ -0,0 +1,94 @@
+/** @file
+
+  Copyright (c) 2021, Arm Limited. All rights reserved.<BR>
+
+  SPDX-License-Identifier: BSD-2-Clause-Patent
+
+  @par Glossary:
+    - Cm or CM   - Configuration Manager
+    - Obj or OBJ - Object
+**/
+
+#ifndef CONFIGURATION_MANAGER_H_
+#define CONFIGURATION_MANAGER_H_
+
+/** C array containing the compiled AML template.
+    This symbol is defined in the auto generated C file
+    containing the AML bytecode array.
+*/
+extern CHAR8  dsdt_aml_code[];
+
+/** The configuration manager version.
+*/
+#define CONFIGURATION_MANAGER_REVISION CREATE_REVISION (1, 0)
+
+/** The OEM ID
+*/
+#define CFG_MGR_OEM_ID    { 'A', 'R', 'M', 'L', 'T', 'D' }
+
+/** A function that prepares Configuration Manager Objects for returning.
+
+  @param [in]  This        Pointer to the Configuration Manager Protocol.
+  @param [in]  CmObjectId  The Configuration Manager Object ID.
+  @param [in]  Token       A token for identifying the object.
+  @param [out] CmObject    Pointer to the Configuration Manager Object
+                           descriptor describing the requested Object.
+
+  @retval EFI_SUCCESS           Success.
+  @retval EFI_INVALID_PARAMETER A parameter is invalid.
+  @retval EFI_NOT_FOUND         The required object information is not found.
+**/
+typedef EFI_STATUS (*CM_OBJECT_HANDLER_PROC) (
+  IN  CONST EDKII_CONFIGURATION_MANAGER_PROTOCOL  * CONST This,
+  IN  CONST CM_OBJECT_ID                                  CmObjectId,
+  IN  CONST CM_OBJECT_TOKEN                               Token,
+  IN  OUT   CM_OBJ_DESCRIPTOR                     * CONST CmObject
+  );
+
+/** A helper macro for mapping a reference token.
+*/
+#define REFERENCE_TOKEN(Field)                                   \
+          (CM_OBJECT_TOKEN)((UINT8*)&KvmtoolPlatRepositoryInfo + \
+           OFFSET_OF (EDKII_PLATFORM_REPOSITORY_INFO, Field))
+
+/** The number of ACPI tables to install
+*/
+#define PLAT_ACPI_TABLE_COUNT       10
+
+/** A structure describing the platform configuration
+    manager repository information
+*/
+typedef struct PlatformRepositoryInfo {
+  /// Configuration Manager Information.
+  CM_STD_OBJ_CONFIGURATION_MANAGER_INFO CmInfo;
+
+  /// List of ACPI tables
+  CM_STD_OBJ_ACPI_TABLE_INFO            CmAcpiTableList[PLAT_ACPI_TABLE_COUNT];
+
+  /// Power management profile information
+  CM_ARM_POWER_MANAGEMENT_PROFILE_INFO  PmProfileInfo;
+
+  /// ITS Group node
+  CM_ARM_ITS_GROUP_NODE                 ItsGroupInfo;
+
+  /// ITS Identifier array
+  CM_ARM_ITS_IDENTIFIER                 ItsIdentifierArray[1];
+
+  /// PCI Root complex node
+  CM_ARM_ROOT_COMPLEX_NODE              RootComplexInfo;
+
+  /// Array of DeviceID mapping
+  CM_ARM_ID_MAPPING                     DeviceIdMapping[1];
+
+  /// Dynamic platform repository.
+  /// CmObj created by parsing the Kvmtool device tree are stored here.
+  DYNAMIC_PLATFORM_REPOSITORY_INFO    * DynamicPlatformRepo;
+
+  /// Base address of the FDT.
+  VOID                                * FdtBase;
+
+  /// A handle to the FDT HwInfoParser.
+  HW_INFO_PARSER_HANDLE                 FdtParserHandle;
+} EDKII_PLATFORM_REPOSITORY_INFO;
+
+#endif // CONFIGURATION_MANAGER_H_
diff --git a/ArmVirtPkg/KvmtoolCfgMgrDxe/ConfigurationManagerDxe.inf b/ArmVirtPkg/KvmtoolCfgMgrDxe/ConfigurationManagerDxe.inf
new file mode 100644
index 000000000000..9f0bf72fce2d
--- /dev/null
+++ b/ArmVirtPkg/KvmtoolCfgMgrDxe/ConfigurationManagerDxe.inf
@@ -0,0 +1,58 @@
+## @file
+#  Configuration Manager Dxe
+#
+#  Copyright (c) 2021, Arm Limited. All rights reserved.<BR>
+#
+#  SPDX-License-Identifier: BSD-2-Clause-Patent
+##
+
+[Defines]
+  INF_VERSION                    = 0x0001001B
+  BASE_NAME                      = ConfigurationManagerDxe
+  FILE_GUID                      = 3C80D366-510C-4154-BB3A-E12439AD337C
+  MODULE_TYPE                    = DXE_DRIVER
+  VERSION_STRING                 = 1.0
+  ENTRY_POINT                    = ConfigurationManagerDxeInitialize
+  UNLOAD_IMAGE                   = ConfigurationManagerDxeUnloadImage
+
+#
+# The following information is for reference only and not required by the build tools.
+#
+#  VALID_ARCHITECTURES           = ARM AARCH64
+#
+
+[Sources]
+  AslTables/Dsdt.asl
+  ConfigurationManager.c
+  ConfigurationManager.h
+  ConfigurationManagerDxe.inf
+
+[Packages]
+  ArmVirtPkg/ArmVirtPkg.dec
+  DynamicTablesPkg/DynamicTablesPkg.dec
+  EmbeddedPkg/EmbeddedPkg.dec
+  MdePkg/MdePkg.dec
+
+[LibraryClasses]
+  DynamicPlatRepoLib
+  HobLib
+  HwInfoParserLib
+  PrintLib
+  TableHelperLib
+  UefiBootServicesTableLib
+  UefiDriverEntryPoint
+  UefiRuntimeServicesTableLib
+
+[Protocols]
+  gEdkiiConfigurationManagerProtocolGuid
+
+[Guids]
+  gFdtHobGuid
+
+[FixedPcd]
+
+[Pcd]
+  gArmVirtTokenSpaceGuid.PcdForceNoAcpi
+
+[Depex]
+  TRUE
-- 
2.17.1

Re: [edk2-devel] [PATCH v1 3/5] ArmVirtPkg: Add Configuration Manager for Kvmtool firmware

[Laszlo Ersek]

On 06/23/21 16:06, PierreGondois wrote:
> From: Sami Mujawar <sami.mujawar@arm.com>
> 
> Add Configuration Manager to enable ACPI tables for Kvmtool
> firmware. The Configuration Manager for Kvmtool uses the DT
> Hardware Information Parser module (FdtHwInfoParser) to parse
> the DT provided by Kvmtool. The FdtHwInfoParser parses the DT
> and invokes the callback function HW_INFO_ADD_OBJECT to add
> the Configuration Manager objects to the Platform Information
> repository.
> 
> The information for some Configuration Manager objects may not
> be available in the DT. Such objects are initialised locally
> by the Configuration Manager.
> 
> Support for the following ACPI tables is provided:
>  - DBG2
>  - DSDT (Empty stub)
>  - FADT
>  - GTDT
>  - MADT
>  - SPCR
>  - SSDT (Cpu Hierarchy)
>  - SSDT (Pcie bus)
> 
> Signed-off-by: Sami Mujawar <sami.mujawar@arm.com>
> Signed-off-by: Pierre Gondois <Pierre.Gondois@arm.com>
> ---
>  ArmVirtPkg/ArmVirtKvmTool.dsc                 |   3 +
>  .../KvmtoolCfgMgrDxe/ConfigurationManager.c   | 948 ++++++++++++++++++
>  .../KvmtoolCfgMgrDxe/ConfigurationManager.h   |  94 ++
>  .../ConfigurationManagerDxe.inf               |  58 ++
>  4 files changed, 1103 insertions(+)
>  create mode 100644 ArmVirtPkg/KvmtoolCfgMgrDxe/ConfigurationManager.c
>  create mode 100644 ArmVirtPkg/KvmtoolCfgMgrDxe/ConfigurationManager.h
>  create mode 100644 ArmVirtPkg/KvmtoolCfgMgrDxe/ConfigurationManagerDxe.inf
> 
> diff --git a/ArmVirtPkg/ArmVirtKvmTool.dsc b/ArmVirtPkg/ArmVirtKvmTool.dsc
> index 3bd1cc72a1eb..920880796ac2 100644
> --- a/ArmVirtPkg/ArmVirtKvmTool.dsc
> +++ b/ArmVirtPkg/ArmVirtKvmTool.dsc
> @@ -71,6 +71,9 @@ [LibraryClasses.common]
>    PlatformHookLib|ArmVirtPkg/Library/Fdt16550SerialPortHookLib/Fdt16550SerialPortHookLib.inf
>    SerialPortLib|MdeModulePkg/Library/BaseSerialPortLib16550/BaseSerialPortLib16550.inf
>  
> +  HwInfoParserLib|DynamicTablesPkg/Library/FdtHwInfoParserLib/FdtHwInfoParserLib.inf
> +  DynamicPlatRepoLib|DynamicTablesPkg/Library/Common/DynamicPlatRepoLib/DynamicPlatRepoLib.inf
> +
>  [LibraryClasses.common.SEC, LibraryClasses.common.PEI_CORE, LibraryClasses.common.PEIM]
>    PciExpressLib|MdePkg/Library/BasePciExpressLib/BasePciExpressLib.inf
>    PlatformHookLib|ArmVirtPkg/Library/Fdt16550SerialPortHookLib/EarlyFdt16550SerialPortHookLib.inf

(1) Please move at least the line

  ArmVirtPkg/KvmtoolCfgMgrDxe/ConfigurationManagerDxe.inf

from the next patch into this patch.

The reason is that without that line in the DSC file, you can't build
the new driver, right after this patch is applied, even with the "-m"
option of the "build" utility. And if you cannot build the driver, then
adding the library class resolutions *here* is useless.

... Alternatively, please move the lib class resolutions to the next
patch (and don't touch the DSC file in this patch).


> diff --git a/ArmVirtPkg/KvmtoolCfgMgrDxe/ConfigurationManager.c b/ArmVirtPkg/KvmtoolCfgMgrDxe/ConfigurationManager.c
> new file mode 100644
> index 000000000000..07b8b403dd4a
> --- /dev/null
> +++ b/ArmVirtPkg/KvmtoolCfgMgrDxe/ConfigurationManager.c
> @@ -0,0 +1,948 @@
> +/** @file
> +  Configuration Manager Dxe
> +
> +  Copyright (c) 2021, Arm Limited. All rights reserved.<BR>
> +
> +  SPDX-License-Identifier: BSD-2-Clause-Patent
> +
> +  @par Glossary:
> +    - Cm or CM   - Configuration Manager
> +    - Obj or OBJ - Object
> +**/
> +
> +#include <IndustryStandard/DebugPort2Table.h>
> +#include <IndustryStandard/IoRemappingTable.h>
> +#include <IndustryStandard/MemoryMappedConfigurationSpaceAccessTable.h>
> +#include <IndustryStandard/SerialPortConsoleRedirectionTable.h>
> +#include <Library/BaseMemoryLib.h>
> +#include <Library/DebugLib.h>
> +#include <Library/DynamicPlatRepoLib.h>
> +#include <Library/HobLib.h>
> +#include <Library/HwInfoParserLib.h>
> +#include <Library/IoLib.h>
> +#include <Library/PcdLib.h>
> +#include <Library/TableHelperLib.h>
> +#include <Library/UefiBootServicesTableLib.h>
> +#include <Protocol/AcpiTable.h>
> +#include <Protocol/ConfigurationManagerProtocol.h>
> +
> +#include "ConfigurationManager.h"
> +
> +/** The platform configuration repository information.
> +*/

(2) This comment style is only used on functions. For other declarations
/ definitions, please use

//
// blah
//

> +STATIC
> +EDKII_PLATFORM_REPOSITORY_INFO KvmtoolPlatRepositoryInfo = {
> +  /// Configuration Manager information

(3) "///" is sometimes used in edk2 indeed, but here it is inconsistent
with the other "//" usages

(4) please add leading and trailing empty comment lines (elsewhere too)

(5) all module global variables must start with a lower-case "m". Such
as "mKvmtoolPlatRepositoryInfo".

> +  { CONFIGURATION_MANAGER_REVISION, CFG_MGR_OEM_ID },
> +
> +  // ACPI Table List
> +  {
> +    // FADT Table
> +    {
> +      EFI_ACPI_6_3_FIXED_ACPI_DESCRIPTION_TABLE_SIGNATURE,
> +      EFI_ACPI_6_3_FIXED_ACPI_DESCRIPTION_TABLE_REVISION,
> +      CREATE_STD_ACPI_TABLE_GEN_ID (EStdAcpiTableIdFadt),
> +      NULL
> +    },
> +    // GTDT Table
> +    {
> +      EFI_ACPI_6_3_GENERIC_TIMER_DESCRIPTION_TABLE_SIGNATURE,
> +      EFI_ACPI_6_3_GENERIC_TIMER_DESCRIPTION_TABLE_REVISION,
> +      CREATE_STD_ACPI_TABLE_GEN_ID (EStdAcpiTableIdGtdt),
> +      NULL
> +    },
> +    // MADT Table
> +    {
> +      EFI_ACPI_6_3_MULTIPLE_APIC_DESCRIPTION_TABLE_SIGNATURE,
> +      EFI_ACPI_6_3_MULTIPLE_APIC_DESCRIPTION_TABLE_REVISION,
> +      CREATE_STD_ACPI_TABLE_GEN_ID (EStdAcpiTableIdMadt),
> +      NULL
> +    },
> +    // SPCR Table
> +    {
> +      EFI_ACPI_6_3_SERIAL_PORT_CONSOLE_REDIRECTION_TABLE_SIGNATURE,
> +      EFI_ACPI_SERIAL_PORT_CONSOLE_REDIRECTION_TABLE_REVISION,
> +      CREATE_STD_ACPI_TABLE_GEN_ID (EStdAcpiTableIdSpcr),
> +      NULL
> +    },
> +    // DSDT Table
> +    {
> +      EFI_ACPI_6_3_DIFFERENTIATED_SYSTEM_DESCRIPTION_TABLE_SIGNATURE,
> +      0, // Unused
> +      CREATE_STD_ACPI_TABLE_GEN_ID (EStdAcpiTableIdDsdt),
> +      (EFI_ACPI_DESCRIPTION_HEADER*)dsdt_aml_code
> +    },
> +    // SSDT Cpu Hierarchy Table
> +    {
> +      EFI_ACPI_6_3_SECONDARY_SYSTEM_DESCRIPTION_TABLE_SIGNATURE,
> +      0, // Unused
> +      CREATE_STD_ACPI_TABLE_GEN_ID (EStdAcpiTableIdSsdtCpuTopology),
> +      NULL
> +    },
> +    // DBG2 Table
> +    {
> +      EFI_ACPI_6_3_DEBUG_PORT_2_TABLE_SIGNATURE,
> +      EFI_ACPI_DBG2_DEBUG_DEVICE_INFORMATION_STRUCT_REVISION,
> +      CREATE_STD_ACPI_TABLE_GEN_ID (EStdAcpiTableIdDbg2),
> +      NULL
> +    },
> +    // PCI MCFG Table
> +    {
> +      EFI_ACPI_6_3_PCI_EXPRESS_MEMORY_MAPPED_CONFIGURATION_SPACE_BASE_ADDRESS_DESCRIPTION_TABLE_SIGNATURE,
> +      EFI_ACPI_MEMORY_MAPPED_CONFIGURATION_SPACE_ACCESS_TABLE_REVISION,
> +      CREATE_STD_ACPI_TABLE_GEN_ID (EStdAcpiTableIdMcfg),
> +      NULL
> +    },
> +    // SSDT table describing the PCI root complex
> +    {
> +      EFI_ACPI_6_3_SECONDARY_SYSTEM_DESCRIPTION_TABLE_SIGNATURE,
> +      0, // Unused
> +      CREATE_STD_ACPI_TABLE_GEN_ID (EStdAcpiTableIdSsdtPciExpress),
> +      NULL
> +    },
> +    // IORT Table
> +    {
> +      EFI_ACPI_6_3_IO_REMAPPING_TABLE_SIGNATURE,
> +      EFI_ACPI_IO_REMAPPING_TABLE_REVISION,
> +      CREATE_STD_ACPI_TABLE_GEN_ID (EStdAcpiTableIdIort),
> +      NULL
> +    },
> +  },
> +
> +  // Power management profile information
> +  { EFI_ACPI_6_3_PM_PROFILE_ENTERPRISE_SERVER },    // PowerManagement Profile
> +
> +  // ITS group node
> +  {
> +    // Reference token for this Iort node
> +    REFERENCE_TOKEN (ItsGroupInfo),
> +    // The number of ITS identifiers in the ITS node.
> +    1,
> +    // Reference token for the ITS identifier array
> +    REFERENCE_TOKEN (ItsIdentifierArray)
> +  },
> +  // ITS identifier array
> +  {
> +    {
> +      // The ITS Identifier
> +      0
> +    }
> +  },
> +
> +  // Root Complex node info
> +  {
> +    // Reference token for this Iort node
> +    REFERENCE_TOKEN (RootComplexInfo),
> +    // Number of ID mappings
> +    1,
> +    // Reference token for the ID mapping array
> +    REFERENCE_TOKEN (DeviceIdMapping[0]),
> +
> +    // Memory access properties : Cache coherent attributes
> +    EFI_ACPI_IORT_MEM_ACCESS_PROP_CCA,
> +    // Memory access properties : Allocation hints
> +    0,
> +    // Memory access properties : Memory access flags
> +    0,
> +    // ATS attributes
> +    EFI_ACPI_IORT_ROOT_COMPLEX_ATS_UNSUPPORTED,
> +    // PCI segment number
> +    0
> +  },
> +
> +  // Array of Device ID mappings
> +  {
> +    /* RootComplex -> ITS Group
> +    */
> +    // Device ID mapping for Root complex node
> +    {
> +      // Input base
> +      0x0,
> +      // Number of input IDs
> +      0x0000FFFF,
> +      // Output Base
> +      0x0,
> +      // Output reference
> +      REFERENCE_TOKEN (ItsGroupInfo),
> +      // Flags
> +      0
> +    },
> +  },
> +};
> +
> +/** A helper function for returning the Configuration Manager Objects.

(6) I think we tend to keep the "/**" comment starter alone on the line,
and the actual function description starts at the next line.

I suggest updating all the other comment blocks, too.

> +
> +  @param [in]       CmObjectId     The Configuration Manager Object ID.
> +  @param [in]       Object         Pointer to the Object(s).
> +  @param [in]       ObjectSize     Total size of the Object(s).
> +  @param [in]       ObjectCount    Number of Objects.
> +  @param [in, out]  CmObjectDesc   Pointer to the Configuration Manager Object
> +                                   descriptor describing the requested Object.
> +
> +  @retval EFI_SUCCESS           Success.
> +**/
> +STATIC
> +EFI_STATUS
> +EFIAPI
> +HandleCmObject (
> +  IN  CONST CM_OBJECT_ID                CmObjectId,
> +  IN        VOID                *       Object,
> +  IN  CONST UINTN                       ObjectSize,
> +  IN  CONST UINTN                       ObjectCount,
> +  IN  OUT   CM_OBJ_DESCRIPTOR   * CONST CmObjectDesc
> +  )
> +{
> +  CmObjectDesc->ObjectId = CmObjectId;
> +  CmObjectDesc->Size = ObjectSize;
> +  CmObjectDesc->Data = (VOID*)Object;

(7) This cast is useless. Please update the rest of the code too, if
necessary.

> +  CmObjectDesc->Count = ObjectCount;
> +  DEBUG ((
> +    DEBUG_INFO,
> +    "INFO: CmObjectId = %x, Ptr = 0x%p, Size = %d, Count = %d\n",
> +    CmObjectId,
> +    CmObjectDesc->Data,
> +    CmObjectDesc->Size,
> +    CmObjectDesc->Count
> +    ));

(8) Please audit the format specifications in all of your DEBUGs.

First, wherever you introduce the CM_OBJECT_ID typedef, please also
#define a new format string macro for printing objects of type
CM_OBJECT_ID. Then, in DEBUG format strings, please use that typedef,
rather than an open-coded %x. "%x" may be proper (I can't tell), but it
should be hidden behind a macro.

#define FMT_CM_OBJECT_ID "%x"

DEBUG ((DEBUG_INFO, "blah " FMT_CM_OBJECT_ID " blah\n", CmObjectId));


Second, *assuming* "CmObjectDesc->Size" has type UINTN, the right way to
print it is not with %d, but (a) casting the value to UINT64, (b)
printing the converted value with %Lu. Same for "CmObjectDesc->Count".


> +  return EFI_SUCCESS;
> +}
> +
> +/** A helper function for returning the Configuration Manager Objects that
> +    match the token.
> +
> +  @param [in]  This               Pointer to the Configuration Manager Protocol.
> +  @param [in]  CmObjectId         The Configuration Manager Object ID.
> +  @param [in]  Object             Pointer to the Object(s).
> +  @param [in]  ObjectSize         Total size of the Object(s).
> +  @param [in]  ObjectCount        Number of Objects.
> +  @param [in]  Token              A token identifying the object.
> +  @param [in]  HandlerProc        A handler function to search the object
> +                                  referenced by the token.
> +  @param [in, out]  CmObjectDesc  Pointer to the Configuration Manager Object
> +                                  descriptor describing the requested Object.
> +
> +  @retval EFI_SUCCESS           Success.
> +  @retval EFI_INVALID_PARAMETER A parameter is invalid.
> +  @retval EFI_NOT_FOUND         The required object information is not found.
> +**/
> +STATIC
> +EFI_STATUS
> +EFIAPI
> +HandleCmObjectRefByToken (
> +  IN  CONST EDKII_CONFIGURATION_MANAGER_PROTOCOL  * CONST This,
> +  IN  CONST CM_OBJECT_ID                                  CmObjectId,
> +  IN        VOID                                  *       Object,
> +  IN  CONST UINTN                                         ObjectSize,
> +  IN  CONST UINTN                                         ObjectCount,
> +  IN  CONST CM_OBJECT_TOKEN                               Token,
> +  IN  CONST CM_OBJECT_HANDLER_PROC                        HandlerProc,
> +  IN  OUT   CM_OBJ_DESCRIPTOR                     * CONST CmObjectDesc
> +  )
> +{
> +  EFI_STATUS  Status;
> +  CmObjectDesc->ObjectId = CmObjectId;
> +  if (Token == CM_NULL_TOKEN) {
> +    CmObjectDesc->Size = ObjectSize;
> +    CmObjectDesc->Data = (VOID*)Object;
> +    CmObjectDesc->Count = ObjectCount;
> +    Status = EFI_SUCCESS;
> +  } else {
> +    Status = HandlerProc (This, CmObjectId, Token, CmObjectDesc);
> +  }
> +
> +  DEBUG ((
> +    DEBUG_INFO,
> +    "INFO: Token = 0x%p, CmObjectId = %x, Ptr = 0x%p, Size = %d, Count = %d\n",
> +    (VOID*)Token,
> +    CmObjectId,
> +    CmObjectDesc->Data,
> +    CmObjectDesc->Size,
> +    CmObjectDesc->Count
> +    ));
> +  return Status;
> +}
> +
> +/** Return an ITS identifier array.
> +
> +  @param [in]  This        Pointer to the Configuration Manager Protocol.
> +  @param [in]  CmObjectId  The Configuration Manager Object ID.
> +  @param [in]  Token       A token for identifying the object
> +  @param [out] CmObject    Pointer to the Configuration Manager Object
> +                           descriptor describing the requested Object.
> +
> +  @retval EFI_SUCCESS           Success.
> +  @retval EFI_INVALID_PARAMETER A parameter is invalid.
> +  @retval EFI_NOT_FOUND         The required object information is not found.
> +**/
> +EFI_STATUS
> +EFIAPI
> +GetItsIdentifierArray (
> +  IN  CONST EDKII_CONFIGURATION_MANAGER_PROTOCOL  * CONST This,
> +  IN  CONST CM_OBJECT_ID                                  CmObjectId,
> +  IN  CONST CM_OBJECT_TOKEN                               Token,
> +  OUT       CM_OBJ_DESCRIPTOR                     * CONST CmObject
> +  )
> +{
> +  EDKII_PLATFORM_REPOSITORY_INFO  * PlatformRepo;
> +
> +  if ((This == NULL) || (CmObject == NULL)) {
> +    ASSERT (0);

(9) should be ASSERT (FALSE) -- please update all instances.

Better yet: in GetStandardNameSpaceObject(), you use ASSERT()
expressions that make a lot more sense, when logged. I suggest replacing
all the ASSERT (0) calls with that (more useful) pattern.

> +    return EFI_INVALID_PARAMETER;
> +  }
> +
> +  PlatformRepo = This->PlatRepoInfo;
> +
> +  if (Token != (CM_OBJECT_TOKEN)&PlatformRepo->ItsIdentifierArray) {
> +    return EFI_NOT_FOUND;
> +  }
> +
> +  CmObject->ObjectId = CmObjectId;
> +  CmObject->Size = sizeof (PlatformRepo->ItsIdentifierArray);
> +  CmObject->Data = (VOID*)&PlatformRepo->ItsIdentifierArray;
> +  CmObject->Count = ARRAY_SIZE (PlatformRepo->ItsIdentifierArray);
> +  return EFI_SUCCESS;
> +}
> +
> +/** Return a device Id mapping array.
> +
> +  @param [in]  This        Pointer to the Configuration Manager Protocol.
> +  @param [in]  CmObjectId  The Configuration Manager Object ID.
> +  @param [in]  Token       A token for identifying the object
> +  @param [out] CmObject    Pointer to the Configuration Manager Object
> +                           descriptor describing the requested Object.
> +
> +  @retval EFI_SUCCESS           Success.
> +  @retval EFI_INVALID_PARAMETER A parameter is invalid.
> +  @retval EFI_NOT_FOUND         The required object information is not found.
> +**/
> +EFI_STATUS
> +EFIAPI
> +GetDeviceIdMappingArray (
> +  IN  CONST EDKII_CONFIGURATION_MANAGER_PROTOCOL  * CONST This,
> +  IN  CONST CM_OBJECT_ID                                  CmObjectId,
> +  IN  CONST CM_OBJECT_TOKEN                               Token,
> +  OUT       CM_OBJ_DESCRIPTOR                     * CONST CmObject
> +  )
> +{
> +  EDKII_PLATFORM_REPOSITORY_INFO  * PlatformRepo;
> +
> +  if ((This == NULL) || (CmObject == NULL)) {
> +    ASSERT (0);
> +    return EFI_INVALID_PARAMETER;
> +  }
> +
> +  PlatformRepo = This->PlatRepoInfo;
> +
> +  if (Token != (CM_OBJECT_TOKEN)&PlatformRepo->DeviceIdMapping[0]) {
> +    return EFI_NOT_FOUND;
> +  }
> +
> +  CmObject->ObjectId = CmObjectId;
> +  CmObject->Size = sizeof (CM_ARM_ID_MAPPING);
> +  CmObject->Data = (VOID*)Token;
> +  CmObject->Count = 1;
> +  return EFI_SUCCESS;
> +}
> +
> +/** Function pointer called by the parser to add information.
> +
> +  Callback function that the parser can use to add new
> +  CmObj. This function must copy the CmObj data and not rely on
> +  the parser preserving the CmObj memory.
> +  This function is responsible of the Token allocation.
> +
> +  @param  [in]  ParserHandle  A handle to the parser instance.
> +  @param  [in]  Context       A pointer to the caller's context provided in
> +                              HwInfoParserInit ().
> +  @param  [in]  CmObjDesc     CM_OBJ_DESCRIPTOR containing the CmObj(s) to add.
> +  @param  [out] Token         If provided and success, contain the token
> +                              generated for the CmObj.
> +
> +  @retval EFI_SUCCESS             The function completed successfully.
> +  @retval EFI_INVALID_PARAMETER   Invalid parameter.
> +**/
> +STATIC
> +EFI_STATUS
> +EFIAPI
> +HwInfoAdd (
> +  IN        HW_INFO_PARSER_HANDLE   ParserHandle,
> +  IN        VOID                  * Context,
> +  IN  CONST CM_OBJ_DESCRIPTOR     * CmObjDesc,
> +  OUT       CM_OBJECT_TOKEN       * Token OPTIONAL
> +  )
> +{
> +  EFI_STATUS                        Status;
> +  EDKII_PLATFORM_REPOSITORY_INFO  * PlatformRepo;
> +
> +  if ((ParserHandle == NULL)  ||
> +      (Context == NULL)       ||
> +      (CmObjDesc == NULL)) {
> +    ASSERT (0);
> +    return EFI_INVALID_PARAMETER;
> +  }
> +
> +  PlatformRepo = (EDKII_PLATFORM_REPOSITORY_INFO*)Context;
> +
> +#ifndef MDEPKG_NDEBUG
> +  // Print the received objects.
> +  ParseCmObjDesc (CmObjDesc);
> +#endif

(10) Please use the DEBUG_CODE() or DEBUG_CODE_BEGIN() /
DEBUG_CODE_END() macros here.

> +
> +  Status = DynPlatRepoAddObject (
> +             PlatformRepo->DynamicPlatformRepo,
> +             CmObjDesc,
> +             Token
> +             );
> +  if (EFI_ERROR (Status)) {
> +    ASSERT (0);
> +  }

(11) Please use the dedicated macro ASSERT_EFI_ERROR().

(There are other instances of the same, please update those as well.)

> +  return Status;
> +}
> +
> +/** Cleanup the platform configuration repository.
> +
> +  @param [in]  This        Pointer to the Configuration Manager Protocol.
> +
> +  @retval EFI_SUCCESS             Success
> +  @retval EFI_INVALID_PARAMETER   A parameter is invalid.
> +**/
> +STATIC
> +EFI_STATUS
> +EFIAPI
> +CleanupPlatformRepository (
> +  IN  CONST EDKII_CONFIGURATION_MANAGER_PROTOCOL  * CONST This
> +  )
> +{
> +  EFI_STATUS                        Status;
> +  EDKII_PLATFORM_REPOSITORY_INFO  * PlatformRepo;
> +
> +  if (This == NULL) {
> +    ASSERT (0);
> +    return EFI_INVALID_PARAMETER;
> +  }
> +
> +  PlatformRepo = This->PlatRepoInfo;
> +
> +  // Shutdown the dynamic repo and free all objects.
> +  Status = DynamicPlatRepoShutdown (PlatformRepo->DynamicPlatformRepo);
> +  if (EFI_ERROR (Status)) {
> +    ASSERT (0);
> +    return Status;
> +  }
> +
> +  // Shutdown parser.
> +  Status = HwInfoParserShutdown (PlatformRepo->FdtParserHandle);
> +  if (EFI_ERROR (Status)) {
> +    ASSERT (0);
> +  }
> +  return Status;
> +}
> +
> +/** Initialize the platform configuration repository.
> +
> +  @param [in]  This        Pointer to the Configuration Manager Protocol.
> +
> +  @retval EFI_SUCCESS             Success
> +  @retval EFI_INVALID_PARAMETER   A parameter is invalid.
> +  @retval EFI_OUT_OF_RESOURCES    An allocation has failed.
> +**/
> +STATIC
> +EFI_STATUS
> +EFIAPI
> +InitializePlatformRepository (
> +  IN  CONST EDKII_CONFIGURATION_MANAGER_PROTOCOL  * CONST This
> +  )
> +{
> +  EFI_STATUS                        Status;
> +  EDKII_PLATFORM_REPOSITORY_INFO  * PlatformRepo;
> +  VOID                            * Hob;
> +
> +  if (This == NULL) {
> +    ASSERT (0);
> +    return EFI_INVALID_PARAMETER;
> +  }
> +
> +  Hob = GetFirstGuidHob (&gFdtHobGuid);
> +  if (Hob == NULL || GET_GUID_HOB_DATA_SIZE (Hob) != sizeof (UINT64)) {
> +    ASSERT (0);
> +    return EFI_NOT_FOUND;
> +  }
> +
> +  PlatformRepo = This->PlatRepoInfo;
> +  PlatformRepo->FdtBase = (VOID *)*(UINTN*)GET_GUID_HOB_DATA (Hob);
> +
> +  // Initialise the dynamic platform repository.
> +  Status = DynamicPlatRepoInit (&PlatformRepo->DynamicPlatformRepo);
> +  if (EFI_ERROR (Status)) {
> +    ASSERT (0);
> +    return Status;
> +  }
> +
> +  // Initialise the FDT parser
> +  Status = HwInfoParserInit (
> +             PlatformRepo->FdtBase,
> +             PlatformRepo,
> +             HwInfoAdd,
> +             &PlatformRepo->FdtParserHandle
> +             );
> +  if (EFI_ERROR (Status)) {
> +    ASSERT (0);
> +    goto error_handler;

(12) This label should be named ErrorHandler or just Error.

(Applies to ConfigurationManagerDxeInitialize() as well.)

> +  }
> +
> +  Status = HwInfoParse (PlatformRepo->FdtParserHandle);
> +  if (EFI_ERROR (Status)) {
> +    ASSERT (0);
> +    goto error_handler;
> +  }
> +
> +  Status = DynamicPlatRepoFinalise (PlatformRepo->DynamicPlatformRepo);
> +  if (EFI_ERROR (Status)) {
> +    ASSERT (0);
> +    goto error_handler;
> +  }
> +
> +  return EFI_SUCCESS;
> +
> +error_handler:
> +  CleanupPlatformRepository (This);
> +  return Status;
> +}
> +
> +/** Return a standard namespace object.
> +
> +  @param [in]      This        Pointer to the Configuration Manager Protocol.
> +  @param [in]      CmObjectId  The Configuration Manager Object ID.
> +  @param [in]      Token       An optional token identifying the object. If
> +                               unused this must be CM_NULL_TOKEN.
> +  @param [in, out] CmObject    Pointer to the Configuration Manager Object
> +                               descriptor describing the requested Object.
> +
> +  @retval EFI_SUCCESS           Success.
> +  @retval EFI_INVALID_PARAMETER A parameter is invalid.
> +  @retval EFI_NOT_FOUND         The required object information is not found.
> +**/
> +EFI_STATUS
> +EFIAPI
> +GetStandardNameSpaceObject (
> +  IN  CONST EDKII_CONFIGURATION_MANAGER_PROTOCOL  * CONST This,
> +  IN  CONST CM_OBJECT_ID                                  CmObjectId,
> +  IN  CONST CM_OBJECT_TOKEN                               Token OPTIONAL,
> +  IN  OUT   CM_OBJ_DESCRIPTOR                     * CONST CmObject
> +  )
> +{
> +  EFI_STATUS                        Status;
> +  EDKII_PLATFORM_REPOSITORY_INFO  * PlatformRepo;
> +  UINTN                             AcpiTableCount;
> +  CM_OBJ_DESCRIPTOR                 CmObjDesc;
> +
> +  if ((This == NULL) || (CmObject == NULL)) {
> +    ASSERT (This != NULL);
> +    ASSERT (CmObject != NULL);
> +    return EFI_INVALID_PARAMETER;
> +  }
> +
> +  Status = EFI_NOT_FOUND;
> +  PlatformRepo = This->PlatRepoInfo;
> +
> +  switch (GET_CM_OBJECT_ID (CmObjectId)) {
> +    case EStdObjCfgMgrInfo:
> +      Status = HandleCmObject (
> +                 CmObjectId,
> +                 &PlatformRepo->CmInfo,
> +                 sizeof (PlatformRepo->CmInfo),
> +                 1,
> +                 CmObject
> +                 );
> +      break;
> +
> +    case EStdObjAcpiTableList:
> +      AcpiTableCount = ARRAY_SIZE (PlatformRepo->CmAcpiTableList);
> +
> +      // Get Pci config space information.
> +      Status = DynamicPlatRepoGetObject (
> +                 PlatformRepo->DynamicPlatformRepo,
> +                 CREATE_CM_ARM_OBJECT_ID (EArmObjPciConfigSpaceInfo),
> +                 CM_NULL_TOKEN,
> +                 &CmObjDesc
> +                 );
> +      if (Status == EFI_NOT_FOUND) {
> +        // The last 3 tables are for PCIe. If PCIe information is not
> +        // present, Kvmtool was launched without the PCIe option.
> +        // Therefore, reduce the table count by 3.
> +        AcpiTableCount -= 3;
> +      } else if (EFI_ERROR (Status)) {
> +        ASSERT (0);
> +        return Status;
> +      }
> +
> +      // Get the Gic version.
> +      Status = DynamicPlatRepoGetObject (
> +                 PlatformRepo->DynamicPlatformRepo,
> +                 CREATE_CM_ARM_OBJECT_ID (EArmObjGicDInfo),
> +                 CM_NULL_TOKEN,
> +                 &CmObjDesc
> +                 );
> +      if (EFI_ERROR (Status)) {
> +        ASSERT (0);
> +        return Status;
> +      }
> +      if (((CM_ARM_GICD_INFO*)CmObjDesc.Data)->GicVersion < 3) {
> +        // IORT is only required for GicV3/4
> +        AcpiTableCount -= 1;
> +      }
> +
> +      Status = HandleCmObject (
> +                 CmObjectId,
> +                 PlatformRepo->CmAcpiTableList,
> +                 (sizeof (PlatformRepo->CmAcpiTableList[0]) * AcpiTableCount),
> +                 AcpiTableCount,
> +                 CmObject
> +                 );
> +      break;
> +
> +    default: {

(13) Please drop the brace.

(applies to ArmKvmtoolPlatformGetObject() as well)

> +      Status = EFI_NOT_FOUND;
> +      DEBUG ((
> +        DEBUG_ERROR,
> +        "ERROR: Object 0x%x. Status = %r\n",
> +        CmObjectId,
> +        Status
> +        ));
> +      break;
> +    }
> +  }
> +
> +  return Status;
> +}
> +
> +/** Return an ARM namespace object.
> +
> +  @param [in]      This        Pointer to the Configuration Manager Protocol.
> +  @param [in]      CmObjectId  The Configuration Manager Object ID.
> +  @param [in]      Token       An optional token identifying the object. If
> +                               unused this must be CM_NULL_TOKEN.
> +  @param [in, out] CmObject    Pointer to the Configuration Manager Object
> +                               descriptor describing the requested Object.
> +
> +  @retval EFI_SUCCESS           Success.
> +  @retval EFI_INVALID_PARAMETER A parameter is invalid.
> +  @retval EFI_NOT_FOUND         The required object information is not found.
> +**/
> +EFI_STATUS
> +EFIAPI
> +GetArmNameSpaceObject (
> +  IN  CONST EDKII_CONFIGURATION_MANAGER_PROTOCOL  * CONST This,
> +  IN  CONST CM_OBJECT_ID                                  CmObjectId,
> +  IN  CONST CM_OBJECT_TOKEN                               Token OPTIONAL,
> +  IN  OUT   CM_OBJ_DESCRIPTOR                     * CONST CmObject
> +  )
> +{
> +  EFI_STATUS                        Status;
> +  EDKII_PLATFORM_REPOSITORY_INFO  * PlatformRepo;
> +
> +  if ((This == NULL) || (CmObject == NULL)) {
> +    ASSERT (This != NULL);
> +    ASSERT (CmObject != NULL);
> +    return EFI_INVALID_PARAMETER;
> +  }
> +
> +  Status = EFI_NOT_FOUND;
> +  PlatformRepo = This->PlatRepoInfo;
> +
> +  // First check among the static objects.
> +  switch (GET_CM_OBJECT_ID (CmObjectId)) {
> +    case EArmObjPowerManagementProfileInfo:
> +      Status = HandleCmObject (
> +                 CmObjectId,
> +                 &PlatformRepo->PmProfileInfo,
> +                 sizeof (PlatformRepo->PmProfileInfo),
> +                 1,
> +                 CmObject
> +                 );
> +      break;
> +
> +    case EArmObjItsGroup:
> +      Status = HandleCmObject (
> +                 CmObjectId,
> +                 &PlatformRepo->ItsGroupInfo,
> +                 sizeof (PlatformRepo->ItsGroupInfo),
> +                 1,
> +                 CmObject
> +                 );
> +      break;
> +
> +    case EArmObjGicItsIdentifierArray:
> +      Status = HandleCmObjectRefByToken (
> +                 This,
> +                 CmObjectId,
> +                 PlatformRepo->ItsIdentifierArray,
> +                 sizeof (PlatformRepo->ItsIdentifierArray),
> +                 ARRAY_SIZE (PlatformRepo->ItsIdentifierArray),
> +                 Token,
> +                 GetItsIdentifierArray,
> +                 CmObject
> +                 );
> +      break;
> +
> +    case EArmObjRootComplex:
> +      Status = HandleCmObject (
> +                 CmObjectId,
> +                 &PlatformRepo->RootComplexInfo,
> +                 sizeof (PlatformRepo->RootComplexInfo),
> +                 1,
> +                 CmObject
> +                 );
> +      break;
> +
> +    case EArmObjIdMappingArray:
> +      Status = HandleCmObjectRefByToken (
> +                 This,
> +                 CmObjectId,
> +                 PlatformRepo->DeviceIdMapping,
> +                 sizeof (PlatformRepo->DeviceIdMapping),
> +                 ARRAY_SIZE (PlatformRepo->DeviceIdMapping),
> +                 Token,
> +                 GetDeviceIdMappingArray,
> +                 CmObject
> +                 );
> +      break;
> +
> +    default:
> +      // No match found among the static objects.
> +      // Check the dynamic objects.
> +      Status = DynamicPlatRepoGetObject (
> +                 PlatformRepo->DynamicPlatformRepo,
> +                 CmObjectId,
> +                 Token,
> +                 CmObject
> +                 );
> +      break;
> +  } // switch
> +
> +  if (Status == EFI_NOT_FOUND) {
> +    DEBUG ((
> +      DEBUG_INFO,
> +      "INFO: Object 0x%x. Status = %r\n",
> +      CmObjectId,
> +      Status
> +      ));
> +  } else {
> +    ASSERT_EFI_ERROR (Status);
> +  }
> +
> +  return Status;
> +}
> +
> +/** Return an OEM namespace object.
> +
> +  @param [in]      This        Pointer to the Configuration Manager Protocol.
> +  @param [in]      CmObjectId  The Configuration Manager Object ID.
> +  @param [in]      Token       An optional token identifying the object. If
> +                               unused this must be CM_NULL_TOKEN.
> +  @param [in, out] CmObject    Pointer to the Configuration Manager Object
> +                               descriptor describing the requested Object.
> +
> +  @retval EFI_SUCCESS           Success.
> +  @retval EFI_INVALID_PARAMETER A parameter is invalid.
> +  @retval EFI_NOT_FOUND         The required object information is not found.
> +**/
> +EFI_STATUS
> +EFIAPI
> +GetOemNameSpaceObject (
> +  IN  CONST EDKII_CONFIGURATION_MANAGER_PROTOCOL  * CONST This,
> +  IN  CONST CM_OBJECT_ID                                  CmObjectId,
> +  IN  CONST CM_OBJECT_TOKEN                               Token OPTIONAL,
> +  IN  OUT   CM_OBJ_DESCRIPTOR                     * CONST CmObject
> +  )
> +{
> +  EFI_STATUS  Status;
> +
> +  Status = EFI_SUCCESS;
> +  if ((This == NULL) || (CmObject == NULL)) {
> +    ASSERT (This != NULL);
> +    ASSERT (CmObject != NULL);
> +    return EFI_INVALID_PARAMETER;
> +  }
> +
> +  switch (GET_CM_OBJECT_ID (CmObjectId)) {
> +    default: {
> +      Status = EFI_NOT_FOUND;
> +      DEBUG ((
> +        DEBUG_ERROR,
> +        "ERROR: Object 0x%x. Status = %r\n",
> +        CmObjectId,
> +        Status
> +        ));
> +      break;
> +    }
> +  }
> +
> +  return Status;
> +}
> +
> +/** The GetObject function defines the interface implemented by the
> +    Configuration Manager Protocol for returning the Configuration
> +    Manager Objects.
> +
> +  @param [in]      This        Pointer to the Configuration Manager Protocol.
> +  @param [in]      CmObjectId  The Configuration Manager Object ID.
> +  @param [in]      Token       An optional token identifying the object. If
> +                               unused this must be CM_NULL_TOKEN.
> +  @param [in, out] CmObject    Pointer to the Configuration Manager Object
> +                               descriptor describing the requested Object.
> +
> +  @retval EFI_SUCCESS           Success.
> +  @retval EFI_INVALID_PARAMETER A parameter is invalid.
> +  @retval EFI_NOT_FOUND         The required object information is not found.
> +**/
> +EFI_STATUS
> +EFIAPI
> +ArmKvmtoolPlatformGetObject (
> +  IN  CONST EDKII_CONFIGURATION_MANAGER_PROTOCOL  * CONST This,
> +  IN  CONST CM_OBJECT_ID                                  CmObjectId,
> +  IN  CONST CM_OBJECT_TOKEN                               Token OPTIONAL,
> +  IN  OUT   CM_OBJ_DESCRIPTOR                     * CONST CmObject
> +  )
> +{
> +  EFI_STATUS  Status;
> +
> +  if ((This == NULL) || (CmObject == NULL)) {
> +    ASSERT (This != NULL);
> +    ASSERT (CmObject != NULL);
> +    return EFI_INVALID_PARAMETER;
> +  }
> +
> +  switch (GET_CM_NAMESPACE_ID (CmObjectId)) {
> +    case EObjNameSpaceStandard:
> +      Status = GetStandardNameSpaceObject (This, CmObjectId, Token, CmObject);
> +      break;
> +    case EObjNameSpaceArm:
> +      Status = GetArmNameSpaceObject (This, CmObjectId, Token, CmObject);
> +      break;
> +    case EObjNameSpaceOem:
> +      Status = GetOemNameSpaceObject (This, CmObjectId, Token, CmObject);
> +      break;
> +    default: {
> +      Status = EFI_INVALID_PARAMETER;
> +      DEBUG ((
> +        DEBUG_ERROR,
> +        "ERROR: Unknown Namespace Object = 0x%x. Status = %r\n",
> +        CmObjectId,
> +        Status
> +        ));
> +      break;
> +    }
> +  }
> +
> +  return Status;
> +}
> +
> +/** The SetObject function defines the interface implemented by the
> +    Configuration Manager Protocol for updating the Configuration
> +    Manager Objects.
> +
> +  @param [in]      This        Pointer to the Configuration Manager Protocol.
> +  @param [in]      CmObjectId  The Configuration Manager Object ID.
> +  @param [in]      Token       An optional token identifying the object. If
> +                               unused this must be CM_NULL_TOKEN.
> +  @param [in]      CmObject    Pointer to the Configuration Manager Object
> +                               descriptor describing the Object.
> +
> +  @retval EFI_UNSUPPORTED  This operation is not supported.
> +**/
> +EFI_STATUS
> +EFIAPI
> +ArmKvmtoolPlatformSetObject (
> +  IN  CONST EDKII_CONFIGURATION_MANAGER_PROTOCOL  * CONST This,
> +  IN  CONST CM_OBJECT_ID                                  CmObjectId,
> +  IN  CONST CM_OBJECT_TOKEN                               Token OPTIONAL,
> +  IN        CM_OBJ_DESCRIPTOR                     * CONST CmObject
> +  )
> +{
> +  return EFI_UNSUPPORTED;
> +}
> +
> +/** A structure describing the configuration manager protocol interface.
> +*/
> +STATIC
> +CONST
> +EDKII_CONFIGURATION_MANAGER_PROTOCOL KvmtoolPlatformConfigManagerProtocol = {
> +  CREATE_REVISION(1,0),
> +  ArmKvmtoolPlatformGetObject,
> +  ArmKvmtoolPlatformSetObject,
> +  &KvmtoolPlatRepositoryInfo
> +};
> +
> +/**
> +  Entrypoint of Configuration Manager Dxe.
> +
> +  @param  ImageHandle
> +  @param  SystemTable
> +
> +  @return EFI_SUCCESS
> +  @return EFI_LOAD_ERROR
> +  @return EFI_OUT_OF_RESOURCES

(14) These should be @retval, not @return.

> +**/
> +EFI_STATUS
> +EFIAPI
> +ConfigurationManagerDxeInitialize (
> +  IN EFI_HANDLE          ImageHandle,
> +  IN EFI_SYSTEM_TABLE  * SystemTable
> +  )
> +{
> +  EFI_STATUS  Status;
> +
> +  if (PcdGetBool (PcdForceNoAcpi)) {
> +    // Use DT and not ACPI.
> +    return EFI_SUCCESS;
> +  }

(15) This is wrong.

KvmtoolPlatformDxe already (correctly) installs either
"gEdkiiPlatformHasAcpiGuid" or "gEdkiiPlatformHasDeviceTreeGuid".
"PcdForceNoAcpi" must not be used outside of that driver.

In this driver, the DEPEX section of the INF file should list
"gEdkiiPlatformHasAcpiGuid". If ACPI is disabled, this driver should not
be dispatched at all.

> +
> +  Status = gBS->InstallProtocolInterface (
> +                  &ImageHandle,
> +                  &gEdkiiConfigurationManagerProtocolGuid,
> +                  EFI_NATIVE_INTERFACE,
> +                  (VOID*)&KvmtoolPlatformConfigManagerProtocol
> +                  );
> +  if (EFI_ERROR (Status)) {
> +    DEBUG ((
> +      DEBUG_ERROR,
> +      "ERROR: Failed to get Install Configuration Manager Protocol." \
> +      " Status = %r\n",
> +      Status
> +      ));
> +    goto error_handler;
> +  }
> +
> +  Status = InitializePlatformRepository (
> +             &KvmtoolPlatformConfigManagerProtocol
> +             );
> +  if (EFI_ERROR (Status)) {
> +    DEBUG ((
> +      DEBUG_ERROR,
> +      "ERROR: Failed to initialize the Platform Configuration Repository." \
> +      " Status = %r\n",
> +      Status
> +      ));
> +  }
> +
> +error_handler:
> +  return Status;
> +}

(16) I've paid basically zero attention to your error handlers, but this
is so obviously wrong that it catches my eye. If
InitializePlatformRepository() fails and so you exit with an error, the
driver will be unloaded, and your installed protocol interface will be a
dangling one.

> +
> +/**
> +  Unload function for this image.
> +
> +  @param ImageHandle   Handle for the image of this driver.
> +
> +  @retval EFI_SUCCESS  Driver unloaded successfully.
> +  @return other        Driver can not unloaded.
> +**/
> +EFI_STATUS
> +EFIAPI
> +ConfigurationManagerDxeUnloadImage (
> +  IN EFI_HANDLE         ImageHandle
> +  )
> +{
> +  return CleanupPlatformRepository (&KvmtoolPlatformConfigManagerProtocol);
> +}
> diff --git a/ArmVirtPkg/KvmtoolCfgMgrDxe/ConfigurationManager.h b/ArmVirtPkg/KvmtoolCfgMgrDxe/ConfigurationManager.h
> new file mode 100644
> index 000000000000..94cfca3b7671
> --- /dev/null
> +++ b/ArmVirtPkg/KvmtoolCfgMgrDxe/ConfigurationManager.h
> @@ -0,0 +1,94 @@
> +/** @file
> +
> +  Copyright (c) 2021, Arm Limited. All rights reserved.<BR>
> +
> +  SPDX-License-Identifier: BSD-2-Clause-Patent
> +
> +  @par Glossary:
> +    - Cm or CM   - Configuration Manager
> +    - Obj or OBJ - Object
> +**/
> +
> +#ifndef CONFIGURATION_MANAGER_H_
> +#define CONFIGURATION_MANAGER_H_
> +
> +/** C array containing the compiled AML template.
> +    This symbol is defined in the auto generated C file
> +    containing the AML bytecode array.
> +*/

(17) Please apply all the comment observations I made for the C file to
the header file.

> +extern CHAR8  dsdt_aml_code[];
> +
> +/** The configuration manager version.
> +*/
> +#define CONFIGURATION_MANAGER_REVISION CREATE_REVISION (1, 0)
> +
> +/** The OEM ID
> +*/
> +#define CFG_MGR_OEM_ID    { 'A', 'R', 'M', 'L', 'T', 'D' }
> +
> +/** A function that prepares Configuration Manager Objects for returning.
> +
> +  @param [in]  This        Pointer to the Configuration Manager Protocol.
> +  @param [in]  CmObjectId  The Configuration Manager Object ID.
> +  @param [in]  Token       A token for identifying the object.
> +  @param [out] CmObject    Pointer to the Configuration Manager Object
> +                           descriptor describing the requested Object.
> +
> +  @retval EFI_SUCCESS           Success.
> +  @retval EFI_INVALID_PARAMETER A parameter is invalid.
> +  @retval EFI_NOT_FOUND         The required object information is not found.
> +**/
> +typedef EFI_STATUS (*CM_OBJECT_HANDLER_PROC) (
> +  IN  CONST EDKII_CONFIGURATION_MANAGER_PROTOCOL  * CONST This,
> +  IN  CONST CM_OBJECT_ID                                  CmObjectId,
> +  IN  CONST CM_OBJECT_TOKEN                               Token,
> +  IN  OUT   CM_OBJ_DESCRIPTOR                     * CONST CmObject
> +  );
> +
> +/** A helper macro for mapping a reference token.
> +*/
> +#define REFERENCE_TOKEN(Field)                                   \
> +          (CM_OBJECT_TOKEN)((UINT8*)&KvmtoolPlatRepositoryInfo + \
> +           OFFSET_OF (EDKII_PLATFORM_REPOSITORY_INFO, Field))
> +
> +/** The number of ACPI tables to install
> +*/
> +#define PLAT_ACPI_TABLE_COUNT       10
> +
> +/** A structure describing the platform configuration
> +    manager repository information
> +*/
> +typedef struct PlatformRepositoryInfo {

(18) the "PlatformRepositoryInfo" struct tag is not spelled
idiomatically. On the other hand, it's also not used anywhere. I suggest
simply dropping it.

> +  /// Configuration Manager Information.
> +  CM_STD_OBJ_CONFIGURATION_MANAGER_INFO CmInfo;
> +
> +  /// List of ACPI tables
> +  CM_STD_OBJ_ACPI_TABLE_INFO            CmAcpiTableList[PLAT_ACPI_TABLE_COUNT];
> +
> +  /// Power management profile information
> +  CM_ARM_POWER_MANAGEMENT_PROFILE_INFO  PmProfileInfo;
> +
> +  /// ITS Group node
> +  CM_ARM_ITS_GROUP_NODE                 ItsGroupInfo;
> +
> +  /// ITS Identifier array
> +  CM_ARM_ITS_IDENTIFIER                 ItsIdentifierArray[1];
> +
> +  /// PCI Root complex node
> +  CM_ARM_ROOT_COMPLEX_NODE              RootComplexInfo;
> +
> +  /// Array of DeviceID mapping
> +  CM_ARM_ID_MAPPING                     DeviceIdMapping[1];
> +
> +  /// Dynamic platform repository.
> +  /// CmObj created by parsing the Kvmtool device tree are stored here.
> +  DYNAMIC_PLATFORM_REPOSITORY_INFO    * DynamicPlatformRepo;
> +
> +  /// Base address of the FDT.
> +  VOID                                * FdtBase;
> +
> +  /// A handle to the FDT HwInfoParser.
> +  HW_INFO_PARSER_HANDLE                 FdtParserHandle;
> +} EDKII_PLATFORM_REPOSITORY_INFO;
> +
> +#endif // CONFIGURATION_MANAGER_H_
> diff --git a/ArmVirtPkg/KvmtoolCfgMgrDxe/ConfigurationManagerDxe.inf b/ArmVirtPkg/KvmtoolCfgMgrDxe/ConfigurationManagerDxe.inf
> new file mode 100644
> index 000000000000..9f0bf72fce2d
> --- /dev/null
> +++ b/ArmVirtPkg/KvmtoolCfgMgrDxe/ConfigurationManagerDxe.inf
> @@ -0,0 +1,58 @@
> +## @file
> +#  Configuration Manager Dxe
> +#
> +#  Copyright (c) 2021, Arm Limited. All rights reserved.<BR>
> +#
> +#  SPDX-License-Identifier: BSD-2-Clause-Patent
> +##
> +
> +[Defines]
> +  INF_VERSION                    = 0x0001001B
> +  BASE_NAME                      = ConfigurationManagerDxe
> +  FILE_GUID                      = 3C80D366-510C-4154-BB3A-E12439AD337C
> +  MODULE_TYPE                    = DXE_DRIVER
> +  VERSION_STRING                 = 1.0
> +  ENTRY_POINT                    = ConfigurationManagerDxeInitialize
> +  UNLOAD_IMAGE                   = ConfigurationManagerDxeUnloadImage
> +
> +#
> +# The following information is for reference only and not required by the build tools.
> +#
> +#  VALID_ARCHITECTURES           = ARM AARCH64
> +#
> +
> +[Sources]
> +  AslTables/Dsdt.asl
> +  ConfigurationManager.c
> +  ConfigurationManager.h
> +  ConfigurationManagerDxe.inf
> +
> +[Packages]
> +  ArmVirtPkg/ArmVirtPkg.dec
> +  DynamicTablesPkg/DynamicTablesPkg.dec
> +  EmbeddedPkg/EmbeddedPkg.dec
> +  MdePkg/MdePkg.dec
> +
> +[LibraryClasses]
> +  DynamicPlatRepoLib
> +  HobLib
> +  HwInfoParserLib
> +  PrintLib
> +  TableHelperLib
> +  UefiBootServicesTableLib
> +  UefiDriverEntryPoint
> +  UefiRuntimeServicesTableLib
> +
> +[Protocols]
> +  gEdkiiConfigurationManagerProtocolGuid
> +
> +[Guids]
> +  gFdtHobGuid
> +
> +[FixedPcd]

(19) please drop this empty section.

> +
> +[Pcd]
> +  gArmVirtTokenSpaceGuid.PcdForceNoAcpi
> +
> +[Depex]
> +  TRUE
> 

I can't see myself reviewing this again (even at this level), so I'll
trust you on all the updates.

Acked-by: Laszlo Ersek <lersek@redhat.com>

Thanks
Laszlo

Re: [edk2-devel] [PATCH v1 3/5] ArmVirtPkg: Add Configuration Manager for Kvmtool firmware

[Laszlo Ersek]

On 06/23/21 16:06, PierreGondois wrote:

> diff --git a/ArmVirtPkg/KvmtoolCfgMgrDxe/ConfigurationManagerDxe.inf b/ArmVirtPkg/KvmtoolCfgMgrDxe/ConfigurationManagerDxe.inf
> new file mode 100644
> index 000000000000..9f0bf72fce2d
> --- /dev/null
> +++ b/ArmVirtPkg/KvmtoolCfgMgrDxe/ConfigurationManagerDxe.inf

> +#
> +# The following information is for reference only and not required by the build tools.
> +#
> +#  VALID_ARCHITECTURES           = ARM AARCH64
> +#

(20) ACPI is undefined for ARM, therefore VALID_ARCHITECTURES should
only list AARCH64.

Thanks
Laszlo

[PATCH v1 4/5] ArmVirtPkg: Enable ACPI support for Kvmtool

[PierreGondois]

From: Sami Mujawar <sami.mujawar@arm.com>

A Configuration Manager that uses the Dynamic Tables framework
to generate ACPI tables for Kvmtool Guests has been provided.
This Configuration Manager uses the FdtHwInfoParser module to
parse the Kvmtool Device Tree and generate the required
Configuration Manager objects for generating the ACPI tables.

Therefore, enable ACPI table generation for Kvmtool.

Signed-off-by: Sami Mujawar <sami.mujawar@arm.com>
Signed-off-by: Pierre Gondois <Pierre.Gondois@arm.com>
---
 ArmVirtPkg/ArmVirtKvmTool.dsc | 15 +++++++++++++--
 ArmVirtPkg/ArmVirtKvmTool.fdf | 11 +++++++++++
 2 files changed, 24 insertions(+), 2 deletions(-)

diff --git a/ArmVirtPkg/ArmVirtKvmTool.dsc b/ArmVirtPkg/ArmVirtKvmTool.dsc
index 920880796ac2..b02324312f18 100644
--- a/ArmVirtPkg/ArmVirtKvmTool.dsc
+++ b/ArmVirtPkg/ArmVirtKvmTool.dsc
@@ -28,6 +28,7 @@ [Defines]
   FLASH_DEFINITION               = ArmVirtPkg/ArmVirtKvmTool.fdf
 
 !include ArmVirtPkg/ArmVirt.dsc.inc
+!include DynamicTablesPkg/DynamicTables.dsc.inc
 
 !include MdePkg/MdeLibs.dsc.inc
 
@@ -144,6 +145,11 @@ [PcdsFixedAtBuild.common]
   #
   gEmbeddedTokenSpaceGuid.PcdPrePiCpuIoSize|16
 
+  #
+  # ACPI Table Version
+  #
+  gEfiMdeModulePkgTokenSpaceGuid.PcdAcpiExposedTableVersions|0x20
+
 [PcdsPatchableInModule.common]
   #
   # This will be overridden in the code
@@ -198,8 +204,8 @@ [PcdsDynamicDefault.common]
   gEfiMdeModulePkgTokenSpaceGuid.PcdSetupVideoHorizontalResolution|640
   gEfiMdeModulePkgTokenSpaceGuid.PcdSetupVideoVerticalResolution|480
 
-  ## Force DTB
-  gArmVirtTokenSpaceGuid.PcdForceNoAcpi|TRUE
+  ## Set default option to ACPI
+  gArmVirtTokenSpaceGuid.PcdForceNoAcpi|FALSE
 
   # Setup Flash storage variables
   gEfiMdeModulePkgTokenSpaceGuid.PcdFlashNvStorageVariableBase|0
@@ -356,3 +362,8 @@ [Components.common]
   }
   OvmfPkg/VirtioPciDeviceDxe/VirtioPciDeviceDxe.inf
   OvmfPkg/Virtio10Dxe/Virtio10.inf
+  #
+  # ACPI Support
+  #
+  MdeModulePkg/Universal/Acpi/AcpiTableDxe/AcpiTableDxe.inf
+  ArmVirtPkg/KvmtoolCfgMgrDxe/ConfigurationManagerDxe.inf
diff --git a/ArmVirtPkg/ArmVirtKvmTool.fdf b/ArmVirtPkg/ArmVirtKvmTool.fdf
index 076155199905..5ba4c579f050 100644
--- a/ArmVirtPkg/ArmVirtKvmTool.fdf
+++ b/ArmVirtPkg/ArmVirtKvmTool.fdf
@@ -204,6 +204,17 @@ [FV.FvMain]
   INF OvmfPkg/VirtioPciDeviceDxe/VirtioPciDeviceDxe.inf
   INF OvmfPkg/Virtio10Dxe/Virtio10.inf
 
+  #
+  # ACPI Support
+  #
+  INF MdeModulePkg/Universal/Acpi/AcpiTableDxe/AcpiTableDxe.inf
+  #
+  # Dynamic Table fdf
+  #
+  !include DynamicTablesPkg/DynamicTables.fdf.inc
+
+  INF ArmVirtPkg/KvmtoolCfgMgrDxe/ConfigurationManagerDxe.inf
+
   #
   # TianoCore logo (splash screen)
   #
-- 
2.17.1

Re: [edk2-devel] [PATCH v1 4/5] ArmVirtPkg: Enable ACPI support for Kvmtool

[Laszlo Ersek]

On 06/23/21 16:06, PierreGondois wrote:
> From: Sami Mujawar <sami.mujawar@arm.com>
> 
> A Configuration Manager that uses the Dynamic Tables framework
> to generate ACPI tables for Kvmtool Guests has been provided.
> This Configuration Manager uses the FdtHwInfoParser module to
> parse the Kvmtool Device Tree and generate the required
> Configuration Manager objects for generating the ACPI tables.
> 
> Therefore, enable ACPI table generation for Kvmtool.
> 
> Signed-off-by: Sami Mujawar <sami.mujawar@arm.com>
> Signed-off-by: Pierre Gondois <Pierre.Gondois@arm.com>
> ---
>  ArmVirtPkg/ArmVirtKvmTool.dsc | 15 +++++++++++++--
>  ArmVirtPkg/ArmVirtKvmTool.fdf | 11 +++++++++++
>  2 files changed, 24 insertions(+), 2 deletions(-)
> 
> diff --git a/ArmVirtPkg/ArmVirtKvmTool.dsc b/ArmVirtPkg/ArmVirtKvmTool.dsc
> index 920880796ac2..b02324312f18 100644
> --- a/ArmVirtPkg/ArmVirtKvmTool.dsc
> +++ b/ArmVirtPkg/ArmVirtKvmTool.dsc
> @@ -28,6 +28,7 @@ [Defines]
>    FLASH_DEFINITION               = ArmVirtPkg/ArmVirtKvmTool.fdf
>  
>  !include ArmVirtPkg/ArmVirt.dsc.inc
> +!include DynamicTablesPkg/DynamicTables.dsc.inc

(1) This doesn't seem right. In fact, ARM (not AARCH64) support claimed in "DynamicTablesPkg/DynamicTablesPkg.dsc" seems bogus in the first place; to my understanding, ACPI is not defined for 32-bit ARM.

More precisely, this !include directive is OK, but "DynamicTablesPkg/DynamicTables.dsc.inc" file should not provide a

  [Components.common]

section, but a

  [Components.AARCH64]

section. Refer to "ArmVirtPkg/ArmVirt.dsc.inc" please:

[Components.AARCH64]
  #
  # ACPI Support
  #
  MdeModulePkg/Universal/Acpi/AcpiTableDxe/AcpiTableDxe.inf {
    <LibraryClasses>
      NULL|EmbeddedPkg/Library/PlatformHasAcpiLib/PlatformHasAcpiLib.inf
  }


>  
>  !include MdePkg/MdeLibs.dsc.inc
>  
> @@ -144,6 +145,11 @@ [PcdsFixedAtBuild.common]
>    #
>    gEmbeddedTokenSpaceGuid.PcdPrePiCpuIoSize|16
>  
> +  #
> +  # ACPI Table Version
> +  #
> +  gEfiMdeModulePkgTokenSpaceGuid.PcdAcpiExposedTableVersions|0x20
> +
>  [PcdsPatchableInModule.common]
>    #
>    # This will be overridden in the code

(2) This hunk is superfluous. Please refer to "MdeModulePkg/MdeModulePkg.dec":

[PcdsFixedAtBuild.AARCH64, PcdsPatchableInModule.AARCH64]
  gEfiMdeModulePkgTokenSpaceGuid.PcdAcpiExposedTableVersions|0x20|UINT32|0x0001004c

If you simply don't list the PCD in your DSC file, you'll get the default value, and the most restrictive access method declared for the PCD in the DEC file (here: fixed-at-build).


> @@ -198,8 +204,8 @@ [PcdsDynamicDefault.common]
>    gEfiMdeModulePkgTokenSpaceGuid.PcdSetupVideoHorizontalResolution|640
>    gEfiMdeModulePkgTokenSpaceGuid.PcdSetupVideoVerticalResolution|480
>  
> -  ## Force DTB
> -  gArmVirtTokenSpaceGuid.PcdForceNoAcpi|TRUE
> +  ## Set default option to ACPI
> +  gArmVirtTokenSpaceGuid.PcdForceNoAcpi|FALSE
>  
>    # Setup Flash storage variables
>    gEfiMdeModulePkgTokenSpaceGuid.PcdFlashNvStorageVariableBase|0

(3) Same story as (2), please refer to "ArmVirtPkg/ArmVirtPkg.dec":

[PcdsDynamic]
  #
  # Whether to force disable ACPI, regardless of the fw_cfg settings
  # exposed by QEMU
  #
  gArmVirtTokenSpaceGuid.PcdForceNoAcpi|0x0|BOOLEAN|0x00000003


> @@ -356,3 +362,8 @@ [Components.common]
>    }
>    OvmfPkg/VirtioPciDeviceDxe/VirtioPciDeviceDxe.inf
>    OvmfPkg/Virtio10Dxe/Virtio10.inf
> +  #
> +  # ACPI Support
> +  #
> +  MdeModulePkg/Universal/Acpi/AcpiTableDxe/AcpiTableDxe.inf

(4) Superfluous by virtue of including "ArmVirtPkg/ArmVirt.dsc.inc" already.


> +  ArmVirtPkg/KvmtoolCfgMgrDxe/ConfigurationManagerDxe.inf

(5) This should be in a [Components.AARCH64] section, not in [Components.common].


> diff --git a/ArmVirtPkg/ArmVirtKvmTool.fdf b/ArmVirtPkg/ArmVirtKvmTool.fdf
> index 076155199905..5ba4c579f050 100644
> --- a/ArmVirtPkg/ArmVirtKvmTool.fdf
> +++ b/ArmVirtPkg/ArmVirtKvmTool.fdf
> @@ -204,6 +204,17 @@ [FV.FvMain]
>    INF OvmfPkg/VirtioPciDeviceDxe/VirtioPciDeviceDxe.inf
>    INF OvmfPkg/Virtio10Dxe/Virtio10.inf
>  
> +  #
> +  # ACPI Support
> +  #
> +  INF MdeModulePkg/Universal/Acpi/AcpiTableDxe/AcpiTableDxe.inf
> +  #
> +  # Dynamic Table fdf
> +  #
> +  !include DynamicTablesPkg/DynamicTables.fdf.inc
> +
> +  INF ArmVirtPkg/KvmtoolCfgMgrDxe/ConfigurationManagerDxe.inf
> +
>    #
>    # TianoCore logo (splash screen)
>    #
> 

(6) Please do what "ArmVirtPkg/ArmVirtQemuFvMain.fdf.inc" does:

!if $(ARCH) == AARCH64
  INF MdeModulePkg/Universal/Acpi/AcpiTableDxe/AcpiTableDxe.inf
  ...
!endif


Acked-by: Laszlo Ersek <lersek@redhat.com>

Thanks
Laszlo

[PATCH v1 5/5] ArmVirtPkg: Enable Acpiview for ArmVirtPkg

[PierreGondois]

From: Sami Mujawar <sami.mujawar@arm.com>

Acpiview is a command line tool allowing to display, dump, or
check installed ACPI tables. Add the tool to ArmVirt platforms.

Signed-off-by: Sami Mujawar <sami.mujawar@arm.com>
Signed-off-by: Pierre Gondois <Pierre.Gondois@arm.com>
---
 ArmVirtPkg/ArmVirt.dsc.inc | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/ArmVirtPkg/ArmVirt.dsc.inc b/ArmVirtPkg/ArmVirt.dsc.inc
index d9abadbe708c..269ac4990a6c 100644
--- a/ArmVirtPkg/ArmVirt.dsc.inc
+++ b/ArmVirtPkg/ArmVirt.dsc.inc
@@ -1,5 +1,5 @@
 #
-#  Copyright (c) 2011-2015, ARM Limited. All rights reserved.
+#  Copyright (c) 2011-2021, Arm Limited. All rights reserved.
 #  Copyright (c) 2014, Linaro Limited. All rights reserved.
 #  Copyright (c) 2015 - 2018, Intel Corporation. All rights reserved.
 #  Copyright (c) Microsoft Corporation.
@@ -398,6 +398,7 @@ [Components.common]
       NULL|ShellPkg/Library/UefiShellLevel3CommandsLib/UefiShellLevel3CommandsLib.inf
       NULL|ShellPkg/Library/UefiShellDriver1CommandsLib/UefiShellDriver1CommandsLib.inf
       NULL|ShellPkg/Library/UefiShellDebug1CommandsLib/UefiShellDebug1CommandsLib.inf
+      NULL|ShellPkg/Library/UefiShellAcpiViewCommandLib/UefiShellAcpiViewCommandLib.inf
       NULL|ShellPkg/Library/UefiShellInstall1CommandsLib/UefiShellInstall1CommandsLib.inf
       NULL|ShellPkg/Library/UefiShellNetwork1CommandsLib/UefiShellNetwork1CommandsLib.inf
 !if $(NETWORK_IP6_ENABLE) == TRUE
-- 
2.17.1

Re: [edk2-devel] [PATCH v1 5/5] ArmVirtPkg: Enable Acpiview for ArmVirtPkg

[Laszlo Ersek]

On 06/23/21 16:06, PierreGondois wrote:
> From: Sami Mujawar <sami.mujawar@arm.com>
> 
> Acpiview is a command line tool allowing to display, dump, or
> check installed ACPI tables. Add the tool to ArmVirt platforms.
> 
> Signed-off-by: Sami Mujawar <sami.mujawar@arm.com>
> Signed-off-by: Pierre Gondois <Pierre.Gondois@arm.com>
> ---
>  ArmVirtPkg/ArmVirt.dsc.inc | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/ArmVirtPkg/ArmVirt.dsc.inc b/ArmVirtPkg/ArmVirt.dsc.inc
> index d9abadbe708c..269ac4990a6c 100644
> --- a/ArmVirtPkg/ArmVirt.dsc.inc
> +++ b/ArmVirtPkg/ArmVirt.dsc.inc
> @@ -1,5 +1,5 @@
>  #
> -#  Copyright (c) 2011-2015, ARM Limited. All rights reserved.
> +#  Copyright (c) 2011-2021, Arm Limited. All rights reserved.
>  #  Copyright (c) 2014, Linaro Limited. All rights reserved.
>  #  Copyright (c) 2015 - 2018, Intel Corporation. All rights reserved.
>  #  Copyright (c) Microsoft Corporation.
> @@ -398,6 +398,7 @@ [Components.common]
>        NULL|ShellPkg/Library/UefiShellLevel3CommandsLib/UefiShellLevel3CommandsLib.inf
>        NULL|ShellPkg/Library/UefiShellDriver1CommandsLib/UefiShellDriver1CommandsLib.inf
>        NULL|ShellPkg/Library/UefiShellDebug1CommandsLib/UefiShellDebug1CommandsLib.inf
> +      NULL|ShellPkg/Library/UefiShellAcpiViewCommandLib/UefiShellAcpiViewCommandLib.inf
>        NULL|ShellPkg/Library/UefiShellInstall1CommandsLib/UefiShellInstall1CommandsLib.inf
>        NULL|ShellPkg/Library/UefiShellNetwork1CommandsLib/UefiShellNetwork1CommandsLib.inf
>  !if $(NETWORK_IP6_ENABLE) == TRUE
> 

I disagree with this patch, as it will cause the Shell binary in all
ArmVirtPkg platforms to include the (rather large) ACPIVIEW command.

ACPIVIEW is super useful for when the tables are (dynamically) generated
by the firmware itself, but that does not apply to the Qemu and Xen
platforms.

Note NETWORK_IP6_ENABLE: UefiShellNetwork2CommandsLib is only hooked
into the shell application if NETWORK_IP6_ENABLE is TRUE.

Please add

  DEFINE ACPIVIEW_ENABLE                 = TRUE

to "ArmVirtPkg/ArmVirtKvmTool.dsc", and in "ArmVirtPkg/ArmVirt.dsc.inc",
include the new command lib conditionally on ACPIVIEW_ENABLE being TRUE.
(Can be in the same patch.)

Acked-by: Laszlo Ersek <lersek@redhat.com>


Thanks
Laszlo

Re: [edk2-devel] [PATCH v1 5/5] ArmVirtPkg: Enable Acpiview for ArmVirtPkg

[Laszlo Ersek]

On 06/24/21 14:59, Laszlo Ersek wrote:
> On 06/23/21 16:06, PierreGondois wrote:
>> From: Sami Mujawar <sami.mujawar@arm.com>
>>
>> Acpiview is a command line tool allowing to display, dump, or
>> check installed ACPI tables. Add the tool to ArmVirt platforms.
>>
>> Signed-off-by: Sami Mujawar <sami.mujawar@arm.com>
>> Signed-off-by: Pierre Gondois <Pierre.Gondois@arm.com>
>> ---
>>  ArmVirtPkg/ArmVirt.dsc.inc | 3 ++-
>>  1 file changed, 2 insertions(+), 1 deletion(-)
>>
>> diff --git a/ArmVirtPkg/ArmVirt.dsc.inc b/ArmVirtPkg/ArmVirt.dsc.inc
>> index d9abadbe708c..269ac4990a6c 100644
>> --- a/ArmVirtPkg/ArmVirt.dsc.inc
>> +++ b/ArmVirtPkg/ArmVirt.dsc.inc
>> @@ -1,5 +1,5 @@
>>  #
>> -#  Copyright (c) 2011-2015, ARM Limited. All rights reserved.
>> +#  Copyright (c) 2011-2021, Arm Limited. All rights reserved.
>>  #  Copyright (c) 2014, Linaro Limited. All rights reserved.
>>  #  Copyright (c) 2015 - 2018, Intel Corporation. All rights reserved.
>>  #  Copyright (c) Microsoft Corporation.
>> @@ -398,6 +398,7 @@ [Components.common]
>>        NULL|ShellPkg/Library/UefiShellLevel3CommandsLib/UefiShellLevel3CommandsLib.inf
>>        NULL|ShellPkg/Library/UefiShellDriver1CommandsLib/UefiShellDriver1CommandsLib.inf
>>        NULL|ShellPkg/Library/UefiShellDebug1CommandsLib/UefiShellDebug1CommandsLib.inf
>> +      NULL|ShellPkg/Library/UefiShellAcpiViewCommandLib/UefiShellAcpiViewCommandLib.inf
>>        NULL|ShellPkg/Library/UefiShellInstall1CommandsLib/UefiShellInstall1CommandsLib.inf
>>        NULL|ShellPkg/Library/UefiShellNetwork1CommandsLib/UefiShellNetwork1CommandsLib.inf
>>  !if $(NETWORK_IP6_ENABLE) == TRUE
>>
> 
> I disagree with this patch, as it will cause the Shell binary in all
> ArmVirtPkg platforms to include the (rather large) ACPIVIEW command.
> 
> ACPIVIEW is super useful for when the tables are (dynamically) generated
> by the firmware itself, but that does not apply to the Qemu and Xen
> platforms.
> 
> Note NETWORK_IP6_ENABLE: UefiShellNetwork2CommandsLib is only hooked
> into the shell application if NETWORK_IP6_ENABLE is TRUE.
> 
> Please add
> 
>   DEFINE ACPIVIEW_ENABLE                 = TRUE
> 
> to "ArmVirtPkg/ArmVirtKvmTool.dsc",

To clarify: please place

  DEFINE ACPIVIEW_ENABLE                 = TRUE

in a new [Defines.AARCH64] section in "ArmVirtPkg/ArmVirtKvmTool.dsc",
not in the existent [Defines] section.

This should happen just before !including "ArmVirtPkg/ArmVirt.dsc.inc".

Thanks
Laszlo

> and in "ArmVirtPkg/ArmVirt.dsc.inc",
> include the new command lib conditionally on ACPIVIEW_ENABLE being TRUE.
> (Can be in the same patch.)
> 
> Acked-by: Laszlo Ersek <lersek@redhat.com>
> 
> 
> Thanks
> Laszlo
> 

Re: [edk2-devel] [PATCH v1 0/5] Add ACPI support for Kvmtool

[Laszlo Ersek]

On 06/23/21 16:06, PierreGondois wrote:
> From: Pierre Gondois <Pierre.Gondois@arm.com>
> 
> Kvmtool dynamically generates a device tree describing the platform
> to boot on. Using the patch-sets listed below, the DynamicTables
> framework generates ACPI tables describing a similar platform.
> 
> This patch-set:
>  - adds a ConfigurationManager allowing to generate ACPI tables
>    for Kvmtool
>  - adds the acpiview command line utility to the ArmVirtPkg
>  - update ArmVirtPkg.ci.yaml to add new words and use the
>    DynamicTablesPkg
> 
> This patch sets also set the default platform description format
> to ACPI instead of the device tree (c.f.: PcdForceNoAcpi is set
> to FALSE).
> 
> The changes can be seen at: https://github.com/PierreARM/edk2/tree/1456_Add_ACPI_support_for_Kvmtool_v1
> The results of the CI can be seen at: https://github.com/tianocore/edk2/pull/1753
> 
> This patch-set is dependent over the following patch-sets:
>   [PATCH v1 00/10] Various DynamicTablesPkg modifications 
>   https://edk2.groups.io/g/devel/message/76929
> and:
>   [PATCH v1 00/13] Create a SSDT CPU topology generator 
>   https://edk2.groups.io/g/devel/message/76941
> and:
>   [PATCH v1 0/7] Create a SSDT PCIe generator 
>   https://edk2.groups.io/g/devel/message/76958
> and:
>   [PATCH v1 00/14] Implement a FdtHwInfoParserLib
>   https://edk2.groups.io/g/devel/message/76967
> and:
>   [PATCH v1 0/5] Add DynamicPlatRepoLib
>   https://edk2.groups.io/g/devel/message/76984

Not sure if you want just one BZ for all of these subfeatures, or one BZ
per subfeature, but we definitely need at least one BZ for this series.
Please update the commit messages accordingly.

Thanks
Laszlo

> 
> Pierre Gondois (1):
>   ArmVirtPkg: Add cspell exceptions
> 
> Sami Mujawar (4):
>   ArmVirtPkg: Add DSDT ACPI table for Kvmtool firmware
>   ArmVirtPkg: Add Configuration Manager for Kvmtool firmware
>   ArmVirtPkg: Enable ACPI support for Kvmtool
>   ArmVirtPkg: Enable Acpiview for ArmVirtPkg
> 
>  ArmVirtPkg/ArmVirt.dsc.inc                    |   3 +-
>  ArmVirtPkg/ArmVirtKvmTool.dsc                 |  18 +-
>  ArmVirtPkg/ArmVirtKvmTool.fdf                 |  11 +
>  ArmVirtPkg/ArmVirtPkg.ci.yaml                 |   3 +
>  .../KvmtoolCfgMgrDxe/AslTables/Dsdt.asl       |  19 +
>  .../KvmtoolCfgMgrDxe/ConfigurationManager.c   | 948 ++++++++++++++++++
>  .../KvmtoolCfgMgrDxe/ConfigurationManager.h   |  94 ++
>  .../ConfigurationManagerDxe.inf               |  58 ++
>  8 files changed, 1151 insertions(+), 3 deletions(-)
>  create mode 100644 ArmVirtPkg/KvmtoolCfgMgrDxe/AslTables/Dsdt.asl
>  create mode 100644 ArmVirtPkg/KvmtoolCfgMgrDxe/ConfigurationManager.c
>  create mode 100644 ArmVirtPkg/KvmtoolCfgMgrDxe/ConfigurationManager.h
>  create mode 100644 ArmVirtPkg/KvmtoolCfgMgrDxe/ConfigurationManagerDxe.inf
> 

Re: [edk2-devel] [PATCH v1 0/5] Add ACPI support for Kvmtool

[Laszlo Ersek]

On 06/23/21 16:06, PierreGondois wrote:

> Pierre Gondois (1):
>   ArmVirtPkg: Add cspell exceptions
> 
> Sami Mujawar (4):
>   ArmVirtPkg: Add DSDT ACPI table for Kvmtool firmware
>   ArmVirtPkg: Add Configuration Manager for Kvmtool firmware
>   ArmVirtPkg: Enable ACPI support for Kvmtool
>   ArmVirtPkg: Enable Acpiview for ArmVirtPkg

The subject lines of Sami's 4 patches should be updated as follows:

ArmVirtPkg/Kvmtool: Add DSDT ACPI table
ArmVirtPkg/Kvmtool: Add Configuration Manager
ArmVirtPkg/Kvmtool: Enable ACPI support
ArmVirtPkg/Kvmtool: Enable Acpiview

(regarding the last patch, I requested in its subthread that ACPIVIEW be
restricted to kvmtool please.)

Thanks
Laszlo