Re: [edk2-devel] [PATCH edk2-platforms v9 1/4] Platform/SbsaQemu: add SbsaQemuHardwareInfoLib

["Leif Lindholm" <quic_llindhol@quicinc.com>]

On Fri, Mar 22, 2024 at 17:08:47 +0100, Marcin Juszkiewicz wrote:
> This library provides functions to check for hardware information.
> For now it covers CPU ones:
> 
> - amount of cpu cores
> - MPIDR value for cpu core
> - NUMA node id for cpu core
> 
> Values are read from TF-A using platform specific SMC calls.
> 
> Signed-off-by: Marcin Juszkiewicz <marcin.juszkiewicz@linaro.org>

All comments below are of the variety: "should we namespace separate this"?

> ---
>  Silicon/Qemu/SbsaQemu/SbsaQemu.dec                   |  2 +
>  Platform/Qemu/SbsaQemu/SbsaQemu.dsc                  |  3 +-
>  .../SbsaQemuHardwareInfoLib.inf                      | 29 ++++++
>  .../SbsaQemu/Include/IndustryStandard/SbsaQemuSmc.h  | 17 +++-
>  .../Qemu/SbsaQemu/Include/Library/HardwareInfoLib.h  | 45 +++++++++
>  .../SbsaQemuHardwareInfoLib.c                        | 96 ++++++++++++++++++++
>  6 files changed, 187 insertions(+), 5 deletions(-)
> 
> diff --git a/Silicon/Qemu/SbsaQemu/SbsaQemu.dec b/Silicon/Qemu/SbsaQemu/SbsaQemu.dec
> index 913d1d75ef29..427ff8b31aac 100644
> --- a/Silicon/Qemu/SbsaQemu/SbsaQemu.dec
> +++ b/Silicon/Qemu/SbsaQemu/SbsaQemu.dec
> @@ -12,6 +12,8 @@ [Defines]
>    PACKAGE_GUID                   = 8db32c5a-2821-43e2-b4ac-bc148e2b0b05
>    PACKAGE_VERSION                = 0.1
>  
> +[LibraryClasses]
> +HardwareInfoLib|Include/Library/HardwareInfoLib.h

We don't *have* to address this now, but it would be worth considering
what function this could have as a core, rather than
platform-specific, library.

What hardware? SoC or whole platform?

As it stands, the name is very generic.
I think we'll eventually want standardised APIs for these kinds of
queries. But maybe until then it would be easier to prepend SbsaQemu
to both filename and LibraryClass (like you do in the actual implementation)?

>  ################################################################################
>  #
>  # Include Section - list of Include Paths that are provided by this package.
> diff --git a/Platform/Qemu/SbsaQemu/SbsaQemu.dsc b/Platform/Qemu/SbsaQemu/SbsaQemu.dsc
> index 378600050df9..3c3d2449bff4 100644
> --- a/Platform/Qemu/SbsaQemu/SbsaQemu.dsc
> +++ b/Platform/Qemu/SbsaQemu/SbsaQemu.dsc
> @@ -1,6 +1,6 @@
>  #
>  #  Copyright (c) 2021, NUVIA Inc. All rights reserved.
> -#  Copyright (c) 2019, Linaro Limited. All rights reserved.
> +#  Copyright (c) 2019-2024, Linaro Ltd. All rights reserved.
>  #
>  #  SPDX-License-Identifier: BSD-2-Clause-Patent
>  #
> @@ -128,6 +128,7 @@ [LibraryClasses.common]
>  
>    FdtHelperLib|Silicon/Qemu/SbsaQemu/Library/FdtHelperLib/FdtHelperLib.inf
>    OemMiscLib|Platform/Qemu/SbsaQemu/OemMiscLib/OemMiscLib.inf
> +  HardwareInfoLib|Silicon/Qemu/SbsaQemu/Library/SbsaQemuHardwareInfoLib/SbsaQemuHardwareInfoLib.inf

Like you do here.

