Hi Vitaly,

UEFI Application does be another scope. And regards your question on “a way to 
dynamically determine the difference between Xeon client and server” … is not 
in current design-in, for long term goal we could consider if there is proper 
way to identify CPU SKU dynamically.

Thanks for sharing your thought and it could be an enhancement on TimerLib in 
the future. ☺


From: Kinney, Michael D
Sent: Friday, August 16, 2019 12:23 AM
To: devel@edk2.groups.io; vit9...@protonmail.com; Kuo, Donald 
<donald....@intel.com>; Kinney, Michael D <michael.d.kin...@intel.com>
Subject: RE: [edk2-devel] [PATCH] UefiCpuPkg: Adding a new TSC library by using 
CPUID(0x15) TSC leaf


When implementing a UEFI Application, if you want maximum compatibility, you 
should use UEFI Services/Protocols and minimize as many HW assumptions as 

I understand, especially for accurate time and clock related services, the that 
the UEFI Specification defined Services/Protocols can be challenging to use.

When adding content that uses HW such as TSC or CPUID the UEFI App should be 
implemented with good detection logic to know it is safe to use that HW, and 
contain the fallback logic to use an alternate mechanism if the required HW is 
not present.  This is a very different approach than some early FW 
initialization code that can safely make some HW assumptions that helps keep 
the FW init code small and efficient.  This does imply that different libraries 
may be required for FW init and UEFI Applications.



From: devel@edk2.groups.io<mailto:devel@edk2.groups.io> 
[mailto:devel@edk2.groups.io] On Behalf Of Vitaly Cheptsov via Groups.Io
Sent: Thursday, August 15, 2019 5:10 AM
To: Kuo, Donald <donald....@intel.com<mailto:donald....@intel.com>>
Cc: devel@edk2.groups.io<mailto:devel@edk2.groups.io>
Subject: Re: [edk2-devel] [PATCH] UefiCpuPkg: Adding a new TSC library by using 
CPUID(0x15) TSC leaf

Hi Donald,

Glad to hear it helped a little, and sorry for some outdated quotes.

Your clarification regarding model range is very helpful. Xeon W being client 
and thus having client clock makes sense, though I must say it was quite not 
obvious. I was referring to the same SDM table, and it would have been great to 
have vertical range binding instead of the misleading CPUID.

With that, however, I still do not see a way to dynamically determine the 
difference between Xeon client and server.

For us it is needed as we use TimerLib differently. For you TimerLib is a part 
of BSP, so you face no issue statically setting the clock frequency as a PCD. 
For us TimerLib is used as a part of an UEFI application compatible with 
different hardware, and in fact not just Intel. We have such "generic TimerLib" 
library, and to determine CPU frequency, as a fallback to CPUID 15H, it relies 
on ACPI PM timer. This is not great for two reasons:
- we have to maintain it ourselves, while we would have liked that be part of 
EDK II with different vendors contributing to the project.
- we still cannot find an obvious way to determine crystal clock when it is not 
reported, in particular for Xeon W and Xeon Scalable case.

I guess this is now out of the scope of this patch and perhaps even this exact 
library, but it will be helpful to hear relevant technical details on the issue 
and opinions on such "generic TimerLib" library to later appear in EDK II.

Best regards,
15 авг. 2019 г., в 11:40, Kuo, Donald 
<donald....@intel.com<mailto:donald....@intel.com>> написал(а):

Hi Vitaly,

Appreciated for reviewing very detail. And looks your captured some piece of 
code is older version. And should be ok, I do got your point.

Item #1
-          I do missed RegEax error handling in case for 
CpuidCoreClockCalculateTscFrequency () should return 0 and EFI_UNSUPPORTED. AR: 
Will update it.

