console: Do not duplicate early printk messages on conring flush

[PATCH] console: Do not duplicate early printk messages on conring flush

Posted by Michal Orzel 4 months, 2 weeks ago

Commit f6d1bfa16052 introduced flushing conring in console_init_preirq().
However, when CONFIG_EARLY_PRINTK is enabled, the early boot messages
had already been sent to serial before main console initialization. This
results in all the early boot messages being duplicated.

Change conring_flush() to accept argument listing devices to which to
flush conring. We don't want to send to serial at console initialization
when using early printk, but we want these messages to be send at conring
dump triggered by keyhandler.

Fixes: f6d1bfa16052 ("xen/console: introduce conring_flush()")
Signed-off-by: Michal Orzel <michal.orzel@amd.com>
---
 xen/drivers/char/console.c | 20 ++++++++++++++------
 1 file changed, 14 insertions(+), 6 deletions(-)

diff --git a/xen/drivers/char/console.c b/xen/drivers/char/console.c
index 9a9836ba91e7..5879e31786ba 100644
--- a/xen/drivers/char/console.c
+++ b/xen/drivers/char/console.c
@@ -453,9 +453,9 @@ void console_serial_puts(const char *s, size_t nr)
 }
 
 /*
- * Flush contents of the conring to the physical console devices.
+ * Flush contents of the conring to the selected console devices.
  */
-static int conring_flush(void)
+static int conring_flush(unsigned int flags)
 {
     uint32_t idx, len, sofar, c;
     unsigned int order;
@@ -479,7 +479,7 @@ static int conring_flush(void)
         c += len;
     }
 
-    console_send(buf, sofar, CONSOLE_SERIAL | CONSOLE_VIDEO | CONSOLE_PV);
+    console_send(buf, sofar, flags);
 
     free_xenheap_pages(buf, order);
 
@@ -491,7 +491,7 @@ 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();
+    rc = conring_flush(CONSOLE_SERIAL | CONSOLE_VIDEO | CONSOLE_PV);
     if ( rc )
         printk("failed to dump console ring buffer: %d\n", rc);
 }
