| Message ID | 20220423212623.1957011-12-Jason@zx2c4.com (mailing list archive) |
|---|---|
| State | Not Applicable |
| Delegated to: | Herbert Xu |
| Headers | show |
| Series | archs/random: fallback to best raw ktime when no cycle counter | expand |
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 <tglx@linutronix.de> > Cc: Arnd Bergmann <arnd@arndb.de> > Cc: Jonas Bonn <jonas@southpole.se> > Cc: Stefan Kristiansson <stefan.kristiansson@saunalahti.fi> > Cc: Stafford Horne <shorne@gmail.com> > Signed-off-by: Jason A. Donenfeld <Jason@zx2c4.com> > --- > 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
Hi Stafford, On Thu, Apr 28, 2022 at 06:42:30AM +0900, Stafford Horne wrote: > 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 <tglx@linutronix.de> > > Cc: Arnd Bergmann <arnd@arndb.de> > > Cc: Jonas Bonn <jonas@southpole.se> > > Cc: Stefan Kristiansson <stefan.kristiansson@saunalahti.fi> > > Cc: Stafford Horne <shorne@gmail.com> > > Signed-off-by: Jason A. Donenfeld <Jason@zx2c4.com> > > --- > > 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 What you've described is what patch 6 of this series already does. However, it does it assuming that if get_cycles() exists, it returns a real value, because most architectures do the right thing by default when they choose to define that function, and thus there is no purpose in adding the needless branch. OpenRISC, however, is a notable outlier in that the code generated by get_cycles() does not correspond to an actual cycle counter on all CPU implementations. In particular, the QEMU simulator always returns 0. And the QEMU simulator is in fact what people are using to test this stuff, so the kernel code needs to actually work for this prevalent "virtual silicon", which I assume is much more widely deployed than any real FPGA running this or that OpenRISC softcore. So this patch includes the branch as part of its definition, so that we get the best of both worlds, which seems to me like a pretty acceptable compromise. Thanks, Jason
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
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 <tglx@linutronix.de> Cc: Arnd Bergmann <arnd@arndb.de> Cc: Jonas Bonn <jonas@southpole.se> Cc: Stefan Kristiansson <stefan.kristiansson@saunalahti.fi> Cc: Stafford Horne <shorne@gmail.com> Signed-off-by: Jason A. Donenfeld <Jason@zx2c4.com> --- arch/openrisc/include/asm/timex.h | 3 +++ 1 file changed, 3 insertions(+)