> Cc: dera...@openbsd.org, kern...@gmail.com, tech@openbsd.org
> From: Paul Irofti <p...@irofti.net>
> Date: Tue, 2 Jun 2020 16:45:16 +0300
>
> On 2020-06-02 16:29, Mark Kettenis wrote:
> >> From: Paul Irofti <p...@irofti.net>
> >> Date: Tue, 2 Jun 2020 16:23:30 +0300
> >>
> >> On 2020-05-31 20:46, Mark Kettenis wrote:
> >>>> From: Paul Irofti <p...@irofti.net>
> >>>> Date: Sun, 31 May 2020 19:12:54 +0300
> >>>>
> >>>> On 2020-05-31 18:25, Theo de Raadt wrote:
> >>>>> Mark Kettenis <mark.kette...@xs4all.nl> wrote:
> >>>>>
> >>>>>>> I changed __amd64 to __amd64__ because I didn't find __powerpc. I'm
> >>>>>>> not sure, but one might move the list of arches to dlfcn/Makefile.inc
> >>>>>>> and do -DTIMEKEEP, like how thread/Makefile.inc does -DFUTEX. One
> >>>>>>> might drop the tc_get_timecount function pointer and just always call
> >>>>>>> the function #ifdef TIMEKEEP.
> >>>>>>
> >>>>>> Yes, we prefer the __xxx__ variants in OpenBSD code; thanks for
> >>>>>> catching that. The benefit of the TIMEKEEP define would be that we
> >>>>>> can eliminate the fallback code completely on architectures that don't
> >>>>>> implement this functionality.
> >>>>>
> >>>>> ...
> >>>>
> >>>> Yeah, I just followed the dlfcn/dlfcn_stubs.c example from libc. Which I
> >>>> see now it is commented out...
> >>>>
> >>>>>>> --- lib/libc/dlfcn/init.c.before Sat May 30 23:26:35 2020
> >>>>>>> +++ lib/libc/dlfcn/init.c Sat May 30 18:00:45 2020
> >>>>>>> @@ -70,7 +70,7 @@
> >>>>>>>
> >>>>>>> /* provide definitions for these */
> >>>>>>> const dl_cb *_dl_cb __relro = NULL;
> >>>>>>> -#if defined(__amd64)
> >>>>>>> +#if defined(__amd64__) || defined(__powerpc__)
> >>>>>>> uint64_t (*const tc_get_timecount)(void) = tc_get_timecount_md;
> >>>>>>> #else
> >>>>>>> uint64_t (*const tc_get_timecount)(void) = NULL;
> >>>>>
> >>>>> 1) I think adding _md to the name is superflous. There will never
> >>>>> be a MI version, so tc_get_timecount() is enough.
> >>>>
> >>>> What about pvclock(4)?
> >>>
> >>> What about it? Seems to me what you're really thinking of here is how
> >>> to support more than just one timecounter for a specific architecture.
> >>> Your function pointer is not really going to help in that case.
> >>> You'll need to dispatch to the right function based on some sort of
> >>> machine-specific clock ID.
> >>>
> >>> Oh and BTW, I don't think you're ever going to support pvclock(4).
> >>> Take a look at the code and think how you would do all that magic in
> >>> userland...
> >>>
> >>>>> 2) I hope we can get away from #ifdef __ arch__.
> >>>>> Maybe this can be split into architectures which
> >>>>> a) have a function called tc_get_timecount()
> >>>>> or
> >>>>> b) tc_get_timecount is #define'd to NULL, though I don't
> >>>>> know which MD include file to do that in
> >>>>
> >>>> If we go with something like this or with something like -DTIMEKEEP, how
> >>>> do we handle the different PROTO_WRAP vs. PROTO_NORMAL declarations?
> >>>> Split them in MD headers? But then we end up in the same place. Sort of.
> >>>
> >>> Forget about all that for a moment. Here is an alternative suggestion:
> >>>
> >>> On sparc64 we need to support both tick_timecounter and
> >>> sys_tick_timecounter. So we need some sort of clockid value to
> >>> distnguish between those two. I already suggested to use the tc_user
> >>> field of the timecounter for that. 0 means that a timecounter is not
> >>> usable in userland, a (small) positive integer means a specific
> >>> timecounter type. The code in libc will need to know whether a
> >>> particular timecounter type can be supported. My proposal would be to
> >>> implement a function *on all architecture* that takes the clockid as
> >>> an argument and returns a pointer to the function that implements
> >>> support for that timecounter. On architectures without support, ir
> >>> when called with a clockid that isn't supported, that function would
> >>> simply return NULL.
> >>>
> >>
> >>
> >> What if we declare in libc/arch/*/SYS.h tc_get_timecount to either be
> >> NULL or the prototype of a function. (I know SYS.c is a bit of a
> >> stretch, if not we can create a separate header usertc.h?) And then we
> >> use tc_user to be an ID for architectures such as sparc64 that have more
> >> than one clock and inside libc/*/gen/usertc.c we check which is it and
> >> call a local static function based on it?
> >>
> >> Would that be OK?
> >
> > How are you going to support multiple timecounters on an architecture?
>
> Let's say tsc sets tc_user=1 and acpihpet sets tc_user=2. Then in
> libc/arch/amd64/gen/usertc.c I do:
>
> static uint64_t
> rdtsc()
> {
> uint32_t hi, lo;
> asm volatile("rdtsc" : "=a"(lo), "=d"(hi));
> return ((uint64_t)lo)|(((uint64_t)hi)<<32);
> }
>
> static uint64_t
> acpihpet()
> {
> return rdtsc(); /* JUST TO COMPILE */
> }
>
> static uint64_t (*get_tc[])(void) =
> {
> rdtsc,
> acpihpet,
> };
>
> uint64_t
> tc_get_timecount_md(struct timekeep *tk)
> {
> return (*get_tc[tk->tk_user])();
> }
Ignoring the off-by-one in the array access, how is this going to work
if we add a new timecounter on the kernel side that has tc_user = 3?
So I'm suggesting again that we need a function that checks whether
libc actually supports a particular timecounter type. And I propose
that we implement that function on *all* architectures which solves
the issue of finding an MD header file.
Note that implementing this isn't entirely trivial as there are
potential TOCTOU issues.
