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=PGniLQAg; 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; Fri, 16 Aug 2019 01:07:16 -0700 Date: Fri, 16 Aug 2019 08:07:09 +0000 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=protonmail.com; s=default; t=1565942832; bh=T/gxohWplJgT8ws87tjFjG7yGChBkWvxtDOGGHcs11g=; h=Date:To:From:Cc:Reply-To:Subject:In-Reply-To:References: Feedback-ID:From; b=PGniLQAgciKsnsI4or+Ka3pk9gSoTss3WGNf0HgvS3lpz3a7xf3HojLZoQLaTQre+ t5f/psrZ7M7enXzxLjsxHF37a7wQgs549EsJwTaQ7YkVKXudqT27R/GanMgrLyLPhk lqd0WrsJuCpk0bWWHNqjus39n36QX0BqaqUQ3Cbw= 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: <4E002E33-36D2-40D8-B84A-D28F42988952@protonmail.com> In-Reply-To: References: <20190815043748.1684-1-donald.kuo@intel.com> <28715.1565853627948551264@groups.io> <55634608-237E-46ED-BCF8-85A5EC333EA4@protonmail.com> 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_FONT_LOW_CONTRAST, 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: 45807 Content-Type: multipart/signed; protocol="application/pgp-signature"; micalg=pgp-sha256; boundary="---------------------2734602f8ee441f0e2343d3e0186f266"; charset=UTF-8 -----------------------2734602f8ee441f0e2343d3e0186f266 Cc: "Kinney, Michael D" , "devel@edk2.groups.io" Content-Type: multipart/alternative; boundary="Apple-Mail=_A0BBCAE5-DB6A-4DD0-8068-B13849636A0F" Date: Fri, 16 Aug 2019 11:07:06 +0300 From: "vit9696@protonmail.com" In-Reply-To: Message-Id: <4E002E33-36D2-40D8-B84A-D28F42988952@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> <55634608-237E-46ED-BCF8-85A5EC333EA4@protonmail.com> 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=_A0BBCAE5-DB6A-4DD0-8068-B13849636A0F Content-Transfer-Encoding: quoted-printable Content-Type: text/plain; charset=utf-8 Donald, Michael, Yes, I see that such a use is quite new and unexpected very well by know. = I expanded my thoughts in a separate e-mail thread[1] as the consideration = opened up an apparently separate problem partially related to the patch. Pe= rhaps, we could continue the discussion there some time later. Best regards, Vitaly [1] Determining TSC frequency programmatically https://edk2.groups.io/g/devel/topic/determining_tsc_frequency/32891598?p= =3D,,,100,0,0,0::recentpostdate%2Fsticky,,,100,2,0,32891598 > 16 =D0=B0=D0=B2=D0=B3. 2019 =D0=B3., =D0=B2 9:56, Kuo, Donald =D0=BD=D0=B0=D0=BF=D0=B8=D1=81=D0=B0=D0=BB(=D0=B0): >=20 > Hi Vitaly, > > UEFI Application does be another scope. And regards your question on =E2= = =80=9Ca way to dynamically determine the difference between Xeon client an= d server=E2=80=9D =E2=80=A6 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 TimerL= ib in the future. J > > Thanks, > Donald > =C2=A0 <> > <>From: Kinney, Michael D=20 > Sent: Friday, August 16, 2019 12:23 AM > To: devel@edk2.groups.io; vit9696@protonmail.com; Kuo, Donald ; Kinney, Michael D > Subject: RE: [edk2-devel] [PATCH] UefiCpuPkg: Adding a new TSC library b= y using CPUID(0x15) TSC leaf > > Vitaly, > > When implementing a UEFI Application, if you want maximum compatibility,= you should use UEFI Services/Protocols and minimize as many HW assumptions= as possible. > > I understand, especially for accurate time and clock related services, t= he that the UEFI Specification defined Services/Protocols can be challengin= g to use. > > When adding content that uses HW such as TSC or CPUID the UEFI App shoul= d be implemented with good detection logic to know it is safe to use that H= W, and contain the fallback logic to use an alternate mechanism if the requ= ired 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. > > Thanks, > > Mike > > From: devel@edk2.groups.io [mailto:devel@e= dk2.groups.io ] On Behalf Of Vitaly Cheptsov v= ia Groups.Io > Sent: Thursday, August 15, 2019 5:10 AM > To: Kuo, Donald > > Cc: devel@edk2.groups.io > Subject: Re: [edk2-devel] [PATCH] UefiCpuPkg: Adding a new TSC library b= y using CPUID(0x15) TSC leaf > > Hi Donald, >=20 > Glad to hear it helped a little, and sorry for some outdated quotes. >=20 > Your clarification regarding model range is very helpful. Xeon W being c= lient and thus having client clock makes sense, though I must say it was qu= ite not obvious. I was referring to the same SDM table, and it would have b= een great to have vertical range binding instead of the misleading CPUID. >=20 > With that, however, I still do not see a way to dynamically determine th= e difference between Xeon client and server. >=20 > 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 compatib= le with different hardware, and in fact not just Intel. We have such "gener= ic TimerLib" library, and to determine CPU frequency, as a fallback to CPUI= D 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 pa= rt 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. >=20 > 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 o= n the issue and opinions on such "generic TimerLib" library to later appear= in EDK II. >=20 > Best regards, > Vitaly >=20 > 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): > > Hi Vitaly, > > Appreciated for reviewing very detail. And looks your captured some piec= e 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 CpuidCoreClockC= alculateTscFrequency () should return 0 and EFI_UNSUPPORTED. AR: Will updat= e 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: > =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) > > Thanks, > Donald > > From: vit9696 via [] [mailto:vit9696=3Dprotonmail.com @[]]=20 > Sent: Thursday, August 15, 2019 3:20 PM > To: Kuo, Donald >; de= vel@edk2.groups.io > Subject: Re: [edk2-devel] [PATCH] UefiCpuPkg: Adding a new TSC library b= y using CPUID(0x15) TSC leaf > > Hello, >=20 > Thank you for the patch! I reviewed the code and noticed select issues e= xplained below. >=20 > 1. The following construction: >=20 > if (RegEbx =3D=3D 0) { > DEBUG ((DEBUG_ERROR, "The CPU is not capble for Core Crystal Clock Frequ= ency !!\n")); > ASSERT (RegEbx !=3D 0); > } >=20 > Does not look good to me, and in my opinion, should be written different= ly: >=20 > if (RegEbx =3D=3D 0 || RegEax =3D=3D 0) { > DEBUG ((DEBUG_ERROR, "The CPU is not capble for Core Crystal Clock Frequ= ency !!\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 = 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 l= ibrary constructor with EFI_UNSUPPORTED return on CpuidCoreClockCalculateTs= cFrequency 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 2= 5000000 (25MHz) >=20 > are misleading in both this library and Intel SDM. Intel Xeon W processo= rs 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= ). >=20 > 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 C= PUID 16H to determine crystal clock frequency (this is what Linux does at t= he moment)? >=20 > 3. Intel Atom Denverton with CPUID signature (06_5FH), also known as Gol= dmont X, reports 0 crystal clock frequency as well, and has 25 MHz frequenc= y. 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 = limited for anything but embedding it into the firmware with known hardware= , which already works just fine by defining the PCD. Missing 0 crystal cloc= k frequency handling in runtime with hardcoding the PCD value looks very ba= d, 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 t= he library essentially impossible to use in any UEFI application or third-p= arty product even when targeting Skylake+ generation. To resolute this I vo= te for additional detection functionality to be added to the library to obt= ain 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 r= esolved prior to merging the patch. >=20 > Best regards, > Vitaly=20 > >=20 --Apple-Mail=_A0BBCAE5-DB6A-4DD0-8068-B13849636A0F Content-Transfer-Encoding: quoted-printable Content-Type: text/html; charset=utf-8 Donald, Michael,

