> Date: Thu, 9 Jul 2020 11:29:05 -0500
> From: Scott Cheloha <[email protected]>
> Cc: [email protected]
> Content-Type: text/plain; charset=us-ascii
> Content-Disposition: inline
>
> On Thu, Jul 09, 2020 at 10:35:46AM +0200, Mark Kettenis wrote:
> >
> > Here is the arm64 version. Again I've taken the approach of copying
> > the kernel timecounter code verbatim. Technically we don't need the
> > Cortex-A73 errata workaround here since the timecounter only uses the
> > low 32 bits. But that is true for the kernel as well! If people
> > think it is worth avoiding this, I'd propose to introduce
> > agtimer_readcnt32() and use that for the timecounter in both the
> > kernel and userland.
>
> I think keeping it simple in userspace is preferable. We only want the
> lower 32 bits, so let's only work with those bits.
>
> While discussing the powerpc usertc code Theo pointed out to me
> (again) that you can get a context switch at any moment in userspace.
> Multiple reads may yield results from multiple cores.
>
> Said another way...
>
> > @@ -18,4 +18,39 @@
> > #include <sys/types.h>
> > #include <sys/timetc.h>
> >
> > -int (*const _tc_get_timecount)(struct timekeep *, u_int *) = NULL;
> > +static inline uint64_t
> > +agtimer_readcnt64(void)
> > +{
> > + uint64_t val0, val1;
> > +
> > + /*
> > + * Work around Cortex-A73 errata 858921, where there is a
> > + * one-cycle window where the read might return the old value
> > + * for the low 32 bits and the new value for the high 32 bits
> > + * upon roll-over of the low 32 bits.
> > + */
> > + __asm volatile("isb" : : : "memory");
> > + __asm volatile("mrs %x0, CNTVCT_EL0" : "=r" (val0));
>
> Hypothetically, what might happen if you were switched *here*?
> Between these two instructions?
I think it would be alright. If the rollover happened during the
context switch we'd detect it and pick the "old" time.
>
> > + __asm volatile("mrs %x0, CNTVCT_EL0" : "=r" (val1));
> > + return ((val0 ^ val1) & 0x100000000ULL) ? val0 : val1;
> > +}
Nevertheless, here is a different take on the problem. Since the
timecounter only uses the low 32 bits we don't need the double read.
This version also changes the timecounter mask from 0x7fffffff to
0xffffffff. That must be ok, since the counter has 64 bits and we are
already using 0xffffffff as a mask on amd64 and sparc64.
ok?
Index: sys/arch/arm64/dev/agtimer.c
===================================================================
RCS file: /cvs/src/sys/arch/arm64/dev/agtimer.c,v
retrieving revision 1.14
diff -u -p -r1.14 agtimer.c
--- sys/arch/arm64/dev/agtimer.c 11 Jul 2020 15:22:44 -0000 1.14
+++ sys/arch/arm64/dev/agtimer.c 11 Jul 2020 18:35:12 -0000
@@ -43,7 +43,8 @@ int32_t agtimer_frequency = TIMER_FREQUE
u_int agtimer_get_timecount(struct timecounter *);
static struct timecounter agtimer_timecounter = {
- agtimer_get_timecount, NULL, 0x7fffffff, 0, "agtimer", 0, NULL, 0
+ agtimer_get_timecount, NULL, 0xffffffff, 0, "agtimer", 0, NULL,
+ TC_AGTIMER
};
struct agtimer_pcpu_softc {
@@ -191,7 +192,15 @@ agtimer_attach(struct device *parent, st
u_int
agtimer_get_timecount(struct timecounter *tc)
{
- return agtimer_readcnt64();
+ uint64_t val;
+
+ /*
+ * No need to work around Cortex-A73 errata 858921 since we
+ * only look at the low 32 bits here.
+ */
+ __asm volatile("isb" ::: "memory");
+ __asm volatile("mrs %x0, CNTVCT_EL0" : "=r" (val));
+ return (val & 0xffffffff);
}
int
Index: lib/libc/arch/aarch64/gen/usertc.c
===================================================================
RCS file: /cvs/src/lib/libc/arch/aarch64/gen/usertc.c,v
retrieving revision 1.1
diff -u -p -r1.1 usertc.c
--- lib/libc/arch/aarch64/gen/usertc.c 6 Jul 2020 13:33:05 -0000 1.1
+++ lib/libc/arch/aarch64/gen/usertc.c 11 Jul 2020 18:35:12 -0000
@@ -1,6 +1,6 @@
-/* $OpenBSD: usertc.c,v 1.1 2020/07/06 13:33:05 pirofti Exp $ */
+/* $OpenBSD$ */
/*
- * Copyright (c) 2020 Paul Irofti <[email protected]>
+ * Copyright (c) 2020 Mark Kettenis <[email protected]>
*
* Permission to use, copy, modify, and distribute this software for any
* purpose with or without fee is hereby granted, provided that the above
@@ -18,4 +18,30 @@
#include <sys/types.h>
#include <sys/timetc.h>
-int (*const _tc_get_timecount)(struct timekeep *, u_int *) = NULL;
+static inline u_int
+agtimer_get_timecount(struct timecounter *tc)
+{
+ uint64_t val;
+
+ /*
+ * No need to work around Cortex-A73 errata 858921 since we
+ * only look at the low 32 bits here.
+ */
+ __asm volatile("isb" ::: "memory");
+ __asm volatile("mrs %x0, CNTVCT_EL0" : "=r" (val));
+ return (val & 0xffffffff);
+}
+
+static int
+tc_get_timecount(struct timekeep *tk, u_int *tc)
+{
+ switch (tk->tk_user) {
+ case TC_AGTIMER:
+ *tc = agtimer_get_timecount(NULL);
+ return 0;
+ }
+
+ return -1;
+}
+
+int (*const _tc_get_timecount)(struct timekeep *, u_int *) = tc_get_timecount;
