On Thu, May 27, 2021 at 09:54:03AM +1000, Benjamin Herrenschmidt wrote:

> I'm trying to understand the rationale for having SMCCC take precedence
> over the architected instruction in
> arch/arm64/include/asm/archrandom.h 

> Especially I don't quite get the comment about SMCCC being more in the
> "spirit" than let's say RNDRSS. It's definitely MUCH slower.

Like the comments say the arm64 RNDR register is not intended to
be directly providing entropy but rather returning the output of
a DRBG which is seeded from an actual entropy source whereas the
SMCCC call is intended to return something from an actual entropy
source directly.  The thinking was that the latter is more in
line with what the callers are expecting for the _seed variants,
it's kind of unclear what the performance/quality tradeoff is
supposed to be though.

See the thread at

   https://lore.kernel.org/r/CAKv+Gu8y4zwpesytU7vffSCuq8YAjWcHwFHwa_LhTW_cLiSfQw@mail.gmail.com

for discussion of the use of RNDR rather than RNDRRS - the main
concern was the potential for issues with excessive use of
entropy especially on bigger systems, possibly amplified if
there's lots of VMs, though in the meantime the random.c code got
updated to read seeds a lot less often (390596c9959c2a4f5b4
"random: avoid arch_get_random_seed_long() when collecting IRQ
randomness") so the considerations have changed a bit since the
original discussion and we could probably already revisit that
one already.

> Also, why not implement arch_get_random_* using RNDR rather than
> forcing these through the kernel CNRG ?

That was the same consideration with potential issues due to
excessive usage, it was dropped at the same time as switching to
RNDR for the _seed variants.  

This was all before I started looking at the series but my
understanding is that the general idea was to go for a
conservative implementation initially and then reevaluate once
we've got actual systems and can measure how things perform in
practice.

In practice most of the non-seed arch_get_random usage is either
a fallback if there's no seed variant or mixed in with the CRNG
output anyway.

> Finally, RNDR/RNDRSS can fail occasionally, we should probably
> implement some kind of delay+retry here. I'm not super familiar with
> the context in which arch_get_random_* can be called (are they allowed
> to sleep ?) but from the implementation I'm aware of at least, the idea
> is that they might under some HW recovery scenarii so we should sleep a
> couple of ms and try again a few times.

The arch_get_random_ interfaces already provide a return code
which the callers can handle as needed, they can do something
appropriate for the scenario rather than the architecture code
having to pick.  Sometimes that's to retry later (random.c does
this when seeding for example), sometimes that's to just carry on
with whatever other entropy they've already got.