Andrew Cooper
2013-Sep-09 14:17 UTC
[PATCH] xen/x86: Introduce early_invalid_op() handler.
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
Jan Beulich
2013-Sep-09 14:30 UTC
Re: [PATCH] xen/x86: Introduce early_invalid_op() handler.
>>> 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(); > +}
Keir Fraser
2013-Sep-09 14:37 UTC
Re: [PATCH] xen/x86: Introduce early_invalid_op() handler.
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(); >> +} > >
Andrew Cooper
2013-Sep-09 14:43 UTC
Re: [PATCH] xen/x86: Introduce early_invalid_op() handler.
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. > > -- KeirOk - 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