| Message ID | 20250318233617.849903-3-dmukhin@ford.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | xen/console: cleanup console input switch logic | expand |
On 19.03.2025 00:36, dmkhn@proton.me wrote: > @@ -564,10 +586,25 @@ static void __serial_rx(char c) > /* Deliver input to the PV shim console. */ > rc = consoled_guest_tx(c); > > - if ( rc ) > + switch ( rc ) > + { > + case 0: > + break; > + > + case -EBUSY: /* Loopback mode */ > + case -ENOSPC: /* FIFO is full */ > guest_printk(d, > XENLOG_WARNING "failed to process console input: %d\n", > rc); > + break; > + > + default: > + d->console.input_allowed = false; This aspect isn't mentioned / justified in the description, and I also can't deduce why you would do so. Or to put it differently, why you'd then not also take away input focus from this domain, for it no longer being eligible to have focus. > --- 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 own physical console input. */ > + bool input_allowed; > + } console; Are further fields going to be added to this sub-struct? If not, is having a sub-struct here actually worth it? Jan
On Wednesday, March 26th, 2025 at 6:44 AM, Jan Beulich <jbeulich@suse.com> wrote: > > > On 19.03.2025 00:36, dmkhn@proton.me wrote: > > > @@ -564,10 +586,25 @@ static void __serial_rx(char c) > > /* Deliver input to the PV shim console. */ > > rc = consoled_guest_tx(c); > > > > - if ( rc ) > > + switch ( rc ) > > + { > > + case 0: > > + break; > > + > > + case -EBUSY: /* Loopback mode / > > + case -ENOSPC: / FIFO is full */ > > guest_printk(d, > > XENLOG_WARNING "failed to process console input: %d\n", > > rc); > > + break; > > + > > + default: > > + d->console.input_allowed = false; > > > This aspect isn't mentioned / justified in the description, and I also > can't deduce why you would do so. Or to put it differently, why you'd > then not also take away input focus from this domain, for it no longer > being eligible to have focus. My idea was to explicitly distinguish "recoverable" errors, such as "FIFO is full", from "emulator logical" errors when input character cannot be delivered to the domain because of an error in emulator, so those are easily seen in the debug logs. I re-inspected return values, the only values currently supported are: (1) 0: success (2) -ENODEV: domain is not configured to have vUART (3) -ENOSPC: FIFO is full -EBUSY is supposed to mean "vUART is in loopback mode" and it is leaked from the future NS16550 emulator code. I will drop that change in v2. > > > --- 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 own physical console input. */ > > + bool input_allowed; > > + } console; > > > Are further fields going to be added to this sub-struct? If not, is having > a sub-struct here actually worth it? I was thinking about grouping all console-related fields in struct domain here in the future: e.g. move pbuf-related fields under "struct console". > > Jan
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 585fd726a9..b9f549c617 100644 --- a/xen/common/domain.c +++ b/xen/common/domain.c @@ -827,6 +827,8 @@ struct domain *domain_create(domid_t domid, old_hwdom = hardware_domain; hardware_domain = d; + + d->console.input_allowed = true; } TRACE_TIME(TRC_DOM0_DOM_ADD, d->domain_id); diff --git a/xen/drivers/char/console.c b/xen/drivers/char/console.c index c3150fbdb7..d7d9800095 100644 --- a/xen/drivers/char/console.c +++ b/xen/drivers/char/console.c @@ -474,11 +474,26 @@ static unsigned int __read_mostly console_rx = 0; #define max_console_rx (max_init_domid + 1) +static struct domain *console_get_domain_by_id(domid_t domid) +{ + struct domain *d = rcu_lock_domain_by_id(domid); + + if ( !d ) + return NULL; + + if ( d->console.input_allowed ) + return d; + + rcu_unlock_domain(d); + + return NULL; +} + struct domain *console_get_domain(void) { if ( console_rx == 0 ) return NULL; - return rcu_lock_domain_by_id(console_rx - 1); + return console_get_domain_by_id(console_rx - 1); } void console_put_domain(struct domain *d) @@ -487,6 +502,15 @@ void console_put_domain(struct domain *d) rcu_unlock_domain(d); } +static bool console_check_focus_by_id(domid_t domid) +{ + struct domain *d = console_get_domain_by_id(domid); + + console_put_domain(d); + + return !!d; +} + static void switch_serial_input(void) { unsigned int next_rx = console_rx; @@ -498,7 +522,6 @@ static void switch_serial_input(void) for ( ; ; ) { domid_t domid; - struct domain *d; if ( next_rx++ >= max_console_rx ) { @@ -511,10 +534,9 @@ static void switch_serial_input(void) domid = get_initial_domain_id(); else domid = next_rx - 1; - d = rcu_lock_domain_by_id(domid); - if ( d ) + + if ( console_check_focus_by_id(domid) ) { - rcu_unlock_domain(d); console_rx = next_rx; printk("*** Serial input to DOM%u", domid); break; @@ -564,10 +586,25 @@ static void __serial_rx(char c) /* Deliver input to the PV shim console. */ rc = consoled_guest_tx(c); - if ( rc ) + switch ( rc ) + { + case 0: + break; + + case -EBUSY: /* Loopback mode */ + case -ENOSPC: /* FIFO is full */ guest_printk(d, XENLOG_WARNING "failed to process console input: %d\n", rc); + break; + + default: + d->console.input_allowed = false; + guest_printk(d, + XENLOG_ERR "disabled console input: %d\n", + rc); + break; + } console_put_domain(d); } diff --git a/xen/include/xen/sched.h b/xen/include/xen/sched.h index 559d201e0c..292b1a91f9 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 own physical console input. */ + bool input_allowed; + } console; } __aligned(PAGE_SIZE); static inline struct page_list_head *page_to_list(
Add new flag in domain structure for marking permission to intercept the physical console input by the domain. Update console input switch logic accordingly. Signed-off-by: Denis Mukhin <dmukhin@ford.com> --- xen/arch/arm/vpl011.c | 2 ++ xen/arch/x86/pv/shim.c | 2 ++ xen/common/domain.c | 2 ++ xen/drivers/char/console.c | 49 +++++++++++++++++++++++++++++++++----- xen/include/xen/sched.h | 8 ++++++- 5 files changed, 56 insertions(+), 7 deletions(-)