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(-)
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.
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@ford.com>
---
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);
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];
+};
+
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;
} __aligned(PAGE_SIZE);
static inline struct page_list_head *page_to_list(
--
2.34.1Hi,
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@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.
>
> 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@ford.com>
> ---
> 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?
> +};
> +
> 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.
Do check how big struct domain is, though.
> } __aligned(PAGE_SIZE);
>
> static inline struct page_list_head *page_to_list(
Cheers,
Alejandro
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@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.
> >
> > 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@ford.com>
> > ---
> > 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 )
Is there a chance that domain being destroyed can also get here with
proper timing? I don't think it's currently possible, as the freeing
of the buffer is done in the last leg of domain destruction, which is
not preemptible.
However there might a race in console_switch_input(), as the rcu lock
is dropped before checking d->console.input_allowed, which could allow
for the domain to be destroyed in the middle? I don't think this is
realistic under a normal scenario, but could be possible if running
Xen nested or similar, where Xen CPUs can get de-scheduled.
> > 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?
>
> > +};
> > +
> > 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.
The buffer can be variable in size in principle, so we should better
keep that as a separate allocation. Also note that system domains
avoid allocating this buffer altogether, which we want to keep doing.
I don't have a strong opinion whether to allocate all the fields, or
just the buffer, but at least the buffer needs to be kept as an
external allocation from struct domain.
Thanks, Roger.
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@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.
> >
> > 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@ford.com>
> > ---
> > 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.
>
> Do check how big struct domain is, though.
struct domain is 2432 bytes (64 bit, x86, default config, SHA 6efafbf13351).
>
> > } __aligned(PAGE_SIZE);
> >
> > static inline struct page_list_head *page_to_list(
>
> Cheers,
> Alejandro
>
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@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.
>> >
>> > 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@ford.com>
>> > ---
>> > 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
Le 16/07/2025 à 07:11, dmukhin@ford.com a écrit :
> 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.
>
> 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@ford.com>
Reviewed-by: Teddy Astie <teddy.astie@vates.tech>
> ---
> 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);
>
> 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];
> +};
> +
> 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;
> } __aligned(PAGE_SIZE);
>
> static inline struct page_list_head *page_to_list(
Teddy Astie | Vates XCP-ng Developer
XCP-ng & Xen Orchestra - Vates solutions
web: https://vates.tech
© 2016 - 2025 Red Hat, Inc.