Hi Laszlo, Thanks so carefully to review this patch, I'm guessing it took your a long time. :) Thanks, Chao 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 >> Cc: Ray Ni >> Cc: Rahul Kumar >> Cc: Gerd Hoffmann >> Signed-off-by: Chao Li >> --- >> .../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: > > > > 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. > >> diff --git a/UefiCpuPkg/Library/BaseLoongArch64CpuTimerLib/BaseLoongArch64CpuTimerLib.inf b/UefiCpuPkg/Library/BaseLoongArch64CpuTimerLib/BaseLoongArch64CpuTimerLib.inf >> new file mode 100644 >> index 0000000000..c00c215aec >> --- /dev/null >> +++ b/UefiCpuPkg/Library/BaseLoongArch64CpuTimerLib/BaseLoongArch64CpuTimerLib.inf >> @@ -0,0 +1,30 @@ >> +## @file >> +# Base CPU Timer Library >> +# >> +# Provides base timer support using CPUCFG 0x4 and 0x5 stable counter frequency. >> +# >> +# Copyright (c) 2023, Loongson Technology Corporation Limited. All rights reserved.
>> +# SPDX-License-Identifier: BSD-2-Clause-Patent >> +# >> +## >> + >> +[Defines] >> + INF_VERSION = 0x00010005 > (2) Can you use the most recent INF file version? (Generally applies to > all new INF files in the series.) > > The latest version is 1.29 > , and you > can just write "1.29" here. OK, I will update this version, include all new INF files for this series. > >> + BASE_NAME = BaseLoongArch64CpuTimerLib >> + FILE_GUID = 740389C7-CC44-4A2F-88DC-89D97D312E7C >> + MODULE_TYPE = BASE >> + VERSION_STRING = 1.0 >> + LIBRARY_CLASS = TimerLib >> + MODULE_UNI_FILE = BaseLoongArch64CpuTimerLib.uni >> + >> +[Sources.common] >> + CpuTimerLib.c > (3) ".common" is not an error, but superfluous here. Yes, I will remove it in V4. > >> + >> +[Packages] >> + MdePkg/MdePkg.dec >> + UefiCpuPkg/UefiCpuPkg.dec > (4) Do you actually consume any artifact from "UefiCpuPkg.dec"? > > Unless you do, there's no need to depend on "UefiCpuPkg.dec" here. Yes, the UefiCpuPkg.dec is indeed not necessary, so I will remove it in this file for V4. > > (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. > >> + >> +[LibraryClasses] >> + BaseLib >> + PcdLib >> + DebugLib > (6) If it's not too much of a burden, it's best to keep library classes > alphabetically sorted. OK, I will sort them for V4. > >> diff --git a/UefiCpuPkg/Library/BaseLoongArch64CpuTimerLib/BaseLoongArch64CpuTimerLib.uni b/UefiCpuPkg/Library/BaseLoongArch64CpuTimerLib/BaseLoongArch64CpuTimerLib.uni >> new file mode 100644 >> index 0000000000..72d38ec679 >> --- /dev/null >> +++ b/UefiCpuPkg/Library/BaseLoongArch64CpuTimerLib/BaseLoongArch64CpuTimerLib.uni >> @@ -0,0 +1,15 @@ >> +// /** @file >> +// Base CPU Timer Library >> +// >> +// Provides base timer support using CPUCFG 0x4 and 0x5 stable counter frequency. >> +// >> +// Copyright (c) 2023, Loongson Technology Corporation Limited. All rights reserved.
>> +// >> +// SPDX-License-Identifier: BSD-2-Clause-Patent >> +// >> +// **/ >> + >> + >> +#string STR_MODULE_ABSTRACT #language en-US "LOONGARCH CPU Timer Library" >> + >> +#string STR_MODULE_DESCRIPTION #language en-US "Provides basic timer support using CPUCFG 0x4 and 0x5 stable counter frequency." >> diff --git a/UefiCpuPkg/Library/BaseLoongArch64CpuTimerLib/CpuTimerLib.c b/UefiCpuPkg/Library/BaseLoongArch64CpuTimerLib/CpuTimerLib.c >> new file mode 100644 >> index 0000000000..349b881cbc >> --- /dev/null >> +++ b/UefiCpuPkg/Library/BaseLoongArch64CpuTimerLib/CpuTimerLib.c >> @@ -0,0 +1,226 @@ >> +/** @file >> + CPUCFG 0x4 and 0x5 for Stable Counter frequency instance of Timer Library. >> + >> + Copyright (c) 2023, Loongson Technology Corporation Limited. All rights reserved.
>> + >> + SPDX-License-Identifier: BSD-2-Clause-Patent >> +**/ >> + >> +#include >> +#include >> +#include >> +#include >> +#include > (7) It's best to keep the Library includes in sync with the > [LibraryClasses] section from the INF file. > > (And to keep the #include directives alphabetically sorted as well.) > > You are including here because the file is going to > define those functions. OK. This is the sole justified difference > between the Library #includes here and the [LibraryClasses] section in > the INF file. > > Regarding dependencies, you are including and > , but [LibraryClasses] in addition lists PcdLib. > Thus, is missing here. (You are likely getting it > included recursively, but that's not clean enough.) OK, I see, I will sort them and keep them in sync with corresponding libraries. > >> + >> +/** >> + Calculate clock frequency using CPUCFG 0x4 and 0x5 registers. >> + >> + @param VOID. > (8) This @param is useless and should be avoided, AFAICT. I think the > EccCheck plugin will not complain. The EccCheck already passed... Any way, I will remove it in V4. > >> + >> + @return The frequency in Hz. >> + >> +**/ >> +UINT32 >> +EFIAPI >> +CalcConstFreq ( > (9) Should be STATIC, and should *not* be EFIAPI. It is not a public > library or protocol interface. I will fix it in V4. > >> + 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. > >> + 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. > >> + ClockMultiplier = CpucfgReg5Data.Bits.CC_MUL & 0xFFFFULL; >> + ClockDivide = CpucfgReg5Data.Bits.CC_DIV & 0xFFFFULL; > (12) Seeing how the Uint32 members of the unions (?) are used for > populating the bitfields, the ULL suffix on the masks seems superfluous. > The results are stored to UINT32 local variables, too. 0xFFFF should > suffice. Yes, the ULL does seem redundant, the UINT32 means unsigned INT32 and 0xFFFF shoud be unsigned, I thought when I wrote this code, ULL was added to ensure that the value was unsigned. > >> + >> + if (!BaseFreq || !ClockMultiplier || !ClockDivide) { > (13) The edk2 coding style does not like scalar types *other* than > BOOLEAN to be used in logical context. Please rewrite as: > > (BaseFreq == 0) || (ClockMultiplier == 0) || (ClockDivide == 0) > >> + DEBUG ((DEBUG_ERROR, "LoongArch Stable Timer is not available in the CPU, hence this library cannot be used.\n")); > (13) I think this line is a bit too long (117 chars). I think the > currently accepted limit (?) is 100 characters (but I'm unsure). > > Recommend to break this up to multiple lines. > >> + StableTimerFreq = 0; > (14) Needless assignment AFAICT. > >> + 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. > >> + } else { > (17) an "else" branch seems awkward here; code after ASSERT (FALSE) / > CpuDeadLoop() is supposed to be unreachable, so whatever you do here can > just be unnested. 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. > >> + StableTimerFreq = (BaseFreq * ClockMultiplier / ClockDivide); > (18) Unsafe multiplication. > > ClockMultiplier is at most 0xFFFF, but BaseFreq (from CC_FREQ) may > theoretically be as high as MAX_UINT32. > > - If there is a LoongArch64 specification that places a 0xFFFF limit on > CC_FREQ, then please add such an ASSERT() here. > > - If there is no such specification, then cast either BaseFreq or > ClockMultiplier to UINT64. A UINT64 multiplication will not overflow > here (because MAX_UINT32 * 0xFFFF fits in 32+16=48 bits). Yes, you are right, there is a risk of overflow, I think changing the ClockMultiplier to UINT64 would do the trick, or force transferring the result to ensure it doesn't overflow? Something like: ((UINT64)(BaseFreq * ClockMultiplier) / ClockDivide). > > > (19) In turn, the result of the division needs to be checked against two > boundaries: > > - it must not be zero (could occur if the divisor is too large, relative > to the product). Returning zero from this function would case problems > elsewhere. I will ASSERT here if the result is zero. > > - it must fit in a UINT32. Better yet, change the return type of the > function to UINT64. Agree. > > >> + } >> + >> + return StableTimerFreq; >> +} >> + >> +/** >> + Stalls the CPU for at least the given number of microseconds. >> + >> + Stalls the CPU for the number of microseconds specified by MicroSeconds. >> + >> + @param MicroSeconds The minimum number of microseconds to delay. >> + >> + @return MicroSeconds >> + >> +**/ >> +UINTN >> +EFIAPI >> +MicroSecondDelay ( >> + IN UINTN MicroSeconds >> + ) >> +{ >> + UINTN Count, Ticks, Start, End; >> + >> + Count = (CalcConstFreq () * MicroSeconds) / 1000000; > (20) Unchecked multiplication. CalcConstFreq() may be as large as > > (MAX_UINT32 * 0xFFFF / 1) > > and then dependent on MicroSeconds we could overflow even a UINT64 here. > > This function does not permit returning errors, so I'm not fully sure > what to do. Normally I'd recommend a safe UINT64 multiplication from > SafeIntLib (and then storing the quotient, from the division, in a UINT64). OK, I will use the SafeIntLib here. > >> + Start = AsmReadStableCounter (); >> + End = Start + Count; >> + >> + do { >> + Ticks = AsmReadStableCounter (); >> + } while (Ticks < End); > The rest of this patch indicates that > > (a) the performance counter is just the stable counter > (GetPerformanceCounter() simply calls AsmReadStableCounter()), and that > > (b) the range is [BIT2, BIT48-1] (both sides inclusive). Sorry, The PerformanceCounter function series are updated in our internal, I will update them in V4. I would say, the performance counter is not same to stable counter, and the stable count in LoongArch64 are 64 bits wide, so no warparound occurs. PLS wait for V4. > > (21) The problem is that this simple loop does not consider wrap-around. > > We should do something like this: > > RETURN_STATUS Status; > UINT64 Remaining; > UINT64 Start; > > Status = SafeUint64Mult (CalcConstFreq (), MicroSeconds, &Remaining); > ASSERT_RETURN_ERROR (Status); > if (RETURN_ERROR (Status)) { > CpuDeadLoop (); > } > > // > // for the given number of microseconds, the needed tick count should > // be rounded upwards > // > Status = SafeUint64Add (Remaining, 1000000 - 1, &Remaining); > ASSERT_RETURN_ERROR (Status); > if (RETURN_ERROR (Status)) { > CpuDeadLoop (); > } > > Remaining = DivU64x32 (Remaining, 1000000); > Start = AsmReadStableCounter (); > > while (Remaining > 0) { > UINT64 Stop; > UINT64 Diff; > > Stop = AsmReadStableCounter (); > > if (Start <= Stop) { > // > // no wrap-around > // > Diff = Stop - Start; > } else { > // > // wrap-around > // > Diff = (BIT48 - Start) + (Stop - BIT2); > } > > Start = Stop; > > Remaining -= MIN (Remaining, Diff); > } Refer to above. > >> + >> + return MicroSeconds; >> +} >> + >> +/** >> + Stalls the CPU for at least the given number of nanoseconds. >> + >> + Stalls the CPU for the number of nanoseconds specified by NanoSeconds. >> + >> + @param NanoSeconds The minimum number of nanoseconds to delay. >> + >> + @return NanoSeconds >> + >> +**/ >> +UINTN >> +EFIAPI >> +NanoSecondDelay ( >> + IN UINTN NanoSeconds >> + ) >> +{ >> + UINT32 MicroSeconds; >> + >> + if (NanoSeconds % 1000 == 0) { >> + MicroSeconds = NanoSeconds/1000; >> + } else { >> + MicroSeconds = NanoSeconds/1000 + 1; >> + } > (22) MicroSeconds should have type UINTN. Agree. > > (23) Using DivU64x32Remainder() would be more idiomatic. > > ( > > I agree that, if we find that the remainder is nonzero, adding 1 to the > quotient is safe, for the original UINTN range too. > > Initial condition: > > ns <= MAX_UINTN > > Divide by 1000 (mathematically): > > ns/1000 <= MAX_UINTN/1000 > > Because floor(x) <= x, we can expand the left hand side: > > floor(ns/1000) <= ns/1000 <= MAX_UINTN/1000 > > Drop the middle: > > floor(ns/1000) <= MAX_UINTN/1000 > > Add 1: > > floor(ns/1000) + 1 <= MAX_UINTN/1000 + 1 > > At the left hand side, we now have the C language result (the truncated, > then incremented, quotinent). > > Now, a small detour: > > 1/1000 < 1 > > Multiply by x (assuming x>0): > > x / 1000 < x > > Substitute MAX_UINTN for x (MAX_UINTN is positive): > > MAX_UINTN/1000 < MAX_UINTN > > Then use this to expand the right hand side: > > floor(ns/1000) + 1 <= MAX_UINTN/1000 + 1 < MAX_UINTN + 1 > > Drop the middle: > > floor(ns/1000) + 1 < MAX_UINTN + 1 > > Given that both sides are integers, we get > > floor(ns/1000) + 1 <= MAX_UINTN > > which is what we needed. > > ) OK, I see. In V4, I will use the DivU64X32Remainder() to change this operation. > >> + >> + MicroSecondDelay (MicroSeconds); >> + >> + return NanoSeconds; >> +} >> + >> +/** >> + Retrieves the current value of a 64-bit free running performance counter. >> + >> + Retrieves the current value of a 64-bit free running performance counter. The >> + counter can either count up by 1 or count down by 1. If the physical >> + performance counter counts by a larger increment, then the counter values >> + must be translated. The properties of the counter can be retrieved from >> + GetPerformanceCounterProperties(). >> + >> + @return The current value of the free running performance counter. >> + >> +**/ >> +UINT64 >> +EFIAPI >> +GetPerformanceCounter ( >> + VOID >> + ) >> +{ >> + return AsmReadStableCounter (); >> +} >> + >> +/** >> + Retrieves the 64-bit frequency in Hz and the range of performance counter >> + values. >> + >> + If StartValue is not NULL, then the value that the performance counter starts >> + with immediately after is it rolls over is returned in StartValue. If >> + EndValue is not NULL, then the value that the performance counter end with >> + immediately before it rolls over is returned in EndValue. The 64-bit >> + frequency of the performance counter in Hz is always returned. If StartValue >> + is less than EndValue, then the performance counter counts up. If StartValue >> + is greater than EndValue, then the performance counter counts down. For >> + example, a 64-bit free running counter that counts up would have a StartValue >> + of 0 and an EndValue of 0xFFFFFFFFFFFFFFFF. A 24-bit free running counter >> + that counts down would have a StartValue of 0xFFFFFF and an EndValue of 0. >> + >> + @param StartValue The value the performance counter starts with when it >> + rolls over. >> + @param EndValue The value that the performance counter ends with before >> + it rolls over. >> + >> + @return The frequency in Hz. >> + >> +**/ >> +UINT64 >> +EFIAPI >> +GetPerformanceCounterProperties ( >> + OUT UINT64 *StartValue OPTIONAL, >> + OUT UINT64 *EndValue OPTIONAL >> + ) >> +{ >> + if (StartValue != NULL) { >> + *StartValue = BIT2; >> + } >> + >> + if (EndValue != NULL) { >> + *EndValue = BIT48 - 1; >> + } >> + >> + return CalcConstFreq (); >> +} >> + >> +/** >> + Converts elapsed ticks of performance counter to time in nanoseconds. >> + >> + This function converts the elapsed ticks of running performance counter to >> + time value in unit of nanoseconds. >> + >> + @param Ticks The number of elapsed ticks of running performance counter. >> + >> + @return The elapsed time in nanoseconds. >> + >> +**/ >> +UINT64 >> +EFIAPI >> +GetTimeInNanoSecond ( >> + IN UINT64 Ticks >> + ) >> +{ >> + UINT64 Frequency; >> + UINT64 NanoSeconds; >> + UINT64 Remainder; >> + INTN Shift; >> + >> + Frequency = GetPerformanceCounterProperties (NULL, NULL); >> + >> + // >> + // Ticks >> + // Time = --------- x 1,000,000,000 >> + // Frequency >> + // >> + NanoSeconds = MultU64x32 (DivU64x64Remainder (Ticks, Frequency, &Remainder), 1000000000u); > (24) I understand this is being copied from elsewhere, but the > multiplication by 10^9 should be checked against overflow, using > SafeIntLib. If we have e.g. a 100 MHz clock, then passing in > Ticks=MAX_UINT64 will lead to an overflow. :( OK, in V4, I will use the SafeIntLib. > >> + >> + // >> + // Ensure (Remainder * 1,000,000,000) will not overflow 64-bit. >> + // Since 2^29 < 1,000,000,000 = 0x3B9ACA00 < 2^30, Remainder should < 2^(64-30) = 2^34, >> + // i.e. highest bit set in Remainder should <= 33. >> + // > What we want here is (Remainder * 10^9 / Frequency), and in that we want > the product not to overflow 2^64 - 1. Thus, we need > > Remainder * 10^9 <= 2^64 - 1 > > Remainder <= (2^64 - 1) / 10^9 > > The floor -- i.e., integral part -- of the RHS is 18,446,744,073 > decimal, or 100_01001011_10000010_11111010_00001001 binary. The most > significant bit that is set is bit 34. But that's not a sufficient > condition, if we only consider the MSB in Remainder, because other bits > that should be clear (such as bit 33) could still be set. The sufficient > condition is thus that the MSB be <= 33. Therefore the comment is OK. > >> + Shift = MAX (0, HighBitSet64 (Remainder) - 33); > Quirky, but correct. > > If Remainder is 0, then HighBitSet64 returns -1, and we compare -34 vs. > 0 -- we get zero. OK, no shift needed. > > Otherwise, if HighBitSet64 returns a value in the range [0..33], then we > compare [-33..0] vs 0 -- we get zero from MAX. OK, no shift needed. > > Otherwise, HighBitSet64 returns a value from the range [34..63], then we > compare [1..30] against zero, and the former prevails (gets stored to > Shift). OK. > >> + Remainder = RShiftU64 (Remainder, (UINTN)Shift); >> + Frequency = RShiftU64 (Frequency, (UINTN)Shift); > I *think* this approach will ensure that Frequency is not zero. > Remainder is strictly smaller than Frequency, and we shift out only so > many LSBs from Remainder (and Frequency) that (Remainder * 10^9) below > *just* not overflow. That means Remainder should not end up being zeroed > out, and so Frequency shouldn't either (the MSB of the original > Frequency is larger than or equal to the MSB of the original Remainder). OK. > >> + NanoSeconds += DivU64x64Remainder (MultU64x32 (Remainder, 1000000000u), Frequency, NULL); > So the RHS here may give us a proper NanoSeconds increment, but how do > we know that the addition will not overflow NanoSeconds? > > Here's an example: > > Ticks = 18,446,744,055,553,255,925 (0xFFFF_FFFB_C5CC_E9F5) > Frequency = 999,999,999 (0x3B9A_C9FF) > > ( > > The idea with this example is that dividing Ticks by Frequency and then > multiplying the result with 1,000,000,000 causes a *very slight* blow-up: > > Ticks / 999,999,999 * 1,000,000,000 = > Ticks * (1,000,000,000 / 999,999,999) > > in such a tricky way that this blow-up: > > (a) *just* doesn't overflow the first part of the function (where we use > the integral multiples of Frequency), but > > (b) does overflow the last part (where we use the remaining, fractional, > multiple of Frequency). > > The way to achieve that, after setting Frequency to just below 1GHz for > the above blow-up, is to start with Ticks = MAX_UINT64, perform the > division, then maximize Remainder in comprison to the divisor (i.e., > Frequency) -- increase Remainder to (Frequency-1) --, then recalculate > Ticks from that *backwards* (increase Ticks the same as we increased > Remainder). > > ) > > And then this happens: > > - the initial DivU64x64Remainder (Ticks, Frequency, &Remainder) call > returns 18,446,744,073 (0x4_4B82_FA09) as quotient, and sets Remainder > to 999,999,998 (0x3B9A_C9FE, binary 111011_10011010_11001001_11111110). > > - the first MultU64x32() call produces 18,446,744,073,000,000,000 in > NanoSeconds (0xFFFF_FFFF_D5B5_1A00). > > - Remainder's MSB is bit 29, thus Shift gets assigned 0. > > - Multiplying Remainder by 1,000,000,000 produces > 999,999,998,000,000,000 (0xDE0_B6B3_302E_6C00). No overflow, as intended. > > - Dividing that product by Frequency produces 999,999,998 (0x3B9A_C9FE) > as the *increment* for NanoSeconds. > > - Adding this increment (0x3B9A_C9FE) to NanoSeconds from earlier > (0xFFFF_FFFF_D5B5_1A00) *does* overflow: the result would be > 0x1_0000_0000_114F_E3FE! > > (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). > > I'm not asking for a tree-wide fix, but using SafeIntLib here would be nice. > > >> + >> + return NanoSeconds; >> +} >> diff --git a/UefiCpuPkg/UefiCpuPkg.dsc b/UefiCpuPkg/UefiCpuPkg.dsc >> index 074fd77461..8e34a9cd6b 100644 >> --- a/UefiCpuPkg/UefiCpuPkg.dsc >> +++ b/UefiCpuPkg/UefiCpuPkg.dsc >> @@ -205,5 +205,8 @@ >> UefiCpuPkg/CpuTimerDxeRiscV64/CpuTimerDxeRiscV64.inf >> UefiCpuPkg/CpuDxeRiscV64/CpuDxeRiscV64.inf >> >> +[Components.LOONGARCH64] >> + UefiCpuPkg/Library/BaseLoongArch64CpuTimerLib/BaseLoongArch64CpuTimerLib.inf >> + >> [BuildOptions] >> *_*_*_CC_FLAGS = -D DISABLE_NEW_DEPRECATED_INTERFACES > (26) And then this should go into MdePkg.dec, according to points (4) > and (5) above. Same to above. > > Thanks > Laszlo > > > > > -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#111667): https://edk2.groups.io/g/devel/message/111667 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] -=-=-=-=-=-=-=-=-=-=-=-