>  
>    # Debug Support
>    PeCoffExtraActionLib|ArmPkg/Library/DebugPeCoffExtraActionLib/DebugPeCoffExtraActionLib.inf
> diff --git a/Silicon/Qemu/SbsaQemu/Library/SbsaQemuHardwareInfoLib/SbsaQemuHardwareInfoLib.inf b/Silicon/Qemu/SbsaQemu/Library/SbsaQemuHardwareInfoLib/SbsaQemuHardwareInfoLib.inf
> new file mode 100644
> index 000000000000..2acb2a1e7c76
> --- /dev/null
> +++ b/Silicon/Qemu/SbsaQemu/Library/SbsaQemuHardwareInfoLib/SbsaQemuHardwareInfoLib.inf
> @@ -0,0 +1,29 @@
> +#/* @file
> +#
> +#  Copyright (c) 2024, Linaro Ltd. All rights reserved.
> +#
> +#  SPDX-License-Identifier: BSD-2-Clause-Patent
> +#
> +#*/
> +
> +[Defines]
> +  INF_VERSION                    = 0x0001001c
> +  BASE_NAME                      = SbsaQemuHardwareInfoLib
> +  FILE_GUID                      = 6454006f-6502-46e2-9be4-4bba8d4b29fb
> +  MODULE_TYPE                    = BASE
> +  VERSION_STRING                 = 1.0
> +  LIBRARY_CLASS                  = HardwareInfoLib
> +
> +[Sources]
> +  SbsaQemuHardwareInfoLib.c
> +
> +[Packages]
> +  ArmPkg/ArmPkg.dec
> +  EmbeddedPkg/EmbeddedPkg.dec
> +  MdePkg/MdePkg.dec
> +  MdeModulePkg/MdeModulePkg.dec
> +  Silicon/Qemu/SbsaQemu/SbsaQemu.dec
> +
> +[LibraryClasses]
> +  ArmSmcLib
> +  ResetSystemLib
> diff --git a/Silicon/Qemu/SbsaQemu/Include/IndustryStandard/SbsaQemuSmc.h b/Silicon/Qemu/SbsaQemu/Include/IndustryStandard/SbsaQemuSmc.h
> index 7934875e4aba..2317c1f0ae69 100644
> --- a/Silicon/Qemu/SbsaQemu/Include/IndustryStandard/SbsaQemuSmc.h
> +++ b/Silicon/Qemu/SbsaQemu/Include/IndustryStandard/SbsaQemuSmc.h
> @@ -1,6 +1,6 @@
>  /** @file
>  *
> -*  Copyright (c) 2023, Linaro Ltd. All rights reserved.<BR>
> +*  Copyright (c) 2023-2024, Linaro Ltd. All rights reserved.<BR>
>  *
>  *  SPDX-License-Identifier: BSD-2-Clause-Patent
>  *
> @@ -11,8 +11,17 @@
>  
>  #include <IndustryStandard/ArmStdSmc.h>
>  
> -#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_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)
> +#define SIP_SVC_GET_CPU_NODE   SMC_SIP_FUNCTION_ID(201)
> +
> +/*
> + *  SMCC does not define return codes for SiP functions.
> + *  We use Architecture ones then.
> + */
> +
> +#define SMC_SIP_CALL_SUCCESS  SMC_ARCH_CALL_SUCCESS
>  
>  #endif /* SBSA_QEMU_SMC_H_ */
> diff --git a/Silicon/Qemu/SbsaQemu/Include/Library/HardwareInfoLib.h b/Silicon/Qemu/SbsaQemu/Include/Library/HardwareInfoLib.h
> new file mode 100644
> index 000000000000..9c7281f123d2
> --- /dev/null
> +++ b/Silicon/Qemu/SbsaQemu/Include/Library/HardwareInfoLib.h
> @@ -0,0 +1,45 @@
> +/** @file
> +*
> +*  Copyright (c) 2024, Linaro Ltd. All rights reserved.
> +*
> +*  SPDX-License-Identifier: BSD-2-Clause-Patent
> +*
> +**/
> +
> +#ifndef HARDWARE_INFO_LIB
> +#define HARDWARE_INFO_LIB

Separate namespace?

