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

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

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()
---
 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..96d8d4f 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"); /* Info available from earlier gprintk(). */
 }
 
 void vmx_do_resume(struct vcpu *v)
diff --git a/xen/arch/x86/traps.c b/xen/arch/x86/traps.c
index a3e8f0c..d57e84c 100644
--- 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));
 }
 
 /*
diff --git a/xen/common/domain.c b/xen/common/domain.c
index 558318e..4225d39 100644
--- a/xen/common/domain.c
+++ b/xen/common/domain.c
@@ -686,7 +686,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 2541ecb..e815787 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, ...) do {                            \
+        printk(XENLOG_G_ERR "domain_crash_sync(%pv) from %s: " fmt, \
+               current, __func__, ## __VA_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