Yes, I see that such a use is q= uite new and unexpected very well by know. I expanded my thoughts in a sepa= rate e-mail thread[1] as the consideration opened up an apparently separate= problem partially related to the patch. Perhaps, we could continue the dis= cussion there some time later.

Best regards,
Vitaly

[1] Determining TSC frequency p= rogrammatically
https://edk2.groups.io/g/devel/t= opic/determining_tsc_frequency/32891598?p=3D,,,100,0,0,0::recentpostdate%2F= sticky,,,100,2,0,32891598

16 =D0=B0=D0=B2=D0=B3. 2019 =D0= =B3., =D0=B2 9:56, Kuo, Donald <donald.kuo@intel.com> =D0=BD=D0=B0= =D0=BF=D0=B8=D1=81=D0=B0=D0=BB(=D0=B0):

Hi Vitaly,
 
UEFI Application=  does be another scope. A= nd regards your question on =E2=80=9Ca way to dynamically determine = the difference between Xeon client and server= =E2=80=9D =E2=80=A6 is not in current design-in, for long term goal we cou= ld consider if there is proper way to identify CPU SKU dynamically.
 
Thanks for sh= aring your thought and it could be an enhancement on TimerLib in the future= . J
 
Thanks,
Donald=
 
From: Kinney, Mich= ael D 
Sent: Frida= y, August 16, 2019 12:23 AM
To: devel@edk2.groups.io; vit9696@prot= onmail.com; Kuo, Donald <donald.kuo@intel.com>; Kinney, Michael D <= ;michael.d.kinney@intel.com>
Subject: RE: [edk2-devel] [PATCH] Ue= fiCpuPkg: Adding a new TSC library by using CPUID(0x15) TSC leaf
 
