Re: [Xen-devel] [PATCH 2/2] Xen/x86: Improve information from domain_crash_synchronous

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> 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:
>>> 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...

>>> @@ -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


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