From mboxrd@z Thu Jan 1 00:00:00 1970 Subject: Re: [edk2-devel] [PATCH] UefiCpuPkg: Adding a new TSC library by using CPUID(0x15) TSC leaf To: Donald Kuo ,devel@edk2.groups.io From: "Vitaly Cheptsov" X-Originating-Location: Moscow, RU (77.232.9.83) X-Originating-Platform: Mac Safari 12.1 User-Agent: GROUPS.IO Web Poster MIME-Version: 1.0 Date: Thu, 15 Aug 2019 00:20:27 -0700 References: <20190815043748.1684-1-donald.kuo@intel.com> In-Reply-To: <20190815043748.1684-1-donald.kuo@intel.com> Message-ID: <28715.1565853627948551264@groups.io> Content-Type: multipart/alternative; boundary="aWi8nXjqpSH2D345b0hZ" --aWi8nXjqpSH2D345b0hZ Content-Type: text/plain; charset="utf-8" Content-Transfer-Encoding: quoted-printable Hello, Thank you for the patch! I reviewed the code and noticed select issues exp= lained below. 1. The following construction: if (RegEbx =3D=3D 0) { DEBUG ((DEBUG_ERROR, "The CPU is not capble for Core Crystal Clock Frequen= cy !!\n")); ASSERT (RegEbx !=3D 0); } Does not look good to me, and in my opinion, should be written differently= : if (RegEbx =3D=3D 0 || RegEax =3D=3D 0 ) { DEBUG ((DEBUG_ERROR, "The CPU is not capble for Core Crystal Clock Frequen= cy !!\n")); ASSERT (RegEbx !=3D 0); ASSERT (RegEax !=3D 0); return 0; } The reason for the above code being wrong is potential division by zero be= low, which behaviour is undefined (and in fact unknown due to unspecified i= nterrupt table state). Even though the intent is to not permit the use of t= his library on an unsupported platform, runtime checks and assertions are e= ssentially NO-OPs, should be separate and not confused. For this to work pr= operly the call to CpuidCoreClockCalculateTscFrequency should happen in lib= rary constructor with EFI_UNSUPPORTED return on CpuidCoreClockCalculateTscF= requency returning 0. 2. The notes about crystal clock frequency for 06_55H CPU signature: "25000000 - Intel Xeon Processor Scalable Family with CPUID signature 06_5= 5H(25MHz).
\n" # Intel Xeon Processor Scalable Family with CPUID signature 06_55H =3D 250= 00000 (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 freq= uency, 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 kn= ows if there is any better approach to distinguish Xeon Scalable CPUs and X= eon W CPUs besides using brand string or using marketing frequency from CPU= ID 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 Goldm= ont 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 i= n the two places from the above to avoid confusion. Besides these 3 points,=C2=A0honestly, the library itself appears to be ve= ry limited for anything but embedding it into the firmware with known hardw= are, which already works just fine by defining the PCD. Missing=C2=A00 crys= tal clock frequency handling in runtime with hardcoding the PCD value looks= very bad, because the number of modern Intel CPU models reporting 0 crysta= l 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 t= his I vote for additional detection functionality to be added to the librar= y to obtain crystal clock frequency when the CPUID reports 0.=C2=A0The PCD = could be the last resort when no other method works. My opinion=C2=A0is tha= t this should be resolved prior to merging the patch. Best regards, Vitaly --aWi8nXjqpSH2D345b0hZ Content-Type: text/html; charset="utf-8" Content-Transfer-Encoding: quoted-printable Hello,

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

1. The following construction:
if (RegEbx =3D=3D 0) {
DEBUG ((DEBUG_ERROR, "The CPU is not capble for Core Crystal Clock Fr= equency !!\n"));
ASSERT (RegEbx !=3D 0);
}

Does not look good to me, and in my opinion, should be written diff= erently:

if (RegEbx =3D= =3D 0 || RegEax =3D=3D 0) {
DEBUG ((DEBUG_ERROR= , "The CPU is not capble for Core Crystal Clock Frequency !!\n"));
ASSERT (RegEbx !=3D 0);<= br style=3D"white-space: pre-wrap;" />  ASSERT (RegEax !=3D 0);
&= nbsp; return 0;
}

The reason for the above code being w= rong 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 Xe= on Processor Scalable Family with CPUID signature 06_55H(25MHz).<BR>\= n"
# Intel Xeon Processo= r Scalable Family with CPUID signature 06_55H =3D 25000000 (25MHz)

are misleading in b= oth this library and Intel SDM. Intel Xeon W processors have the same CPUID= signature (06_55H), they rep= ort 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 m= any Intel guys are here, I wonder whether anybody knows if there is any bet= ter approach to distinguish Xeon Scalable CPUs and Xeon W CPUs besides usin= g brand string or using marketing frequency from CPUID 16H to determine cry= stal clock frequency (this is what Linux does at the moment)?
<= br />3. Intel Atom Denverton with CPUID signature (06_5FH), also known as G= oldmont X, reports 0 crystal clock frequency as well, and has 25 MHz freque= ncy. This is missing from SDM, but once again I believe it should be includ= ed in the two places from the above to avoid confusion.

Besides = these 3 points, honestly, the library itself appears to be very limite= d for anything but embedding it into the firmware with known hardware, whic= h 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 f= requency and having non 24 MHz frequency is quite far from 0. This makes th= e library essentially impossible to use in any UEFI application or third-pa= rty product even when targeting Skylake+ generation. To resolute this I vot= e for additional detection functionality to be added to the library to obta= in 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 sh= ould be resolved prior to merging the patch.

Best regards,
= Vitaly --aWi8nXjqpSH2D345b0hZ--