Re: [PATCH edk2-platforms 1/1] Silicon/Qemu: Move SbsaQemu MPIDR-retrieval function to FdtHelperLib

["Graeme Gregory" <graeme@nuviainc.com>]

On 02/03/2021 13:38, Leif Lindholm wrote:
> Commit 822634fc1bf1 ("SbsaQemu: Update SbsaQemuAcpiDxe to use FdtHelperLib")
> replaced the CountCpusFromFdt() function in SbsaQemuAcpiDxe with a call to
> FdtHelperCountCpus() in FdtHelperLib. This ended up leaving static variables
> FdtFirstCpuOffset and FdtCpuNodeSize uninitialised, such that the GetMpidr()
> function kept returning the value for cpu 0.
> 
> Resolve this by moving the GetMpidr() function over to FdtHelperLib, where
> it can again share these variables with FdtHelperCountCpus().
> 
> Fix up coding style issues as part of copy:
> - Add m prefix to module-global variables.
> - Add doxygen function comment header.
> 
> Cc: Ard Biesheuvel <ardb+tianocore@kernel.org>
> Cc: Graeme Gregory <graeme@nuviainc.com>
> Cc: Radoslaw Biernacki <rad@semihalf.com>
> Cc: Tanmay Jagdale <tanmay.jagdale@linaro.org>
> Cc: Rebecca Cran <rebecca@nuviainc.com>
> Reported-by: Marcin Juszkiewicz <marcin.juszkiewicz@linaro.org>
> Signed-off-by: Leif Lindholm <leif@nuviainc.com>

Tested-By: Graeme Gregory <graeme@nuviainc.com>

This fixes the issue from inspection of APIC table with acpiview!

