[PATCH v1 0/3] xen/console: few cleanups in console driver

[PATCH v1 0/3] xen/console: few cleanups in console driver

[dmkhn]

The patch series introduces a few cleanups aimed at reducing code duplication
in the console driver.

Originally, patches 2 and 3 were part of NS16550 emulator v3 series [1].

Patch 1 removes some code duplication for logging via conring facility.

Patch 2 (see [2]) removes code duplication between __putstr() and the rest of
the driver. It also introduces private flags to select console devices for
printout which simplifies some code paths.

Patch 3 (see [3]) adds conring_flush() to send contents of conring to all
currently available console devices.

[1] https://lore.kernel.org/xen-devel/20250103-vuart-ns8250-v3-v1-0-c5d36b31d66c@ford.com/
[2] https://lore.kernel.org/xen-devel/20250103-vuart-ns8250-v3-v1-16-c5d36b31d66c@ford.com/
[3] https://lore.kernel.org/xen-devel/20250103-vuart-ns8250-v3-v1-17-c5d36b31d66c@ford.com/
[4] CI link: https://gitlab.com/xen-project/people/dmukhin/xen/-/pipelines/1749584524

Denis Mukhin (3):
  xen/console: cleanup conring management
  xen/console: introduce console_puts()
  xen/console: introduce conring_flush()

 xen/drivers/char/console.c | 175 ++++++++++++++++++++++---------------
 1 file changed, 104 insertions(+), 71 deletions(-)

-- 
2.34.1

[PATCH v1 1/3] xen/console: cleanup conring management

[dmkhn]

From: Denis Mukhin <dmukhin@ford.com>

Move console_locks_busted handling inside conring_puts() to remove
tasklet code duplication.

Signed-off-by: Denis Mukhin <dmukhin@ford.com>
---
 xen/drivers/char/console.c | 29 ++++++++++++++---------------
 1 file changed, 14 insertions(+), 15 deletions(-)

diff --git a/xen/drivers/char/console.c b/xen/drivers/char/console.c
index c3150fbdb7..aaa97088aa 100644
--- a/xen/drivers/char/console.c
+++ b/xen/drivers/char/console.c
@@ -325,6 +325,17 @@ static void cf_check do_dec_thresh(unsigned char key, bool unused)
  * ********************************************************
  */
 
+static void cf_check notify_dom0_con_ring(void *unused)
+{
+    send_global_virq(VIRQ_CON_RING);
+}
+
+static DECLARE_SOFTIRQ_TASKLET(notify_dom0_con_ring_tasklet,
+                               notify_dom0_con_ring,
+                               NULL);
+
+static bool console_locks_busted;
+
 static void conring_puts(const char *str, size_t len)
 {
     ASSERT(rspin_is_locked(&console_lock));
@@ -334,6 +345,9 @@ static void conring_puts(const char *str, size_t len)
 
     if ( conringp - conringc > conring_size )
         conringc = conringp - conring_size;
+
+    if ( !console_locks_busted )
+        tasklet_schedule(&notify_dom0_con_ring_tasklet);
 }
 
 long read_console_ring(struct xen_sysctl_readconsole *op)
@@ -594,13 +608,6 @@ static void cf_check serial_rx(char c)
     __serial_rx(c);
 }
 
