Messages in this thread | | | Date | Thu, 7 Sep 2017 11:54:37 +0200 | From | Borislav Petkov <> | Subject | Re: [PATCH 1/2] x86/mm: Reinitialize TLB state on hotplug and resume |
| |
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 <torvalds@linux-foundation.org> > Reported-by: Jiri Kosina <jikos@kernel.org> > Fixes: 10af6235e0d3 ("x86/mm: Implement PCID based optimization: try to preserve old TLB entries using PCID") > Signed-off-by: Andy Lutomirski <luto@kernel.org> > --- > 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) --
| |