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