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: Thomas Bogendoerfer <[email protected]>
Signed-off-by: Jason A. Donenfeld <[email protected]>
---
arch/mips/include/asm/timex.h | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/arch/mips/include/asm/timex.h b/arch/mips/include/asm/timex.h
index b05bb70a2e46..abc60a6395e3 100644
--- a/arch/mips/include/asm/timex.h
+++ b/arch/mips/include/asm/timex.h
@@ -94,7 +94,7 @@ static inline unsigned long random_get_entropy(void)
else if (likely(imp != PRID_IMP_R6000 && imp != PRID_IMP_R6000A))
return read_c0_random();
else
- return 0; /* no usable register */
+ return random_get_entropy_fallback(); /* no usable register */
}
#define random_get_entropy random_get_entropy
--
2.35.1
Hi Jason,
> However, one thing that I've been thinking about is that the c0 random
> register is actually kind of garbage. In my fuzzy decade-old memory of
> MIPS, I believe the c0 random register starts at the maximum number of
> TLB entries (16?), and then counts down cyclically, decrementing once
> per CPU cycle. Is that right?
Yes, for the relevant CPUs the range is 63-8 << 8 for R3k machines and
47-0 (the lower bound can be higher if wired entries are used, which I
think we occasionally do) for R4k machines with a buggy CP0 counter. So
there are either 56 or up to 48 distinct CP0 Random register values.
> If it is, there are some real pros and cons here to consider:
> - Pro: decrementing each CPU cycle means pretty good granularity
> - Con: wrapping at, like, 16 or something really is very limited, to
> the point of being almost bad
>
> Meanwhile, on systems without the c0 cycles counter, what is the
> actual resolution of random_get_entropy_fallback()? Is this just
> falling back to jiffies?
It depends on the exact system. Some have a 32-bit high-resolution
counter in the chipset (arch/mips/kernel/csrc-ioasic.c) giving like 25MHz
resolution, some have nothing but jiffies.
> IF (a) the fallback is jiffies AND (b) c0 wraps at 16, then actually,
> what would be really nice would be something like:
>
> return (jiffies << 4) | read_c0_random();
>
> It seems like that would give us something somewhat more ideal than
> the status quo. Still crap, of course, but undoubtedly better.
It seems like a reasonable idea to me, but the details would have to be
sorted out, because where a chipset high-resolution counter is available
we want to factor it in, and otherwise we need to extract the right bits
from the CP0 Random register, either 13:8 for the R3k or 5:0 for the R4k.
Maciej
Hi Maciej,
On Thu, Apr 14, 2022 at 02:16:18AM +0100, Maciej W. Rozycki wrote:
> Yes, for the relevant CPUs the range is 63-8 << 8 for R3k machines and
> 47-0 (the lower bound can be higher if wired entries are used, which I
> think we occasionally do) for R4k machines with a buggy CP0 counter. So
> there are either 56 or up to 48 distinct CP0 Random register values.
Ahh interesting, so it varies a bit, but it remains rather small.
> It depends on the exact system. Some have a 32-bit high-resolution
> counter in the chipset (arch/mips/kernel/csrc-ioasic.c) giving like 25MHz
> resolution, some have nothing but jiffies.
Alright, so there _are_ machines with no c0 cycles but with a good
clock. Yet, 25MHz is still less than the cpu cycle, so this c0 random
ORing trick remains useful perhaps.
> It seems like a reasonable idea to me, but the details would have to be
> sorted out, because where a chipset high-resolution counter is available
> we want to factor it in, and otherwise we need to extract the right bits
> from the CP0 Random register, either 13:8 for the R3k or 5:0 for the R4k.
One thing we could do here that would seemingly cover all the cases
without losing _that_ much would be:
return (random_get_entropy_fallback() << 13) | ((1<<13) - read_c0_random());
Or in case the 13 turns out to be wrong on some hardware, we could
mitigate the effect with:
return (random_get_entropy_fallback() << 13) ^ ((1<<13) - read_c0_random());
As mentioned in the 1/xx patch of this series,
random_get_entropy_fallback() should call the highest resolution thing.
We then shave off the least-changing bits and stuff in the
faster-changing bits from read_c0_random(). Then, in order to keep it
counting up instead of down, we do the subtraction there.
What do you think of this plan?
Jason
Hi Maciej,
On Wed, Apr 13, 2022 at 2:46 PM Maciej W. Rozycki <[email protected]> wrote:
>
> On Wed, 13 Apr 2022, Thomas Bogendoerfer wrote:
>
> > > diff --git a/arch/mips/include/asm/timex.h b/arch/mips/include/asm/timex.h
> > > index b05bb70a2e46..abc60a6395e3 100644
> > > --- a/arch/mips/include/asm/timex.h
> > > +++ b/arch/mips/include/asm/timex.h
> > > @@ -94,7 +94,7 @@ static inline unsigned long random_get_entropy(void)
> > > else if (likely(imp != PRID_IMP_R6000 && imp != PRID_IMP_R6000A))
> > > return read_c0_random();
> > > else
> > > - return 0; /* no usable register */
> > > + return random_get_entropy_fallback(); /* no usable register */
> > > }
> > > #define random_get_entropy random_get_entropy
> > >
> > > --
> > > 2.35.1
> >
> > Acked-by: Thomas Bogendoerfer <[email protected]>
>
> Or we could drop the PRID_IMP_R6000/A check and the final `else' clause
> entirely, as we don't even pretend to support the R6k at all anymore, and
> this is the final reference remaining. For one we no longer handle the
> CPU in `cpu_probe_legacy' so any attempt to boot on such a CPU would
> inevitably fail as no CPU options would be set (we probably should have a
> `panic' or suchlike as the default case for the switch statement there).
>
> Therefore I'm all for removing this piece instead, complementing commit
> 3b2db173f012 ("MIPS: Remove unused R6000 support"), where it should have
> really happened.
That's fine with me, if that's what Thomas wants to do, and I can
submit a v5 with that in it. Indeed, from our previous conversations,
I'm aware that the `else` there is probably never hit.
However, one thing that I've been thinking about is that the c0 random
register is actually kind of garbage. In my fuzzy decade-old memory of
MIPS, I believe the c0 random register starts at the maximum number of
TLB entries (16?), and then counts down cyclically, decrementing once
per CPU cycle. Is that right?
If it is, there are some real pros and cons here to consider:
- Pro: decrementing each CPU cycle means pretty good granularity
- Con: wrapping at, like, 16 or something really is very limited, to
the point of being almost bad
Meanwhile, on systems without the c0 cycles counter, what is the
actual resolution of random_get_entropy_fallback()? Is this just
falling back to jiffies?
IF (a) the fallback is jiffies AND (b) c0 wraps at 16, then actually,
what would be really nice would be something like:
return (jiffies << 4) | read_c0_random();
It seems like that would give us something somewhat more ideal than
the status quo. Still crap, of course, but undoubtedly better.
Unfortunately, I don't know enough about whether assumptions (a) and
(b) hold for all hardware that doesn't have the c0 cycle counter. Can
you or Thomas confirm/deny this? And if it is more nuanced than my
optimistic assumption above, can we think of some scheme together that
nicely combines jiffies or the fallback function with the c0 random
register that would be sensible?
Jason
Hi Jason,
> > There are two variants only of the CP0 Random register that we can ever
> > encounter, as it's been de-facto standardised in early 1990s already and
> > then written down in the MIPSr1 architecture specification ~2000. So I
> > think it may make sense to actually handle them both explictitly with
> > individual calculations, possibly conditionalised on a CONFIG setting or
> > `cpu_has_3kex', because kernels that support the two variants of the MMU
> > architecture are mutually incompatible.
>
> Okay, I can give this a shot, but this certainly isn't my forté. It
> may ultimately wind up being simpler for you to just send some code of
> what you envision for this, but if I understand your idea correctly,
> what you're saying is something like:
>
> 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))
> return read_c0_count();
>
> if (cpu_has_3kex)
> c0_random = (read_c0_random() >> 8) & 0x3f;
> else
> c0_random = read_c0_random() & 0x3f;
> return (random_get_entropy_fallback() << 6) | (0x3f - c0_random);
> }
>
> What do you think of that? Some tweak I'm missing?
It certainly looks good to me. Do you have a way I could verify how this
function performs? If so, then I could put it through my systems as I can
cover all the cases handled here.
Any improvements I previously discussed can then be made locally in the
MIPS port as follow-up changes.
> > Isn't it going to be an issue for an entropy source that the distribution
> > of values obtained from the CP0 Random bit-field is not even, that is some
> > values from the 6-bit range will never appear?
>
> It's the same situation without inverting the order: instead of some
> bits on the top never happening, some bits on the bottom never happen
> instead. In general, counters don't form uniform distributions anyway,
> since the lower bits change faster, and neither are they independent,
> since one sample in large part depends on the previous. This is just
> sort of the nature of the beast, and the code that calls
> random_get_entropy() deals with this appropriately (by, at the moment,
> just hashing all the bits).
OK then, thanks for your clarification.
Maciej
Hi Maciej,
On Sat, Apr 16, 2022 at 03:44:53PM +0100, Maciej W. Rozycki wrote:
> > Okay, I can give this a shot, but this certainly isn't my forté. It
> > may ultimately wind up being simpler for you to just send some code of
> > what you envision for this, but if I understand your idea correctly,
> > what you're saying is something like:
> >
> > 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))
> > return read_c0_count();
> >
> > if (cpu_has_3kex)
> > c0_random = (read_c0_random() >> 8) & 0x3f;
> > else
> > c0_random = read_c0_random() & 0x3f;
> > return (random_get_entropy_fallback() << 6) | (0x3f - c0_random);
> > }
> >
> > What do you think of that? Some tweak I'm missing?
>
> It certainly looks good to me. Do you have a way I could verify how this
> function performs? If so, then I could put it through my systems as I can
> cover all the cases handled here.
Oh, awesome about the test rig. Below is a little patch that adds some
printf code to init, calling the above sequence 70 times in a busy loop
and then 30 times after that with a scheduler 1 ms delay in there,
printing lots of various about the above calculation. Hopefully that's
enough information that it'll be possible to notice if something looks
really off when we squint at it.
Jason
-------------------8<-----------------------------------------------------
diff --git a/drivers/char/random.c b/drivers/char/random.c
index 3a293f919af9..0b32203aa47f 100644
--- a/drivers/char/random.c
+++ b/drivers/char/random.c
@@ -48,6 +48,7 @@
#include <linux/ptrace.h>
#include <linux/workqueue.h>
#include <linux/irq.h>
+#include <linux/delay.h>
#include <linux/ratelimit.h>
#include <linux/syscalls.h>
#include <linux/completion.h>
@@ -1781,6 +1782,26 @@ static struct ctl_table random_table[] = {
*/
static int __init random_sysctls_init(void)
{
+
+ int i;
+ for (i = 0; i < 100; ++i) {
+ if (can_use_mips_counter(read_c0_prid()))
+ pr_err("SARU %d c0 count: %08x\n", i, read_c0_count());
+ else {
+ unsigned int c0_random = read_c0_random(), c0_random_mask;
+ unsigned long fallback = random_get_entropy_fallback(), out;
+ if (cpu_has_3kex)
+ c0_random_mask = (c0_random >> 8) & 0x3f;
+ else
+ c0_random_mask = c0_random & 0x3f;
+ out = (fallback << 6) | (0x3f - c0_random_mask);
+ pr_err("SARU: %d (%08x >> n) & 0x3f = %08x, inverted = %08x, fallback = %08lx, fallback << 6 = %08lx, combined = %08lx\n",
+ i, c0_random, c0_random_mask, 0x3f - c0_random_mask, fallback, fallback << 6, out);
+ }
+ if (i > 70)
+ msleep(1);
+ }
+
register_sysctl_init("kernel/random", random_table);
return 0;
}
diff --git a/include/linux/timex.h b/include/linux/timex.h
index 5745c90c8800..3871b06bd302 100644
--- a/include/linux/timex.h
+++ b/include/linux/timex.h
@@ -62,6 +62,8 @@
#include <linux/types.h>
#include <linux/param.h>
+unsigned long random_get_entropy_fallback(void);
+
#include <asm/timex.h>
#ifndef random_get_entropy
@@ -74,8 +76,14 @@
*
* By default we use get_cycles() for this purpose, but individual
* architectures may override this in their asm/timex.h header file.
+ * If a given arch does not have get_cycles(), then we fallback to
+ * using random_get_entropy_fallback().
*/
+#ifdef get_cycles
#define random_get_entropy() ((unsigned long)get_cycles())
+#else
+#define random_get_entropy() random_get_entropy_fallback()
+#endif
#endif
/*
diff --git a/kernel/time/timekeeping.c b/kernel/time/timekeeping.c
index dcdcb85121e4..7cd2ec239cae 100644
--- a/kernel/time/timekeeping.c
+++ b/kernel/time/timekeeping.c
@@ -17,6 +17,7 @@
#include <linux/clocksource.h>
#include <linux/jiffies.h>
#include <linux/time.h>
+#include <linux/timex.h>
#include <linux/tick.h>
#include <linux/stop_machine.h>
#include <linux/pvclock_gtod.h>
@@ -2380,6 +2381,15 @@ static int timekeeping_validate_timex(const struct __kernel_timex *txc)
return 0;
}
+/**
+ * random_get_entropy_fallback - Returns the raw clock source value,
+ * used by random.c for platforms with no valid random_get_entropy().
+ */
+unsigned long random_get_entropy_fallback(void)
+{
+ return tk_clock_read(&tk_core.timekeeper.tkr_mono);
+}
+EXPORT_SYMBOL_GPL(random_get_entropy_fallback);
/**
* do_adjtimex() - Accessor function to NTP __do_adjtimex function
On Fri, Apr 15, 2022 at 01:26:48PM +0100, Maciej W. Rozycki wrote:
> Hi Jason,
>
> > > It depends on the exact system. Some have a 32-bit high-resolution
> > > counter in the chipset (arch/mips/kernel/csrc-ioasic.c) giving like 25MHz
> > > resolution, some have nothing but jiffies.
> >
> > Alright, so there _are_ machines with no c0 cycles but with a good
> > clock. Yet, 25MHz is still less than the cpu cycle, so this c0 random
> > ORing trick remains useful perhaps.
>
> It's not much less than the CPU cycle really, given that the R3k CPUs are
> clocked at up to 40MHz in the systems concerned and likewise the buggy R4k
> CPUs run at up to 60MHz (and mind that their CP0 Count register increments
> at half the clock rate, so the rate is up to 30MHz anyway). The overhead
> of the calculation is more than that, let alone the latency and issue rate
> of an uncached MMIO access to the chipset register.
>
> Also the systems I have in mind and that lack a counter in the chipset
> actually can make use of the buggy CP0 timer, because it's only when CP0
> timer interrupts are used that the erratum matters, but they use a DS1287
> RTC interrupt instead unconditionally as the clock event (see the comment
> at the bottom of arch/mips/dec/time.c). But this has not been factored in
> with `can_use_mips_counter' (should it just check for `mips_hpt_frequency'
> being zero perhaps, meaning the timer interrupt not being used?).
>
> Thomas, do you happen to know if any of the SGI systems that we support
> had buggy early R4k chips?
IP22 has probably seen all buggy MIPS chips produced, so yes I even own
Indy/Indigo2 CPU boards with early R4k chips.
Thomas.
--
Crap can work. Given enough thrust pigs will fly, but it's not necessarily a
good idea. [ RFC1925, 2.3 ]
On Mon, 18 Apr 2022, Thomas Bogendoerfer wrote:
> > Also the systems I have in mind and that lack a counter in the chipset
> > actually can make use of the buggy CP0 timer, because it's only when CP0
> > timer interrupts are used that the erratum matters, but they use a DS1287
> > RTC interrupt instead unconditionally as the clock event (see the comment
> > at the bottom of arch/mips/dec/time.c). But this has not been factored in
> > with `can_use_mips_counter' (should it just check for `mips_hpt_frequency'
> > being zero perhaps, meaning the timer interrupt not being used?).
> >
> > Thomas, do you happen to know if any of the SGI systems that we support
> > had buggy early R4k chips?
>
> IP22 has probably seen all buggy MIPS chips produced, so yes I even own
> Indy/Indigo2 CPU boards with early R4k chips.
Do they actually use the CP0 timer as a clock event device? Do they have
an alternative high-precision timer available?
In the course of verifying this change I have noticed my DECstation
5000/260, which has a high-precision timer in the chipset available as a
clock source device, does register the CP0 timer as a clock source device
regardless. Upon a closer inspection I have noticed that the CP0 timer
interrupt is non-functional in this machine, which I have then confirmed
as a valid CPU hardware configuration via the TimIntDis/TimerIntDis (the
R4k CPU manual is inconsistent in naming here) boot-mode bit. It allows
IP7 to be used as an external interrupt source instead. I used not to be
aware of the presence of this boot-mode bit.
I find this arrangement odd, because IP7 used to be wired internally as
the FPU interrupt with the 5000/240's CPU module, so it's not usable as an
external interrupt anyway with this system's mainboard.
That means however that this machine (and possibly the 5000/150 as well,
but I'll have to verify that once I get at the KN04 CPU module I have in a
drawer at my other place) can use the CP0 timer as a clock source device
unconditionally. I think this discovery asks for code optimisation, which
I'll try to cook up sometime.
I don't expect the IP22 to have a similar arrangement with the CP0 timer
interrupt given that the CPU was an in-house design at SGI, but who knows?
Do you?
Maciej
On Sun, Apr 24, 2022 at 12:33:44AM +0100, Maciej W. Rozycki wrote:
> unconditionally. I think this discovery asks for code optimisation, which
> I'll try to cook up sometime.
At some point too, by the way, we might also consider putting that into
a .c file rather than a static inline in the .h, since that function is
starting to get sort of big.
Jason
On Sun, 24 Apr 2022, Jason A. Donenfeld wrote:
> > unconditionally. I think this discovery asks for code optimisation, which
> > I'll try to cook up sometime.
>
> At some point too, by the way, we might also consider putting that into
> a .c file rather than a static inline in the .h, since that function is
> starting to get sort of big.
This code is supposed to produce one to a couple of machine instructions
for the majority of configurations. This is because the conditionals used
are usually compile-time constants. Therefore I think it will be good to
continue having it as `static inline' functions. Cf. the analysis in
commit 06947aaaf9bf ("MIPS: Implement random_get_entropy with CP0
Random").
If this code does expand to a longer sequence for some platforms, then
either they need to be verified whether they can be optimised (just as I
note here for the DEC systems) or we can consider making these functions
`extern inline' instead, with out-of-line code available from a .a file in
case the compiler decides the code is too large for inlining to be worth
doing after all. Though I don't expect the latter case to be required
really.
Maciej