On 10.07.2025 03:35, dm...@proton.me 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
