[PATCH] x86/traps: Dump instruction stream in show_execution_state()

[PATCH] x86/traps: Dump instruction stream in show_execution_state()

[Andrew Cooper]

For first pass triage of crashes, it is useful to have the instruction
stream present, especially now that Xen binary patches itself.

A sample output now looks like:

(XEN) ----[ Xen-4.6-unstable  x86_64  debug=y  Not tainted ]----
(XEN) CPU:    0
(XEN) RIP:    e008:[<ffff82d0801607e4>] default_idle+0x76/0x7b
(XEN) RFLAGS: 0000000000000246   CONTEXT: hypervisor
(XEN) rax: ffff82d080331030   rbx: ffff83007fce8000   rcx: 0000000000000000
(XEN) rdx: 0000000000000000   rsi: ffff82d080331b98   rdi: 0000000000000000
(XEN) rbp: ffff83007fcefef0   rsp: ffff83007fcefef0   r8:  ffff83007faf8118
(XEN) r9:  00000009983e89fd   r10: 00000009983e89fd   r11: 0000000000000246
(XEN) r12: ffff83007fd61000   r13: 00000000ffffffff   r14: ffff83007fad9000
(XEN) r15: ffff83007fae3000   cr0: 000000008005003b   cr4: 00000000000026e0
(XEN) cr3: 000000007fc9b000   cr2: 00007f70976b3fed
(XEN) ds: 0000   es: 0000   fs: 0000   gs: 0000   ss: e010   cs: e008
(XEN) Xen code around e008:ffff82d0801607e4 (default_idle+0x76/0x7b):
(XEN)  83 3c 10 00 75 04 fb f4 <eb> 01 fb 5d c3 55 48 89 e5 3b 3d 0d 50 12 00 72
(XEN) Xen stack trace from rsp=ffff83007fcefef0:
(XEN)    ffff83007fceff10 ffff82d080160e08 ffff82d08012c40a ffff83007faf9000
(XEN)    ffff83007fcefdd8 ffffffff81a01fd8 ffff88002f07d4c0 ffffffff81a01fd8
(XEN)    0000000000000000 ffffffff81a01e58 ffffffff81a01fd8 0000000000000246
(XEN)    00000000ffff0052 0000000000000000 0000000000000000 0000000000000000
(XEN)    ffffffff810013aa 0000000000000001 00000000deadbeef 00000000deadbeef
(XEN)    0000010000000000 ffffffff810013aa 000000000000e033 0000000000000246
(XEN)    ffffffff81a01e40 000000000000e02b 0000000000000000 0000000000000000
(XEN)    0000000000000000 0000000000000000 0000000000000000 ffff83007faf9000
(XEN)    0000000000000000 0000000000000000
(XEN) Xen call trace:
(XEN)    [<ffff82d0801607e4>] default_idle+0x76/0x7b
(XEN)    [<ffff82d080160e08>] idle_loop+0x51/0x6e
(XEN)

Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
CC: Jan Beulich <JBeulich@suse.com>

---
Currently limited to just hypervisor context, but it could be extended
to vcpus as well.
---
 xen/arch/x86/traps.c |   26 ++++++++++++++++++++++++++
 1 file changed, 26 insertions(+)

diff --git a/xen/arch/x86/traps.c b/xen/arch/x86/traps.c
index 6a03582..2ee0d80 100644
--- a/xen/arch/x86/traps.c
+++ b/xen/arch/x86/traps.c
@@ -115,6 +115,31 @@
 #define stack_words_per_line 4
 #define ESP_BEFORE_EXCEPTION(regs) ((unsigned long *)regs->rsp)
 
