From mboxrd@z Thu Jan 1 00:00:00 1970 Authentication-Results: mx.groups.io; dkim=pass header.i=@protonmail.com header.s=default header.b=x4K1RkKw; spf=pass (domain: protonmail.com, ip: 185.70.40.22, mailfrom: vit9696@protonmail.com) Received: from mail2.protonmail.ch (mail2.protonmail.ch [185.70.40.22]) by groups.io with SMTP; Thu, 15 Aug 2019 05:09:55 -0700 Date: Thu, 15 Aug 2019 12:09:48 +0000 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=protonmail.com; s=default; t=1565870992; bh=FW1yayLyCHWRYqDxkBg6kxaKX+XO0px29ucd2A/Ww4M=; h=Date:To:From:Cc:Reply-To:Subject:In-Reply-To:References: Feedback-ID:From; b=x4K1RkKw9Ma0ixxgZDyOE6O+JAJ7zL8CvRZgvjk/od/C4z4s9NECiudTvE04BMKI4 JkFH3ffESUo3f2xd7b+vvkrLUX+Oj2K7mxPJiiuVep3n+H5h3f3BiaT8Y5uC2AlMBL /Mkd7GPAFiJwaSvaLRmV1gjLgomrKXZjH5+/yTeY= To: donald.kuo@intel.com From: "Vitaly Cheptsov" Cc: "devel@edk2.groups.io" Reply-To: "vit9696@protonmail.com" Subject: Re: [edk2-devel] [PATCH] UefiCpuPkg: Adding a new TSC library by using CPUID(0x15) TSC leaf Message-ID: <55634608-237E-46ED-BCF8-85A5EC333EA4@protonmail.com> In-Reply-To: References: <20190815043748.1684-1-donald.kuo@intel.com> <28715.1565853627948551264@groups.io> Feedback-ID: p9QuX-L1wMgUm6nrSvNrf8juLupNs0VSnzXGVXuYDxlEahFdWtaedWDMB9zpwGDklGt7kzs1-RBc0cqz327Gcg==:Ext:ProtonMail MIME-Version: 1.0 X-Spam-Status: No, score=-0.7 required=7.0 tests=ALL_TRUSTED,DKIM_SIGNED, DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,FREEMAIL_ENVFROM_END_DIGIT, FREEMAIL_FROM,FREEMAIL_REPLYTO_END_DIGIT,HTML_MESSAGE autolearn=no autolearn_force=no version=3.4.2 X-Spam-Checker-Version: SpamAssassin 3.4.2 (2018-09-13) on mail.protonmail.ch X-Groupsio-MsgNum: 45718 Content-Type: multipart/signed; protocol="application/pgp-signature"; micalg=pgp-sha256; boundary="---------------------3c5e2c6d1a7b4dc5a26d8d7aff630daa"; charset=UTF-8 -----------------------3c5e2c6d1a7b4dc5a26d8d7aff630daa Cc: "devel@edk2.groups.io" , "Dong, Eric" , "Ni, Ray" , "Gao, Liming" Content-Type: multipart/alternative; boundary="Apple-Mail=_8DC9A08C-EDED-4CDD-9A2A-4A50AEE3B2BC" Date: Thu, 15 Aug 2019 15:09:46 +0300 From: "vit9696@protonmail.com" In-Reply-To: Message-Id: <55634608-237E-46ED-BCF8-85A5EC333EA4@protonmail.com> Mime-Version: 1.0 (Mac OS X Mail 12.4 \(3445.104.11\)) References: <20190815043748.1684-1-donald.kuo@intel.com> <28715.1565853627948551264@groups.io> Subject: Re: [edk2-devel] [PATCH] UefiCpuPkg: Adding a new TSC library by using CPUID(0x15) TSC leaf To: "Kuo, Donald" X-Mailer: Apple Mail (2.3445.104.11) --Apple-Mail=_8DC9A08C-EDED-4CDD-9A2A-4A50AEE3B2BC Content-Transfer-Encoding: quoted-printable Content-Type: text/plain; charset=utf-8 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 clie= nt 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 d= ifference between Xeon client and server. For us it is needed as we use TimerLib differently. For you TimerLib is a p= art 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 1= 5H, 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 ex= act library, but it will be helpful to hear relevant technical details on t= he issue and opinions on such "generic TimerLib" library to later appear in= EDK II. Best regards, Vitaly > 15 =D0=B0=D0=B2=D0=B3. 2019 =D0=B3., =D0=B2 11:40, Kuo, Donald =D0=BD=D0=B0=D0=BF=D0=B8=D1=81=D0=B0=D0=BB(=D0=B0): >=20 > Hi Vitaly, > =20 > 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. > =20 > Item #1 > - I do missed RegEax error handling in case for CpuidCoreClockCa= lculateTscFrequency () should return 0 and EFI_UNSUPPORTED. AR: Will update= it. > =20 > Item #2: > - Actually the information is from SDM Table 18-85 > o As we know CPUID signature could be hard to identify processor XTAL f= requency is? So we only check CPUID Leaf 0x15 > o And the PCD will be defined depends on platform specific and during p= roject early development. Based on SDM, Intel processor for CPUID.15h EAX a= nd 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: > =C2=A7 Intel Xeon Server family: 25MHz > =C2=A7 Intel Core and Intel Xeon W family: 24MHz > =C2=A7 Intel Atom : 19.2MHz > =C2=A7 So regards the =E2=80=9C06_55h=E2=80=9D 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) > =20 > Thanks, > Donald > =C2=A0 <> > <>From: vit9696 via [] [mailto:vit9696=3Dprotonmail.com @[]]=20 > Sent: Thursday, August 15, 2019 3:20 PM > To: Kuo, Donald >; dev= el@edk2.groups.io > Subject: Re: [edk2-devel] [PATCH] UefiCpuPkg: Adding a new TSC library by= using CPUID(0x15) TSC leaf > =20 > Hello, >=20 > Thank you for the patch! I reviewed the code and noticed select issues ex= plained below. >=20 > 1. The following construction: >=20 > if (RegEbx =3D=3D 0) { > DEBUG ((DEBUG_ERROR, "The CPU is not capble for Core Crystal Clock Freque= ncy !!\n")); > ASSERT (RegEbx !=3D 0); > } >=20 > Does not look good to me, and in my opinion, should be written differentl= y: >=20 > if (RegEbx =3D=3D 0 || RegEax =3D=3D 0) { > DEBUG ((DEBUG_ERROR, "The CPU is not capble for Core Crystal Clock Freque= ncy !!\n")); > ASSERT (RegEbx !=3D 0); > ASSERT (RegEax !=3D 0); > return 0; > } >=20 > The reason for the above code being wrong is potential division by zero b= elow, 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 p= roperly the call to CpuidCoreClockCalculateTscFrequency should happen in li= brary constructor with EFI_UNSUPPORTED return on CpuidCoreClockCalculateTsc= Frequency returning 0. >=20 > 2. The notes about crystal clock frequency for 06_55H CPU signature: > "25000000 - Intel Xeon Processor Scalable Family with CPUID signature 06_= 55H(25MHz).
\n" > # Intel Xeon Processor Scalable Family with CPUID signature 06_55H =3D 25= 000000 (25MHz) >=20 > are misleading in both this library and Intel SDM. Intel Xeon W processor= s have the same CPUID signature (06_55H), they report 0 crystal clock frequ= ency, 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)= . >=20 > Actually, given that many Intel guys are here, I wonder whether anybody k= nows if there is any better approach to distinguish Xeon Scalable CPUs and = Xeon W CPUs besides using brand string or using marketing frequency from CP= UID 16H to determine crystal clock frequency (this is what Linux does at th= e moment)? >=20 > 3. Intel Atom Denverton with CPUID signature (06_5FH), also known as Gold= mont 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. >=20 > Besides these 3 points, honestly, the library itself appears to be very l= imited 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 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 l= ast resort when no other method works. My opinion is that this should be re= solved prior to merging the patch. >=20 > Best regards, > Vitaly=20 --Apple-Mail=_8DC9A08C-EDED-4CDD-9A2A-4A50AEE3B2BC Content-Transfer-Encoding: quoted-printable Content-Type: text/html; charset=utf-8 Hi Donald,
<= br class=3D"">Glad to hear it helped a little, and sorry for some outdated = quotes.

