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

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 14.07.15 at 18:15, <andrew.cooper3@xxxxxxxxxx> 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


_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel