> 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.