On Tue, Aug 04, 2020 at 16:02:52 +0530, Tanmay Jagdale wrote:
Add a new ACPI driver for the SbsaQemu platform which would handle any modifications needed for the static ACPI tables.
Added 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 | 9 ++- Platform/Qemu/SbsaQemu/SbsaQemu.fdf | 7 +- Silicon/Qemu/SbsaQemu/Acpi.dsc.inc | 10 +-- .../Qemu/SbsaQemu/AcpiTables/AcpiTables.inf | 2 - Silicon/Qemu/SbsaQemu/AcpiTables/Madt.aslc | 4 +- .../Drivers/SbsaQemuAcpiDxe/SbsaQemuAcpiDxe.c | 72 +++++++++++++++++++ .../SbsaQemuAcpiDxe/SbsaQemuAcpiDxe.inf | 49 +++++++++++++ .../SbsaQemuPlatformDxe/SbsaQemuPlatformDxe.c | 6 -- .../SbsaQemuPlatformDxe.inf | 7 -- Silicon/Qemu/SbsaQemu/SbsaQemu.dec | 4 ++ 10 files changed, 134 insertions(+), 36 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 ac1398af8f..d42b9cd4de 100644 --- a/Platform/Qemu/SbsaQemu/SbsaQemu.dsc +++ b/Platform/Qemu/SbsaQemu/SbsaQemu.dsc @@ -75,6 +75,7 @@ DEFINE NETWORK_HTTP_BOOT_ENABLE = FALSE ShellLib|ShellPkg/Library/UefiShellLib/UefiShellLib.inf FileHandleLib|MdePkg/Library/UefiFileHandleLib/UefiFileHandleLib.inf
- FdtLib|EmbeddedPkg/Library/FdtLib/FdtLib.inf
Oh, we're adding use of fdtlib? What have we changed that has added this dependency?
UefiRuntimeLib|MdePkg/Library/UefiRuntimeLib/UefiRuntimeLib.inf OrderedCollectionLib|MdePkg/Library/BaseOrderedCollectionRedBlackTreeLib/BaseOrderedCollectionRedBlackTreeLib.inf @@ -217,7 +218,6 @@ DEFINE NETWORK_HTTP_BOOT_ENABLE = FALSE PeiServicesTablePointerLib|ArmPkg/Library/PeiServicesTablePointerLib/PeiServicesTablePointerLib.inf
- FdtLib|EmbeddedPkg/Library/FdtLib/FdtLib.inf
Oh, nothing. Then please don't randomly move it around.
ArmPlatformLib|Silicon/Qemu/SbsaQemu/Library/SbsaQemuLib/SbsaQemuLib.inf [LibraryClasses.common.DXE_CORE] @@ -300,10 +300,6 @@ DEFINE NETWORK_HTTP_BOOT_ENABLE = FALSE gEfiMdePkgTokenSpaceGuid.PcdPostCodePropertyMask|0 gEfiMdePkgTokenSpaceGuid.PcdUefiLibMaxPrintBufferSize|320
- # Core and Cluster Count
- gArmPlatformTokenSpaceGuid.PcdCoreCount|4
- gArmPlatformTokenSpaceGuid.PcdClusterCount|1
- # DEBUG_ASSERT_ENABLED 0x01 # DEBUG_PRINT_ENABLED 0x02 # DEBUG_CODE_ENABLED 0x04
@@ -480,6 +476,9 @@ DEFINE NETWORK_HTTP_BOOT_ENABLE = FALSE [PcdsDynamicDefault.common] gEfiMdePkgTokenSpaceGuid.PcdPlatformBootTimeOut|3
- # Core and Cluster Count
- gArmVirtSbsaQemuPlatformTokenSpaceGuid.PcdCoreCount|1
- gArmVirtSbsaQemuPlatformTokenSpaceGuid.PcdClusterCount|1
Once you've run SetupGit.py in this repo, the diff would actually give me context for which section this was moved from (and to, if that hadn't happened to be in the context lines anyway).
But regardless, I would be wondering why this is being moved around instead of simply changed. And a description in the commit message of PcdCoreCount being changed to 1 because <fill in blank> would be useful.
# 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 7f1a60e3ee..3bcf0bf004 100644 --- a/Platform/Qemu/SbsaQemu/SbsaQemu.fdf +++ b/Platform/Qemu/SbsaQemu/SbsaQemu.fdf @@ -232,6 +232,7 @@ READ_LOCK_STATUS = TRUE # 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
@@ -309,9 +310,3 @@ READ_LOCK_STATUS = TRUE } !include ArmVirtPkg/ArmVirtRules.fdf.inc
-[Rule.Common.USER_DEFINED.ACPITABLE]
- FILE FREEFORM = $(NAMED_GUID) {
- RAW ACPI |.acpi
- RAW ASL |.aml
- }
Didn't we just add this one?
diff --git a/Silicon/Qemu/SbsaQemu/Acpi.dsc.inc b/Silicon/Qemu/SbsaQemu/Acpi.dsc.inc index 76d0fac079..c3e910763c 100644 --- a/Silicon/Qemu/SbsaQemu/Acpi.dsc.inc +++ b/Silicon/Qemu/SbsaQemu/Acpi.dsc.inc @@ -30,13 +30,7 @@ # # ACPI support #
- MdeModulePkg/Universal/Acpi/AcpiTableDxe/AcpiTableDxe.inf {
<LibraryClasses>
NULL|EmbeddedPkg/Library/PlatformHasAcpiLib/PlatformHasAcpiLib.inf
<PcdsFixedAtBuild>
# support ACPI v5.0 or later
gEfiMdeModulePkgTokenSpaceGuid.PcdAcpiExposedTableVersions|0x20
- }
- 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/AcpiTables/AcpiTables.inf b/Silicon/Qemu/SbsaQemu/AcpiTables/AcpiTables.inf index 63d5754e16..9f8ced9db0 100644 --- a/Silicon/Qemu/SbsaQemu/AcpiTables/AcpiTables.inf +++ b/Silicon/Qemu/SbsaQemu/AcpiTables/AcpiTables.inf @@ -33,8 +33,6 @@ MdeModulePkg/MdeModulePkg.dec [FixedPcd]
- gArmPlatformTokenSpaceGuid.PcdClusterCount
- gArmPlatformTokenSpaceGuid.PcdCoreCount gArmTokenSpaceGuid.PcdGicDistributorBase gArmTokenSpaceGuid.PcdGicRedistributorsBase
diff --git a/Silicon/Qemu/SbsaQemu/AcpiTables/Madt.aslc b/Silicon/Qemu/SbsaQemu/AcpiTables/Madt.aslc index 7bec2d4ef3..9e39e2e139 100644 --- a/Silicon/Qemu/SbsaQemu/AcpiTables/Madt.aslc +++ b/Silicon/Qemu/SbsaQemu/AcpiTables/Madt.aslc @@ -15,8 +15,8 @@ #include "AcpiTables.h" -#define CORES (FixedPcdGet32 (PcdClusterCount) * \
FixedPcdGet32 (PcdCoreCount))
+#define CORES 4
Why are we changing this to the a hardcoded value equal to what PcdClusterCount * PcdCoreCount was before we (in this patch) changed PcdCoreCount to 1?
// // Multiple APIC Description Table // diff --git a/Silicon/Qemu/SbsaQemu/Drivers/SbsaQemuAcpiDxe/SbsaQemuAcpiDxe.c b/Silicon/Qemu/SbsaQemu/Drivers/SbsaQemuAcpiDxe/SbsaQemuAcpiDxe.c new file mode 100644 index 0000000000..87f2c1aaec --- /dev/null +++ b/Silicon/Qemu/SbsaQemu/Drivers/SbsaQemuAcpiDxe/SbsaQemuAcpiDxe.c @@ -0,0 +1,72 @@ +/** @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.
- */
+VOID +SbsaQemuCountCpusFromFdt(
This function appears to be only ever called from within this file. So it could be STATIC, and doesn't need the SbsaQemu prefix.
- VOID
+) +{
- VOID *DeviceTreeBase;
- INT32 Node, Prev;
- RETURN_STATUS PcdStatus;
- INT32 CpuNode;
- INT32 CpuCount;
- DeviceTreeBase = (VOID *)(UINTN)PcdGet64 (PcdDeviceTreeBaseAddress);
- ASSERT (DeviceTreeBase != NULL);
Hmm ... I realise this was in the original contribution of sbsa-qemu, but really, we could move PcdDeviceTreeBaseAddress to a #define. This isn't something we'll be moving around.
- // 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.
- for (Prev = fdt_first_subnode(DeviceTreeBase, CpuNode);; Prev = Node) {
Space before (DeviceTreeBase But this is quite messy as a for-loop, so...
- CpuCount++;
- Node = fdt_next_subnode (DeviceTreeBase, Prev);
- if (Node < 0) {
break;
- }
- }
... I would rewrite this as
Prev = fdt_first_subnode(DeviceTreeBase, CpuNode); while (Prev >= 0) { CpuCount++; Node = fdt_next_subnode (DeviceTreeBase, Prev); if (Node < 0) { break; } Prev = Node; }
And probably have another ASSERT on CpuCount > 0.
(not even compile tested, sanity checking required)
/ Leif
- 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
- SbsaQemuCountCpusFromFdt();
- 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 0000000000..efc4d295bf --- /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
- PcdLib
- UefiDriverEntryPoint
- UefiLib
- UefiRuntimeServicesTableLib
+[Pcd]
- gArmVirtSbsaQemuPlatformTokenSpaceGuid.PcdCoreCount
- gArmVirtSbsaQemuPlatformTokenSpaceGuid.PcdClusterCount
- gArmVirtSbsaQemuPlatformTokenSpaceGuid.PcdDeviceTreeBaseAddress
+[Depex]
- TRUE
diff --git a/Silicon/Qemu/SbsaQemu/Drivers/SbsaQemuPlatformDxe/SbsaQemuPlatformDxe.c b/Silicon/Qemu/SbsaQemu/Drivers/SbsaQemuPlatformDxe/SbsaQemuPlatformDxe.c index f55dbdb453..b7270a07ab 100644 --- a/Silicon/Qemu/SbsaQemu/Drivers/SbsaQemuPlatformDxe/SbsaQemuPlatformDxe.c +++ b/Silicon/Qemu/SbsaQemu/Drivers/SbsaQemuPlatformDxe/SbsaQemuPlatformDxe.c @@ -51,11 +51,5 @@ InitializeSbsaQemuPlatformDxe ( return Status; }
- Status = gBS->InstallMultipleProtocolInterfaces (&ImageHandle,
&gEdkiiPlatformHasAcpiGuid, NULL, NULL);
- if (EFI_ERROR (Status)) {
- DEBUG ((DEBUG_ERROR, "FAILED TO INSTALL gEDKIIPlatformHasAcpiGuid protocol\n"));
- }
- return EFI_SUCCESS;
} diff --git a/Silicon/Qemu/SbsaQemu/Drivers/SbsaQemuPlatformDxe/SbsaQemuPlatformDxe.inf b/Silicon/Qemu/SbsaQemu/Drivers/SbsaQemuPlatformDxe/SbsaQemuPlatformDxe.inf index a50d563782..6c9c28946e 100644 --- a/Silicon/Qemu/SbsaQemu/Drivers/SbsaQemuPlatformDxe/SbsaQemuPlatformDxe.inf +++ b/Silicon/Qemu/SbsaQemu/Drivers/SbsaQemuPlatformDxe/SbsaQemuPlatformDxe.inf @@ -39,13 +39,6 @@ [Depex] TRUE -[Guids]
- gEdkiiPlatformHasAcpiGuid
[Protocols] gEdkiiNonDiscoverableDeviceProtocolGuid ## PRODUCES
- gEfiAcpiTableProtocolGuid ## CONSUMES gEfiPciIoProtocolGuid ## CONSUMES
-[FixedPcd]
- gEfiMdeModulePkgTokenSpaceGuid.PcdAcpiDefaultOemRevision
diff --git a/Silicon/Qemu/SbsaQemu/SbsaQemu.dec b/Silicon/Qemu/SbsaQemu/SbsaQemu.dec index cd879f4dbd..6aca632996 100644 --- a/Silicon/Qemu/SbsaQemu/SbsaQemu.dec +++ b/Silicon/Qemu/SbsaQemu/SbsaQemu.dec @@ -35,3 +35,7 @@ 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