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.
If CONFIG_X86_TSC=n, then it's possible that we're running on a 486 with
no RDTSC, so we only need the fallback code for that case. While we're
at it, fix up both the new function and the get_cycles() function from
which its derived to use cpu_feature_enabled() rather than boot_cpu_has().
Cc: Thomas Gleixner <[email protected]>
Cc: Arnd Bergmann <[email protected]>
Cc: Borislav Petkov <[email protected]>
Cc: [email protected]
Signed-off-by: Jason A. Donenfeld <[email protected]>
---
arch/x86/include/asm/timex.h | 10 ++++++++++
arch/x86/include/asm/tsc.h | 4 ++--
2 files changed, 12 insertions(+), 2 deletions(-)
diff --git a/arch/x86/include/asm/timex.h b/arch/x86/include/asm/timex.h
index a4a8b1b16c0c..2b841c39d876 100644
--- a/arch/x86/include/asm/timex.h
+++ b/arch/x86/include/asm/timex.h
@@ -5,6 +5,16 @@
#include <asm/processor.h>
#include <asm/tsc.h>
+static inline unsigned long random_get_entropy(void)
+{
+#ifndef CONFIG_X86_TSC
+ if (!cpu_feature_enabled(X86_FEATURE_TSC))
+ return random_get_entropy_fallback();
+#endif
+ return rdtsc();
+}
+#define random_get_entropy random_get_entropy
+
/* Assume we use the PIT time source for the clock tick */
#define CLOCK_TICK_RATE PIT_TICK_RATE
diff --git a/arch/x86/include/asm/tsc.h b/arch/x86/include/asm/tsc.h
index 01a300a9700b..192ea48ec80b 100644
--- a/arch/x86/include/asm/tsc.h
+++ b/arch/x86/include/asm/tsc.h
@@ -21,12 +21,12 @@ extern void disable_TSC(void);
static inline cycles_t get_cycles(void)
{
#ifndef CONFIG_X86_TSC
- if (!boot_cpu_has(X86_FEATURE_TSC))
+ if (!cpu_feature_enabled(X86_FEATURE_TSC))
return 0;
#endif
-
return rdtsc();
}
+#define get_cycles get_cycles
extern struct system_counterval_t convert_art_to_tsc(u64 art);
extern struct system_counterval_t convert_art_ns_to_tsc(u64 art_ns);
--
2.35.1
On Mon, Apr 25, 2022 at 03:41:00PM +0200, Jason A. Donenfeld wrote:
> .text:0000000000001A70 add_interrupt_randomness proc near
> .text:0000000000001A70 movsxd rcx, edi
> .text:0000000000001A73 rdtsc
> .text:0000000000001A75 shl rdx, 20h
> .text:0000000000001A79 mov rdi, [rsp+0]
> .text:0000000000001A7D or rax, rdx
> .text:0000000000001A80 mov rdx, offset irq_randomness
> .text:0000000000001A87 mov rsi, gs:__irq_regs
> .text:0000000000001A8F add rdx, gs:this_cpu_off
For context, here's what your suggested change looks like:
.text:0000000000001AF0 add_interrupt_randomness proc near
.text:0000000000001AF0 push rbx
.text:0000000000001AF1 movsxd rbx, edi
.text:0000000000001AF4 jmp loc_25AA
.text:0000000000001AF9 loc_1AF9: ; CODE XREF: add_interrupt_randomness+AC1↓j
.text:0000000000001AF9 rdtsc
.text:0000000000001AFB shl rdx, 20h
.text:0000000000001AFF or rax, rdx
.text:0000000000001B02
.text:0000000000001B02 loc_1B02: ; CODE XREF: add_interrupt_randomness+12C↓j
.text:0000000000001B02 mov rdx, offset irq_randomness
.text:0000000000001B09 mov rcx, gs:__irq_regs
.text:0000000000001B11 add rdx, gs:this_cpu_off
[...]
.altinstr_aux:00000000000025AA loc_25AA: ; CODE XREF: add_interrupt_randomness+4↑j
.altinstr_aux:00000000000025AA test byte ptr cs:boot_cpu_data+20h, 10h
.altinstr_aux:00000000000025B1 jnz loc_1AF9
.altinstr_aux:00000000000025B7 jmp loc_1C17
So an extra push, and then that jmp gets nop'd out (I hope). Plus the
altinstr area. I guess that's not awful, but I fail to see what we
actually gain here over the existing code, which doesn't bother messing
with this at runtime when the kernel builder opts out. The ifdef has
always been there; what's the reason for adding it now in 2022?
Virtualization cannot be it.
Jason
On Sat, Apr 23 2022 at 23:26, Jason A. Donenfeld wrote:
Please follow the guidelines of the tip maintainers when touching x86
code. https://www.kernel.org/doc/html/latest/process/maintainer-tip.html#patch-subject
> 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.
>
> If CONFIG_X86_TSC=n, then it's possible that we're running on a 486
> with no RDTSC, so we only need the fallback code for that case.
There are also 586 CPUs which lack TSC.
> While we're at it, fix up both the new function and the get_cycles()
> function from which its derived to use cpu_feature_enabled() rather
> than boot_cpu_has().
Lot's of 'we' here. We are not doing anything.
https://www.kernel.org/doc/html/latest/process/maintainer-tip.html#changelog
> +static inline unsigned long random_get_entropy(void)
> +{
> +#ifndef CONFIG_X86_TSC
> + if (!cpu_feature_enabled(X86_FEATURE_TSC))
> + return random_get_entropy_fallback();
> +#endif
Please get rid of this ifdeffery. While you are right, that anything
with CONFIG_X86_TSC=y should have a TSC, there is virt ....
cpu_feature_enabled() is runtime patched and only evaluated before
alternative patching, so the win of this ifdef is marginally, if even
noticable.
We surely can think about making TSC mandatory, but not selectively in a
particalur context.
Thanks,
tglx
Hi Thomas,
On Mon, Apr 25, 2022 at 02:35:39PM +0200, Thomas Gleixner wrote:
> On Sat, Apr 23 2022 at 23:26, Jason A. Donenfeld wrote:
>
> Please follow the guidelines of the tip maintainers when touching x86
> code. https://www.kernel.org/doc/html/latest/process/maintainer-tip.html#patch-subject
> https://www.kernel.org/doc/html/latest/process/maintainer-tip.html#changelog
Will fix up the message and topic.
> > 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.
> >
> > If CONFIG_X86_TSC=n, then it's possible that we're running on a 486
> > with no RDTSC, so we only need the fallback code for that case.
>
> There are also 586 CPUs which lack TSC.
Will note this in the rewritten commit message.
> > +static inline unsigned long random_get_entropy(void)
> > +{
> > +#ifndef CONFIG_X86_TSC
> > + if (!cpu_feature_enabled(X86_FEATURE_TSC))
> > + return random_get_entropy_fallback();
> > +#endif
>
> Please get rid of this ifdeffery. While you are right, that anything
> with CONFIG_X86_TSC=y should have a TSC, there is virt ....
>
> cpu_feature_enabled() is runtime patched and only evaluated before
> alternative patching, so the win of this ifdef is marginally, if even
> noticable.
>
> We surely can think about making TSC mandatory, but not selectively in a
> particalur context.
This would be a regression of sorts from the current code, which reads:
static inline cycles_t get_cycles(void)
{
#ifndef CONFIG_X86_TSC
if (!boot_cpu_has(X86_FEATURE_TSC))
return 0;
#endif
return rdtsc();
}
So my ifdef is just copying that one. I can make it into a `if
(IS_ENABLED(CONFIG_X86_TSC))` thing, if you'd prefer that. But on
systems where CONFIG_X86_TSC=n, including the extra code there seems
kind of undesirable. Consider the current interrupt handler random.c
code on x86, whose first lines (usually) compile to:
.text:0000000000001A70 add_interrupt_randomness proc near
.text:0000000000001A70 movsxd rcx, edi
.text:0000000000001A73 rdtsc
.text:0000000000001A75 shl rdx, 20h
.text:0000000000001A79 mov rdi, [rsp+0]
.text:0000000000001A7D or rax, rdx
.text:0000000000001A80 mov rdx, offset irq_randomness
.text:0000000000001A87 mov rsi, gs:__irq_regs
.text:0000000000001A8F add rdx, gs:this_cpu_off
I'm not sure what advantage we'd get by changing that from the ifdefs to
unconditionally including that if statement. Your argument about virt
can't be valid, since the current get_cycles() code uses the ifdefs. And
if you're arguing from the basis that CONFIG_X86_TSC=y is not reliable,
then why does CONFIG_X86_TSC even exist in the first place?
Rather the stronger argument you could make would be that moving from
boot_cpu_has() to cpu_feature_enabled() (Borislav's suggestion) means
that there's now a static branch here, so the actual cost to the
assembly is a few meaningless nops, which you don't think would play a
part in quantizing when rdtsc is called. If this is actually what you
have in mind, and you find that ifdef ugly enough that this would be
worth it, I can understand, and I'll make that change for v7. But I
don't understand you to currently be making that argument, so I'm not
quite sure what your position is.
Could you clarify what you mean here and what your expectations are? And
how does that point of view tie into the fact that get_cycles()
currently uses the ifdef?
Thanks,
Jason
On Mon, Apr 25 2022 at 15:41, Jason A. Donenfeld wrote:
> On Mon, Apr 25, 2022 at 02:35:39PM +0200, Thomas Gleixner wrote:
>> > +static inline unsigned long random_get_entropy(void)
>> > +{
>> > +#ifndef CONFIG_X86_TSC
>> > + if (!cpu_feature_enabled(X86_FEATURE_TSC))
>> > + return random_get_entropy_fallback();
>> > +#endif
>>
>> Please get rid of this ifdeffery. While you are right, that anything
>> with CONFIG_X86_TSC=y should have a TSC, there is virt ....
>>
>> cpu_feature_enabled() is runtime patched and only evaluated before
>> alternative patching, so the win of this ifdef is marginally, if even
>> noticable.
>>
>> We surely can think about making TSC mandatory, but not selectively in a
>> particalur context.
>
> This would be a regression of sorts from the current code, which reads:
>
> static inline cycles_t get_cycles(void)
> {
> #ifndef CONFIG_X86_TSC
> if (!boot_cpu_has(X86_FEATURE_TSC))
> return 0;
> #endif
> return rdtsc();
> }
Bah. Indeed. Misread the patch, but yes, if you are at it to make that
cpu_feature_enabled() then change the config thing to IS_ENABLED() too.
Thanks,
tglx
Hey Thomas,
On Mon, Apr 25, 2022 at 07:03:54PM +0200, Thomas Gleixner wrote:
> Bah. Indeed. Misread the patch, but yes, if you are at it to make that
> cpu_feature_enabled() then change the config thing to IS_ENABLED() too.
Alright, will do for v7. I also just confirmed the !IS_ENABLED() way
produces the same code as the #ifndef.
Jason
In the event that random_get_entropy() can't access a cycle counter or
similar, falling back to returning 0 is suboptimal. Instead, fallback
to calling random_get_entropy_fallback(), which isn't extremely high
precision or guaranteed to be entropic, but is certainly better than
returning zero all the time.
If CONFIG_X86_TSC=n, then it's possible for the kernel to run on systems
without RDTSC, such as 486 and certain 586, so the fallback code is only
required for that case.
As well, fix up both the new function and the get_cycles() function from
which it was derived to use cpu_feature_enabled() rather than
boot_cpu_has(), and use !IS_ENABLED() instead of #ifndef.
Cc: Thomas Gleixner <[email protected]>
Cc: Arnd Bergmann <[email protected]>
Cc: Borislav Petkov <[email protected]>
Cc: [email protected]
Signed-off-by: Jason A. Donenfeld <[email protected]>
---
Changes v6->v7:
- Adjust commit subject and body to match tip commit style.
- Use !IS_ENABLED() instead of #ifndef.
arch/x86/include/asm/timex.h | 9 +++++++++
arch/x86/include/asm/tsc.h | 7 +++----
2 files changed, 12 insertions(+), 4 deletions(-)
diff --git a/arch/x86/include/asm/timex.h b/arch/x86/include/asm/timex.h
index a4a8b1b16c0c..956e4145311b 100644
--- a/arch/x86/include/asm/timex.h
+++ b/arch/x86/include/asm/timex.h
@@ -5,6 +5,15 @@
#include <asm/processor.h>
#include <asm/tsc.h>
+static inline unsigned long random_get_entropy(void)
+{
+ if (!IS_ENABLED(CONFIG_X86_TSC) &&
+ !cpu_feature_enabled(X86_FEATURE_TSC))
+ return random_get_entropy_fallback();
+ return rdtsc();
+}
+#define random_get_entropy random_get_entropy
+
/* Assume we use the PIT time source for the clock tick */
#define CLOCK_TICK_RATE PIT_TICK_RATE
diff --git a/arch/x86/include/asm/tsc.h b/arch/x86/include/asm/tsc.h
index 01a300a9700b..fbdc3d951494 100644
--- a/arch/x86/include/asm/tsc.h
+++ b/arch/x86/include/asm/tsc.h
@@ -20,13 +20,12 @@ extern void disable_TSC(void);
static inline cycles_t get_cycles(void)
{
-#ifndef CONFIG_X86_TSC
- if (!boot_cpu_has(X86_FEATURE_TSC))
+ if (!IS_ENABLED(CONFIG_X86_TSC) &&
+ !cpu_feature_enabled(X86_FEATURE_TSC))
return 0;
-#endif
-
return rdtsc();
}
+#define get_cycles get_cycles
extern struct system_counterval_t convert_art_to_tsc(u64 art);
extern struct system_counterval_t convert_art_ns_to_tsc(u64 art_ns);
--
2.35.1