In the event that random_get_entropy() can't access a cycle counter or
similar, falling back to returning 0 is really not the best we can do.
Instead, at least calling random_get_entropy_fallback() would be
preferable, because that always needs to return _something_, even
falling back to jiffies eventually. It's not as though
random_get_entropy_fallback() is super high precision or guaranteed to
be entropic, but basically anything that's not zero all the time is
better than returning zero all the time.
Cc: Thomas Gleixner <[email protected]>
Cc: Arnd Bergmann <[email protected]>
Cc: Jonas Bonn <[email protected]>
Cc: Stefan Kristiansson <[email protected]>
Cc: Stafford Horne <[email protected]>
Signed-off-by: Jason A. Donenfeld <[email protected]>
---
arch/openrisc/include/asm/timex.h | 3 +++
1 file changed, 3 insertions(+)
diff --git a/arch/openrisc/include/asm/timex.h b/arch/openrisc/include/asm/timex.h
index d52b4e536e3f..115e89b336cd 100644
--- a/arch/openrisc/include/asm/timex.h
+++ b/arch/openrisc/include/asm/timex.h
@@ -23,6 +23,9 @@ static inline cycles_t get_cycles(void)
{
return mfspr(SPR_TTCR);
}
+#define get_cycles get_cycles
+
+#define random_get_entropy() (((unsigned long)get_cycles()) ?: random_get_entropy_fallback())
/* This isn't really used any more */
#define CLOCK_TICK_RATE 1000
--
2.35.1
On Sat, Apr 23, 2022 at 11:26:17PM +0200, Jason A. Donenfeld wrote:
> In the event that random_get_entropy() can't access a cycle counter or
> similar, falling back to returning 0 is really not the best we can do.
> Instead, at least calling random_get_entropy_fallback() would be
> preferable, because that always needs to return _something_, even
> falling back to jiffies eventually. It's not as though
> random_get_entropy_fallback() is super high precision or guaranteed to
> be entropic, but basically anything that's not zero all the time is
> better than returning zero all the time.
>
> Cc: Thomas Gleixner <[email protected]>
> Cc: Arnd Bergmann <[email protected]>
> Cc: Jonas Bonn <[email protected]>
> Cc: Stefan Kristiansson <[email protected]>
> Cc: Stafford Horne <[email protected]>
> Signed-off-by: Jason A. Donenfeld <[email protected]>
> ---
> arch/openrisc/include/asm/timex.h | 3 +++
> 1 file changed, 3 insertions(+)
>
> diff --git a/arch/openrisc/include/asm/timex.h b/arch/openrisc/include/asm/timex.h
> index d52b4e536e3f..115e89b336cd 100644
> --- a/arch/openrisc/include/asm/timex.h
> +++ b/arch/openrisc/include/asm/timex.h
> @@ -23,6 +23,9 @@ static inline cycles_t get_cycles(void)
> {
> return mfspr(SPR_TTCR);
> }
> +#define get_cycles get_cycles
> +
> +#define random_get_entropy() (((unsigned long)get_cycles()) ?: random_get_entropy_fallback())
>
> /* This isn't really used any more */
> #define CLOCK_TICK_RATE 1000
> --
> 2.35.1
Hi Jason,
This looks OK, but I am wondering why we cannot add this to
"include/linux/timex.h" as the default implementation of random_get_entropy
if get_cycles is defined. I see there are 2 cases:
1. architectures that define get_cycles, and random_get_entropy is defined in
include/linux/timex.h.
(openrisc, sparc*, xtensa*, nios2, um*, arm)
2. architectures that define random_get_entropy explicitly
(mips, m68k, riscv, x86)
* Those marked with * just define get_cycles as 0.
I would think in "include/linux/timex.h" we could define.
#ifndef random_get_entropy
#ifdef get_cycles
#define random_get_entropy() (((unsigned long)get_cycles()) ?: random_get_entropy_fallback())
#else
#define random_get_entropy() random_get_entropy_fallback()
#endif
#endif
For architectures that define get_cycles as 0, they can be cleaned up.
-Stafford
As a sanity check, this series makes sure that during early boot, the
cycle counter isn't returning all zeros. However, OpenRISC's TTCR timer
can be rather slow and starts out as zero during stages of early boot.
We know it works, however. So just always add 1 to random_get_entropy()
so that it doesn't trigger these checks.
Cc: Thomas Gleixner <[email protected]>
Cc: Arnd Bergmann <[email protected]>
Cc: Jonas Bonn <[email protected]>
Cc: Stefan Kristiansson <[email protected]>
Acked-by: Stafford Horne <[email protected]>
Signed-off-by: Jason A. Donenfeld <[email protected]>
---
Changes v6->v7:
- Add 1 to cycle counter to account for functional but slow-to-begin
counter on QEMU.
arch/openrisc/include/asm/timex.h | 3 +++
1 file changed, 3 insertions(+)
diff --git a/arch/openrisc/include/asm/timex.h b/arch/openrisc/include/asm/timex.h
index d52b4e536e3f..a78a5807c927 100644
--- a/arch/openrisc/include/asm/timex.h
+++ b/arch/openrisc/include/asm/timex.h
@@ -23,6 +23,9 @@ static inline cycles_t get_cycles(void)
{
return mfspr(SPR_TTCR);
}
+#define get_cycles get_cycles
+
+#define random_get_entropy() ((unsigned long)get_cycles() + 1)
/* This isn't really used any more */
#define CLOCK_TICK_RATE 1000
--
2.35.1
Hi Stafford,
On Sat, Apr 30, 2022 at 10:19:03AM +0900, Stafford Horne wrote:
> Hi Jason,
>
> On Fri, Apr 29, 2022 at 02:16:48AM +0200, Jason A. Donenfeld wrote:
> > As a sanity check, this series makes sure that during early boot, the
> > cycle counter isn't returning all zeros. However, OpenRISC's TTCR timer
> > can be rather slow and starts out as zero during stages of early boot.
> > We know it works, however. So just always add 1 to random_get_entropy()
> > so that it doesn't trigger these checks.
>
> Just one nit, you might want to qualify that this is related to simulators/qemu:
> * "However, in simulators OpenRISC's TTCR timer can be rather slow..."
Nice catch, will do.
Jason
>
> > Cc: Thomas Gleixner <[email protected]>
> > Cc: Arnd Bergmann <[email protected]>
> > Cc: Jonas Bonn <[email protected]>
> > Cc: Stefan Kristiansson <[email protected]>
> > Acked-by: Stafford Horne <[email protected]>
> > Signed-off-by: Jason A. Donenfeld <[email protected]>
> > ---
> > Changes v6->v7:
> > - Add 1 to cycle counter to account for functional but slow-to-begin
> > counter on QEMU.
> >
> > arch/openrisc/include/asm/timex.h | 3 +++
> > 1 file changed, 3 insertions(+)
> >
> > diff --git a/arch/openrisc/include/asm/timex.h b/arch/openrisc/include/asm/timex.h
> > index d52b4e536e3f..a78a5807c927 100644
> > --- a/arch/openrisc/include/asm/timex.h
> > +++ b/arch/openrisc/include/asm/timex.h
> > @@ -23,6 +23,9 @@ static inline cycles_t get_cycles(void)
> > {
> > return mfspr(SPR_TTCR);
> > }
> > +#define get_cycles get_cycles
> > +
> > +#define random_get_entropy() ((unsigned long)get_cycles() + 1)
> >
> > /* This isn't really used any more */
> > #define CLOCK_TICK_RATE 1000
>
> Thanks,
>
> -Stafford
On Sat, Apr 30, 2022 at 03:29:37AM +0200, Jason A. Donenfeld wrote:
> Hi Stafford,
>
> On Sat, Apr 30, 2022 at 10:19:03AM +0900, Stafford Horne wrote:
> > Hi Jason,
> >
> > On Fri, Apr 29, 2022 at 02:16:48AM +0200, Jason A. Donenfeld wrote:
> > > As a sanity check, this series makes sure that during early boot, the
> > > cycle counter isn't returning all zeros. However, OpenRISC's TTCR timer
> > > can be rather slow and starts out as zero during stages of early boot.
> > > We know it works, however. So just always add 1 to random_get_entropy()
> > > so that it doesn't trigger these checks.
> >
> > Just one nit, you might want to qualify that this is related to simulators/qemu:
> > * "However, in simulators OpenRISC's TTCR timer can be rather slow..."
>
> Nice catch, will do.
I was thinking about this, the reason the tick timer is returing 0 is because
the timer is not started. It's getting initialized right after the random
number generator.
A patch like this helps to startup the timer during intial startup, but I am not
sure its the best thing:
diff --git a/arch/openrisc/kernel/head.S b/arch/openrisc/kernel/head.S
index 15f1b38dfe03..a9b3b5614e13 100644
--- a/arch/openrisc/kernel/head.S
+++ b/arch/openrisc/kernel/head.S
@@ -521,6 +521,9 @@ _start:
l.ori r3,r0,0x1
l.mtspr r0,r3,SPR_SR
+ l.movhi r3,hi(SPR_TTMR_CR)
+ l.mtspr r0,r3,SPR_TTMR
+
CLEAR_GPR(r1)
CLEAR_GPR(r2)
CLEAR_GPR(r3)
I was testing this by removing the get_cycles() + 1 patch and I get the
following warning. Starting the tick timer early on in kernel boot helps fix
this issue as well.
But I wonder:
- Why don't any other architectures have similar issues.
- Is there any more correct place to do an early timer kick off.
exec: /home/shorne/work/openrisc/qemu/build/or1k-softmmu/qemu-system-or1k -cpu or1200 -M or1k-sim -kernel /home/shorne/work/linux/vmlinux -initrd /home/shorne/work/linux/initramfs.cpio.gz -serial mon:stdi
o -nographic -gdb tcp::10001 -m 64
[ 0.000000] OpenRISC Linux -- http://openrisc.io
[ 0.000000] percpu: Embedded 5 pages/cpu s11584 r8192 d21184 u40960
[ 0.000000] Built 1 zonelists, mobility grouping on. Total pages: 8160
[ 0.000000] Kernel command line:
[ 0.000000] Dentry cache hash table entries: 8192 (order: 2, 32768 bytes, linear)
[ 0.000000] Inode-cache hash table entries: 4096 (order: 1, 16384 bytes, linear)
[ 0.000000] Sorting __ex_table...
[ 0.000000] mem auto-init: stack:all(zero), heap alloc:off, heap free:off
[ 0.000000] mem_init_done ...........................................
[ 0.000000] Memory: 57424K/65536K available (4595K kernel code, 157K rwdata, 976K rodata, 195K init, 89K bss, 8112K reserved, 0K cma-reserved)
[ 0.000000] rcu: Hierarchical RCU implementation.
[ 0.000000] rcu: RCU restricting CPUs from NR_CPUS=2 to nr_cpu_ids=1.
[ 0.000000] rcu: RCU calculated value of scheduler-enlistment delay is 10 jiffies.
[ 0.000000] rcu: Adjusting geometry for rcu_fanout_leaf=16, nr_cpu_ids=1
[ 0.000000] NR_IRQS: 32, nr_irqs: 32, preallocated irqs: 0
(Warning for get_cycles returning 0)
[ 0.000000] ------------[ cut here ]------------
[ 0.000000] WARNING: CPU: 0 PID: 0 at drivers/char/random.c:1016 rand_initialize+0x15c/0x18c
[ 0.000000] Missing cycle counter and fallback timer; RNG entropy collection will consequently suffer.
[ 0.000000] Modules linked in:
[ 0.000000] CPU: 0 PID: 0 Comm: swapper/0 Not tainted 5.18.0-rc4-17398-gc576c8ccdc30-dirty #724
[ 0.000000] Call trace:
[ 0.000000] [<(ptrval)>] dump_stack_lvl+0x78/0xa0
[ 0.000000] [<(ptrval)>] dump_stack+0x1c/0x2c
[ 0.000000] [<(ptrval)>] __warn+0x100/0x13c
[ 0.000000] [<(ptrval)>] ? rand_initialize+0x15c/0x18c
[ 0.000000] [<(ptrval)>] warn_slowpath_fmt+0x84/0x9c
[ 0.000000] [<(ptrval)>] rand_initialize+0x15c/0x18c
[ 0.000000] [<(ptrval)>] ? start_kernel+0x5dc/0x7c4
[ 0.000000] [<(ptrval)>] ? start_kernel+0x0/0x7c4
[ 0.000000] ---[ end trace 0000000000000000 ]---
(Start of OpenRISC tick timer)
[ 0.000000] clocksource: openrisc_timer: mask: 0xffffffff max_cycles: 0xffffffff, max_idle_ns: 95563022313 ns
[ 0.000000] 40.00 BogoMIPS (lpj=200000)
[ 0.000000] pid_max: default: 32768 minimum: 301
[ 0.000000] random: get_random_bytes called from net_ns_init+0x94/0x3f8 with crng_init=0
[ 0.000000] Mount-cache hash table entries: 2048 (order: 0, 8192 bytes, linear)
[ 0.000000] Mountpoint-cache hash table entries: 2048 (order: 0, 8192 bytes, linear)
[ 0.010000] rcu: Hierarchical SRCU implementation.
Hi Jason,
On Fri, Apr 29, 2022 at 02:16:48AM +0200, Jason A. Donenfeld wrote:
> As a sanity check, this series makes sure that during early boot, the
> cycle counter isn't returning all zeros. However, OpenRISC's TTCR timer
> can be rather slow and starts out as zero during stages of early boot.
> We know it works, however. So just always add 1 to random_get_entropy()
> so that it doesn't trigger these checks.
Just one nit, you might want to qualify that this is related to simulators/qemu:
* "However, in simulators OpenRISC's TTCR timer can be rather slow..."
> Cc: Thomas Gleixner <[email protected]>
> Cc: Arnd Bergmann <[email protected]>
> Cc: Jonas Bonn <[email protected]>
> Cc: Stefan Kristiansson <[email protected]>
> Acked-by: Stafford Horne <[email protected]>
> Signed-off-by: Jason A. Donenfeld <[email protected]>
> ---
> Changes v6->v7:
> - Add 1 to cycle counter to account for functional but slow-to-begin
> counter on QEMU.
>
> arch/openrisc/include/asm/timex.h | 3 +++
> 1 file changed, 3 insertions(+)
>
> diff --git a/arch/openrisc/include/asm/timex.h b/arch/openrisc/include/asm/timex.h
> index d52b4e536e3f..a78a5807c927 100644
> --- a/arch/openrisc/include/asm/timex.h
> +++ b/arch/openrisc/include/asm/timex.h
> @@ -23,6 +23,9 @@ static inline cycles_t get_cycles(void)
> {
> return mfspr(SPR_TTCR);
> }
> +#define get_cycles get_cycles
> +
> +#define random_get_entropy() ((unsigned long)get_cycles() + 1)
>
> /* This isn't really used any more */
> #define CLOCK_TICK_RATE 1000
Thanks,
-Stafford