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.biesheu...@linaro.org] > Sent: Monday, June 11, 2018 2:25 PM > To: edk2-devel@lists.01.org > Cc: Leif Lindholm <leif.lindh...@linaro.org>; Zeng, Star > <star.z...@intel.com>; > Yao, Jiewen <jiewen....@intel.com>; Kinney, Michael D > <michael.d.kin...@intel.com>; Ard Biesheuvel <ard.biesheu...@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.biesheu...@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....@intel.com> > > Contributed-under: TianoCore Contribution Agreement 1.1 > > Signed-off-by: Ard Biesheuvel <ard.biesheu...@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 > > _______________________________________________ edk2-devel mailing list edk2-devel@lists.01.org https://lists.01.org/mailman/listinfo/edk2-devel
