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

["Yao, Jiewen" <jiewen.yao@intel.com>]

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.

Thank you
Yao Jiewen


> -----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
> >