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

["Kinney, Michael D" <michael.d.kinney@intel.com>]

Hi Ard,

I would still prefer the cache maintenance be done independent
of capsules in case there is other memory ranges that need to
be flushed for other features.

The CacheMaintenanceLib for ARM has several services that are
not implemented and ASSERT().  I found some ARM documentation 
that says that caches can be flushed by looping through the 
indexes.  

http://infocenter.arm.com/help/index.jsp?topic=/com.arm.doc.ddi0201d/ch03s03s05.html

I know you mentioned that there can be alternate MMU
implementations that may not work with that algorithm.  However
that could require a customized CacheMaintenanceLib.  Can you
provide the flush all using the documented algorithm and use that
instance for systems that are compatible with the documented algorithm.
Then, the flush all API can be used in ResetSystem() or SEC Phase
when the boot mode is warm reset.

Mike

> -----Original Message-----
> From: Ard Biesheuvel [mailto:ard.biesheuvel@linaro.org]
> Sent: Monday, June 11, 2018 2:29 PM
> To: Yao, Jiewen <jiewen.yao@intel.com>
> Cc: edk2-devel@lists.01.org; Leif Lindholm
> <leif.lindholm@linaro.org>; Zeng, Star
> <star.zeng@intel.com>; Kinney, Michael D
> <michael.d.kinney@intel.com>
> Subject: Re: [PATCH v2 1/5] MdeModulePkg/CapsulePei: clean
> Dcache before consuming capsule data
> 
> On 11 June 2018 at 23:27, Yao, Jiewen
> <jiewen.yao@intel.com> wrote:
> > Hi Ard
> > May I suggest that we split the Capsule Dispatch patch
> from the cache maintenance patch?
> >
> > I think the former may require more time for design
> discussion.
> >
> 
> Yes, that is fine. As I explained, it has mainly to do with
> dispatching the capsule at a time when the console or GOP
> is available
> to report progress. This is separate from the cache
> maintenance issue.
> 
> 
> >> -----Original Message-----
> >> From: Ard Biesheuvel [mailto:ard.biesheuvel@linaro.org]
> >> Sent: Monday, June 11, 2018 2:25 PM
> >> To: edk2-devel@lists.01.org
> >> Cc: Leif Lindholm <leif.lindholm@linaro.org>; Zeng, Star
> <star.zeng@intel.com>;
> >> Yao, Jiewen <jiewen.yao@intel.com>; Kinney, Michael D
> >> <michael.d.kinney@intel.com>; Ard Biesheuvel
> <ard.biesheuvel@linaro.org>
> >> Subject: Re: [PATCH v2 1/5] MdeModulePkg/CapsulePei:
> clean Dcache before
> >> consuming capsule data
> >>
> >> On 8 June 2018 at 08:58, Ard Biesheuvel
> <ard.biesheuvel@linaro.org> wrote:
> >> > 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>
> >>
> >> Star,
> >>
> >> If you are ok with this version of the patch, please let
> me know.
> >>
> >> This patch and the PsciResetSystemLib one are
> prerequisites for making
> >> PersistAcrossReset capsules work at all on ARM systems.
> The remaining
> >> patches are only relevant when using the new progress
> reporting APIs,
> >> so those can wait, but I would like to merge this one as
> soon as it is
> >> ready.
> >>
> >> Thanks,
> >> Ard.
> >>
> >>
> >> > ---
> >> >  MdeModulePkg/Universal/CapsulePei/CapsulePei.inf
> |  1 +
> >> >
> MdeModulePkg/Universal/CapsulePei/Common/CapsuleCoalesce.c
> | 38
> >> ++++++++++++++------
> >> >  2 files changed, 28 insertions(+), 11 deletions(-)
> >> >
> >> > 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..52b80e30b479 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>
> >> > @@ -253,6 +254,7 @@ ValidateCapsuleByMemoryResource (
> >> >    )
> >> >  {
> >> >    UINTN             Index;
> >> > +  BOOLEAN           Valid;
> >> >
> >> >    //
> >> >    // Sanity Check
> >> > @@ -270,25 +272,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;
> >> >  }
> >> >
> >> >  /**
> >> > --
> >> > 2.17.0
> >> >