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

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

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


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

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