-static void cf_check notify_dom0_con_ring(void *unused)
-{
-    send_global_virq(VIRQ_CON_RING);
-}
-static DECLARE_SOFTIRQ_TASKLET(notify_dom0_con_ring_tasklet,
-                               notify_dom0_con_ring, NULL);
-
 #ifdef CONFIG_X86
 static inline void xen_console_write_debug_port(const char *buf, size_t len)
 {
@@ -648,10 +655,7 @@ static long guest_console_write(XEN_GUEST_HANDLE_PARAM(char) buffer,
 #endif
 
             if ( opt_console_to_ring )
-            {
                 conring_puts(kbuf, kcount);
-                tasklet_schedule(&notify_dom0_con_ring_tasklet);
-            }
 
             nrspin_unlock_irq(&console_lock);
         }
@@ -753,8 +757,6 @@ long do_console_io(
  * *****************************************************
  */
 
-static bool console_locks_busted;
-
 static void __putstr(const char *str)
 {
     size_t len = strlen(str);
@@ -775,9 +777,6 @@ static void __putstr(const char *str)
 #endif
 
     conring_puts(str, len);
-
-    if ( !console_locks_busted )
-        tasklet_schedule(&notify_dom0_con_ring_tasklet);
 }
 
 static int printk_prefix_check(char *p, char **pp)
-- 
2.34.1

[PATCH v1 2/3] xen/console: introduce console_puts()

[dmkhn]

From: Denis Mukhin <dmukhin@ford.com>

guest_console_write() duplicates the code from __putstr(), eliminate code
duplication.

Introduce console_puts() for writing a buffer to console devices.

Also, introduce internal console flags to control which console devices
should be used.

Signed-off-by: Denis Mukhin <dmukhin@ford.com>
---
 xen/drivers/char/console.c | 112 ++++++++++++++++++++++---------------
 1 file changed, 66 insertions(+), 46 deletions(-)

diff --git a/xen/drivers/char/console.c b/xen/drivers/char/console.c
index aaa97088aa..2618c2e47d 100644
--- a/xen/drivers/char/console.c
+++ b/xen/drivers/char/console.c
@@ -41,6 +41,20 @@
 #include <asm/vpl011.h>
 #endif
 
+/* Internal console flags. */
+enum {
+    CONSOLE_SERIAL  = BIT(0, U),    /* Use serial device. */
+    CONSOLE_PV      = BIT(1, U),    /* Use PV console. */
+    CONSOLE_VIDEO   = BIT(2, U),    /* Use video device. */
+    CONSOLE_DEBUG   = BIT(3, U),    /* Use debug device. */
+    CONSOLE_RING    = BIT(4, U),    /* Use console ring. */
+    CONSOLE_DEFAULT = CONSOLE_SERIAL | CONSOLE_PV | CONSOLE_VIDEO |
+                      CONSOLE_DEBUG,
+    CONSOLE_ALL     = CONSOLE_DEFAULT | CONSOLE_RING,
+};
+
+static void console_puts(const char *str, size_t len, unsigned int flags);
+
 /* console: comma-separated list of console outputs. */
 static char __initdata opt_console[30] = OPT_CONSOLE_STR;
 string_param("console", opt_console);
@@ -338,8 +352,6 @@ static bool console_locks_busted;
 
 static void conring_puts(const char *str, size_t len)
 {
-    ASSERT(rspin_is_locked(&console_lock));
-
     while ( len-- )
         conring[CONRING_IDX_MASK(conringp++)] = *str++;
 
@@ -432,9 +444,6 @@ void console_serial_puts(const char *s, size_t nr)
         serial_steal_fn(s, nr);
     else
         serial_puts(sercon_handle, s, nr);
-
-    /* Copy all serial output into PV console */
-    pv_console_puts(s, nr);
 }
 
 static void cf_check dump_console_ring_key(unsigned char key)
@@ -468,8 +477,7 @@ static void cf_check dump_console_ring_key(unsigned char key)
         c += len;
     }
 
-    console_serial_puts(buf, sofar);
-    video_puts(buf, sofar);
+    console_puts(buf, sofar, CONSOLE_SERIAL | CONSOLE_VIDEO | CONSOLE_PV);
 
     free_xenheap_pages(buf, order);
 }
@@ -618,11 +626,61 @@ static inline void xen_console_write_debug_port(const char *buf, size_t len)
 }
 #endif
 
