Date: Sat, 30 Dec 2017 19:21:21 +0100 (CET)
From: Thomas Gleixner <tglx@linutronix.de>
To: Dave Hansen <dave.hansen@linux.intel.com>
cc: Dominik Brodowski <linux@dominikbrodowski.net>, luto@kernel.org,
        linux-kernel@vger.kernel.org
Subject: Re: x86/pti: smp_processor_id() called while preemptible in
 resume-from-sleep
In-Reply-To: <83e0e8b4-9873-b938-5e03-1bfe7ffbf3ff@linux.intel.com>
Message-ID: <alpine.DEB.2.20.1712301921040.1899@nanos>
References: <20171230132927.GA2731@light.dominikbrodowski.net> <alpine.DEB.2.20.1712301558500.1899@nanos> <20171230153054.GA1604@light.dominikbrodowski.net> <83e0e8b4-9873-b938-5e03-1bfe7ffbf3ff@linux.intel.com>
User-Agent: Alpine 2.20 (DEB 67 2015-01-07)
MIME-Version: 1.0
Content-Type: text/plain; charset=US-ASCII
Sender: linux-kernel-owner@vger.kernel.org
Content-Length: 2010
Lines: 46

On Sat, 30 Dec 2017, Dave Hansen wrote:
> On 12/30/2017 07:30 AM, Dominik Brodowski wrote:
> > 
> > native_cpu_up+0x447/0xa30:
> > kern_pcid at arch/x86/include/asm/tlbflush.h:105
> >  (inlined by) invalidate_user_asid at arch/x86/include/asm/tlbflush.h:342
> >  (inlined by) __native_flush_tlb at arch/x86/include/asm/tlbflush.h:351
> >  (inlined by) smpboot_restore_warm_reset_vector at arch/x86/kernel/smpboot.c:146
> >  (inlined by) do_boot_cpu at arch/x86/kernel/smpboot.c:1022
> >  (inlined by) native_cpu_up at arch/x86/kernel/smpboot.c:1070
> 
> This appears to be this path:
> 
> > static inline void smpboot_restore_warm_reset_vector(void)
> > {
> >         unsigned long flags;
> > 
> >         /*
> >          * Install writable page 0 entry to set BIOS data area.
> >          */
> >         local_flush_tlb();
> 
> The PTI code is now tracking when a given ASID needs to get flushed in a
> per-cpu variable, and we use smp_processor_id() in local_flush_tlb() to
> do that tracking.  That's the *proximate* cause of the new warning.  I
> think it's actually a quite valid warning that's catching something
> questionable.
> 
> I'm limited here by not knowing how the warm reset vector actually
> works, though.  I don't know why we TLB flush at all, much less why we
> do it after CMOS_WRITE() in the "setup" path but _before_ CMOS_WRITE()
> in the "restore" one.  Where do we actually "Install writable page 0
> entry to set BIOS data area"?  Shouldn't we just be flushing _there_?
> 
> But, even _doing_ a TLB flush with preempt enabled and interrupts on
> seems wrong to me.  It just fundamentally doesn't mean anything because
> it can theoretically run anywhere and flush *any* TLB.  There might be
> some other implicit preempt-thwarting going on here, but I can't find it.
> 
> The naive fix here is to just preempt_dis/enable() over the area doing
> the flush and the writes to the TRAMPOLINE_* area.  That'll definitely
> shut up the warnings.

Well, yes, but it makes no sense at all.