> ---
>   Silicon/Qemu/SbsaQemu/Drivers/SbsaQemuAcpiDxe/SbsaQemuAcpiDxe.inf |  2 --
>   Silicon/Qemu/SbsaQemu/Library/FdtHelperLib/FdtHelperLib.inf       |  5 +++
>   Silicon/Qemu/SbsaQemu/Include/Library/FdtHelperLib.h              | 12 +++++++
>   Silicon/Qemu/SbsaQemu/Drivers/SbsaQemuAcpiDxe/SbsaQemuAcpiDxe.c   | 35 +------------------
>   Silicon/Qemu/SbsaQemu/Library/FdtHelperLib/FdtHelperLib.c         | 36 ++++++++++++++++++++
>   5 files changed, 54 insertions(+), 36 deletions(-)
> 
> diff --git a/Silicon/Qemu/SbsaQemu/Drivers/SbsaQemuAcpiDxe/SbsaQemuAcpiDxe.inf b/Silicon/Qemu/SbsaQemu/Drivers/SbsaQemuAcpiDxe/SbsaQemuAcpiDxe.inf
> index a58ebfaf76d5..c6de685bd2c4 100644
> --- a/Silicon/Qemu/SbsaQemu/Drivers/SbsaQemuAcpiDxe/SbsaQemuAcpiDxe.inf
> +++ b/Silicon/Qemu/SbsaQemu/Drivers/SbsaQemuAcpiDxe/SbsaQemuAcpiDxe.inf
> @@ -35,7 +35,6 @@ [LibraryClasses]
>     DebugLib
>     DxeServicesLib
>     FdtHelperLib
> -  FdtLib
>     PcdLib
>     PrintLib
>     UefiDriverEntryPoint
> @@ -46,7 +45,6 @@ [Pcd]
>     gEfiMdeModulePkgTokenSpaceGuid.PcdAcpiTableStorageFile
>     gArmVirtSbsaQemuPlatformTokenSpaceGuid.PcdCoreCount
>     gArmVirtSbsaQemuPlatformTokenSpaceGuid.PcdClusterCount
> -  gArmVirtSbsaQemuPlatformTokenSpaceGuid.PcdDeviceTreeBaseAddress
>   
>   [Depex]
>     gEfiAcpiTableProtocolGuid                       ## CONSUMES
> diff --git a/Silicon/Qemu/SbsaQemu/Library/FdtHelperLib/FdtHelperLib.inf b/Silicon/Qemu/SbsaQemu/Library/FdtHelperLib/FdtHelperLib.inf
> index d84c16f888d1..9c059f3e5851 100644
> --- a/Silicon/Qemu/SbsaQemu/Library/FdtHelperLib/FdtHelperLib.inf
> +++ b/Silicon/Qemu/SbsaQemu/Library/FdtHelperLib/FdtHelperLib.inf
> @@ -24,5 +24,10 @@ [Packages]
>     MdePkg/MdePkg.dec
>     Silicon/Qemu/SbsaQemu/SbsaQemu.dec
>   
> +[LibraryClasses]
> +  DebugLib
> +  FdtLib
> +  PcdLib
> +
>   [FixedPcd]
>     gArmVirtSbsaQemuPlatformTokenSpaceGuid.PcdDeviceTreeBaseAddress
> diff --git a/Silicon/Qemu/SbsaQemu/Include/Library/FdtHelperLib.h b/Silicon/Qemu/SbsaQemu/Include/Library/FdtHelperLib.h
> index f9045fd5df45..ea9159857215 100644
> --- a/Silicon/Qemu/SbsaQemu/Include/Library/FdtHelperLib.h
> +++ b/Silicon/Qemu/SbsaQemu/Include/Library/FdtHelperLib.h
> @@ -10,6 +10,18 @@
>   #ifndef FDT_HELPER_LIB_
>   #define FDT_HELPER_LIB_
>   
> +/**
> +  Get MPIDR for a given cpu from device tree passed by Qemu.
> +
> +  @param [in]   CpuId    Index of cpu to retrieve MPIDR value for.
> +
> +  @retval                MPIDR value of CPU at index <CpuId>
> +**/
> +UINT64
> +FdtHelperGetMpidr (
> +  IN UINTN   CpuId
> +  );
> +
>   /** Walks through the Device Tree created by Qemu and counts the number
>       of CPUs present in it.
>   
> diff --git a/Silicon/Qemu/SbsaQemu/Drivers/SbsaQemuAcpiDxe/SbsaQemuAcpiDxe.c b/Silicon/Qemu/SbsaQemu/Drivers/SbsaQemuAcpiDxe/SbsaQemuAcpiDxe.c
> index 84120f1c1b51..b8901030ecd0 100644
> --- a/Silicon/Qemu/SbsaQemu/Drivers/SbsaQemuAcpiDxe/SbsaQemuAcpiDxe.c
> +++ b/Silicon/Qemu/SbsaQemu/Drivers/SbsaQemuAcpiDxe/SbsaQemuAcpiDxe.c
> @@ -20,39 +20,6 @@
>   #include <Library/UefiDriverEntryPoint.h>
>   #include <Library/UefiLib.h>
>   #include <Protocol/AcpiTable.h>
> -#include <Protocol/FdtClient.h>
> -#include <libfdt.h>
> -
> -STATIC INT32 FdtFirstCpuOffset;
> -STATIC INT32 FdtCpuNodeSize;
> -
> -/*
> - * Get MPIDR from device tree passed by Qemu
> - */
> -STATIC
> -UINT64
> -GetMpidr (
> -  IN UINTN   CpuId
> -  )
> -{
> -  VOID           *DeviceTreeBase;
> -  CONST UINT64   *RegVal;
> -  INT32          Len;
> -
> -  DeviceTreeBase = (VOID *)(UINTN)PcdGet64 (PcdDeviceTreeBaseAddress);
> -  ASSERT (DeviceTreeBase != NULL);
> -
> -  RegVal = fdt_getprop (DeviceTreeBase,
> -             FdtFirstCpuOffset + (CpuId * FdtCpuNodeSize),
> -             "reg",
> -             &Len);
> -  if (!RegVal) {
> -    DEBUG ((DEBUG_ERROR, "Couldn't find reg property for CPU:%d\n", CpuId));
> -    return 0;
> -  }
> -
> -  return (fdt64_to_cpu (ReadUnaligned64 (RegVal)));
> -}
>   
>   /*
>    * A Function to Compute the ACPI Table Checksum
> @@ -159,7 +126,7 @@ AddMadtTable (
>       CopyMem (New, &Gicc, sizeof (EFI_ACPI_6_0_GIC_STRUCTURE));
>       GiccPtr = (EFI_ACPI_6_0_GIC_STRUCTURE *) New;
>       GiccPtr->AcpiProcessorUid = CoreIndex;
> -    GiccPtr->MPIDR = GetMpidr (CoreIndex);
> +    GiccPtr->MPIDR = FdtHelperGetMpidr (CoreIndex);
>       New += sizeof (EFI_ACPI_6_0_GIC_STRUCTURE);
>     }
>   
> diff --git a/Silicon/Qemu/SbsaQemu/Library/FdtHelperLib/FdtHelperLib.c b/Silicon/Qemu/SbsaQemu/Library/FdtHelperLib/FdtHelperLib.c
> index bbd756f01a21..7fdfb055db76 100644
> --- a/Silicon/Qemu/SbsaQemu/Library/FdtHelperLib/FdtHelperLib.c
> +++ b/Silicon/Qemu/SbsaQemu/Library/FdtHelperLib/FdtHelperLib.c
> @@ -14,6 +14,40 @@
>   #include <Library/PcdLib.h>
>   #include <libfdt.h>
>   
> +STATIC INT32 mFdtFirstCpuOffset;
> +STATIC INT32 mFdtCpuNodeSize;
> +
> +/**
> +  Get MPIDR for a given cpu from device tree passed by Qemu.
> +
> +  @param [in]   CpuId    Index of cpu to retrieve MPIDR value for.
> +
> +  @retval                MPIDR value of CPU at index <CpuId>
> +**/
> +UINT64
> +FdtHelperGetMpidr (
> +  IN UINTN   CpuId
> +  )
> +{
> +  VOID           *DeviceTreeBase;
> +  CONST UINT64   *RegVal;
> +  INT32          Len;
> +
> +  DeviceTreeBase = (VOID *)(UINTN)PcdGet64 (PcdDeviceTreeBaseAddress);
> +  ASSERT (DeviceTreeBase != NULL);
> +
> +  RegVal = fdt_getprop (DeviceTreeBase,
> +             mFdtFirstCpuOffset + (CpuId * mFdtCpuNodeSize),
> +             "reg",
> +             &Len);
> +  if (!RegVal) {
> +    DEBUG ((DEBUG_ERROR, "Couldn't find reg property for CPU:%d\n", CpuId));
> +    return 0;
> +  }
> +
> +  return (fdt64_to_cpu (ReadUnaligned64 (RegVal)));
> +}
> +
>   /** Walks through the Device Tree created by Qemu and counts the number
>       of CPUs present in it.
>   
> @@ -49,12 +83,14 @@ FdtHelperCountCpus (
>     // 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;
>     }
>   
>