+static inline void console_debug_puts(const char *str, size_t len)
+{
+#ifdef CONFIG_X86
+    if ( opt_console_xen )
+    {
+        if ( xen_guest )
+            xen_hypercall_console_write(str, len);
+        else
+            xen_console_write_debug_port(str, len);
+    }
+#endif
+}
+
+/*
+ * Write buffer to all enabled console devices.
+ *
+ * That will handle all possible scenarios working w/ console
+ * - physical console (serial console, VGA console (x86 only));
+ * - PV console;
+ * - debug console (x86 only): debug I/O port or __HYPERVISOR_console_io
+ *   hypercall;
+ * - console ring.
+ */
+static void console_puts(const char *str, size_t len, unsigned int flags)
+{
+    ASSERT(rspin_is_locked(&console_lock));
+
+    if ( flags & CONSOLE_SERIAL )
+        console_serial_puts(str, len);
+
+    if ( flags & CONSOLE_PV )
+        pv_console_puts(str, len);
+
+    if ( flags & CONSOLE_VIDEO )
+        video_puts(str, len);
+
+    if ( flags & CONSOLE_DEBUG )
+        console_debug_puts(str, len);
+
+    if ( flags & CONSOLE_RING )
+        conring_puts(str, len);
+}
+
+static inline void __putstr(const char *str)
+{
+    console_puts(str, strlen(str), CONSOLE_ALL);
+}
+
 static long guest_console_write(XEN_GUEST_HANDLE_PARAM(char) buffer,
                                 unsigned int count)
 {
     char kbuf[128];
     unsigned int kcount = 0;
+    unsigned int flags = opt_console_to_ring
+                         ? CONSOLE_ALL : CONSOLE_DEFAULT;
     struct domain *cd = current->domain;
 
     while ( count > 0 )
@@ -640,23 +698,7 @@ static long guest_console_write(XEN_GUEST_HANDLE_PARAM(char) buffer,
         {
             /* Use direct console output as it could be interactive */
             nrspin_lock_irq(&console_lock);
-
-            console_serial_puts(kbuf, kcount);
-            video_puts(kbuf, kcount);
-
-#ifdef CONFIG_X86
-            if ( opt_console_xen )
-            {
-                if ( xen_guest )
-                    xen_hypercall_console_write(kbuf, kcount);
-                else
-                    xen_console_write_debug_port(kbuf, kcount);
-            }
-#endif
-
-            if ( opt_console_to_ring )
-                conring_puts(kbuf, kcount);
-
+            console_puts(kbuf, kcount, flags);
             nrspin_unlock_irq(&console_lock);
         }
         else
@@ -757,28 +799,6 @@ long do_console_io(
  * *****************************************************
  */
 
-static void __putstr(const char *str)
-{
-    size_t len = strlen(str);
-
-    ASSERT(rspin_is_locked(&console_lock));
-
-    console_serial_puts(str, len);
-    video_puts(str, len);
-
-#ifdef CONFIG_X86
-    if ( opt_console_xen )
-    {
-        if ( xen_guest )
-            xen_hypercall_console_write(str, len);
-        else
-            xen_console_write_debug_port(str, len);
-    }
-#endif
-
-    conring_puts(str, len);
-}
-
 static int printk_prefix_check(char *p, char **pp)
 {
     int loglvl = -1;
-- 
2.34.1

[PATCH v1 3/3] xen/console: introduce conring_flush()

[dmkhn]

From: Denis Mukhin <dmukhin@ford.com>

Introduce conring_flush() to ensure all messages kept in the internal
console ring are sent to all physical consoles (serial, VGA (x86))
after their initialization is completed.

Resolves: https://gitlab.com/xen-project/xen/-/issues/184
Signed-off-by: Denis Mukhin <dmukhin@ford.com>
---
The patch conflicts with
  https://lore.kernel.org/xen-devel/20250331230508.440198-7-dmukhin@ford.com/
in console_init_preirq()
---
 xen/drivers/char/console.c | 34 ++++++++++++++++++++++++----------
 1 file changed, 24 insertions(+), 10 deletions(-)

diff --git a/xen/drivers/char/console.c b/xen/drivers/char/console.c
index 2618c2e47d..18eb66df89 100644
--- a/xen/drivers/char/console.c
+++ b/xen/drivers/char/console.c
@@ -446,24 +446,22 @@ void console_serial_puts(const char *s, size_t nr)
         serial_puts(sercon_handle, s, nr);
 }
 
-static void cf_check dump_console_ring_key(unsigned char key)
+/*
+ * Flush contents of the conring to the physical console devices.
+ */
+static int conring_flush(void)
 {
     uint32_t idx, len, sofar, c;
     unsigned int order;
     char *buf;
+    unsigned long flags;
 
-    printk("'%c' pressed -> dumping console ring buffer (dmesg)\n", key);
-
-    /* create a buffer in which we'll copy the ring in the correct
-       order and NUL terminate */
     order = get_order_from_bytes(conring_size + 1);
     buf = alloc_xenheap_pages(order, 0);
     if ( buf == NULL )
-    {
-        printk("unable to allocate memory!\n");
-        return;
-    }
+        return -ENOMEM;
 
+    flags = console_lock_recursive_irqsave();
     c = conringc;
     sofar = 0;
     while ( (c != conringp) )
@@ -478,8 +476,21 @@ static void cf_check dump_console_ring_key(unsigned char key)
     }
 
     console_puts(buf, sofar, CONSOLE_SERIAL | CONSOLE_VIDEO | CONSOLE_PV);
+    console_unlock_recursive_irqrestore(flags);
 
     free_xenheap_pages(buf, order);
+
+    return 0;
+}
+
+static void cf_check conring_dump_keyhandler(unsigned char key)
+{
+    int rc;
+
+    printk("'%c' pressed -> dumping console ring buffer (dmesg)\n", key);
+    rc = conring_flush();
+    if ( rc )
+        printk("failed to dump console ring buffer: %d\n", rc);
 }
 
 /*
@@ -1044,6 +1055,9 @@ void __init console_init_preirq(void)
     serial_set_rx_handler(sercon_handle, serial_rx);
     pv_console_set_rx_handler(serial_rx);
 
+    /* NB: send conring contents to all enabled physical consoles, if any */
+    conring_flush();
+
     /* HELLO WORLD --- start-of-day banner text. */
     nrspin_lock(&console_lock);
     __putstr(xen_banner());
@@ -1134,7 +1148,7 @@ void __init console_endboot(void)
     if ( opt_conswitch[1] == 'x' )
         console_rx = max_console_rx;
 
-    register_keyhandler('w', dump_console_ring_key,
+    register_keyhandler('w', conring_dump_keyhandler,
                         "synchronously dump console ring buffer (dmesg)", 0);
     register_irq_keyhandler('+', &do_inc_thresh,
                             "increase log level threshold", 0);
-- 
2.34.1

Re: [PATCH v1 1/3] xen/console: cleanup conring management

[Jan Beulich]

On 03.04.2025 02:06, dmkhn@proton.me wrote:
> From: Denis Mukhin <dmukhin@ford.com>
> 
> Move console_locks_busted handling inside conring_puts() to remove
> tasklet code duplication.

I for one wouldn't expect tasklet scheduling to happen in a function of
this name. IOW I think the little bit of duplication is warranted here.

Jan

Re: [PATCH v1 1/3] xen/console: cleanup conring management

[Stefano Stabellini]

On Thu, 3 Apr 2025, dmkhn@proton.me wrote:
> From: Denis Mukhin <dmukhin@ford.com>
> 
> Move console_locks_busted handling inside conring_puts() to remove
> tasklet code duplication.
> 
> Signed-off-by: Denis Mukhin <dmukhin@ford.com>

This patch is a good cleanup but makes one functional change: previously
guest_console_write would always call tasklet_schedule. Now, it only
calls tasklet_schedule if !console_locks_busted.

On ARM, we don't call console_force_unlock and never set
console_locks_busted. It makes no difference.

On x86, there are a few callers of console_force_unlock, so it would
make a difference. However, looking at the callers, it seems to me that
the change is for the better and better aligns the code with the
intention behind console_force_unlock.

On my side:
Reviewed-by: Stefano Stabellini <sstabellini@kernel.org>

but it would be good for an x86 maintainer to confirm


> ---
>  xen/drivers/char/console.c | 29 ++++++++++++++---------------
>  1 file changed, 14 insertions(+), 15 deletions(-)
> 
> diff --git a/xen/drivers/char/console.c b/xen/drivers/char/console.c
> index c3150fbdb7..aaa97088aa 100644
> --- a/xen/drivers/char/console.c
> +++ b/xen/drivers/char/console.c
> @@ -325,6 +325,17 @@ static void cf_check do_dec_thresh(unsigned char key, bool unused)
>   * ********************************************************
>   */
>  
> +static void cf_check notify_dom0_con_ring(void *unused)
> +{
> +    send_global_virq(VIRQ_CON_RING);
> +}
> +
> +static DECLARE_SOFTIRQ_TASKLET(notify_dom0_con_ring_tasklet,
> +                               notify_dom0_con_ring,
> +                               NULL);
> +
> +static bool console_locks_busted;
> +
>  static void conring_puts(const char *str, size_t len)
>  {
>      ASSERT(rspin_is_locked(&console_lock));
> @@ -334,6 +345,9 @@ static void conring_puts(const char *str, size_t len)
>  
>      if ( conringp - conringc > conring_size )
>          conringc = conringp - conring_size;
> +
> +    if ( !console_locks_busted )
> +        tasklet_schedule(&notify_dom0_con_ring_tasklet);
>  }
>  
>  long read_console_ring(struct xen_sysctl_readconsole *op)
> @@ -594,13 +608,6 @@ static void cf_check serial_rx(char c)
>      __serial_rx(c);
>  }
>  
> -static void cf_check notify_dom0_con_ring(void *unused)
> -{
> -    send_global_virq(VIRQ_CON_RING);
> -}
> -static DECLARE_SOFTIRQ_TASKLET(notify_dom0_con_ring_tasklet,
> -                               notify_dom0_con_ring, NULL);
> -
>  #ifdef CONFIG_X86
>  static inline void xen_console_write_debug_port(const char *buf, size_t len)
>  {
> @@ -648,10 +655,7 @@ static long guest_console_write(XEN_GUEST_HANDLE_PARAM(char) buffer,
>  #endif
>  
>              if ( opt_console_to_ring )
> -            {
>                  conring_puts(kbuf, kcount);
> -                tasklet_schedule(&notify_dom0_con_ring_tasklet);
> -            }
>  
>              nrspin_unlock_irq(&console_lock);
>          }
> @@ -753,8 +757,6 @@ long do_console_io(
>   * *****************************************************
>   */
>  
> -static bool console_locks_busted;
> -
>  static void __putstr(const char *str)
>  {
>      size_t len = strlen(str);
> @@ -775,9 +777,6 @@ static void __putstr(const char *str)
>  #endif
>  
>      conring_puts(str, len);
> -
> -    if ( !console_locks_busted )
> -        tasklet_schedule(&notify_dom0_con_ring_tasklet);
>  }
>  
>  static int printk_prefix_check(char *p, char **pp)
> -- 
> 2.34.1
> 
> 

Re: [PATCH v1 2/3] xen/console: introduce console_puts()

[Stefano Stabellini]

On Thu, 3 Apr 2025, dmkhn@proton.me wrote:
> From: Denis Mukhin <dmukhin@ford.com>
> 
> guest_console_write() duplicates the code from __putstr(), eliminate code
> duplication.
> 
> Introduce console_puts() for writing a buffer to console devices.
> 
> Also, introduce internal console flags to control which console devices
> should be used.
> 
> Signed-off-by: Denis Mukhin <dmukhin@ford.com>

Reviewed-by: Stefano Stabellini <sstabellini@kernel.org>

Re: [PATCH v1 2/3] xen/console: introduce console_puts()

[Stefano Stabellini]

On Thu, 3 Apr 2025, dmkhn@proton.me wrote:
> From: Denis Mukhin <dmukhin@ford.com>
> 
> guest_console_write() duplicates the code from __putstr(), eliminate code
> duplication.
> 
> Introduce console_puts() for writing a buffer to console devices.
> 
> Also, introduce internal console flags to control which console devices
> should be used.
> 
> Signed-off-by: Denis Mukhin <dmukhin@ford.com>
> ---
>  xen/drivers/char/console.c | 112 ++++++++++++++++++++++---------------
>  1 file changed, 66 insertions(+), 46 deletions(-)
> 
> diff --git a/xen/drivers/char/console.c b/xen/drivers/char/console.c
> index aaa97088aa..2618c2e47d 100644
> --- a/xen/drivers/char/console.c
> +++ b/xen/drivers/char/console.c
> @@ -41,6 +41,20 @@
>  #include <asm/vpl011.h>
>  #endif
>  
> +/* Internal console flags. */
> +enum {
> +    CONSOLE_SERIAL  = BIT(0, U),    /* Use serial device. */
> +    CONSOLE_PV      = BIT(1, U),    /* Use PV console. */
> +    CONSOLE_VIDEO   = BIT(2, U),    /* Use video device. */
> +    CONSOLE_DEBUG   = BIT(3, U),    /* Use debug device. */
> +    CONSOLE_RING    = BIT(4, U),    /* Use console ring. */
> +    CONSOLE_DEFAULT = CONSOLE_SERIAL | CONSOLE_PV | CONSOLE_VIDEO |
> +                      CONSOLE_DEBUG,
> +    CONSOLE_ALL     = CONSOLE_DEFAULT | CONSOLE_RING,
> +};
> +
> +static void console_puts(const char *str, size_t len, unsigned int flags);
> +
>  /* console: comma-separated list of console outputs. */
>  static char __initdata opt_console[30] = OPT_CONSOLE_STR;
>  string_param("console", opt_console);
> @@ -338,8 +352,6 @@ static bool console_locks_busted;
>  
>  static void conring_puts(const char *str, size_t len)
>  {
> -    ASSERT(rspin_is_locked(&console_lock));
> -
>      while ( len-- )
>          conring[CONRING_IDX_MASK(conringp++)] = *str++;
>  
> @@ -432,9 +444,6 @@ void console_serial_puts(const char *s, size_t nr)
>          serial_steal_fn(s, nr);
>      else
>          serial_puts(sercon_handle, s, nr);
> -
> -    /* Copy all serial output into PV console */
> -    pv_console_puts(s, nr);
>  }
>  
>  static void cf_check dump_console_ring_key(unsigned char key)
> @@ -468,8 +477,7 @@ static void cf_check dump_console_ring_key(unsigned char key)
>          c += len;
>      }
>  
> -    console_serial_puts(buf, sofar);
> -    video_puts(buf, sofar);
> +    console_puts(buf, sofar, CONSOLE_SERIAL | CONSOLE_VIDEO | CONSOLE_PV);

Actually I take back the R-b. It looks like this change is breaking
because console_puts now requires the console_lock to be held, while
here the console_lock is not held.

If I am not mistaken if you try to use the 'w' key with this patch
applied you'll hit the ASSERT at the beginning of console_puts


>      free_xenheap_pages(buf, order);
>  }
> @@ -618,11 +626,61 @@ static inline void xen_console_write_debug_port(const char *buf, size_t len)
>  }
>  #endif
>  
> +static inline void console_debug_puts(const char *str, size_t len)
> +{
> +#ifdef CONFIG_X86
> +    if ( opt_console_xen )
> +    {
> +        if ( xen_guest )
> +            xen_hypercall_console_write(str, len);
> +        else
> +            xen_console_write_debug_port(str, len);
> +    }
> +#endif
> +}
> +
> +/*
> + * Write buffer to all enabled console devices.
> + *
> + * That will handle all possible scenarios working w/ console
> + * - physical console (serial console, VGA console (x86 only));
> + * - PV console;
> + * - debug console (x86 only): debug I/O port or __HYPERVISOR_console_io
> + *   hypercall;
> + * - console ring.
> + */
> +static void console_puts(const char *str, size_t len, unsigned int flags)
> +{
> +    ASSERT(rspin_is_locked(&console_lock));
> +
> +    if ( flags & CONSOLE_SERIAL )
> +        console_serial_puts(str, len);
> +
> +    if ( flags & CONSOLE_PV )
> +        pv_console_puts(str, len);
> +
> +    if ( flags & CONSOLE_VIDEO )
> +        video_puts(str, len);
> +
> +    if ( flags & CONSOLE_DEBUG )
> +        console_debug_puts(str, len);
> +
> +    if ( flags & CONSOLE_RING )
> +        conring_puts(str, len);
> +}
> +
> +static inline void __putstr(const char *str)
> +{
> +    console_puts(str, strlen(str), CONSOLE_ALL);
> +}
> +
>  static long guest_console_write(XEN_GUEST_HANDLE_PARAM(char) buffer,
>                                  unsigned int count)
>  {
>      char kbuf[128];
>      unsigned int kcount = 0;
> +    unsigned int flags = opt_console_to_ring
> +                         ? CONSOLE_ALL : CONSOLE_DEFAULT;
>      struct domain *cd = current->domain;
>  
>      while ( count > 0 )
> @@ -640,23 +698,7 @@ static long guest_console_write(XEN_GUEST_HANDLE_PARAM(char) buffer,
>          {
>              /* Use direct console output as it could be interactive */
>              nrspin_lock_irq(&console_lock);
> -
> -            console_serial_puts(kbuf, kcount);
> -            video_puts(kbuf, kcount);
> -
> -#ifdef CONFIG_X86
> -            if ( opt_console_xen )
> -            {
> -                if ( xen_guest )
> -                    xen_hypercall_console_write(kbuf, kcount);
> -                else
> -                    xen_console_write_debug_port(kbuf, kcount);
> -            }
> -#endif
> -
> -            if ( opt_console_to_ring )
> -                conring_puts(kbuf, kcount);
> -
> +            console_puts(kbuf, kcount, flags);
>              nrspin_unlock_irq(&console_lock);
>          }
>          else
> @@ -757,28 +799,6 @@ long do_console_io(
>   * *****************************************************
>   */
>  
> -static void __putstr(const char *str)
> -{
> -    size_t len = strlen(str);
> -
> -    ASSERT(rspin_is_locked(&console_lock));
> -
> -    console_serial_puts(str, len);
> -    video_puts(str, len);
> -
> -#ifdef CONFIG_X86
> -    if ( opt_console_xen )
> -    {
> -        if ( xen_guest )
> -            xen_hypercall_console_write(str, len);
> -        else
> -            xen_console_write_debug_port(str, len);
> -    }
> -#endif
> -
> -    conring_puts(str, len);
> -}
> -
>  static int printk_prefix_check(char *p, char **pp)
>  {
>      int loglvl = -1;
> -- 
> 2.34.1
> 
> 

Re: [PATCH v1 3/3] xen/console: introduce conring_flush()

[Stefano Stabellini]

On Thu, 3 Apr 2025, dmkhn@proton.me wrote:
> From: Denis Mukhin <dmukhin@ford.com>
> 
> Introduce conring_flush() to ensure all messages kept in the internal
> console ring are sent to all physical consoles (serial, VGA (x86))
> after their initialization is completed.
> 
> Resolves: https://gitlab.com/xen-project/xen/-/issues/184
> Signed-off-by: Denis Mukhin <dmukhin@ford.com>
> ---
> The patch conflicts with
>   https://lore.kernel.org/xen-devel/20250331230508.440198-7-dmukhin@ford.com/
> in console_init_preirq()
> ---
>  xen/drivers/char/console.c | 34 ++++++++++++++++++++++++----------
>  1 file changed, 24 insertions(+), 10 deletions(-)
> 
> diff --git a/xen/drivers/char/console.c b/xen/drivers/char/console.c
> index 2618c2e47d..18eb66df89 100644
> --- a/xen/drivers/char/console.c
> +++ b/xen/drivers/char/console.c
> @@ -446,24 +446,22 @@ void console_serial_puts(const char *s, size_t nr)
>          serial_puts(sercon_handle, s, nr);
>  }
>  
> -static void cf_check dump_console_ring_key(unsigned char key)
> +/*
> + * Flush contents of the conring to the physical console devices.
> + */
> +static int conring_flush(void)
>  {
>      uint32_t idx, len, sofar, c;
>      unsigned int order;
>      char *buf;
> +    unsigned long flags;
>  
> -    printk("'%c' pressed -> dumping console ring buffer (dmesg)\n", key);
> -
> -    /* create a buffer in which we'll copy the ring in the correct
> -       order and NUL terminate */
>      order = get_order_from_bytes(conring_size + 1);
>      buf = alloc_xenheap_pages(order, 0);
>      if ( buf == NULL )
> -    {
> -        printk("unable to allocate memory!\n");
> -        return;
> -    }
> +        return -ENOMEM;
>  
> +    flags = console_lock_recursive_irqsave();

This patch is OK but it looks like this bit might have to change to
rebase on top of a modified patch #2


>      c = conringc;
>      sofar = 0;
>      while ( (c != conringp) )
> @@ -478,8 +476,21 @@ static void cf_check dump_console_ring_key(unsigned char key)
>      }
>  
>      console_puts(buf, sofar, CONSOLE_SERIAL | CONSOLE_VIDEO | CONSOLE_PV);
> +    console_unlock_recursive_irqrestore(flags);
>  
>      free_xenheap_pages(buf, order);
> +
> +    return 0;
> +}
> +
> +static void cf_check conring_dump_keyhandler(unsigned char key)
> +{
> +    int rc;
> +
> +    printk("'%c' pressed -> dumping console ring buffer (dmesg)\n", key);
> +    rc = conring_flush();
> +    if ( rc )
> +        printk("failed to dump console ring buffer: %d\n", rc);
>  }
>  
>  /*
> @@ -1044,6 +1055,9 @@ void __init console_init_preirq(void)
>      serial_set_rx_handler(sercon_handle, serial_rx);
>      pv_console_set_rx_handler(serial_rx);
>  
> +    /* NB: send conring contents to all enabled physical consoles, if any */
> +    conring_flush();
> +
>      /* HELLO WORLD --- start-of-day banner text. */
>      nrspin_lock(&console_lock);
>      __putstr(xen_banner());
> @@ -1134,7 +1148,7 @@ void __init console_endboot(void)
>      if ( opt_conswitch[1] == 'x' )
>          console_rx = max_console_rx;
>  
> -    register_keyhandler('w', dump_console_ring_key,
> +    register_keyhandler('w', conring_dump_keyhandler,
>                          "synchronously dump console ring buffer (dmesg)", 0);
>      register_irq_keyhandler('+', &do_inc_thresh,
>                              "increase log level threshold", 0);
> -- 
> 2.34.1
> 
> 

Re: [PATCH v1 2/3] xen/console: introduce console_puts()

[dmkhn]

On Fri, Apr 25, 2025 at 03:47:53PM -0700, Stefano Stabellini wrote:
> On Thu, 3 Apr 2025, dmkhn@proton.me wrote:
> > From: Denis Mukhin <dmukhin@ford.com>
> >
> > guest_console_write() duplicates the code from __putstr(), eliminate code
> > duplication.
> >
> > Introduce console_puts() for writing a buffer to console devices.
> >
> > Also, introduce internal console flags to control which console devices
> > should be used.
> >
> > Signed-off-by: Denis Mukhin <dmukhin@ford.com>
> > ---
> >  xen/drivers/char/console.c | 112 ++++++++++++++++++++++---------------
> >  1 file changed, 66 insertions(+), 46 deletions(-)
> >
> > diff --git a/xen/drivers/char/console.c b/xen/drivers/char/console.c
> > index aaa97088aa..2618c2e47d 100644
> > --- a/xen/drivers/char/console.c
> > +++ b/xen/drivers/char/console.c
> > @@ -41,6 +41,20 @@
> >  #include <asm/vpl011.h>
> >  #endif
> >
> > +/* Internal console flags. */
> > +enum {
> > +    CONSOLE_SERIAL  = BIT(0, U),    /* Use serial device. */
> > +    CONSOLE_PV      = BIT(1, U),    /* Use PV console. */
> > +    CONSOLE_VIDEO   = BIT(2, U),    /* Use video device. */
> > +    CONSOLE_DEBUG   = BIT(3, U),    /* Use debug device. */
> > +    CONSOLE_RING    = BIT(4, U),    /* Use console ring. */
> > +    CONSOLE_DEFAULT = CONSOLE_SERIAL | CONSOLE_PV | CONSOLE_VIDEO |
> > +                      CONSOLE_DEBUG,
> > +    CONSOLE_ALL     = CONSOLE_DEFAULT | CONSOLE_RING,
> > +};
> > +
> > +static void console_puts(const char *str, size_t len, unsigned int flags);
> > +
> >  /* console: comma-separated list of console outputs. */
> >  static char __initdata opt_console[30] = OPT_CONSOLE_STR;
> >  string_param("console", opt_console);
> > @@ -338,8 +352,6 @@ static bool console_locks_busted;
> >
> >  static void conring_puts(const char *str, size_t len)
> >  {
> > -    ASSERT(rspin_is_locked(&console_lock));
> > -
> >      while ( len-- )
> >          conring[CONRING_IDX_MASK(conringp++)] = *str++;
> >
> > @@ -432,9 +444,6 @@ void console_serial_puts(const char *s, size_t nr)
> >          serial_steal_fn(s, nr);
> >      else
> >          serial_puts(sercon_handle, s, nr);
> > -
> > -    /* Copy all serial output into PV console */
> > -    pv_console_puts(s, nr);
> >  }
> >
> >  static void cf_check dump_console_ring_key(unsigned char key)
> > @@ -468,8 +477,7 @@ static void cf_check dump_console_ring_key(unsigned char key)
> >          c += len;
> >      }
> >
> > -    console_serial_puts(buf, sofar);
> > -    video_puts(buf, sofar);
> > +    console_puts(buf, sofar, CONSOLE_SERIAL | CONSOLE_VIDEO | CONSOLE_PV);
> 
> Actually I take back the R-b. It looks like this change is breaking
> because console_puts now requires the console_lock to be held, while
> here the console_lock is not held.

Yes, the locking is wrong, thanks for the catch!
Lock adjustment from Patch 3 should be moved to Patch 2 (here).

> 
> If I am not mistaken if you try to use the 'w' key with this patch
> applied you'll hit the ASSERT at the beginning of console_puts
> 
> 
> >      free_xenheap_pages(buf, order);
> >  }
> > @@ -618,11 +626,61 @@ static inline void xen_console_write_debug_port(const char *buf, size_t len)
> >  }
> >  #endif
> >
> > +static inline void console_debug_puts(const char *str, size_t len)
> > +{
> > +#ifdef CONFIG_X86
> > +    if ( opt_console_xen )
> > +    {
> > +        if ( xen_guest )
> > +            xen_hypercall_console_write(str, len);
> > +        else
> > +            xen_console_write_debug_port(str, len);
> > +    }
> > +#endif
> > +}
> > +
> > +/*
> > + * Write buffer to all enabled console devices.
> > + *
> > + * That will handle all possible scenarios working w/ console
> > + * - physical console (serial console, VGA console (x86 only));
> > + * - PV console;
> > + * - debug console (x86 only): debug I/O port or __HYPERVISOR_console_io
> > + *   hypercall;
> > + * - console ring.
> > + */
> > +static void console_puts(const char *str, size_t len, unsigned int flags)
> > +{
> > +    ASSERT(rspin_is_locked(&console_lock));
> > +
> > +    if ( flags & CONSOLE_SERIAL )
> > +        console_serial_puts(str, len);
> > +
> > +    if ( flags & CONSOLE_PV )
> > +        pv_console_puts(str, len);
> > +
> > +    if ( flags & CONSOLE_VIDEO )
> > +        video_puts(str, len);
> > +
> > +    if ( flags & CONSOLE_DEBUG )
> > +        console_debug_puts(str, len);
> > +
> > +    if ( flags & CONSOLE_RING )
> > +        conring_puts(str, len);
> > +}
> > +
> > +static inline void __putstr(const char *str)
> > +{
> > +    console_puts(str, strlen(str), CONSOLE_ALL);
> > +}
> > +
> >  static long guest_console_write(XEN_GUEST_HANDLE_PARAM(char) buffer,
> >                                  unsigned int count)
> >  {
> >      char kbuf[128];
> >      unsigned int kcount = 0;
> > +    unsigned int flags = opt_console_to_ring
> > +                         ? CONSOLE_ALL : CONSOLE_DEFAULT;
> >      struct domain *cd = current->domain;
> >
> >      while ( count > 0 )
> > @@ -640,23 +698,7 @@ static long guest_console_write(XEN_GUEST_HANDLE_PARAM(char) buffer,
> >          {
> >              /* Use direct console output as it could be interactive */
> >              nrspin_lock_irq(&console_lock);
> > -
> > -            console_serial_puts(kbuf, kcount);
> > -            video_puts(kbuf, kcount);
> > -
> > -#ifdef CONFIG_X86
> > -            if ( opt_console_xen )
> > -            {
> > -                if ( xen_guest )
> > -                    xen_hypercall_console_write(kbuf, kcount);
> > -                else
> > -                    xen_console_write_debug_port(kbuf, kcount);
> > -            }
> > -#endif
> > -
> > -            if ( opt_console_to_ring )
> > -                conring_puts(kbuf, kcount);
> > -
> > +            console_puts(kbuf, kcount, flags);
> >              nrspin_unlock_irq(&console_lock);
> >          }
> >          else
> > @@ -757,28 +799,6 @@ long do_console_io(
> >   * *****************************************************
> >   */
> >
> > -static void __putstr(const char *str)
> > -{
> > -    size_t len = strlen(str);
> > -
> > -    ASSERT(rspin_is_locked(&console_lock));
> > -
> > -    console_serial_puts(str, len);
> > -    video_puts(str, len);
> > -
> > -#ifdef CONFIG_X86
> > -    if ( opt_console_xen )
> > -    {
> > -        if ( xen_guest )
> > -            xen_hypercall_console_write(str, len);
> > -        else
> > -            xen_console_write_debug_port(str, len);
> > -    }
> > -#endif
> > -
> > -    conring_puts(str, len);
> > -}
> > -
> >  static int printk_prefix_check(char *p, char **pp)
> >  {
> >      int loglvl = -1;
> > --
> > 2.34.1
> >
> >

Re: [PATCH v1 1/3] xen/console: cleanup conring management

[Jan Beulich]

On 26.04.2025 00:18, Stefano Stabellini wrote:
> On Thu, 3 Apr 2025, dmkhn@proton.me wrote:
>> From: Denis Mukhin <dmukhin@ford.com>
>>
>> Move console_locks_busted handling inside conring_puts() to remove
>> tasklet code duplication.
>>
>> Signed-off-by: Denis Mukhin <dmukhin@ford.com>
> 
> This patch is a good cleanup but makes one functional change: previously
> guest_console_write would always call tasklet_schedule. Now, it only
> calls tasklet_schedule if !console_locks_busted.
> 
> On ARM, we don't call console_force_unlock and never set
> console_locks_busted. It makes no difference.
> 
> On x86, there are a few callers of console_force_unlock, so it would
> make a difference. However, looking at the callers, it seems to me that
> the change is for the better and better aligns the code with the
> intention behind console_force_unlock.

Denis, I see you submitted v2 without any adjustment to the description.
With Stefano having pointed out the aspect, it should have been pretty
clear that such a (kind of hidden) functional change wants justifying.

Furthermore, you added Stefano's R-b without any hint towards the extra
request he had put up above wrt x86.

Jan

Re: [PATCH v1 1/3] xen/console: cleanup conring management

[dmkhn]

On Mon, Apr 28, 2025 at 08:47:01AM +0200, Jan Beulich wrote:
> On 26.04.2025 00:18, Stefano Stabellini wrote:
> > On Thu, 3 Apr 2025, dmkhn@proton.me wrote:
> >> From: Denis Mukhin <dmukhin@ford.com>
> >>
> >> Move console_locks_busted handling inside conring_puts() to remove
> >> tasklet code duplication.
> >>
> >> Signed-off-by: Denis Mukhin <dmukhin@ford.com>
> >
> > This patch is a good cleanup but makes one functional change: previously
> > guest_console_write would always call tasklet_schedule. Now, it only
> > calls tasklet_schedule if !console_locks_busted.
> >
> > On ARM, we don't call console_force_unlock and never set
> > console_locks_busted. It makes no difference.
> >
> > On x86, there are a few callers of console_force_unlock, so it would
> > make a difference. However, looking at the callers, it seems to me that
> > the change is for the better and better aligns the code with the
> > intention behind console_force_unlock.
> 
> Denis, I see you submitted v2 without any adjustment to the description.
> With Stefano having pointed out the aspect, it should have been pretty
> clear that such a (kind of hidden) functional change wants justifying.
> 
> Furthermore, you added Stefano's R-b without any hint towards the extra
> request he had put up above wrt x86.

Sorry for that, I will update the descriptions in the next iteration.
I am preparing the update, which reverts the locking part completely. 

> 
> Jan