Jordan, Thanks for the feedback and the review. I will fix the commit messages.
Mike -----Original Message----- From: Justen, Jordan L Sent: Monday, April 27, 2015 11:38 AM To: Kinney, Michael D; [email protected] Subject: Re: [edk2][Patch V2 1/10] Add Quark SoC Compatibility: MdePkg : BaseCacheMaintenanceLib How about making the first commit message line be a bit more specific? == 01/10 == MdePkg/BaseCacheMaintenanceLib: Support IA32 processors without CLFLUSH == 02/10 == MdePkg/BaseLib: Support IA32 processors without CLFLUSH == 03/10 == MdePkg/BaseLib: Support IA32 processors without CMOVx etc... This better matches our recommended commit message format: https://github.com/tianocore/tianocore.github.io/wiki/Commit-Message-Format I'm not so sure about all the extra uses of cpuid, but like you said before, we can look to optimize it if we see performance issues. With the changes to the commit message, you can add my Reviewed-by: Jordan Justen <[email protected]> for the series. By the way, the formatting of your patches looked to be double spaced (see below). Oh well. Maybe someday we can all be using git format-patch / send-email so issues like these can be fixed, and also creating and sending patches will be less tedious for the patch sender! :) Thanks for your time, -Jordan On 2015-04-26 22:54:54, Kinney, Michael D wrote: > Use CPUID Leaf 01 to detect support for CLFLUSH instruction. If CLFLUSH > is supported, use CPUID to determine the cache line size to use with > CLFLUSH. > > > > Contributed-under: TianoCore Contribution Agreement 1.0 > > Signed-off by: Michael Kinney <[email protected]> > > > > Index: Library/BaseCacheMaintenanceLib/BaseCacheMaintenanceLib.inf > > =================================================================== > > --- Library/BaseCacheMaintenanceLib/BaseCacheMaintenanceLib.inf > (revision 17201) > > +++ Library/BaseCacheMaintenanceLib/BaseCacheMaintenanceLib.inf > (working copy) > > @@ -4,7 +4,7 @@ > > # Cache Maintenance Library that uses Base Library services to maintain > caches. > > # This library assumes there are no chipset dependencies required to > maintain caches. > > # > > -# Copyright (c) 2007 - 2014, Intel Corporation. All rights reserved.<BR> > > +# Copyright (c) 2007 - 2015, Intel Corporation. All rights reserved.<BR> > > # Portions copyright (c) 2008 - 2009, Apple Inc. All rights reserved.<BR> > > # > > # This program and the accompanying materials > > @@ -23,7 +23,7 @@ > > MODULE_UNI_FILE = BaseCacheMaintenanceLib.uni > > FILE_GUID = 123dd843-57c9-4158-8418-ce68b3944ce7 > > MODULE_TYPE = BASE > > - VERSION_STRING = 1.0 > > + VERSION_STRING = 1.1 > > LIBRARY_CLASS = CacheMaintenanceLib > > > > > > Index: Library/BaseCacheMaintenanceLib/X86Cache.c > > =================================================================== > > --- Library/BaseCacheMaintenanceLib/X86Cache.c (revision > 17201) > > +++ Library/BaseCacheMaintenanceLib/X86Cache.c (working copy) > > @@ -1,7 +1,7 @@ > > /** @file > > Cache Maintenance Functions. > > - Copyright (c) 2006 - 2008, Intel Corporation. All rights reserved.<BR> > > + Copyright (c) 2006 - 2015, Intel Corporation. All rights reserved.<BR> > > This program and the accompanying materials > > are licensed and made available under the terms and conditions of the > BSD License > > which accompanies this distribution. The full text of the license may > be found at > > @@ -17,12 +17,6 @@ > > #include <Library/BaseLib.h> > > #include <Library/DebugLib.h> > > -// > > -// This size must be at or below the smallest cache size possible among > all > > -// supported processors > > -// > > -#define CACHE_LINE_SIZE 0x20 > > - > > /** > > Invalidates the entire instruction cache in cache coherency domain of > the > > calling CPU. > > @@ -128,6 +122,9 @@ > > IN UINTN Length > > ) > > { > > + UINT32 RegEbx; > > + UINT32 RegEdx; > > + UINTN CacheLineSize; > > UINTN Start; > > UINTN End; > > @@ -137,15 +134,30 @@ > > ASSERT ((Length - 1) <= (MAX_ADDRESS - (UINTN)Address)); > > + // > > + // If the CPU does not support CLFLUSH instruction, > > + // then promote flush range to flush entire cache. > > + // > > + AsmCpuid (0x01, NULL, &RegEbx, NULL, &RegEdx); > > + if ((RegEdx & BIT19) == 0) { > > + AsmWbinvd (); > > + return Address; > > + } > > + > > + // > > + // Cache line size is 8 * Bits 15-08 of EBX returned from CPUID 01H > > + // > > + CacheLineSize = (RegEbx & 0xff00) >> 5; > > + > > Start = (UINTN)Address; > > // > > // Calculate the cache line alignment > > - // > > - End = (Start + Length + (CACHE_LINE_SIZE - 1)) & ~(CACHE_LINE_SIZE - > 1); > > - Start &= ~((UINTN) CACHE_LINE_SIZE - 1); > > + // > > + End = (Start + Length + (CacheLineSize - 1)) & ~(CacheLineSize - 1); > > + Start &= ~((UINTN)CacheLineSize - 1); > > do { > > - Start = (UINTN)AsmFlushCacheLine ((VOID*)Start) + CACHE_LINE_SIZE; > > + Start = (UINTN)AsmFlushCacheLine ((VOID*)Start) + CacheLineSize; > > } while (Start != End); > > return Address; > > } ------------------------------------------------------------------------------ One dashboard for servers and applications across Physical-Virtual-Cloud Widest out-of-the-box monitoring support with 50+ applications Performance metrics, stats and reports that give you Actionable Insights Deep dive visibility with transaction tracing using APM Insight. http://ad.doubleclick.net/ddm/clk/290420510;117567292;y _______________________________________________ edk2-devel mailing list [email protected] https://lists.sourceforge.net/lists/listinfo/edk2-devel
