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

[Roger Pau Monné <roger.pau@xxxxxxxxxx>]

On Fri, Jul 11, 2025 at 07:04:59PM +0000, dmkhn@xxxxxxxxx wrote:
> On Fri, Jul 11, 2025 at 12:54:11PM +0200, Roger Pau Monné wrote:
> > On Fri, Jul 11, 2025 at 12:40:11AM +0000, 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 allocation size to 256.
> > >
> > > Rename domain console buffer size symbol to DOMAIN_CONSOLE_BUF_SIZE.
> > >
> > > Finally, update the domain_console allocation and initialization code.
> > >
> > > No functional change.
> > >
> > > Signed-off-by: Denis Mukhin <dmukhin@xxxxxxxx>
> > > ---
> > > Changes since v6:
> > > - reverted allocation of domain_console as in v5
> > > - dropped the commentary for DOMAIN_CONSOLE_BUF_SIZE
> > >
> > > Link to v6: 
> > > https://lore.kernel.org/xen-devel/20250710013421.2321353-1-dmukhin@xxxxxxxx/
> > > CI: 
> > > https://gitlab.com/xen-project/people/dmukhin/xen/-/pipelines/1919473093
> > > ---
> > >  xen/arch/arm/vpl011.c      |  2 +-
> > >  xen/arch/x86/hvm/hvm.c     | 17 +++++++++--------
> > >  xen/arch/x86/pv/shim.c     |  2 +-
> > >  xen/common/domain.c        | 19 +++++++++----------
> > >  xen/drivers/char/console.c | 21 +++++++++++----------
> > >  xen/include/xen/sched.h    | 24 ++++++++++++++----------
> > >  6 files changed, 45 insertions(+), 40 deletions(-)
> > >
> > > diff --git a/xen/arch/arm/vpl011.c b/xen/arch/arm/vpl011.c
> > > index 2b6f2a09bca6..f4a840da10c5 100644
> > > --- a/xen/arch/arm/vpl011.c
> > > +++ b/xen/arch/arm/vpl011.c
> > > @@ -713,7 +713,7 @@ int domain_vpl011_init(struct domain *d, struct 
> > > vpl011_init_info *info)
> > >      }
> > >      else
> > >      {
> > > -        d->console.input_allowed = true;
> > > +        d->console->input_allowed = true;
> > >          vpl011->backend_in_domain = false;
> > >
> > >          vpl011->backend.xen = xzalloc(struct vpl011_xen_backend);
> > > diff --git a/xen/arch/x86/hvm/hvm.c b/xen/arch/x86/hvm/hvm.c
> > > index 56c7de39778b..2be98f6ccbd1 100644
> > > --- a/xen/arch/x86/hvm/hvm.c
> > > +++ b/xen/arch/x86/hvm/hvm.c
> > > @@ -559,7 +559,8 @@ 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;
> > > +    const struct domain *d = current->domain;
> > > +    struct domain_console *cons = d->console;
> > >      char c = *val;
> > >
> > >      ASSERT(bytes == 1 && port == XEN_HVM_DEBUGCONS_IOPORT);
> > > @@ -571,16 +572,16 @@ static int cf_check hvm_print_line(
> > >      if ( !is_console_printable(c) )
> > >          return X86EMUL_OKAY;
> > >
> > > -    spin_lock(&cd->pbuf_lock);
> > > +    spin_lock(&cons->lock);
> > >      if ( c != '\n' )
> > > -        cd->pbuf[cd->pbuf_idx++] = c;
> > > -    if ( (cd->pbuf_idx == (DOMAIN_PBUF_SIZE - 1)) || (c == '\n') )
> > > +        cons->buf[cons->idx++] = c;
> > > +    if ( (cons->idx == (DOMAIN_CONSOLE_BUF_SIZE - 1)) || (c == '\n') )
> > >      {
> > > -        cd->pbuf[cd->pbuf_idx] = '\0';
> > > -        guest_printk(cd, XENLOG_G_DEBUG "%s\n", cd->pbuf);
> > > -        cd->pbuf_idx = 0;
> > > +        cons->buf[cons->idx] = '\0';
> > > +        guest_printk(d, XENLOG_G_DEBUG "%s\n", cons->buf);
> > > +        cons->idx = 0;
> > >      }
> > > -    spin_unlock(&cd->pbuf_lock);
> > > +    spin_unlock(&cons->lock);
> > >
> > >      return X86EMUL_OKAY;
> > >  }
> > > diff --git a/xen/arch/x86/pv/shim.c b/xen/arch/x86/pv/shim.c
> > > index bc2a7dd5fae5..bd29c53a2d34 100644
> > > --- a/xen/arch/x86/pv/shim.c
> > > +++ b/xen/arch/x86/pv/shim.c
> > > @@ -239,7 +239,7 @@ void __init pv_shim_setup_dom(struct domain *d, 
> > > l4_pgentry_t *l4start,
> > >       */
> > >      d->max_pages = domain_tot_pages(d);
> > >
> > > -    d->console.input_allowed = true;
> > > +    d->console->input_allowed = true;
> > >  }
> > >
> > >  static void write_start_info(struct domain *d)
> > > diff --git a/xen/common/domain.c b/xen/common/domain.c
> > > index 303c338ef293..caef4cc8d649 100644
> > > --- 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);
> > > +    xvfree(d->console);
> > >
> > >      argo_destroy(d);
> > >
> > > @@ -835,8 +835,6 @@ struct domain *domain_create(domid_t domid,
> > >          flags |= CDF_hardware;
> > >          if ( old_hwdom )
> > >              old_hwdom->cdf &= ~CDF_hardware;
> > > -
> > > -        d->console.input_allowed = true;
> > >      }
> > >
> > >      /* Holding CDF_* internal flags. */
> > > @@ -866,8 +864,6 @@ 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);
> > > -
> > >      rwlock_init(&d->vnuma_rwlock);
> > >
> > >  #ifdef CONFIG_HAS_PCI
> > > @@ -877,6 +873,14 @@ struct domain *domain_create(domid_t domid,
> > >
> > >      /* All error paths can depend on the above setup. */
> > >
> > > +    err = -ENOMEM;
> > > +    d->console = xvzalloc(typeof(*d->console));
> > > +    if ( !d->console )
> > > +        goto fail;
> > > +
> > > +    spin_lock_init(&d->console->lock);
> > > +    d->console->input_allowed = is_hardware_domain(d);
> > > +
> > >      /*
> > >       * Allocate d->vcpu[] and set ->max_vcpus up early.  Various 
> > > per-domain
> > >       * resources want to be sized based on max_vcpus.
> > > @@ -959,11 +963,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;
> > > -
> > >      if ( (err = sched_init_domain(d, config->cpupool_id)) != 0 )
> > >          goto fail;
> > >
> > > diff --git a/xen/drivers/char/console.c b/xen/drivers/char/console.c
> > > index f54632bc0811..469f5e8832da 100644
> > > --- a/xen/drivers/char/console.c
> > > +++ b/xen/drivers/char/console.c
> > > @@ -526,7 +526,7 @@ struct domain *console_get_domain(void)
> > >      if ( !d )
> > >          return NULL;
> > >
> > > -    if ( d->console.input_allowed )
> > > +    if ( d->console->input_allowed )
> > >          return d;
> > >
> > >      rcu_unlock_domain(d);
> > > @@ -569,7 +569,7 @@ static void console_switch_input(void)
> > >          {
> > >              rcu_unlock_domain(d);
> > >
> > > -            if ( !d->console.input_allowed )
> > > +            if ( !d->console->input_allowed )
> > >                  continue;
> > >
> > >              console_rx = next_rx;
> > > @@ -788,6 +788,7 @@ static long 
> > > guest_console_write(XEN_GUEST_HANDLE_PARAM(char) buffer,
> > >          else
> > >          {
> > >              char *kin = kbuf, *kout = kbuf, c;
> > > +            struct domain_console *cons = cd->console;
> > >
> > >              /* Strip non-printable characters */
> > >              do
> > > @@ -800,22 +801,22 @@ 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)) )
> > >              {
> > >                  /* 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;
> > >              }
> > >              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);
> > > +                cons->idx = 0;
> > >              }
> > > -            spin_unlock(&cd->pbuf_lock);
> > > +            spin_unlock(&cons->lock);
> > >          }
> > >
> > >          guest_handle_add_offset(buffer, kcount);
> > > diff --git a/xen/include/xen/sched.h b/xen/include/xen/sched.h
> > > index fe53d4fab7ba..8771b7f22b48 100644
> > > --- a/xen/include/xen/sched.h
> > > +++ b/xen/include/xen/sched.h
> > > @@ -371,6 +371,19 @@ struct evtchn_port_ops;
> > >
> > >  #define MAX_NR_IOREQ_SERVERS 8
> > >
> > > +#define DOMAIN_CONSOLE_BUF_SIZE (256)
> > > +
> > > +/* 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[DOMAIN_CONSOLE_BUF_SIZE];
> > > +};
> > > +
> > >  struct domain
> > >  {
> > >      domid_t          domain_id;
> > > @@ -562,12 +575,6 @@ struct domain
> > >      /* Control-plane tools handle for this domain. */
> > >      xen_domain_handle_t handle;
> > >
> > > -    /* hvm_print_line() and guest_console_write() logging. */
> > > -#define DOMAIN_PBUF_SIZE 200
> > > -    char       *pbuf;
> > > -    unsigned int pbuf_idx;
> > > -    spinlock_t  pbuf_lock;
> > > -
> > >      /* OProfile support. */
> > >      struct xenoprof *xenoprof;
> > >
> > > @@ -653,10 +660,7 @@ struct domain
> > >  #endif
> > >
> > >      /* Console settings. */
> > > -    struct {
> > > -        /* Permission to take ownership of the physical console input. */
> > > -        bool input_allowed;
> > > -    } console;
> > > +    struct domain_console *console;
> > 
> > I assume I'm missing some context, what's the reason for converting
> > this field into a pointer, instead of moving the existing fields
> > inside and allocate the buffer at domain_create()?
> 
> The reason is avoid crossing PAGE_SIZE boundary for domain structure *if* more
> console-related fields are added to the "domain console settings" (which will
> happen for console focus management and probably for vUARTs).

Oh, I see, maybe worth adding to the commit message, as otherwise
there's not much context as to why moving into a separately allocated
structure is done.

Thanks, Roger.