[Asa-dev] Re: [PATCH edk2-platforms 1/1] SbsaQemu: get CoreCount from TF-A

On Wed, 22 Nov 2023 at 11:20, Marcin Juszkiewicz marcin.juszkiewicz@linaro.org wrote:

As part of removing DeviceTree use from EDK2 we moved counting of cpu cores to TF-A. Then SMC call gets value on platform initialization.

Reading MPIDR value for MADT table is broken - OS gets just one cpu. This gets sorted in separate patch.

Signed-off-by: Marcin Juszkiewicz marcin.juszkiewicz@linaro.org

.../Qemu/SbsaQemu/OemMiscLib/OemMiscLib.inf | 1 + .../SbsaQemuPlatformDxe.inf | 1 + .../Include/IndustryStandard/SbsaQemuSmc.h | 1 + .../Qemu/SbsaQemu/OemMiscLib/OemMiscLib.c | 6 +-- .../Drivers/SbsaQemuAcpiDxe/SbsaQemuAcpiDxe.c | 8 +-- .../SbsaQemuPlatformDxe/SbsaQemuPlatformDxe.c | 10 ++++ .../Library/FdtHelperLib/FdtHelperLib.c | 49 -------------------

How are you ensuring that the PCD is set before it is read?

Generally, dynamic PCDs are not really suitable for this kind of thing. If there is a collection of data items that you receive from the secure firmware, it would be better to model this as a protocol, so that other drivers can DEPEX on it (i.e., the drivers that need this information will not be dispatched before the driver that exposes it)

7 files changed, 17 insertions(+), 59 deletions(-)

diff --git a/Platform/Qemu/SbsaQemu/OemMiscLib/OemMiscLib.inf b/Platform/Qemu/SbsaQemu/OemMiscLib/OemMiscLib.inf index a34f54d431d4..11277e226cdf 100644 --- a/Platform/Qemu/SbsaQemu/OemMiscLib/OemMiscLib.inf +++ b/Platform/Qemu/SbsaQemu/OemMiscLib/OemMiscLib.inf @@ -56,3 +56,4 @@ [Pcd] gArmVirtSbsaQemuPlatformTokenSpaceGuid.PcdChassisManufacturer gArmVirtSbsaQemuPlatformTokenSpaceGuid.PcdChassisAssetTag gArmVirtSbsaQemuPlatformTokenSpaceGuid.PcdChassisSKU

• gArmVirtSbsaQemuPlatformTokenSpaceGuid.PcdCoreCount

diff --git a/Silicon/Qemu/SbsaQemu/Drivers/SbsaQemuPlatformDxe/SbsaQemuPlatformDxe.inf b/Silicon/Qemu/SbsaQemu/Drivers/SbsaQemuPlatformDxe/SbsaQemuPlatformDxe.inf index 19534b7a274a..a0563f574b76 100644 --- a/Silicon/Qemu/SbsaQemu/Drivers/SbsaQemuPlatformDxe/SbsaQemuPlatformDxe.inf +++ b/Silicon/Qemu/SbsaQemu/Drivers/SbsaQemuPlatformDxe/SbsaQemuPlatformDxe.inf @@ -46,6 +46,7 @@ [Pcd] gArmTokenSpaceGuid.PcdGicDistributorBase gArmTokenSpaceGuid.PcdGicRedistributorsBase gArmVirtSbsaQemuPlatformTokenSpaceGuid.PcdGicItsBase

• gArmVirtSbsaQemuPlatformTokenSpaceGuid.PcdCoreCount

[Depex] diff --git a/Silicon/Qemu/SbsaQemu/Include/IndustryStandard/SbsaQemuSmc.h b/Silicon/Qemu/SbsaQemu/Include/IndustryStandard/SbsaQemuSmc.h index 7934875e4aba..3138de327367 100644 --- a/Silicon/Qemu/SbsaQemu/Include/IndustryStandard/SbsaQemuSmc.h +++ b/Silicon/Qemu/SbsaQemu/Include/IndustryStandard/SbsaQemuSmc.h @@ -14,5 +14,6 @@ #define SIP_SVC_VERSION SMC_SIP_FUNCTION_ID(1) #define SIP_SVC_GET_GIC SMC_SIP_FUNCTION_ID(100) #define SIP_SVC_GET_GIC_ITS SMC_SIP_FUNCTION_ID(101) +#define SIP_SVC_GET_CPU_COUNT SMC_SIP_FUNCTION_ID(200)

#endif /* SBSA_QEMU_SMC_H_ */ diff --git a/Platform/Qemu/SbsaQemu/OemMiscLib/OemMiscLib.c b/Platform/Qemu/SbsaQemu/OemMiscLib/OemMiscLib.c index c38f2851904f..56aad6d02388 100644 --- a/Platform/Qemu/SbsaQemu/OemMiscLib/OemMiscLib.c +++ b/Platform/Qemu/SbsaQemu/OemMiscLib/OemMiscLib.c @@ -33,7 +33,7 @@ OemIsProcessorPresent ( UINTN ProcessorIndex ) {

• if (ProcessorIndex < FdtHelperCountCpus ()) {

• if (ProcessorIndex < PcdGet32 (PcdCoreCount)) { return TRUE; }

@@ -76,7 +76,7 @@ OemGetProcessorInformation ( { UINT16 ProcessorCount;

• ProcessorCount = FdtHelperCountCpus ();

• ProcessorCount = PcdGet32 (PcdCoreCount);

  if (ProcessorIndex < ProcessorCount) { ProcessorStatus->Bits.CpuStatus = 1; // CPU enabled

@@ -121,7 +121,7 @@ OemGetMaxProcessors ( VOID ) {

• return FdtHelperCountCpus ();

• return PcdGet32 (PcdCoreCount);

}

/** Gets information about the cache at the specified cache level. diff --git a/Silicon/Qemu/SbsaQemu/Drivers/SbsaQemuAcpiDxe/SbsaQemuAcpiDxe.c b/Silicon/Qemu/SbsaQemu/Drivers/SbsaQemuAcpiDxe/SbsaQemuAcpiDxe.c index 9fb17151d7b8..b59e999b23ce 100644 --- a/Silicon/Qemu/SbsaQemu/Drivers/SbsaQemuAcpiDxe/SbsaQemuAcpiDxe.c +++ b/Silicon/Qemu/SbsaQemu/Drivers/SbsaQemuAcpiDxe/SbsaQemuAcpiDxe.c @@ -255,7 +255,7 @@ AddMadtTable ( // Initialize GIC Redistributor Structure EFI_ACPI_6_0_GICR_STRUCTURE Gicr = SBSAQEMU_MADT_GICR_INIT();

• // Get CoreCount which was determined eariler after parsing device tree

• // Get CoreCount which was determined earlier from TF-A NumCores = PcdGet32 (PcdCoreCount);

  // Calculate the new table size based on the number of cores

@@ -758,12 +758,6 @@ InitializeSbsaQemuAcpiDxe ( { EFI_STATUS Status; EFI_ACPI_TABLE_PROTOCOL *AcpiTable;

• UINT32 NumCores;

• // Parse the device tree and get the number of CPUs

• NumCores = FdtHelperCountCpus ();

• Status = PcdSet32S (PcdCoreCount, NumCores);

• ASSERT_RETURN_ERROR (Status);

  // Check if ACPI Table Protocol has been installed Status = gBS->LocateProtocol (

diff --git a/Silicon/Qemu/SbsaQemu/Drivers/SbsaQemuPlatformDxe/SbsaQemuPlatformDxe.c b/Silicon/Qemu/SbsaQemu/Drivers/SbsaQemuPlatformDxe/SbsaQemuPlatformDxe.c index 4ebbe7c93a19..cc2ac346e197 100644 --- a/Silicon/Qemu/SbsaQemu/Drivers/SbsaQemuPlatformDxe/SbsaQemuPlatformDxe.c +++ b/Silicon/Qemu/SbsaQemu/Drivers/SbsaQemuPlatformDxe/SbsaQemuPlatformDxe.c @@ -123,5 +123,15 @@ InitializeSbsaQemuPlatformDxe ( } }

• SmcResult = ArmCallSmc0 (SIP_SVC_GET_CPU_COUNT, &Arg0, NULL, NULL);
• if (SmcResult == SMC_ARCH_CALL_SUCCESS) {
• Result = PcdSet32S (PcdCoreCount, Arg0);
• ASSERT_RETURN_ERROR (Result);
• }
• Arg0 = PcdGet32 (PcdCoreCount);
• DEBUG ((DEBUG_INFO, "We have %d cpus.\n", Arg0));
• return EFI_SUCCESS;

} diff --git a/Silicon/Qemu/SbsaQemu/Library/FdtHelperLib/FdtHelperLib.c b/Silicon/Qemu/SbsaQemu/Library/FdtHelperLib/FdtHelperLib.c index 7fdfb055db76..822605a940ca 100644 --- a/Silicon/Qemu/SbsaQemu/Library/FdtHelperLib/FdtHelperLib.c +++ b/Silicon/Qemu/SbsaQemu/Library/FdtHelperLib/FdtHelperLib.c @@ -47,52 +47,3 @@ FdtHelperGetMpidr (

return (fdt64_to_cpu (ReadUnaligned64 (RegVal))); }

-/** Walks through the Device Tree created by Qemu and counts the number

• of CPUs present in it.
• @return The number of CPUs present.

-**/ -EFIAPI -UINT32 -FdtHelperCountCpus (

• VOID
• )

-{

• VOID *DeviceTreeBase;
• INT32 Node;
• INT32 Prev;
• INT32 CpuNode;
• UINT32 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 ((DEBUG_ERROR, "Unable to locate /cpus in device tree\n"));
• return 0;
• }
• CpuCount = 0;
• // Walk through /cpus node and count the number of subnodes.
• // The count of these subnodes corresponds to the number of
• // CPUs created by Qemu.
• Prev = fdt_first_subnode (DeviceTreeBase, CpuNode);
• mFdtFirstCpuOffset = Prev;
• while (1) {
• CpuCount++;
• Node = fdt_next_subnode (DeviceTreeBase, Prev);
• if (Node < 0) {

•  break;
  

• }
• mFdtCpuNodeSize = Node - Prev;
• Prev = Node;
• }
• return CpuCount;

-}

2.42.0