Your clarification regarding model ran= ge 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 s= ame SDM table, and it would have been great to have vertical range bin= ding instead of the misleading CPUID.

With tha= t, however, I still do not see a way to dynamically determine the differenc= e 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 u= s TimerLib is used as a part of an UEFI application compatible with differe= nt hardware, and in fact not just Intel. We have such "generic TimerLi= b" 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 o= f 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 pe= rhaps even this exact library, but it will be helpful to hear relevant tech= nical details on the issue and opinions on such "generic TimerLib" library = to later appear in EDK II.

Best regards,
Vitaly

15 =D0=B0=D0=B2=D0=B3. 2019 =D0=B3., =D0=B2 11:= 40, Kuo, Donald <dona= ld.kuo@intel.com> =D0=BD=D0=B0=D0=BF=D0=B8=D1=81=D0=B0=D0=BB(=D0=B0)= :

Hi Vitaly,
 
Appreciated fo= r reviewing very detail. And looks your captured some piece of code is olde= r version. And should be ok, I do got your point.
 
Item #1
-        &nbs= p; <= span style=3D"font-size: 11pt; font-family: Calibri, sans-serif; color: rgb= (31, 73, 125);" class=3D"">I do missed RegEax error handling in case for Cp= uidCoreClockCalculateTscFrequency () should return 0 and EFI_UNSU= PPORTED. AR: Will update it.
 
