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

["Roger Pau Monné" <roger.pau@citrix.com>]

On Fri, Jul 25, 2025 at 09:08:02PM +0000, dmkhn@proton.me wrote:
> From: Denis Mukhin <dmukhin@ford.com> 
> 
> 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.
> 
> Ensure accesses to domain_console pointer are valid in console_switch_input().
> 
> Bump the domain console buffer allocation size to 256. No extra symbol for the
> value since it is used only once during data structure declaration. All size
> checks use ARRAY_SIZE().
> 
> Allocate domain_console from the heap so that the parent domain struct size
> stays below PAGE_SIZE boundary to account for more console-related fields
> added in the future.
> 
> Finally, update the domain_console allocation and initialization code.
> 
> Not a functional change.
> 
> Signed-off-by: Denis Mukhin <dmukhin@ford.com>

Just one request about the allocation of the domain_console in
domain_create().

> ---
> Changes since v9:
> - kept cd as is in hvm_print_line() as requested
> - dropped domain lock in hvm_print_line()
> 
> Link to v9: https://lore.kernel.org/xen-devel/20250723001116.186009-1-dmukhin@ford.com/
> ---
>  xen/arch/arm/vpl011.c      |  2 +-
>  xen/arch/x86/hvm/hvm.c     | 16 +++++++++-------
>  xen/arch/x86/pv/shim.c     |  2 +-
>  xen/common/domain.c        | 19 +++++++++----------
>  xen/drivers/char/console.c | 28 ++++++++++++++++------------
>  xen/include/xen/sched.h    | 24 +++++++++++++-----------
>  6 files changed, 49 insertions(+), 42 deletions(-)
> 
> diff --git a/xen/arch/arm/vpl011.c b/xen/arch/arm/vpl011.c
> index 480fc664fc62..d0d17c76b72c 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..37af507a8d90 100644
> --- a/xen/arch/x86/hvm/hvm.c
> +++ b/xen/arch/x86/hvm/hvm.c
> @@ -560,6 +560,7 @@ static int cf_check hvm_print_line(
>      int dir, unsigned int port, unsigned int bytes, uint32_t *val)
>  {
>      struct domain *cd = current->domain;
> +    struct domain_console *cons = cd->console;
>      char c = *val;
>  
>      ASSERT(bytes == 1 && port == XEN_HVM_DEBUGCONS_IOPORT);
> @@ -571,16 +572,17 @@ static int cf_check hvm_print_line(
>      if ( !is_console_printable(c) )
>          return X86EMUL_OKAY;
>  
> -    spin_lock(&cd->pbuf_lock);
> +    spin_lock(&cons->lock);
> +    ASSERT(cons->idx < ARRAY_SIZE(cons->buf));
>      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 == (ARRAY_SIZE(cons->buf) - 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(cd, 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);

Nit: even if the struct if being freed just a couple of lines below, I
would use XVFREE() here.

>  
>      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);

That's too early to do the allocation of the console buffer AFAICT.
It needs to be done after the:

    if ( is_system_domain(d) )
        return d;

check, as allocating a console buffer for system domains is pointless.

> +
>      /*
>       * 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 963c7b043cd8..75fa033ce74d 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);
> @@ -567,10 +567,13 @@ static void console_switch_input(void)
>          d = rcu_lock_domain_by_id(domid);
>          if ( d )
>          {
> -            rcu_unlock_domain(d);
> -
> -            if ( !d->console.input_allowed )
> +            if ( !d->console->input_allowed )
> +            {
> +                rcu_unlock_domain(d);
>                  continue;
> +            }
> +
> +            rcu_unlock_domain(d);
>  
>              console_rx = next_rx;
>              printk("*** Serial input to DOM%u", domid);
> @@ -749,6 +752,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
> @@ -761,22 +765,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) < (ARRAY_SIZE(cons->buf) - 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..c828d5626dea 100644
> --- a/xen/include/xen/sched.h
> +++ b/xen/include/xen/sched.h
> @@ -371,6 +371,17 @@ struct evtchn_port_ops;
>  
>  #define MAX_NR_IOREQ_SERVERS 8
>  
> +/* 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[256];
> +};
> +
>  struct domain
>  {
>      domid_t          domain_id;
> @@ -562,12 +573,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;
>  
> @@ -652,11 +657,8 @@ struct domain
>      const unsigned int *llc_colors;
>  #endif
>  
> -    /* Console settings. */
> -    struct {
> -        /* Permission to take ownership of the physical console input. */
> -        bool input_allowed;
> -    } console;
> +    /* Pointer to console settings; NULL for system Xen domains. */

Nit: I would drop Xen from the sentence above, "system domains" is a
well known entity in this context.

Thanks, Roger.