On 11/14/19 3:25 PM, Mark Rutland wrote:
> On Thu, Nov 14, 2019 at 12:39:32PM +0100, richard.henderson@xxxxxxxxxx wrote:
>> +bool arch_get_random_seed_long(unsigned long *v)
>> +{
>> + bool ok;
>> +
>> + if (static_branch_likely(&arm64_const_caps_ready)) {
>> + if (__cpus_have_const_cap(ARM64_HAS_RNG))
>> + return arm64_rndr(v);
>> + return false;
>> + }
>> +
>> + /*
>> + * Before const_caps_ready, check the current cpu.
>> + * This will generally be the boot cpu for rand_initialize().
>> + */
>> + preempt_disable_notrace();
>> + ok = this_cpu_has_cap(ARM64_HAS_RNG) && arm64_rndr(v);
>> + preempt_enable_notrace();
>> +
>> + return ok;
>> +}
>
> As I asked previously, please separate the common case and the boot-cpu
> init-time case into separate functions.
Ok, beyond just making arch_get_random_seed_long be a function pointer, how?
I honestly don't understand how what you want is different from what's here.
> The runtime function should just check the RNG cap before using the
> instruction, without any preemption check or explicit check of
> arm64_const_caps_ready. i.e.
>
> static bool arm64_rndr(unsigned long *v)
> {
> bool ok;
>
> if (!cpus_have_const_cap(ARM64_HAS_RNG))
> return false;
>
> /*
> * Reads of RNDR set PSTATE.NZCV to 0b0000 on success,
> * and set PSTATE.NZCV to 0b0100 otherwise.
> */
> asm volatile(
> __mrs_s("%0", SYS_RNDR_EL0) "\n"
> " cset %w1, ne\n"
> : "=r" (*v), "=r" (ok)
> :
> : "cc");
>
> return ok;
This is exactly what I have above, in arch_get_random_seed_long(), in the
arm64_const_caps_ready case.
BTW, you can't stick the cpus_have_const_cap check in arm64_rndr(), or it isn't
usable before setup_cpu_features(). The embedded cpus_have_cap() check will
not pass for the boot cpu alone, unless we use
ARM64_CPUCAP_STRICT_BOOT_CPU_FEATURE, which does not have the semantics that
you have requested in previous review rounds.
Which is *why* I wrote the test exactly as I did, so that when
!arm64_const_caps_ready, I can use a different test than cpus_have_cap().
> Any boot-time seeding should be in a separate function that external
> callers cannot invoke at runtime. Either have an arch function that the
> common random code calls at init time on the boot CPU, or have some
> arch_add_foo_entropy() function that the arm64 code can call somewhere
> around setup_arch().
What "external callers" are you talking about?
My boot-time code above can only be reached until arm64_const_caps_ready, at
which point the branch is patched out. So after boot-time, "external callers"
only get
if (__cpus_have_const_cap(ARM64_HAS_RNG))
return arm64_rndr(v);
return false;
Which to my mind satisfies your "cannot invoke" clause.
Anyway, the preempt_disable is present on my boot path because preempt *is*
enabled during rand_initialize(). If I do not disable it, I do trigger the
warning within this_cpu_has_cap()
As for arch_add_boot_entropy() or whatnot... you're now you're asking for
non-trivial changes to the common drivers/char/random.c code. I'm not keen on
designing such a thing when I really don't know what the requirements are. In
particular, how it would differ from what I have.
Color me confused.
r~
