| Message ID | e503aee3ef743210a8188a7da9e70a169dec1287.1744126720.git.oleksii.kurochko@gmail.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | riscv: introduce basic UART support and interrupts for hypervisor mode | expand |
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)? > --- 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@suse.com> Jan
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@suse.com> I'll be happy with that. Thank you very much. ~ Oleksii
diff --git a/xen/arch/riscv/include/asm/time.h b/xen/arch/riscv/include/asm/time.h index e8d9ffec57..0f6aa99ab1 100644 --- a/xen/arch/riscv/include/asm/time.h +++ b/xen/arch/riscv/include/asm/time.h @@ -3,6 +3,7 @@ #define ASM__RISCV__TIME_H #include <xen/bug.h> +#include <xen/lib.h> #include <xen/types.h> #include <asm/csr.h> @@ -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); +} + void preinit_xen_time(void); #endif /* ASM__RISCV__TIME_H */ diff --git a/xen/arch/riscv/stubs.c b/xen/arch/riscv/stubs.c index a1d64534cd..83416d3350 100644 --- a/xen/arch/riscv/stubs.c +++ b/xen/arch/riscv/stubs.c @@ -27,11 +27,6 @@ nodemask_t __read_mostly node_online_map = { { [0] = 1UL } }; /* time.c */ -s_time_t get_s_time(void) -{ - BUG_ON("unimplemented"); -} - int reprogram_timer(s_time_t timeout) { BUG_ON("unimplemented"); diff --git a/xen/arch/riscv/time.c b/xen/arch/riscv/time.c index 905bb13eb4..81e06781f8 100644 --- 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); +} + /* Set up the timer on the boot CPU (early init function) */ static void __init preinit_dt_xen_time(void) {
Also tick_to_ns() is implemeted as it is used in get_s_time(). Signed-off-by: Oleksii Kurochko <oleksii.kurochko@gmail.com> --- xen/arch/riscv/include/asm/time.h | 6 ++++++ xen/arch/riscv/stubs.c | 5 ----- xen/arch/riscv/time.c | 7 +++++++ 3 files changed, 13 insertions(+), 5 deletions(-)