Re: [PATCH edk2-platforms 3/7] SbsaQemu: Add new ACPI driver and FDT parser to count CPUs

["Leif Lindholm" <leif@nuviainc.com>]

On Wed, Aug 19, 2020 at 20:00:01 +0530, Tanmay Jagdale wrote:
> - Add a new ACPI driver for the SbsaQemu platform which would
>   handle any modifications needed for the ACPI tables.
> 
> - Move FdtLib from LibraryClasses.common.PEIM to LibraryClasses.common
>   so that SbsaQemuAcpiDxe driver can use the device tree APIs
> 
> - Since the core count is controlled by Qemu, move the PcdCoreCount
>   from PcdsFixedAtBuild to PcdsDynamic.
> 
> - Add a parser function in this driver which parses the FDT created
>   by Qemu to determine the number of CPUs and hence update the
>   PcdCoreCount variable.
> 
> Signed-off-by: Tanmay Jagdale <tanmay.jagdale@linaro.org>
> ---
>  Platform/Qemu/SbsaQemu/SbsaQemu.dsc           |  6 +-
>  Platform/Qemu/SbsaQemu/SbsaQemu.fdf           |  1 +
>  Silicon/Qemu/SbsaQemu/Acpi.dsc.inc            |  1 +
>  .../Drivers/SbsaQemuAcpiDxe/SbsaQemuAcpiDxe.c | 76 +++++++++++++++++++
>  .../SbsaQemuAcpiDxe/SbsaQemuAcpiDxe.inf       | 49 ++++++++++++
>  Silicon/Qemu/SbsaQemu/SbsaQemu.dec            |  4 +
>  6 files changed, 135 insertions(+), 2 deletions(-)
>  create mode 100644 Silicon/Qemu/SbsaQemu/Drivers/SbsaQemuAcpiDxe/SbsaQemuAcpiDxe.c
>  create mode 100644 Silicon/Qemu/SbsaQemu/Drivers/SbsaQemuAcpiDxe/SbsaQemuAcpiDxe.inf
> 
> diff --git a/Platform/Qemu/SbsaQemu/SbsaQemu.dsc b/Platform/Qemu/SbsaQemu/SbsaQemu.dsc
> index 4739443cae93..d42b9cd4de49 100644
> --- a/Platform/Qemu/SbsaQemu/SbsaQemu.dsc
> +++ b/Platform/Qemu/SbsaQemu/SbsaQemu.dsc
> @@ -75,6 +75,7 @@ [LibraryClasses.common]

Ah, yes, I see here you *have* set up the diff format (probably using
SetupGit.py), so the issue is simply an out-of-date edk2 tree.
Do try to keep edk2 as up to date as possible - if edk2 changes causes
breakage in the platform ports, I'm not going to take any new patches
until those have been fixed anyway.

>    ShellLib|ShellPkg/Library/UefiShellLib/UefiShellLib.inf
>    FileHandleLib|MdePkg/Library/UefiFileHandleLib/UefiFileHandleLib.inf
>  
> +  FdtLib|EmbeddedPkg/Library/FdtLib/FdtLib.inf

Why are we adding FdtLib?

>    UefiRuntimeLib|MdePkg/Library/UefiRuntimeLib/UefiRuntimeLib.inf
>    OrderedCollectionLib|MdePkg/Library/BaseOrderedCollectionRedBlackTreeLib/BaseOrderedCollectionRedBlackTreeLib.inf
>  
> @@ -217,7 +218,6 @@ [LibraryClasses.common.PEIM]
>  
>    PeiServicesTablePointerLib|ArmPkg/Library/PeiServicesTablePointerLib/PeiServicesTablePointerLib.inf
>  
> -  FdtLib|EmbeddedPkg/Library/FdtLib/FdtLib.inf

Why are we removing FdtLib?

>    ArmPlatformLib|Silicon/Qemu/SbsaQemu/Library/SbsaQemuLib/SbsaQemuLib.inf
>  
>  [LibraryClasses.common.DXE_CORE]
> @@ -376,7 +376,6 @@ [PcdsFixedAtBuild.common]
>    #
>    gEfiMdeModulePkgTokenSpaceGuid.PcdSetNxForStack|TRUE
>  
> -  gArmPlatformTokenSpaceGuid.PcdCoreCount|1

Why are we removing PcdCoreCount?

>    gArmTokenSpaceGuid.PcdVFPEnabled|1
>  
>    # System Memory Base -- fixed
> @@ -477,6 +476,9 @@ [PcdsFixedAtBuild.common]
>  [PcdsDynamicDefault.common]
>    gEfiMdePkgTokenSpaceGuid.PcdPlatformBootTimeOut|3
>  
> +  # Core and Cluster Count
> +  gArmVirtSbsaQemuPlatformTokenSpaceGuid.PcdCoreCount|1

Why are we adding PcdCoreCount?

