[PATCH] xen/x86: Introduce early_invalid_op() handler.

[PATCH] xen/x86: Introduce early_invalid_op() handler.

[Andrew Cooper]

As discovered when debugging the problem eventually identified and fixed in
c/s 0fbf3208d9c1, bugframes from early code were being caught by the
ignore_int handler with the following printed to the console.

Unknown interrupt (cr2=0000000000000000)

This changeset adds an early_invalid_op() handler alongside early_page_fault()
which can can identify a ud2 instruction under the fault eip, and suggesting
that a developer might want to look around the indicated address.

While doing this, I noticed that there is a window where the
"BUG_ON(smp_processor_id() != 0);" can spuriously fail.  To compensate, move
the "set_processor_id(0)" and friends earlier, as it is safe to do so.

Furthermore, make use of halt() inside the forever loop, to be slightly more
power-friendly.

Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
CC: Keir Fraser <keir@xen.org>
CC: Jan Beulich <JBeulich@suse.com>
---
 xen/arch/x86/setup.c        |    7 ++++---
 xen/arch/x86/traps.c        |   20 +++++++++++++++++++-
 xen/arch/x86/x86_64/entry.S |    7 +++++++
 xen/include/asm-x86/setup.h |    1 +
 4 files changed, 31 insertions(+), 4 deletions(-)

diff --git a/xen/arch/x86/setup.c b/xen/arch/x86/setup.c
index c550e8e..67dd2dc 100644
--- a/xen/arch/x86/setup.c
+++ b/xen/arch/x86/setup.c
@@ -558,8 +558,12 @@ void __init __start_xen(unsigned long mbi_p)
     };
 
     percpu_init_areas();
+    set_current((struct vcpu *)0xfffff000); /* debug sanity */
+    idle_vcpu[0] = current;
+    set_processor_id(0); /* needed early, for smp_processor_id() */
 
     set_intr_gate(TRAP_page_fault, &early_page_fault);
+    set_intr_gate(TRAP_invalid_op, &early_invalid_op);
 
     loader = (mbi->flags & MBI_LOADERNAME)
         ? (char *)__va(mbi->boot_loader_name) : "unknown";
@@ -587,9 +591,6 @@ void __init __start_xen(unsigned long mbi_p)
 
     parse_video_info();
 
-    set_current((struct vcpu *)0xfffff000); /* debug sanity */
-    idle_vcpu[0] = current;
-    set_processor_id(0); /* needed early, for smp_processor_id() */
     if ( cpu_has_efer )
         rdmsrl(MSR_EFER, this_cpu(efer));
     asm volatile ( "mov %%cr4,%0" : "=r" (this_cpu(cr4)) );
diff --git a/xen/arch/x86/traps.c b/xen/arch/x86/traps.c
index 9db42c82..61e91b1 100644
--- a/xen/arch/x86/traps.c
+++ b/xen/arch/x86/traps.c
@@ -1431,10 +1431,28 @@ void __init do_early_page_fault(struct cpu_user_regs *regs)
         printk("Stack dump: ");
         while ( ((long)stk & ((PAGE_SIZE - 1) & ~(BYTES_PER_LONG - 1))) != 0 )
             printk("%p ", _p(*stk++));
-        for ( ; ; ) ;
+        for ( ; ; )
+            halt();
     }
 }
 
+/*
+ * Early #UD handler capable of identifying bugframes before the real
+ * invalid_op handler is installed.  Otherwise they get caught and reported
+ * incorrectly by ingore_int
+ */
+void __init  __attribute__((noreturn))
+do_early_invalid_op(struct cpu_user_regs *regs)
+{
+    if ( *(u16 *)regs->eip == 0x0b0f )
+        printk("Early ud2 at %p - BUG/WARN/ASSERT perhaps?\n", _p(regs->eip));
+    else
+        printk("Unidentified early #UD at %p\n", _p(regs->eip));
+
+    for ( ; ; )
+        halt();
+}
+
 long do_fpu_taskswitch(int set)
 {
     struct vcpu *v = current;
diff --git a/xen/arch/x86/x86_64/entry.S b/xen/arch/x86/x86_64/entry.S
index f64e871..56ad158 100644
--- a/xen/arch/x86/x86_64/entry.S
+++ b/xen/arch/x86/x86_64/entry.S
@@ -619,6 +619,13 @@ ENTRY(early_page_fault)
         movq  %rsp,%rdi
         call  do_early_page_fault
         jmp   restore_all_xen
+
+ENTRY(early_invalid_op)
+        pushq $0
+        movl  $TRAP_invalid_op,4(%rsp)
+        SAVE_ALL
+        movq  %rsp,%rdi
+        jmp   do_early_invalid_op
         .popsection
 
 ENTRY(nmi)
diff --git a/xen/include/asm-x86/setup.h b/xen/include/asm-x86/setup.h
index 3039e1b..f818fb3 100644
--- a/xen/include/asm-x86/setup.h
+++ b/xen/include/asm-x86/setup.h
@@ -8,6 +8,7 @@ extern unsigned long xenheap_initial_phys_start;
 void early_cpu_init(void);
 void early_time_init(void);
 void early_page_fault(void);
+void early_invalid_op(void);
 
 int intel_cpu_init(void);
 int amd_init_cpu(void);
-- 
1.7.10.4

Re: [PATCH] xen/x86: Introduce early_invalid_op() handler.

[Jan Beulich]

>>> On 09.09.13 at 16:17, Andrew Cooper <andrew.cooper3@citrix.com> wrote:
> +void __init  __attribute__((noreturn))
> +do_early_invalid_op(struct cpu_user_regs *regs)
> +{
> +    if ( *(u16 *)regs->eip == 0x0b0f )

Without even a range check on regs->eip? I don't think we want to
needlessly risk #PF or #GP here...

> +        printk("Early ud2 at %p - BUG/WARN/ASSERT perhaps?\n", _p(regs->eip));
> +    else
> +        printk("Unidentified early #UD at %p\n", _p(regs->eip));
> +

You probably also meant to at least print the same raw stack
dump that do_early_page_fault() produces?

Jan

> +    for ( ; ; )
> +        halt();
> +}

Re: [PATCH] xen/x86: Introduce early_invalid_op() handler.

[Keir Fraser]

On 09/09/2013 07:30, "Jan Beulich" <JBeulich@suse.com> wrote:

>>>> On 09.09.13 at 16:17, Andrew Cooper <andrew.cooper3@citrix.com> wrote:
>> +void __init  __attribute__((noreturn))
>> +do_early_invalid_op(struct cpu_user_regs *regs)
>> +{
>> +    if ( *(u16 *)regs->eip == 0x0b0f )
> 
> Without even a range check on regs->eip? I don't think we want to
> needlessly risk #PF or #GP here...
> 
>> +        printk("Early ud2 at %p - BUG/WARN/ASSERT perhaps?\n",
>> _p(regs->eip));
>> +    else
>> +        printk("Unidentified early #UD at %p\n", _p(regs->eip));
>> +
> 
> You probably also meant to at least print the same raw stack
> dump that do_early_page_fault() produces?

I suggest less cleverness in this printk and indeed dump regs and error
code. More useful, potentially. Also then the handler will not be
UD-specific and could be called for all early exceptions (except those with
a more specific handler such as #PG).

All that would be needed in asm is a per-exception push/mov and jmp to
common asm which does the SAVE_ALL stuff and jmp to C.

 -- Keir

> Jan
> 
>> +    for ( ; ; )
>> +        halt();
>> +}
> 
> 

Re: [PATCH] xen/x86: Introduce early_invalid_op() handler.

[Andrew Cooper]

On 09/09/13 15:37, Keir Fraser wrote:
> On 09/09/2013 07:30, "Jan Beulich" <JBeulich@suse.com> wrote:
>
>>>>> On 09.09.13 at 16:17, Andrew Cooper <andrew.cooper3@citrix.com> wrote:
>>> +void __init  __attribute__((noreturn))
>>> +do_early_invalid_op(struct cpu_user_regs *regs)
>>> +{
>>> +    if ( *(u16 *)regs->eip == 0x0b0f )
>> Without even a range check on regs->eip? I don't think we want to
>> needlessly risk #PF or #GP here...
>>
>>> +        printk("Early ud2 at %p - BUG/WARN/ASSERT perhaps?\n",
>>> _p(regs->eip));
>>> +    else
>>> +        printk("Unidentified early #UD at %p\n", _p(regs->eip));
>>> +
>> You probably also meant to at least print the same raw stack
>> dump that do_early_page_fault() produces?
> I suggest less cleverness in this printk and indeed dump regs and error
> code. More useful, potentially. Also then the handler will not be
> UD-specific and could be called for all early exceptions (except those with
> a more specific handler such as #PG).
>
> All that would be needed in asm is a per-exception push/mov and jmp to
> common asm which does the SAVE_ALL stuff and jmp to C.
>
>  -- Keir

Ok - I will see about implementing this.

~Andrew

>
>> Jan
>>
>>> +    for ( ; ; )
>>> +        halt();
>>> +}
>>
>
>
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xen.org
> http://lists.xen.org/xen-devel