Re: [PATCH v2 23/35] xen/console: introduce console_write()

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

On Thu, Dec 05, 2024 at 08:41:53PM -0800, Denis Mukhin via B4 Relay wrote:
> From: Denis Mukhin <dmukhin@xxxxxxxx>
> 
> PV Linux kernel uses HYPERVISOR_console_io hypercall for early console which
> ends up being handled by Xen's console driver's guest_console_write().
> 
> guest_console_write() duplicates the code from __putstr(), elimitate code
> duplication.

It might be better to split the code that unifies
guest_console_write() and __putstr() as a non-functional change.
While the introduction of use_conring is likely a functional change.

> Signed-off-by: Denis Mukhin <dmukhin@xxxxxxxx>
> ---
>  xen/drivers/char/console.c | 97 
> +++++++++++++++++++++++++---------------------
>  1 file changed, 53 insertions(+), 44 deletions(-)
> 
> diff --git a/xen/drivers/char/console.c b/xen/drivers/char/console.c
> index 
> ce3639a4cdcda00ea63e3bf119bc3b242cbfdf6a..115967d179998cba4a81578caba09db4e4aca7f7
>  100644
> --- a/xen/drivers/char/console.c
> +++ b/xen/drivers/char/console.c
> @@ -63,6 +63,8 @@ static const char __initconst warning_sync_console[] =
>      "However it can introduce SIGNIFICANT latencies and affect\n"
>      "timekeeping. It is NOT recommended for production use!\n";
>  
> +/* Flag: use conring for early console; switches to opt_console_to_ring */
> +static bool __read_mostly use_conring = true;

__ro_after_init instead of __read_mostly.

>  /* console_to_ring: send guest (incl. dom 0) console data to console ring. */
>  static bool __read_mostly opt_console_to_ring;
>  boolean_param("console_to_ring", opt_console_to_ring);
> @@ -661,6 +663,16 @@ static void cf_check notify_dom0_con_ring(void *unused)
>  static DECLARE_SOFTIRQ_TASKLET(notify_dom0_con_ring_tasklet,
>                                 notify_dom0_con_ring, NULL);
>  
> +static bool console_locks_busted;
> +
> +static void conring_write(const char *str, size_t len)
> +{
> +    conring_puts(str, len);
> +
> +    if ( !console_locks_busted )
> +        tasklet_schedule(&notify_dom0_con_ring_tasklet);
> +}
> +
>  #ifdef CONFIG_X86
>  static inline void xen_console_write_debug_port(const char *buf, size_t len)
>  {
> @@ -669,8 +681,44 @@ static inline void xen_console_write_debug_port(const 
> char *buf, size_t len)
>                     : "=&S" (tmp), "=&c" (tmp)
>                     : "0" (buf), "1" (len), "d" (XEN_HVM_DEBUGCONS_IOPORT) );
>  }
> +
> +static void xen_console_write(const char *str, size_t len)
> +{
> +    if ( xen_guest )
> +        xen_hypercall_console_write(str, len);
> +    else
> +        xen_console_write_debug_port(str, len);
> +}
> +#else
> +static inline void xen_console_write(const char *str, size_t len)
> +{

opt_console_xen would only be set on x86 with the current command line
parsing done in console_init_preirq(), so you could add an
ASSERT_UNREACHABLE() here.

> +}
>  #endif
>  
> +/*
> + * Write characters to console.
> + *
> + * That will handle all possible scenarios working w/ console
> + * - serial console;
> + * - video output;
> + * - __HYPERVISOR_console_io hypercall (x86 only);
> + * - debug I/O port (x86 only);
> + * - forward to Xen event channel.

"Xen event channel" is not the correct term.  I would use "PV
console".  The event channel is just used to send the notification.

> + */
> +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);
> +
> +    if ( opt_console_xen )
> +        xen_console_write(str, len);

Are you sure this builds?  opt_console_xen is only defined on x86, and
AFAICT console_write() is generic.  AFAICT you need to keep the X86
preprocessor guards, or alternatively do something like:

#define opt_console_xen false

For non-x86 arches in xen/console.h

> +
> +    if ( use_conring )
> +        conring_write(str, len);
> +}
> +
>  static long guest_console_write(XEN_GUEST_HANDLE_PARAM(char) buffer,
>                                  unsigned int count)
>  {
> @@ -691,28 +739,8 @@ static long 
> guest_console_write(XEN_GUEST_HANDLE_PARAM(char) buffer,
>  
>          if ( is_hardware_domain(cd) )
>          {
> -            /* 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);
> -                tasklet_schedule(&notify_dom0_con_ring_tasklet);
> -            }
> -
> +            console_write(kbuf, kcount);
>              nrspin_unlock_irq(&console_lock);
>          }
>          else
> @@ -813,31 +841,9 @@ long do_console_io(
>   * *****************************************************
>   */
>  
> -static bool console_locks_busted;
> -
>  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);
> -
> -    if ( !console_locks_busted )
> -        tasklet_schedule(&notify_dom0_con_ring_tasklet);
> +    console_write(str, strlen(str));
>  }
>  
>  static int printk_prefix_check(char *p, char **pp)
> @@ -1171,6 +1177,9 @@ void __init console_endboot(void)
>  
>      video_endboot();
>  
> +    use_conring = opt_console_to_ring;
> +    smp_wmb();

Do you really need the barrier?  If so it would need a comment
describing exactly why it's needed.  I don't think it's possible for
the write to be reordered past the return of the function, which would
be enough to ensure correctness?

Thanks, Roger.