2020-11-05 17:19:09

On 05/11/2020 14:38, Mark Rutland wrote:

Hi,

> On Thu, Nov 05, 2020 at 02:29:49PM +0000, Mark Brown wrote:
>> On Thu, Nov 05, 2020 at 02:03:22PM +0000, Mark Rutland wrote:
>>> On Thu, Nov 05, 2020 at 01:41:42PM +0000, Mark Brown wrote:
>>
>>>> It isn't obvious to me why we don't fall through to trying the SMCCC
>>>> TRNG here if for some reason the v8.5-RNG didn't give us something.
>>>> Definitely an obscure possibility but still...
>>
>>> I think it's better to assume that if we have a HW RNG and it's not
>>> giving us entropy, it's not worthwhile trapping to the host, which might
>>> encounter the exact same issue.
>>
>> There's definitely a good argument for that, but OTOH it's possible the
>> SMCCC implementation is doing something else (it'd be an interesting
>> implementation decision but...). That said I don't really mind, I think
>> my comment was more that if we're doing this the code should be explicit
>> about what the intent is since right now it isn't obvious. Either a
>> comment or having an explicit "what method are we choosing" thing.
>>
>>> That said, I'm not sure it's great to plumb this under the
>>> arch_get_random*() interfaces, e.g. given this measn that
>>> add_interrupt_randomness() will end up trapping to the host all the time
>>> when it calls arch_get_random_seed_long().
>>
>>> Is there an existing interface for "slow" runtime entropy that we can
>>> plumb this into instead?
>>
>> Yeah, I was wondering about this myself - it seems like a better fit for
>> hwrng rather than the arch interfaces but that's not used until
>> userspace comes up, the arch stuff is all expected to be quick. I
>> suppose we could implement the SMCCC stuff for the early variants of the
>> API you added so it gets used for bootstrapping purposes and then we
>> rely on userspace keeping things topped up by fetching entropy through
>> hwrng or otherwise but that feels confused so I have a hard time getting
>> enthusiastic about it.
>
> I'm perfectly happy for the early functions to call this, or for us to
> add something new firmwware_get_random_*() functions that we can call
> early (and potentially at runtime, but less often than
> arch_get_random_*()).
>
> I suspect the easy thing to do for now is plumb this into the existing
> early arch functions and hwrng.

So coming back to this: With Ard's patch to remove arch_get_random from
add_interrupt_randomness(), I see this called much less often: basically
once at early boot, then 16 longs every 5 minutes or so, from the
periodic crng reseed.
The only exception would be the KVM code now, so we are at the grace of
a guest to not swamp us with seed requests. Alternatively we could
remove the direct arch_get_random call from the KVM code, relying on the
general kernel pool instead.

Is this new situation now good enough to keep the SMCCC calls in this
interface here?

I have the hwrng driver ready, which could coexist with the arch_random
implementation. But if the only purpose of /dev/hwrng is to let rngd
feed this entropy back into the kernel, it would be pointless.
I found the driver useful to debug and test the firmware implementation
and to assess the random number quality (by feeding the raw stream into
rngtest or dieharder), but that might not justify a merge.

Ard objected against the driver, I guess to keep things simple and
architectural.

So what is the plan here? Shall I post a v3 with or without the hwrng
driver? And do we keep the SMCCC arch_random implementation?

Cheers,
Andre