diff mbox series

[v7,11/17] openrisc: account for 0 starting value in random_get_entropy()

Message ID 20220429001648.1671472-1-Jason@zx2c4.com (mailing list archive)
State Not Applicable
Delegated to: Herbert Xu
Headers show
Series None | expand

Commit Message

Jason A. Donenfeld April 29, 2022, 12:16 a.m. UTC 
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 <tglx@linutronix.de>
Cc: Arnd Bergmann <arnd@arndb.de>
Cc: Jonas Bonn <jonas@southpole.se>
Cc: Stefan Kristiansson <stefan.kristiansson@saunalahti.fi>
Acked-by: Stafford Horne <shorne@gmail.com>
Signed-off-by: Jason A. Donenfeld <Jason@zx2c4.com>
---
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(+)

Comments

Stafford Horne April 30, 2022, 1:19 a.m. UTC | #1 
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 <tglx@linutronix.de>
> Cc: Arnd Bergmann <arnd@arndb.de>
> Cc: Jonas Bonn <jonas@southpole.se>
> Cc: Stefan Kristiansson <stefan.kristiansson@saunalahti.fi>
> Acked-by: Stafford Horne <shorne@gmail.com>
> Signed-off-by: Jason A. Donenfeld <Jason@zx2c4.com>
> ---
> 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
Jason A. Donenfeld April 30, 2022, 1:29 a.m. UTC | #2 
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 <tglx@linutronix.de>
> > Cc: Arnd Bergmann <arnd@arndb.de>
> > Cc: Jonas Bonn <jonas@southpole.se>
> > Cc: Stefan Kristiansson <stefan.kristiansson@saunalahti.fi>
> > Acked-by: Stafford Horne <shorne@gmail.com>
> > Signed-off-by: Jason A. Donenfeld <Jason@zx2c4.com>
> > ---
> > 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
Stafford Horne April 30, 2022, 10:11 p.m. UTC | #3 
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.
Jason A. Donenfeld April 30, 2022, 10:34 p.m. UTC | #4 
Hi Stafford,

On Sun, May 01, 2022 at 07:11:37AM +0900, Stafford Horne wrote:
 
> 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)

Yea, great, I was thinking about doing it in assembly earlier in boot
too, but didn't know how you'd feel about that. I like this better.

The reason I think this is a good approach is that it means the cycle
counter includes some information about how long startup takes from the
earliest stages -- which could involve probing various devices or
strange things. So enabling the timer in head.S seems good to me.

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

I think most other archs (like, say, x86) have their cycle counter
enabled by default at boot time. I was surprised to see that the or1k
risc cycle counter comes disabled by default actually.

I'll send a v9 incorporating your suggested assembly change.

Jason
Stafford Horne April 30, 2022, 10:44 p.m. UTC | #5 
On Sun, May 01, 2022 at 12:34:02AM +0200, Jason A. Donenfeld wrote:
> Hi Stafford,
> 
> On Sun, May 01, 2022 at 07:11:37AM +0900, Stafford Horne wrote:
>  
> > 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)
> 
> Yea, great, I was thinking about doing it in assembly earlier in boot
> too, but didn't know how you'd feel about that. I like this better.
> 
> The reason I think this is a good approach is that it means the cycle
> counter includes some information about how long startup takes from the
> earliest stages -- which could involve probing various devices or
> strange things. So enabling the timer in head.S seems good to me.
> 
> > 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.
> 
> I think most other archs (like, say, x86) have their cycle counter
> enabled by default at boot time. I was surprised to see that the or1k
> risc cycle counter comes disabled by default actually.
> 
> I'll send a v9 incorporating your suggested assembly change.

Thanks!

-Stafford
diff mbox series

Patch

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