On 7 June 2018 at 12:12, Zeng, Star <[email protected]> wrote: > 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. >
OK. I think it is a bad idea to perform cache maintenance on an address that may be invalid. So I'd prefer to keep the single invocation. > > 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
