Since capsule data pointer may be invalid (for example, point to MMIO), we enhanced code to validate the capsule by memory resource HOB, and *recommend* platform/silicon (memory reference code) to report memory resource HOB before capsule coalescing. To consider and compatible with some platform may not report memory resource HOB before capsule coalescing, then MemoryResource == NULL and the code thinks it is valid.
Thanks, Star -----Original Message----- From: Ard Biesheuvel [mailto:[email protected]] Sent: Thursday, June 7, 2018 5:52 PM To: Zeng, Star <[email protected]> Cc: Ni, Ruiyu <[email protected]>; [email protected]; Gao, Liming <[email protected]>; Yao, Jiewen <[email protected]>; Leif Lindholm <[email protected]>; Kinney, Michael D <[email protected]> Subject: Re: [edk2] [PATCH] MdeModulePkg/CapsulePei: clean Dcache before consuming capsule data On 7 June 2018 at 11:46, Zeng, Star <[email protected]> wrote: > Ok, I want to know whether others have some idea, so let's wait some time? > > About the code change, I have three minor comments below. > 1. I suggest adding some code comment for the new line code. OK > 2. There are two paths in ValidateCapsuleByMemoryResource() to return TRUE, > should the new line code be added for both of them? Good question: In which circumstances is the MemoryResource == NULL case expected to occur? > 3. There is VS2015 building failure like below with the patch. The code needs > to typecast parameter 'Size'. > > warning C4244: 'function': conversion from 'UINT64' to 'UINTN', > possible loss of data > Will fix. > > Thanks, > Star > -----Original Message----- > From: edk2-devel [mailto:[email protected]] On Behalf Of > Ard Biesheuvel > Sent: Thursday, June 7, 2018 2:00 PM > To: Zeng, Star <[email protected]> > Cc: Ni, Ruiyu <[email protected]>; [email protected]; Gao, > Liming <[email protected]>; Yao, Jiewen <[email protected]>; > Leif Lindholm <[email protected]>; Kinney, Michael D > <[email protected]> > Subject: Re: [edk2] [PATCH] MdeModulePkg/CapsulePei: clean Dcache > before consuming capsule data > > On 7 June 2018 at 07:41, Zeng, Star <[email protected]> wrote: >> Thanks, got the point. >> >> It seems vague that who to ensure the cache coherency. MMU? Caller of >> UpdateCapsule? UpdateCapsule? Consumer of capsule data? >> > > Unfortunately, since the spec does not mention it at all, we cannot rely on > the caller of UpdateCapsule() to ensure this. That would require a spec > change, and break backward compatibility with older revisions. > > The main issue here is that ARM does not provide the means to clean the > entire cache, the only architectural method is clean cacheline by virtual > address. > > So that leaves: > - UpdateCapsule - problematic because it may be called at runtime and > the firmware has no means of translating the physical addresses > - ResetSystem - same as above: it is a runtime service, and so it > cannot rely on a mapping to exist for those physical addresses > - SEC - lacks the information about where the capsule resides > - CapsulePei - already extracts the information about the capsule address in > memory, and can perform the cache maintenance right before consuming the data. > > So unless anyone has any other suggestions, I think this approach is the only > feasible one. > > If there are any concerns about adding this for architectures, I can look > into refactoring CapsulePei and add it only for ARM/AARCH64. > > Thanks, > Ard. > > > >> -----Original Message----- >> From: edk2-devel [mailto:[email protected]] On Behalf >> Of Ard Biesheuvel >> Sent: Thursday, June 7, 2018 12:50 PM >> To: Zeng, Star <[email protected]> >> Cc: Ni, Ruiyu <[email protected]>; [email protected]; Leif >> Lindholm <[email protected]>; Yao, Jiewen >> <[email protected]>; Gao, Liming <[email protected]>; Kinney, >> Michael D <[email protected]> >> Subject: Re: [edk2] [PATCH] MdeModulePkg/CapsulePei: clean Dcache >> before consuming capsule data >> >> On 7 June 2018 at 03:37, Zeng, Star <[email protected]> wrote: >>> Hi Ard, >>> >>> The input parameter CapsuleHeaderArray of UpdateCapsule has the virtual >>> address. >>> >> >> It has the virtual address of the capsules yes. But how about the data >> structures passed as the ScatterGatherList? >> >> >>> CapsuleHeaderArray >>> Virtual pointer to an array of virtual pointers to the capsules >>> being passed into update capsule. Each capsules is assumed to stored >>> in contiguous virtual memory. The capsules in the CapsuleHeaderArray >>> must be the same capsules as the ScatterGatherList. The >>> CapsuleHeaderArray must have the >>> >>> >>> Thanks, >>> Star >>> -----Original Message----- >>> From: edk2-devel [mailto:[email protected]] On Behalf >>> Of Ard Biesheuvel >>> Sent: Wednesday, June 6, 2018 8:10 PM >>> To: [email protected] >>> Cc: Ni, Ruiyu <[email protected]>; Ard Biesheuvel >>> <[email protected]>; Gao, Liming <[email protected]>; >>> Yao, Jiewen <[email protected]>; Leif Lindholm >>> <[email protected]>; Kinney, Michael D >>> <[email protected]>; Zeng, Star <[email protected]> >>> Subject: Re: [edk2] [PATCH] MdeModulePkg/CapsulePei: clean Dcache >>> before consuming capsule data >>> >>> On 6 June 2018 at 11:52, Ard Biesheuvel <[email protected]> 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. >>>> >>>> Contributed-under: TianoCore Contribution Agreement 1.1 >>>> Signed-off-by: Ard Biesheuvel <[email protected]> >>> >>> Leif asked me to include a note why this cannot be done when >>> UpdateCapsule() is called. >>> >>> >>> """ >>> 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. >>> """ >>> >>>> --- >>>> MdeModulePkg/Universal/CapsulePei/CapsulePei.inf | 1 + >>>> MdeModulePkg/Universal/CapsulePei/Common/CapsuleCoalesce.c | 4 >>>> ++++ >>>> 2 files changed, 5 insertions(+) >>>> >>>> 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..1730f925adc5 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> >>>> @@ -283,6 +284,9 @@ ValidateCapsuleByMemoryResource ( >>>> 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)); >>>> + >>>> + WriteBackDataCacheRange ((VOID *)(UINTN)Address, Size); >>>> + >>>> return TRUE; >>>> } >>>> } >>>> -- >>>> 2.17.0 >>>> >>> _______________________________________________ >>> edk2-devel mailing list >>> [email protected] >>> https://lists.01.org/mailman/listinfo/edk2-devel >> _______________________________________________ >> edk2-devel mailing list >> [email protected] >> https://lists.01.org/mailman/listinfo/edk2-devel > _______________________________________________ > edk2-devel mailing list > [email protected] > https://lists.01.org/mailman/listinfo/edk2-devel _______________________________________________ edk2-devel mailing list [email protected] https://lists.01.org/mailman/listinfo/edk2-devel
