From: Denis Mukhin <dmkhn@proton.me>
From: Denis Mukhin <dmukhin@ford.com>
Add new flag to domain structure for marking permission to intercept
the physical console input by the domain.
Update console input switch logic accordingly.
No functional change intended.
Signed-off-by: Denis Mukhin <dmukhin@ford.com>
---
Changes since v3:
- rebased
---
xen/arch/arm/vpl011.c | 2 ++
xen/arch/x86/pv/shim.c | 2 ++
xen/common/domain.c | 2 ++
xen/drivers/char/console.c | 18 +++++++++++++++++-
xen/include/xen/sched.h | 8 +++++++-
5 files changed, 30 insertions(+), 2 deletions(-)
diff --git a/xen/arch/arm/vpl011.c b/xen/arch/arm/vpl011.c
index 66047bf33c..147958eee8 100644
--- a/xen/arch/arm/vpl011.c
+++ b/xen/arch/arm/vpl011.c
@@ -737,6 +737,8 @@ int domain_vpl011_init(struct domain *d, struct vpl011_init_info *info)
register_mmio_handler(d, &vpl011_mmio_handler,
vpl011->base_addr, GUEST_PL011_SIZE, NULL);
+ d->console.input_allowed = true;
+
return 0;
out1:
diff --git a/xen/arch/x86/pv/shim.c b/xen/arch/x86/pv/shim.c
index c506cc0bec..bc2a7dd5fa 100644
--- a/xen/arch/x86/pv/shim.c
+++ b/xen/arch/x86/pv/shim.c
@@ -238,6 +238,8 @@ void __init pv_shim_setup_dom(struct domain *d, l4_pgentry_t *l4start,
* guest from depleting the shim memory pool.
*/
d->max_pages = domain_tot_pages(d);
+
+ 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 87e5be35e5..9bc66d80c4 100644
--- a/xen/common/domain.c
+++ b/xen/common/domain.c
@@ -835,6 +835,8 @@ 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. */
diff --git a/xen/drivers/char/console.c b/xen/drivers/char/console.c
index 30701ae0b0..8a0bcff78f 100644
--- a/xen/drivers/char/console.c
+++ b/xen/drivers/char/console.c
@@ -512,9 +512,21 @@ static unsigned int __read_mostly console_rx = 0;
struct domain *console_get_domain(void)
{
+ struct domain *d;
+
if ( console_rx == 0 )
return NULL;
- return rcu_lock_domain_by_id(console_rx - 1);
+
+ d = rcu_lock_domain_by_id(console_rx - 1);
+ if ( !d )
+ return NULL;
+
+ if ( d->console.input_allowed )
+ return d;
+
+ rcu_unlock_domain(d);
+
+ return NULL;
}
void console_put_domain(struct domain *d)
@@ -551,6 +563,10 @@ static void console_switch_input(void)
if ( d )
{
rcu_unlock_domain(d);
+
+ if ( !d->console.input_allowed )
+ break;
+
console_rx = next_rx;
printk("*** Serial input to DOM%u", domid);
break;
diff --git a/xen/include/xen/sched.h b/xen/include/xen/sched.h
index 559d201e0c..e91c99a8f3 100644
--- a/xen/include/xen/sched.h
+++ b/xen/include/xen/sched.h
@@ -512,7 +512,7 @@ struct domain
bool auto_node_affinity;
/* Is this guest fully privileged (aka dom0)? */
bool is_privileged;
- /* Can this guest access the Xen console? */
+ /* XSM: permission to use HYPERCALL_console_io hypercall */
bool is_console;
/* Is this guest being debugged by dom0? */
bool debugger_attached;
@@ -651,6 +651,12 @@ struct domain
unsigned int num_llc_colors;
const unsigned int *llc_colors;
#endif
+
+ /* Console settings. */
+ struct {
+ /* Permission to take ownership of the physical console input. */
+ bool input_allowed;
+ } console;
} __aligned(PAGE_SIZE);
static inline struct page_list_head *page_to_list(
--
2.34.1On Thu, 29 May 2025, dmkhn@proton.me wrote:
> Add new flag to domain structure for marking permission to intercept
> the physical console input by the domain.
>
> Update console input switch logic accordingly.
>
> No functional change intended.
>
> Signed-off-by: Denis Mukhin <dmukhin@ford.com>
> ---
> Changes since v3:
> - rebased
> ---
> xen/arch/arm/vpl011.c | 2 ++
> xen/arch/x86/pv/shim.c | 2 ++
> xen/common/domain.c | 2 ++
> xen/drivers/char/console.c | 18 +++++++++++++++++-
> xen/include/xen/sched.h | 8 +++++++-
> 5 files changed, 30 insertions(+), 2 deletions(-)
>
> diff --git a/xen/arch/arm/vpl011.c b/xen/arch/arm/vpl011.c
> index 66047bf33c..147958eee8 100644
> --- a/xen/arch/arm/vpl011.c
> +++ b/xen/arch/arm/vpl011.c
> @@ -737,6 +737,8 @@ int domain_vpl011_init(struct domain *d, struct vpl011_init_info *info)
> register_mmio_handler(d, &vpl011_mmio_handler,
> vpl011->base_addr, GUEST_PL011_SIZE, NULL);
>
> + d->console.input_allowed = true;
This should be set only when backend_in_domain = false.
> return 0;
>
> out1:
> diff --git a/xen/arch/x86/pv/shim.c b/xen/arch/x86/pv/shim.c
> index c506cc0bec..bc2a7dd5fa 100644
> --- a/xen/arch/x86/pv/shim.c
> +++ b/xen/arch/x86/pv/shim.c
> @@ -238,6 +238,8 @@ void __init pv_shim_setup_dom(struct domain *d, l4_pgentry_t *l4start,
> * guest from depleting the shim memory pool.
> */
> d->max_pages = domain_tot_pages(d);
> +
> + 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 87e5be35e5..9bc66d80c4 100644
> --- a/xen/common/domain.c
> +++ b/xen/common/domain.c
> @@ -835,6 +835,8 @@ 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. */
> diff --git a/xen/drivers/char/console.c b/xen/drivers/char/console.c
> index 30701ae0b0..8a0bcff78f 100644
> --- a/xen/drivers/char/console.c
> +++ b/xen/drivers/char/console.c
> @@ -512,9 +512,21 @@ static unsigned int __read_mostly console_rx = 0;
>
> struct domain *console_get_domain(void)
> {
> + struct domain *d;
> +
> if ( console_rx == 0 )
> return NULL;
> - return rcu_lock_domain_by_id(console_rx - 1);
> +
> + d = rcu_lock_domain_by_id(console_rx - 1);
> + if ( !d )
> + return NULL;
> +
> + if ( d->console.input_allowed )
> + return d;
> +
> + rcu_unlock_domain(d);
> +
> + return NULL;
The original idea was to skip over domains that cannot have any input so
I don't think we should get in this situation. We could even have an
assert.
> }
>
> void console_put_domain(struct domain *d)
> @@ -551,6 +563,10 @@ static void console_switch_input(void)
> if ( d )
> {
> rcu_unlock_domain(d);
> +
> + if ( !d->console.input_allowed )
> + break;
shouldn't this be continue instead of break?
> console_rx = next_rx;
> printk("*** Serial input to DOM%u", domid);
> break;
> diff --git a/xen/include/xen/sched.h b/xen/include/xen/sched.h
> index 559d201e0c..e91c99a8f3 100644
> --- a/xen/include/xen/sched.h
> +++ b/xen/include/xen/sched.h
> @@ -512,7 +512,7 @@ struct domain
> bool auto_node_affinity;
> /* Is this guest fully privileged (aka dom0)? */
> bool is_privileged;
> - /* Can this guest access the Xen console? */
> + /* XSM: permission to use HYPERCALL_console_io hypercall */
> bool is_console;
While I am in favor of this direction and we certainly need a better way
to distinguish domains that can use HYPERCALL_console_io hypercall from
others, could we simplify this and just assume that "is_console" implies
input_allowed and also set is_console = true in all the same places you
are setting input_allowed = true in this patch?
For clarity, I am suggesting:
- do not add input_allowed
- set is_console = true in domain_vpl011_init, pv_shim_setup_dom, etc.
The only side effect is that we would allow domains with vpl011 to also
use console hypercalls but I don't think there is any harm in that?
I don't feel strongly about this, I am just trying to find ways to make
things simple. I apologize if it was already discussed during review of
one of the previous versions.
I am also OK with this approach.
> /* Is this guest being debugged by dom0? */
> bool debugger_attached;
> @@ -651,6 +651,12 @@ struct domain
> unsigned int num_llc_colors;
> const unsigned int *llc_colors;
> #endif
> +
> + /* Console settings. */
> + struct {
> + /* Permission to take ownership of the physical console input. */
> + bool input_allowed;
> + } console;
> } __aligned(PAGE_SIZE);
>
> static inline struct page_list_head *page_to_list(
> --
> 2.34.1
>
>
On Thu, May 29, 2025 at 05:58:00PM -0700, Stefano Stabellini wrote:
> On Thu, 29 May 2025, dmkhn@proton.me wrote:
> > Add new flag to domain structure for marking permission to intercept
> > the physical console input by the domain.
> >
> > Update console input switch logic accordingly.
> >
> > No functional change intended.
> >
> > Signed-off-by: Denis Mukhin <dmukhin@ford.com>
> > ---
> > Changes since v3:
> > - rebased
> > ---
> > xen/arch/arm/vpl011.c | 2 ++
> > xen/arch/x86/pv/shim.c | 2 ++
> > xen/common/domain.c | 2 ++
> > xen/drivers/char/console.c | 18 +++++++++++++++++-
> > xen/include/xen/sched.h | 8 +++++++-
> > 5 files changed, 30 insertions(+), 2 deletions(-)
> >
> > diff --git a/xen/arch/arm/vpl011.c b/xen/arch/arm/vpl011.c
> > index 66047bf33c..147958eee8 100644
> > --- a/xen/arch/arm/vpl011.c
> > +++ b/xen/arch/arm/vpl011.c
> > @@ -737,6 +737,8 @@ int domain_vpl011_init(struct domain *d, struct vpl011_init_info *info)
> > register_mmio_handler(d, &vpl011_mmio_handler,
> > vpl011->base_addr, GUEST_PL011_SIZE, NULL);
> >
> > + d->console.input_allowed = true;
>
> This should be set only when backend_in_domain = false.
>
>
> > return 0;
> >
> > out1:
> > diff --git a/xen/arch/x86/pv/shim.c b/xen/arch/x86/pv/shim.c
> > index c506cc0bec..bc2a7dd5fa 100644
> > --- a/xen/arch/x86/pv/shim.c
> > +++ b/xen/arch/x86/pv/shim.c
> > @@ -238,6 +238,8 @@ void __init pv_shim_setup_dom(struct domain *d, l4_pgentry_t *l4start,
> > * guest from depleting the shim memory pool.
> > */
> > d->max_pages = domain_tot_pages(d);
> > +
> > + 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 87e5be35e5..9bc66d80c4 100644
> > --- a/xen/common/domain.c
> > +++ b/xen/common/domain.c
> > @@ -835,6 +835,8 @@ 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. */
> > diff --git a/xen/drivers/char/console.c b/xen/drivers/char/console.c
> > index 30701ae0b0..8a0bcff78f 100644
> > --- a/xen/drivers/char/console.c
> > +++ b/xen/drivers/char/console.c
> > @@ -512,9 +512,21 @@ static unsigned int __read_mostly console_rx = 0;
> >
> > struct domain *console_get_domain(void)
> > {
> > + struct domain *d;
> > +
> > if ( console_rx == 0 )
> > return NULL;
> > - return rcu_lock_domain_by_id(console_rx - 1);
> > +
> > + d = rcu_lock_domain_by_id(console_rx - 1);
> > + if ( !d )
> > + return NULL;
> > +
> > + if ( d->console.input_allowed )
> > + return d;
> > +
> > + rcu_unlock_domain(d);
> > +
> > + return NULL;
>
> The original idea was to skip over domains that cannot have any input so
> I don't think we should get in this situation. We could even have an
> assert.
>
>
> > }
> >
> > void console_put_domain(struct domain *d)
> > @@ -551,6 +563,10 @@ static void console_switch_input(void)
> > if ( d )
> > {
> > rcu_unlock_domain(d);
> > +
> > + if ( !d->console.input_allowed )
> > + break;
>
> shouldn't this be continue instead of break?
>
>
> > console_rx = next_rx;
> > printk("*** Serial input to DOM%u", domid);
> > break;
> > diff --git a/xen/include/xen/sched.h b/xen/include/xen/sched.h
> > index 559d201e0c..e91c99a8f3 100644
> > --- a/xen/include/xen/sched.h
> > +++ b/xen/include/xen/sched.h
> > @@ -512,7 +512,7 @@ struct domain
> > bool auto_node_affinity;
> > /* Is this guest fully privileged (aka dom0)? */
> > bool is_privileged;
> > - /* Can this guest access the Xen console? */
> > + /* XSM: permission to use HYPERCALL_console_io hypercall */
> > bool is_console;
>
> While I am in favor of this direction and we certainly need a better way
> to distinguish domains that can use HYPERCALL_console_io hypercall from
> others, could we simplify this and just assume that "is_console" implies
> input_allowed and also set is_console = true in all the same places you
> are setting input_allowed = true in this patch?
>
> For clarity, I am suggesting:
> - do not add input_allowed
> - set is_console = true in domain_vpl011_init, pv_shim_setup_dom, etc.
>
> The only side effect is that we would allow domains with vpl011 to also
> use console hypercalls but I don't think there is any harm in that?
>
> I don't feel strongly about this, I am just trying to find ways to make
> things simple. I apologize if it was already discussed during review of
> one of the previous versions.
There was feedback on using is_console:
https://lore.kernel.org/xen-devel/e899f63b-6182-4b53-9fb4-9a821e75648b@suse.com/
AFAIU, since XSM is the existing user of is_console, there should be a new
separate flag to avoid collision with the existing one.
>
> I am also OK with this approach.
>
>
> > /* Is this guest being debugged by dom0? */
> > bool debugger_attached;
> > @@ -651,6 +651,12 @@ struct domain
> > unsigned int num_llc_colors;
> > const unsigned int *llc_colors;
> > #endif
> > +
> > + /* Console settings. */
> > + struct {
> > + /* Permission to take ownership of the physical console input. */
> > + bool input_allowed;
> > + } console;
> > } __aligned(PAGE_SIZE);
> >
> > static inline struct page_list_head *page_to_list(
> > --
> > 2.34.1
> >
> >
>
On Fri, 30 May 2025, dmkhn@proton.me wrote:
> On Thu, May 29, 2025 at 05:58:00PM -0700, Stefano Stabellini wrote:
> > On Thu, 29 May 2025, dmkhn@proton.me wrote:
> > > Add new flag to domain structure for marking permission to intercept
> > > the physical console input by the domain.
> > >
> > > Update console input switch logic accordingly.
> > >
> > > No functional change intended.
> > >
> > > Signed-off-by: Denis Mukhin <dmukhin@ford.com>
> > > ---
> > > Changes since v3:
> > > - rebased
> > > ---
> > > xen/arch/arm/vpl011.c | 2 ++
> > > xen/arch/x86/pv/shim.c | 2 ++
> > > xen/common/domain.c | 2 ++
> > > xen/drivers/char/console.c | 18 +++++++++++++++++-
> > > xen/include/xen/sched.h | 8 +++++++-
> > > 5 files changed, 30 insertions(+), 2 deletions(-)
> > >
> > > diff --git a/xen/arch/arm/vpl011.c b/xen/arch/arm/vpl011.c
> > > index 66047bf33c..147958eee8 100644
> > > --- a/xen/arch/arm/vpl011.c
> > > +++ b/xen/arch/arm/vpl011.c
> > > @@ -737,6 +737,8 @@ int domain_vpl011_init(struct domain *d, struct vpl011_init_info *info)
> > > register_mmio_handler(d, &vpl011_mmio_handler,
> > > vpl011->base_addr, GUEST_PL011_SIZE, NULL);
> > >
> > > + d->console.input_allowed = true;
> >
> > This should be set only when backend_in_domain = false.
> >
> >
> > > return 0;
> > >
> > > out1:
> > > diff --git a/xen/arch/x86/pv/shim.c b/xen/arch/x86/pv/shim.c
> > > index c506cc0bec..bc2a7dd5fa 100644
> > > --- a/xen/arch/x86/pv/shim.c
> > > +++ b/xen/arch/x86/pv/shim.c
> > > @@ -238,6 +238,8 @@ void __init pv_shim_setup_dom(struct domain *d, l4_pgentry_t *l4start,
> > > * guest from depleting the shim memory pool.
> > > */
> > > d->max_pages = domain_tot_pages(d);
> > > +
> > > + 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 87e5be35e5..9bc66d80c4 100644
> > > --- a/xen/common/domain.c
> > > +++ b/xen/common/domain.c
> > > @@ -835,6 +835,8 @@ 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. */
> > > diff --git a/xen/drivers/char/console.c b/xen/drivers/char/console.c
> > > index 30701ae0b0..8a0bcff78f 100644
> > > --- a/xen/drivers/char/console.c
> > > +++ b/xen/drivers/char/console.c
> > > @@ -512,9 +512,21 @@ static unsigned int __read_mostly console_rx = 0;
> > >
> > > struct domain *console_get_domain(void)
> > > {
> > > + struct domain *d;
> > > +
> > > if ( console_rx == 0 )
> > > return NULL;
> > > - return rcu_lock_domain_by_id(console_rx - 1);
> > > +
> > > + d = rcu_lock_domain_by_id(console_rx - 1);
> > > + if ( !d )
> > > + return NULL;
> > > +
> > > + if ( d->console.input_allowed )
> > > + return d;
> > > +
> > > + rcu_unlock_domain(d);
> > > +
> > > + return NULL;
> >
> > The original idea was to skip over domains that cannot have any input so
> > I don't think we should get in this situation. We could even have an
> > assert.
> >
> >
> > > }
> > >
> > > void console_put_domain(struct domain *d)
> > > @@ -551,6 +563,10 @@ static void console_switch_input(void)
> > > if ( d )
> > > {
> > > rcu_unlock_domain(d);
> > > +
> > > + if ( !d->console.input_allowed )
> > > + break;
> >
> > shouldn't this be continue instead of break?
> >
> >
> > > console_rx = next_rx;
> > > printk("*** Serial input to DOM%u", domid);
> > > break;
> > > diff --git a/xen/include/xen/sched.h b/xen/include/xen/sched.h
> > > index 559d201e0c..e91c99a8f3 100644
> > > --- a/xen/include/xen/sched.h
> > > +++ b/xen/include/xen/sched.h
> > > @@ -512,7 +512,7 @@ struct domain
> > > bool auto_node_affinity;
> > > /* Is this guest fully privileged (aka dom0)? */
> > > bool is_privileged;
> > > - /* Can this guest access the Xen console? */
> > > + /* XSM: permission to use HYPERCALL_console_io hypercall */
> > > bool is_console;
> >
> > While I am in favor of this direction and we certainly need a better way
> > to distinguish domains that can use HYPERCALL_console_io hypercall from
> > others, could we simplify this and just assume that "is_console" implies
> > input_allowed and also set is_console = true in all the same places you
> > are setting input_allowed = true in this patch?
> >
> > For clarity, I am suggesting:
> > - do not add input_allowed
> > - set is_console = true in domain_vpl011_init, pv_shim_setup_dom, etc.
> >
> > The only side effect is that we would allow domains with vpl011 to also
> > use console hypercalls but I don't think there is any harm in that?
> >
> > I don't feel strongly about this, I am just trying to find ways to make
> > things simple. I apologize if it was already discussed during review of
> > one of the previous versions.
>
> There was feedback on using is_console:
>
> https://lore.kernel.org/xen-devel/e899f63b-6182-4b53-9fb4-9a821e75648b@suse.com/
>
> AFAIU, since XSM is the existing user of is_console, there should be a new
> separate flag to avoid collision with the existing one.
OK, I suspected as much. In that case, it is fine to continue with the
new flag.
© 2016 - 2026 Red Hat, Inc.