On 11 June 2018 at 23:27, Yao, Jiewen <jiewen....@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.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
