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

[Alejandro Vallejo <alejandro.garciavallejo@xxxxxxx>]

On Sat Jul 19, 2025 at 1:45 AM CEST, dmkhn wrote:
> On Fri, Jul 18, 2025 at 01:04:56PM +0200, Alejandro Vallejo wrote:
>> Hi,
>> 
>> I like the new encapsulation, but I have a few questions.
>> 
>> On Wed Jul 16, 2025 at 4:04 AM CEST, dmukhin 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. 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.
>> >
>> > No functional change.
>> >
>> > Signed-off-by: Denis Mukhin <dmukhin@xxxxxxxx>
>> > ---
>> > Changes since v7:
>> > - use ARRAY_SIZE() for cons->buf checks
>> > - update the commit message
>> > ---
>> >  xen/arch/arm/vpl011.c      |  2 +-
>> >  xen/arch/x86/hvm/hvm.c     | 18 ++++++++++--------
>> >  xen/arch/x86/pv/shim.c     |  2 +-
>> >  xen/common/domain.c        | 19 +++++++++----------
>> >  xen/drivers/char/console.c | 21 +++++++++++----------
>> >  xen/include/xen/sched.h    | 22 ++++++++++++----------
>> >  6 files changed, 44 insertions(+), 40 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..684d2c0c195c 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,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(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);
>> 
>> XVFREE() to avoid a dangling pointer. But look at the end. I'd just remove 
>> the
>> pointer altogether.
>> 
>> >
>> >      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 ba5a809a99fb..b0d50a910e27 100644
>> > --- a/xen/drivers/char/console.c
>> > +++ b/xen/drivers/char/console.c
>> > @@ -521,7 +521,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);
>> > @@ -564,7 +564,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;
>> > @@ -744,6 +744,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
>> > @@ -756,22 +757,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..f7bb44328bbc 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];
>> 
>> I'd rather see things a bit shuffled. Like:
>> 
>>       /* hvm_print_line() and guest_console_write() logging. */
>>       spinlock_t lock;
>>       unsigned int idx;
>>       char buf[255];
>> 
>>       /* Permission to take ownership of the physical console input. */
>>       bool input_allowed;
>> 
>> That way the struct is fully packed (note the s/256/255/) and input_allowed
>> remains visually away from the other group of fields.
>> input_allowed remains away from the lock, indicating that input_allowed
>> 
>> Note that the lock is 64bit aligned when CONFIG_DEBUG_LOCK_PROFILE is set.
>> 
>> Thoughts?
>
> I can do that, no problem!
>
>> 
>> > +};
>> > +
>> >  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;
>> >
>> > @@ -653,10 +658,7 @@ struct domain
>> >  #endif
>> >
>> >      /* Console settings. */
>> > -    struct {
>> > -        /* Permission to take ownership of the physical console input. */
>> > -        bool input_allowed;
>> > -    } console;
>> > +    struct domain_console *console;
>> 
>> Why a pointer? domain_console is a fixed-size type, so unless the domain is
>> overflowing PAGE_SIZE (which we very definitely don't want either) I'd 
>> consider
>> making it a regular member Simplifies memory management too. No alloc/free 
>> nor
>> potential dangling pointers.
>
> domain_console may grow in the future wrt console focus and further vUART
> management changes, that's why I moved the entire stuct to be heap-allocated.
>
> Let me redo that.

Actually, don't. Thinking some more about it, this is confidential data we'll
want out for ASI, so it's easier to spot if it's still a x*alloc'ed.

nvm about it. Sorry for the headache!

Cheers,
Alejandro