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

[Jan Beulich <jbeulich@xxxxxxxx>]

On 10.07.2025 03:35, dmkhn@xxxxxxxxx wrote:
> @@ -877,6 +873,16 @@ struct domain *domain_create(domid_t domid,
>  
>      /* All error paths can depend on the above setup. */
>  
> +    BUILD_BUG_ON(DOMAIN_CONSOLE_BUF_SIZE <= 0);

While the "equals 0" case can in principle happen, the "less than" part
is dead code (and hence this needs checking differently): The type of
DOMAIN_CONSOLE_BUF_SIZE is an unsigned one, so wrapping through 0 will
yield huge positive values.

> +    err = -ENOMEM;
> +    d->console = xzalloc_bytes(DOMAIN_CONSOLE_SIZE);

As previously indicated, new code ought to use the xv*alloc family of
functions, which deliberately doesn't include any ..._bytes() forms.
Note how instead there is xvzalloc_flex_struct() for situations like
the one here.

> @@ -371,6 +373,26 @@ struct evtchn_port_ops;
>  
>  #define MAX_NR_IOREQ_SERVERS 8
>  
> +/*
> + * Domain console settings is the dynamically-allocated data structure.
> + * Using an even multiple of a cache line size may help to optimize the
> + * allocation overhead.
> + */
> +#define DOMAIN_CONSOLE_SIZE     ROUNDUP(256, SMP_CACHE_BYTES)
> +#define DOMAIN_CONSOLE_BUF_SIZE (DOMAIN_CONSOLE_SIZE - \
> +                                 sizeof(struct domain_console))

But you're aware that there's allocation overhead, which consumes part of
a cacheline? I simply don't understand why this struct is so different
from others that such cleverness needs building in. Yet if it's relevant,
it really needs doing correctly.

> +/* Domain console settings. */
> +struct domain_console {
> +    /* Permission to take ownership of the physical console input. */
> +    bool input_allowed;
> +
> +    /* hvm_print_line() and guest_console_write() logging. */
> +    unsigned int idx;
> +    spinlock_t lock;
> +    char buf[0];

Iirc recent gcc warns about the use of this historic gcc extension, since
there has been the better form using just [] for quite a long time.

Jan