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.kuroc...@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
