xen/arch/arm/vpl011.c | 2 +- xen/arch/x86/hvm/hvm.c | 17 +++++++++-------- xen/arch/x86/pv/shim.c | 2 +- xen/common/domain.c | 21 +++++++++++---------- xen/drivers/char/console.c | 21 +++++++++++---------- xen/include/xen/sched.h | 33 +++++++++++++++++++++++---------- 6 files changed, 56 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 allocation size to the multiple of a cache line size
(256).
Rename domain console buffer size symbol to DOMAIN_CONSOLE_BUF_SIZE.
Finally, update the domain_console allocation and initialization code.
No functional change.
Signed-off-by: Denis Mukhin <dmukhin@ford.com>
---
Changes since v5:
- updated commentary for the allocation size selection
- updated the new data structure allocation size
- added use of VLA for the domain console buffer
Link to v5: https://lore.kernel.org/xen-devel/20250625002956.408945-1-dmukhin@ford.com/
Link to CI: https://gitlab.com/xen-project/people/dmukhin/xen/-/pipelines/1917680099
---
xen/arch/arm/vpl011.c | 2 +-
xen/arch/x86/hvm/hvm.c | 17 +++++++++--------
xen/arch/x86/pv/shim.c | 2 +-
xen/common/domain.c | 21 +++++++++++----------
xen/drivers/char/console.c | 21 +++++++++++----------
xen/include/xen/sched.h | 33 +++++++++++++++++++++++----------
6 files changed, 56 insertions(+), 40 deletions(-)
diff --git a/xen/arch/arm/vpl011.c b/xen/arch/arm/vpl011.c
index 2b6f2a09bca6..f4a840da10c5 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..2be98f6ccbd1 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,16 @@ static int cf_check hvm_print_line(
if ( !is_console_printable(c) )
return X86EMUL_OKAY;
- spin_lock(&cd->pbuf_lock);
+ spin_lock(&cons->lock);
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 == (DOMAIN_CONSOLE_BUF_SIZE - 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..3ffefcbbff77 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);
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,16 @@ struct domain *domain_create(domid_t domid,
/* All error paths can depend on the above setup. */
+ BUILD_BUG_ON(DOMAIN_CONSOLE_BUF_SIZE <= 0);
+
+ err = -ENOMEM;
+ d->console = xzalloc_bytes(DOMAIN_CONSOLE_SIZE);
+ 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 +965,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 f54632bc0811..469f5e8832da 100644
--- a/xen/drivers/char/console.c
+++ b/xen/drivers/char/console.c
@@ -526,7 +526,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);
@@ -569,7 +569,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;
@@ -788,6 +788,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
@@ -800,22 +801,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) < (DOMAIN_CONSOLE_BUF_SIZE - 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..006937d32260 100644
--- a/xen/include/xen/sched.h
+++ b/xen/include/xen/sched.h
@@ -35,6 +35,8 @@
DEFINE_XEN_GUEST_HANDLE(vcpu_runstate_info_compat_t);
#endif
+struct domain_console;
+
/*
* Stats
*
@@ -371,6 +373,26 @@ struct evtchn_port_ops;
#define MAX_NR_IOREQ_SERVERS 8
+/*
+ * Domain console settings is the dynamically-allocated data structure.
+ * Using an even multiple of a cache line size may help to optimize the
+ * allocation overhead.
+ */
+#define DOMAIN_CONSOLE_SIZE ROUNDUP(256, SMP_CACHE_BYTES)
+#define DOMAIN_CONSOLE_BUF_SIZE (DOMAIN_CONSOLE_SIZE - \
+ sizeof(struct domain_console))
+
+/* 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[0];
+};
+
struct domain
{
domid_t domain_id;
@@ -562,12 +584,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 +669,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.1On 10.07.2025 03:35, dmkhn@proton.me wrote:
> @@ -877,6 +873,16 @@ struct domain *domain_create(domid_t domid,
>
> /* All error paths can depend on the above setup. */
>
> + BUILD_BUG_ON(DOMAIN_CONSOLE_BUF_SIZE <= 0);
While the "equals 0" case can in principle happen, the "less than" part
is dead code (and hence this needs checking differently): The type of
DOMAIN_CONSOLE_BUF_SIZE is an unsigned one, so wrapping through 0 will
yield huge positive values.
> + err = -ENOMEM;
> + d->console = xzalloc_bytes(DOMAIN_CONSOLE_SIZE);
As previously indicated, new code ought to use the xv*alloc family of
functions, which deliberately doesn't include any ..._bytes() forms.
Note how instead there is xvzalloc_flex_struct() for situations like
the one here.
> @@ -371,6 +373,26 @@ struct evtchn_port_ops;
>
> #define MAX_NR_IOREQ_SERVERS 8
>
> +/*
> + * Domain console settings is the dynamically-allocated data structure.
> + * Using an even multiple of a cache line size may help to optimize the
> + * allocation overhead.
> + */
> +#define DOMAIN_CONSOLE_SIZE ROUNDUP(256, SMP_CACHE_BYTES)
> +#define DOMAIN_CONSOLE_BUF_SIZE (DOMAIN_CONSOLE_SIZE - \
> + sizeof(struct domain_console))
But you're aware that there's allocation overhead, which consumes part of
a cacheline? I simply don't understand why this struct is so different
from others that such cleverness needs building in. Yet if it's relevant,
it really needs doing correctly.
> +/* 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[0];
Iirc recent gcc warns about the use of this historic gcc extension, since
there has been the better form using just [] for quite a long time.
Jan
On Thu, Jul 10, 2025 at 01:16:24PM +0200, Jan Beulich wrote:
> On 10.07.2025 03:35, dmkhn@proton.me wrote:
> > @@ -877,6 +873,16 @@ struct domain *domain_create(domid_t domid,
> >
> > /* All error paths can depend on the above setup. */
> >
> > + BUILD_BUG_ON(DOMAIN_CONSOLE_BUF_SIZE <= 0);
>
> While the "equals 0" case can in principle happen, the "less than" part
> is dead code (and hence this needs checking differently): The type of
> DOMAIN_CONSOLE_BUF_SIZE is an unsigned one, so wrapping through 0 will
> yield huge positive values.
>
> > + err = -ENOMEM;
> > + d->console = xzalloc_bytes(DOMAIN_CONSOLE_SIZE);
>
> As previously indicated, new code ought to use the xv*alloc family of
> functions, which deliberately doesn't include any ..._bytes() forms.
> Note how instead there is xvzalloc_flex_struct() for situations like
> the one here.
Looks like xvzalloc_flex_struct() is not used anywhere in the code base...
>
> > @@ -371,6 +373,26 @@ struct evtchn_port_ops;
> >
> > #define MAX_NR_IOREQ_SERVERS 8
> >
> > +/*
> > + * Domain console settings is the dynamically-allocated data structure.
> > + * Using an even multiple of a cache line size may help to optimize the
> > + * allocation overhead.
> > + */
> > +#define DOMAIN_CONSOLE_SIZE ROUNDUP(256, SMP_CACHE_BYTES)
> > +#define DOMAIN_CONSOLE_BUF_SIZE (DOMAIN_CONSOLE_SIZE - \
> > + sizeof(struct domain_console))
>
> But you're aware that there's allocation overhead, which consumes part of
> a cacheline? I simply don't understand why this struct is so different
> from others that such cleverness needs building in. Yet if it's relevant,
> it really needs doing correctly.
>
> > +/* 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[0];
>
> Iirc recent gcc warns about the use of this historic gcc extension, since
> there has been the better form using just [] for quite a long time.
>
> Jan
On 11.07.2025 03:07, dmkhn@proton.me wrote: > On Thu, Jul 10, 2025 at 01:16:24PM +0200, Jan Beulich wrote: >> On 10.07.2025 03:35, dmkhn@proton.me wrote: >>> @@ -877,6 +873,16 @@ struct domain *domain_create(domid_t domid, >>> >>> /* All error paths can depend on the above setup. */ >>> >>> + BUILD_BUG_ON(DOMAIN_CONSOLE_BUF_SIZE <= 0); >> >> While the "equals 0" case can in principle happen, the "less than" part >> is dead code (and hence this needs checking differently): The type of >> DOMAIN_CONSOLE_BUF_SIZE is an unsigned one, so wrapping through 0 will >> yield huge positive values. >> >>> + err = -ENOMEM; >>> + d->console = xzalloc_bytes(DOMAIN_CONSOLE_SIZE); >> >> As previously indicated, new code ought to use the xv*alloc family of >> functions, which deliberately doesn't include any ..._bytes() forms. >> Note how instead there is xvzalloc_flex_struct() for situations like >> the one here. > > Looks like xvzalloc_flex_struct() is not used anywhere in the code base... And what do you mean to tell me by that? xvmalloc_flex_struct() is used, and x[mz]alloc_flex_struct() uses are still in need of conversion (which is going to be a slow-going process). Jan
On Fri, Jul 11, 2025 at 07:59:27AM +0200, Jan Beulich wrote: > On 11.07.2025 03:07, dmkhn@proton.me wrote: > > On Thu, Jul 10, 2025 at 01:16:24PM +0200, Jan Beulich wrote: > >> On 10.07.2025 03:35, dmkhn@proton.me wrote: > >>> @@ -877,6 +873,16 @@ struct domain *domain_create(domid_t domid, > >>> > >>> /* All error paths can depend on the above setup. */ > >>> > >>> + BUILD_BUG_ON(DOMAIN_CONSOLE_BUF_SIZE <= 0); > >> > >> While the "equals 0" case can in principle happen, the "less than" part > >> is dead code (and hence this needs checking differently): The type of > >> DOMAIN_CONSOLE_BUF_SIZE is an unsigned one, so wrapping through 0 will > >> yield huge positive values. > >> > >>> + err = -ENOMEM; > >>> + d->console = xzalloc_bytes(DOMAIN_CONSOLE_SIZE); > >> > >> As previously indicated, new code ought to use the xv*alloc family of > >> functions, which deliberately doesn't include any ..._bytes() forms. > >> Note how instead there is xvzalloc_flex_struct() for situations like > >> the one here. > > > > Looks like xvzalloc_flex_struct() is not used anywhere in the code base... > > And what do you mean to tell me by that? xvmalloc_flex_struct() is used, > and x[mz]alloc_flex_struct() uses are still in need of conversion (which > is going to be a slow-going process). Just an observation; I was not aware of these APIs and I was not aware that there's some slow-going process of switching to preferred use of xvzalloc_flex_struct(). > > Jan
On 11.07.2025 09:25, dmkhn@proton.me wrote: > On Fri, Jul 11, 2025 at 07:59:27AM +0200, Jan Beulich wrote: >> On 11.07.2025 03:07, dmkhn@proton.me wrote: >>> On Thu, Jul 10, 2025 at 01:16:24PM +0200, Jan Beulich wrote: >>>> On 10.07.2025 03:35, dmkhn@proton.me wrote: >>>>> @@ -877,6 +873,16 @@ struct domain *domain_create(domid_t domid, >>>>> >>>>> /* All error paths can depend on the above setup. */ >>>>> >>>>> + BUILD_BUG_ON(DOMAIN_CONSOLE_BUF_SIZE <= 0); >>>> >>>> While the "equals 0" case can in principle happen, the "less than" part >>>> is dead code (and hence this needs checking differently): The type of >>>> DOMAIN_CONSOLE_BUF_SIZE is an unsigned one, so wrapping through 0 will >>>> yield huge positive values. >>>> >>>>> + err = -ENOMEM; >>>>> + d->console = xzalloc_bytes(DOMAIN_CONSOLE_SIZE); >>>> >>>> As previously indicated, new code ought to use the xv*alloc family of >>>> functions, which deliberately doesn't include any ..._bytes() forms. >>>> Note how instead there is xvzalloc_flex_struct() for situations like >>>> the one here. >>> >>> Looks like xvzalloc_flex_struct() is not used anywhere in the code base... >> >> And what do you mean to tell me by that? xvmalloc_flex_struct() is used, >> and x[mz]alloc_flex_struct() uses are still in need of conversion (which >> is going to be a slow-going process). > > Just an observation; I was not aware of these APIs and I was not aware that > there's some slow-going process of switching to preferred use of > xvzalloc_flex_struct(). The ongoing conversion is (more generally) from x[mz]alloc family of functions to the xv[mz]alloc one, as is made explicit by the comment near the top of xvmalloc.h. Jan
© 2016 - 2025 Red Hat, Inc.