[lkml]   [2022]   [Apr]   [25]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
Messages in this thread
SubjectRe: [PATCH v6 13/17] x86: use fallback for random_get_entropy() instead of zero
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.

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;
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?


 \ /
  Last update: 2022-04-25 15:42    [W:0.065 / U:6.332 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site