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

["Jan Beulich" <JBeulich@xxxxxxxx>]

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

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

Jan


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