Re: lguest: unhandled trap 13 and CONFIG_MICROCODE_INTEL_EARLY

[Rusty Russell <rusty-8n+1lVoiYb80n/F98K4Iww@public.gmane.org>]

Paul Bolle <pebolle-IWqWACnzNjzz+pZb47iToQ@public.gmane.org> writes:
> 0) Since v3.9 launching a guest kernel with the lguest tool triggers an
> "unhandled trap 13" error if CONFIG_MICROCODE_INTEL_EARLY is set (and
> one runs on an Intel CPU). That option was introduced in v3.9.

Yech... me too:

lguest: unhandled trap 13 at 0x537b8d (0x0)

> 1) For instance, on qemu I ran into this error:
>     lguest: unhandled trap 13 at 0x97087d (0x0)
>
> 2) Disassembling that address (with the page offset added) in vmlinux
> (in gdb) showed:
>
> Dump of assembler code for function collect_cpu_info_early:
>    0xc09707e4 <+0>:     push   %ebp
>    [...]
>    0xc097087d <+153>:   wrmsr  
>    [...]
>    0xc09708a5 <+193>:   ret    
> End of assembler dump.
>
> 3) The internet tells me wrmsr will trigger a general protection fault
> if the CPU is not running in ring 0. And a guest kernel seems to do that
> since v3.9:
>
> arch/x86/kernel/head_32.S:call load_ucode_bsp
>      arch/x86/kernel/microcode_core_early.c:load_ucode_bsp()
>          arch/x86/kernel/microcode_intel_early.c:_load_ucode_intel_bsp()
>                                                      collect_cpu_info_early()

Yes, there's also the known issue that CONFIG_OLPC breaks lguest boot.

HPA, how about we extend the KEEP_SEGMENTS flag to mean "don't touch
privileged registers" in general?  That's what it used to do when it
was introduced, and AFAIK only lguest uses it.  Xen folk CC'd.

%ebx seems to be free, how about something like this?

Subject: x86: treat KEEP_SEGMENTS flag as advice not to try any privileged operations

The i386 boot path used to only do one privileged thing before entering the
per-platform boot-path: load segments.  Thus KEEP_SEGMENTS was born, used
by lguest (and could be used by others like Xen).

CONFIG_OLPC and CONFIG_MICROCODE_EARLY were added since then, so with those
options, lguest guest crash.  I've been telling people to turn them off, but
that's a bit naff.

Expand KEEP_SEGMENTS to mean "don't try to do ring0 stuff" and it does the
right thing.

Signed-off-by: Rusty Russell <rusty-8n+1lVoiYb80n/F98K4Iww@public.gmane.org>

diff --git a/arch/x86/kernel/head_32.S b/arch/x86/kernel/head_32.S
index 73afd11..8b50829 100644
--- a/arch/x86/kernel/head_32.S
+++ b/arch/x86/kernel/head_32.S
@@ -89,8 +89,9 @@ ENTRY(startup_32)
 	movl pa(stack_start),%ecx
 	
 	/* test KEEP_SEGMENTS flag to see if the bootloader is asking
-		us to not reload segments */
-	testb $(1<<6), BP_loadflags(%esi)
+		us to not reload segments: keep in %ebx. */
+	movb BP_loadflags(%esi),%ebx
+	andb $(1<<6),%ebx
 	jnz 2f
 
 /*
@@ -138,6 +139,9 @@ ENTRY(startup_32)
 	movsl
 1:
 
+	/* KEEP_SEGMENTS implies we don't touch priv regs at all. */
+	andb %ebx,%ebx
+	jnz 2f
 #ifdef CONFIG_OLPC
 	/* save OFW's pgdir table for later use when calling into OFW */
 	movl %cr3, %eax
@@ -149,6 +153,7 @@ ENTRY(startup_32)
 	call load_ucode_bsp
 #endif
 
+2:
 /*
  * Initialize page tables.  This creates a PDE and a set of page
  * tables, which are located immediately beyond __brk_base.  The variable