Vitaly,
=  
When implementing a UEFI Application, = if you want maximum compatibility, you should use UEFI Services/Protocols a= nd minimize as many HW assumptions as possible.
 
I understand, especially for accura= te time and clock related services, the that the UEFI Specification defined= Services/Protocols can be challenging to use.=
 
When adding content that uses HW suc= h as TSC or CPUID the UEFI App should be implemented with good detection lo= gic to know it is safe to use that HW, and contain the fallback logic to us= e an alternate mechanism if the required HW is not present.  This is a= very different approach than some early FW initialization code that can sa= fely make some HW assumptions that helps keep the FW init code small and ef= ficient.  This does imply that different libraries may be required for= FW init and UEFI Applications.
 
Thanks,
 
Mike
 
From: devel@edk2.groups.io [ma= ilto:devel@edk2.groups.io] <= /span>On Behalf Of = ;Vitaly Cheptsov via Groups.Io
Sent:=  Thursday, August 15,= 2019 5:10 AM
To: Kuo, Donald <donal= d.kuo@intel.com>
Cc: 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 f= or some outdated quotes.

Your clarification re= garding model range is very helpful. Xeon W being client and thus having cl= ient clock makes sense, though I must say it was quite not obvious. I was r= eferring to the same SDM table, and it would have been great to have vertic= al range binding instead of the misleading CPUID.

With that, however, I still do not see a way to dynamically determ= ine 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 freque= ncy as a PCD. For us TimerLib is used as a part of an UEFI application comp= atible with different hardware, and in fact not just Intel. We have su= ch "generic TimerLib" library, and to determine CPU frequency, as a fallbac= k to CPUID 15H, it relies on ACPI PM timer. This is not great for two reaso= ns:
- we have to maintain it ourselves, while we would have l= iked that be part of EDK II with different vendors contributing to the proj= ect.
- we still cannot find an obvious way to determine cryst= al clock when it is not reported, in particular for Xeon W and Xeon Scalabl= e case.

I guess this is now out of the scope o= f 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,
Vitaly

15 =D0=B0=D0=B2=D0= =B3. 2019 =D0=B3., =D0=B2 11:40, Kuo, Donald <donald.kuo@intel.com> =D0=BD=D0=B0=D0=BF=D0=B8=D1=81=D0=B0=D0= = =BB(=D0=B0):
 
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
-     &nbs= p;    I do missed RegEax error handling in case fo= r CpuidCoreClockCalculateTscFrequency () should return 0 and EFI_= UNSUPPORTED. AR: Will update it.
 
Item #= 2:
-      &nb= sp;   Actually the information is from SDM Table 18-85<= /span>
<= span style=3D"font-size: 11pt; font-family: "Courier New"; color:= rgb(31, 73, 125);" class=3D"">o   As we know CPUID sig= nature could be hard to identify processor XTAL frequency is? So we only ch= eck CPUID Leaf 0x15
o   And the PCD will be defined depends on platform specific and during projec= t early development. Based on SDM, Intel processor for CPUID.15h EAX and EB= X is enumerated, but ECX could be possible not enumerated. So that is why w= e 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
<= span style=3D"font-size: 11pt; font-family: Wingdings; color: rgb(31, 73, 1= 25);" class=3D"">=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 whet= her remove it??
Item #3:
-     &nb= sp;    Again, the XTAL frequency is optional and s= hould be define in early phase of new project. Default should still use Acp= iTimer.
o    Platfo= rm / 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)<= /span>
 
Thanks,
Donald
 
From: vit9696 via [] [mai= lto:vit9696=3Dprotonmail.com@[]] 
Sent: Thursday, August 15, 2019 3:20 PM
To: Kuo, Donald <donald.kuo@intel.com>; devel@edk2.groups.io=
Subject: Re: [edk2-devel] [PATCH] UefiCpuPkg: Adding a new TSC = library by using CPUID(0x15) TSC leaf
&= nbsp;
Hello,

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

1. The following construction:

if (RegEbx =3D=3D 0) {
DEBUG ((DEBUG_ERROR, "The C= PU is not capble for Core Crystal Clock Frequency !!\n"));
AS= SERT (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 Frequ= ency !!\n"));
ASSERT (RegEbx !=3D 0);
  AS= SERT (RegEax !=3D 0);
  return 0;
}

The reason for the above code being wrong is potenti= al division by zero below, which behaviour is undefined (and in fact unknow= n due to unspecified interrupt table state). Even though the intent is to n= ot permit the use of this library on an unsupported platform, runtime check= s and assertions are essentially NO-OPs, should be separate and not confuse= d. For this to work properly the call to CpuidCoreClockCalculateTscFre= quency 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 sign= ature 06_55H(25MHz).<BR>\n"
# Intel Xeon Processor Scal= able Family with CPUID signature 06_55H =3D 25000000 (25MHz)
=
are misleading in both this library and Intel SDM. Intel Xeo= n W processors have the same CPUID signature (06_55H), they report 0 crysta= l clock frequency, and their crystal clock frequency is 24 MHz. This should= at least be mentioned in the lower part mentioning Intel Xeon W Proce= ssor 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 br= and 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 kno= wn 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. Mis= sing 0 crystal clock frequency handling in runtime with hardcoding the= PCD value looks very bad, because the number of modern Intel CPU models re= porting 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 UEF= I application or third-party product even when targeting Skylake+ generatio= n. To resolute this I vote for additional detection functionality to be add= ed 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 opi= nion is that this should be resolved prior to merging the patch.

Best regards,
Vitaly 
<= /div>
 

<= /div> --Apple-Mail=_A0BBCAE5-DB6A-4DD0-8068-B13849636A0F-- -----------------------2734602f8ee441f0e2343d3e0186f266 Content-Type: application/pgp-signature; name="signature.asc" Content-Description: OpenPGP digital signature Content-Disposition: attachment; filename="signature.asc" -----BEGIN PGP SIGNATURE----- Version: ProtonMail wsBmBAEBCAAQBQJdVmQsCRBPsoxt7Hy0xQAKCRBPsoxt7Hy0xWf4CABZcL8W 9bcWN1pMwu4Fl0LhX8omBHzT5VHKNynTq9fjMkIu1SzVfvh3JRPRKeSVZ6AK MqeGT/AfntZ0MsgQetHSo3ST36BBT6mqC+ufcrJJfPbHeFJETSOy3yIkjb7p ukP6EPjCvgKHjU6redOxh0NDImhYZcRLEQDit8/cxUwqRPZuOMrG4i6GuBuE v/UPLLhQ3TNr2sKikFAWI21a5L2zXy/K0KwcnBZXtEeTg0bfBHpTPdI2gAEX mf9O4ZCMR8nypaUGOy0hAVHzvDNAL7SKZNAarWFC6xpQrqwrkK4vNMbpMXXX o4nEhAp+QNXpE5PdGZu7Lfoa9MXtGMDkJ4KI =ZsKL -----END PGP SIGNATURE----- -----------------------2734602f8ee441f0e2343d3e0186f266--