[edk2,1/5] MdeModulePkg/CapsulePei: clean Dcache before consuming capsule data

Message ID 20180607110812.26778-2-ard.biesheuvel@linaro.org
State New
Headers show
Series
  • MdeModulePkg ArmPkg: support for persistent capsules and progress reporting
Related show

Commit Message

Ard Biesheuvel June 7, 2018, 11:08 a.m.
When capsule updates are staged for processing after a warm reboot,
they are copied into memory with the MMU and caches enabled. When
the capsule PEI gets around to coalescing the capsule, the MMU and
caches may still be disabled, and so on architectures where uncached
accesses are incoherent with the caches (such as ARM and AARCH64),
we may read stale data if we don't clean the caches to memory first.

Note that this cache maintenance cannot be done during the invocation
of UpdateCapsule(), since the ScatterGatherList structures are only
identified by physical address, and at runtime, the firmware doesn't
know whether and where this memory is mapped, and cache maintenance
requires a virtual address.

Reviewed-by: Jiewen Yao <jiewen.yao@intel.com>

Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>

---
 MdeModulePkg/Universal/CapsulePei/CapsulePei.inf           |  1 +
 MdeModulePkg/Universal/CapsulePei/Common/CapsuleCoalesce.c | 10 ++++++++++
 2 files changed, 11 insertions(+)

-- 
2.17.0

_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

Comments

Zeng, Star June 8, 2018, 2:53 a.m. | #1
I suggest to use goto/adjust code to have one place for both paths to perform cache maintenance (with comments).


Thanks,
Star
-----Original Message-----
From: Ard Biesheuvel [mailto:ard.biesheuvel@linaro.org] 

Sent: Thursday, June 7, 2018 7:08 PM
To: edk2-devel@lists.01.org
Cc: leif.lindholm@linaro.org; Kinney, Michael D <michael.d.kinney@intel.com>; Yao, Jiewen <jiewen.yao@intel.com>; Zeng, Star <star.zeng@intel.com>; Ard Biesheuvel <ard.biesheuvel@linaro.org>
Subject: [PATCH 1/5] MdeModulePkg/CapsulePei: clean Dcache before consuming capsule data

When capsule updates are staged for processing after a warm reboot, they are copied into memory with the MMU and caches enabled. When the capsule PEI gets around to coalescing the capsule, the MMU and caches may still be disabled, and so on architectures where uncached accesses are incoherent with the caches (such as ARM and AARCH64), we may read stale data if we don't clean the caches to memory first.

Note that this cache maintenance cannot be done during the invocation of UpdateCapsule(), since the ScatterGatherList structures are only identified by physical address, and at runtime, the firmware doesn't know whether and where this memory is mapped, and cache maintenance requires a virtual address.

Reviewed-by: Jiewen Yao <jiewen.yao@intel.com>

Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>

---
 MdeModulePkg/Universal/CapsulePei/CapsulePei.inf           |  1 +
 MdeModulePkg/Universal/CapsulePei/Common/CapsuleCoalesce.c | 10 ++++++++++
 2 files changed, 11 insertions(+)

diff --git a/MdeModulePkg/Universal/CapsulePei/CapsulePei.inf b/MdeModulePkg/Universal/CapsulePei/CapsulePei.inf
index c54bc21a95a8..594e110d1f8a 100644
--- a/MdeModulePkg/Universal/CapsulePei/CapsulePei.inf
+++ b/MdeModulePkg/Universal/CapsulePei/CapsulePei.inf
@@ -48,6 +48,7 @@ [Packages]
 
 [LibraryClasses]
   BaseLib
+  CacheMaintenanceLib
   HobLib
   BaseMemoryLib
   PeiServicesLib
diff --git a/MdeModulePkg/Universal/CapsulePei/Common/CapsuleCoalesce.c b/MdeModulePkg/Universal/CapsulePei/Common/CapsuleCoalesce.c
index 3e7054cd38a9..fb59f338f100 100644
--- a/MdeModulePkg/Universal/CapsulePei/Common/CapsuleCoalesce.c
+++ b/MdeModulePkg/Universal/CapsulePei/Common/CapsuleCoalesce.c
@@ -27,6 +27,7 @@ WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER EXPRESS OR IMPLIED.
 #include <Guid/CapsuleVendor.h>
 
 #include <Library/BaseMemoryLib.h>
+#include <Library/CacheMaintenanceLib.h>
 #include <Library/DebugLib.h>
 #include <Library/PrintLib.h>
 #include <Library/BaseLib.h>
