Received-SPF: pass (google.com: best guess record for domain of linux-crypto-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
Subject: Re: [PATCH v7] arm64: Implement archrandom.h for ARMv8.5-RNG
To:     Mark Rutland <mark.rutland@arm.com>
Cc:     linux-arm-kernel@lists.infradead.org, ard.biesheuvel@linaro.org,
        linux-crypto@vger.kernel.org
References: <20191114113932.26186-1-richard.henderson@linaro.org>
 <20191114142512.GC37865@lakrids.cambridge.arm.com>
From:   Richard Henderson <richard.henderson@linaro.org>
Openpgp: preference=signencrypt
Message-ID: <3b1d5f2a-5a8d-0c33-176a-f1c35b8356de@linaro.org>
Date:   Thu, 14 Nov 2019 19:11:29 +0100
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:60.0) Gecko/20100101
 Thunderbird/60.9.0
MIME-Version: 1.0
In-Reply-To: <20191114142512.GC37865@lakrids.cambridge.arm.com>
Content-Type: text/plain; charset=utf-8
Content-Language: en-US
Content-Transfer-Encoding: 7bit
Sender: linux-crypto-owner@vger.kernel.org
Precedence: bulk

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~