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

["Oliver Smith-Denny" <osde@linux.microsoft.com>]

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;