Item #2:
-          Actually the information is from SDM Table 18-85
o   As we know CPUID signature could be hard to identify processor XTAL 
frequency is? So we only check CPUID Leaf 0x15
o   And the PCD will be defined depends on platform specific and during project 
early development. Based on SDM, Intel processor for CPUID.15h EAX and EBX is 
enumerated, but ECX could be possible not enumerated. So that is why we based 
on  SDM Table 18-85 to create PCD as below:
•  Intel Xeon Server family: 25MHz
•  Intel Core and Intel Xeon W family: 24MHz
•  Intel Atom : 19.2MHz
•  So regards the “06_55h” might cause the confused, we could review it whether 
remove it??
Item #3:
-          Again, the XTAL frequency is optional and should be define in early 
phase of new project. Default should still use AcpiTimer.
o    Platform / developer owner should well know the CPU generation XTAL 
frequency is? Server / Client / Atom ?
o   For those CPU not supported should still use AcpiTimerLib (default)


From: vit9696 via [] [mailto:vit9696=protonmail.com<http://protonmail.com/>@[]]
Sent: Thursday, August 15, 2019 3:20 PM
To: Kuo, Donald <donald....@intel.com<mailto:donald....@intel.com>>; 
Subject: Re: [edk2-devel] [PATCH] UefiCpuPkg: Adding a new TSC library by using 
CPUID(0x15) TSC leaf


Thank you for the patch! I reviewed the code and noticed select issues 
explained below.

1. The following construction:

if (RegEbx == 0) {
DEBUG ((DEBUG_ERROR, "The CPU is not capble for Core Crystal Clock Frequency 
ASSERT (RegEbx != 0);

Does not look good to me, and in my opinion, should be written differently:

if (RegEbx == 0 || RegEax == 0) {
DEBUG ((DEBUG_ERROR, "The CPU is not capble for Core Crystal Clock Frequency 
ASSERT (RegEbx != 0);
  ASSERT (RegEax != 0);
  return 0;

The reason for the above code being wrong is potential division by zero below, 
which behaviour is undefined (and in fact unknown due to unspecified interrupt 
table state). Even though the intent is to not permit the use of this library 
on an unsupported platform, runtime checks and assertions are essentially 
NO-OPs, should be separate and not confused. For this to work properly the call 
to CpuidCoreClockCalculateTscFrequency should happen in library constructor 
with EFI_UNSUPPORTED return on CpuidCoreClockCalculateTscFrequency returning 0.

2. The notes about crystal clock frequency for 06_55H CPU signature:
"25000000 - Intel Xeon Processor Scalable Family with CPUID signature 
# Intel Xeon Processor Scalable Family with CPUID signature 06_55H = 25000000 

are misleading in both this library and Intel SDM. Intel Xeon W processors have 
the same CPUID signature (06_55H), they report 0 crystal clock frequency, and 
their crystal clock frequency is 24 MHz. This should at least be mentioned in 
the lower part mentioning Intel Xeon W Processor Family(24MHz).

Actually, given that many Intel guys are here, I wonder whether anybody knows 
if there is any better approach to distinguish Xeon Scalable CPUs and Xeon W 
CPUs besides using brand string or using marketing frequency from CPUID 16H to 
determine crystal clock frequency (this is what Linux does at the moment)?

3. Intel Atom Denverton with CPUID signature (06_5FH), also known as Goldmont 
X, reports 0 crystal clock frequency as well, and has 25 MHz frequency. This is 
missing from SDM, but once again I believe it should be included in the two 
places from the above to avoid confusion.

Besides these 3 points, honestly, the library itself appears to be very limited 
for anything but embedding it into the firmware with known hardware, which 
already works just fine by defining the PCD. Missing 0 crystal clock frequency 
handling in runtime with hardcoding the PCD value looks very bad, because the 
number of modern Intel CPU models reporting 0 crystal clock frequency and 
having non 24 MHz frequency is quite far from 0. This makes the library 
essentially impossible to use in any UEFI application or third-party product 
even when targeting Skylake+ generation. To resolute this I vote for additional 
detection functionality to be added to the library to obtain crystal clock 
frequency when the CPUID reports 0. The PCD could be the last resort when no 
other method works. My opinion is that this should be resolved prior to merging 
the patch.

Best regards,

Groups.io Links: You receive all messages sent to this group.

View/Reply Online (#45795): https://edk2.groups.io/g/devel/message/45795
Mute This Topic: https://groups.io/mt/32839184/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub  [arch...@mail-archive.com]

Reply via email to