(In short - stop moving things around randomly. If you want to
reorganise a file that's fine, but that should be a separate commit
without functional changes.)

> +  gArmVirtSbsaQemuPlatformTokenSpaceGuid.PcdClusterCount|1
>  
>    # System Memory Size -- 128 MB initially, actual size will be fetched from DT
>    # TODO as no DT will be used we should pass this by some other method
> diff --git a/Platform/Qemu/SbsaQemu/SbsaQemu.fdf b/Platform/Qemu/SbsaQemu/SbsaQemu.fdf
> index 4526eaaa02c5..3bcf0bf0040a 100644
> --- a/Platform/Qemu/SbsaQemu/SbsaQemu.fdf
> +++ b/Platform/Qemu/SbsaQemu/SbsaQemu.fdf
> @@ -232,6 +232,7 @@ [FV.FvMain]
>    #
>    INF MdeModulePkg/Universal/Acpi/AcpiTableDxe/AcpiTableDxe.inf
>    INF MdeModulePkg/Universal/Acpi/AcpiPlatformDxe/AcpiPlatformDxe.inf
> +  INF Silicon/Qemu/SbsaQemu/Drivers/SbsaQemuAcpiDxe/SbsaQemuAcpiDxe.inf
>    INF RuleOverride = ACPITABLE Silicon/Qemu/SbsaQemu/AcpiTables/AcpiTables.inf
>    INF MdeModulePkg/Universal/Acpi/BootGraphicsResourceTableDxe/BootGraphicsResourceTableDxe.inf
>  
> diff --git a/Silicon/Qemu/SbsaQemu/Acpi.dsc.inc b/Silicon/Qemu/SbsaQemu/Acpi.dsc.inc
> index c4a8d7a27b78..593670383750 100644
> --- a/Silicon/Qemu/SbsaQemu/Acpi.dsc.inc
> +++ b/Silicon/Qemu/SbsaQemu/Acpi.dsc.inc
> @@ -33,3 +33,4 @@ [Components.common]
>    MdeModulePkg/Universal/Acpi/AcpiTableDxe/AcpiTableDxe.inf
>    MdeModulePkg/Universal/Acpi/AcpiPlatformDxe/AcpiPlatformDxe.inf
>    Silicon/Qemu/SbsaQemu/AcpiTables/AcpiTables.inf
> +  Silicon/Qemu/SbsaQemu/Drivers/SbsaQemuAcpiDxe/SbsaQemuAcpiDxe.inf
> diff --git a/Silicon/Qemu/SbsaQemu/Drivers/SbsaQemuAcpiDxe/SbsaQemuAcpiDxe.c b/Silicon/Qemu/SbsaQemu/Drivers/SbsaQemuAcpiDxe/SbsaQemuAcpiDxe.c
> new file mode 100644
> index 000000000000..09e5ba432a59
> --- /dev/null
> +++ b/Silicon/Qemu/SbsaQemu/Drivers/SbsaQemuAcpiDxe/SbsaQemuAcpiDxe.c
> @@ -0,0 +1,76 @@
> +/** @file
> +*  This file is an ACPI driver for the Qemu SBSA platform.
> +*
> +*  Copyright (c) 2020, Linaro Ltd. All rights reserved.
> +*
> +*  SPDX-License-Identifier: BSD-2-Clause-Patent
> +*
> +**/
> +#include <Library/DebugLib.h>
> +#include <Library/PcdLib.h>
> +#include <Library/UefiBootServicesTableLib.h>
> +#include <Library/UefiDriverEntryPoint.h>
> +#include <Library/UefiLib.h>
> +#include <Protocol/FdtClient.h>
> +#include <libfdt.h>
> +
> +/*
> + * A function that walks through the Device Tree created
> + * by Qemu and counts the number of CPUs present in it.
> + */
> +STATIC
> +VOID
> +CountCpusFromFdt (
> +  VOID
> +)
> +{
> +  VOID           *DeviceTreeBase;
> +  INT32          Node, Prev;
> +  RETURN_STATUS  PcdStatus;
> +  INT32          CpuNode;
> +  INT32          CpuCount;
> +
> +  DeviceTreeBase = (VOID *)(UINTN)PcdGet64 (PcdDeviceTreeBaseAddress);
> +  ASSERT (DeviceTreeBase != NULL);
> +
> +  // Make sure we have a valid device tree blob
> +  ASSERT (fdt_check_header (DeviceTreeBase) == 0);
> +
> +  CpuNode = fdt_path_offset (DeviceTreeBase, "/cpus");
> +  if (CpuNode <= 0) {
> +    DEBUG((EFI_D_ERROR, "Unable to locate /cpus in device tree\n"));
> +    return;
> +  }
> +
> +  CpuCount = 0;
> +
> +  // Walk through /cpus node and count the number of subnodes.
> +  // The count of these subnodes corresponds to the numer of
> +  // CPUs created by Qemu.
> +  Prev = fdt_first_subnode (DeviceTreeBase, CpuNode);
> +  while (1) {
> +    CpuCount++;
> +    Node = fdt_next_subnode (DeviceTreeBase, Prev);
> +    if (Node < 0) {
> +      break;
> +    }
> +    Prev = Node;
> +  }
> +  ASSERT (CpuCount > 0);
> +
> +  PcdStatus = PcdSet32S (PcdCoreCount, CpuCount);
> +  ASSERT_RETURN_ERROR (PcdStatus);
> +}
> +
> +EFI_STATUS
> +EFIAPI
> +InitializeSbsaQemuAcpiDxe (
> +  IN EFI_HANDLE           ImageHandle,
> +  IN EFI_SYSTEM_TABLE     *SystemTable
> +  )
> +{
> +  // Parse the device tree and get the number of CPUs
> +  CountCpusFromFdt ();
> +
> +  return EFI_SUCCESS;
> +}
> diff --git a/Silicon/Qemu/SbsaQemu/Drivers/SbsaQemuAcpiDxe/SbsaQemuAcpiDxe.inf b/Silicon/Qemu/SbsaQemu/Drivers/SbsaQemuAcpiDxe/SbsaQemuAcpiDxe.inf
> new file mode 100644
> index 000000000000..efc4d295bfb7
> --- /dev/null
> +++ b/Silicon/Qemu/SbsaQemu/Drivers/SbsaQemuAcpiDxe/SbsaQemuAcpiDxe.inf
> @@ -0,0 +1,49 @@
> +## @file
> +#  This driver modifies ACPI tables for the Qemu SBSA platform
> +#
> +#  Copyright (c) 2020, Linaro Ltd. All rights reserved.
> +#
> +#  SPDX-License-Identifier: BSD-2-Clause-Patent
> +#
> +##
> +
> +[Defines]
> +  INF_VERSION                    = 0x0001001d
> +  BASE_NAME                      = SbsaQemuAcpiDxe
> +  FILE_GUID                      = 6c592dc9-76c8-474f-93b2-bf1e8f15ae35
> +  MODULE_TYPE                    = DXE_DRIVER
> +  VERSION_STRING                 = 1.0
> +
> +  ENTRY_POINT                    = InitializeSbsaQemuAcpiDxe
> +
> +[Sources]
> +  SbsaQemuAcpiDxe.c
> +
> +[Packages]
> +  ArmPkg/ArmPkg.dec
> +  ArmPlatformPkg/ArmPlatformPkg.dec
> +  ArmVirtPkg/ArmVirtPkg.dec
> +  EmbeddedPkg/EmbeddedPkg.dec
> +  MdeModulePkg/MdeModulePkg.dec
> +  MdePkg/MdePkg.dec
> +  Silicon/Qemu/SbsaQemu/SbsaQemu.dec
> +
> +[LibraryClasses]
> +  ArmLib
> +  BaseMemoryLib
> +  DebugLib
> +  BaseLib
> +  FdtLib
> +  DxeServicesLib