@@ -274,6 +275,7 @@ ValidateCapsuleByMemoryResource (
     //
     // No memory resource descriptor reported in HOB list before capsule Coalesce.
     //
+    WriteBackDataCacheRange ((VOID *)(UINTN)Address, (UINTN)Size);
     return TRUE;
   }
 
@@ -283,6 +285,14 @@ ValidateCapsuleByMemoryResource (
       DEBUG ((EFI_D_INFO, "Address(0x%lx) Size(0x%lx) in MemoryResource[0x%x] - Start(0x%lx) Length(0x%lx)\n",
                           Address, Size,
                           Index, MemoryResource[Index].PhysicalStart, MemoryResource[Index].ResourceLength));
+
+      //
+      // At this point, we may still be running with the MMU and caches disabled,
+      // and on architectures such as ARM or AARCH64, capsule [meta]data loaded
+      // into memory with the caches on is only guaranteed to be visible to the
+      // CPU running with the caches off after performing an explicit writeback.
+      //
+      WriteBackDataCacheRange ((VOID *)(UINTN)Address, (UINTN)Size);
       return TRUE;
     }
   }
--
2.17.0

_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
Ard Biesheuvel June 8, 2018, 6:06 a.m. | #2
On 8 June 2018 at 04:53, Zeng, Star <star.zeng@intel.com> wrote:
> I suggest to use goto/adjust code to have one place for both paths to perform cache maintenance (with comments).

>


Something like this?

@@ -253,6 +254,7 @@ ValidateCapsuleByMemoryResource (
   )
 {
   UINTN             Index;
+  BOOLEAN           Found;

   //
   // Sanity Check
@@ -274,19 +276,32 @@ ValidateCapsuleByMemoryResource (
     //
     // No memory resource descriptor reported in HOB list before
capsule Coalesce.
     //
-    return TRUE;
+    Found = TRUE;
+  } else {
+    Found = FALSE;
   }

-  for (Index = 0; MemoryResource[Index].ResourceLength != 0; Index++) {
+  for (Index = 0; !Found && MemoryResource[Index].ResourceLength !=
0; Index++) {
     if ((Address >= MemoryResource[Index].PhysicalStart) &&
         ((Address + Size) <= (MemoryResource[Index].PhysicalStart +
MemoryResource[Index].ResourceLength))) {
       DEBUG ((EFI_D_INFO, "Address(0x%lx) Size(0x%lx) in
MemoryResource[0x%x] - Start(0x%lx) Length(0x%lx)\n",
                           Address, Size,
                           Index, MemoryResource[Index].PhysicalStart,
MemoryResource[Index].ResourceLength));
-      return TRUE;
+      Found = TRUE;
     }
   }

+  if (Found) {
+    //
+    // At this point, we may still be running with the MMU and caches disabled,
+    // and on architectures such as ARM or AARCH64, capsule [meta]data loaded
+    // into memory with the caches on is only guaranteed to be visible to the
+    // CPU running with the caches off after performing an explicit writeback.
+    //
+    WriteBackDataCacheRange ((VOID *)(UINTN)Address, (UINTN)Size);
+    return TRUE;
+  }
+
   DEBUG ((EFI_D_ERROR, "ERROR: Address(0x%lx) Size(0x%lx) not in any
MemoryResource\n", Address, Size));
   return FALSE;
 }


>

> Thanks,

> Star

> -----Original Message-----

> From: Ard Biesheuvel [mailto:ard.biesheuvel@linaro.org]

> Sent: Thursday, June 7, 2018 7:08 PM

> To: edk2-devel@lists.01.org

> Cc: leif.lindholm@linaro.org; Kinney, Michael D <michael.d.kinney@intel.com>; Yao, Jiewen <jiewen.yao@intel.com>; Zeng, Star <star.zeng@intel.com>; Ard Biesheuvel <ard.biesheuvel@linaro.org>

> Subject: [PATCH 1/5] MdeModulePkg/CapsulePei: clean Dcache before consuming capsule data

>

> When capsule updates are staged for processing after a warm reboot, they are copied into memory with the MMU and caches enabled. When the capsule PEI gets around to coalescing the capsule, the MMU and caches may still be disabled, and so on architectures where uncached accesses are incoherent with the caches (such as ARM and AARCH64), we may read stale data if we don't clean the caches to memory first.

>

> Note that this cache maintenance cannot be done during the invocation of UpdateCapsule(), since the ScatterGatherList structures are only identified by physical address, and at runtime, the firmware doesn't know whether and where this memory is mapped, and cache maintenance requires a virtual address.

>

> Reviewed-by: Jiewen Yao <jiewen.yao@intel.com>

> Contributed-under: TianoCore Contribution Agreement 1.1

> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>

> ---

>  MdeModulePkg/Universal/CapsulePei/CapsulePei.inf           |  1 +

>  MdeModulePkg/Universal/CapsulePei/Common/CapsuleCoalesce.c | 10 ++++++++++

>  2 files changed, 11 insertions(+)

>

> diff --git a/MdeModulePkg/Universal/CapsulePei/CapsulePei.inf b/MdeModulePkg/Universal/CapsulePei/CapsulePei.inf

> index c54bc21a95a8..594e110d1f8a 100644

> --- a/MdeModulePkg/Universal/CapsulePei/CapsulePei.inf

> +++ b/MdeModulePkg/Universal/CapsulePei/CapsulePei.inf

> @@ -48,6 +48,7 @@ [Packages]

>

>  [LibraryClasses]

>    BaseLib

> +  CacheMaintenanceLib

>    HobLib

>    BaseMemoryLib

>    PeiServicesLib

> diff --git a/MdeModulePkg/Universal/CapsulePei/Common/CapsuleCoalesce.c b/MdeModulePkg/Universal/CapsulePei/Common/CapsuleCoalesce.c

> index 3e7054cd38a9..fb59f338f100 100644

> --- a/MdeModulePkg/Universal/CapsulePei/Common/CapsuleCoalesce.c

> +++ b/MdeModulePkg/Universal/CapsulePei/Common/CapsuleCoalesce.c

> @@ -27,6 +27,7 @@ WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER EXPRESS OR IMPLIED.

>  #include <Guid/CapsuleVendor.h>

>

>  #include <Library/BaseMemoryLib.h>

> +#include <Library/CacheMaintenanceLib.h>

>  #include <Library/DebugLib.h>

>  #include <Library/PrintLib.h>

>  #include <Library/BaseLib.h>

> @@ -274,6 +275,7 @@ ValidateCapsuleByMemoryResource (

>      //

>      // No memory resource descriptor reported in HOB list before capsule Coalesce.

>      //

> +    WriteBackDataCacheRange ((VOID *)(UINTN)Address, (UINTN)Size);

>      return TRUE;

>    }

>

> @@ -283,6 +285,14 @@ ValidateCapsuleByMemoryResource (

>        DEBUG ((EFI_D_INFO, "Address(0x%lx) Size(0x%lx) in MemoryResource[0x%x] - Start(0x%lx) Length(0x%lx)\n",

>                            Address, Size,

>                            Index, MemoryResource[Index].PhysicalStart, MemoryResource[Index].ResourceLength));

> +

> +      //

> +      // At this point, we may still be running with the MMU and caches disabled,

> +      // and on architectures such as ARM or AARCH64, capsule [meta]data loaded

> +      // into memory with the caches on is only guaranteed to be visible to the

> +      // CPU running with the caches off after performing an explicit writeback.

> +      //

> +      WriteBackDataCacheRange ((VOID *)(UINTN)Address, (UINTN)Size);

>        return TRUE;

>      }

>    }

> --

> 2.17.0

>

_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
Zeng, Star June 8, 2018, 6:21 a.m. | #3
My thought is like below, FYR.

===================================================
8bf218e00d8bd5c4f01c83f3d16c636140d32fda
 .../Universal/CapsulePei/Common/CapsuleCoalesce.c  | 37 +++++++++++++++-------
 1 file changed, 26 insertions(+), 11 deletions(-)

diff --git a/MdeModulePkg/Universal/CapsulePei/Common/CapsuleCoalesce.c b/MdeModulePkg/Universal/CapsulePei/Common/CapsuleCoalesce.c
index 3e7054cd38a9..da047034c988 100644
--- a/MdeModulePkg/Universal/CapsulePei/Common/CapsuleCoalesce.c
+++ b/MdeModulePkg/Universal/CapsulePei/Common/CapsuleCoalesce.c
@@ -253,6 +253,7 @@ ValidateCapsuleByMemoryResource (
   )
 {
   UINTN             Index;
+  BOOLEAN           Valid;
 
   //
   // Sanity Check
@@ -270,25 +271,39 @@ ValidateCapsuleByMemoryResource (
     return FALSE;
   }
 
+  Valid = FALSE;
   if (MemoryResource == NULL) {
     //
     // No memory resource descriptor reported in HOB list before capsule Coalesce.
     //
-    return TRUE;
+    Valid = TRUE;
+  } else {
+    for (Index = 0; MemoryResource[Index].ResourceLength != 0; Index++) {
+      if ((Address >= MemoryResource[Index].PhysicalStart) &&
+          ((Address + Size) <= (MemoryResource[Index].PhysicalStart + MemoryResource[Index].ResourceLength))) {
+        DEBUG ((EFI_D_INFO, "Address(0x%lx) Size(0x%lx) in MemoryResource[0x%x] - Start(0x%lx) Length(0x%lx)\n",
+                            Address, Size,
+                            Index, MemoryResource[Index].PhysicalStart, MemoryResource[Index].ResourceLength));
+        Valid = TRUE;
+        break;
+      }
+    }
+    if (!Valid) {
+      DEBUG ((EFI_D_ERROR, "ERROR: Address(0x%lx) Size(0x%lx) not in any MemoryResource\n", Address, Size));
+    }
   }
 
-  for (Index = 0; MemoryResource[Index].ResourceLength != 0; Index++) {
-    if ((Address >= MemoryResource[Index].PhysicalStart) &&
-        ((Address + Size) <= (MemoryResource[Index].PhysicalStart + MemoryResource[Index].ResourceLength))) {
-      DEBUG ((EFI_D_INFO, "Address(0x%lx) Size(0x%lx) in MemoryResource[0x%x] - Start(0x%lx) Length(0x%lx)\n",
-                          Address, Size,
-                          Index, MemoryResource[Index].PhysicalStart, MemoryResource[Index].ResourceLength));
-      return TRUE;
-    }
+  if (Valid) {
+    //
+    // At this point, we may still be running with the MMU and caches disabled,
+    // and on architectures such as ARM or AARCH64, capsule [meta]data loaded
+    // into memory with the caches on is only guaranteed to be visible to the
+    // CPU running with the caches off after performing an explicit writeback.
+    //
+    WriteBackDataCacheRange ((VOID *)(UINTN)Address, (UINTN)Size);
   }
 
-  DEBUG ((EFI_D_ERROR, "ERROR: Address(0x%lx) Size(0x%lx) not in any MemoryResource\n", Address, Size));
-  return FALSE;
+  return Valid;
 }
 
 /**
===================================================


Thanks,
Star
-----Original Message-----
From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf Of Ard Biesheuvel

Sent: Friday, June 8, 2018 2:07 PM
To: Zeng, Star <star.zeng@intel.com>
Cc: Kinney, Michael D <michael.d.kinney@intel.com>; edk2-devel@lists.01.org; Yao, Jiewen <jiewen.yao@intel.com>; leif.lindholm@linaro.org
Subject: Re: [edk2] [PATCH 1/5] MdeModulePkg/CapsulePei: clean Dcache before consuming capsule data

On 8 June 2018 at 04:53, Zeng, Star <star.zeng@intel.com> wrote:
> I suggest to use goto/adjust code to have one place for both paths to perform cache maintenance (with comments).

>


Something like this?

@@ -253,6 +254,7 @@ ValidateCapsuleByMemoryResource (
   )
 {
   UINTN             Index;
+  BOOLEAN           Found;

   //
   // Sanity Check
@@ -274,19 +276,32 @@ ValidateCapsuleByMemoryResource (
     //
     // No memory resource descriptor reported in HOB list before capsule Coalesce.
     //
-    return TRUE;
+    Found = TRUE;
+  } else {
+    Found = FALSE;
   }

-  for (Index = 0; MemoryResource[Index].ResourceLength != 0; Index++) {
+  for (Index = 0; !Found && MemoryResource[Index].ResourceLength !=
0; Index++) {
     if ((Address >= MemoryResource[Index].PhysicalStart) &&
         ((Address + Size) <= (MemoryResource[Index].PhysicalStart +
MemoryResource[Index].ResourceLength))) {
       DEBUG ((EFI_D_INFO, "Address(0x%lx) Size(0x%lx) in MemoryResource[0x%x] - Start(0x%lx) Length(0x%lx)\n",
                           Address, Size,
                           Index, MemoryResource[Index].PhysicalStart,
MemoryResource[Index].ResourceLength));
-      return TRUE;
+      Found = TRUE;
     }
   }

+  if (Found) {
+    //
+    // At this point, we may still be running with the MMU and caches disabled,
+    // and on architectures such as ARM or AARCH64, capsule [meta]data loaded
+    // into memory with the caches on is only guaranteed to be visible to the
+    // CPU running with the caches off after performing an explicit writeback.
+    //
+    WriteBackDataCacheRange ((VOID *)(UINTN)Address, (UINTN)Size);
+    return TRUE;
+  }
+
   DEBUG ((EFI_D_ERROR, "ERROR: Address(0x%lx) Size(0x%lx) not in any MemoryResource\n", Address, Size));
   return FALSE;
 }


>

> Thanks,

> Star

> -----Original Message-----

> From: Ard Biesheuvel [mailto:ard.biesheuvel@linaro.org]

> Sent: Thursday, June 7, 2018 7:08 PM

> To: edk2-devel@lists.01.org

> Cc: leif.lindholm@linaro.org; Kinney, Michael D 

> <michael.d.kinney@intel.com>; Yao, Jiewen <jiewen.yao@intel.com>; 

> Zeng, Star <star.zeng@intel.com>; Ard Biesheuvel 

> <ard.biesheuvel@linaro.org>

> Subject: [PATCH 1/5] MdeModulePkg/CapsulePei: clean Dcache before 

> consuming capsule data

>

> When capsule updates are staged for processing after a warm reboot, they are copied into memory with the MMU and caches enabled. When the capsule PEI gets around to coalescing the capsule, the MMU and caches may still be disabled, and so on architectures where uncached accesses are incoherent with the caches (such as ARM and AARCH64), we may read stale data if we don't clean the caches to memory first.

>

> Note that this cache maintenance cannot be done during the invocation of UpdateCapsule(), since the ScatterGatherList structures are only identified by physical address, and at runtime, the firmware doesn't know whether and where this memory is mapped, and cache maintenance requires a virtual address.

>

> Reviewed-by: Jiewen Yao <jiewen.yao@intel.com>

> Contributed-under: TianoCore Contribution Agreement 1.1

> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>

> ---

>  MdeModulePkg/Universal/CapsulePei/CapsulePei.inf           |  1 +

>  MdeModulePkg/Universal/CapsulePei/Common/CapsuleCoalesce.c | 10 

> ++++++++++

>  2 files changed, 11 insertions(+)

>

> diff --git a/MdeModulePkg/Universal/CapsulePei/CapsulePei.inf 

> b/MdeModulePkg/Universal/CapsulePei/CapsulePei.inf

> index c54bc21a95a8..594e110d1f8a 100644

> --- a/MdeModulePkg/Universal/CapsulePei/CapsulePei.inf

> +++ b/MdeModulePkg/Universal/CapsulePei/CapsulePei.inf

> @@ -48,6 +48,7 @@ [Packages]

>

>  [LibraryClasses]

>    BaseLib

> +  CacheMaintenanceLib

>    HobLib

>    BaseMemoryLib

>    PeiServicesLib

> diff --git 

> a/MdeModulePkg/Universal/CapsulePei/Common/CapsuleCoalesce.c 

> b/MdeModulePkg/Universal/CapsulePei/Common/CapsuleCoalesce.c

> index 3e7054cd38a9..fb59f338f100 100644

> --- a/MdeModulePkg/Universal/CapsulePei/Common/CapsuleCoalesce.c

> +++ b/MdeModulePkg/Universal/CapsulePei/Common/CapsuleCoalesce.c

> @@ -27,6 +27,7 @@ WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER EXPRESS OR IMPLIED.

>  #include <Guid/CapsuleVendor.h>

>

>  #include <Library/BaseMemoryLib.h>

> +#include <Library/CacheMaintenanceLib.h>

>  #include <Library/DebugLib.h>

>  #include <Library/PrintLib.h>

>  #include <Library/BaseLib.h>

> @@ -274,6 +275,7 @@ ValidateCapsuleByMemoryResource (

>      //

>      // No memory resource descriptor reported in HOB list before capsule Coalesce.

>      //

> +    WriteBackDataCacheRange ((VOID *)(UINTN)Address, (UINTN)Size);

>      return TRUE;

>    }

>

> @@ -283,6 +285,14 @@ ValidateCapsuleByMemoryResource (

>        DEBUG ((EFI_D_INFO, "Address(0x%lx) Size(0x%lx) in MemoryResource[0x%x] - Start(0x%lx) Length(0x%lx)\n",

>                            Address, Size,

>                            Index, MemoryResource[Index].PhysicalStart, 

> MemoryResource[Index].ResourceLength));

> +

> +      //

> +      // At this point, we may still be running with the MMU and caches disabled,

> +      // and on architectures such as ARM or AARCH64, capsule [meta]data loaded

> +      // into memory with the caches on is only guaranteed to be visible to the

> +      // CPU running with the caches off after performing an explicit writeback.

> +      //

> +      WriteBackDataCacheRange ((VOID *)(UINTN)Address, (UINTN)Size);

>        return TRUE;

>      }

>    }

> --

> 2.17.0

>

_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
Ard Biesheuvel June 8, 2018, 6:24 a.m. | #4
On 8 June 2018 at 08:21, Zeng, Star <star.zeng@intel.com> wrote:
> My thought is like below, FYR.

>


Thanks. That works for me.

I will update the patch.

> ===================================================

> 8bf218e00d8bd5c4f01c83f3d16c636140d32fda

>  .../Universal/CapsulePei/Common/CapsuleCoalesce.c  | 37 +++++++++++++++-------

>  1 file changed, 26 insertions(+), 11 deletions(-)

>

> diff --git a/MdeModulePkg/Universal/CapsulePei/Common/CapsuleCoalesce.c b/MdeModulePkg/Universal/CapsulePei/Common/CapsuleCoalesce.c

> index 3e7054cd38a9..da047034c988 100644

> --- a/MdeModulePkg/Universal/CapsulePei/Common/CapsuleCoalesce.c

> +++ b/MdeModulePkg/Universal/CapsulePei/Common/CapsuleCoalesce.c

> @@ -253,6 +253,7 @@ ValidateCapsuleByMemoryResource (

>    )

>  {

>    UINTN             Index;

> +  BOOLEAN           Valid;

>

>    //

>    // Sanity Check

> @@ -270,25 +271,39 @@ ValidateCapsuleByMemoryResource (

>      return FALSE;

>    }

>

> +  Valid = FALSE;

>    if (MemoryResource == NULL) {

>      //

>      // No memory resource descriptor reported in HOB list before capsule Coalesce.

>      //

> -    return TRUE;

> +    Valid = TRUE;

> +  } else {

> +    for (Index = 0; MemoryResource[Index].ResourceLength != 0; Index++) {

> +      if ((Address >= MemoryResource[Index].PhysicalStart) &&

> +          ((Address + Size) <= (MemoryResource[Index].PhysicalStart + MemoryResource[Index].ResourceLength))) {

> +        DEBUG ((EFI_D_INFO, "Address(0x%lx) Size(0x%lx) in MemoryResource[0x%x] - Start(0x%lx) Length(0x%lx)\n",

> +                            Address, Size,

> +                            Index, MemoryResource[Index].PhysicalStart, MemoryResource[Index].ResourceLength));

> +        Valid = TRUE;

> +        break;

> +      }

> +    }

> +    if (!Valid) {

> +      DEBUG ((EFI_D_ERROR, "ERROR: Address(0x%lx) Size(0x%lx) not in any MemoryResource\n", Address, Size));

> +    }

>    }

>

> -  for (Index = 0; MemoryResource[Index].ResourceLength != 0; Index++) {

> -    if ((Address >= MemoryResource[Index].PhysicalStart) &&

> -        ((Address + Size) <= (MemoryResource[Index].PhysicalStart + MemoryResource[Index].ResourceLength))) {

> -      DEBUG ((EFI_D_INFO, "Address(0x%lx) Size(0x%lx) in MemoryResource[0x%x] - Start(0x%lx) Length(0x%lx)\n",

> -                          Address, Size,

> -                          Index, MemoryResource[Index].PhysicalStart, MemoryResource[Index].ResourceLength));

> -      return TRUE;

> -    }

> +  if (Valid) {

> +    //

> +    // At this point, we may still be running with the MMU and caches disabled,

> +    // and on architectures such as ARM or AARCH64, capsule [meta]data loaded

> +    // into memory with the caches on is only guaranteed to be visible to the

> +    // CPU running with the caches off after performing an explicit writeback.

> +    //

> +    WriteBackDataCacheRange ((VOID *)(UINTN)Address, (UINTN)Size);

>    }

>

> -  DEBUG ((EFI_D_ERROR, "ERROR: Address(0x%lx) Size(0x%lx) not in any MemoryResource\n", Address, Size));

> -  return FALSE;

> +  return Valid;

>  }

>

>  /**

> ===================================================

>

>

> Thanks,

> Star

> -----Original Message-----

> From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf Of Ard Biesheuvel

> Sent: Friday, June 8, 2018 2:07 PM

> To: Zeng, Star <star.zeng@intel.com>

> Cc: Kinney, Michael D <michael.d.kinney@intel.com>; edk2-devel@lists.01.org; Yao, Jiewen <jiewen.yao@intel.com>; leif.lindholm@linaro.org

> Subject: Re: [edk2] [PATCH 1/5] MdeModulePkg/CapsulePei: clean Dcache before consuming capsule data

>

> On 8 June 2018 at 04:53, Zeng, Star <star.zeng@intel.com> wrote:

>> I suggest to use goto/adjust code to have one place for both paths to perform cache maintenance (with comments).

>>

>

> Something like this?

>

> @@ -253,6 +254,7 @@ ValidateCapsuleByMemoryResource (

>    )

>  {

>    UINTN             Index;

> +  BOOLEAN           Found;

>

>    //

>    // Sanity Check

> @@ -274,19 +276,32 @@ ValidateCapsuleByMemoryResource (

>      //

>      // No memory resource descriptor reported in HOB list before capsule Coalesce.

>      //

> -    return TRUE;

> +    Found = TRUE;

> +  } else {

> +    Found = FALSE;

>    }

>

> -  for (Index = 0; MemoryResource[Index].ResourceLength != 0; Index++) {

> +  for (Index = 0; !Found && MemoryResource[Index].ResourceLength !=

> 0; Index++) {

>      if ((Address >= MemoryResource[Index].PhysicalStart) &&

>          ((Address + Size) <= (MemoryResource[Index].PhysicalStart +

> MemoryResource[Index].ResourceLength))) {

>        DEBUG ((EFI_D_INFO, "Address(0x%lx) Size(0x%lx) in MemoryResource[0x%x] - Start(0x%lx) Length(0x%lx)\n",

>                            Address, Size,

>                            Index, MemoryResource[Index].PhysicalStart,

> MemoryResource[Index].ResourceLength));

> -      return TRUE;

> +      Found = TRUE;

>      }

>    }

>

> +  if (Found) {

> +    //

> +    // At this point, we may still be running with the MMU and caches disabled,

> +    // and on architectures such as ARM or AARCH64, capsule [meta]data loaded

> +    // into memory with the caches on is only guaranteed to be visible to the

> +    // CPU running with the caches off after performing an explicit writeback.

> +    //

> +    WriteBackDataCacheRange ((VOID *)(UINTN)Address, (UINTN)Size);

> +    return TRUE;

> +  }

> +

>    DEBUG ((EFI_D_ERROR, "ERROR: Address(0x%lx) Size(0x%lx) not in any MemoryResource\n", Address, Size));

>    return FALSE;

>  }

>

>

>>

>> Thanks,

>> Star

>> -----Original Message-----

>> From: Ard Biesheuvel [mailto:ard.biesheuvel@linaro.org]

>> Sent: Thursday, June 7, 2018 7:08 PM

>> To: edk2-devel@lists.01.org

>> Cc: leif.lindholm@linaro.org; Kinney, Michael D

>> <michael.d.kinney@intel.com>; Yao, Jiewen <jiewen.yao@intel.com>;

>> Zeng, Star <star.zeng@intel.com>; Ard Biesheuvel

>> <ard.biesheuvel@linaro.org>

>> Subject: [PATCH 1/5] MdeModulePkg/CapsulePei: clean Dcache before

>> consuming capsule data

>>

>> When capsule updates are staged for processing after a warm reboot, they are copied into memory with the MMU and caches enabled. When the capsule PEI gets around to coalescing the capsule, the MMU and caches may still be disabled, and so on architectures where uncached accesses are incoherent with the caches (such as ARM and AARCH64), we may read stale data if we don't clean the caches to memory first.

>>

>> Note that this cache maintenance cannot be done during the invocation of UpdateCapsule(), since the ScatterGatherList structures are only identified by physical address, and at runtime, the firmware doesn't know whether and where this memory is mapped, and cache maintenance requires a virtual address.

>>

>> Reviewed-by: Jiewen Yao <jiewen.yao@intel.com>

>> Contributed-under: TianoCore Contribution Agreement 1.1

>> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>

>> ---

>>  MdeModulePkg/Universal/CapsulePei/CapsulePei.inf           |  1 +

>>  MdeModulePkg/Universal/CapsulePei/Common/CapsuleCoalesce.c | 10

>> ++++++++++

>>  2 files changed, 11 insertions(+)

>>

>> diff --git a/MdeModulePkg/Universal/CapsulePei/CapsulePei.inf

>> b/MdeModulePkg/Universal/CapsulePei/CapsulePei.inf

>> index c54bc21a95a8..594e110d1f8a 100644

>> --- a/MdeModulePkg/Universal/CapsulePei/CapsulePei.inf

>> +++ b/MdeModulePkg/Universal/CapsulePei/CapsulePei.inf

>> @@ -48,6 +48,7 @@ [Packages]

>>

>>  [LibraryClasses]

>>    BaseLib

>> +  CacheMaintenanceLib

>>    HobLib

>>    BaseMemoryLib

>>    PeiServicesLib

>> diff --git

>> a/MdeModulePkg/Universal/CapsulePei/Common/CapsuleCoalesce.c

>> b/MdeModulePkg/Universal/CapsulePei/Common/CapsuleCoalesce.c

>> index 3e7054cd38a9..fb59f338f100 100644

>> --- a/MdeModulePkg/Universal/CapsulePei/Common/CapsuleCoalesce.c

>> +++ b/MdeModulePkg/Universal/CapsulePei/Common/CapsuleCoalesce.c

>> @@ -27,6 +27,7 @@ WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER EXPRESS OR IMPLIED.

>>  #include <Guid/CapsuleVendor.h>

>>

>>  #include <Library/BaseMemoryLib.h>

>> +#include <Library/CacheMaintenanceLib.h>

>>  #include <Library/DebugLib.h>

>>  #include <Library/PrintLib.h>

>>  #include <Library/BaseLib.h>

>> @@ -274,6 +275,7 @@ ValidateCapsuleByMemoryResource (

>>      //

>>      // No memory resource descriptor reported in HOB list before capsule Coalesce.

>>      //

>> +    WriteBackDataCacheRange ((VOID *)(UINTN)Address, (UINTN)Size);

>>      return TRUE;

>>    }

>>

>> @@ -283,6 +285,14 @@ ValidateCapsuleByMemoryResource (

>>        DEBUG ((EFI_D_INFO, "Address(0x%lx) Size(0x%lx) in MemoryResource[0x%x] - Start(0x%lx) Length(0x%lx)\n",

>>                            Address, Size,

>>                            Index, MemoryResource[Index].PhysicalStart,

>> MemoryResource[Index].ResourceLength));

>> +

>> +      //

>> +      // At this point, we may still be running with the MMU and caches disabled,

>> +      // and on architectures such as ARM or AARCH64, capsule [meta]data loaded

>> +      // into memory with the caches on is only guaranteed to be visible to the

>> +      // CPU running with the caches off after performing an explicit writeback.

>> +      //

>> +      WriteBackDataCacheRange ((VOID *)(UINTN)Address, (UINTN)Size);

>>        return TRUE;

>>      }

>>    }

>> --

>> 2.17.0

>>

> _______________________________________________

> edk2-devel mailing list

> edk2-devel@lists.01.org

> https://lists.01.org/mailman/listinfo/edk2-devel

_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

Patch

diff --git a/MdeModulePkg/Universal/CapsulePei/CapsulePei.inf b/MdeModulePkg/Universal/CapsulePei/CapsulePei.inf
index c54bc21a95a8..594e110d1f8a 100644
--- a/MdeModulePkg/Universal/CapsulePei/CapsulePei.inf
+++ b/MdeModulePkg/Universal/CapsulePei/CapsulePei.inf
@@ -48,6 +48,7 @@  [Packages]
 
 [LibraryClasses]
   BaseLib
+  CacheMaintenanceLib
   HobLib
   BaseMemoryLib
   PeiServicesLib
diff --git a/MdeModulePkg/Universal/CapsulePei/Common/CapsuleCoalesce.c b/MdeModulePkg/Universal/CapsulePei/Common/CapsuleCoalesce.c
index 3e7054cd38a9..fb59f338f100 100644
--- a/MdeModulePkg/Universal/CapsulePei/Common/CapsuleCoalesce.c
+++ b/MdeModulePkg/Universal/CapsulePei/Common/CapsuleCoalesce.c
@@ -27,6 +27,7 @@  WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER EXPRESS OR IMPLIED.
 #include <Guid/CapsuleVendor.h>
 
 #include <Library/BaseMemoryLib.h>
+#include <Library/CacheMaintenanceLib.h>
 #include <Library/DebugLib.h>
 #include <Library/PrintLib.h>
 #include <Library/BaseLib.h>
@@ -274,6 +275,7 @@  ValidateCapsuleByMemoryResource (
     //
     // No memory resource descriptor reported in HOB list before capsule Coalesce.
     //
+    WriteBackDataCacheRange ((VOID *)(UINTN)Address, (UINTN)Size);
     return TRUE;
   }
 
@@ -283,6 +285,14 @@  ValidateCapsuleByMemoryResource (
       DEBUG ((EFI_D_INFO, "Address(0x%lx) Size(0x%lx) in MemoryResource[0x%x] - Start(0x%lx) Length(0x%lx)\n",
                           Address, Size,
                           Index, MemoryResource[Index].PhysicalStart, MemoryResource[Index].ResourceLength));
+
+      //
+      // At this point, we may still be running with the MMU and caches disabled,
+      // and on architectures such as ARM or AARCH64, capsule [meta]data loaded
+      // into memory with the caches on is only guaranteed to be visible to the
+      // CPU running with the caches off after performing an explicit writeback.
+      //
+      WriteBackDataCacheRange ((VOID *)(UINTN)Address, (UINTN)Size);
       return TRUE;
     }
   }