> The set/way operations are really only suitable for managing the caches > themselves
This makes sense to me and I agree that the majority of developers should only be dealing with managing buffers and should only use the VA/address-range interface. > 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. I fully expect this given the nature of the ARM architecture - the scope architectural specifications stop at the interconnect so if somebody wants to implement wacky hardware then there's nothing to stop them. Presumably for whole-cache management some extra stuff is required on these systems to make it work - which is fine although not that helpful for interoperability purposes. > 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. Defaulting to safe/correct for all architectures makes sense to me. But it would be nice for those of us that understand our architectures (no system level caches, SMP config understood) to choose an option to turn on thresholding through a PCD setting or library class selection. > I am not proposing to remove this functionality. Ok - I may have interpreted patch 4 as removing more than PoU but I can see it's just removing entire-cache flushing to PoU - this is fine by me since cache maintenance for code loading should always reference the relevant VAs. 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. 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
