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

[dmkhn@xxxxxxxxx]

On Tue, Jun 17, 2025 at 11:48:10AM +0200, Jan Beulich wrote:
> On 17.06.2025 03:27, dmkhn@xxxxxxxxx wrote:
> > From: Denis Mukhin <dmukhin@xxxxxxxx>
> >
> > Introduce domain_console for grouping data structures used for integrating
> > domain's diagnostic console with Xen's console driver.
> >
> > Group all pbuf-related data structures under domain_console. Rename the 
> > moved
> > fields to plain .buf, .idx and .lock names, since all uses of the fields are
> > touched.
> >
> > Bump the domain console buffer size to the closest power of 2 (256) and
> > rename the symbol to DOMAIN_CONSOLE_BUF_SIZE.
> >
> > Move d->console->buf management under CONFIG_VERBOSE_DEBUG when the
> > HYPERCALL_console_io handler is enabled.
> 
> This, if at all, needs to be a separate change (with its own justification).
> I for one don't think VERBOSE_DEBUG is intended to control any kind of guest
> output.
> 
> > --- a/xen/arch/x86/hvm/hvm.c
> > +++ b/xen/arch/x86/hvm/hvm.c
> > @@ -559,7 +559,6 @@ void hvm_do_resume(struct vcpu *v)
> >  static int cf_check hvm_print_line(
> >      int dir, unsigned int port, unsigned int bytes, uint32_t *val)
> >  {
> > -    struct domain *cd = current->domain;
> >      char c = *val;
> >
> >      ASSERT(bytes == 1 && port == XEN_HVM_DEBUGCONS_IOPORT);
> > @@ -570,17 +569,24 @@ static int cf_check hvm_print_line(
> >
> >      if ( !is_console_printable(c) )
> >          return X86EMUL_OKAY;
> 
> After this "return" ...
> 
> > -
> > -    spin_lock(&cd->pbuf_lock);
> > -    if ( c != '\n' )
> > -        cd->pbuf[cd->pbuf_idx++] = c;
> > -    if ( (cd->pbuf_idx == (DOMAIN_PBUF_SIZE - 1)) || (c == '\n') )
> > +#ifdef CONFIG_VERBOSE_DEBUG
> > +    else
> 
> ... there's no point to have "else" here.
> 
> >      {
> > -        cd->pbuf[cd->pbuf_idx] = '\0';
> > -        guest_printk(cd, XENLOG_G_DEBUG "%s\n", cd->pbuf);
> > -        cd->pbuf_idx = 0;
> > +        struct domain *cd = current->domain;
> 
> We normally name such a variable d. It also looks as if this could be pointer-
> to-const.
> 
> > +        struct domain_console *cons = cd->console;
> > +
> > +        spin_lock(&cons->lock);
> > +        if ( c != '\n' )
> > +            cons->buf[cons->idx++] = c;
> > +        if ( (cons->idx == (DOMAIN_CONSOLE_BUF_SIZE - 1)) || (c == '\n') )
> > +        {
> > +            cons->buf[cons->idx] = '\0';
> > +            guest_printk(cd, XENLOG_G_DEBUG "%s\n", cons->buf);
> > +            cons->idx = 0;
> > +        }
> > +        spin_unlock(&cons->lock);
> >      }
> > -    spin_unlock(&cd->pbuf_lock);
> > +#endif
> 
> None of the re-indentation is really warranted here (and will likely go away
> anyway once the #ifdef is dropped).
> 
> > --- a/xen/common/domain.c
> > +++ b/xen/common/domain.c
> > @@ -669,7 +669,7 @@ static void _domain_destroy(struct domain *d)
> >      BUG_ON(!d->is_dying);
> >      BUG_ON(atomic_read(&d->refcnt) != DOMAIN_DESTROYED);
> >
> > -    xfree(d->pbuf);
> > +    xfree(d->console);
> >
> >      argo_destroy(d);
> >
> > @@ -800,6 +800,11 @@ struct domain *domain_create(domid_t domid,
> >      if ( (d = alloc_domain_struct()) == NULL )
> >          return ERR_PTR(-ENOMEM);
> >
> > +    err = -ENOMEM;
> > +    d->console = xvzalloc(typeof(*d->console));
> > +    if ( !d->console )
> > +        goto fail;
> 
> This definitely need to move down some, at least ...
> 
> >      /* Sort out our idea of is_system_domain(). */
> >      d->domain_id = domid;
> >      d->unique_id = get_unique_id();
> 
> ... past here. There absolutely must not be struct domain instances be
> passed around (see e.g. the call to sched_destroy_domain()) without the
> domain ID set. It's hard to see ...
> 
> > @@ -862,7 +867,9 @@ struct domain *domain_create(domid_t domid,
> >      spin_lock_init(&d->shutdown_lock);
> >      d->shutdown_code = SHUTDOWN_CODE_INVALID;
> >
> > -    spin_lock_init(&d->pbuf_lock);
> > +#ifdef CONFIG_VERBOSE_DEBUG
> > +    spin_lock_init(&d->console->lock);
> > +#endif
> 
> .. why here or ...
> 
> > @@ -955,11 +962,6 @@ struct domain *domain_create(domid_t domid,
> >      if ( (err = argo_init(d)) != 0 )
> >          goto fail;
> >
> > -    err = -ENOMEM;
> > -    d->pbuf = xzalloc_array(char, DOMAIN_PBUF_SIZE);
> > -    if ( !d->pbuf )
> > -        goto fail;
> 
> ... even here wouldn't be early enough anyway.
> 
> And btw - where did this buffer allocation move? I don't see anywhere
> that d->console->buf would now be initialized. (However, see below.)
> 
> > --- a/xen/include/xen/sched.h
> > +++ b/xen/include/xen/sched.h
> > @@ -371,6 +371,22 @@ struct evtchn_port_ops;
> >
> >  #define MAX_NR_IOREQ_SERVERS 8
> >
> > +/* Arbitrary value; must be a multiple of the cacheline size. */
> > +#define DOMAIN_CONSOLE_BUF_SIZE 256
> 
> Where does the relationship with cache line size come from? What if
> Xen was to run on hardware (whichever arch) with 512-byte cache lines?
> 
> > +/* Domain console settings. */
> > +struct domain_console {
> > +#ifdef CONFIG_VERBOSE_DEBUG
> > +    /* hvm_print_line() and guest_console_write() logging. */
> > +    char *buf;
> 
> To avoid the need to do yet another separate allocation, how about ...
> 
> > +    unsigned int idx;
> > +    spinlock_t lock;
> > +#endif /* CONFIG_VERBOSE_DEBUG */
> > +
> > +    /* Permission to take ownership of the physical console input. */
> > +    bool input_allowed;
> 
>     char buf[DOMAIN_CONSOLE_BUF_SIZE];
> 
> Ultimately this would allow the buffer size to be e.g. command line
> controlled (if so desired), by then simply converting to a flexible
> array member.

Whoops, apologies for the bogus change.
Looks like I posted from the wrong branch.

> 
> Jan