Re: [PATCH v3 16/24] xen/console: introduce console_write()

[Jan Beulich <jbeulich@xxxxxxxx>]

On 04.01.2025 02:58, Denis Mukhin via B4 Relay wrote:
> --- a/xen/drivers/char/console.c
> +++ b/xen/drivers/char/console.c
> @@ -40,6 +40,11 @@
>  #include <asm/guest.h>
>  #endif
>  
> +/* Console flags. */
> +enum {
> +    CONSOLE_RING    = BIT(0, U),
> +};

These aren't console flags, but flags passed to the new console_write().
The comment wants adjusting accordingly, and I guess the enum would also
best move immediately ahead of the function.

> @@ -636,6 +641,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);
> +}

This doesn't really need to be a separate function, does it?

> @@ -644,8 +659,47 @@ 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);
> +}

Nor does this, I suppose. The more that the sole call to here ...

>  #endif
>  
> +/*
> + * Write characters to console.
> + *
> + * That will handle all possible scenarios working w/ console
> + * - serial console;
> + * - VGA console (x86 only);
> + * - __HYPERVISOR_console_io hypercall (x86 only);
> + * - debug I/O port (x86 only);
> + * - PV console.
> + */
> +static void console_write(const char *str, size_t len, unsigned int flags)
> +{
> +    ASSERT(rspin_is_locked(&console_lock));
> +
> +    console_serial_puts(str, len);
> +    video_puts(str, len);
> +
> +#ifdef CONFIG_X86
> +    if ( opt_console_xen )
> +        xen_console_write(str, len);
> +#endif

... continues to sit in an #ifdef.

> @@ -666,28 +720,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 */

This comment looks like being removed with no replacement? Why?

>              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, !!opt_console_to_ring);

Here you assume CONSOLE_RING is bit 0. Please don't create such (then
also un-annotated) implicit dependencies.

> @@ -788,33 +822,6 @@ long do_console_io(
>   * *****************************************************
>   */
>  
> -static bool console_locks_busted;
> -
> -static void __putstr(const char *str)
> -{

Please note the comment up from here, the tail of which is visible in
context. I wonder whether the new function wouldn't better live here,
then also reducing the diff quite a bit. Requiring a forward decl
for this to work isn't very nice, but a reasonable price to pay, I
think.

Jan