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

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

[dmkhn]

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 )
             {
                 /* 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;
-- 
2.34.1

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

[Teddy Astie]

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.

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

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

[dmkhn]

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

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

[Jan Beulich]

On 07.06.2025 01:06, dmkhn@proton.me wrote:
> On Fri, Jun 06, 2025 at 08:24:44PM +0000, Teddy Astie wrote:
>> Le 06/06/2025 à 21:51, dmkhn@proton.me a écrit :
>>> --- 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.

I don't see any issue with doing so - they don't serve much of a purpose 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)) )
> 
> ?

Well, indentation of the latter of the two new lines is two too deep, as it
looks.

Jan

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

[Jan Beulich]

On 06.06.2025 21:49, dmkhn@proton.me wrote:
> From: Denis Mukhin <dmukhin@ford.com>
> 
> Group all pbuf-related data structures under domain's console field.

Fine with me in principle, as I was indeed wondering about the lack of
grouping when the sub-struct was introduced, but ...

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

... since all uses of the fields need touching anyway, can we perhaps
think of giving the fields better names? I never understood what the
'p' in "pbuf" actually stands for, for example. My suggestion would
be to replace "pbuf" by "glog" (for "guest logging"), but surely there
are alternatives.

Jan

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

[Andrew Cooper]

On 10/06/2025 9:10 am, Jan Beulich wrote:
> On 06.06.2025 21:49, dmkhn@proton.me wrote:
>> From: Denis Mukhin <dmukhin@ford.com>
>>
>> Group all pbuf-related data structures under domain's console field.
> Fine with me in principle, as I was indeed wondering about the lack of
> grouping when the sub-struct was introduced, but ...
>
>> @@ -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;
> ... since all uses of the fields need touching anyway, can we perhaps
> think of giving the fields better names? I never understood what the
> 'p' in "pbuf" actually stands for, for example.

I always assumed it was Hungarian notation, so pointer.

As it's namespaced within .console, plain .buf, .idx and .lock names
work fine.

Separately, 200 is a silly and arbitrary number.  Furthermore the
allocation is unconditional, despite the fact that in !VERSBOSE builds,
domUs can't use this facility.  Also, where's the input buffer?

~Andrew

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

[dmkhn]

On Tue, Jun 10, 2025 at 10:10:44AM +0200, Jan Beulich wrote:
> On 06.06.2025 21:49, dmkhn@proton.me wrote:
> > From: Denis Mukhin <dmukhin@ford.com>
> >
> > Group all pbuf-related data structures under domain's console field.
> 
> Fine with me in principle, as I was indeed wondering about the lack of
> grouping when the sub-struct was introduced, but ...
> 
> > @@ -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;
> 
> ... since all uses of the fields need touching anyway, can we perhaps
> think of giving the fields better names? I never understood what the
> 'p' in "pbuf" actually stands for, for example. My suggestion would
> be to replace "pbuf" by "glog" (for "guest logging"), but surely there
> are alternatives.

Sounds good to me.
I can do renaming in v2.

> 
> Jan
> 

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

[dmkhn]

On Tue, Jun 10, 2025 at 12:24:57PM +0100, Andrew Cooper wrote:
> On 10/06/2025 9:10 am, Jan Beulich wrote:
> > On 06.06.2025 21:49, dmkhn@proton.me wrote:
> >> From: Denis Mukhin <dmukhin@ford.com>
> >>
> >> Group all pbuf-related data structures under domain's console field.
> > Fine with me in principle, as I was indeed wondering about the lack of
> > grouping when the sub-struct was introduced, but ...
> >
> >> @@ -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;
> > ... since all uses of the fields need touching anyway, can we perhaps
> > think of giving the fields better names? I never understood what the
> > 'p' in "pbuf" actually stands for, for example.
> 
> I always assumed it was Hungarian notation, so pointer.
> 
> As it's namespaced within .console, plain .buf, .idx and .lock names
> work fine.

Ack.

> 
> Separately, 200 is a silly and arbitrary number.  Furthermore the
> allocation is unconditional, despite the fact that in !VERSBOSE builds,
> domUs can't use this facility.  Also, where's the input buffer?

Thanks!

I will try to address those under individual changes.

re: arbitrary number: Will bumping the buffer size to the next power of 2 ==
256 work?

re: input buffer: Looks like there's only global serial_rx_ring buffer in
current design. My guess - because the input buffer is shared between domains
and Xen itself which does not have domain struct associated with it.

> 
> ~Andrew

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

[Jan Beulich]

On 10.06.2025 20:02, dmkhn@proton.me wrote:
> On Tue, Jun 10, 2025 at 12:24:57PM +0100, Andrew Cooper wrote:
>> Separately, 200 is a silly and arbitrary number.  Furthermore the
>> allocation is unconditional, despite the fact that in !VERSBOSE builds,
>> domUs can't use this facility.  Also, where's the input buffer?
> 
> Thanks!
> 
> I will try to address those under individual changes.
> 
> re: arbitrary number: Will bumping the buffer size to the next power of 2 ==
> 256 work?

Any other number would work, but would be as arbitrary. Since the buffer is
dynamically allocated, one non-arbitrary aspect of the selection may want to
be to make the number such that including allocation overhead it's an even
multiple of cache line size.

Jan