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

[Oleksii Kurochko <oleksii.kurochko@xxxxxxxxx>]

On 3/20/25 8:36 AM, Jan Beulich wrote:

> On 19.03.2025 18:29, Oleksii Kurochko wrote:
> > On 3/17/25 4:24 PM, Jan Beulich wrote:
> > > On 11.03.2025 17:19, Oleksii Kurochko wrote:
> > > > --- /dev/null
> > > > +++ b/xen/arch/riscv/time.c
> > > > @@ -0,0 +1,38 @@
> > > > +#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 __read_mostly boot_count;
> > > >           
> > > Why not also __ro_after_init? And what is this variable actually needed
> > > for? Common code doesn't use it, so a better name (describing what it
> > > really holds) might be desirable, even if this then means not being in
> > > sync with Arm code.
> > >         
> > To calculate more accurate amount of time since boot.
> >       
> Okay. But how does the name of the variable reflect that? I.e. what it
> is that the count of is being stored? The only meaning I could associate
> to a variable of this name is the number of boot cycles a system went
> through. I.e. nothing that an OS (or hypervisor) would normally count.
>     
But an OS (or hypervisor) doesn't count it, they initialize a variable
once (in my case, it was named as boot_count) and then just subtract it from
get_cycles() to get time relative to this variable (so since Xen boot) and not
since power on as nothing guarantee (at least, I can't find that in the RISC-V spec)
that after power on the value in CSR_TIME will start from 0 what could lead to
some issues, if my understanding is correct, such as if on SoC A timer starts from
let it be 1000 and on SoC B timer value starts from 5000 then all measurements will be
incorrect as Xen will think that for SoC B it was spent more time then for SoC A.

What do you think if boot_count will be renamed to xen_start_time or {initial_}boot_time?

~ Oleksii
> > I think it can be __ro_after_init as it is going to be initialized once.
> >
> > > Furthermore, I can't spot a declaration of this variable. Was it meant
> > > to be static?
> > >         
> > It is going to be used for vtimer functionality and in repogram_timer()
> > so it can't be static.
> >
> > I will add a declaration to asm/time.h:
> > ```
> >    /* Counter value at boot time */
> >    extern uint64_t boot_count;
> > ```
> >
> > Thanks.
> >
> > ~ Oleksii
> >
> >       
>