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. Perhaps, 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=,,,100,0,0,0::recentpostdate%2Fsticky,,,100,2,0,32891598

> 16 авг. 2019 г., в 9:56, Kuo, Donald <donald....@intel.com> написал(а):
> 
> 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. J
>
> Thanks,
> Donald
>   <>
>  <>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
>
> 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, 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.
>
> Thanks,
>
> Mike
>
> From: devel@edk2.groups.io <mailto: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,
> Vitaly
> 
> 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)
>
> Thanks,
> Donald
>
> 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>>; 
> 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
>
> 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 (#45807): https://edk2.groups.io/g/devel/message/45807
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]
-=-=-=-=-=-=-=-=-=-=-=-

Attachment: signature.asc
Description: OpenPGP digital signature

Reply via email to