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

[Oleksii Kurochko <oleksii.kurochko@gmail.com>]

[-- Attachment #1: Type: text/plain, Size: 2132 bytes --]


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

[-- Attachment #2: Type: text/html, Size: 3042 bytes --]