[PATCH v2 1/1] ArmPkg: Fix uninitialised variable in ArmMmuStandaloneMmLib

[PATCH v2 1/1] ArmPkg: Fix uninitialised variable in ArmMmuStandaloneMmLib

[Sami Mujawar]

The following patches added support for StandaloneMM using FF-A:
9da5ee116a28 ArmPkg: Allow FF-A calls to set memory region's attributes
0e43e02b9bd8 ArmPkg: Allow FF-A calls to get memory region's attributes

However, in the error handling logic for the Get/Set Memory attributes,
the CLANG compiler reports that a status variable could be used without
initialisation. This issue is a false positive and is not seen with GCC.

The Get/Set Memory attributes operation is atomic and therefore an
FFA_INTERRUPT or FFA_SUCCESS response is not expected in response
to FFA_MSG_SEND_DIRECT_REQ. So the remaining cases that could occur
are:
 - the target sends FFA_MSG_SEND_DIRECT_RESP with a success or
   failure code.
 or
 - FFA_MSG_SEND_DIRECT_REQ transmission failure.

Therefore,
 - reorder the error handling conditions such that it prevents the
   uninitialised variable issue being flagged by CLANG.
 - move the repetitive code to a static helper function and add
   documentation at the appropriate places.
 - fix error handling in functions that invoke GetMemoryPermissions().

Signed-off-by: Sami Mujawar <sami.mujawar@arm.com>
---
The changes can be seen at:
https://github.com/samimujawar/edk2/tree/1657_stmm_ffa_fix_unused_var_v2

Notes:
    v2:
     - Move common code to a static helper function.             [LEIF]
     - Updated based on review feedback. Also refactored,        [SAMI]
       added documentation, and made some general improvements.

 ArmPkg/Library/StandaloneMmMmuLib/AArch64/ArmMmuStandaloneMmLib.c | 365 +++++++++++---------
 1 file changed, 200 insertions(+), 165 deletions(-)

diff --git a/ArmPkg/Library/StandaloneMmMmuLib/AArch64/ArmMmuStandaloneMmLib.c b/ArmPkg/Library/StandaloneMmMmuLib/AArch64/ArmMmuStandaloneMmLib.c
index a30369af9c91fb8045dfec7a68e2bd072706d101..5f453d18e4156b1e076f503de7c56ada411aaa25 100644
--- a/ArmPkg/Library/StandaloneMmMmuLib/AArch64/ArmMmuStandaloneMmLib.c
+++ b/ArmPkg/Library/StandaloneMmMmuLib/AArch64/ArmMmuStandaloneMmLib.c
@@ -1,10 +1,15 @@
 /** @file
-*  File managing the MMU for ARMv8 architecture in S-EL0
-*
-*  Copyright (c) 2017 - 2021, Arm Limited. All rights reserved.<BR>
-*
-*  SPDX-License-Identifier: BSD-2-Clause-Patent
-*
+  File managing the MMU for ARMv8 architecture in S-EL0
+
+  Copyright (c) 2017 - 2021, Arm Limited. All rights reserved.<BR>
+  SPDX-License-Identifier: BSD-2-Clause-Patent
+
+  @par Reference(s):
+  - [1] SPM based on the MM interface.
+        (https://trustedfirmware-a.readthedocs.io/en/latest/components/
+         secure-partition-manager-mm.html)
+  - [2] Arm Firmware Framework for Armv8-A, DEN0077A, version 1.0
+        (https://developer.arm.com/documentation/den0077/a)
 **/
 
 #include <Uefi.h>
@@ -19,6 +24,126 @@
 #include <Library/DebugLib.h>
 #include <Library/PcdLib.h>
 
