Jordan, Thanks for the feedback.
I agree that the each patch should be for a single issue and the commit message should only discuss that one issue. I will resend the patches with the patches inline to better support review comments. I did consider a feature flag for Quark so the code associated with Quark should be conditionally enabled. From the analysis I have done, none of these changes are in performance paths, and I thought it would be better for EDK II app/drivers to include Quark compatibility by default if possible. If real performance issues are seen from these changes, then I will reconsider adding a feature flag. Auto promoting a flush of a range to flush entire is a good optimization if the range is large enough. However, that logic should exist in the code that calls the CacheMaintenanceLib. Not in the CacheMaintenanceLib implementation itself. The calling code can evaluate the cache size and cache attributes and make that policy decision. The CacheMaintenanceLib was designed on purpose to provide functions that operate in the entire cache and functions that operate on a range to give the calling code that option. I agree that adding a DXE specific version of the CacheMaintenanceLib could cache the CPUID results. That could be considered for a new feature if real performance issues are seen with the design proposed here. Mike -----Original Message----- From: Justen, Jordan L Sent: Tuesday, April 14, 2015 11:32 PM To: Kinney, Michael D; edk2-devel@lists.sourceforge.net Subject: Re: [edk2] [Patch 1/8] MdePkg: BaseCacheMaintenanceLib: Add Quark SoC compatibility On 2015-04-14 09:49:00, Kinney, Michael D wrote: > Please review the attached patch that adds Quark SoC compatibility to the > BaseCacheMaintenanceLib in the MdePkg. > > > > Some EDK II libraries assume capabilities that are not available on Quark > SoC. With these changes, applications and drivers built for IA32 are > compatible with Quark SoC. > > These patches do not modify X64 specific sources or SSE/SSE2 specific > sources. > > > > Change Summary: > > =============== > > 1) Remove use of CMOVx from IA32 assembly. This matches compiler flags for > all supported C compilers. > > 2) Use CPUID Leaf 01 to detect CPU capabilities for CLFLUSH, Cache Line > Size, FXSAVE/FXTSTOR, CR4.DE, and CR4.OSFXR Maybe we should have a Dxe library version that can only call cpuid once in a constructor? Or, would it make sense to have a Quark feature flag? Another case to think about for cflush is that wbinvd can also make sense if the memory range size is bigger than the actual cache size. (Maybe not too likely of a case for firmware to encounter. :) This patch only has changes from item #2. I don't think it makes sense to have all 4 items listed in the commit message for this patch. Rather than fixing all 4 items in each module separately, I think it would be better to fix MdePkg for item #1 in all MdePkg modules in a single patch. (And so on for items 2, 3 and 4.) I think if someone encounters an issue from these changes, they might want to use a bisect to track down when the the issue was introduced. If they encounter a patch that is doing up to 4 different things, they will then have to split it apart to find out what the issue is. This would also allow you to have a more accurate commit message, because it will be just making 1 type of change to MdePkg. -Jordan > 3) Use CPUID Family/Model/Stepping to detect support for > MSR_IA32_APIC_BASE_ADDRESS. Quark SoC does not support this MSR. > > 4) Add stride PCD to MdeModulePkg to support 16550 UARTs with a register > stride that is not 1 byte. Quark SoC uses a stride of 4 bytes. > > Contributed-under: TianoCore Contribution Agreement 1.0 > Signed-off by: Michael Kinney <michael.d.kin...@intel.com> > > ------------------------------------------------------------------------------ BPM Camp - Free Virtual Workshop May 6th at 10am PDT/1PM EDT Develop your own process in accordance with the BPMN 2 standard Learn Process modeling best practices with Bonita BPM through live exercises http://www.bonitasoft.com/be-part-of-it/events/bpm-camp-virtual- event?utm_ source=Sourceforge_BPM_Camp_5_6_15&utm_medium=email&utm_campaign=VA_SF _______________________________________________ edk2-devel mailing list edk2-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/edk2-devel
