Andrew, Thanks for the reminder for CpuDxe. I think we saw CpuDxe calculation some time ago, and it was most likely a reason we went with a very similar approach to determine the TSC frequency as a fallback mechanism. It, however, is in fact prone to issues, putting aside the non-guaranteed period and CPU frequency match. Basically, there are two problems: 1. The code implemented in C is subject to minor time drifts, which results in slightly different results across the reboots. The calculation based on CPUID 15H is more accurate. 2. The lack of TPL changes during frequency measurement results in EFI_EVENTs potentially interrupting the process and thus making the resulting value very inaccurate. Since the value is cached there is not always a possibility to avoid it. EmulatorPkg is a bit of a special case, which we think we know about :), actually thanks for fixing it up on macOS, this is appreciated! All in all, I would say that there really needs to exist a decent library, which most likely CpuDxe should make a use of as well. Best wishes, Vitaly > 16 авг. 2019 г., в 21:35, Andrew Fish написал(а): > > Vitaly, > > As Mike mentioned platforms can know more info about how they are constructed thus you may not want to have a lot of generic discovery code floating about if you don't really need it. > > One option could be to pass up the TSC Frequency/Period via some EFI mechanism so generic code can be told by platform specific code. > > The PI spec already has an abstraction for a CPU based timer that is architecture neutral. The CPU Architectural Protocol has a GetTimerValue() member function. > https://github.com/tianocore/edk2/blob/master/MdePkg/Include/Protocol/Cpu.h#L220 > > For X86 it returns TSC > https://github.com/tianocore/edk2/blob/master/UefiCpuPkg/CpuDxe/CpuDxe.c#L289 > > EFI Systems are not required to implement PI so we usually don't encourage generic EFI code to go after PI APIs. > > I'd also point out that using TSC can break in things like the EmulatorPkg as you end up running in ring 0 and TSC access is blocked. > https://github.com/tianocore/edk2/blob/master/EmulatorPkg/CpuRuntimeDxe/Cpu.c#L352 > https://github.com/tianocore/edk2/blob/master/EmulatorPkg/Unix/Host/EmuThunk.c#L250 > > > I would point that a library that did TSC frequency discovery would likely be useful for the UefiCpuPkg CpuDxe driver. > > Thanks, > > Andrew Fish > >> On Aug 15, 2019, at 2:10 PM, Vitaly Cheptsov via Groups.Io > wrote: >> >> Hello, >> >> I initially raised this question in a new TimerLib patch[1], but as the discussion was getting more distracted, I decided to create a separate thread in hopes new people could join. >> >> The issue is that our UEFI bootloader needs to obtain TSC frequency to pass it to our specialised operating system that uses TSC for scheduling on x86. >> >> For a while we went with ACPI power management timer to measure the frequency, but as modern Intel CPUs support CPUID 15H leaf (CPUID_TIME_STAMP_COUNTER) we try to use where possible for better accuracy. The issue with this CPUID leaf is that the crystal clock frequency returned in ECX register is optional and therefore can be 0. Intel SDM suggests to use a static value in this case[2], but it is completely opaque on how to match the running CPU with its static value from SDM. >> >> Initially we went with CPUID model checking, but this failed badly for Xeon Scalable and Xeon W, as they share the CPUID (06_55H) but have different crystal clock frequencies (25 MHz vs 24 MHz accordingly). Donald Kuo gave a good hint in the previous thread that client CPUs usually get 24 MHz crystal clock, server CPUs have 25, and Atoms have 19.2. This, however, does not make the situation easier as we do not see a way to determine CPU vertical segment without e.g. parsing the CPUID brand string. >> >> Apparently, we are not alone, and different open-source operating systems have different workarounds to this issue. For example, Linux kernel went with using marketing frequency from CPUID 16H leaf (CPUID_PROCESSOR_FREQUENCY)[3], and BSD flavours fallback to older methods when neither crystal clock frequency can be obtained through CPUID 15H, nor unambiguous CPUID models exist to be able to use static values. >> >> Another issue we see with EDK II TimerLib implementations for x86 is that they are very model specific. As Michael Kinney said, the situation is not a problem when you use TimerLib for BSP bringup, as you already know the CPU family you target, however, it is not great for other uses, although they may be discouraged. In our opinion, regardless of the use, this approach has a number of design issues. >> >> All modern Intel x86 CPUs have virtual TSC counter that has fixed frequency. In fact, this is true for most, if not all, modern x86 CPUs by other vendors as well. This makes us believe that EDK II should have generic TscTimerLib for x86, which will work anywhere when given valid TSC frequency, and TscFrequencyLib, which would determine TSC frequency in a vendor-specific method. Ideally there exists GenericTscFrequencyLib that can do this for a wide majority of CPUs through different methods at runtime, including CPUID 15H, ACPI power management, vendor-specific extensions, etc. >> >> To summarise: >> - We need to know how to match current running CPU with its crystal clock frequency when CPUID 15H reports 0 ECX. >> - We would like to suggest to split TSC-based TimerLib in two libraries: generic with timer implementation and platform-specific with TSC frequency discovery. >> - We wonder whether a generic vendor-supported TSC frequency discovery library working on a wide range of CPUs is feasible to have in EDK II mainline. >> >> Best regards, >> Vitaly >> >> [1] https://edk2.groups.io/g/devel/topic/patch_ueficpupkg_adding_a/32839184?p=,,,20,0,0,0::recentpostdate%2Fsticky,,,20,2,0,32839184 >> [PATCH] UefiCpuPkg: Adding a new TSC library by using CPUID(0x15) TSC leaf >> [2] 18.7.3 Determining the Processor Base Frequency >> Table 18-75. Nominal Core Crystal Clock Frequency >> [3] https://lore.kernel.org/lkml/20190509055417.13152-1-drake@endlessm.com/ >> >> >