@@ -1042,6 +1042,7 @@ void __init console_init_preirq(void)
 {
     char *p;
     int sh;
+    unsigned int flags = CONSOLE_SERIAL | CONSOLE_VIDEO | CONSOLE_PV;
 
     serial_init_preirq();
 
@@ -1084,8 +1085,15 @@ 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();
+    /*
+     * NB: send conring contents to all enabled physical consoles, if any.
+     * Skip serial if CONFIG_EARLY_PRINTK is enabled, which means the early
+     * messages have already been sent to serial.
+     */
+    if ( IS_ENABLED(CONFIG_EARLY_PRINTK) )
+        flags &= ~CONSOLE_SERIAL;
+
+    conring_flush(flags);
 
     /* HELLO WORLD --- start-of-day banner text. */
     nrspin_lock(&console_lock);
-- 
2.25.1

Re: [PATCH] console: Do not duplicate early printk messages on conring flush

Posted by dmkhn@proton.me 4 months, 2 weeks ago

On Tue, Jun 17, 2025 at 09:19:40AM +0200, Michal Orzel wrote:
> Commit f6d1bfa16052 introduced flushing conring in console_init_preirq().
> However, when CONFIG_EARLY_PRINTK is enabled, the early boot messages
> had already been sent to serial before main console initialization. This
> results in all the early boot messages being duplicated.
> 
> Change conring_flush() to accept argument listing devices to which to
> flush conring. We don't want to send to serial at console initialization
> when using early printk, but we want these messages to be send at conring
> dump triggered by keyhandler.
> 
> Fixes: f6d1bfa16052 ("xen/console: introduce conring_flush()")
> Signed-off-by: Michal Orzel <michal.orzel@amd.com>

Reviewed-by: Denis Mukhin <dmukhin@ford.com>

> ---
>  xen/drivers/char/console.c | 20 ++++++++++++++------
>  1 file changed, 14 insertions(+), 6 deletions(-)
> 
> diff --git a/xen/drivers/char/console.c b/xen/drivers/char/console.c
> index 9a9836ba91e7..5879e31786ba 100644
> --- a/xen/drivers/char/console.c
> +++ b/xen/drivers/char/console.c
> @@ -453,9 +453,9 @@ void console_serial_puts(const char *s, size_t nr)
>  }
> 
>  /*
> - * Flush contents of the conring to the physical console devices.
> + * Flush contents of the conring to the selected console devices.
>   */
> -static int conring_flush(void)
> +static int conring_flush(unsigned int flags)
>  {
>      uint32_t idx, len, sofar, c;
>      unsigned int order;
> @@ -479,7 +479,7 @@ static int conring_flush(void)
>          c += len;
>      }
> 
> -    console_send(buf, sofar, CONSOLE_SERIAL | CONSOLE_VIDEO | CONSOLE_PV);
> +    console_send(buf, sofar, flags);
> 
>      free_xenheap_pages(buf, order);
> 
> @@ -491,7 +491,7 @@ 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();
> +    rc = conring_flush(CONSOLE_SERIAL | CONSOLE_VIDEO | CONSOLE_PV);
>      if ( rc )
>          printk("failed to dump console ring buffer: %d\n", rc);
>  }
> @@ -1042,6 +1042,7 @@ void __init console_init_preirq(void)
>  {
>      char *p;
>      int sh;
> +    unsigned int flags = CONSOLE_SERIAL | CONSOLE_VIDEO | CONSOLE_PV;
> 
>      serial_init_preirq();
> 
> @@ -1084,8 +1085,15 @@ 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();
> +    /*
> +     * NB: send conring contents to all enabled physical consoles, if any.
> +     * Skip serial if CONFIG_EARLY_PRINTK is enabled, which means the early
> +     * messages have already been sent to serial.
> +     */
> +    if ( IS_ENABLED(CONFIG_EARLY_PRINTK) )
> +        flags &= ~CONSOLE_SERIAL;
> +
> +    conring_flush(flags);
> 
>      /* HELLO WORLD --- start-of-day banner text. */
>      nrspin_lock(&console_lock);
> --
> 2.25.1
> 
>

Re: [PATCH] console: Do not duplicate early printk messages on conring flush

Posted by Stefano Stabellini 4 months, 2 weeks ago

On Tue, 17 Jun 2025, Michal Orzel wrote:
> Commit f6d1bfa16052 introduced flushing conring in console_init_preirq().
> However, when CONFIG_EARLY_PRINTK is enabled, the early boot messages
> had already been sent to serial before main console initialization. This
> results in all the early boot messages being duplicated.
> 
> Change conring_flush() to accept argument listing devices to which to
> flush conring. We don't want to send to serial at console initialization
> when using early printk, but we want these messages to be send at conring
> dump triggered by keyhandler.
> 
> Fixes: f6d1bfa16052 ("xen/console: introduce conring_flush()")
> Signed-off-by: Michal Orzel <michal.orzel@amd.com>

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


> ---
>  xen/drivers/char/console.c | 20 ++++++++++++++------
>  1 file changed, 14 insertions(+), 6 deletions(-)
> 
> diff --git a/xen/drivers/char/console.c b/xen/drivers/char/console.c
> index 9a9836ba91e7..5879e31786ba 100644
> --- a/xen/drivers/char/console.c
> +++ b/xen/drivers/char/console.c
> @@ -453,9 +453,9 @@ void console_serial_puts(const char *s, size_t nr)
>  }
>  
>  /*
> - * Flush contents of the conring to the physical console devices.
> + * Flush contents of the conring to the selected console devices.
>   */
> -static int conring_flush(void)
> +static int conring_flush(unsigned int flags)
>  {
>      uint32_t idx, len, sofar, c;
>      unsigned int order;
> @@ -479,7 +479,7 @@ static int conring_flush(void)
>          c += len;
>      }
>  
> -    console_send(buf, sofar, CONSOLE_SERIAL | CONSOLE_VIDEO | CONSOLE_PV);
> +    console_send(buf, sofar, flags);
>  
>      free_xenheap_pages(buf, order);
>  
> @@ -491,7 +491,7 @@ 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();
> +    rc = conring_flush(CONSOLE_SERIAL | CONSOLE_VIDEO | CONSOLE_PV);
>      if ( rc )
>          printk("failed to dump console ring buffer: %d\n", rc);
>  }
> @@ -1042,6 +1042,7 @@ void __init console_init_preirq(void)
>  {
>      char *p;
>      int sh;
> +    unsigned int flags = CONSOLE_SERIAL | CONSOLE_VIDEO | CONSOLE_PV;
>  
>      serial_init_preirq();
>  
> @@ -1084,8 +1085,15 @@ 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();
> +    /*
> +     * NB: send conring contents to all enabled physical consoles, if any.
> +     * Skip serial if CONFIG_EARLY_PRINTK is enabled, which means the early
> +     * messages have already been sent to serial.
> +     */
> +    if ( IS_ENABLED(CONFIG_EARLY_PRINTK) )
> +        flags &= ~CONSOLE_SERIAL;
> +
> +    conring_flush(flags);
>  
>      /* HELLO WORLD --- start-of-day banner text. */
>      nrspin_lock(&console_lock);
> -- 
> 2.25.1
>

Re: [PATCH] console: Do not duplicate early printk messages on conring flush

Posted by Jan Beulich 4 months, 2 weeks ago

On 17.06.2025 09:19, Michal Orzel wrote:
> @@ -1084,8 +1085,15 @@ 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();
> +    /*
> +     * NB: send conring contents to all enabled physical consoles, if any.
> +     * Skip serial if CONFIG_EARLY_PRINTK is enabled, which means the early
> +     * messages have already been sent to serial.
> +     */
> +    if ( IS_ENABLED(CONFIG_EARLY_PRINTK) )
> +        flags &= ~CONSOLE_SERIAL;

Does EARLY_PRINTK=y alone guarantee everything was output? That is, is there
no chance that early-console init may have failed? (Sorry, I don't know much
about early-console in Xen, as that's Arm-only.)

Jan

Re: [PATCH] console: Do not duplicate early printk messages on conring flush

Posted by Orzel, Michal 4 months, 2 weeks ago


On 17/06/2025 11:26, Jan Beulich wrote:
> On 17.06.2025 09:19, Michal Orzel wrote:
>> @@ -1084,8 +1085,15 @@ 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();
>> +    /*
>> +     * NB: send conring contents to all enabled physical consoles, if any.
>> +     * Skip serial if CONFIG_EARLY_PRINTK is enabled, which means the early
>> +     * messages have already been sent to serial.
>> +     */
>> +    if ( IS_ENABLED(CONFIG_EARLY_PRINTK) )
>> +        flags &= ~CONSOLE_SERIAL;
> 
> Does EARLY_PRINTK=y alone guarantee everything was output? That is, is there
> no chance that early-console init may have failed? (Sorry, I don't know much
> about early-console in Xen, as that's Arm-only.)
There is no init function for the early printk and no function related to early
printk returns something.

~Michal

Re: [PATCH] console: Do not duplicate early printk messages on conring flush

Posted by Luca Fancellu 4 months, 2 weeks ago

Hi Michal,

> On 17 Jun 2025, at 08:19, Michal Orzel <michal.orzel@amd.com> wrote:
> 
> Commit f6d1bfa16052 introduced flushing conring in console_init_preirq().
> However, when CONFIG_EARLY_PRINTK is enabled, the early boot messages
> had already been sent to serial before main console initialization. This
> results in all the early boot messages being duplicated.
> 
> Change conring_flush() to accept argument listing devices to which to
> flush conring. We don't want to send to serial at console initialization
> when using early printk, but we want these messages to be send at conring

NIT: s/send/sent/

> dump triggered by keyhandler.
> 
> Fixes: f6d1bfa16052 ("xen/console: introduce conring_flush()")
> Signed-off-by: Michal Orzel <michal.orzel@amd.com>
> ---
> xen/drivers/char/console.c | 20 ++++++++++++++------
> 1 file changed, 14 insertions(+), 6 deletions(-)
> 

This LGTM, I’ve also tested with fvp-base for arm64 with CONFIG_EARLY_PRINTK.

Reviewed-by: Luca Fancellu <luca.fancellu@arm.com>
Tested-by: Luca Fancellu <luca.fancellu@arm.com>

Cheers,
Luca