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

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

On 24/01/18 16:15, Andrew Cooper wrote:
> 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 ?

I don't see any other GNU extensions which work here.  Unless you can
point out exactly which one you mean, I'll stay with the ## __VA_ARGS__
version, because that is consistent with other examples in our codebase.

~Andrew

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