Re: [PATCH v6 13/17] x86: use fallback for random_get_entropy() instead of zero

["Jason A. Donenfeld" <Jason@zx2c4.com>]

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