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

[Oleksii Kurochko <oleksii.kurochko@xxxxxxxxx>]

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