Re: [PATCH v1 01/14] xen/riscv: implement get_s_time()

[Oleksii Kurochko <oleksii.kurochko@xxxxxxxxx>]

On 4/10/25 2:52 PM, Jan Beulich wrote:

> On 08.04.2025 17:57, Oleksii Kurochko wrote:
> > @@ -23,6 +24,11 @@ static inline cycles_t get_cycles(void)
> >      return csr_read(CSR_TIME);
> >  }
> >  
> > +static inline s_time_t ticks_to_ns(uint64_t ticks)
> > +{
> > +    return muldiv64(ticks, SECONDS(1), 1000 * cpu_khz);
> > +}
> >       
> Why the extra multiplication by 1000? I.e. why not
> "muldiv64(ticks, MILLISECONDS(1), cpu_khz)", getting away with just one
> multiplication and a reduced risk of encountering intermediate overflow
> (affecting only hypothetical above 4THz CPUs then)?
>     
Multiplication by 1000 was needed to convert khz to hz, but yes, your option
would be better.

> > --- a/xen/arch/riscv/time.c
> > +++ b/xen/arch/riscv/time.c
> > @@ -4,10 +4,17 @@
> >  #include <xen/init.h>
> >  #include <xen/lib.h>
> >  #include <xen/sections.h>
> > +#include <xen/types.h>
> >  
> >  unsigned long __ro_after_init cpu_khz; /* CPU clock frequency in kHz. */
> >  uint64_t __ro_after_init boot_clock_cycles;
> >  
> > +s_time_t get_s_time(void)
> > +{
> > +    uint64_t ticks = get_cycles() - boot_clock_cycles;
> > +    return ticks_to_ns(ticks);
> >       
> Nit: Blank line between declaration(s) and statement(s) please, as well as
> ahead of the main "return" of a function.
>
> Happy to make both adjustments upon committing, so long as you agree; then:
> Reviewed-by: Jan Beulich <jbeulich@xxxxxxxx>
>     
I'll be happy with that.

Thank you very much.

~ Oleksii