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

["Teddy Astie" <teddy.astie@xxxxxxxxxx>]

Hello,

Le 06/06/2025 à 21:51, dmkhn@xxxxxxxxx a écrit :
> From: Denis Mukhin <dmukhin@xxxxxxxx>
>
> Group all pbuf-related data structures under domain's console field.
>
> No functional change.
>
> Signed-off-by: Denis Mukhin <dmukhin@xxxxxxxx>
> ---
>   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.

>               {
>                   /* 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