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

[Andrew Cooper <andrew.cooper3@citrix.com>]

On 05/09/13 10:51, Jan Beulich wrote:
>>>> On 05.09.13 at 11:44, Andrew Cooper <andrew.cooper3@citrix.com> wrote:
>> On 05/09/13 09:15, Jan Beulich wrote:
>>>>>> On 04.09.13 at 20:18, Andrew Cooper <andrew.cooper3@citrix.com> 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.