+static void show_code(const struct cpu_user_regs *regs)
+{
+    char insns[24];
+    unsigned int i, not_copied;
+    void *__user start_ip = (void *)regs->rip - 8;
+
+    if ( guest_mode(regs) )
+        return;
+
+    not_copied = __copy_from_user(insns, start_ip, ARRAY_SIZE(insns));
+
+    printk("Xen code around %04x:%p (%ps)%s:\n",
+           regs->cs, _p(regs->rip), _p(regs->rip),
+           !!not_copied ? " [fault on access]" : "");
+
+    for ( i = 0; i < ARRAY_SIZE(insns) - not_copied; ++i )
+    {
+        if ( (unsigned long)(start_ip + i) == regs->rip )
+            printk(" <%02x>", (unsigned char)insns[i]);
+        else
+            printk(" %02x", (unsigned char)insns[i]);
+    }
+    printk("\n");
+}
+
 static void show_guest_stack(struct vcpu *v, const struct cpu_user_regs *regs)
 {
     int i;
@@ -418,6 +443,7 @@ void show_stack_overflow(unsigned int cpu, const struct cpu_user_regs *regs)
 void show_execution_state(const struct cpu_user_regs *regs)
 {
     show_registers(regs);
+    show_code(regs);
     show_stack(regs);
 }
 
-- 
1.7.10.4

Re: [PATCH] x86/traps: Dump instruction stream in show_execution_state()

[Jan Beulich]

>>> On 14.07.15 at 18:15, <andrew.cooper3@citrix.com> wrote:
> Currently limited to just hypervisor context, but it could be extended
> to vcpus as well.

Considering this ...

> --- a/xen/arch/x86/traps.c
> +++ b/xen/arch/x86/traps.c
> @@ -115,6 +115,31 @@
>  #define stack_words_per_line 4
>  #define ESP_BEFORE_EXCEPTION(regs) ((unsigned long *)regs->rsp)
>  
> +static void show_code(const struct cpu_user_regs *regs)
> +{
> +    char insns[24];
> +    unsigned int i, not_copied;
> +    void *__user start_ip = (void *)regs->rip - 8;
> +
> +    if ( guest_mode(regs) )
> +        return;
> +
> +    not_copied = __copy_from_user(insns, start_ip, ARRAY_SIZE(insns));
> +
> +    printk("Xen code around %04x:%p (%ps)%s:\n",

... I'd prefer the "Xen " here to be dropped.

> +           regs->cs, _p(regs->rip), _p(regs->rip),
> +           !!not_copied ? " [fault on access]" : "");

Pointless !!.

> +    for ( i = 0; i < ARRAY_SIZE(insns) - not_copied; ++i )
> +    {
> +        if ( (unsigned long)(start_ip + i) == regs->rip )
> +            printk(" <%02x>", (unsigned char)insns[i]);
> +        else
> +            printk(" %02x", (unsigned char)insns[i]);

Why not have insns[] be unsigned char right away?

Also I think you should avoid the subtraction from regs->rip to wrap
through zero, or even bail when RIP doesn't point into Xen space.

Jan

Re: [PATCH] x86/traps: Dump instruction stream in show_execution_state()

[Andrew Cooper]

On 15/07/15 09:53, Jan Beulich wrote:
>>>> On 14.07.15 at 18:15, <andrew.cooper3@citrix.com> wrote:
>> Currently limited to just hypervisor context, but it could be extended
>> to vcpus as well.
> Considering this ...
>
>> --- a/xen/arch/x86/traps.c
>> +++ b/xen/arch/x86/traps.c
>> @@ -115,6 +115,31 @@
>>  #define stack_words_per_line 4
>>  #define ESP_BEFORE_EXCEPTION(regs) ((unsigned long *)regs->rsp)
>>  
>> +static void show_code(const struct cpu_user_regs *regs)
>> +{
>> +    char insns[24];
>> +    unsigned int i, not_copied;
>> +    void *__user start_ip = (void *)regs->rip - 8;
>> +
>> +    if ( guest_mode(regs) )
>> +        return;
>> +
>> +    not_copied = __copy_from_user(insns, start_ip, ARRAY_SIZE(insns));
>> +
>> +    printk("Xen code around %04x:%p (%ps)%s:\n",
> ... I'd prefer the "Xen " here to be dropped.

This particular bit of code might be trivially reused for PV vcpus, but
not for HVM.  The %p and %ps make the printk Xen-specific, and I was
following the prevaling layout of "Xen stack trace" and "Xen call trace"

In the case of a vcpu, I was considering a show_guest_code() similar to
show_guest_stack(), breaking off at the guest_mode(regs) check.

>
>> +           regs->cs, _p(regs->rip), _p(regs->rip),
>> +           !!not_copied ? " [fault on access]" : "");
> Pointless !!.
>
>> +    for ( i = 0; i < ARRAY_SIZE(insns) - not_copied; ++i )
>> +    {
>> +        if ( (unsigned long)(start_ip + i) == regs->rip )
>> +            printk(" <%02x>", (unsigned char)insns[i]);
>> +        else
>> +            printk(" %02x", (unsigned char)insns[i]);
> Why not have insns[] be unsigned char right away?

I really should have done.

>
> Also I think you should avoid the subtraction from regs->rip to wrap
> through zero, or even bail when RIP doesn't point into Xen space.

If the instruction stream under eip is accessible, it should be printed,
even if it doesn't point into Xen space.  Bear in mind that anything
could have gone wrong by the point we get here; we may have accidentally
jumped into userspace or jumped into some data.

The wrapping through zero will be caught by the error handling in
__copy_from_user(), but I admit that it is not very obvious.  The
information will be available based on the numeric value of eip.

~Andrew

Re: [PATCH] x86/traps: Dump instruction stream in show_execution_state()

[Jan Beulich]

>>> On 15.07.15 at 11:26, <andrew.cooper3@citrix.com> wrote:
> On 15/07/15 09:53, Jan Beulich wrote:
>> Also I think you should avoid the subtraction from regs->rip to wrap
>> through zero, or even bail when RIP doesn't point into Xen space.
> 
> If the instruction stream under eip is accessible, it should be printed,
> even if it doesn't point into Xen space.  Bear in mind that anything
> could have gone wrong by the point we get here; we may have accidentally
> jumped into userspace or jumped into some data.

In which case that fact (seen by RIP itself being off) is enough to
know what happened. What exact instruction caused the fault is
then of no interest anymore.

> The wrapping through zero will be caught by the error handling in
> __copy_from_user(), but I admit that it is not very obvious.  The
> information will be available based on the numeric value of eip.

No, by passing the wrapped pointer to __coppy_from_user() you
will get the non-interesting bytes (if any) printed, but not the one
RIP actually points to.

Jan