[PATCH 1/1] ArmPlatformPkg/PrePeiCore: replace set/way cache ops with by-VA ones

[PATCH 1/1] ArmPlatformPkg/PrePeiCore: replace set/way cache ops with by-VA ones

[Ard Biesheuvel]

Cache maintenance operations by set/way are only intended to be used
in the context of on/offlining a core, while it has been taken out of
the coherency domain. Any use intended to ensure that the contents of
the cache have made it to main memory is unreliable, since cacheline
migration and non-architected system caches may cause these contents
to linger elsewhere, without being visible in main memory once the
MMU and caches are disabled.

In KVM on Linux, there are horrid hacks in place to ensure that such
set/way operations are trapped, and replaced with a single by-VA
clean/invalidate of the entire guest VA space once the MMU state
changes, which can be costly, and is unnecessary if we manage the
caches a bit more carefully, and perform maintenance by virtual
address only.

So let's get rid of the call to ArmInvalidateDataCache () in the
PrePeiCore startup code, and instead, invalidate the temporary RAM
region by virtual address, which is the only memory region we will
be touching with the caches and MMU both disabled and enabled.
(This will lead to data corruption if data written with the MMU off
is shadowed by clean, stale cachelines that stick around when the
MMU is enabled again.)

Signed-off-by: Ard Biesheuvel <ard.biesheuvel@arm.com>
---
Tested on bare metal (SynQuacer 32-bit) and KVM (mach-virt 64-bit)

 ArmPlatformPkg/PrePeiCore/PrePeiCore.c          | 6 ++++--
 ArmPlatformPkg/PrePeiCore/PrePeiCoreMPCore.inf  | 1 +
 ArmPlatformPkg/PrePeiCore/PrePeiCoreUniCore.inf | 1 +
 3 files changed, 6 insertions(+), 2 deletions(-)

diff --git a/ArmPlatformPkg/PrePeiCore/PrePeiCore.c b/ArmPlatformPkg/PrePeiCore/PrePeiCore.c
index 4911f67577a2..a7a61fe9ddb5 100644
--- a/ArmPlatformPkg/PrePeiCore/PrePeiCore.c
+++ b/ArmPlatformPkg/PrePeiCore/PrePeiCore.c
@@ -8,6 +8,7 @@
 **/
 
 #include <Library/BaseLib.h>
+#include <Library/CacheMaintenanceLib.h>
 #include <Library/DebugAgentLib.h>
 #include <Library/ArmLib.h>
 