> +
> +/**
> +  Get CPU count from information passed by Qemu.
> +
> +**/
> +UINT32
> +GetCpuCount (

Here also, "GetCpuCount", "SbsaQemuGetCpuCount" is tedious, but very
clear. (Same for below functions, at least until/if we create this as
a core interface.)

/
    Leif

> +  VOID
> +  );
> +
> +/**
> +  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
> +GetMpidr (
> +  IN UINTN  CpuId
> +  );
> +
> +/**
> +  Get NUMA node id for a given cpu from device tree passed by Qemu.
> +
> +  @param [in]   CpuId    Index of cpu to retrieve NUMA node id for.
> +
> +  @retval                NUMA node id for CPU at index <CpuId>
> +**/
> +UINT64
> +GetCpuNumaNode (
> +  IN UINTN  CpuId
> +  );
> +
> +#endif /* HARDWARE_INFO_LIB */
> diff --git a/Silicon/Qemu/SbsaQemu/Library/SbsaQemuHardwareInfoLib/SbsaQemuHardwareInfoLib.c b/Silicon/Qemu/SbsaQemu/Library/SbsaQemuHardwareInfoLib/SbsaQemuHardwareInfoLib.c
> new file mode 100644
> index 000000000000..e96328978a55
> --- /dev/null
> +++ b/Silicon/Qemu/SbsaQemu/Library/SbsaQemuHardwareInfoLib/SbsaQemuHardwareInfoLib.c
> @@ -0,0 +1,96 @@
> +/** @file
> +*
> +*  Copyright (c) 2021, NUVIA Inc. All rights reserved.
> +*  Copyright (c) 2024, Linaro Ltd. All rights reserved.
> +*
> +*  SPDX-License-Identifier: BSD-2-Clause-Patent
> +*
> +**/
> +
> +#include <Library/ArmSmcLib.h>
> +#include <Library/DebugLib.h>
> +#include <Library/PcdLib.h>
> +#include <Library/ResetSystemLib.h>
> +#include <Library/HardwareInfoLib.h>
> +#include <IndustryStandard/SbsaQemuSmc.h>
> +
> +/**
> +  Get CPU count from information passed by Qemu.
> +
> +**/
> +UINT32
> +GetCpuCount (
> +  VOID
> +  )
> +{
> +  UINTN          Arg0;
> +  UINTN          SmcResult;
> +
> +  SmcResult = ArmCallSmc0 (SIP_SVC_GET_CPU_COUNT, &Arg0, NULL, NULL);
> +  if (SmcResult != SMC_SIP_CALL_SUCCESS) {
> +    DEBUG ((DEBUG_ERROR, "%a: SIP_SVC_GET_CPU_COUNT call failed. We have no cpu information.\n", __FUNCTION__));
> +    ResetShutdown ();
> +  }
> +
> +  DEBUG ((DEBUG_INFO, "%a: We have %d cpus.\n", __FUNCTION__, Arg0));
> +
> +  return Arg0;
> +}
> +
> +/**
> +  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
> +GetMpidr (
> +  IN UINTN  CpuId
> +  )
> +{
> +  UINTN  SmcResult;
> +  UINTN  Arg0;
> +  UINTN  Arg1;
> +
> +  Arg0 = CpuId;
> +
> +  SmcResult = ArmCallSmc0 (SIP_SVC_GET_CPU_NODE, &Arg0, &Arg1, NULL);
> +  if (SmcResult != SMC_SIP_CALL_SUCCESS) {
> +    DEBUG ((DEBUG_ERROR, "%a: SIP_SVC_GET_CPU_NODE call failed. We have no MPIDR for CPU%d.\n", __FUNCTION__, CpuId));
> +    ResetShutdown ();
> +  }
> +
> +  DEBUG ((DEBUG_INFO, "%a: MPIDR for CPU%d: = %d\n", __FUNCTION__, CpuId, Arg1));
> +
> +  return Arg1;
> +}
> +
> +/**
> +  Get NUMA node id for a given cpu from device tree passed by Qemu.
> +
> +  @param [in]   CpuId    Index of cpu to retrieve NUMA node id for.
> +
> +  @retval                NUMA node id for CPU at index <CpuId>
> +**/
> +UINT64
> +GetCpuNumaNode (
> +  IN UINTN  CpuId
> +  )
> +{
> +  UINTN  SmcResult;
> +  UINTN  Arg0;
> +  UINTN  Arg1;
> +
> +  Arg0 = CpuId;
> +
> +  SmcResult = ArmCallSmc0 (SIP_SVC_GET_CPU_NODE, &Arg0, &Arg1, NULL);
> +  if (SmcResult != SMC_SIP_CALL_SUCCESS) {
> +    DEBUG ((DEBUG_ERROR, "%a: SIP_SVC_GET_CPU_NODE call failed. Could not find information for CPU%d.\n", __FUNCTION__, CpuId));
> +    return 0;
> +  }
> +
> +  DEBUG ((DEBUG_INFO, "%a: NUMA node for CPU%d: = %d\n", __FUNCTION__, CpuId, Arg0));
> +
> +  return Arg0;
> +}
> 
> -- 
> 2.44.0
> 


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#117088): https://edk2.groups.io/g/devel/message/117088
Mute This Topic: https://groups.io/mt/105088436/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-