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

["Jan Beulich" <JBeulich@xxxxxxxx>]

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