Re: Patch: CPU RNG framework with stub x86 implementation

[Taylor R Campbell <campbell+netbsd-tech-kern%mumble.net@localhost>]

   Date: Sun, 10 Jan 2016 21:08:45 -0500
   From: Thor Lancelot Simon <tls%panix.com@localhost>

   As requested - here's just "cpu_rng" itself for review.  I believe this
   version addresses all comments received so far except that I _like_ the
   construct "size_t cnt = <constant>; type_t foo[x]" so I've retained
   that.

At the very least qualify the declaration of cnt with const.

Note that for

	type_t foo[<constant expression>];

you can always write, and the standard idiom in our tree is,
__arraycount(foo) for the value of <constant expression>.

I would personally prefer that over your idiom, but I don't object to
yours, as long as you make it const.

   On the other hand, I _dislike_ needless forward declarations so I've
   moved a small amount of code in kern_rndq.c to avoid another one of
   those.

OK, but can you make cosmetic changes like that in a separate commit?

Similarly, make all the memset->explicit_memset changes in a separate
commit -- just go ahead and do that one first.

   If this looks OK, I'll commit it and the next thing for review will be
   the RDRAND backend; then RDSEED, then VIA.  I have no hardware with RDSEED
   nor a VIA RNG to test, so I'd appreciate help with that.

OK.  I'm not sure about Intel hardware with RDSEED, but I may have VIA
hardware with their CPU RNG, if that's standard on all their hardware.
(Finding time to do tests, though, is another matter entirely...)

Some in-line comments:

   +static void
   +rnd_cpu_get(size_t bytes, void *priv)
   +{
   +       krndsource_t *cpusrcp = priv;
   +       KASSERT(cpusrcp == &rnd_cpu.source);
   +
   +       if (__predict_true(rnd_cpu.intr != NULL)) {
   +               rnd_schedule_softint(rnd_cpu.intr);
   +       }
   +}

All the conditionally initialized softint business keeps tripping me
up.  It seems that we ought not to install the cpurng source until we
have established the softint.  Can we not softint_establish before we
attach the cpurng rndsource?

Also, why does this need to happen in softint context anyway?  Isn't
the point of the CPU RNG that it is super-cheap and never requires
asynchronous access?  The previous patch didn't do that -- why do it
now?

Another question:  Is the cpurng(9) API supposed to always make sense
independently and concurrently on every CPU?  Certainly RDRAND/RDSEED
satisfy that, but I don't know about, e.g., the VIA CPU RNG, or the
Broadcom one, &c.

I'm asking because I would like to know whether you intend for it to
generically makes sense to attach one cpurng(9) rndsource per CPU.  If
so, then maybe we should just do that, and in the callback, skip if
it's not our CPU, rather than using locks.  If not, then maybe it's
not really an appropriate abstraction for RDRAND/RDSEED, and maybe it
should still be up to the machine platform to decide what CPU RNGs to
attach without generic code in kern_rndq.c.

   +static void
   +rnd_cpu_intr(void *priv)
   +{
   +       size_t entropy = 0;
   +       size_t cnt = 2 * RND_ENTROPY_THRESHOLD / sizeof(cpu_rng_t);
   +       cpu_rng_t buf[cnt];
   +       krndsource_t *cpusrcp = &rnd_cpu.source;
   +
   +        if (RND_ENABLED(cpusrcp)) {

We ought to make rnd_getmore skip disabled sources so that such
conditionals as this are not necessary.

(Looks like you mixed tabs and spaces there, by the way -- C-x h M-x
tabify RET.)

   +#if defined(__HAVE_CPU_RNG)
   +       if (cpu_rng_init()) {
   +               /* IPL_VM because taken while rnd_global.lock is held.  */
   +               mutex_init(&rnd_cpu.lock, MUTEX_DEFAULT, IPL_VM);

Really?  Why is this taken while rnd_global.lock is held?  Is this
copypasta of rnd_skew.lock?  I think both can be IPL_NONE, or
IPL_SOFTFOO, after the rearrangements I made last year so that
rnd_getmore holds no rnd locks when it calls the callbacks.

...nope: rndsinks.lock (which is also at IPL_VM) might be held.  But
not rnd_global.lock.  So both comments should be updated.

   +               rndsource_setcb(&rnd_cpu.source, rnd_cpu_get, &rnd_cpu.source);
   +               rnd_attach_source(&rnd_cpu.source, "cpurng",
   +                   RND_TYPE_RNG, RND_FLAG_COLLECT_VALUE|
   +                   RND_FLAG_HASCB|RND_FLAG_HASENABLE);
   +               rnd_cpu_intr(NULL);

Either both calls to rnd_cpu_get should pass NULL, or both calls
should pass &rnd_cpu.source -- no reason to make them different.