On 11/04/15 13:17, Cohen, Eugene wrote: > Editorial: I know there's been a lot of talk about what tools to use > to review code and a lot of people like email - but for me it's hard > for me to get proper context with 10 patches in email when I don't > work on the code every day - I'd like to think if I saw these in a > tool like gerrit I would have had proper context sooner.
I agree that more context is always better; personally, when in doubt, I apply the series on a separate branch (either from email or directly by pulling). I can then freely move between the patches (-> git checkout) and look at the full source with my ctags-compatible editor at any stage. (Building / testing / grepping are possible too, of course.) There's probably no better context than that. ... Ard, did you push this series and provide the URL in the blurb? (Rhetorical question :)) Thanks Laszlo > > Thanks for the context - glad we have you working on this. :) > > Eugene > > -----Original Message----- > From: Ard Biesheuvel [mailto:[email protected]] > Sent: Wednesday, November 04, 2015 1:03 AM > To: Cohen, Eugene <[email protected]> > Cc: [email protected]; [email protected]; [email protected]; > [email protected] > Subject: Re: [edk2] [PATCH 08/10] ArmCacheMaintenanceLib: disallow whole > D-cache maintenance operations > > On 3 November 2015 at 23:19, Cohen, Eugene <[email protected]> wrote: >> Ard, >> >> This is probably a reflection of my lack of deep understanding of ARM cache >> maintenance on more complex systems. >> >> The "entire cache" operations exist to allow transitions across major >> environments (initialization of non-self initializing caches, changing >> phases of boot, etc) to clean/clean+invalidate/invalidate cache without >> knowing apriori what virtual addresses have dirty data. The set/way >> solution was a matter of convenience since the ARM did not have an >> architectural means of operating on the full cache in one instruction. We >> do have cases where we need to turn off caches entirely - booting our OS as >> well as in core power management scenarios - I think we have to do a full >> cache flush in these cases if not by set/way then somehow. >> > > It probably makes sense to distinguish more clearly between cache ops that > manage the /contents/ of the caches, and cache ops that manage the caches > themselves. The set/way operations are really only suitable for managing the > caches themselves, and core power management is one of the few cases where > they are actually appropriate. For managing the contents of the caches, i.e., > making sure that all writes have made it to main memory and are not shadowed > by stale cache data at any level to a CPU that is doing cacheable accesses, > we really only have the by-VA operations. > >> I don't think we can expect any one component (or whatever phase-thunking >> code) to know what virtual addresses may have dirty data nor can we expect >> it to loop across all possible cacheable virtual addresses. >> > > Then that is a problem. ArmCleanDataCache() does not guarantee that data has > made it to main memory, so we simply cannot rely on it. Mind you, this is not > a theoretical debate, there are examples of architecturally compliant systems > with system level caches where cleaned data has been observed to only make it > to main memory if you clean by VA. That is the rationale behind this > particular patch: the use of 'WriteBackDataCache()' in generic code imposes a > requirement that we cannot fulfil, and it is better to flag it than to > pretend we can guarantee that all data has made its way to main memory after > the call returns. > >> I also see that the "threshold" based modification of the cache flush from >> MVA to entire-cache was removed earlier this year (I missed that!). Have >> you quantified the performance impact from removing of the threshold >> methodology? >> > > That fixed an actual bug. The performance went from 'could not boot' > to 'could boot' :-) But seriously, I understand that there is a performance > concern here, but 'promoting' a clean by VA operation to clean by set/way > really makes no sense at all. Since Tianocore is the reference implementation > for UEFI on ARM, and is widely used by silicon vendors, especially the ones > that are new to PI/UEFI, we _must_ abide by architectural rules. > >> Since PEI/DXE/UEFI Boot Services is uniprocessor I don't know if it makes >> sense to eliminate the "entire cache" methodology entirely - I agree for >> other phases like runtime and TZ/SMM that it should be banished. >> > > I am not proposing to remove this functionality. What I am doing is removing > the calls from places where they are inappropriate. This patch is one such > example, and the PrePi case is another one (i.e., where the UEFI image has > been loaded into DRAM by some other means, and executes from there). Now that > I think if it, the PrePeiCore version could actually execute from NOR > straight out of reset, so perhaps the ArmCleanDataCache() could be > appropriate there. But let's discuss that in the context of the other patch. > > -- > Ard. > > >> -----Original Message----- >> From: Ard Biesheuvel [mailto:[email protected]] >> Sent: Tuesday, November 03, 2015 7:30 AM >> To: Cohen, Eugene <[email protected]> >> Cc: [email protected]; [email protected]; [email protected]; >> [email protected] >> Subject: Re: [edk2] [PATCH 08/10] ArmCacheMaintenanceLib: disallow whole >> D-cache maintenance operations >> >> On 3 November 2015 at 14:51, Cohen, Eugene <[email protected]> wrote: >>> Please don't remove this functionality. At times we do want to use this >>> library to turn off the cache in preparation for going to another >>> environment (say, loading an OS) and this is useful. >>> >> >> Note that this is not about turning off the caches. >> >> As Mark points out, the set/way operations are not suitable for ensuring >> that the OS boot image has made it all the way to memory, and that it will >> be visible to the CPU with the MMU off. The cache maintenance by VA >> operations do offer those guarantees, so the architecturally sound method of >> achieving that is to clean the >> region(s) you care about by VA before disabling the MMU. >> >> Under virtualization, these issues are a bit more prominent, and I have had >> to add by-VA cache maintenance on several occasions to fix actual problems. >> >> Note that this patch does not remove any of the functionality. It just >> changes the CacheMaintenanceLib implementation so that this functionality is >> not exported to generic code. If any non-ARM specific code in the tree >> requires the ability to clean and/or invalidate the whole data cache, it is >> making incorrect assumptions about the platform that it runs on. >> >> Regards, >> Ard. >> >> >> >>> -----Original Message----- >>> From: edk2-devel [mailto:[email protected]] On Behalf Of >>> Ard Biesheuvel >>> Sent: Tuesday, November 03, 2015 3:17 AM >>> To: [email protected]; [email protected]; >>> [email protected] >>> Cc: [email protected]; Ard Biesheuvel <[email protected]> >>> Subject: [edk2] [PATCH 08/10] ArmCacheMaintenanceLib: disallow whole >>> D-cache maintenance operations >>> >>> The ARM architecture provides no reliable way to clean or invalidate the >>> entire data cache at runtime. The reason is that such maintenance requires >>> the use of set/way maintenance operations, which are suitable only for the >>> kind of maintenance that is carried out when the cache is taken offline >>> entirely. >>> >>> So ASSERT () when any of the CacheMaintenanceLib whole data cache routines >>> are invoked rather than pretending we can do anything meaningful here. >>> >>> Contributed-under: TianoCore Contribution Agreement 1.0 >>> Signed-off-by: Ard Biesheuvel <[email protected]> >>> --- >>> ArmPkg/Library/ArmCacheMaintenanceLib/ArmCacheMaintenanceLib.c | 8 >>> ++++---- >>> 1 file changed, 4 insertions(+), 4 deletions(-) >>> >>> diff --git >>> a/ArmPkg/Library/ArmCacheMaintenanceLib/ArmCacheMaintenanceLib.c >>> b/ArmPkg/Library/ArmCacheMaintenanceLib/ArmCacheMaintenanceLib.c >>> index 175d29496c32..8cc1990b3266 100644 >>> --- a/ArmPkg/Library/ArmCacheMaintenanceLib/ArmCacheMaintenanceLib.c >>> +++ b/ArmPkg/Library/ArmCacheMaintenanceLib/ArmCacheMaintenanceLib.c >>> @@ -14,6 +14,7 @@ >>> **/ >>> #include <Base.h> >>> #include <Library/ArmLib.h> >>> +#include <Library/DebugLib.h> >>> #include <Library/PcdLib.h> >>> >>> VOID >>> @@ -44,7 +45,6 @@ InvalidateInstructionCache ( >>> VOID >>> ) >>> { >>> - ArmCleanDataCache(); >>> ArmInvalidateInstructionCache(); >>> } >>> >>> @@ -54,7 +54,7 @@ InvalidateDataCache ( >>> VOID >>> ) >>> { >>> - ArmInvalidateDataCache(); >>> + ASSERT (FALSE); >>> } >>> >>> VOID * >>> @@ -76,7 +76,7 @@ WriteBackInvalidateDataCache ( >>> VOID >>> ) >>> { >>> - ArmCleanInvalidateDataCache(); >>> + ASSERT (FALSE); >>> } >>> >>> VOID * >>> @@ -96,7 +96,7 @@ WriteBackDataCache ( >>> VOID >>> ) >>> { >>> - ArmCleanDataCache(); >>> + ASSERT (FALSE); >>> } >>> >>> VOID * >>> -- >>> 1.9.1 >>> >>> _______________________________________________ >>> 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
