Add the initialization table for the early trap setup and replace the early
trap init code.
Signed-off-by: Thomas Gleixner <[email protected]>
---
arch/x86/kernel/idt.c | 52 ++++++++++++++++++++++++++++++++++++++++++++++++
arch/x86/kernel/setup.c | 4 +--
arch/x86/kernel/traps.c | 27 ------------------------
3 files changed, 54 insertions(+), 29 deletions(-)
--- a/arch/x86/kernel/idt.c
+++ b/arch/x86/kernel/idt.c
@@ -48,6 +48,28 @@ struct idt_data {
#define TSKG(_vector, _gdt) \
G(_vector, NULL, DEFAULT_STACK, GATE_TASK, DPL0, _gdt << 3)
+/*
+ * Early traps running on the DEFAULT_STACK because the other interrupt
+ * stacks work only after cpu_init().
+ */
+static const __initdata struct idt_data early_idts[] = {
+ INTG(X86_TRAP_DB, debug),
+ SYSG(X86_TRAP_BP, int3),
+#ifdef CONFIG_X86_32
+ INTG(X86_TRAP_PF, page_fault),
+#endif
+};
+
+#ifdef CONFIG_X86_64
+/*
+ * Early traps running on the DEFAULT_STACK because the other interrupt
+ * stacks work only after cpu_init().
+ */
+static const __initdata struct idt_data early_pf_idts[] = {
+ INTG(X86_TRAP_PF, page_fault),
+};
+#endif
+
/* Must be page-aligned because the real IDT is used in a fixmap. */
gate_desc idt_table[IDT_ENTRIES] __page_aligned_bss;
@@ -93,6 +115,36 @@ idt_setup_from_table(gate_desc *idt, con
}
/**
+ * idt_setup_early_traps - Initialize the idt table with early traps
+ *
+ * On X8664 these traps do not use interrupt stacks as they can't work
+ * before cpu_init() is invoked and sets up TSS. The IST variants are
+ * installed after that.
+ */
+void __init idt_setup_early_traps(void)
+{
+ idt_setup_from_table(idt_table, early_idts, ARRAY_SIZE(early_idts));
+}
+
+#ifdef CONFIG_X86_64
+/**
+ * idt_setup_early_pf - Initialize the idt table with early pagefault handler
+ *
+ * On X8664 this does not use interrupt stacks as they can't work before
+ * cpu_init() is invoked and sets up TSS. The IST variant is installed
+ * after that.
+ *
+ * FIXME: Why is 32bit and 64bit installing the PF handler at different
+ * places in the early setup code?
+ */
+void __init idt_setup_early_pf(void)
+{
+ idt_setup_from_table(idt_table, early_pf_idts,
+ ARRAY_SIZE(early_pf_idts));
+}
+#endif
+
+/**
* idt_setup_early_handler - Initializes the idt table with early handlers
*/
void __init idt_setup_early_handler(void)
--- a/arch/x86/kernel/setup.c
+++ b/arch/x86/kernel/setup.c
@@ -891,7 +891,7 @@ void __init setup_arch(char **cmdline_p)
*/
olpc_ofw_detect();
- early_trap_init();
+ idt_setup_early_traps();
early_cpu_init();
early_ioremap_init();
@@ -1162,7 +1162,7 @@ void __init setup_arch(char **cmdline_p)
init_mem_mapping();
- early_trap_pf_init();
+ idt_setup_early_pf();
/*
* Update mmu_cr4_features (and, indirectly, trampoline_cr4_features)
--- a/arch/x86/kernel/traps.c
+++ b/arch/x86/kernel/traps.c
@@ -923,33 +923,6 @@ dotraplinkage void do_iret_error(struct
}
#endif
-/* Set of traps needed for early debugging. */
-void __init early_trap_init(void)
-{
- /*
- * Don't use IST to set DEBUG_STACK as it doesn't work until TSS
- * is ready in cpu_init() <-- trap_init(). Before trap_init(),
- * CPU runs at ring 0 so it is impossible to hit an invalid
- * stack. Using the original stack works well enough at this
- * early stage. DEBUG_STACK will be equipped after cpu_init() in
- * trap_init().
- */
- set_intr_gate(X86_TRAP_DB, debug);
- /* int3 can be called from all */
- set_system_intr_gate(X86_TRAP_BP, &int3);
-#ifdef CONFIG_X86_32
- set_intr_gate(X86_TRAP_PF, page_fault);
-#endif
- load_idt(&idt_descr);
-}
-
-void __init early_trap_pf_init(void)
-{
-#ifdef CONFIG_X86_64
- set_intr_gate(X86_TRAP_PF, page_fault);
-#endif
-}
-
void __init trap_init(void)
{
int i;
On 08/25/2017 05:47 PM, Thomas Gleixner wrote:
>
> -/* Set of traps needed for early debugging. */
> -void __init early_trap_init(void)
> -{
> - /*
> - * Don't use IST to set DEBUG_STACK as it doesn't work until TSS
> - * is ready in cpu_init() <-- trap_init(). Before trap_init(),
> - * CPU runs at ring 0 so it is impossible to hit an invalid
> - * stack. Using the original stack works well enough at this
> - * early stage. DEBUG_STACK will be equipped after cpu_init() in
> - * trap_init().
> - */
> - set_intr_gate(X86_TRAP_DB, debug);
> - /* int3 can be called from all */
> - set_system_intr_gate(X86_TRAP_BP, &int3);
> -#ifdef CONFIG_X86_32
> - set_intr_gate(X86_TRAP_PF, page_fault);
> -#endif
> - load_idt(&idt_descr);
This breaks 64-bit Xen PV guests: idt_setup_early_handler() is called
from x86_64_start_kernel(), which is not executed on those guests.
The patch below fixes it.
-boris
diff --git a/arch/x86/xen/enlighten_pv.c b/arch/x86/xen/enlighten_pv.c
index 03fb07d..ed0fb8a 100644
--- a/arch/x86/xen/enlighten_pv.c
+++ b/arch/x86/xen/enlighten_pv.c
@@ -1374,6 +1374,7 @@ asmlinkage __visible void __init
xen_start_kernel(void)
i386_start_kernel();
#else
cr4_init_shadow(); /* 32b kernel does this in i386_start_kernel() */
+ load_idt(&idt_descr);
x86_64_start_reservations((char *)__pa_symbol(&boot_params));
#endif
}
On Fri, 25 Aug 2017, Boris Ostrovsky wrote:
> On 08/25/2017 05:47 PM, Thomas Gleixner wrote:
> >
> > -/* Set of traps needed for early debugging. */
> > -void __init early_trap_init(void)
> > -{
> > - /*
> > - * Don't use IST to set DEBUG_STACK as it doesn't work until TSS
> > - * is ready in cpu_init() <-- trap_init(). Before trap_init(),
> > - * CPU runs at ring 0 so it is impossible to hit an invalid
> > - * stack. Using the original stack works well enough at this
> > - * early stage. DEBUG_STACK will be equipped after cpu_init() in
> > - * trap_init().
> > - */
> > - set_intr_gate(X86_TRAP_DB, debug);
> > - /* int3 can be called from all */
> > - set_system_intr_gate(X86_TRAP_BP, &int3);
> > -#ifdef CONFIG_X86_32
> > - set_intr_gate(X86_TRAP_PF, page_fault);
> > -#endif
> > - load_idt(&idt_descr);
>
> This breaks 64-bit Xen PV guests: idt_setup_early_handler() is called
> from x86_64_start_kernel(), which is not executed on those guests.
early_trap_init() is called from setup_arch(), not from
x86_64_start_kernel().
This patch merily renames the function and moves the code into a different
source file. So it's not changing the behaviour in any way.
> The patch below fixes it.
I doubt that :)
Thanks,
tglx
> diff --git a/arch/x86/xen/enlighten_pv.c b/arch/x86/xen/enlighten_pv.c
> index 03fb07d..ed0fb8a 100644
> --- a/arch/x86/xen/enlighten_pv.c
> +++ b/arch/x86/xen/enlighten_pv.c
> @@ -1374,6 +1374,7 @@ asmlinkage __visible void __init
> xen_start_kernel(void)
> i386_start_kernel();
> #else
> cr4_init_shadow(); /* 32b kernel does this in i386_start_kernel() */
> + load_idt(&idt_descr);
> x86_64_start_reservations((char *)__pa_symbol(&boot_params));
> #endif
> }
On 26/08/17 10:16, Thomas Gleixner wrote:
> On Fri, 25 Aug 2017, Boris Ostrovsky wrote:
>> On 08/25/2017 05:47 PM, Thomas Gleixner wrote:
>>>
>>> -/* Set of traps needed for early debugging. */
>>> -void __init early_trap_init(void)
>>> -{
>>> - /*
>>> - * Don't use IST to set DEBUG_STACK as it doesn't work until TSS
>>> - * is ready in cpu_init() <-- trap_init(). Before trap_init(),
>>> - * CPU runs at ring 0 so it is impossible to hit an invalid
>>> - * stack. Using the original stack works well enough at this
>>> - * early stage. DEBUG_STACK will be equipped after cpu_init() in
>>> - * trap_init().
>>> - */
>>> - set_intr_gate(X86_TRAP_DB, debug);
>>> - /* int3 can be called from all */
>>> - set_system_intr_gate(X86_TRAP_BP, &int3);
>>> -#ifdef CONFIG_X86_32
>>> - set_intr_gate(X86_TRAP_PF, page_fault);
>>> -#endif
>>> - load_idt(&idt_descr);
>>
>> This breaks 64-bit Xen PV guests: idt_setup_early_handler() is called
>> from x86_64_start_kernel(), which is not executed on those guests.
>
> early_trap_init() is called from setup_arch(), not from
> x86_64_start_kernel().
>
> This patch merily renames the function and moves the code into a different
> source file. So it's not changing the behaviour in any way.
It does: the IDT entries are now statically defined. Before that any
call of set_intr_gate() would have registered the trap handler with
the hypervisor, now this is done only when the IDT is being activated.
Boris' patch is doing that.
>
>> The patch below fixes it.
>
> I doubt that :)
Me not. :-)
Juergen
>
> Thanks,
>
> tglx
>
>> diff --git a/arch/x86/xen/enlighten_pv.c b/arch/x86/xen/enlighten_pv.c
>> index 03fb07d..ed0fb8a 100644
>> --- a/arch/x86/xen/enlighten_pv.c
>> +++ b/arch/x86/xen/enlighten_pv.c
>> @@ -1374,6 +1374,7 @@ asmlinkage __visible void __init
>> xen_start_kernel(void)
>> i386_start_kernel();
>> #else
>> cr4_init_shadow(); /* 32b kernel does this in i386_start_kernel() */
>> + load_idt(&idt_descr);
>> x86_64_start_reservations((char *)__pa_symbol(&boot_params));
>> #endif
>> }
>
>
On Sat, 26 Aug 2017, Juergen Gross wrote:
> On 26/08/17 10:16, Thomas Gleixner wrote:
> > early_trap_init() is called from setup_arch(), not from
> > x86_64_start_kernel().
> >
> > This patch merily renames the function and moves the code into a different
> > source file. So it's not changing the behaviour in any way.
>
> It does: the IDT entries are now statically defined. Before that any
> call of set_intr_gate() would have registered the trap handler with
> the hypervisor, now this is done only when the IDT is being activated.
> Boris' patch is doing that.
The IDT entries are not statically defined, bcause that's impossible as
that would require the linker to split the address into bits and pieces and
then it would require relocation entries which do the same split again.
+void __init idt_setup_early_traps(void)
+{
+ idt_setup_from_table(idt_table, early_idts, ARRAY_SIZE(early_idts));
+}
It sets up the IDT entries from the table instead of having a gazillion
calls to set_intr_gate() in the code. So that ends up in write_idt_entry()
which is paravirtualized and ends up where it ended up before.
What occured to me right now, is that the patch removes:
load_idt(&idt_descr);
from the original function without adding it to the new one. So that needs
to be fixed, but not in the XEN code. It simply wants to be added to
idt_setup_early_traps(). I'll send out a V3 of that particular patch.
Thanks,
tglx
On 08/26/2017 09:05 AM, Thomas Gleixner wrote:
>
> What occured to me right now, is that the patch removes:
>
> load_idt(&idt_descr);
>
> from the original function without adding it to the new one.
Right, that's exactly what was broken for Xen.
> So that needs
> to be fixed, but not in the XEN code. It simply wants to be added to
> idt_setup_early_traps(). I'll send out a V3 of that particular patch.
Yes, that will work too (and keep original logic).
-boris