Re: PROBLEM: consolidated IDT invalidation causes kexec to reboot

[Alexandru Chirvasitu <achirvasub@gmail.com>]

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
> <torvalds@linux-foundation.org> wrote:
> > On Sat, Dec 23, 2017 at 5:44 PM, Alexandru Chirvasitu
> > <achirvasub@gmail.com> 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