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

[dmkhn@xxxxxxxxx]

On Fri, Apr 25, 2025 at 03:47:53PM -0700, Stefano Stabellini wrote:
> On Thu, 3 Apr 2025, dmkhn@xxxxxxxxx wrote:
> > From: Denis Mukhin <dmukhin@xxxxxxxx>
> >
> > 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@xxxxxxxx>
> > ---
> >  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
> >
> >