On 7 June 2018 at 12:27, Zeng, Star <[email protected]> wrote: > I think checking validity is other code's responsibility, after that the new > code to perform cache maintenance. They are separated. > I prefer to cover both paths. >
OK, fair enough. > > Thanks, > Star > -----Original Message----- > From: edk2-devel [mailto:[email protected]] On Behalf Of Ard > Biesheuvel > Sent: Thursday, June 7, 2018 6:14 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 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 _______________________________________________ edk2-devel mailing list [email protected] https://lists.01.org/mailman/listinfo/edk2-devel
