Re: [PATCH v2 27/35] xen/console: flush console ring to physical console

[Roger Pau Monné <roger.pau@xxxxxxxxxx>]

On Thu, Dec 05, 2024 at 08:41:57PM -0800, Denis Mukhin via B4 Relay wrote:
> From: Denis Mukhin <dmukhin@xxxxxxxx>
> 
> Messages printed before the serial and VGA consoles are initialized end up in
> the internal console buffer (conring). Flush contents of conring to all
> external consoles after external consoles are fully initialied.
> 
> Link: https://gitlab.com/xen-project/xen/-/issues/184
> Signed-off-by: Denis Mukhin <dmukhin@xxxxxxxx>
> ---
>  xen/drivers/char/console.c | 54 
> +++++++++++++++++++++++++++++++++-------------
>  1 file changed, 39 insertions(+), 15 deletions(-)
> 
> diff --git a/xen/drivers/char/console.c b/xen/drivers/char/console.c
> index 
> 581ee22b85302a54db5b9d5d28e8b2d689d31403..a26daee9c4c4b1134d0ae3d105ffdb656340b6df
>  100644
> --- a/xen/drivers/char/console.c
> +++ b/xen/drivers/char/console.c
> @@ -426,23 +426,35 @@ void console_serial_puts(const char *s, size_t nr)
>      pv_console_puts(s, nr);
>  }
>  
> -static void cf_check dump_console_ring_key(unsigned char key)
> +/*
> + * Write characters to physical console(s).
> + * That covers:
> + * - serial console;
> + * - video output.
> + */
> +static void console_puts(const char *str, size_t len)
> +{
> +    ASSERT(rspin_is_locked(&console_lock));
> +
> +    console_serial_puts(str, len);
> +    video_puts(str, len);
> +}
> +
> +/*
> + * Flush contents of the conring to the physical console devices.
> + */
> +static int console_flush(void)
>  {
>      uint32_t idx, len, sofar, c;
>      unsigned int order;
>      char *buf;
>  
> -    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;
> +
> +    nrspin_lock_irq(&console_lock);
>  
>      c = conringc;
>      sofar = 0;
> @@ -457,10 +469,23 @@ 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);
> +
> +    nrspin_unlock_irq(&console_lock);
>  
>      free_xenheap_pages(buf, order);
> +
> +    return 0;
> +}
> +
> +static void cf_check dump_console_ring_key(unsigned char key)
> +{
> +    int rc;
> +
> +    printk("'%c' pressed -> dumping console ring buffer (dmesg)\n", key);
> +    rc = console_flush();
> +    if ( rc )
> +        printk("failed to dump console ring buffer: %d\n", rc);
>  }
>  
>  /*
> @@ -707,10 +732,7 @@ static inline void xen_console_write(const char *str, 
> size_t len)
>   */
>  static void console_write(const char *str, size_t len)
>  {
> -    ASSERT(rspin_is_locked(&console_lock));
> -
> -    console_serial_puts(str, len);
> -    video_puts(str, len);
> +    console_puts(str, len);
>  
>      if ( opt_console_xen )
>          xen_console_write(str, len);
> @@ -1177,6 +1199,8 @@ void __init console_endboot(void)
>  
>      video_endboot();
>  
> +    /* NB: send conring contents to all enabled physical consoles, if any */
> +    console_flush();

This is way too late: at this point Xen has already finished booting,
and is about to handover execution to dom0.  Flushing here will result
in duplicating almost all Xen output already printed?

The flush needs to be done inside of console_init_preirq(), just
before printing xen_banner() I think.  This sequentially after the
console has been initialized.  Otherwise you are just duplicating
messages.

Thanks, Roger.