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

[Denis Mukhin <dmkhn@xxxxxxxxx>]

On Thursday, December 12th, 2024 at 6:21 AM, Roger Pau Monné 
<roger.pau@xxxxxxxxxx> wrote:

>
>
> 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?

Yes, indeed; fixed.

>
> 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.

Fixed.

>
> Thanks, Roger.