@@ -59,13 +60,14 @@ CEntryPoint (
 {
   // Data Cache enabled on Primary core when MMU is enabled.
   ArmDisableDataCache ();
-  // Invalidate Data cache
-  ArmInvalidateDataCache ();
   // Invalidate instruction cache
   ArmInvalidateInstructionCache ();
   // Enable Instruction Caches on all cores.
   ArmEnableInstructionCache ();
 
+  InvalidateDataCacheRange ((VOID *)(UINTN)PcdGet64 (PcdCPUCoresStackBase),
+                            PcdGet32 (PcdCPUCorePrimaryStackSize));
+
   //
   // Note: Doesn't have to Enable CPU interface in non-secure world,
   // as Non-secure interface is already enabled in Secure world.
diff --git a/ArmPlatformPkg/PrePeiCore/PrePeiCoreMPCore.inf b/ArmPlatformPkg/PrePeiCore/PrePeiCoreMPCore.inf
index f2ac45d171bc..9d90d46dcfc5 100644
--- a/ArmPlatformPkg/PrePeiCore/PrePeiCoreMPCore.inf
+++ b/ArmPlatformPkg/PrePeiCore/PrePeiCoreMPCore.inf
@@ -44,6 +44,7 @@ [Packages]
 [LibraryClasses]
   ArmLib
   ArmPlatformLib
+  CacheMaintenanceLib
   BaseLib
   DebugLib
   DebugAgentLib
diff --git a/ArmPlatformPkg/PrePeiCore/PrePeiCoreUniCore.inf b/ArmPlatformPkg/PrePeiCore/PrePeiCoreUniCore.inf
index 84c319c3679b..0749a6d575cf 100644
--- a/ArmPlatformPkg/PrePeiCore/PrePeiCoreUniCore.inf
+++ b/ArmPlatformPkg/PrePeiCore/PrePeiCoreUniCore.inf
@@ -44,6 +44,7 @@ [Packages]
 [LibraryClasses]
   ArmLib
   ArmPlatformLib
+  CacheMaintenanceLib
   BaseLib
   DebugLib
   DebugAgentLib
-- 
2.17.1

Re: [edk2-devel] [PATCH 1/1] ArmPlatformPkg/PrePeiCore: replace set/way cache ops with by-VA ones

[Leif Lindholm]

On Fri, Feb 21, 2020 at 12:07:14 +0100, Ard Biesheuvel wrote:
> Cache maintenance operations by set/way are only intended to be used
> in the context of on/offlining a core, while it has been taken out of
> the coherency domain. Any use intended to ensure that the contents of
> the cache have made it to main memory is unreliable, since cacheline
> migration and non-architected system caches may cause these contents
> to linger elsewhere, without being visible in main memory once the
> MMU and caches are disabled.
> 
> In KVM on Linux, there are horrid hacks in place to ensure that such
> set/way operations are trapped, and replaced with a single by-VA
> clean/invalidate of the entire guest VA space once the MMU state
> changes, which can be costly, and is unnecessary if we manage the
> caches a bit more carefully, and perform maintenance by virtual
> address only.
> 
> So let's get rid of the call to ArmInvalidateDataCache () in the
> PrePeiCore startup code, and instead, invalidate the temporary RAM
> region by virtual address, which is the only memory region we will
> be touching with the caches and MMU both disabled and enabled.
> (This will lead to data corruption if data written with the MMU off
> is shadowed by clean, stale cachelines that stick around when the
> MMU is enabled again.)
> 
> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@arm.com>

In a world where we have Trusted Firmware, this looks like a bugfix
to me. But I still won't suggest we include it in the upcoming stable
tag - so please hold off pushing it until the tag has been made :)

Reviewed-by: Leif Lindholm <leif@nuviainc.com>

> ---
> Tested on bare metal (SynQuacer 32-bit) and KVM (mach-virt 64-bit)
> 
>  ArmPlatformPkg/PrePeiCore/PrePeiCore.c          | 6 ++++--
>  ArmPlatformPkg/PrePeiCore/PrePeiCoreMPCore.inf  | 1 +
>  ArmPlatformPkg/PrePeiCore/PrePeiCoreUniCore.inf | 1 +
>  3 files changed, 6 insertions(+), 2 deletions(-)
> 
> diff --git a/ArmPlatformPkg/PrePeiCore/PrePeiCore.c b/ArmPlatformPkg/PrePeiCore/PrePeiCore.c
> index 4911f67577a2..a7a61fe9ddb5 100644
> --- a/ArmPlatformPkg/PrePeiCore/PrePeiCore.c
> +++ b/ArmPlatformPkg/PrePeiCore/PrePeiCore.c
> @@ -8,6 +8,7 @@
>  **/
>  
>  #include <Library/BaseLib.h>
> +#include <Library/CacheMaintenanceLib.h>
>  #include <Library/DebugAgentLib.h>
>  #include <Library/ArmLib.h>
>  
> @@ -59,13 +60,14 @@ CEntryPoint (
>  {
>    // Data Cache enabled on Primary core when MMU is enabled.
>    ArmDisableDataCache ();
> -  // Invalidate Data cache
> -  ArmInvalidateDataCache ();
>    // Invalidate instruction cache
>    ArmInvalidateInstructionCache ();
>    // Enable Instruction Caches on all cores.
>    ArmEnableInstructionCache ();
>  
> +  InvalidateDataCacheRange ((VOID *)(UINTN)PcdGet64 (PcdCPUCoresStackBase),
> +                            PcdGet32 (PcdCPUCorePrimaryStackSize));
> +
>    //
>    // Note: Doesn't have to Enable CPU interface in non-secure world,
>    // as Non-secure interface is already enabled in Secure world.
> diff --git a/ArmPlatformPkg/PrePeiCore/PrePeiCoreMPCore.inf b/ArmPlatformPkg/PrePeiCore/PrePeiCoreMPCore.inf
> index f2ac45d171bc..9d90d46dcfc5 100644
> --- a/ArmPlatformPkg/PrePeiCore/PrePeiCoreMPCore.inf
> +++ b/ArmPlatformPkg/PrePeiCore/PrePeiCoreMPCore.inf
> @@ -44,6 +44,7 @@ [Packages]
>  [LibraryClasses]
>    ArmLib
>    ArmPlatformLib
> +  CacheMaintenanceLib
>    BaseLib
>    DebugLib
>    DebugAgentLib
> diff --git a/ArmPlatformPkg/PrePeiCore/PrePeiCoreUniCore.inf b/ArmPlatformPkg/PrePeiCore/PrePeiCoreUniCore.inf
> index 84c319c3679b..0749a6d575cf 100644
> --- a/ArmPlatformPkg/PrePeiCore/PrePeiCoreUniCore.inf
> +++ b/ArmPlatformPkg/PrePeiCore/PrePeiCoreUniCore.inf
> @@ -44,6 +44,7 @@ [Packages]
>  [LibraryClasses]
>    ArmLib
>    ArmPlatformLib
> +  CacheMaintenanceLib
>    BaseLib
>    DebugLib
>    DebugAgentLib
> -- 
> 2.17.1
> 
> 
> 
> 

Re: [PATCH 1/1] ArmPlatformPkg/PrePeiCore: replace set/way cache ops with by-VA ones

[Laszlo Ersek]

On 02/21/20 12:07, Ard Biesheuvel wrote:
> Cache maintenance operations by set/way are only intended to be used
> in the context of on/offlining a core, while it has been taken out of
> the coherency domain. Any use intended to ensure that the contents of
> the cache have made it to main memory is unreliable, since cacheline
> migration and non-architected system caches may cause these contents
> to linger elsewhere, without being visible in main memory once the
> MMU and caches are disabled.
> 
> In KVM on Linux, there are horrid hacks in place to ensure that such
> set/way operations are trapped, and replaced with a single by-VA
> clean/invalidate of the entire guest VA space once the MMU state
> changes, which can be costly, and is unnecessary if we manage the
> caches a bit more carefully, and perform maintenance by virtual
> address only.
> 
> So let's get rid of the call to ArmInvalidateDataCache () in the
> PrePeiCore startup code, and instead, invalidate the temporary RAM
> region by virtual address, which is the only memory region we will
> be touching with the caches and MMU both disabled and enabled.
> (This will lead to data corruption if data written with the MMU off
> is shadowed by clean, stale cachelines that stick around when the
> MMU is enabled again.)
> 
> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@arm.com>
> ---
> Tested on bare metal (SynQuacer 32-bit) and KVM (mach-virt 64-bit)
> 
>  ArmPlatformPkg/PrePeiCore/PrePeiCore.c          | 6 ++++--
>  ArmPlatformPkg/PrePeiCore/PrePeiCoreMPCore.inf  | 1 +
>  ArmPlatformPkg/PrePeiCore/PrePeiCoreUniCore.inf | 1 +
>  3 files changed, 6 insertions(+), 2 deletions(-)
> 
> diff --git a/ArmPlatformPkg/PrePeiCore/PrePeiCore.c b/ArmPlatformPkg/PrePeiCore/PrePeiCore.c
> index 4911f67577a2..a7a61fe9ddb5 100644
> --- a/ArmPlatformPkg/PrePeiCore/PrePeiCore.c
> +++ b/ArmPlatformPkg/PrePeiCore/PrePeiCore.c
> @@ -8,6 +8,7 @@
>  **/
>  
>  #include <Library/BaseLib.h>
> +#include <Library/CacheMaintenanceLib.h>
>  #include <Library/DebugAgentLib.h>
>  #include <Library/ArmLib.h>
>  
> @@ -59,13 +60,14 @@ CEntryPoint (
>  {
>    // Data Cache enabled on Primary core when MMU is enabled.
>    ArmDisableDataCache ();
> -  // Invalidate Data cache
> -  ArmInvalidateDataCache ();
>    // Invalidate instruction cache
>    ArmInvalidateInstructionCache ();
>    // Enable Instruction Caches on all cores.
>    ArmEnableInstructionCache ();
>  
> +  InvalidateDataCacheRange ((VOID *)(UINTN)PcdGet64 (PcdCPUCoresStackBase),
> +                            PcdGet32 (PcdCPUCorePrimaryStackSize));
> +
>    //
>    // Note: Doesn't have to Enable CPU interface in non-secure world,
>    // as Non-secure interface is already enabled in Secure world.
> diff --git a/ArmPlatformPkg/PrePeiCore/PrePeiCoreMPCore.inf b/ArmPlatformPkg/PrePeiCore/PrePeiCoreMPCore.inf
> index f2ac45d171bc..9d90d46dcfc5 100644
> --- a/ArmPlatformPkg/PrePeiCore/PrePeiCoreMPCore.inf
> +++ b/ArmPlatformPkg/PrePeiCore/PrePeiCoreMPCore.inf
> @@ -44,6 +44,7 @@ [Packages]
>  [LibraryClasses]
>    ArmLib
>    ArmPlatformLib
> +  CacheMaintenanceLib
>    BaseLib
>    DebugLib
>    DebugAgentLib
> diff --git a/ArmPlatformPkg/PrePeiCore/PrePeiCoreUniCore.inf b/ArmPlatformPkg/PrePeiCore/PrePeiCoreUniCore.inf
> index 84c319c3679b..0749a6d575cf 100644
> --- a/ArmPlatformPkg/PrePeiCore/PrePeiCoreUniCore.inf
> +++ b/ArmPlatformPkg/PrePeiCore/PrePeiCoreUniCore.inf
> @@ -44,6 +44,7 @@ [Packages]
>  [LibraryClasses]
>    ArmLib
>    ArmPlatformLib
> +  CacheMaintenanceLib
>    BaseLib
>    DebugLib
>    DebugAgentLib
> 

Acked-by: Laszlo Ersek <lersek@redhat.com>

Re: [edk2-devel] [PATCH 1/1] ArmPlatformPkg/PrePeiCore: replace set/way cache ops with by-VA ones

[Ard Biesheuvel]

On Fri, 21 Feb 2020 at 16:54, Laszlo Ersek <lersek@redhat.com> wrote:
>
> On 02/21/20 12:07, Ard Biesheuvel wrote:
> > Cache maintenance operations by set/way are only intended to be used
> > in the context of on/offlining a core, while it has been taken out of
> > the coherency domain. Any use intended to ensure that the contents of
> > the cache have made it to main memory is unreliable, since cacheline
> > migration and non-architected system caches may cause these contents
> > to linger elsewhere, without being visible in main memory once the
> > MMU and caches are disabled.
> >
> > In KVM on Linux, there are horrid hacks in place to ensure that such
> > set/way operations are trapped, and replaced with a single by-VA
> > clean/invalidate of the entire guest VA space once the MMU state
> > changes, which can be costly, and is unnecessary if we manage the
> > caches a bit more carefully, and perform maintenance by virtual
> > address only.
> >
> > So let's get rid of the call to ArmInvalidateDataCache () in the
> > PrePeiCore startup code, and instead, invalidate the temporary RAM
> > region by virtual address, which is the only memory region we will
> > be touching with the caches and MMU both disabled and enabled.
> > (This will lead to data corruption if data written with the MMU off
> > is shadowed by clean, stale cachelines that stick around when the
> > MMU is enabled again.)
> >
> > Signed-off-by: Ard Biesheuvel <ard.biesheuvel@arm.com>
> > ---
> > Tested on bare metal (SynQuacer 32-bit) and KVM (mach-virt 64-bit)
> >
> >  ArmPlatformPkg/PrePeiCore/PrePeiCore.c          | 6 ++++--
> >  ArmPlatformPkg/PrePeiCore/PrePeiCoreMPCore.inf  | 1 +
> >  ArmPlatformPkg/PrePeiCore/PrePeiCoreUniCore.inf | 1 +
> >  3 files changed, 6 insertions(+), 2 deletions(-)
> >
> > diff --git a/ArmPlatformPkg/PrePeiCore/PrePeiCore.c b/ArmPlatformPkg/PrePeiCore/PrePeiCore.c
> > index 4911f67577a2..a7a61fe9ddb5 100644
> > --- a/ArmPlatformPkg/PrePeiCore/PrePeiCore.c
> > +++ b/ArmPlatformPkg/PrePeiCore/PrePeiCore.c
> > @@ -8,6 +8,7 @@
> >  **/
> >
> >  #include <Library/BaseLib.h>
> > +#include <Library/CacheMaintenanceLib.h>
> >  #include <Library/DebugAgentLib.h>
> >  #include <Library/ArmLib.h>
> >
> > @@ -59,13 +60,14 @@ CEntryPoint (
> >  {
> >    // Data Cache enabled on Primary core when MMU is enabled.
> >    ArmDisableDataCache ();
> > -  // Invalidate Data cache
> > -  ArmInvalidateDataCache ();
> >    // Invalidate instruction cache
> >    ArmInvalidateInstructionCache ();
> >    // Enable Instruction Caches on all cores.
> >    ArmEnableInstructionCache ();
> >
> > +  InvalidateDataCacheRange ((VOID *)(UINTN)PcdGet64 (PcdCPUCoresStackBase),
> > +                            PcdGet32 (PcdCPUCorePrimaryStackSize));
> > +
> >    //
> >    // Note: Doesn't have to Enable CPU interface in non-secure world,
> >    // as Non-secure interface is already enabled in Secure world.
> > diff --git a/ArmPlatformPkg/PrePeiCore/PrePeiCoreMPCore.inf b/ArmPlatformPkg/PrePeiCore/PrePeiCoreMPCore.inf
> > index f2ac45d171bc..9d90d46dcfc5 100644
> > --- a/ArmPlatformPkg/PrePeiCore/PrePeiCoreMPCore.inf
> > +++ b/ArmPlatformPkg/PrePeiCore/PrePeiCoreMPCore.inf
> > @@ -44,6 +44,7 @@ [Packages]
> >  [LibraryClasses]
> >    ArmLib
> >    ArmPlatformLib
> > +  CacheMaintenanceLib
> >    BaseLib
> >    DebugLib
> >    DebugAgentLib
> > diff --git a/ArmPlatformPkg/PrePeiCore/PrePeiCoreUniCore.inf b/ArmPlatformPkg/PrePeiCore/PrePeiCoreUniCore.inf
> > index 84c319c3679b..0749a6d575cf 100644
> > --- a/ArmPlatformPkg/PrePeiCore/PrePeiCoreUniCore.inf
> > +++ b/ArmPlatformPkg/PrePeiCore/PrePeiCoreUniCore.inf
> > @@ -44,6 +44,7 @@ [Packages]
> >  [LibraryClasses]
> >    ArmLib
> >    ArmPlatformLib
> > +  CacheMaintenanceLib
> >    BaseLib
> >    DebugLib
> >    DebugAgentLib
> >
>
> Acked-by: Laszlo Ersek <lersek@redhat.com>
>


Pushed as 6c9a3d4233d78a04db5f25aeed254396740f4cae

Thanks all