Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1754151AbdIGJyu (ORCPT ); Thu, 7 Sep 2017 05:54:50 -0400 Received: from mx2.suse.de ([195.135.220.15]:39093 "EHLO mx1.suse.de" rhost-flags-OK-OK-OK-FAIL) by vger.kernel.org with ESMTP id S1752315AbdIGJyt (ORCPT ); Thu, 7 Sep 2017 05:54:49 -0400 Date: Thu, 7 Sep 2017 11:54:37 +0200 From: Borislav Petkov To: Andy Lutomirski Cc: X86 ML , "linux-kernel@vger.kernel.org" , Linus Torvalds Subject: Re: [PATCH 1/2] x86/mm: Reinitialize TLB state on hotplug and resume Message-ID: <20170907095437.fs53v7w4smhsbd6m@pd.tnic> References: MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline Content-Transfer-Encoding: 8bit In-Reply-To: User-Agent: NeoMutt/20170113 (1.7.2) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 3547 Lines: 95 Just nitpicks: On Wed, Sep 06, 2017 at 07:54:53PM -0700, Andy Lutomirski wrote: > When Linux brings a CPU down and back up, it switches to init_mm and then > loads swapper_pg_dir into CR3. With PCID enabled, this has the side effect > of masking off the ASID bits in CR3. > > This can result in some confusion in the TLB handling code. If we > bring a CPU down and back up with any ASID other than 0, we end up > with the wrong ASID active on the CPU after resume. This could > cause our internal state to become corrupt, although major > corruption is unlikely because init_mm doesn't have any user pages. > More obviously, if CONFIG_DEBUG_VM=y, we'll trip over an assertion > in the next context switch. The result of *that* is a failure to > resume from suspend with probability 1 - 1/6^(cpus-1). > > Fix it by reinitializing cpu_tlbstate on resume and CPU bringup. > > Reported-by: Linus Torvalds > Reported-by: Jiri Kosina > Fixes: 10af6235e0d3 ("x86/mm: Implement PCID based optimization: try to preserve old TLB entries using PCID") > Signed-off-by: Andy Lutomirski > --- > arch/x86/include/asm/tlbflush.h | 2 ++ > arch/x86/kernel/cpu/common.c | 2 ++ > arch/x86/mm/tlb.c | 44 +++++++++++++++++++++++++++++++++++++++++ > arch/x86/power/cpu.c | 1 + > 4 files changed, 49 insertions(+) > > diff --git a/arch/x86/include/asm/tlbflush.h b/arch/x86/include/asm/tlbflush.h > index d23e61dc0640..4893abf7f74f 100644 > --- a/arch/x86/include/asm/tlbflush.h > +++ b/arch/x86/include/asm/tlbflush.h > @@ -198,6 +198,8 @@ static inline void cr4_set_bits_and_update_boot(unsigned long mask) > cr4_set_bits(mask); > } > > +extern void initialize_tlbstate_and_flush(void); Let's put that declaration at the end. > static inline void __native_flush_tlb(void) > { > /* > diff --git a/arch/x86/kernel/cpu/common.c b/arch/x86/kernel/cpu/common.c > index efba8e3da3e2..40cb4d0a5982 100644 > --- a/arch/x86/kernel/cpu/common.c > +++ b/arch/x86/kernel/cpu/common.c > @@ -1583,6 +1583,7 @@ void cpu_init(void) > mmgrab(&init_mm); > me->active_mm = &init_mm; > BUG_ON(me->mm); > + initialize_tlbstate_and_flush(); > enter_lazy_tlb(&init_mm, me); > > load_sp0(t, ¤t->thread); > @@ -1637,6 +1638,7 @@ void cpu_init(void) > mmgrab(&init_mm); > curr->active_mm = &init_mm; > BUG_ON(curr->mm); > + initialize_tlbstate_and_flush(); > enter_lazy_tlb(&init_mm, curr); > > load_sp0(t, thread); > diff --git a/arch/x86/mm/tlb.c b/arch/x86/mm/tlb.c > index ce104b962a17..dbbcfd59726a 100644 > --- a/arch/x86/mm/tlb.c > +++ b/arch/x86/mm/tlb.c > @@ -214,6 +214,50 @@ void switch_mm_irqs_off(struct mm_struct *prev, struct mm_struct *next, > } > > /* > + * Call this when reinitializing a CPU. It fixes the following potential > + * problems: > + * > + * - The ASID changed from what cpu_tlbstate thinks it is (most likely > + * because the CPU was taken down and came back up with CR3's PCID > + * bits clear. CPU hotplug can do this. > + * > + * - The TLB contains junk in slots corresponding to inactive ASIDs. > + * > + * - The CPU went so far out to lunch that it may have missed a TLB > + * flush. > + */ > +void initialize_tlbstate_and_flush(void) I think we should prefix all those visible, TLB-handling functions with "tlb_". So you'd have tlb_init_state_and_flush(). -- Regards/Gruss, Boris. SUSE Linux GmbH, GF: Felix Imendörffer, Jane Smithard, Graham Norton, HRB 21284 (AG Nürnberg) --