From mboxrd@z Thu Jan 1 00:00:00 1970 From: Andrew Cooper Subject: Re: [PATCH] x86/traps: Dump instruction stream in show_execution_state() Date: Wed, 15 Jul 2015 10:26:15 +0100 Message-ID: <55A62737.4010900@citrix.com> References: <1436890502-11426-1-git-send-email-andrew.cooper3@citrix.com> <55A63BB20200007800091276@mail.emea.novell.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: <55A63BB20200007800091276@mail.emea.novell.com> List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Sender: xen-devel-bounces@lists.xen.org Errors-To: xen-devel-bounces@lists.xen.org To: Jan Beulich Cc: Xen-devel List-Id: xen-devel@lists.xenproject.org On 15/07/15 09:53, Jan Beulich wrote: >>>> On 14.07.15 at 18:15, 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