diff mbox series

[v6,09/17] mips: use fallback for random_get_entropy() instead of just c0 random

Message ID 20220423212623.1957011-10-Jason@zx2c4.com
State New
Headers show
Series archs/random: fallback to best raw ktime when no cycle counter | expand

Commit Message

Jason A. Donenfeld April 23, 2022, 9:26 p.m. UTC 
For situations in which we don't have a c0 counter register available,
we've been falling back to reading the c0 "random" register, which is
usually bounded by the amount of TLB entries and changes every other
cycle or so. This means it wraps extremely often. We can do better by
combining this fast-changing counter with a potentially slower-changing
counter from random_get_entropy_fallback() in the more significant bits.
This commit combines the two, taking into account that the changing bits
are in a different bit position depending on the CPU model. In addition,
we previously were falling back to 0 for ancient CPUs that Linux does
not support anyway; remove that dead path entirely.

Cc: Thomas Gleixner <tglx@linutronix.de>
Cc: Arnd Bergmann <arnd@arndb.de>
Tested-by: Maciej W. Rozycki <macro@orcam.me.uk>
Acked-by: Thomas Bogendoerfer <tsbogend@alpha.franken.de>
Signed-off-by: Jason A. Donenfeld <Jason@zx2c4.com>
---
 arch/mips/include/asm/timex.h | 17 ++++++++---------
 1 file changed, 8 insertions(+), 9 deletions(-)

Comments

Maciej W. Rozycki April 26, 2022, 8:28 p.m. UTC | #1 
On Sat, 23 Apr 2022, Jason A. Donenfeld wrote:

> diff --git a/arch/mips/include/asm/timex.h b/arch/mips/include/asm/timex.h
> index b05bb70a2e46..8cfa485d19e6 100644
> --- a/arch/mips/include/asm/timex.h
> +++ b/arch/mips/include/asm/timex.h
[...]
> +	if (cpu_has_3kex)
> +		c0_random = (read_c0_random() >> 8) & 0x3f;

 Hmm, I wonder whether we do need to mask the contents of the register out 
here given that known implementations return zeros in reserved bits.  Even 
though R3000 documentation I have access to makes no guarantee as to the 
values of the reserved bits here I think we can safely proceed according 
to what systems we do actually support do (even though it only saves one 
instruction).

>  	else
> -		return 0;	/* no usable register */
> +		c0_random = read_c0_random() & 0x3f;

 Here the architecture guarantees unused bits to be zero, but the number 
of them varies between implementations.  However we'll only ever use this 
leg for the R4000/R4400 processors, which have 48 TLB entries, and for the 
Ingenic XBurst cores, which I have now verified in documentation (which 
user-reported dumps from /proc/cpuinfo are consistent with) that have 32 
TLB entries.  So I think this mask operation can go as well.

 I guess these updates can be made with a follow-up change though.

  Maciej
Jason A. Donenfeld April 27, 2022, 1:29 a.m. UTC | #2 
Hey Maciej,

On Tue, Apr 26, 2022 at 09:28:39PM +0100, Maciej W. Rozycki wrote:
> On Sat, 23 Apr 2022, Jason A. Donenfeld wrote:
> 
> > diff --git a/arch/mips/include/asm/timex.h b/arch/mips/include/asm/timex.h
> > index b05bb70a2e46..8cfa485d19e6 100644
> > --- a/arch/mips/include/asm/timex.h
> > +++ b/arch/mips/include/asm/timex.h
> [...]
> > +	if (cpu_has_3kex)
> > +		c0_random = (read_c0_random() >> 8) & 0x3f;
> 
>  Hmm, I wonder whether we do need to mask the contents of the register out 
> here given that known implementations return zeros in reserved bits.  Even 
> though R3000 documentation I have access to makes no guarantee as to the 
> values of the reserved bits here I think we can safely proceed according 
> to what systems we do actually support do (even though it only saves one 
> instruction).
> 
> >  	else
> > -		return 0;	/* no usable register */
> > +		c0_random = read_c0_random() & 0x3f;
> 
>  Here the architecture guarantees unused bits to be zero, but the number 
> of them varies between implementations.  However we'll only ever use this 
> leg for the R4000/R4400 processors, which have 48 TLB entries, and for the 
> Ingenic XBurst cores, which I have now verified in documentation (which 
> user-reported dumps from /proc/cpuinfo are consistent with) that have 32 
> TLB entries.  So I think this mask operation can go as well.
> 
>  I guess these updates can be made with a follow-up change though.

There is lots of optimization potential on a few fronts we've identified
in this thread. Let's save these for a follow-up. I'd rather this
initial one be at least somewhat simple, so that as it gets optimized,
it'll be easy to handle regressions. Also, it probably makes sense for
you to send the patches for these, since you have both the hardware
chops and the hardware itself to assess these ideas. I am interested in
the topic though, so please do CC me.

Jason
Jason A. Donenfeld June 24, 2022, 1:04 p.m. UTC | #3 
Hey Maciej,

On Wed, Apr 27, 2022 at 3:29 AM Jason A. Donenfeld <Jason@zx2c4.com> wrote:
>
> Hey Maciej,
>
> On Tue, Apr 26, 2022 at 09:28:39PM +0100, Maciej W. Rozycki wrote:
> > On Sat, 23 Apr 2022, Jason A. Donenfeld wrote:
> >
> > > diff --git a/arch/mips/include/asm/timex.h b/arch/mips/include/asm/timex.h
> > > index b05bb70a2e46..8cfa485d19e6 100644
> > > --- a/arch/mips/include/asm/timex.h
> > > +++ b/arch/mips/include/asm/timex.h
> > [...]
> > > +   if (cpu_has_3kex)
> > > +           c0_random = (read_c0_random() >> 8) & 0x3f;
> >
> >  Hmm, I wonder whether we do need to mask the contents of the register out
> > here given that known implementations return zeros in reserved bits.  Even
> > though R3000 documentation I have access to makes no guarantee as to the
> > values of the reserved bits here I think we can safely proceed according
> > to what systems we do actually support do (even though it only saves one
> > instruction).
> >
> > >     else
> > > -           return 0;       /* no usable register */
> > > +           c0_random = read_c0_random() & 0x3f;
> >
> >  Here the architecture guarantees unused bits to be zero, but the number
> > of them varies between implementations.  However we'll only ever use this
> > leg for the R4000/R4400 processors, which have 48 TLB entries, and for the
> > Ingenic XBurst cores, which I have now verified in documentation (which
> > user-reported dumps from /proc/cpuinfo are consistent with) that have 32
> > TLB entries.  So I think this mask operation can go as well.
> >
> >  I guess these updates can be made with a follow-up change though.
>
> There is lots of optimization potential on a few fronts we've identified
> in this thread. Let's save these for a follow-up. I'd rather this
> initial one be at least somewhat simple, so that as it gets optimized,
> it'll be easy to handle regressions. Also, it probably makes sense for
> you to send the patches for these, since you have both the hardware
> chops and the hardware itself to assess these ideas. I am interested in
> the topic though, so please do CC me.

Everything has been upstream for a little while now, which means
development of this can move back to the proper MIPS tree like normal.
Did you want to submit some optimizations? Would be happy to look at
whatever you have in mind.

Jason
Maciej W. Rozycki June 24, 2022, 1:25 p.m. UTC | #4 
Hi Jason,

> > There is lots of optimization potential on a few fronts we've identified
> > in this thread. Let's save these for a follow-up. I'd rather this
> > initial one be at least somewhat simple, so that as it gets optimized,
> > it'll be easy to handle regressions. Also, it probably makes sense for
> > you to send the patches for these, since you have both the hardware
> > chops and the hardware itself to assess these ideas. I am interested in
> > the topic though, so please do CC me.
> 
> Everything has been upstream for a little while now, which means
> development of this can move back to the proper MIPS tree like normal.
> Did you want to submit some optimizations? Would be happy to look at
> whatever you have in mind.

 Thank you for the heads-up!

 Unfortunately I'm a little stuck at the moment, especially as one of my
main MIPS machines (a 5Kc Malta system) died mid-May while operating.  It 
seems to be a faulty CPU core card and the base board may be fine, though 
I cannot know for sure as I only have one each and I don't have a logic 
analyser or at least a JTAG probe to peek at the system and see what's 
going on inside.

 If anyone knows a source of a replacement Malta, preferably with a 5Kc 
