Re: [PATCH v4 26/30] x86,tlb: Make __flush_tlb_global() noinstr-compliant

[Valentin Schneider <vschneid@xxxxxxxxxx>]

On 14/01/25 13:45, Dave Hansen wrote:
> On 1/14/25 09:51, Valentin Schneider wrote:
>> +    cr4 = this_cpu_read(cpu_tlbstate.cr4);
>> +    asm volatile("mov %0,%%cr4": : "r" (cr4 ^ X86_CR4_PGE) : "memory");
>> +    asm volatile("mov %0,%%cr4": : "r" (cr4) : "memory");
>> +    /*
>> +     * In lieu of not having the pinning crap, hard fail if CR4 doesn't
>> +     * match the expected value. This ensures that anybody doing dodgy gets
>> +     * the fallthrough check.
>> +     */
>> +    BUG_ON(cr4 != this_cpu_read(cpu_tlbstate.cr4));
>
> Let's say someone managed to write to cpu_tlbstate.cr4 where they
> cleared one of the pinned bits.
>
> Before this patch, CR4 pinning would WARN_ONCE() about it pretty quickly
> and also reset the cleared bits.
>
> After this patch, the first native_flush_tlb_global() can clear pinned
> bits, at least until native_write_cr4() gets called the next time. That
> seems like it'll undermine CR4 pinning at least somewhat.
>

The BUG_ON() should still catch any pinned bit mishandling, however...

> What keeps native_write_cr4() from being noinstr-compliant now? Is it
> just the WARN_ONCE()?
>

I don't think that's even an issue since __WARN_printf() wraps the print in
instrumentation_{begin,end}(). In v3 I made native_write_cr4() noinstr and
added a non-noinstr wrapper to be used in existing callsites.

AFAICT if acceptable we could make the whole thing noinstr and stick with
that; Peter, is there something I missed that made you write the handmade
noinstr CR4 RMW?