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

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

On Tue, Mar 02, 2021 at 16:01:56 +0100, Marcin Juszkiewicz wrote:
> W dniu 02.03.2021 o 15:14, Graeme Gregory pisze:
> > 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>
> 
> Tested-by: Marcin Juszkiewicz <marcin.juszkiewicz@linaro.org>
> 
> sbsa-acs now finish in seconds like before.

Thanks all.
Pushed as a3ce6f8df2b6.

> > 
> > > ---
> > >   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;
> > >     }
> > > 
> > 
>