Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1753174AbdLYV2D (ORCPT ); Mon, 25 Dec 2017 16:28:03 -0500 Received: from mail-qt0-f169.google.com ([209.85.216.169]:36017 "EHLO mail-qt0-f169.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1753034AbdLYV2B (ORCPT ); Mon, 25 Dec 2017 16:28:01 -0500 X-Google-Smtp-Source: ACJfBovlGTvDPj8YOQIkF242dCj50y72twDpBw1yLE3nuJnVI3zDN4CY7pmrIOigM+AnboCJn5DUgg== Date: Mon, 25 Dec 2017 16:29:34 -0500 From: Alexandru Chirvasitu To: Andy Lutomirski Cc: Linus Torvalds , Thomas Gleixner , kernel list , Borislav Petkov , Brian Gerst , Denys Vlasenko , "H. Peter Anvin" , Josh Poimboeuf , Peter Zijlstra , Steven Rostedt , Ingo Molnar Subject: Re: PROBLEM: consolidated IDT invalidation causes kexec to reboot Message-ID: <20171225212934.GA1410@arch-chirva.localdomain> References: <20171224014415.GA5663@chirva-void> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: User-Agent: Mutt/1.9.2 (2017-12-15) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 3152 Lines: 99 Thanks for that! On Mon, Dec 25, 2017 at 06:40:14AM -0800, Andy Lutomirski wrote: > On Sat, Dec 23, 2017 at 7:30 PM, Linus Torvalds > wrote: > > On Sat, Dec 23, 2017 at 5:44 PM, Alexandru Chirvasitu > > wrote: > >> > >> For testing purposes, I've altered machine_kexec_32.c making the > >> following toy commit. It naively undoes part of e802a51, solely to > >> confirm that's where it goes awry in my setup. > > > > That's really funky. > > > > The idt_invalidate() seems to do *exactly* the same thing. It uses > > "load_idt()" on an IDT with size 0, and the supplied address. > > > > Can you disassemble your "set_idt()" code vs the "idt_invalidate()"? > > > >> Is this expected behaviour? > > > > No. The code literally seems identical. The only difference is > > > > (a) where the 0 limit comes from > > > > (b) perhaps build flags and whether it is inlined or not due to being > > in a different file > > > > and neither of those should matter, but maybe they do. > > > > Which is why I'd like you to actually look at the generated code and > > see if you can see any difference.. > > > > This is presumably the same call-tracing-without-TLS-working problem. > idt_invalidate() is out-of-line and is compiled with full tracing on, > and we're calling it from a context without TLS working (it's > explicitly disabled in load_segments()) in machine_kexec_32.c. The > right fix is probably to inline idt_invalidate() and to add a comment. > This works.. I went back to the troublesome commit e802a51 and modified it as follows: -------------------------------------------------------- make idt_invalidate static inline in header file diff --git a/arch/x86/include/asm/desc.h b/arch/x86/include/asm/desc.h index 33aff45..87ca363 100644 --- a/arch/x86/include/asm/desc.h +++ b/arch/x86/include/asm/desc.h @@ -504,6 +504,17 @@ static inline void load_current_idt(void) load_idt((const struct desc_ptr *)&idt_descr); } -extern void idt_invalidate(void *addr); + +/** + * idt_invalidate - Invalidate interrupt descriptor table + * @addr: The virtual address of the 'invalid' IDT + */ +static inline void idt_invalidate(void *addr) +{ + struct desc_ptr idt = { .address = (unsigned long) addr, .size = 0 }; + + load_idt(&idt); +} + #endif /* _ASM_X86_DESC_H */ diff --git a/arch/x86/kernel/idt.c b/arch/x86/kernel/idt.c index cd4658c..fec44c6 100644 --- a/arch/x86/kernel/idt.c +++ b/arch/x86/kernel/idt.c @@ -25,13 +25,3 @@ const struct desc_ptr debug_idt_descr = { }; #endif -/** - * idt_invalidate - Invalidate interrupt descriptor table - * @addr: The virtual address of the 'invalid' IDT - */ -void idt_invalidate(void *addr) -{ - struct desc_ptr idt = { .address = (unsigned long) addr, .size = 0 }; - - load_idt(&idt); -} -------------------------------------------------------- kexec now works as expected; tested repeatedly, both with direct execution and crash triggering. I had to google 'inline function' :)). > Also, why idt_invalidate(phys_to_virt(0))? That makes rather little > sense to me. > > --Andy