Re: [Minios-devel] [UNIKRAFT PATCHv4 23/43] plat/kvm: Add Arm64 virtual timer library to provide ticks

[Julien Grall <julien.grall@xxxxxxx>]

Hi Wei,

On 07/06/2018 10:03 AM, Wei Chen wrote:
> On KVM platform, print debug message will use ukplat_monotonic_clock
> to provide timestamp. So we implement this simple virtual timer
> library for timestamp.
>
> Signed-off-by: Wei Chen <Wei.Chen@xxxxxxx>
> ---
>   plat/kvm/arm/time.c | 127 ++++++++++++++++++++++++++++++++++++++++++++
>   1 file changed, 127 insertions(+)
>   create mode 100644 plat/kvm/arm/time.c
>
> diff --git a/plat/kvm/arm/time.c b/plat/kvm/arm/time.c
> new file mode 100644
> index 0000000..ab4968f
> --- /dev/null
> +++ b/plat/kvm/arm/time.c
The timer is generic on Arm. How about moving that to common arm64 code?

> @@ -0,0 +1,127 @@
> +/* SPDX-License-Identifier: BSD-3-Clause */
Same remark as before for SPDX.

> +/*
> + * Authors: Wei Chen <Wei.Chen@xxxxxxx>
> + *
> + * Copyright (c) 2018, Arm Ltd. All rights reserved.
> + *
> + * Redistribution and use in source and binary forms, with or without
> + * modification, are permitted provided that the following conditions
> + * are met:
> + *
> + * 1. Redistributions of source code must retain the above copyright
> + *    notice, this list of conditions and the following disclaimer.
> + * 2. Redistributions in binary form must reproduce the above copyright
> + *    notice, this list of conditions and the following disclaimer in the
> + *    documentation and/or other materials provided with the distribution.
> + * 3. Neither the name of the copyright holder nor the names of its
> + *    contributors may be used to endorse or promote products derived from
> + *    this software without specific prior written permission.
> + *
> + * THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS "AS IS"
> + * AND ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED TO, THE
> + * IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR A PARTICULAR PURPOSE
> + * ARE DISCLAIMED. IN NO EVENT SHALL THE COPYRIGHT HOLDER OR CONTRIBUTORS BE
> + * LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL, SPECIAL, EXEMPLARY, OR
> + * CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT LIMITED TO, PROCUREMENT OF
> + * SUBSTITUTE GOODS OR SERVICES; LOSS OF USE, DATA, OR PROFITS; OR BUSINESS
> + * INTERRUPTION) HOWEVER CAUSED AND ON ANY THEORY OF LIABILITY, WHETHER IN
> + * CONTRACT, STRICT LIABILITY, OR TORT (INCLUDING NEGLIGENCE OR OTHERWISE)
> + * ARISING IN ANY WAY OUT OF THE USE OF THIS SOFTWARE, EVEN IF ADVISED OF THE
> + * POSSIBILITY OF SUCH DAMAGE.
> + *
> + * THIS HEADER MAY NOT BE EXTRACTED OR MODIFIED IN ANY WAY.
> + */
> +#include <stdlib.h>
> +#include <uk/assert.h>
> +#include <uk/plat/time.h>
> +
> +static uint64_t cntvct_at_init;
How about boot_ticks here?

> +static uint32_t counter_freq;
> +/*
> + * Shift factor for TSC scaling multiplier; referred to as S in the following
TSC has no meaning on Arm.

> + * comments.
> + */
> +static uint8_t tsc_shift;
> +
> +/* Multiplier for converting TSC ticks to nsecs. (0.S) fixed point. */
> +static uint32_t ns_per_tick;
> +
> +/*
> + * System Time
> + * 64 bit value containing the nanoseconds elapsed since boot time.
> + * This value is adjusted by frequency drift.
> + * NOW() returns the current time.
> + * The other macros are for convenience to approximate short intervals
> + * of real time into system time
This looks like a copy of include/uk/arch/time.h. I don't really 
understand how this fit in the Arm context. For instance what does 
"frequency drift" stand on Arm?
> + */
> +#define NSEC_PER_SEC 1000000000ULL
It looks like to me this should go in common code.

> +
> +static inline uint64_t ticks_to_ns(uint64_t ticks)
> +{
> +       return (ns_per_tick * ticks) >> tsc_shift;
> +}
> +
> +static inline uint64_t get_counter_frequency(void)
> +{
> +       uint64_t frq;
> +
> +       __asm__ __volatile__("mrs %0, cntfrq_el0" : "=r" (frq):: "memory");
> +
> +       return frq;
> +}
This should really be in a arch header and use SYSREG_READ.

> +
> +static inline uint64_t read_virtual_count(void)
> +{
> +       uint64_t val;
> +
> +       __asm__ __volatile__("mrs %0, cntvct_el0" : "=r" (val)::);
> +       return val;
> +}
Same here.


> +
> +/* monotonic_clock(): returns # of nanoseconds passed since time_init()
Coding style:

/*
 * monotonic_clock ...
 * ...
 */
> + * Note: This function is required to return accurate
> + *       time even in the absence of multiple timer ticks.
I don't understand this comment.

> + */
> +__nsec ukplat_monotonic_clock(void)
> +{
> +       return (__nsec) ticks_to_ns(read_virtual_count() - cntvct_at_init);
> +}
> +
> +void ukplat_time_init(void)
> +{
> +       /*
> +        * Calculate TSC shift factor and scaling multiplier.
> +        *
> +        * tsc_shift (S) needs to be the largest (<=32) shift factor where the
> +        * result of the tsc_mult calculcation below fits into uint32_t without
s/calculcation/calculation/

> +        * truncation. Note that we disallow an S of zero to ensure the loop 
> always
> +        * terminates.
> +        *
> +        * (0.S) tsc_mult = NSEC_PER_SEC (S.S) / tsc_freq (S.0)
> +        */
> +       uint64_t tmp;
> +
> +       counter_freq = get_counter_frequency();
> +       tsc_shift = 32;
> +       do {
> +               tmp = (NSEC_PER_SEC << tsc_shift) / counter_freq;
> +               if ((tmp & 0xFFFFFFFF00000000L) == 0L)
> +                       ns_per_tick = (uint32_t)tmp;
> +               else
> +                       tsc_shift--;
> +       } while (tsc_shift > 0 && ns_per_tick == 0L);
> +#if 0 /* assert has been implemented for kvm */
The comment seems to contradict the implementation. So what's the state 
of assert? Looking at it, there seem to have a generic implementation 
for that.
> +       assert(ns_per_tick != 0L);
> +#endif
> +
> +       /*
> +        * Monotonic time begins at tsc_base (first read of TSC before
tsc_base is not defined here. Did you mean cntvct_at_init?

> +        * calibration).
> +        */
> +       cntvct_at_init = read_virtual_count();
> +}
> +
> +void ukplat_time_fini(void)
> +{
> +       /* TODO */
> +}
Cheers,

--
Julien Grall

_______________________________________________
Minios-devel mailing list
Minios-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/minios-devel