| Message ID | 20220413115411.21489-5-Jason@zx2c4.com |
|---|---|
| State | New |
| Headers | show |
| Series | archs/random: fallback to best raw ktime when no cycle counter | expand |
On Wed, Apr 13, 2022 at 01:54:04PM +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: Thomas Bogendoerfer <tsbogend@alpha.franken.de> > Signed-off-by: Jason A. Donenfeld <Jason@zx2c4.com> > --- > 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 Acked-by: Thomas Bogendoerfer <tsbogend@alpha.franken.de>
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 <tsbogend@alpha.franken.de> 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. Maciej
Hi Maciej, On Wed, Apr 13, 2022 at 2:46 PM Maciej W. Rozycki <macro@orcam.me.uk> 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 <tsbogend@alpha.franken.de> > > 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, > 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 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? > > 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()); Except this would have to be: return (random_get_entropy_fallback() << 14) | ((1<<14) - read_c0_random()); of course, as bit 13 is still one of the active ones in the R3k CP0 Random register. > 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()); 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. Ah, there's that buggy non-compliant JZ4740 chip too. I guess we can figure out how many CP0 Random bits it implements, though it may be worth noting that architecturally the register is not required to decrement, so again it may be good to double-check how the JZ4740 selects the values there. I think the check for a buggy CP0 timer in `can_use_mips_counter' should also be qualified with !(CONFIG_CPU_MIPS32 || CONFIG_CPU_MIPS64), which will reduce the function to a constant 1 for the overwhelming majority of systems out there, without a need to refer to CP0 PRId every time. > 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. 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? > What do you think of this plan? Otherwise it makes absolute sense to me. Maciej
Hi Maciej, On Fri, Apr 15, 2022 at 2:26 PM Maciej W. Rozycki <macro@orcam.me.uk> wrote: > return (random_get_entropy_fallback() << 14) | ((1<<14) - read_c0_random()); > > of course, as bit 13 is still one of the active ones in the R3k CP0 Random > register. Ah, thanks, will do that. > 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? > 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). 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.
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
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
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: Thomas Bogendoerfer <tsbogend@alpha.franken.de> Signed-off-by: Jason A. Donenfeld <Jason@zx2c4.com> --- arch/mips/include/asm/timex.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)