[PATCH] x86/traps: Make nmi_show_execution_state() more useful

Andrew Cooper posted 1 patch 1 year, 8 months ago
Test gitlab-ci passed
Patches applied successfully (tree, apply log)
git fetch https://gitlab.com/xen-project/patchew/xen tags/patchew/20220812173322.2222-1-andrew.cooper3@citrix.com
xen/arch/x86/traps.c | 10 ++++++++--
1 file changed, 8 insertions(+), 2 deletions(-)
[PATCH] x86/traps: Make nmi_show_execution_state() more useful
Posted by Andrew Cooper 1 year, 8 months ago
 * Always emit current.  It's critically important.
 * Do not render (0000000000000000) for the symbol in guest context.  It's
   just line-noise.  Instead, explicitly identify which Xen vs guest context.
 * Try to tabulate the data, because there is often lots of it.

Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
---
CC: Jan Beulich <JBeulich@suse.com>
CC: Roger Pau Monné <roger.pau@citrix.com>
CC: Wei Liu <wl@xen.org>
---
 xen/arch/x86/traps.c | 10 ++++++++--
 1 file changed, 8 insertions(+), 2 deletions(-)

diff --git a/xen/arch/x86/traps.c b/xen/arch/x86/traps.c
index b713ef7e77ce..95ef59c93bcd 100644
--- a/xen/arch/x86/traps.c
+++ b/xen/arch/x86/traps.c
@@ -768,8 +768,14 @@ static int cf_check nmi_show_execution_state(
     if ( opt_show_all )
         show_execution_state(regs);
     else
-        printk(XENLOG_ERR "CPU%d @ %04x:%08lx (%pS)\n", cpu, regs->cs,
-               regs->rip, guest_mode(regs) ? NULL : _p(regs->rip));
+    {
+        if ( guest_mode(regs) )
+            printk(XENLOG_ERR "CPU%d\t%pv\t%04x:%p in guest\n",
+                   cpu, current, regs->cs, _p(regs->rip));
+        else
+            printk(XENLOG_ERR "CPU%d\t%pv\t%04x:%p in Xen: %pS\n",
+                   cpu, current, regs->cs, _p(regs->rip), _p(regs->rip));
+    }
     cpumask_clear_cpu(cpu, &show_state_mask);
 
     return 1;
-- 
2.11.0


Re: [PATCH] x86/traps: Make nmi_show_execution_state() more useful
Posted by Jan Beulich 1 year, 8 months ago
On 12.08.2022 19:33, Andrew Cooper wrote:
>  * Always emit current.  It's critically important.

I agree it may be relevant, yet I'm not convinced of "critically". But
anyway.

> --- a/xen/arch/x86/traps.c
> +++ b/xen/arch/x86/traps.c
> @@ -768,8 +768,14 @@ static int cf_check nmi_show_execution_state(
>      if ( opt_show_all )
>          show_execution_state(regs);
>      else
> -        printk(XENLOG_ERR "CPU%d @ %04x:%08lx (%pS)\n", cpu, regs->cs,
> -               regs->rip, guest_mode(regs) ? NULL : _p(regs->rip));
> +    {
> +        if ( guest_mode(regs) )
> +            printk(XENLOG_ERR "CPU%d\t%pv\t%04x:%p in guest\n",
> +                   cpu, current, regs->cs, _p(regs->rip));
> +        else
> +            printk(XENLOG_ERR "CPU%d\t%pv\t%04x:%p in Xen: %pS\n",
> +                   cpu, current, regs->cs, _p(regs->rip), _p(regs->rip));
> +    }

Could I talk you into avoiding the extra level of indentation, by using
"else if" in the middle? Preferably done that way:
Reviewed-by: Jan Beulich <jbeulich@suse.com>

Jan