[v7] arm64: Implement archrandom.h for ARMv8.5-RNG

From: Richard Henderson <richard.henderson@linaro.org>


Expose the ID_AA64ISAR0.RNDR field to userspace, as the RNG system
registers are always available at EL0.

Implement arch_get_random_seed_long using RNDR.  Given that the
TRNG is likely to be a shared resource between cores, and VMs,
do not explicitly force re-seeding with RNDRRS.

Within arch_get_random_seed_long, use arm64_const_caps_ready to
notice when we're being called before system capabilities are
finalized, e.g. within rand_initialize().  In that case, use
this_cpu_has_cap to test support on the boot cpu.

After arm64_const_caps_ready is set, the static_branch'es resolve
to nops or unconditional branches based on the whole system state.

Signed-off-by: Richard Henderson <richard.henderson@linaro.org>

---
v2: Use __mrs_s and fix missing cc clobber (Mark),
    Log rng failures with pr_warn (Mark),
    Use __must_check; put RNDR in arch_get_random_long and RNDRRS
    in arch_get_random_seed_long (Ard),
    Use ARM64_CPUCAP_WEAK_LOCAL_CPU_FEATURE, and check this_cpu_has_cap
    when reading random data.  Move everything out of line, now that
    there are 5 other function calls involved, and to unify the rate
    limiting on the pr_warn.
v3: Keep arch_get_random{,_seed}_long in sync.
v4: Use __cpus_have_const_cap before falling back to this_cpu_has_cap.
v5: Improve commentary; fix some checkpatch warnings.
v6: Drop arch_get_random_long, use RNDR in arch_get_random_seed_long,
    do not use RNDRRS at all (Ard).
    Drop the pr_warn mis-communication (Mark).
    Use ARM64_CPUCAP_SYSTEM_FEATURE for feature detection (Mark).
    Move arch_get_random_seed_long back inline using ALTERNATIVE_CB.
v7: Drop ALTERNATIVE_CB, use only static_branch_likely
    and __cpus_have_const_cap.
    Split RANDOM_TRUST_CPU change to a separate patch.
---
 Documentation/arm64/cpu-feature-registers.rst |  2 +
 arch/arm64/include/asm/archrandom.h           | 29 +++++++++++
 arch/arm64/include/asm/cpucaps.h              |  3 +-
 arch/arm64/include/asm/sysreg.h               |  4 ++
 arch/arm64/kernel/cpufeature.c                | 13 +++++
 arch/arm64/kernel/random.c                    | 49 +++++++++++++++++++
 arch/arm64/Kconfig                            | 12 +++++
 arch/arm64/kernel/Makefile                    |  1 +
 8 files changed, 112 insertions(+), 1 deletion(-)
 create mode 100644 arch/arm64/include/asm/archrandom.h
 create mode 100644 arch/arm64/kernel/random.c

-- 
2.17.1

On Thu, Nov 14, 2019 at 12:39:32PM +0100, richard.henderson@linaro.org 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.

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;
}

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

Thanks,
Mark.

On 11/14/19 3:25 PM, Mark Rutland wrote:
> On Thu, Nov 14, 2019 at 12:39:32PM +0100, richard.henderson@linaro.org 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~

On Thu, Nov 14, 2019 at 07:11:29PM +0100, Richard Henderson wrote:
> On 11/14/19 3:25 PM, Mark Rutland wrote:


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


I believe that what Mark is saying when he says you should change the
arch hooks is that you should change the interface between the core code
and the architecture code to separate the runtime and early init
interfaces with the goal of making it clear the separation between the
two.

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


Again Mark can correct me if I'm wrong here but anything that isn't
the arch code or the core random code.

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


The biggest issue here is one of reviewability and maintainability
around early use of the capabilities code.  Without being really up to
speed on those interfaces it can be hard to tell what conditions are
true when and what interfaces are safe to use at any given point in boot
which makes things harder to work with.  This is especially true when we
start to mix in things like preemption disables which also need a lot of
attention.  Avoiding mixing code that needs to run in both init and
normal runtime contexts in the same function makes things easier to
understand.

Looking briefly at the random code you may find that the existing
add_device_randomness() is already close to the arch_add_foo_entropy()
suggested by Mark if not actually good enough, data injected with that
is not going to be credited as entropy but it will feed into the pool.