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

[Konrad Rzeszutek Wilk <konrad.wilk@xxxxxxxxxx>]

On Mon, Feb 05, 2018 at 11:16:55AM +0000, Andrew Cooper 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 calling function is identified, and the message is emitted as
> a non-debug guest error.
> 
> Signed-off-by: Andrew Cooper <andrew.cooper3@xxxxxxxxxx>

Reviewed-by: Konrad Rzeszutek Wilk <konrad.wilk@xxxxxxxxxx>
> ---
> CC: Jan Beulich <JBeulich@xxxxxxxx>
> CC: Jun Nakajima <jun.nakajima@xxxxxxxxx>
> CC: Kevin Tian <kevin.tian@xxxxxxxxx>
> ---
>  xen/arch/x86/hvm/vmx/vmcs.c |  2 +-
>  xen/arch/x86/traps.c        |  5 +----
>  xen/common/domain.c         |  2 +-
>  xen/common/wait.c           | 16 ++++------------
>  xen/include/xen/sched.h     | 11 ++++++-----
>  5 files changed, 13 insertions(+), 23 deletions(-)
> 
> diff --git a/xen/arch/x86/hvm/vmx/vmcs.c b/xen/arch/x86/hvm/vmx/vmcs.c
> index e7818ca..5370ffa 100644
> --- a/xen/arch/x86/hvm/vmx/vmcs.c
> +++ b/xen/arch/x86/hvm/vmx/vmcs.c
> @@ -1669,7 +1669,7 @@ void vmx_vmentry_failure(void)
>           error == VMX_INSN_INVALID_HOST_STATE )
>          vmcs_dump_vcpu(curr);
>  
> -    domain_crash_synchronous();
> +    domain_crash_sync("\n"); /* Nothing further interesting to print. */
>  }
>  
>  void vmx_do_resume(struct vcpu *v)
> diff --git a/xen/arch/x86/traps.c b/xen/arch/x86/traps.c
> index 1187fd9..3c11582 100644
> --- a/xen/arch/x86/traps.c
> +++ b/xen/arch/x86/traps.c
> @@ -2111,10 +2111,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 %pS [%p]\n", _p(addr), _p(addr));
>  }
>  
>  /*
> diff --git a/xen/common/domain.c b/xen/common/domain.c
> index 4567773..d3ba2c2 100644
> --- a/xen/common/domain.c
> +++ b/xen/common/domain.c
> @@ -685,7 +685,7 @@ void __domain_crash(struct domain *d)
>  }
>  
>  
> -void __domain_crash_synchronous(void)
> +void __domain_crash_sync(void)
>  {
>      __domain_crash(current->domain);
>  
> diff --git a/xen/common/wait.c b/xen/common/wait.c
> index a57bc10..153a59e 100644
> --- a/xen/common/wait.c
> +++ b/xen/common/wait.c
> @@ -133,10 +133,7 @@ static void __prepare_to_wait(struct waitqueue_vcpu *wqv)
>      wqv->wakeup_cpu = smp_processor_id();
>      cpumask_copy(&wqv->saved_affinity, curr->cpu_hard_affinity);
>      if ( vcpu_set_hard_affinity(curr, cpumask_of(wqv->wakeup_cpu)) )
> -    {
> -        gdprintk(XENLOG_ERR, "Unable to set vcpu affinity\n");
> -        domain_crash_synchronous();
> -    }
> +        domain_crash_sync("Unable to set vcpu affinity\n");
>  
>      /* Hand-rolled setjmp(). */
>      asm volatile (
> @@ -164,10 +161,7 @@ static void __prepare_to_wait(struct waitqueue_vcpu *wqv)
>          : "memory" );
>  
>      if ( unlikely(wqv->esp == 0) )
> -    {
> -        gdprintk(XENLOG_ERR, "Stack too large in %s\n", __func__);
> -        domain_crash_synchronous();
> -    }
> +        domain_crash_sync("Stack too large\n");
>  
>      cpu_info->guest_cpu_user_regs.entry_vector = entry_vector;
>  }
> @@ -194,10 +188,8 @@ void check_wakeup_from_wait(void)
>          struct vcpu *curr = current;
>          cpumask_copy(&wqv->saved_affinity, curr->cpu_hard_affinity);
>          if ( vcpu_set_hard_affinity(curr, cpumask_of(wqv->wakeup_cpu)) )
> -        {
> -            gdprintk(XENLOG_ERR, "Unable to set vcpu affinity\n");
> -            domain_crash_synchronous();
> -        }
> +            domain_crash_sync("Unable to set vcpu affinity\n");
> +
>          wait(); /* takes us back into the scheduler */
>      }
>  
> diff --git a/xen/include/xen/sched.h b/xen/include/xen/sched.h
> index 39f9386..1da93aa 100644
> --- 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, args...) do {                            \
> +        printk(XENLOG_G_ERR "domain_crash_sync called from %s: " fmt,   \
> +               __func__, ## args);                                      \
> +        __domain_crash_sync();                                          \
> +    } while (0)
>  
>  /*
>   * Called from assembly code, with an optional address to help indicate why
> -- 
> 2.1.4
> 
> 
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@xxxxxxxxxxxxxxxxxxxx
> https://lists.xenproject.org/mailman/listinfo/xen-devel

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