Re: [edk2-devel] [PATCH v3 11/39] UefiCpuPkg: Add LoongArch64 CPU Timer library

["Chao Li" <lichao@loongson.cn>]

[-- Attachment #1: Type: text/plain, Size: 11224 bytes --]

Hi Laszlo,

Thanks for reviewing again this patch while you are sick and please take 
care.

My opinion is as follow:


Thanks,
Chao
On 2023/12/12 01:16, Laszlo Ersek wrote:
> On 11/23/23 12:43, Chao Li wrote:
>> On 2023/11/23 00:12, Laszlo Ersek wrote:
>>> On 11/17/23 11:00, Chao Li wrote:
>>>> Add the LoongArch64 CPU Timer library, using CPUCFG 0x4 and 0x5 for
>>>> Stable Counter frequency.
>>>>
>>>> BZ:https://bugzilla.tianocore.org/show_bug.cgi?id=4584
>>>>
>>>> Cc: Eric Dong<eric.dong@intel.com>
>>>> Cc: Ray Ni<ray.ni@intel.com>
>>>> Cc: Rahul Kumar<rahul1.kumar@intel.com>
>>>> Cc: Gerd Hoffmann<kraxel@redhat.com>
>>>> Signed-off-by: Chao Li<lichao@loongson.cn>
>>>> ---
>>>>   .../BaseLoongArch64CpuTimerLib.inf            |  30 +++
>>>>   .../BaseLoongArch64CpuTimerLib.uni            |  15 ++
>>>>   .../BaseLoongArch64CpuTimerLib/CpuTimerLib.c  | 226 ++++++++++++++++++
>>>>   UefiCpuPkg/UefiCpuPkg.dsc                     |   3 +
>>>>   4 files changed, 274 insertions(+)
>>>>   create mode 100644 UefiCpuPkg/Library/BaseLoongArch64CpuTimerLib/BaseLoongArch64CpuTimerLib.inf
>>>>   create mode 100644 UefiCpuPkg/Library/BaseLoongArch64CpuTimerLib/BaseLoongArch64CpuTimerLib.uni
>>>>   create mode 100644 UefiCpuPkg/Library/BaseLoongArch64CpuTimerLib/CpuTimerLib.c
>>> (1) sorry about the annoying comment, but the library should be
>>> called (preferably) BaseTimerLibLoongArch64Cpu.
>>>
>>> We're frequently not consistent with the following library instance
>>> naming scheme, but in theory anyway, library instances should be
>>> named as follows:
>>>
>>>    <firmware module type><lib class name><library instance identifier>
>>>
>>> Thus, in this case, Base + TimerLib + LoongArch64Cpu.
>>>
>>> update UNI file name, INF file name, directory name, BASE_NAME and
>>> MODULE_UNI_FILE accordingly (if you agree)
>> There has a SPEC for naming style:
>>
>> https://github.com/tianocore-docs/edk2-CCodingStandardsSpecification/blob/master/4_naming_conventions/42_directory_names.md
>>
>> Please check 4.2.3 EDKII Library directory, and most directory naming
>> follows this SPEC.
> I didn't remember (or know about) this part of the coding standards
> spec; thanks for the pointer.
>
> It seems like my suggestion matches the second alternative
> (<Phase><LibraryClassName>[<Dependency>]).
>
> Your naming seems to follow the first alternative
> (<Phase>[<CpuArch>]<LibraryClassName>). OK.
>
> However, that's still not a perfect match. For <Phase>, you have "Base".
> For <CpuArch>, you have "LoongArch64". But for <LibraryClassName>, you
> have "CpuTimerLib", and that's wrong. We don't have a "CpuTimerLib"
> class; instead it's the "TimerLib" class.
The part of "CpuTimerLib" uses the name located at 
UefiCpuPkg/Library/CpuTimerLib, so this library is named 
BaseLoongArch64CpuTimerLib.
>
> The LIBRARY_CLASS define in your INF file also says "TimerLib", so
> "CpuTimerLib" is wrong / inconsistent in the lib instance name.
> "BaseLoongArch64TimerLib" should be fine. If you want to add "Cpu", I
> guess you could add it as [<Dependency>], as a suffix, under the first
> naming scheme alternative: "BaseLoongArch64TimerLibCpu".

I think the name "CpuTimerLib" highlights the "CPU", because this 
library relies on the CPU interal timer. So should I use the 
"CpuTimerLib" as <LibraryClassName>?

Ray, what do you suggest?

>
>>> (5) Relatedly: the TimerLib class is declared in "MdePkg.dec". Thus,
>>> if you indeed don't need anything from "UefiCpuPkg.dec", I'd suggest
>>> moving this library instance under MdePkg.
>> It is inappropriate to place this library in MdePkg, because the
>> MdePkg doesn't have any CpuTimerLib instance, and this class library
>> are HW implementation-related, so it more appropriate to place it in
>> UefiCpuPkg.
> This ties in with my above comment. The reason for MdePkg not having a
> "CpuTimerLib" instance is that there is no such lib class in edk2.
>
> The library class is called "TimerLib". We have multiple instances in
> edk2:
>
> $ git grep -l 'LIBRARY_CLASS *= *TimerLib\>' -- '*inf'
>
>    ArmPkg/Library/ArmArchTimerLib/ArmArchTimerLib.inf
>    EmulatorPkg/Library/DxeCoreTimerLib/DxeCoreTimerLib.inf
>    EmulatorPkg/Library/DxeTimerLib/DxeTimerLib.inf
>    EmulatorPkg/Library/PeiTimerLib/PeiTimerLib.inf
>    MdePkg/Library/BaseTimerLibNullTemplate/BaseTimerLibNullTemplate.inf
>    MdePkg/Library/SecPeiDxeTimerLibCpu/SecPeiDxeTimerLibCpu.inf
>    OvmfPkg/Library/AcpiTimerLib/BaseAcpiTimerLib.inf
>    OvmfPkg/Library/AcpiTimerLib/BaseAcpiTimerLibBhyve.inf
>    OvmfPkg/Library/AcpiTimerLib/BaseRomAcpiTimerLib.inf
>    OvmfPkg/Library/AcpiTimerLib/DxeAcpiTimerLib.inf
>    PcAtChipsetPkg/Library/AcpiTimerLib/BaseAcpiTimerLib.inf
>    PcAtChipsetPkg/Library/AcpiTimerLib/DxeAcpiTimerLib.inf
>    PcAtChipsetPkg/Library/AcpiTimerLib/PeiAcpiTimerLib.inf
>    PcAtChipsetPkg/Library/AcpiTimerLib/StandaloneMmAcpiTimerLib.inf
>    UefiCpuPkg/Library/BaseRiscV64CpuTimerLib/BaseRiscV64CpuTimerLib.inf
>    UefiCpuPkg/Library/CpuTimerLib/BaseCpuTimerLib.inf
>    UefiCpuPkg/Library/SecPeiDxeTimerLibUefiCpu/SecPeiDxeTimerLibUefiCpu.inf
>    UefiPayloadPkg/Library/AcpiTimerLib/AcpiTimerLib.inf
>
> This "prior art" suggests the new library instance could go in MdePkg or
> UefiCpuPkg alike.
>
> That's where my comment about "UefiCpuPkg.dec" dependencies becomes
> relevant. Compare:
>
> - MdePkg/Library/SecPeiDxeTimerLibCpu/SecPeiDxeTimerLibCpu.inf
> - UefiCpuPkg/Library/SecPeiDxeTimerLibUefiCpu/SecPeiDxeTimerLibUefiCpu.inf
>
> The difference between these library instances (as the INF files
> explain) is xAPIC vs. x2APIC support. The latter instance supports
> x2APIC. And, for x2APIC support, the latter instance depends on the
> LocalApicLib class. The LocalApicLib class is declared in
> "UefiCpuPkg.dec". And that is the dependency that forces the second
> library instance to exist under UefiCpuPkg.
>
> Thus, my original point stands -- if you need nothing from "UefiCpuPkg",
> then your library instance "BaseLoongArch64TimerLib" (or
> "BaseLoongArch64TimerLibCpu", if you like) should go under MdePkg, in my
> opinion anyway.
>
> (A different example:
> "UefiCpuPkg/Library/CpuTimerLib/BaseCpuTimerLib.inf" lives under
> UefiCpuPkg due to depending on "PcdCpuCoreCrystalClockFrequency", which
> also comes from "UefiCpuPkg.dec".)

I'm confused after you said it. In my opion, the libraries or driver 
instance that implements dependencies should be under the UefiCpuPkg, 
and the architecture common library or driver should be under the MdePkg.

Liming, Mike and Ray, what suggestions do you have?

>
>>>> +  VOID
>>>> +  )
>>>> +{
>>>> +  UINT32                 BaseFreq;
>>>> +  UINT32                 ClockMultiplier;
>>>> +  UINT32                 ClockDivide;
>>>> +  CPUCFG_REG4_INFO_DATA  CCFreq;
>>> (10) Per edk2 coding style, when acronyms and abbreviations are grouped
>>> together, each individual acronym itself is supposed to be CamelCase.
>>> For example, when we use "PCI" in identifiers, it becomes "Pci". So I
>>> think this variable should be called "CcFreq", where "CC" stands for
>>> "CPU Config". If I'm wrong about "CC", then please ignore this point.
>> The two letter 'C' means: first 'C' means Constant, and the second means
>> Crystal. In LoongArch SPEC, this area is named CC_FREQ, which means:
>> Constant frequency timer and the crystal frequency corresponding to the
>> clock used by the timer. So I think CCFreq is more suitable.
> "PCI" is also an acronym ("Peripheral Component Interconnect"), so "CC"
> for "Constant Crystal" is no different, from this perspective. The same
> way we write PciRead8(), the variable should be CcFreq.
Ok, I agree.
>
>>>> +  CPUCFG_REG5_INFO_DATA  CpucfgReg5Data;
>>>> +  UINT32                 StableTimerFreq;
>>>> +
>>>> +  //
>>>> +  // Get the the crystal frequency corresponding to the constant
>>>> +  // frequency timer and the clock used by the timer.
>>>> +  //
>>>> +  AsmCpucfg (CPUCFG_REG4_INFO, &CCFreq.Uint32);
>>>> +
>>>> +  //
>>>> +  // Get the multiplication factor and frequency division factor
>>>> +  // corresponding to the constant frequency timer and the clock
>>>> +  // used by the timer.
>>>> +  //
>>>> +  AsmCpucfg (CPUCFG_REG5_INFO, &CpucfgReg5Data.Uint32);
>>>> +
>>>> +  BaseFreq        = CCFreq.Bits.CC_FREQ;
>>> (11) My comment here probably affects the header file where you
>>> introduced the CC_FREQ field name. It should be CcFreq.
>>>
>>> Same for MUL and DIV below.
>> The same to above.
> I understand the LoongArch spec may call this "CC_FREQ", but that's just
> not how edk2 spells field names. "CC_FREQ" is not idiomatic, the field
> too should be called "CcFreq".
>
> Anyway, I don't insist, just wanted to make you aware.
Ok, I agree.
>
>>>> +    ASSERT (0);
>>> (15) Should be ASSERT (FALSE).
>>>
>>> (16) I recommend that a CpuDeadLoop() call be added here too, because
>>> ASSERT()s are compiled out of RELEASE builds.
>> For double (13), (14), (15), (16) and (17), I agree, will fix them in
>> V4. But for 15, did the CpuDeadLoop() be needed? I think if the DEBUG
>> mode doesn't work, the RELEASE mode can not be created.
> You are not asserting an algorithmic invariant, but an environmental
> circumstance (namely that the clock does not show unreasonable
> properties). Meaning that the predicate can be falsified due to
> environmental reasons, even if there is no programming or design error
> in the code. Therefore, in my opinion, such *environment* checks are
> best preserved in RELEASE builds as well.
>
> Either way, up to you.
Ok, I will add the CpuDeadLoop in the V4.
>
>>> (25) This proves that, generally speaking, the final "+=" operation
>>> (increment) is unsafe -- in every TimerLib instance in edk2 that uses
>>> this calculation --, and should be replaced with an addition from
>>> SafeIntLib.
>> Do you recommend changing the final "+=" to the SafeIntLib way?
>> Something like: NanoSeconds = SafeUint64Add (NanoSeconds,
>> DivU64x64Remainder (MultU64x32 (Remainder, 1000000000u), Frequency,
>> NULL), &NanoSeconds).
> Something like that, yes, please.
>
> While at it, using some helper variables could improve readability, plus
> AFAIR, SafeUint64Add() returns a RETURN_STATUS vaule.
I have noticed.
>
> ... sorry about the late followup. I've been out sick for two weeks, and
> have returned now for only a few days again, before the year ends. I
> think it unlikely that I can continue reviewing v3 in any timely manner,
> so feel free to post v4 whenever you're ready to do so (assuming no-one
> else wants to continue reviewing v3).
Please take care.
>
> Thanks
> Laszlo
>
>
>
> 
>


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#112355): https://edk2.groups.io/g/devel/message/112355
Mute This Topic: https://groups.io/mt/102644766/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-



[-- Attachment #2: Type: text/html, Size: 16851 bytes --]