[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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



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

 


Rackspace

Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.