Re: [Xen-devel] [PATCH RFC] xen: Improvements to domain_crash_sync()

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

On 24/01/18 16:11, Jan Beulich wrote:
>>>> On 24.01.18 at 16:49, <andrew.cooper3@xxxxxxxxxx> wrote:
>> The use of __LINE__ in a printk() is problematic for livepatching, as it
>> causes unnecessary binary differences.
>>
>> Furthermore, diagnostic information around calls is inconsistent and
>> occasionally unhelpful.  (e.g. diagnosing logs from the field which might be
>> release builds, or likely without exact source code).
>>
>> Take the opportunity to improve things.  Shorten the name to
>> domain_crash_sync() and require the user to pass a print message in.
>>
>> Internally, the current vcpu and calling function are identified, and the
>> message is emitted as a non-debug guest error.
>>
>> Signed-off-by: Andrew Cooper <andrew.cooper3@xxxxxxxxxx>
>> ---
>> CC: Jan Beulich <JBeulich@xxxxxxxx>
>> CC: Stefano Stabellini <sstabellini@xxxxxxxxxx>
>> CC: Julien Grall <julien.grall@xxxxxxx>
>>
>> This is RFC for now as it only does the x86 side of things.
>>
>> If it is considered generally acceptable, I'll do the ARM side of things, and
>> a similar patch for domain_crash()
> I'm fine with this, just two remarks:
>
>> --- a/xen/arch/x86/traps.c
>> +++ b/xen/arch/x86/traps.c
>> @@ -2083,10 +2083,7 @@ void asm_domain_crash_synchronous(unsigned long addr)
>>      if ( addr == 0 )
>>          addr = this_cpu(last_extable_addr);
>>  
>> -    printk("domain_crash_sync called from entry.S: fault at %p %pS\n",
>> -           _p(addr), _p(addr));
>> -
>> -    __domain_crash_synchronous();
>> +    domain_crash_sync("entry.S fault at %p %pS\n", _p(addr), _p(addr));
> Could we try to aim for some consistency here going forward?
> Either make %pS always _also_ print the raw number, or (if
> that's undesirable in some use cases) re-arrange the above to
> achieve the same effect, which I's in particular like to be the
> deciphered value first, and the raw one in e.g. square brackets
> (like iirc Linux does):
>
>     domain_crash_sync("entry.S fault at %pS [%p]\n", _p(addr), _p(addr));

Can do (although the reason I didn't shorten this function name is
because it isn't long for the world, once I dust off my
create_bounce_frame in C series).

>
>> --- a/xen/include/xen/sched.h
>> +++ b/xen/include/xen/sched.h
>> @@ -627,11 +627,12 @@ void __domain_crash(struct domain *d);
>>   * Mark current domain as crashed and synchronously deschedule from the 
>> local
>>   * processor. This function never returns.
>>   */
>> -void noreturn __domain_crash_synchronous(void);
>> -#define domain_crash_synchronous() do {                                   \
>> -    printk("domain_crash_sync called from %s:%d\n", __FILE__, __LINE__);  \
>> -    __domain_crash_synchronous();                                         \
>> -} while (0)
>> +void noreturn __domain_crash_sync(void);
>> +#define domain_crash_sync(fmt, ...) do {                            \
>> +        printk(XENLOG_G_ERR "domain_crash_sync(%pv) from %s: " fmt, \
>> +               current, __func__, ## __VA_ARGS__);                  \
> This isn't C standard mandated usage of __VA_ARGS__; I generally
> think it is better to use the older GCC extension when the number
> of actuals may validly be zero (which the C standard doesn't allow).

Do you mean go with the (fmt, args...) version ?

~Andrew

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel