[v3] xen/console: print Xen version via keyhandler

[PATCH v3] xen/console: print Xen version via keyhandler

Posted by dmkhn@proton.me 8 months, 3 weeks ago

Add Xen version printout to 'h' keyhandler output.

That is useful for debugging systems that have been left intact for a long
time.

Signed-off-by: Denis Mukhin <dmukhin@ford.com>
---
Changes since v2:
- moved new function declarations to xen/lib.h
- dropped xen_ prefix in new functions
---
 xen/common/keyhandler.c    |  4 ++++
 xen/common/version.c       | 20 ++++++++++++++++++--
 xen/drivers/char/console.c |  8 +++-----
 xen/include/xen/lib.h      |  3 +++
 4 files changed, 28 insertions(+), 7 deletions(-)

diff --git a/xen/common/keyhandler.c b/xen/common/keyhandler.c
index 6ea54838d4..0bb842ec00 100644
--- a/xen/common/keyhandler.c
+++ b/xen/common/keyhandler.c
@@ -129,6 +129,10 @@ static void cf_check show_handlers(unsigned char key)
     unsigned int i;
 
     printk("'%c' pressed -> showing installed handlers\n", key);
+
+    print_version();
+    print_build_id();
+
     for ( i = 0; i < ARRAY_SIZE(key_table); i++ )
         if ( key_table[i].fn )
             printk(" key '%c' (ascii '%02x') => %s\n",
diff --git a/xen/common/version.c b/xen/common/version.c
index bc3714b45f..8672d5544e 100644
--- a/xen/common/version.c
+++ b/xen/common/version.c
@@ -210,9 +210,25 @@ void __init xen_build_init(void)
         }
     }
 #endif /* CONFIG_X86 */
-    if ( !rc )
-        printk(XENLOG_INFO "build-id: %*phN\n", build_id_len, build_id_p);
 }
+
+void print_version(void)
+{
+    printk("Xen version %d.%d%s (%s@%s) (%s) %s %s\n",
+           xen_major_version(), xen_minor_version(), xen_extra_version(),
+           xen_compile_by(), xen_compile_domain(), xen_compiler(),
+           xen_build_info(), xen_compile_date());
+
+    printk("Latest ChangeSet: %s\n", xen_changeset());
+}
+
+void print_build_id(void)
+{
+    BUG_ON(!build_id_p);
+    BUG_ON(!build_id_len);
+    printk(XENLOG_INFO "build-id: %*phN\n", build_id_len, build_id_p);
+}
+
 #endif /* BUILD_ID */
 /*
  * Local variables:
diff --git a/xen/drivers/char/console.c b/xen/drivers/char/console.c
index 73d24a7821..235d55bbff 100644
--- a/xen/drivers/char/console.c
+++ b/xen/drivers/char/console.c
@@ -1024,14 +1024,12 @@ void __init console_init_preirq(void)
     nrspin_lock(&console_lock);
     __putstr(xen_banner());
     nrspin_unlock(&console_lock);
-    printk("Xen version %d.%d%s (%s@%s) (%s) %s %s\n",
-           xen_major_version(), xen_minor_version(), xen_extra_version(),
-           xen_compile_by(), xen_compile_domain(), xen_compiler(),
-           xen_build_info(), xen_compile_date());
-    printk("Latest ChangeSet: %s\n", xen_changeset());
+
+    print_version();
 
     /* Locate and print the buildid, if applicable. */
     xen_build_init();
+    print_build_id();
 
     if ( opt_sync_console )
     {
diff --git a/xen/include/xen/lib.h b/xen/include/xen/lib.h
index 81b722ea3e..686899a63e 100644
--- a/xen/include/xen/lib.h
+++ b/xen/include/xen/lib.h
@@ -47,6 +47,9 @@ int parse_signed_integer(const char *name, const char *s, const char *e,
  */
 int cmdline_strcmp(const char *frag, const char *name);
 
+void print_version(void);
+void print_build_id(void);
+
 #ifdef CONFIG_DEBUG_TRACE
 extern void debugtrace_dump(void);
 extern void debugtrace_printk(const char *fmt, ...)
-- 
2.34.1

Re: [PATCH v3] xen/console: print Xen version via keyhandler

Posted by Jan Beulich 8 months, 2 weeks ago

On 13.02.2025 22:41, dmkhn@proton.me wrote:
> Add Xen version printout to 'h' keyhandler output.
> 
> That is useful for debugging systems that have been left intact for a long
> time.
> 
> Signed-off-by: Denis Mukhin <dmukhin@ford.com>
> ---
> Changes since v2:
> - moved new function declarations to xen/lib.h
> - dropped xen_ prefix in new functions
> ---
>  xen/common/keyhandler.c    |  4 ++++
>  xen/common/version.c       | 20 ++++++++++++++++++--
>  xen/drivers/char/console.c |  8 +++-----
>  xen/include/xen/lib.h      |  3 +++
>  4 files changed, 28 insertions(+), 7 deletions(-)
> 
> diff --git a/xen/common/keyhandler.c b/xen/common/keyhandler.c
> index 6ea54838d4..0bb842ec00 100644
> --- a/xen/common/keyhandler.c
> +++ b/xen/common/keyhandler.c
> @@ -129,6 +129,10 @@ static void cf_check show_handlers(unsigned char key)
>      unsigned int i;
>  
>      printk("'%c' pressed -> showing installed handlers\n", key);
> +
> +    print_version();
> +    print_build_id();
> +
>      for ( i = 0; i < ARRAY_SIZE(key_table); i++ )
>          if ( key_table[i].fn )
>              printk(" key '%c' (ascii '%02x') => %s\n",
> diff --git a/xen/common/version.c b/xen/common/version.c
> index bc3714b45f..8672d5544e 100644
> --- a/xen/common/version.c
> +++ b/xen/common/version.c
> @@ -210,9 +210,25 @@ void __init xen_build_init(void)
>          }
>      }
>  #endif /* CONFIG_X86 */
> -    if ( !rc )
> -        printk(XENLOG_INFO "build-id: %*phN\n", build_id_len, build_id_p);
>  }
> +
> +void print_version(void)
> +{
> +    printk("Xen version %d.%d%s (%s@%s) (%s) %s %s\n",
> +           xen_major_version(), xen_minor_version(), xen_extra_version(),
> +           xen_compile_by(), xen_compile_domain(), xen_compiler(),
> +           xen_build_info(), xen_compile_date());
> +
> +    printk("Latest ChangeSet: %s\n", xen_changeset());
> +}
> +
> +void print_build_id(void)
> +{
> +    BUG_ON(!build_id_p);
> +    BUG_ON(!build_id_len);

Technically one of the two would likely suffice, if we need such checks
at all. Question is why you added them. After all ...

> +    printk(XENLOG_INFO "build-id: %*phN\n", build_id_len, build_id_p);

... this isn't supposed to malfunction when passed a NULL pointer (with
zero length). If it can malfunction, it wants fixing there imo. And that
extends to the case of non-zero length as well: Any extensions to %p
that we add would better retain the basic property of %p printing when
NULL is passed as argument. (I'm sorry for not paying enough attention
to this on v2 already.)

(Later) Oh, actually these BUG_ON() are both wrong. They would trigger
when building with a build-id-incapable linker. See the detection logic
in the top level Makefile (search for XEN_HAS_BUILD_ID). To retain
original behavior you will want to make the printk() conditional upon
e.g. build_id_len being non-zero.

Jan

Re: [PATCH v3] xen/console: print Xen version via keyhandler

Posted by Denis Mukhin 8 months, 2 weeks ago

On Thursday, February 13th, 2025 at 11:54 PM, Jan Beulich <jbeulich@suse.com> wrote:

> 
> 
> On 13.02.2025 22:41, dmkhn@proton.me wrote:
> 
> > Add Xen version printout to 'h' keyhandler output.
> > 
> > That is useful for debugging systems that have been left intact for a long
> > time.
> > 
> > Signed-off-by: Denis Mukhin dmukhin@ford.com
> > ---
> > Changes since v2:
> > - moved new function declarations to xen/lib.h
> > - dropped xen_ prefix in new functions
> > ---
> > xen/common/keyhandler.c | 4 ++++
> > xen/common/version.c | 20 ++++++++++++++++++--
> > xen/drivers/char/console.c | 8 +++-----
> > xen/include/xen/lib.h | 3 +++
> > 4 files changed, 28 insertions(+), 7 deletions(-)
> > 
> > diff --git a/xen/common/keyhandler.c b/xen/common/keyhandler.c
> > index 6ea54838d4..0bb842ec00 100644
> > --- a/xen/common/keyhandler.c
> > +++ b/xen/common/keyhandler.c
> > @@ -129,6 +129,10 @@ static void cf_check show_handlers(unsigned char key)
> > unsigned int i;
> > 
> > printk("'%c' pressed -> showing installed handlers\n", key);
> > +
> > + print_version();
> > + print_build_id();
> > +
> > for ( i = 0; i < ARRAY_SIZE(key_table); i++ )
> > if ( key_table[i].fn )
> > printk(" key '%c' (ascii '%02x') => %s\n",
> > diff --git a/xen/common/version.c b/xen/common/version.c
> > index bc3714b45f..8672d5544e 100644
> > --- a/xen/common/version.c
> > +++ b/xen/common/version.c
> > @@ -210,9 +210,25 @@ void __init xen_build_init(void)
> > }
> > }
> > #endif /* CONFIG_X86 */
> > - if ( !rc )
> > - printk(XENLOG_INFO "build-id: %*phN\n", build_id_len, build_id_p);
> > }
> > +
> > +void print_version(void)
> > +{
> > + printk("Xen version %d.%d%s (%s@%s) (%s) %s %s\n",
> > + xen_major_version(), xen_minor_version(), xen_extra_version(),
> > + xen_compile_by(), xen_compile_domain(), xen_compiler(),
> > + xen_build_info(), xen_compile_date());
> > +
> > + printk("Latest ChangeSet: %s\n", xen_changeset());
> > +}
> > +
> > +void print_build_id(void)
> > +{
> > + BUG_ON(!build_id_p);
> > + BUG_ON(!build_id_len);
> 
> 
> Technically one of the two would likely suffice, if we need such checks
> at all. Question is why you added them. After all ...

I added assertions so that any misuse can bee easily seen.
But I did not know about XEN_HAS_BUILD_ID switch.

> 
> > + printk(XENLOG_INFO "build-id: %*phN\n", build_id_len, build_id_p);
> 
> 
> ... this isn't supposed to malfunction when passed a NULL pointer (with
> zero length). If it can malfunction, it wants fixing there imo. And that
> extends to the case of non-zero length as well: Any extensions to %p
> that we add would better retain the basic property of %p printing when
> NULL is passed as argument. (I'm sorry for not paying enough attention
> to this on v2 already.)

No problem! Thanks for the feedback, much appreciated.

> 
> (Later) Oh, actually these BUG_ON() are both wrong. They would trigger
> when building with a build-id-incapable linker. See the detection logic
> in the top level Makefile (search for XEN_HAS_BUILD_ID). To retain
> original behavior you will want to make the printk() conditional upon
> e.g. build_id_len being non-zero.

Oh, I see.
%p in printk() behaves correctly when passing NULL pointer - it prints
nothing.

> 
> Jan