Item #2:
-          Actually the information is from SDM Table 18-85<= /o:p>
o   As = we know CPUID signature could be hard to identify processor XTAL frequency = is? So we only check CPUID Leaf 0x15
o   <= /span>And the PCD will be defined= depends on platform specific and during project early development. Based o= n SDM, Intel processor for CPUID.15h EAX and EBX is enumerated, but ECX cou= ld be possible not enumerated. So that is why we based on  SDM Table 1= 8-85 to create PCD as below:
=C2=A7=   Intel Xeon Server family: 25MHz
=C2=A7 &= nbsp;Intel Core and= Intel Xeon W family: 24MHz
=C2=A7 = ; Intel Atom : 19.2MHz=
=C2=A7  So= regards the =E2=80=9C06_55h=E2=80=9D might cause the confused, we could re= view it whether remove it??
Item #3:<= o:p class=3D"">
= -    &nbs= p;     Again, the XTAL frequenc= y is optional and should be define in early phase of new project. Default s= hould still use AcpiTimer.
o    Platform / developer owner shou= ld well know the CPU generation XTAL frequency is? Server / Client / Atom ?=
o   For those CPU not supported should still use AcpiTimerLib (de= fault)
 
T= hanks,
Donald
 
From: vit9696 via [] [mailto:vit9696=3Dprotonmail.com@[]] 
Sent: Thursday, August 15, 2019 3:20 PM
To: Kuo, Donald <donald.kuo@int= el.com>; devel@edk2.groups.io
Subject: R= e: [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 =3D=3D 0) {
DEBUG ((DEBUG_ERR= OR, "The CPU is not capble for Core Crystal Clock Frequency !!\n"));
ASSERT (RegEbx !=3D 0);
}

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

if (RegEbx =3D=3D 0 || RegEax =3D=3D 0) {DEBUG ((DEBUG_ERROR, "The CPU is not capble for Core Crystal C= lock Frequency !!\n"));
ASSERT (RegEbx !=3D 0);
  ASSERT (RegEax !=3D 0);
  return 0;
}

The reason for the above code being wrong = is potential division by zero below, which behaviour is undefined (and in f= act unknown due to unspecified interrupt table state). Even though the inte= nt is to not permit the use of this library on an unsupported platform, run= time checks and assertions are essentially NO-OPs, should be separate and n= ot confused. For this to work properly the call to CpuidCoreClockCalcu= lateTscFrequency should happen in library constructor with EFI_UNSUPPORTED = return on CpuidCoreClockCalculateTscFrequency returning 0.
2. The notes about crystal clock frequency for 06_55H CPU sign= ature:
"25000000 - Intel Xeon Processor Scalable Family with = CPUID signature 06_55H(25MHz).<BR>\n"
# Intel Xeon Proc= essor Scalable Family with CPUID signature 06_55H =3D 25000000 (25MHz)

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

Actually, given = that many Intel guys are here, I wonder whether anybody knows if there is a= ny better approach to distinguish Xeon Scalable CPUs and Xeon W CPUs beside= s using brand string or using marketing frequency from CPUID 16H to determi= ne 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 libra= ry itself appears to be very limited for anything but embedding it into the= firmware with known hardware, which already works just fine by defining th= e PCD. Missing 0 crystal clock frequency handling in runtime with hard= coding 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 CPUI= D reports 0. The PCD could be the last resort when no other method wor= ks. My opinion is that this should be resolved prior to merging the pa= tch.

Best regards,
Vitaly 

--Apple-Mail=_8DC9A08C-EDED-4CDD-9A2A-4A50AEE3B2BC-- -----------------------3c5e2c6d1a7b4dc5a26d8d7aff630daa Content-Type: application/pgp-signature; name="signature.asc" Content-Description: OpenPGP digital signature Content-Disposition: attachment; filename="signature.asc" -----BEGIN PGP SIGNATURE----- Version: ProtonMail wsBmBAEBCAAQBQJdVUuLCRBPsoxt7Hy0xQAKCRBPsoxt7Hy0xbNaCAA1GYe9 fzMtbkRPNhsEJiC9JMVeEUUJ/CakU74fnulzxwxSWg9XSDxMJ/z2DKxHFAlF upnwQmn+eW7f22tUoXljd4Ga5FhVR1u+oXadwR5+lVbhzr6TMTycG8Q9n72o 43XV+qm87A8HQuR3v9UWKIFmHTfIN2AYgWq6R2dJ24iw49xysEArMMbLbiPO AeNuKavgBrg+mm6tTCdEzoAxqAOPhxdhRMdXQzzKV4kxyqO6HXFTHN/eHq+9 9BStXEBH2ip8KzjfHT/NQPEDEBLC6XaP9/ga4BURgl1O2sUvOtrCMXCsRg6L 9pw+VI3Lqv8S8/enz/AQdGzXhhJdpCa32VyN =bTM7 -----END PGP SIGNATURE----- -----------------------3c5e2c6d1a7b4dc5a26d8d7aff630daa--