Re: [Xen-devel] [PATCH v3 3/5] xentrace: Timestamp support for ARM platform

[Julien Grall <julien.grall@xxxxxxx>]

Hello Benjamin,

On 04/04/16 19:48, Benjamin Sanda wrote:
> Moved get_cycles() to time.c and modified to return the core timestamp
> tick count for use by the trace buffer timestamping routines in
> xentrace. get_cycles() was moved to the C file to avoid including the
> register specific header file in time.h and to commonize it with the
> get_s_time() function. Also defined cycles_t as uint64_t to simplify
> casting.
I'm not sure what you mean by "simplify casting".

The type cycles_t is not correctly defined for ARM32 because "unsigned 
long" is always 32-bits. However, the physical count register (CNTPCT) 
is always 64-bits. So the number of cycles would have been truncated.
The rest of the patch looks good to me.

> get_s_time() was also modified to now use the updated get_cycles() to
> retrieve the tick count instead of directly reading it.
>
> Signed-off-by: Benjamin Sanda <ben.sanda@xxxxxxxxxxxxxxx>
>
> ---
> Changed since v2:
>    * Combined v2 patches 7 and 6 into one patch in v3. No code change.
>
> ---
> Changed since v1:
>    * Moved get_cycles() to time.c
>    * Added function prototype for get_cycles()
> ---
>   xen/arch/arm/time.c        |  9 ++++++++-
>   xen/include/asm-arm/time.h | 11 +++++------
>   2 files changed, 13 insertions(+), 7 deletions(-)
>
> diff --git a/xen/arch/arm/time.c b/xen/arch/arm/time.c
> index 7dae28b..9aface3 100644
> --- a/xen/arch/arm/time.c
> +++ b/xen/arch/arm/time.c
> @@ -192,10 +192,17 @@ int __init init_xen_time(void)
>   /* Return number of nanoseconds since boot */
>   s_time_t get_s_time(void)
>   {
> -    uint64_t ticks = READ_SYSREG64(CNTPCT_EL0) - boot_count;
> +    cycles_t ticks = get_cycles();
>       return ticks_to_ns(ticks);
>   }
>
> +/* Return the number of ticks since boot */
> +cycles_t get_cycles(void)
> +{
> +        /* return raw tick count of main timer */
> +        return READ_SYSREG64(CNTPCT_EL0) - boot_count;
> +}
> +
>   /* Set the timer to wake us up at a particular time.
>    * Timeout is a Xen system time (nanoseconds since boot); 0 disables the 
> timer.
>    * Returns 1 on success; 0 if the timeout is too soon or is in the past. */
> diff --git a/xen/include/asm-arm/time.h b/xen/include/asm-arm/time.h
> index 5b9a31d..b57f4c1 100644
> --- a/xen/include/asm-arm/time.h
> +++ b/xen/include/asm-arm/time.h
> @@ -5,12 +5,8 @@
>       DT_MATCH_COMPATIBLE("arm,armv7-timer"), \
>       DT_MATCH_COMPATIBLE("arm,armv8-timer")
>
> -typedef unsigned long cycles_t;
> -
> -static inline cycles_t get_cycles (void)
> -{
> -        return 0;
> -}
> +/* Tick count type */
> +typedef uint64_t cycles_t;
>
>   /* List of timer's IRQ */
>   enum timer_ppi
> @@ -37,6 +33,9 @@ extern void init_timer_interrupt(void);
>   /* Counter value at boot time */
>   extern uint64_t boot_count;
>
> +/* Get raw system tick count */
> +cycles_t get_cycles(void);
> +
>   extern s_time_t ticks_to_ns(uint64_t ticks);
>   extern uint64_t ns_to_ticks(s_time_t ns);
Regards,

--
Julien Grall

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel