Re: [PATCH v1] xen/console: group pbuf under console field

[dmkhn@proton.me]

On Fri, Jun 06, 2025 at 08:24:44PM +0000, Teddy Astie wrote:
> Hello,
> 
> Le 06/06/2025 à 21:51, dmkhn@proton.me a écrit :
> > From: Denis Mukhin <dmukhin@ford.com>
> >
> > Group all pbuf-related data structures under domain's console field.
> >
> > No functional change.
> >
> > Signed-off-by: Denis Mukhin <dmukhin@ford.com>
> > ---
> >   xen/arch/x86/hvm/hvm.c     | 14 +++++++-------
> >   xen/common/domain.c        |  8 ++++----
> >   xen/drivers/char/console.c | 19 +++++++++++--------
> >   xen/include/xen/sched.h    | 12 ++++++------
> >   4 files changed, 28 insertions(+), 25 deletions(-)
> >
> > diff --git a/xen/arch/x86/hvm/hvm.c b/xen/arch/x86/hvm/hvm.c
> > index 4cb2e13046..17d1fd42ce 100644
> > --- a/xen/arch/x86/hvm/hvm.c
> > +++ b/xen/arch/x86/hvm/hvm.c
> > @@ -571,16 +571,16 @@ static int cf_check hvm_print_line(
> >       if ( !is_console_printable(c) )
> >           return X86EMUL_OKAY;
> >
> > -    spin_lock(&cd->pbuf_lock);
> > +    spin_lock(&cd->console.pbuf_lock);
> >       if ( c != '\n' )
> > -        cd->pbuf[cd->pbuf_idx++] = c;
> > -    if ( (cd->pbuf_idx == (DOMAIN_PBUF_SIZE - 1)) || (c == '\n') )
> > +        cd->console.pbuf[cd->console.pbuf_idx++] = c;
> > +    if ( (cd->console.pbuf_idx == (DOMAIN_PBUF_SIZE - 1)) || (c == '\n') )
> >       {
> > -        cd->pbuf[cd->pbuf_idx] = '\0';
> > -        guest_printk(cd, XENLOG_G_DEBUG "%s\n", cd->pbuf);
> > -        cd->pbuf_idx = 0;
> > +        cd->console.pbuf[cd->console.pbuf_idx] = '\0';
> > +        guest_printk(cd, XENLOG_G_DEBUG "%s\n", cd->console.pbuf);
> > +        cd->console.pbuf_idx = 0;
> >       }
> > -    spin_unlock(&cd->pbuf_lock);
> > +    spin_unlock(&cd->console.pbuf_lock);
> >
> >       return X86EMUL_OKAY;
> >   }
> > diff --git a/xen/common/domain.c b/xen/common/domain.c
> > index 153cd75340..dd1867b2fe 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);
> > +    xfree(d->console.pbuf);
> >
> >       argo_destroy(d);
> >
> > @@ -862,7 +862,7 @@ 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);
> > +    spin_lock_init(&d->console.pbuf_lock);
> >
> >       rwlock_init(&d->vnuma_rwlock);
> >
> > @@ -956,8 +956,8 @@ struct domain *domain_create(domid_t domid,
> >           goto fail;
> >
> >       err = -ENOMEM;
> > -    d->pbuf = xzalloc_array(char, DOMAIN_PBUF_SIZE);
> > -    if ( !d->pbuf )
> > +    d->console.pbuf = xzalloc_array(char, DOMAIN_PBUF_SIZE);
> > +    if ( !d->console.pbuf )
> >           goto fail;
> >
> >       if ( (err = sched_init_domain(d, config->cpupool_id)) != 0 )
> > diff --git a/xen/drivers/char/console.c b/xen/drivers/char/console.c
> > index 616f4968b0..3855962af7 100644
> > --- a/xen/drivers/char/console.c
> > +++ b/xen/drivers/char/console.c
> > @@ -769,22 +769,25 @@ static long guest_console_write(XEN_GUEST_HANDLE_PARAM(char) buffer,
> >               } while ( --kcount > 0 );
> >
> >               *kout = '\0';
> > -            spin_lock(&cd->pbuf_lock);
> > +            spin_lock(&cd->console.pbuf_lock);
> >               kcount = kin - kbuf;
> >               if ( c != '\n' &&
> > -                 (cd->pbuf_idx + (kout - kbuf) < (DOMAIN_PBUF_SIZE - 1)) )
> > +                 cd->console.pbuf_idx + kout - kbuf < DOMAIN_PBUF_SIZE - 1 )
> 
> I don't think we want to drop the parentheses here.

The line will will exceed 80 chars if I keep parentheses.

Will something like the following work:

-                 (cd->pbuf_idx + (kout - kbuf) < (DOMAIN_PBUF_SIZE - 1)) )
+                 (cd->console.pbuf_idx + (kout - kbuf) <
+                    (DOMAIN_PBUF_SIZE - 1)) )

?

> 
> >               {
> >                   /* buffer the output until a newline */
> > -                memcpy(cd->pbuf + cd->pbuf_idx, kbuf, kout - kbuf);
> > -                cd->pbuf_idx += (kout - kbuf);
> > +                memcpy(cd->console.pbuf + cd->console.pbuf_idx,
> > +                       kbuf,
> > +                       kout - kbuf);
> > +                cd->console.pbuf_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;
> > +                cd->console.pbuf[cd->console.pbuf_idx] = '\0';
> > +                guest_printk(cd,
> > +                             XENLOG_G_DEBUG "%s%s\n", cd->console.pbuf, kbuf);
> > +                cd->console.pbuf_idx = 0;
> >               }
> > -            spin_unlock(&cd->pbuf_lock);
> > +            spin_unlock(&cd->console.pbuf_lock);
> >           }
> >
> >           guest_handle_add_offset(buffer, kcount);
> > diff --git a/xen/include/xen/sched.h b/xen/include/xen/sched.h
> > index fe53d4fab7..637aa09ec4 100644
> > --- a/xen/include/xen/sched.h
> > +++ b/xen/include/xen/sched.h
> > @@ -562,12 +562,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;
> >
> > @@ -654,6 +648,12 @@ struct domain
> >
> >       /* Console settings. */
> >       struct {
> > +        /* hvm_print_line() and guest_console_write() logging. */
> > +#define DOMAIN_PBUF_SIZE 200
> > +        char *pbuf;
> > +        unsigned int pbuf_idx;
> > +        spinlock_t pbuf_lock;
> > +
> >           /* Permission to take ownership of the physical console input. */
> >           bool input_allowed;
> >       } console;
> 
> 
> Teddy Astie | Vates XCP-ng Developer
> 
> XCP-ng & Xen Orchestra - Vates solutions
> 
> web: https://vates.tech
> 
>