Re: [edk2-devel] [edk2-platforms][PATCH V1 01/20] ArmPkg: Change PcdFfaEnable flag datatype

["Chris Fernald" <chris.fernald@outlook.com>]

On top of Oliver's question, in the changes 
to|||StandaloneMmPkg/Library/StandaloneMmCoreEntryPoint/StandaloneMmCoreEntryPoint.inf, 
we should probably leave the .ARM and .AARCH64 specifiers since that is 
not an ARM specific INF.
|

-Chris

On 7/12/2023 10:21 AM, Oliver Smith-Denny wrote:
> On 7/11/2023 7:36 AM, Nishant Sharma wrote:
>> FeatureFlag type PCD flags are declared by typecasting an integer value
>> to BOOLEAN. These flags cannot be use in assembly code as assembler does
>> not recognise C primitive types. Change the flag data type from BOOLEAN
>> to UINT32.
>>
>> Signed-off-by: Nishant Sharma <nishant.sharma@arm.com>
>> ---
>> ArmPkg/ArmPkg.dec | 14 +++++++-------
>> ArmPkg/Library/StandaloneMmMmuLib/ArmMmuStandaloneMmLib.inf |  4 ++--
>> StandaloneMmPkg/Library/StandaloneMmCoreEntryPoint/StandaloneMmCoreEntryPoint.inf 
>> |  4 ++--
>> ArmPkg/Library/StandaloneMmMmuLib/ArmMmuStandaloneMmLib.c |  8 ++++----
>> StandaloneMmPkg/Library/StandaloneMmCoreEntryPoint/Arm/StandaloneMmCoreEntryPoint.c 
>> |  8 ++++----
>>   5 files changed, 19 insertions(+), 19 deletions(-)
>>
>> diff --git a/ArmPkg/ArmPkg.dec b/ArmPkg/ArmPkg.dec
>> index 1a16d044c94b..c36c23e2e059 100644
>> --- a/ArmPkg/ArmPkg.dec
>> +++ b/ArmPkg/ArmPkg.dec
>> @@ -155,13 +155,6 @@
>>     # hardware coherency (i.e., no virtualization or cache coherent DMA)
>> gArmTokenSpaceGuid.PcdNormalMemoryNonshareableOverride|FALSE|BOOLEAN|0x00000043
>>   -[PcdsFeatureFlag.AARCH64, PcdsFeatureFlag.ARM]
>> -  ## Used to select method for requesting services from S-EL1.<BR><BR>
>> -  #   TRUE  - Selects FF-A calls for communication between S-EL0 and 
>> SPMC.<BR>
>> -  #   FALSE - Selects SVC calls for communication between S-EL0 and 
>> SPMC.<BR>
>> -  # @Prompt Enable FF-A support.
>> -  gArmTokenSpaceGuid.PcdFfaEnable|FALSE|BOOLEAN|0x0000005B
>> -
>>   [PcdsFixedAtBuild.common]
>> gArmTokenSpaceGuid.PcdTrustzoneSupport|FALSE|BOOLEAN|0x00000006
>>   @@ -298,6 +291,13 @@
>>     # not currently supported.
>> gArmTokenSpaceGuid.PcdArmNonSecModeTransition|0x3c9|UINT32|0x0000003E
>>   +  ## Used to select method for requesting services from 
>> S-EL1.<BR><BR>
>> +  #   1 - Selects FF-A calls for communication between S-EL0 and 
>> SPMC.<BR>
>> +  #   0 - Selects SVC calls for communication between S-EL0 and 
>> SPMC.<BR>
>> +  #   Using unsigned integer as boolean does not work on assembler.
>> +  # @Prompt Enable FF-A support.
>> +  gArmTokenSpaceGuid.PcdFfaEnable|0|UINT32|0x0000005B
>> +
>
> A small nit: any reason not to go to UINT8 from BOOLEAN? It would seem
> the logical conversion, unless you envision it extending greatly in the
> future.
>
> Thanks,
> Oliver
>
>>     #
>>   # These PCDs are also defined as 'PcdsDynamic' or 
>> 'PcdsPatchableInModule' to be
>> diff --git 
>> a/ArmPkg/Library/StandaloneMmMmuLib/ArmMmuStandaloneMmLib.inf 
>> b/ArmPkg/Library/StandaloneMmMmuLib/ArmMmuStandaloneMmLib.inf
>> index ff20e5898051..3c733585f573 100644
>> --- a/ArmPkg/Library/StandaloneMmMmuLib/ArmMmuStandaloneMmLib.inf
>> +++ b/ArmPkg/Library/StandaloneMmMmuLib/ArmMmuStandaloneMmLib.inf
>> @@ -1,6 +1,6 @@
>>   #/** @file
>>   #
>> -#  Copyright (c) 2017 - 2021, Arm Limited. All rights reserved.<BR>
>> +#  Copyright (c) 2017 - 2023, Arm Limited. All rights reserved.
>>   #
>>   #  SPDX-License-Identifier: BSD-2-Clause-Patent
>>   #
>> @@ -23,7 +23,7 @@
>>     ArmPkg/ArmPkg.dec
>>     MdePkg/MdePkg.dec
>>   -[FeaturePcd.ARM, FeaturePcd.AARCH64]
>> +[FixedPcd]
>>     gArmTokenSpaceGuid.PcdFfaEnable
>>     [LibraryClasses]
>> diff --git 
>> a/StandaloneMmPkg/Library/StandaloneMmCoreEntryPoint/StandaloneMmCoreEntryPoint.inf 
>> b/StandaloneMmPkg/Library/StandaloneMmCoreEntryPoint/StandaloneMmCoreEntryPoint.inf 
>>
>> index 75cfb98c0e75..dc6d3d859911 100644
>> --- 
>> a/StandaloneMmPkg/Library/StandaloneMmCoreEntryPoint/StandaloneMmCoreEntryPoint.inf
>> +++ 
>> b/StandaloneMmPkg/Library/StandaloneMmCoreEntryPoint/StandaloneMmCoreEntryPoint.inf
>> @@ -1,7 +1,7 @@
>>   ## @file
>>   # Module entry point library for DXE core.
>>   #
>> -# Copyright (c) 2017 - 2021, Arm Ltd. All rights reserved.<BR>
>> +# Copyright (c) 2017 - 2023, Arm Ltd. All rights reserved.
>>   #
>>   #  SPDX-License-Identifier: BSD-2-Clause-Patent
>>   #
>> @@ -51,7 +51,7 @@
>>     gEfiStandaloneMmNonSecureBufferGuid
>>     gEfiArmTfCpuDriverEpDescriptorGuid
>>   -[FeaturePcd.ARM, FeaturePcd.AARCH64]
>> +[FixedPcd]
>>     gArmTokenSpaceGuid.PcdFfaEnable
>>     #
>> diff --git 
>> a/ArmPkg/Library/StandaloneMmMmuLib/ArmMmuStandaloneMmLib.c 
>> b/ArmPkg/Library/StandaloneMmMmuLib/ArmMmuStandaloneMmLib.c
>> index d55aff76201e..1a41a289ef17 100644
>> --- a/ArmPkg/Library/StandaloneMmMmuLib/ArmMmuStandaloneMmLib.c
>> +++ b/ArmPkg/Library/StandaloneMmMmuLib/ArmMmuStandaloneMmLib.c
>> @@ -1,7 +1,7 @@
>>   /** @file
>>     File managing the MMU for ARMv8 architecture in S-EL0
>>   -  Copyright (c) 2017 - 2021, Arm Limited. All rights reserved.<BR>
>> +  Copyright (c) 2017 - 2023, Arm Limited. All rights reserved.
>>     Copyright (c) 2021, Linaro Limited
>>     SPDX-License-Identifier: BSD-2-Clause-Patent
>>   @@ -54,7 +54,7 @@ SendMemoryPermissionRequest (
>>     }
>>       ArmCallSvc (SvcArgs);
>> -  if (FeaturePcdGet (PcdFfaEnable)) {
>> +  if (FixedPcdGet32 (PcdFfaEnable) != 0) {
>>       // Get/Set memory attributes is an atomic call, with
>>       // StandaloneMm at S-EL0 being the caller and the SPM
>>       // core being the callee. Thus there won't be a
>> @@ -163,7 +163,7 @@ GetMemoryPermissions (
>>     // Prepare the message parameters.
>>     // See [1], Section 13.5.5.1 MM_SP_MEMORY_ATTRIBUTES_GET_AARCH64.
>>     ZeroMem (&SvcArgs, sizeof (ARM_SVC_ARGS));
>> -  if (FeaturePcdGet (PcdFfaEnable)) {
>> +  if (FixedPcdGet32 (PcdFfaEnable) != 0) {
>>       // See [2], Section 10.2 FFA_MSG_SEND_DIRECT_REQ.
>>       SvcArgs.Arg0 = ARM_SVC_ID_FFA_MSG_SEND_DIRECT_REQ;
>>       SvcArgs.Arg1 = ARM_FFA_DESTINATION_ENDPOINT_ID;
>> @@ -218,7 +218,7 @@ RequestMemoryPermissionChange (
>>     // Prepare the message parameters.
>>     // See [1], Section 13.5.5.2 MM_SP_MEMORY_ATTRIBUTES_SET_AARCH64.
>>     ZeroMem (&SvcArgs, sizeof (ARM_SVC_ARGS));
>> -  if (FeaturePcdGet (PcdFfaEnable)) {
>> +  if (FixedPcdGet32 (PcdFfaEnable) != 0) {
>>       // See [2], Section 10.2 FFA_MSG_SEND_DIRECT_REQ.
>>       SvcArgs.Arg0 = ARM_SVC_ID_FFA_MSG_SEND_DIRECT_REQ;
>>       SvcArgs.Arg1 = ARM_FFA_DESTINATION_ENDPOINT_ID;
>> diff --git 
>> a/StandaloneMmPkg/Library/StandaloneMmCoreEntryPoint/Arm/StandaloneMmCoreEntryPoint.c 
>> b/StandaloneMmPkg/Library/StandaloneMmCoreEntryPoint/Arm/StandaloneMmCoreEntryPoint.c 
>>
>> index 96de10405af8..5dd1d9747995 100644
>> --- 
>> a/StandaloneMmPkg/Library/StandaloneMmCoreEntryPoint/Arm/StandaloneMmCoreEntryPoint.c
>> +++ 
>> b/StandaloneMmPkg/Library/StandaloneMmCoreEntryPoint/Arm/StandaloneMmCoreEntryPoint.c
>> @@ -2,7 +2,7 @@
>>     Entry point to the Standalone MM Foundation when initialized 
>> during the SEC
>>     phase on ARM platforms
>>   -Copyright (c) 2017 - 2021, Arm Ltd. All rights reserved.<BR>
>> +Copyright (c) 2017 - 2023, Arm Ltd. All rights reserved.
>>   SPDX-License-Identifier: BSD-2-Clause-Patent
>>     **/
>> @@ -140,7 +140,7 @@ DelegatedEventLoop (
>>       DEBUG ((DEBUG_INFO, "X6 :  0x%x\n", 
>> (UINT32)EventCompleteSvcArgs->Arg6));
>>       DEBUG ((DEBUG_INFO, "X7 :  0x%x\n", 
>> (UINT32)EventCompleteSvcArgs->Arg7));
>>   -    FfaEnabled = FeaturePcdGet (PcdFfaEnable);
>> +    FfaEnabled = FixedPcdGet32 (PcdFfaEnable != 0);
>>       if (FfaEnabled) {
>>         Status = CpuDriverEntryPoint (
>>                    EventCompleteSvcArgs->Arg0,
>> @@ -225,7 +225,7 @@ GetSpmVersion (
>>     UINT32        SpmVersion;
>>     ARM_SVC_ARGS  SpmVersionArgs;
>>   -  if (FeaturePcdGet (PcdFfaEnable)) {
>> +  if (FixedPcdGet32 (PcdFfaEnable) != 0) {
>>       SpmVersionArgs.Arg0  = ARM_SVC_ID_FFA_VERSION_AARCH32;
>>       SpmVersionArgs.Arg1  = mSpmMajorVerFfa << SPM_MAJOR_VER_SHIFT;
>>       SpmVersionArgs.Arg1 |= mSpmMinorVerFfa;
>> @@ -293,7 +293,7 @@ InitArmSvcArgs (
>>     OUT INT32         *Ret
>>     )
>>   {
>> -  if (FeaturePcdGet (PcdFfaEnable)) {
>> +  if (FixedPcdGet32 (PcdFfaEnable) != 0) {
>>       InitMmFoundationSvcArgs->Arg0 = 
>> ARM_SVC_ID_FFA_MSG_SEND_DIRECT_RESP;
>>       InitMmFoundationSvcArgs->Arg1 = 0;
>>       InitMmFoundationSvcArgs->Arg2 = 0;
>
>
> 
>
>