+/** Send memory permission request to target.
+
+  @param [in, out]  SvcArgs     Pointer to SVC arguments to send. On
+                                return it contains the response parameters.
+  @param [out]      RetVal      Pointer to return the response value.
+
+  @retval EFI_SUCCESS           Request successfull.
+  @retval EFI_INVALID_PARAMETER A parameter is invalid.
+  @retval EFI_NOT_READY         Callee is busy or not in a state to handle
+                                this request.
+  @retval EFI_UNSUPPORTED       This function is not implemented by the
+                                callee.
+  @retval EFI_ABORTED           Message target ran into an unexpected error
+                                and has aborted.
+  @retval EFI_ACCESS_DENIED     Access denied.
+  @retval EFI_OUT_OF_RESOURCES  Out of memory to perform operation.
+**/
+STATIC
+EFI_STATUS
+SendMemoryPermissionRequest (
+  IN OUT  ARM_SVC_ARGS *SvcArgs,
+     OUT  INT32        *RetVal
+  )
+{
+  if ((SvcArgs == NULL) || (RetVal == NULL)) {
+    return EFI_INVALID_PARAMETER;
+  }
+
+  ArmCallSvc (SvcArgs);
+  if (FeaturePcdGet (PcdFfaEnable)) {
+    // 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
+    // FFA_INTERRUPT or FFA_SUCCESS response to the Direct
+    // Request sent above. This will have to be considered
+    // for other Direct Request calls which are not atomic
+    // We therefore check only for Direct Response by the
+    // callee.
+    if (SvcArgs->Arg0 == ARM_SVC_ID_FFA_MSG_SEND_DIRECT_RESP_AARCH64) {
+      // A Direct Response means FF-A success
+      // Now check the payload for errors
+      // The callee sends back the return value
+      // in Arg3
+      *RetVal = SvcArgs->Arg3;
+    } else {
+      // If Arg0 is not a Direct Response, that means we
+      // have an FF-A error. We need to check Arg2 for the
+      // FF-A error code.
+      // See [2], Table 10.8: FFA_ERROR encoding.
+      *RetVal = SvcArgs->Arg2;
+      switch (*RetVal) {
+        case ARM_FFA_SPM_RET_INVALID_PARAMETERS:
+          return EFI_INVALID_PARAMETER;
+
+        case ARM_FFA_SPM_RET_DENIED:
+          return EFI_ACCESS_DENIED;
+
+        case ARM_FFA_SPM_RET_NOT_SUPPORTED:
+          return EFI_UNSUPPORTED;
+
+        case ARM_FFA_SPM_RET_BUSY:
+          return EFI_NOT_READY;
+
+        case ARM_FFA_SPM_RET_ABORTED:
+          return EFI_ABORTED;
+
+        default:
+          // Undefined error code received.
+          ASSERT (0);
+          return EFI_INVALID_PARAMETER;
+      }
+    }
+  } else {
+    *RetVal = SvcArgs->Arg0;
+  }
+
+  // Check error response from Callee.
+  if (*RetVal & BIT31) {
+    // Bit 31 set means there is an error retured
+    // See [1], Section 13.5.5.1 MM_SP_MEMORY_ATTRIBUTES_GET_AARCH64 and
+    // Section 13.5.5.2 MM_SP_MEMORY_ATTRIBUTES_SET_AARCH64.
+    switch (*RetVal) {
+      case ARM_SVC_SPM_RET_NOT_SUPPORTED:
+        return EFI_UNSUPPORTED;
+
+      case ARM_SVC_SPM_RET_INVALID_PARAMS:
+        return EFI_INVALID_PARAMETER;
+
+      case ARM_SVC_SPM_RET_DENIED:
+        return EFI_ACCESS_DENIED;
+
+      case ARM_SVC_SPM_RET_NO_MEMORY:
+        return EFI_OUT_OF_RESOURCES;
+
+      default:
+        // Undefined error code received.
+        ASSERT (0);
+        return EFI_INVALID_PARAMETER;
+    }
+  }
+
+  return EFI_SUCCESS;
+}
+
+/** Request the permission attributes of a memory region from S-EL0.
+
+  @param [in]   BaseAddress          Base address for the memory region.
+  @param [out]  MemoryAttributes     Pointer to return the memory attributes.
+
+  @retval EFI_SUCCESS             Request successfull.
+  @retval EFI_INVALID_PARAMETER   A parameter is invalid.
+  @retval EFI_NOT_READY           Callee is busy or not in a state to handle
+                                  this request.
+  @retval EFI_UNSUPPORTED         This function is not implemented by the
+                                  callee.
+  @retval EFI_ABORTED             Message target ran into an unexpected error
+                                  and has aborted.
+  @retval EFI_ACCESS_DENIED       Access denied.
+  @retval EFI_OUT_OF_RESOURCES    Out of memory to perform operation.
+**/
 STATIC
 EFI_STATUS
 GetMemoryPermissions (
@@ -26,179 +151,89 @@ GetMemoryPermissions (
   OUT UINT32                    *MemoryAttributes
   )
 {
+  EFI_STATUS    Status;
   INT32         Ret;
-  ARM_SVC_ARGS  GetMemoryPermissionsSvcArgs;
-  BOOLEAN       FfaEnabled;
+  ARM_SVC_ARGS  SvcArgs;
 
-  ZeroMem (&GetMemoryPermissionsSvcArgs, sizeof (ARM_SVC_ARGS));
-
-  FfaEnabled = FeaturePcdGet (PcdFfaEnable);
-  if (FfaEnabled) {
-    GetMemoryPermissionsSvcArgs.Arg0 = ARM_SVC_ID_FFA_MSG_SEND_DIRECT_REQ_AARCH64;
-    GetMemoryPermissionsSvcArgs.Arg1 = ARM_FFA_DESTINATION_ENDPOINT_ID;
-    GetMemoryPermissionsSvcArgs.Arg2 = 0;
-    GetMemoryPermissionsSvcArgs.Arg3 = ARM_SVC_ID_SP_GET_MEM_ATTRIBUTES_AARCH64;
-    GetMemoryPermissionsSvcArgs.Arg4 = BaseAddress;
-  } else {
-    GetMemoryPermissionsSvcArgs.Arg0 = ARM_SVC_ID_SP_GET_MEM_ATTRIBUTES_AARCH64;
-    GetMemoryPermissionsSvcArgs.Arg1 = BaseAddress;
-    GetMemoryPermissionsSvcArgs.Arg2 = 0;
-    GetMemoryPermissionsSvcArgs.Arg3 = 0;
+  if (MemoryAttributes == NULL) {
+    return EFI_INVALID_PARAMETER;
   }
 
-  *MemoryAttributes = 0;
-  ArmCallSvc (&GetMemoryPermissionsSvcArgs);
-  if (FfaEnabled) {
-    // Getting 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
-    // FFA_INTERRUPT or FFA_SUCCESS response to the Direct
-    // Request sent above. This will have to be considered
-    // for other Direct Request calls which are not atomic
-    // We therefore check only for Direct Response by the
-    // callee.
-    if (GetMemoryPermissionsSvcArgs.Arg0 !=
-        ARM_SVC_ID_FFA_MSG_SEND_DIRECT_RESP_AARCH64) {
-      // If Arg0 is not a Direct Response, that means we
-      // have an FF-A error. We need to check Arg2 for the
-      // FF-A error code.
-      Ret = GetMemoryPermissionsSvcArgs.Arg2;
-      switch (Ret) {
-      case ARM_FFA_SPM_RET_INVALID_PARAMETERS:
-
-        return EFI_INVALID_PARAMETER;
-
-      case ARM_FFA_SPM_RET_DENIED:
-        return EFI_NOT_READY;
-
-      case ARM_FFA_SPM_RET_NOT_SUPPORTED:
-        return EFI_UNSUPPORTED;
-
-      case ARM_FFA_SPM_RET_BUSY:
-        return EFI_NOT_READY;
-
-      case ARM_FFA_SPM_RET_ABORTED:
-        return EFI_ABORTED;
-      }
-    } else if (GetMemoryPermissionsSvcArgs.Arg0 ==
-               ARM_SVC_ID_FFA_MSG_SEND_DIRECT_RESP_AARCH64) {
-      // A Direct Response means FF-A success
-      // Now check the payload for errors
-      // The callee sends back the return value
-      // in Arg3
-      Ret = GetMemoryPermissionsSvcArgs.Arg3;
-    }
+  // 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)) {
+    // See [2], Section 10.2 FFA_MSG_SEND_DIRECT_REQ.
+    SvcArgs.Arg0 = ARM_SVC_ID_FFA_MSG_SEND_DIRECT_REQ_AARCH64;
+    SvcArgs.Arg1 = ARM_FFA_DESTINATION_ENDPOINT_ID;
+    SvcArgs.Arg2 = 0;
+    SvcArgs.Arg3 = ARM_SVC_ID_SP_GET_MEM_ATTRIBUTES_AARCH64;
+    SvcArgs.Arg4 = BaseAddress;
   } else {
-    Ret = GetMemoryPermissionsSvcArgs.Arg0;
+    SvcArgs.Arg0 = ARM_SVC_ID_SP_GET_MEM_ATTRIBUTES_AARCH64;
+    SvcArgs.Arg1 = BaseAddress;
+    SvcArgs.Arg2 = 0;
+    SvcArgs.Arg3 = 0;
   }
 
-  if (Ret & BIT31) {
-    // Bit 31 set means there is an error retured
-    switch (Ret) {
-    case ARM_SVC_SPM_RET_INVALID_PARAMS:
-      return EFI_INVALID_PARAMETER;
-
-    case ARM_SVC_SPM_RET_NOT_SUPPORTED:
-      return EFI_UNSUPPORTED;
-    }
-  } else {
-    *MemoryAttributes = Ret;
+  Status = SendMemoryPermissionRequest (&SvcArgs, &Ret);
+  if (EFI_ERROR (Status)) {
+    *MemoryAttributes = 0;
+    return Status;
   }
 
-  return EFI_SUCCESS;
+  *MemoryAttributes = Ret;
+  return Status;
 }
 
+/** Set the permission attributes of a memory region from S-EL0.
+
+  @param [in]  BaseAddress     Base address for the memory region.
+  @param [in]  Length          Length of the memory region.
+  @param [in]  Permissions     Memory access controls attributes.
+
+  @retval EFI_SUCCESS             Request successfull.
+  @retval EFI_INVALID_PARAMETER   A parameter is invalid.
+  @retval EFI_NOT_READY           Callee is busy or not in a state to handle
+                                  this request.
+  @retval EFI_UNSUPPORTED         This function is not implemented by the
+                                  callee.
+  @retval EFI_ABORTED             Message target ran into an unexpected error
+                                  and has aborted.
+  @retval EFI_ACCESS_DENIED       Access denied.
+  @retval EFI_OUT_OF_RESOURCES    Out of memory to perform operation.
+**/
 STATIC
 EFI_STATUS
 RequestMemoryPermissionChange (
   IN  EFI_PHYSICAL_ADDRESS      BaseAddress,
   IN  UINT64                    Length,
-  IN  UINTN                     Permissions
+  IN  UINT32                    Permissions
   )
 {
   INT32         Ret;
-  BOOLEAN       FfaEnabled;
-  ARM_SVC_ARGS  ChangeMemoryPermissionsSvcArgs;
-
-  ZeroMem (&ChangeMemoryPermissionsSvcArgs, sizeof (ARM_SVC_ARGS));
-
-  FfaEnabled = FeaturePcdGet (PcdFfaEnable);
-
-  if (FfaEnabled) {
-    ChangeMemoryPermissionsSvcArgs.Arg0 = ARM_SVC_ID_FFA_MSG_SEND_DIRECT_REQ_AARCH64;
-    ChangeMemoryPermissionsSvcArgs.Arg1 = ARM_FFA_DESTINATION_ENDPOINT_ID;
-    ChangeMemoryPermissionsSvcArgs.Arg2 = 0;
-    ChangeMemoryPermissionsSvcArgs.Arg3 = ARM_SVC_ID_SP_SET_MEM_ATTRIBUTES_AARCH64;
-    ChangeMemoryPermissionsSvcArgs.Arg4 = BaseAddress;
-    ChangeMemoryPermissionsSvcArgs.Arg5 = EFI_SIZE_TO_PAGES (Length);
-    ChangeMemoryPermissionsSvcArgs.Arg6 = Permissions;
-  } else {
-    ChangeMemoryPermissionsSvcArgs.Arg0 = ARM_SVC_ID_SP_SET_MEM_ATTRIBUTES_AARCH64;
-    ChangeMemoryPermissionsSvcArgs.Arg1 = BaseAddress;
-    ChangeMemoryPermissionsSvcArgs.Arg2 = EFI_SIZE_TO_PAGES (Length);
-    ChangeMemoryPermissionsSvcArgs.Arg3 = Permissions;
-  }
-
-  ArmCallSvc (&ChangeMemoryPermissionsSvcArgs);
-
-  if (FfaEnabled) {
-    // Setting 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
-    // FFA_INTERRUPT or FFA_SUCCESS response to the Direct
-    // Request sent above. This will have to be considered
-    // for other Direct Request calls which are not atomic
-    // We therefore check only for Direct Response by the
-    // callee.
-    if (ChangeMemoryPermissionsSvcArgs.Arg0 !=
-        ARM_SVC_ID_FFA_MSG_SEND_DIRECT_RESP_AARCH64) {
-      // If Arg0 is not a Direct Response, that means we
-      // have an FF-A error. We need to check Arg2 for the
-      // FF-A error code.
-      Ret = ChangeMemoryPermissionsSvcArgs.Arg2;
-      switch (Ret) {
-      case ARM_FFA_SPM_RET_INVALID_PARAMETERS:
-        return EFI_INVALID_PARAMETER;
-
-      case ARM_FFA_SPM_RET_DENIED:
-        return EFI_NOT_READY;
-
-      case ARM_FFA_SPM_RET_NOT_SUPPORTED:
-        return EFI_UNSUPPORTED;
-
-      case ARM_FFA_SPM_RET_BUSY:
-        return EFI_NOT_READY;
-
-      case ARM_FFA_SPM_RET_ABORTED:
-        return EFI_ABORTED;
-      }
-    } else if (ChangeMemoryPermissionsSvcArgs.Arg0 ==
-               ARM_SVC_ID_FFA_MSG_SEND_DIRECT_RESP_AARCH64) {
-      // A Direct Response means FF-A success
-      // Now check the payload for errors
-      // The callee sends back the return value
-      // in Arg3
-      Ret = ChangeMemoryPermissionsSvcArgs.Arg3;
-    }
+  ARM_SVC_ARGS  SvcArgs;
+
+  // 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)) {
+    // See [2], Section 10.2 FFA_MSG_SEND_DIRECT_REQ.
+    SvcArgs.Arg0 = ARM_SVC_ID_FFA_MSG_SEND_DIRECT_REQ_AARCH64;
+    SvcArgs.Arg1 = ARM_FFA_DESTINATION_ENDPOINT_ID;
+    SvcArgs.Arg2 = 0;
+    SvcArgs.Arg3 = ARM_SVC_ID_SP_SET_MEM_ATTRIBUTES_AARCH64;
+    SvcArgs.Arg4 = BaseAddress;
+    SvcArgs.Arg5 = EFI_SIZE_TO_PAGES (Length);
+    SvcArgs.Arg6 = Permissions;
   } else {
-    Ret = ChangeMemoryPermissionsSvcArgs.Arg0;
-  }
-
-  switch (Ret) {
-  case ARM_SVC_SPM_RET_NOT_SUPPORTED:
-    return EFI_UNSUPPORTED;
-
-  case ARM_SVC_SPM_RET_INVALID_PARAMS:
-    return EFI_INVALID_PARAMETER;
-
-  case ARM_SVC_SPM_RET_DENIED:
-    return EFI_ACCESS_DENIED;
-
-  case ARM_SVC_SPM_RET_NO_MEMORY:
-    return EFI_BAD_BUFFER_SIZE;
+    SvcArgs.Arg0 = ARM_SVC_ID_SP_SET_MEM_ATTRIBUTES_AARCH64;
+    SvcArgs.Arg1 = BaseAddress;
+    SvcArgs.Arg2 = EFI_SIZE_TO_PAGES (Length);
+    SvcArgs.Arg3 = Permissions;
   }
 
-  return EFI_SUCCESS;
+  return SendMemoryPermissionRequest (&SvcArgs, &Ret);
 }
 
 EFI_STATUS
@@ -212,7 +247,7 @@ ArmSetMemoryRegionNoExec (
   UINT32 CodePermission;
 
   Status = GetMemoryPermissions (BaseAddress, &MemoryAttributes);
-  if (Status != EFI_INVALID_PARAMETER) {
+  if (!EFI_ERROR (Status)) {
     CodePermission = SET_MEM_ATTR_CODE_PERM_XN << SET_MEM_ATTR_CODE_PERM_SHIFT;
     return RequestMemoryPermissionChange (
              BaseAddress,
@@ -220,7 +255,7 @@ ArmSetMemoryRegionNoExec (
              MemoryAttributes | CodePermission
              );
   }
-  return EFI_INVALID_PARAMETER;
+  return Status;
 }
 
 EFI_STATUS
@@ -234,7 +269,7 @@ ArmClearMemoryRegionNoExec (
   UINT32 CodePermission;
 
   Status = GetMemoryPermissions (BaseAddress, &MemoryAttributes);
-  if (Status != EFI_INVALID_PARAMETER) {
+  if (!EFI_ERROR (Status)) {
     CodePermission = SET_MEM_ATTR_CODE_PERM_XN << SET_MEM_ATTR_CODE_PERM_SHIFT;
     return RequestMemoryPermissionChange (
              BaseAddress,
@@ -242,7 +277,7 @@ ArmClearMemoryRegionNoExec (
              MemoryAttributes & ~CodePermission
              );
   }
-  return EFI_INVALID_PARAMETER;
+  return Status;
 }
 
 EFI_STATUS
@@ -256,7 +291,7 @@ ArmSetMemoryRegionReadOnly (
   UINT32 DataPermission;
 
   Status = GetMemoryPermissions (BaseAddress, &MemoryAttributes);
-  if (Status != EFI_INVALID_PARAMETER) {
+  if (!EFI_ERROR (Status)) {
     DataPermission = SET_MEM_ATTR_DATA_PERM_RO << SET_MEM_ATTR_DATA_PERM_SHIFT;
     return RequestMemoryPermissionChange (
              BaseAddress,
@@ -264,7 +299,7 @@ ArmSetMemoryRegionReadOnly (
              MemoryAttributes | DataPermission
              );
   }
-  return EFI_INVALID_PARAMETER;
+  return Status;
 }
 
 EFI_STATUS
@@ -278,7 +313,7 @@ ArmClearMemoryRegionReadOnly (
   UINT32 PermissionRequest;
 
   Status = GetMemoryPermissions (BaseAddress, &MemoryAttributes);
-  if (Status != EFI_INVALID_PARAMETER) {
+  if (!EFI_ERROR (Status)) {
     PermissionRequest = SET_MEM_ATTR_MAKE_PERM_REQUEST (SET_MEM_ATTR_DATA_PERM_RW,
                                                         MemoryAttributes);
     return RequestMemoryPermissionChange (
@@ -287,5 +322,5 @@ ArmClearMemoryRegionReadOnly (
              PermissionRequest
              );
   }
-  return EFI_INVALID_PARAMETER;
+  return Status;
 }
-- 
'Guid(CE165669-3EF3-493F-B85D-6190EE5B9759)'

Re: [edk2-devel] [PATCH v2 1/1] ArmPkg: Fix uninitialised variable in ArmMmuStandaloneMmLib

[Sughosh Ganu]

[-- Attachment #1: Type: text/plain, Size: 1547 bytes --]

On Thu, 25 Feb 2021 at 22:41, Sami Mujawar <sami.mujawar@arm.com> wrote:

> The following patches added support for StandaloneMM using FF-A:
> 9da5ee116a28 ArmPkg: Allow FF-A calls to set memory region's attributes
> 0e43e02b9bd8 ArmPkg: Allow FF-A calls to get memory region's attributes
>
> However, in the error handling logic for the Get/Set Memory attributes,
> the CLANG compiler reports that a status variable could be used without
> initialisation. This issue is a false positive and is not seen with GCC.
>
> The Get/Set Memory attributes operation is atomic and therefore an
> FFA_INTERRUPT or FFA_SUCCESS response is not expected in response
> to FFA_MSG_SEND_DIRECT_REQ. So the remaining cases that could occur
> are:
>  - the target sends FFA_MSG_SEND_DIRECT_RESP with a success or
>    failure code.
>  or
>  - FFA_MSG_SEND_DIRECT_REQ transmission failure.
>
> Therefore,
>  - reorder the error handling conditions such that it prevents the
>    uninitialised variable issue being flagged by CLANG.
>  - move the repetitive code to a static helper function and add
>    documentation at the appropriate places.
>  - fix error handling in functions that invoke GetMemoryPermissions().
>
> Signed-off-by: Sami Mujawar <sami.mujawar@arm.com>
> ---
> The changes can be seen at:
> https://github.com/samimujawar/edk2/tree/1657_stmm_ffa_fix_unused_var_v2


 Tested the changes on the StandaloneMm image on the Qemu platform.

Tested-by: Sughosh Ganu <sughosh.ganu@linaro.org>
Reviewed-by: Sughosh Ganu <sughosh.ganu@linaro.org>

-sughosh

[-- Attachment #2: Type: text/html, Size: 2320 bytes --]

Re: [edk2-devel] [PATCH v2 1/1] ArmPkg: Fix uninitialised variable in ArmMmuStandaloneMmLib

[Ard Biesheuvel]

On Fri, 26 Feb 2021 at 11:58, Sughosh Ganu <sughosh.ganu@linaro.org> wrote:
>
>
> On Thu, 25 Feb 2021 at 22:41, Sami Mujawar <sami.mujawar@arm.com> wrote:
>>
>> The following patches added support for StandaloneMM using FF-A:
>> 9da5ee116a28 ArmPkg: Allow FF-A calls to set memory region's attributes
>> 0e43e02b9bd8 ArmPkg: Allow FF-A calls to get memory region's attributes
>>
>> However, in the error handling logic for the Get/Set Memory attributes,
>> the CLANG compiler reports that a status variable could be used without
>> initialisation. This issue is a false positive and is not seen with GCC.
>>
>> The Get/Set Memory attributes operation is atomic and therefore an
>> FFA_INTERRUPT or FFA_SUCCESS response is not expected in response
>> to FFA_MSG_SEND_DIRECT_REQ. So the remaining cases that could occur
>> are:
>>  - the target sends FFA_MSG_SEND_DIRECT_RESP with a success or
>>    failure code.
>>  or
>>  - FFA_MSG_SEND_DIRECT_REQ transmission failure.
>>
>> Therefore,
>>  - reorder the error handling conditions such that it prevents the
>>    uninitialised variable issue being flagged by CLANG.
>>  - move the repetitive code to a static helper function and add
>>    documentation at the appropriate places.
>>  - fix error handling in functions that invoke GetMemoryPermissions().
>>
>> Signed-off-by: Sami Mujawar <sami.mujawar@arm.com>
>> ---
>> The changes can be seen at:
>> https://github.com/samimujawar/edk2/tree/1657_stmm_ffa_fix_unused_var_v2
>
>
>  Tested the changes on the StandaloneMm image on the Qemu platform.
>
> Tested-by: Sughosh Ganu <sughosh.ganu@linaro.org>
> Reviewed-by: Sughosh Ganu <sughosh.ganu@linaro.org>
>

Thanks. Sami, can you confirm that this patch fixes the CI failure I
reported to you in private? If so, I intend to merge this during the
freeze (assuming Liming is ok with that)

Re: [edk2-devel] [PATCH v2 1/1] ArmPkg: Fix uninitialised variable in ArmMmuStandaloneMmLib

[Sami Mujawar]

[-- Attachment #1: Type: text/plain, Size: 2363 bytes --]

Hi Ard,

Yes. This patch with fix the build failure reported by the CI.

Regards,

Sami Mujawar

________________________________
From: Ard Biesheuvel <ardb@kernel.org>
Sent: Friday, 26 February 2021, 6:46 pm
To: Sughosh Ganu; Liming Gao (Byosoft address)
Cc: devel@edk2.groups.io; Sami Mujawar; Ard Biesheuvel; Leif Lindholm; Matteo Carlini; Ben Adderson; nd
Subject: Re: [edk2-devel] [PATCH v2 1/1] ArmPkg: Fix uninitialised variable in ArmMmuStandaloneMmLib

On Fri, 26 Feb 2021 at 11:58, Sughosh Ganu <sughosh.ganu@linaro.org> wrote:
>
>
> On Thu, 25 Feb 2021 at 22:41, Sami Mujawar <sami.mujawar@arm.com> wrote:
>>
>> The following patches added support for StandaloneMM using FF-A:
>> 9da5ee116a28 ArmPkg: Allow FF-A calls to set memory region's attributes
>> 0e43e02b9bd8 ArmPkg: Allow FF-A calls to get memory region's attributes
>>
>> However, in the error handling logic for the Get/Set Memory attributes,
>> the CLANG compiler reports that a status variable could be used without
>> initialisation. This issue is a false positive and is not seen with GCC.
>>
>> The Get/Set Memory attributes operation is atomic and therefore an
>> FFA_INTERRUPT or FFA_SUCCESS response is not expected in response
>> to FFA_MSG_SEND_DIRECT_REQ. So the remaining cases that could occur
>> are:
>>  - the target sends FFA_MSG_SEND_DIRECT_RESP with a success or
>>    failure code.
>>  or
>>  - FFA_MSG_SEND_DIRECT_REQ transmission failure.
>>
>> Therefore,
>>  - reorder the error handling conditions such that it prevents the
>>    uninitialised variable issue being flagged by CLANG.
>>  - move the repetitive code to a static helper function and add
>>    documentation at the appropriate places.
>>  - fix error handling in functions that invoke GetMemoryPermissions().
>>
>> Signed-off-by: Sami Mujawar <sami.mujawar@arm.com>
>> ---
>> The changes can be seen at:
>> https://github.com/samimujawar/edk2/tree/1657_stmm_ffa_fix_unused_var_v2
>
>
>  Tested the changes on the StandaloneMm image on the Qemu platform.
>
> Tested-by: Sughosh Ganu <sughosh.ganu@linaro.org>
> Reviewed-by: Sughosh Ganu <sughosh.ganu@linaro.org>
>

Thanks. Sami, can you confirm that this patch fixes the CI failure I
reported to you in private? If so, I intend to merge this during the
freeze (assuming Liming is ok with that)


[-- Attachment #2: Type: text/html, Size: 4524 bytes --]

回复: [edk2-devel] [PATCH v2 1/1] ArmPkg: Fix uninitialised variable in ArmMmuStandaloneMmLib

[gaoliming]

Ard:
  This patch fixes the compiler build error. It is a bug fix. I am OK to merge it for this stable tag. 

Thanks
Liming
> -----邮件原件-----
> 发件人: Ard Biesheuvel <ardb@kernel.org>
> 发送时间: 2021年2月27日 2:46
> 收件人: Sughosh Ganu <sughosh.ganu@linaro.org>; Liming Gao (Byosoft
> address) <gaoliming@byosoft.com.cn>
> 抄送: devel@edk2.groups.io; Sami Mujawar <sami.mujawar@arm.com>; Ard
> Biesheuvel <ardb+tianocore@kernel.org>; Leif Lindholm <leif@nuviainc.com>;
> Matteo.Carlini@arm.com; Ben.Adderson@arm.com; nd <nd@arm.com>
> 主题: Re: [edk2-devel] [PATCH v2 1/1] ArmPkg: Fix uninitialised variable in
> ArmMmuStandaloneMmLib
> 
> On Fri, 26 Feb 2021 at 11:58, Sughosh Ganu <sughosh.ganu@linaro.org>
> wrote:
> >
> >
> > On Thu, 25 Feb 2021 at 22:41, Sami Mujawar <sami.mujawar@arm.com>
> wrote:
> >>
> >> The following patches added support for StandaloneMM using FF-A:
> >> 9da5ee116a28 ArmPkg: Allow FF-A calls to set memory region's attributes
> >> 0e43e02b9bd8 ArmPkg: Allow FF-A calls to get memory region's attributes
> >>
> >> However, in the error handling logic for the Get/Set Memory attributes,
> >> the CLANG compiler reports that a status variable could be used without
> >> initialisation. This issue is a false positive and is not seen with GCC.
> >>
> >> The Get/Set Memory attributes operation is atomic and therefore an
> >> FFA_INTERRUPT or FFA_SUCCESS response is not expected in response
> >> to FFA_MSG_SEND_DIRECT_REQ. So the remaining cases that could occur
> >> are:
> >>  - the target sends FFA_MSG_SEND_DIRECT_RESP with a success or
> >>    failure code.
> >>  or
> >>  - FFA_MSG_SEND_DIRECT_REQ transmission failure.
> >>
> >> Therefore,
> >>  - reorder the error handling conditions such that it prevents the
> >>    uninitialised variable issue being flagged by CLANG.
> >>  - move the repetitive code to a static helper function and add
> >>    documentation at the appropriate places.
> >>  - fix error handling in functions that invoke GetMemoryPermissions().
> >>
> >> Signed-off-by: Sami Mujawar <sami.mujawar@arm.com>
> >> ---
> >> The changes can be seen at:
> >>
> https://github.com/samimujawar/edk2/tree/1657_stmm_ffa_fix_unused_var
> _v2
> >
> >
> >  Tested the changes on the StandaloneMm image on the Qemu platform.
> >
> > Tested-by: Sughosh Ganu <sughosh.ganu@linaro.org>
> > Reviewed-by: Sughosh Ganu <sughosh.ganu@linaro.org>
> >
> 
> Thanks. Sami, can you confirm that this patch fixes the CI failure I
> reported to you in private? If so, I intend to merge this during the
> freeze (assuming Liming is ok with that)

Re: [edk2-devel] [PATCH v2 1/1] ArmPkg: Fix uninitialised variable in ArmMmuStandaloneMmLib

[Ard Biesheuvel]

On Sat, 27 Feb 2021 at 10:40, gaoliming <gaoliming@byosoft.com.cn> wrote:
>
> Ard:
>   This patch fixes the compiler build error. It is a bug fix. I am OK to merge it for this stable tag.
>

OK, thanks,

Merged as #1467

> > -----邮件原件-----
> > 发件人: Ard Biesheuvel <ardb@kernel.org>
> > 发送时间: 2021年2月27日 2:46
> > 收件人: Sughosh Ganu <sughosh.ganu@linaro.org>; Liming Gao (Byosoft
> > address) <gaoliming@byosoft.com.cn>
> > 抄送: devel@edk2.groups.io; Sami Mujawar <sami.mujawar@arm.com>; Ard
> > Biesheuvel <ardb+tianocore@kernel.org>; Leif Lindholm <leif@nuviainc.com>;
> > Matteo.Carlini@arm.com; Ben.Adderson@arm.com; nd <nd@arm.com>
> > 主题: Re: [edk2-devel] [PATCH v2 1/1] ArmPkg: Fix uninitialised variable in
> > ArmMmuStandaloneMmLib
> >
> > On Fri, 26 Feb 2021 at 11:58, Sughosh Ganu <sughosh.ganu@linaro.org>
> > wrote:
> > >
> > >
> > > On Thu, 25 Feb 2021 at 22:41, Sami Mujawar <sami.mujawar@arm.com>
> > wrote:
> > >>
> > >> The following patches added support for StandaloneMM using FF-A:
> > >> 9da5ee116a28 ArmPkg: Allow FF-A calls to set memory region's attributes
> > >> 0e43e02b9bd8 ArmPkg: Allow FF-A calls to get memory region's attributes
> > >>
> > >> However, in the error handling logic for the Get/Set Memory attributes,
> > >> the CLANG compiler reports that a status variable could be used without
> > >> initialisation. This issue is a false positive and is not seen with GCC.
> > >>
> > >> The Get/Set Memory attributes operation is atomic and therefore an
> > >> FFA_INTERRUPT or FFA_SUCCESS response is not expected in response
> > >> to FFA_MSG_SEND_DIRECT_REQ. So the remaining cases that could occur
> > >> are:
> > >>  - the target sends FFA_MSG_SEND_DIRECT_RESP with a success or
> > >>    failure code.
> > >>  or
> > >>  - FFA_MSG_SEND_DIRECT_REQ transmission failure.
> > >>
> > >> Therefore,
> > >>  - reorder the error handling conditions such that it prevents the
> > >>    uninitialised variable issue being flagged by CLANG.
> > >>  - move the repetitive code to a static helper function and add
> > >>    documentation at the appropriate places.
> > >>  - fix error handling in functions that invoke GetMemoryPermissions().
> > >>
> > >> Signed-off-by: Sami Mujawar <sami.mujawar@arm.com>
> > >> ---
> > >> The changes can be seen at:
> > >>
> > https://github.com/samimujawar/edk2/tree/1657_stmm_ffa_fix_unused_var
> > _v2
> > >
> > >
> > >  Tested the changes on the StandaloneMm image on the Qemu platform.
> > >
> > > Tested-by: Sughosh Ganu <sughosh.ganu@linaro.org>
> > > Reviewed-by: Sughosh Ganu <sughosh.ganu@linaro.org>
> > >
> >
> > Thanks. Sami, can you confirm that this patch fixes the CI failure I
> > reported to you in private? If so, I intend to merge this during the
> > freeze (assuming Liming is ok with that)
>
>
>
>
> 
>
>