From mboxrd@z Thu Jan 1 00:00:00 1970 From: Andrew Cooper Subject: Re: [PATCH 2/2] Xen/x86: Improve information from domain_crash_synchronous Date: Thu, 5 Sep 2013 10:57:20 +0100 Message-ID: <52285580.8000109@citrix.com> References: <1378318727-29518-1-git-send-email-andrew.cooper3@citrix.com> <1378318727-29518-3-git-send-email-andrew.cooper3@citrix.com> <522859C402000078000F0AF8@nat28.tlf.novell.com> <52285287.2070208@citrix.com> <5228704902000078000F0BC0@nat28.tlf.novell.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: Received: from mail6.bemta5.messagelabs.com ([195.245.231.135]) by lists.xen.org with esmtp (Exim 4.72) (envelope-from ) id 1VHWUf-0003wa-0Z for xen-devel@lists.xenproject.org; Thu, 05 Sep 2013 10:08:53 +0000 In-Reply-To: <5228704902000078000F0BC0@nat28.tlf.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 , Keir Fraser List-Id: xen-devel@lists.xenproject.org On 05/09/13 10:51, Jan Beulich wrote: >>>> On 05.09.13 at 11:44, Andrew Cooper wrote: >> On 05/09/13 09:15, Jan Beulich wrote: >>>>>> On 04.09.13 at 20:18, Andrew Cooper wrote: >>>> As it currently stands, the string "domain_crash_sync called from entry.S" >> is >>>> not helpful at identifying why the domain was crashed, and a debug build of >>>> Xen doesn't help the matter >>>> >>>> This patch improves the information printed, by pointing to where the crash >>>> decision was made. >>> Looks quite useful. >>> >>>> --- a/xen/arch/x86/x86_64/compat/entry.S >>>> +++ b/xen/arch/x86/x86_64/compat/entry.S >>>> @@ -263,6 +263,7 @@ ENTRY(compat_int80_direct_trap) >>>> /* {[ERRCODE,] EIP, CS, EFLAGS, [ESP, SS]} */ >>>> /* %rdx: trap_bounce, %rbx: struct vcpu */ >>>> /* On return only %rbx and %rdx are guaranteed non-clobbered. */ >>>> +.globl compat_create_bounce_frame >>>> compat_create_bounce_frame: >>> Is the addition above a left-over? I don't see any use of the >>> label outside of this file. >> This is for the benifit of print_symbol(), which will now give >> create_bounce_frame()+some rather than trap_nop()+loads. > That shouldn't be happening - whether a symbol is local or global > should not matter to symbol table generation and consumption. > The matter would be different is the label started with .L... Hmm - this was caught by my testing. I had initially assumed that print_symbol() would DTRT, but it didn't. Perhaps it is been fed off the global symbol table rather than the debug symbol table. > >>>> @@ -329,7 +330,12 @@ UNLIKELY_END(compat_bounce_failsafe) >>>> movzwl TRAPBOUNCE_cs(%rdx),%eax >>>> /* Null selectors (0-3) are not allowed. */ >>>> testl $~3,%eax >>>> - jz domain_crash_synchronous >>>> +.Lcompat_bounce_null_selector: >>>> +UNLIKELY_START(z, compat_bounce_null_selector) >>>> + lea .Lcompat_bounce_null_selector(%rip), %rdi >>>> + jmp asm_domain_crash_synchronous >>>> + ud2a >>>> +UNLIKELY_END(compat_bounce_null_selector) >>> Here and further down you don't really need the label at the >>> start of the unlikely section - the place can as well be identified >>> by using >>> >>> lea (%rip), %rdi >>> >>> inside that section (the place is still unique, just outside the >>> original code stream, i.e. just slightly more difficult to >>> re-associate). >> But in an unlikely section, %rip is shifted quite a lot from %rip of the >> code immediately before. This is also for the benefit of print_symbol() >> which will pick up the {compat_,}create_bounce_frame rather than the >> global symbol surrounding the unlikely section. > I understand that, but stray labels are at clear risk of getting > deleted by a subsequent cleanup patch anyway. Hence either > we need a solution without stray labels, or live with the need > to re-associate the address pointed to be the crash log > messages to the original function. > > Jan > That was eluded to in my patch 0 (perhaps not well enough), where I intend to augment UNLIKELY_START() to automatically generate this symbol, and provide an __UNLIKLEY_ENTRY_SYM() accessor. The code for that was rather tangled with your UNLIKELY_DONE() patch, which is why I left it and was going to fix up after your series is committed.