Re: [PATCH v2 1/4] xen/riscv: introduce preinit_xen_time()

[Jan Beulich <jbeulich@xxxxxxxx>]

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@xxxxxxxxx>
> ---
> 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