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

[Andrew Cooper <andrew.cooper3@citrix.com>]

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