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

[dmkhn@xxxxxxxxx]

On Thu, Jul 10, 2025 at 01:16:24PM +0200, Jan Beulich wrote:
> 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.

Looks like xvzalloc_flex_struct() is not used anywhere in the code base...

> 
> > @@ -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