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

[Oleksii Kurochko <oleksii.kurochko@xxxxxxxxx>]

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

>