[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [Xen-devel] [PATCH 2/2] Xen/x86: Improve information from domain_crash_synchronous
>>> On 05.09.13 at 11:57, Andrew Cooper <andrew.cooper3@xxxxxxxxxx> wrote: > On 05/09/13 10:51, Jan Beulich wrote: >>>>> On 05.09.13 at 11:44, Andrew Cooper <andrew.cooper3@xxxxxxxxxx> wrote: >>> On 05/09/13 09:15, Jan Beulich wrote: >>>>>>> On 04.09.13 at 20:18, Andrew Cooper <andrew.cooper3@xxxxxxxxxx> wrote: >>>>> --- 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. The debug symbol table never gets used, but local symbols should always end up in the normal ELF symbol table. I just checked - they do here. >>>>> @@ -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. > > 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. I did realize those intentions, but whether an orphan label gets added here or in the macro doesn't matter - the label remains orphaned, and hence would be a likely subject to janitorial work. Jan _______________________________________________ Xen-devel mailing list Xen-devel@xxxxxxxxxxxxx http://lists.xen.org/xen-devel
|
Lists.xenproject.org is hosted with RackSpace, monitoring our |