| Message ID | 1a04711f46a1c0a7cdf709abc37f143747215495.1742918184.git.oleksii.kurochko@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [v2,1/4] xen/riscv: introduce preinit_xen_time() | expand |
On 25.03.2025 18:36, Oleksii Kurochko wrote: > preinit_xen_time() does two things: > 1. Parse timebase-frequency properpy of /cpus node to initialize > cpu_khz variable. > 2. Initialize xen_start_clock_cycles with the current time counter > value. > > Signed-off-by: Oleksii Kurochko <oleksii.kurochko@gmail.com> > --- > Changes in v2: > - Update SPDX tag for time.c > - s/read_mostly/__ro_after_init for boot_count variable. > - Add declaration of boot_count to asm/time.h. > - Rename boot_count to xen_start_clock_cycles. I'm not going to insist on another name change, but I'm having a hard time seeing why almost any global variable in Xen would need to have a xen_ prefix. "start" also can be ambiguous, so imo boot_clock_cycles would have been best. > --- /dev/null > +++ b/xen/arch/riscv/time.c > @@ -0,0 +1,39 @@ > +/* SPDX-License-Identifier: GPL-2.0-only */ > +#include <xen/device_tree.h> > +#include <xen/init.h> > +#include <xen/lib.h> > +#include <xen/sections.h> > + > +unsigned long __ro_after_init cpu_khz; /* CPU clock frequency in kHz. */ > +unsigned long __ro_after_init xen_start_clock_cycles; For the theoretical at this point case of support RV32, will unsigned long be wide enough? I.e. is the counter only 32 bits wide when XLEN=32? > +static __initdata struct dt_device_node *timer; Please can __initdata (and alike) live at its canonical place, between type and identifier? It's also unclear at this point why this needs to be a file scope variable. > +/* Set up the timer on the boot CPU (early init function) */ > +static void __init preinit_dt_xen_time(void) > +{ > + static const struct dt_device_match __initconstrel timer_ids[] = > + { > + DT_MATCH_PATH("/cpus"), > + { /* sentinel */ }, > + }; > + uint32_t rate; > + > + timer = dt_find_matching_node(NULL, timer_ids); > + if ( !timer ) > + panic("Unable to find a compatible timer in the device tree\n"); > + > + dt_device_set_used_by(timer, DOMID_XEN); > + > + if ( !dt_property_read_u32(timer, "timebase-frequency", &rate) ) > + panic("Unable to find clock frequency\n"); > + > + cpu_khz = rate / 1000; "rate" being only 32 bits wide, what about clock rates above 4GHz? Or is this some external clock running at a much lower rate than the CPU would? > +} > + > +void __init preinit_xen_time(void) > +{ > + preinit_dt_xen_time(); > + > + xen_start_clock_cycles = get_cycles(); > +} I take it that more stuff is going to be added to this function? Else I wouldn't see why preinit_dt_xen_time() needed to be a separate function. Jan
On 3/26/25 4:13 PM, Jan Beulich wrote: > On 25.03.2025 18:36, Oleksii Kurochko wrote: >> preinit_xen_time() does two things: >> 1. Parse timebase-frequency properpy of /cpus node to initialize >> cpu_khz variable. >> 2. Initialize xen_start_clock_cycles with the current time counter >> value. >> >> Signed-off-by: Oleksii Kurochko<oleksii.kurochko@gmail.com> >> --- >> Changes in v2: >> - Update SPDX tag for time.c >> - s/read_mostly/__ro_after_init for boot_count variable. >> - Add declaration of boot_count to asm/time.h. >> - Rename boot_count to xen_start_clock_cycles. > I'm not going to insist on another name change, but I'm having a hard time > seeing why almost any global variable in Xen would need to have a xen_ > prefix. "start" also can be ambiguous, so imo boot_clock_cycles would have > been best. I can change that during the work on the version of this patch. > >> --- /dev/null >> +++ b/xen/arch/riscv/time.c >> @@ -0,0 +1,39 @@ >> +/* SPDX-License-Identifier: GPL-2.0-only */ >> +#include <xen/device_tree.h> >> +#include <xen/init.h> >> +#include <xen/lib.h> >> +#include <xen/sections.h> >> + >> +unsigned long __ro_after_init cpu_khz; /* CPU clock frequency in kHz. */ >> +unsigned long __ro_after_init xen_start_clock_cycles; > For the theoretical at this point case of support RV32, will unsigned long > be wide enough? I.e. is the counter only 32 bits wide when XLEN=32? No, it will be really an issue and the type should be uint64_t for this variable because on RV32I the timeh CSR is a read-only shadow of the upper 32 bits of the memory-mapped mtime register, while time shadows only the lower 32 bits of mtime. So on RV32 it is still 64-bit long and requires two reads to get cycle value. > >> +static __initdata struct dt_device_node *timer; > Please can __initdata (and alike) live at its canonical place, between > type and identifier? > > It's also unclear at this point why this needs to be a file scope variable. Because this variable is used and will be used only in preinit_dt_xen_time(). But I think we could move the declaration of this variable to preinit_dt_xen_time() as it is used only during preinit_dt_xen_time(). > >> +/* Set up the timer on the boot CPU (early init function) */ >> +static void __init preinit_dt_xen_time(void) >> +{ >> + static const struct dt_device_match __initconstrel timer_ids[] = >> + { >> + DT_MATCH_PATH("/cpus"), >> + { /* sentinel */ }, >> + }; >> + uint32_t rate; >> + >> + timer = dt_find_matching_node(NULL, timer_ids); >> + if ( !timer ) >> + panic("Unable to find a compatible timer in the device tree\n"); >> + >> + dt_device_set_used_by(timer, DOMID_XEN); >> + >> + if ( !dt_property_read_u32(timer, "timebase-frequency", &rate) ) >> + panic("Unable to find clock frequency\n"); >> + >> + cpu_khz = rate / 1000; > "rate" being only 32 bits wide, what about clock rates above 4GHz? Or is > this some external clock running at a much lower rate than the CPU would? It is the frequency of the hardware timer that drives the |mtime|register, it defines the frequency (in Hz) at which the timer increments. > >> +} >> + >> +void __init preinit_xen_time(void) >> +{ >> + preinit_dt_xen_time(); >> + >> + xen_start_clock_cycles = get_cycles(); >> +} > I take it that more stuff is going to be added to this function? Else I > wouldn't see why preinit_dt_xen_time() needed to be a separate function. Actually, I checked my latest downstream branch and I haven't added nothing extra to this function. Only one thing that I want to add is ACPI case: if ( acpi_disabled ) preinit_dt_xen_time(); else panic("TBD\n"); /* preinit_acpi_xen_time(); */ ~ Oleksii
On 26.03.2025 20:49, Oleksii Kurochko wrote: > On 3/26/25 4:13 PM, Jan Beulich wrote: >> On 25.03.2025 18:36, Oleksii Kurochko wrote: >>> +/* Set up the timer on the boot CPU (early init function) */ >>> +static void __init preinit_dt_xen_time(void) >>> +{ >>> + static const struct dt_device_match __initconstrel timer_ids[] = >>> + { >>> + DT_MATCH_PATH("/cpus"), >>> + { /* sentinel */ }, >>> + }; >>> + uint32_t rate; >>> + >>> + timer = dt_find_matching_node(NULL, timer_ids); >>> + if ( !timer ) >>> + panic("Unable to find a compatible timer in the device tree\n"); >>> + >>> + dt_device_set_used_by(timer, DOMID_XEN); >>> + >>> + if ( !dt_property_read_u32(timer, "timebase-frequency", &rate) ) >>> + panic("Unable to find clock frequency\n"); >>> + >>> + cpu_khz = rate / 1000; >> "rate" being only 32 bits wide, what about clock rates above 4GHz? Or is >> this some external clock running at a much lower rate than the CPU would? > > It is the frequency of the hardware timer that drives the > |mtime|register, it defines the frequency (in Hz) at which the timer > increments. And that timer can't plausibly run at more than 4 GHz? >>> +void __init preinit_xen_time(void) >>> +{ >>> + preinit_dt_xen_time(); >>> + >>> + xen_start_clock_cycles = get_cycles(); >>> +} >> I take it that more stuff is going to be added to this function? Else I >> wouldn't see why preinit_dt_xen_time() needed to be a separate function. > > Actually, I checked my latest downstream branch and I haven't added nothing > extra to this function. > Only one thing that I want to add is ACPI case: > if ( acpi_disabled ) > preinit_dt_xen_time(); > else > panic("TBD\n"); /* preinit_acpi_xen_time(); */ That's probably a good enough reason then to keep the function separate. Jan
On 3/27/25 8:42 AM, Jan Beulich wrote: > On 26.03.2025 20:49, Oleksii Kurochko wrote: >> On 3/26/25 4:13 PM, Jan Beulich wrote: >>> On 25.03.2025 18:36, Oleksii Kurochko wrote: >>>> +/* Set up the timer on the boot CPU (early init function) */ >>>> +static void __init preinit_dt_xen_time(void) >>>> +{ >>>> + static const struct dt_device_match __initconstrel timer_ids[] = >>>> + { >>>> + DT_MATCH_PATH("/cpus"), >>>> + { /* sentinel */ }, >>>> + }; >>>> + uint32_t rate; >>>> + >>>> + timer = dt_find_matching_node(NULL, timer_ids); >>>> + if ( !timer ) >>>> + panic("Unable to find a compatible timer in the device tree\n"); >>>> + >>>> + dt_device_set_used_by(timer, DOMID_XEN); >>>> + >>>> + if ( !dt_property_read_u32(timer, "timebase-frequency", &rate) ) >>>> + panic("Unable to find clock frequency\n"); >>>> + >>>> + cpu_khz = rate / 1000; >>> "rate" being only 32 bits wide, what about clock rates above 4GHz? Or is >>> this some external clock running at a much lower rate than the CPU would? >> It is the frequency of the hardware timer that drives the >> |mtime|register, it defines the frequency (in Hz) at which the timer >> increments. > And that timer can't plausibly run at more than 4 GHz? I haven't seen yet such big timer frequency. But if to look at device tree spec: timebase-frequency: Specifies the current frequency at which the timebase and decrementer registers are updated (in Hertz). The value is a <prop-encoded-array> in one of two forms: • A 32-bit integer consisting of one <u32> specifying the frequency. • A 64-bit integer represented as a <u64>. Interesting that Linux kernel reads timebase-frequency as u32: u32 prop; ... if (!cpu || of_property_read_u32(cpu, "timebase-frequency", &prop)) and then saves it to: riscv_timebase = prop; where riscv_timebase is declared as unsigned long. I think it can be left as it is now as if timebase-frequency will be u64 then it means that it will be needed to read two u32 values and in this case dt_property_read_u32() will return 0 and the panic will occur. ~ Oleksii
On 27.03.2025 12:48, Oleksii Kurochko wrote: > I think it can be left as it is now as if timebase-frequency will be u64 then > it means that it will be needed to read two u32 values and in this case dt_property_read_u32() > will return 0 and the panic will occur. Fair enough; please say a word on this aspect in the description, though. Jan
diff --git a/xen/arch/riscv/Makefile b/xen/arch/riscv/Makefile index b0c8270a99..82016a957a 100644 --- a/xen/arch/riscv/Makefile +++ b/xen/arch/riscv/Makefile @@ -9,6 +9,7 @@ obj-y += setup.o obj-y += shutdown.o obj-y += smp.o obj-y += stubs.o +obj-y += time.o obj-y += traps.o obj-y += vm_event.o diff --git a/xen/arch/riscv/include/asm/time.h b/xen/arch/riscv/include/asm/time.h index fc1572e9b4..c24745508b 100644 --- a/xen/arch/riscv/include/asm/time.h +++ b/xen/arch/riscv/include/asm/time.h @@ -5,6 +5,9 @@ #include <xen/bug.h> #include <asm/csr.h> +/* Clock cycles count at Xen startup */ +extern unsigned long xen_start_clock_cycles; + struct vcpu; static inline void force_update_vcpu_system_time(struct vcpu *v) @@ -19,6 +22,8 @@ static inline cycles_t get_cycles(void) return csr_read(CSR_TIME); } +void preinit_xen_time(void); + #endif /* ASM__RISCV__TIME_H */ /* diff --git a/xen/arch/riscv/setup.c b/xen/arch/riscv/setup.c index b0e587678e..836ad16fed 100644 --- a/xen/arch/riscv/setup.c +++ b/xen/arch/riscv/setup.c @@ -126,6 +126,8 @@ void __init noreturn start_xen(unsigned long bootcpu_id, riscv_fill_hwcap(); + preinit_xen_time(); + printk("All set up\n"); machine_halt(); diff --git a/xen/arch/riscv/stubs.c b/xen/arch/riscv/stubs.c index 5951b0ce91..caa133de84 100644 --- a/xen/arch/riscv/stubs.c +++ b/xen/arch/riscv/stubs.c @@ -27,8 +27,6 @@ nodemask_t __read_mostly node_online_map = { { [0] = 1UL } }; /* time.c */ -unsigned long __ro_after_init cpu_khz; /* CPU clock frequency in kHz. */ - s_time_t get_s_time(void) { BUG_ON("unimplemented"); diff --git a/xen/arch/riscv/time.c b/xen/arch/riscv/time.c new file mode 100644 index 0000000000..ae8efa3c59 --- /dev/null +++ b/xen/arch/riscv/time.c @@ -0,0 +1,39 @@ +/* SPDX-License-Identifier: GPL-2.0-only */ +#include <xen/device_tree.h> +#include <xen/init.h> +#include <xen/lib.h> +#include <xen/sections.h> + +unsigned long __ro_after_init cpu_khz; /* CPU clock frequency in kHz. */ +unsigned long __ro_after_init xen_start_clock_cycles; + +static __initdata struct dt_device_node *timer; + +/* Set up the timer on the boot CPU (early init function) */ +static void __init preinit_dt_xen_time(void) +{ + static const struct dt_device_match __initconstrel timer_ids[] = + { + DT_MATCH_PATH("/cpus"), + { /* sentinel */ }, + }; + uint32_t rate; + + timer = dt_find_matching_node(NULL, timer_ids); + if ( !timer ) + panic("Unable to find a compatible timer in the device tree\n"); + + dt_device_set_used_by(timer, DOMID_XEN); + + if ( !dt_property_read_u32(timer, "timebase-frequency", &rate) ) + panic("Unable to find clock frequency\n"); + + cpu_khz = rate / 1000; +} + +void __init preinit_xen_time(void) +{ + preinit_dt_xen_time(); + + xen_start_clock_cycles = get_cycles(); +}
preinit_xen_time() does two things: 1. Parse timebase-frequency properpy of /cpus node to initialize cpu_khz variable. 2. Initialize xen_start_clock_cycles with the current time counter value. Signed-off-by: Oleksii Kurochko <oleksii.kurochko@gmail.com> --- Changes in v2: - Update SPDX tag for time.c - s/read_mostly/__ro_after_init for boot_count variable. - Add declaration of boot_count to asm/time.h. - Rename boot_count to xen_start_clock_cycles. - Stray double blank in the defintion of timer_ids. - s/u32/uint32_t. - Drop full stop. - Update the commit message. - s/__initconst/__initconstrel for timer_ids[]. --- xen/arch/riscv/Makefile | 1 + xen/arch/riscv/include/asm/time.h | 5 ++++ xen/arch/riscv/setup.c | 2 ++ xen/arch/riscv/stubs.c | 2 -- xen/arch/riscv/time.c | 39 +++++++++++++++++++++++++++++++ 5 files changed, 47 insertions(+), 2 deletions(-) create mode 100644 xen/arch/riscv/time.c