Re: [PATCH v3] xen/console: introduce domain_console struct

[Jan Beulich <jbeulich@xxxxxxxx>]

On 23.06.2025 03:34, dmkhn@xxxxxxxxx wrote:
> @@ -769,22 +770,23 @@ static long 
> guest_console_write(XEN_GUEST_HANDLE_PARAM(char) buffer,
>              } while ( --kcount > 0 );
>  
>              *kout = '\0';
> -            spin_lock(&cd->pbuf_lock);
> +            spin_lock(&cons->lock);
>              kcount = kin - kbuf;
>              if ( c != '\n' &&
> -                 (cd->pbuf_idx + (kout - kbuf) < (DOMAIN_PBUF_SIZE - 1)) )
> +                 cons->idx + kout - kbuf < DOMAIN_CONSOLE_BUF_SIZE - 1 )

The dropping of the parentheses around the pointer subtraction is
problematic: You open up UB opportunities this way, as evaluation order
changes. We had UBSAN trip up on similar constructs already in the past.

>              {
>                  /* buffer the output until a newline */
> -                memcpy(cd->pbuf + cd->pbuf_idx, kbuf, kout - kbuf);
> -                cd->pbuf_idx += (kout - kbuf);
> +                memcpy(cons->buf + cons->idx, kbuf, kout - kbuf);
> +                cons->idx += (kout - kbuf);

Here, otoh, the parentheses could be dropped while touching the line.

>              }
>              else
>              {
> -                cd->pbuf[cd->pbuf_idx] = '\0';
> -                guest_printk(cd, XENLOG_G_DEBUG "%s%s\n", cd->pbuf, kbuf);
> -                cd->pbuf_idx = 0;
> +                cons->buf[cons->idx] = '\0';
> +                guest_printk(cd,
> +                             XENLOG_G_DEBUG "%s%s\n", cons->buf, kbuf);

There's no need to wrap lines here, is there?

> --- a/xen/include/xen/sched.h
> +++ b/xen/include/xen/sched.h
> @@ -371,6 +371,20 @@ struct evtchn_port_ops;
>  
>  #define MAX_NR_IOREQ_SERVERS 8
>  
> +/* Arbitrary value. */
> +#define DOMAIN_CONSOLE_BUF_SIZE 256

Nit: The value isn't entirely arbitrary.

> +/* Domain console settings. */
> +struct domain_console {
> +    /* hvm_print_line() and guest_console_write() logging. */
> +    char buf[DOMAIN_CONSOLE_BUF_SIZE];

This placement will negatively affect code generation on at least x86.
I did suggest putting the array at the end, for this very reason.

Jan