Hi Jan, Jan Beulich <[email protected]> writes:
> On 24.12.2025 18:03, Oleksii Kurochko wrote: >> Signed-off-by: Oleksii Kurochko <[email protected]> >> --- >> xen/arch/riscv/include/asm/time.h | 5 +++++ >> 1 file changed, 5 insertions(+) > > Looks okay and read to go in as is (no dependencies on earlier patches > afaics), > but: > >> --- a/xen/arch/riscv/include/asm/time.h >> +++ b/xen/arch/riscv/include/asm/time.h >> @@ -29,6 +29,11 @@ static inline s_time_t ticks_to_ns(uint64_t ticks) >> return muldiv64(ticks, MILLISECS(1), cpu_khz); >> } >> >> +static inline uint64_t ns_to_ticks(s_time_t ns) >> +{ >> + return muldiv64(ns, cpu_khz, MILLISECS(1)); >> +} > > It's hard to see what's arch-dependent about this or ticks_to_ns(). They're > similar but not identical to Arm's version, and I actually wonder why that > difference exists. Questions to Arm people: > 1) Why are they out-of-line functions there? That's interesting question. According to git blame this is how it was introduced in 2012 and after that no one touched this part. Original patch had cntfrq defined as `static`, this explains why these functions were declared out-of-line. > 2) Why the involvement of the constant 1000 there? 1000 * cpu_khz can > actually overflow in 32 bits. The forms above aren't prone to such an > issue. Patch "xen: move XEN_SYSCTL_physinfo, XEN_SYSCTL_numainfo and XEN_SYSCTL_topologyinfo to common code" (096578b4e48) changed hz to khz. This added that 1000 multiplication. Also this patch removed `static` qualifier from the counter variable. Anyways, latest ARM ARM suggests that timer frequency should be fixed at 1GHz, which is shy of 32-bit overflow. So most new platforms will be fine. And older platforms had much lower frequencies. > If the delta isn't justified, I think we'd better put RISC-V's functions in > common code (xen/time.h). They're not presently needed by x86, but as > inline functions they also shouldn't do any harm. I'm mere reviewer, but I agree that proposed approach is better and more resilient. -- WBR, Volodymyr
