[edk2-devel] [PATCH] UefiCpuPkg: Adding a new TSC library by using CPUID(0x15) TSC leaf

[Donald Kuo]

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)

Thanks,
Donald

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

Hello,

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 !!\n"));
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 !!\n"));
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 06_55H(25MHz).<BR>\n"
# Intel Xeon Processor Scalable Family with CPUID signature 06_55H = 25000000 (25MHz)

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,
Vitaly

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

View/Reply Online (#45699): https://edk2.groups.io/g/devel/message/45699
Mute This Topic: https://groups.io/mt/32839184/1813853
Group Owner: devel+owner at edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub  [edk2-devel-archive at redhat.com]
-=-=-=-=-=-=-=-=-=-=-=-

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://listman.redhat.com/archives/edk2-devel-archive/attachments/20190815/a2c694f9/attachment.htm>