CoreLV CPU module or another 64-bit hard core card (a number of different 
ones have been made), then I'll appreciate if you let me know.  I feel 
rather depressed knowing that many if not most hit the scrapper already 
while they could still find a good use.  Somehow it is easier to get way 
more obsolete hardware from 1980/90s just because it was general purpose 
rather than niche.

 Otherwise I'll try to get back to this stuff later in the year with 
whatever I have that still runs, but don't hold your breath.  Sorry!

  Maciej
Maciej W. Rozycki Sept. 25, 2022, 2:17 p.m. UTC | #5 
Hi Jason,

> >  Unfortunately I'm a little stuck at the moment, especially as one of my
> > main MIPS machines (a 5Kc Malta system) died mid-May while operating.  It 
> > seems to be a faulty CPU core card and the base board may be fine, though 
> > I cannot know for sure as I only have one each and I don't have a logic 
> > analyser or at least a JTAG probe to peek at the system and see what's 
> > going on inside.
> > 
> >  If anyone knows a source of a replacement Malta, preferably with a 5Kc 
> > CoreLV CPU module or another 64-bit hard core card (a number of different 
> > ones have been made), then I'll appreciate if you let me know.  I feel 
> > rather depressed knowing that many if not most hit the scrapper already 
> > while they could still find a good use.  Somehow it is easier to get way 
> > more obsolete hardware from 1980/90s just because it was general purpose 
> > rather than niche.
> > 
> >  Otherwise I'll try to get back to this stuff later in the year with 
> > whatever I have that still runs, but don't hold your breath.  Sorry!
> 
> Just thought I'd poke you about this, on the off chance that you found
> some new hardware and feel like tinkering around with cycle counters
> again. Some old MIPS platforms were recently dropped, too, which makes
> me wonder whether there's some room for more simplification here.

 I have a replacement CPU module now, but it is not the same as the old 
one was and it has a built-in different system controller (a MIPS SOC-it 
IP implementation) rather than a discrete Galileo GT-64120A chip the 
original module had.

 And the system controller suffers from PCI handling issues, i.e. the 
YAMON firmware hangs in PCI enumeration if there are more than 2 buses 
present (my configuration used to have 3) and in Linux one of the option 
cards seems not to respond to MMIO accesses (a mapping error?), with 
exactly the same hardware configuration that worked just fine with the 
Galileo.  I had to pull some hardware to make the system boot at all.

 It's not clear to me yet if these are symptoms of hardware errata or bugs 
in the respective pieces of software, but I fear debugging fatal issues 
has to take precedence over optimisations for me, so I have to defer cycle 
counter tinkering yet more.

 NB the replacement CPU module is also 32-bit rather than 64-bit as the 
broken one was and therefore I'm still looking for a replacement 64-bit 
CPU module.  Malta CPU modules seem exceedingly scarce nowadays, it seems 
easier to track down a 30 years old MIPS R3000 DECstation machine, sigh...

  Maciej
diff mbox series

Patch

diff --git a/arch/mips/include/asm/timex.h b/arch/mips/include/asm/timex.h
index b05bb70a2e46..8cfa485d19e6 100644
--- a/arch/mips/include/asm/timex.h
+++ b/arch/mips/include/asm/timex.h
@@ -76,25 +76,24 @@  static inline cycles_t get_cycles(void)
 	else
 		return 0;	/* no usable counter */
 }
+#define get_cycles get_cycles
 
 /*
  * Like get_cycles - but where c0_count is not available we desperately
  * use c0_random in an attempt to get at least a little bit of entropy.
- *
- * R6000 and R6000A neither have a count register nor a random register.
- * That leaves no entropy source in the CPU itself.
  */
 static inline unsigned long random_get_entropy(void)
 {
-	unsigned int prid = read_c0_prid();
-	unsigned int imp = prid & PRID_IMP_MASK;
+	unsigned int c0_random;
 
-	if (can_use_mips_counter(prid))
+	if (can_use_mips_counter(read_c0_prid()))
 		return read_c0_count();
-	else if (likely(imp != PRID_IMP_R6000 && imp != PRID_IMP_R6000A))
-		return read_c0_random();
+
+	if (cpu_has_3kex)
+		c0_random = (read_c0_random() >> 8) & 0x3f;
 	else
-		return 0;	/* no usable register */
+		c0_random = read_c0_random() & 0x3f;
+	return (random_get_entropy_fallback() << 6) | (0x3f - c0_random);
 }
 #define random_get_entropy random_get_entropy