Please sort Above LibraryClasses alphabetically.

/
    Leif

> +  PcdLib
> +  UefiDriverEntryPoint
> +  UefiLib
> +  UefiRuntimeServicesTableLib
> +
> +[Pcd]
> +  gArmVirtSbsaQemuPlatformTokenSpaceGuid.PcdCoreCount
> +  gArmVirtSbsaQemuPlatformTokenSpaceGuid.PcdClusterCount
> +  gArmVirtSbsaQemuPlatformTokenSpaceGuid.PcdDeviceTreeBaseAddress
> +
> +[Depex]
> +  TRUE
> diff --git a/Silicon/Qemu/SbsaQemu/SbsaQemu.dec b/Silicon/Qemu/SbsaQemu/SbsaQemu.dec
> index 71ba55a082e2..ed87d15de003 100644
> --- a/Silicon/Qemu/SbsaQemu/SbsaQemu.dec
> +++ b/Silicon/Qemu/SbsaQemu/SbsaQemu.dec
> @@ -35,3 +35,7 @@ [PcdsFixedAtBuild.common]
>    gArmVirtSbsaQemuPlatformTokenSpaceGuid.PcdPlatformEhciBase|0|UINT64|0x00000003
>    gArmVirtSbsaQemuPlatformTokenSpaceGuid.PcdPlatformEhciSize|0x10000|UINT32|0x00000004
>    gArmVirtSbsaQemuPlatformTokenSpaceGuid.PcdDeviceTreeBaseAddress|0x10000000000|UINT64|0x00000005
> +
> +[PcdsDynamic.common]
> +  gArmVirtSbsaQemuPlatformTokenSpaceGuid.PcdCoreCount|0x1|UINT32|0x00000006
> +  gArmVirtSbsaQemuPlatformTokenSpaceGuid.PcdClusterCount|0x1|UINT32|0x00000007
> -- 
> 2.28.0
>