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

["Marcin Juszkiewicz" <marcin.juszkiewicz@linaro.org>]

W dniu 19.01.2024 o 20:18, Leif Lindholm pisze:
> On Tue, Jan 16, 2024 at 08:48:32 +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>
>> ---
>>   Platform/Qemu/SbsaQemu/SbsaQemu.dsc                 |   3 +-
>>   .../SbsaQemuHardwareInfoLib.inf                     |  32 +++++++
>>   .../SbsaQemu/Include/IndustryStandard/SbsaQemuSmc.h |   2 +
>>   .../Include/Library/SbsaQemuHardwareInfoLib.h       |  45 +++++++++
>>   .../SbsaQemuHardwareInfoLib.c                       | 100 ++++++++++++++++++++
>>   5 files changed, 181 insertions(+), 1 deletion(-)
>>


>> diff --git a/Silicon/Qemu/SbsaQemu/Library/SbsaQemuHardwareInfoLib/SbsaQemuHardwareInfoLib.c b/Silicon/Qemu/SbsaQemu/Library/SbsaQemuHardwareInfoLib/SbsaQemuHardwareInfoLib.c
>> new file mode 100644
>> index 000000000000..4df973fda75e
>> --- /dev/null
>> +++ b/Silicon/Qemu/SbsaQemu/Library/SbsaQemuHardwareInfoLib/SbsaQemuHardwareInfoLib.c
>> @@ -0,0 +1,100 @@
>> +/** @file
>> +*
>> +*  Copyright (c) 2021, NUVIA Inc. All rights reserved.
>> +*  Copyright (c) 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/SbsaQemuHardwareInfoLib.h>
>> +#include <IndustryStandard/SbsaQemuSmc.h>
>> +
>> +/**
>> +  Get CPU count from information passed by Qemu.
>> +
>> +**/
>> +VOID
>> +SbsaQemuGetCpuCount (
>> +  VOID
>> +  )
>> +{
>> +  UINTN          Arg0;
>> +  UINTN          SmcResult;
>> +  RETURN_STATUS  Result;
>> +
>> +  SmcResult = ArmCallSmc0 (SIP_SVC_GET_CPU_COUNT, &Arg0, NULL, NULL);
>> +  if (SmcResult != SMC_ARCH_CALL_SUCCESS) {
> 
> So, this may sound a little bit bonkers, but SMCCC doesn't define
> return codes for SiP functions. And the SMC_ARCH_CALL_SUCCESS macro
> denotes an Arm Architectural function.
> Could you #define an SMC_SIP_CALL_SUCCESS *as* SMC_ARCH_CALL_SUCCESS
> in some platform-specific header and then use that?

Added it in Silicon/Qemu/SbsaQemu/Include/IndustryStandard/SbsaQemuSmc.h 
and used.


>> +    DEBUG ((DEBUG_INFO, "Too old TF-A. We have to get cpu info from DT.\n"));
> 
> We don't *know* the TF-A is too old. Rather print which call failed
> (and that we need to attempt to get the cpu count from DT).

Changed:

     DEBUG ((DEBUG_INFO, "SIP_SVC_GET_CPU_COUNT call failed. We have to 
get cpu info from DT.\n"));


>> +    Arg0 = FdtHelperCountCpus();
> 
> We should probably assert this as != 0? Or is that done in the function?

Added code to stop going if we do not have cpu information. In such case 
we will not be back here with wrong info.

> 
>> +  }
>> +
>> +  Result = PcdSet32S (PcdCoreCount, Arg0);
>> +  ASSERT_RETURN_ERROR (Result);
>> +
>> +  Arg0 = PcdGet32 (PcdCoreCount);
> 
> Seems redundant to re-set it to the value we know it already holds.

Done

>> +
>> +  DEBUG ((DEBUG_INFO, "We have %d cpus.\n", 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
>> +SbsaQemuGetMpidr (
>> +  IN UINTN   CpuId
>> +  )
>> +{
>> +  UINTN SmcResult;
>> +  UINTN Arg0;
>> +  UINTN Arg1;
>> +
>> +  Arg0 = CpuId;
>> +
>> +  SmcResult = ArmCallSmc0 (SIP_SVC_GET_CPU_NODE, &Arg0, &Arg1, NULL);
>> +  if (SmcResult != SMC_ARCH_CALL_SUCCESS) {
>> +    DEBUG ((DEBUG_INFO, "Too old TF-A. We have to get cpu info from DT.\n"));
> 
> We don't *know* the TF-A is too old. Rather print which call failed
> (and that we need to attempt to get the cpu date from DT).

done

>> +UINT64
>> +SbsaQemuGetCpuNumaNode (
>> +  IN UINTN   CpuId
>> +  )
>> +{
>> +  UINTN SmcResult;
>> +  UINTN Arg0;
>> +  UINTN Arg1;
>> +
>> +  Arg0 = CpuId;
>> +
>> +  SmcResult = ArmCallSmc0 (SIP_SVC_GET_CPU_NODE, &Arg0, &Arg1, NULL);
> 
> It does seem a bit wasteful we're making the same call twice per core,
> discarding one of the results.
> Could we have an init function that allocates an array and
> prepopulates it, with the Get-functions just returning values from the array?

good idea, will look into

>> +  if (SmcResult != SMC_ARCH_CALL_SUCCESS) {
>> +    /* No fallback to DeviceTree as we did not had that info earlier. */
> 
> We don't *know* the TF-A is too old. Rather print which call failed
> (and that we need to attempt to get the cpu count from DT).

done



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