Hi all, This short patch series enable the usage of console_io hypercalls for dom0less guests. Cheers, Stefano Allow multiple dom0less domains to use the console_io hypercalls to print to the console. Handle them in a similar way to vpl011: only the domain which has focus can read from the console. All domains can write to the console but the ones without focus have a prefix. In this case the prefix is applied by using guest_printk instead of printk or console_puts which is what the original code was already doing. When switching focus using Ctrl-AAA, discard any unread data in the input buffer. Input is read quickly and the user would be aware of it being slow or stuck as they use Ctrl-AAA to switch focus domain. In that situation, it is to be expected that the unread input is lost. The domain writes are buffered when the domain is not in focus. Push out the buffer after the domain enters focus on the first guest write. Locking updates: - Guard every mutation of serial_rx_cons/prod with console_lock and discard unread input under the lock when switching focus (including when returning to Xen) so that cross-domain reads can't see stale data - Require is_focus_domain() callers to hold console_lock, and take that lock around the entire CONSOLEIO_read loop, re-checking focus after each chunk so a focus change simply stops further copies without duplicating or leaking input - Hold cons->lock while flushing buffered writes in the focus path so the direct-write fast path does not race buffered guests or HVM debug output Signed-off-by: Stefano Stabellini <stefano.stabellini@amd.com> --- Changes in v7: - move vpl011 hunk to new patch - updated commit message - add ASSERT - const pointer - more in-code comments - move spin_unlock earlier - code style --- xen/drivers/char/console.c | 75 +++++++++++++++++++++++++++++++++----- 1 file changed, 65 insertions(+), 10 deletions(-) diff --git a/xen/drivers/char/console.c b/xen/drivers/char/console.c index XXXXXXX..XXXXXXX 100644 --- a/xen/drivers/char/console.c +++ b/xen/drivers/char/console.c @@ -XXX,XX +XXX,XX @@ void console_put_domain(struct domain *d) rcu_unlock_domain(d); } +static bool is_focus_domain(const struct domain *d) +{ + ASSERT(rspin_is_locked(&console_lock)); + return d != NULL && d->domain_id == console_rx - 1; +} + static void console_switch_input(void) { unsigned int next_rx = console_rx; @@ -XXX,XX +XXX,XX @@ static void console_switch_input(void) if ( next_rx++ >= max_console_rx ) { - console_rx = 0; printk("*** Serial input to Xen"); + nrspin_lock_irq(&console_lock); + console_rx = 0; + serial_rx_cons = serial_rx_prod; + nrspin_unlock_irq(&console_lock); break; } @@ -XXX,XX +XXX,XX @@ static void console_switch_input(void) rcu_unlock_domain(d); - console_rx = next_rx; printk("*** Serial input to DOM%u", domid); + nrspin_lock_irq(&console_lock); + console_rx = next_rx; + /* Don't let the next dom read the previous dom's unread data. */ + serial_rx_cons = serial_rx_prod; + nrspin_unlock_irq(&console_lock); break; } } @@ -XXX,XX +XXX,XX @@ static void __serial_rx(char c) * Deliver input to the hardware domain buffer, unless it is * already full. */ + nrspin_lock_irq(&console_lock); if ( (serial_rx_prod - serial_rx_cons) != SERIAL_RX_SIZE ) serial_rx_ring[SERIAL_RX_MASK(serial_rx_prod++)] = c; + nrspin_unlock_irq(&console_lock); /* * Always notify the hardware domain: prevents receive path from @@ -XXX,XX +XXX,XX @@ static long guest_console_write(XEN_GUEST_HANDLE_PARAM(char) buffer, unsigned int flags = opt_console_to_ring ? CONSOLE_ALL : CONSOLE_DEFAULT; struct domain *cd = current->domain; + struct domain_console *cons = cd->console; while ( count > 0 ) { @@ -XXX,XX +XXX,XX @@ static long guest_console_write(XEN_GUEST_HANDLE_PARAM(char) buffer, if ( copy_from_guest(kbuf, buffer, kcount) ) return -EFAULT; - if ( is_hardware_domain(cd) ) + /* + * Take both cons->lock and console_lock: + * - cons->lock protects cons->buf and cons->idx + * - console_lock protects console_send and is_focus_domain + * checks + * + * The order must be respected. guest_printk takes the + * console_lock so it is important that cons->lock is taken + * first. + */ + spin_lock(&cons->lock); + nrspin_lock_irq(&console_lock); + if ( is_focus_domain(cd) ) { + if ( cons->idx ) + { + console_send(cons->buf, cons->idx, flags); + cons->idx = 0; + } + spin_unlock(&cons->lock); + /* Use direct console output as it could be interactive */ - nrspin_lock_irq(&console_lock); console_send(kbuf, kcount, flags); nrspin_unlock_irq(&console_lock); } else { char *kin = kbuf, *kout = kbuf, c; - struct domain_console *cons = cd->console; + + nrspin_unlock_irq(&console_lock); /* Strip non-printable characters */ do @@ -XXX,XX +XXX,XX @@ static long guest_console_write(XEN_GUEST_HANDLE_PARAM(char) buffer, } while ( --kcount > 0 ); *kout = '\0'; - spin_lock(&cons->lock); kcount = kin - kbuf; if ( c != '\n' && (cons->idx + (kout - kbuf) < (ARRAY_SIZE(cons->buf) - 1)) ) @@ -XXX,XX +XXX,XX @@ long do_console_io( if ( count > INT_MAX ) break; + nrspin_lock_irq(&console_lock); + if ( !is_focus_domain(current->domain) ) + { + nrspin_unlock_irq(&console_lock); + return 0; + } + rc = 0; while ( (serial_rx_cons != serial_rx_prod) && (rc < count) ) { @@ -XXX,XX +XXX,XX @@ long do_console_io( len = SERIAL_RX_SIZE - idx; if ( (rc + len) > count ) len = count - rc; + nrspin_unlock_irq(&console_lock); if ( copy_to_guest_offset(buffer, rc, &serial_rx_ring[idx], len) ) - { - rc = -EFAULT; - break; - } + return -EFAULT; rc += len; + + /* + * First get console_lock again, then check is_focus_domain. + * It is possible that we switched focus domain during + * copy_to_guest_offset. In that case, serial_rx_cons and + * serial_rx_prod have been reset so it would be wrong to + * update serial_rx_cons here. Get out of the loop instead. + * + * rc is updated regardless to provide the correct return + * value to the VM as the data has been copied. + */ + nrspin_lock_irq(&console_lock); + if ( !is_focus_domain(current->domain) ) + break; + serial_rx_cons += len; } + nrspin_unlock_irq(&console_lock); break; default: rc = -ENOSYS; -- 2.25.1 Enable dom0less guests on ARM to use console_io hypercalls: - set input_allow = true for dom0less domains - update the in-code comment in console.c - prioritize the VUART check to retain the same behavior as today Signed-off-by: Stefano Stabellini <stefano.stabellini@amd.com> --- xen/common/device-tree/dom0less-build.c | 2 ++ xen/drivers/char/console.c | 17 ++++++++++------- 2 files changed, 12 insertions(+), 7 deletions(-) diff --git a/xen/common/device-tree/dom0less-build.c b/xen/common/device-tree/dom0less-build.c index XXXXXXX..XXXXXXX 100644 --- a/xen/common/device-tree/dom0less-build.c +++ b/xen/common/device-tree/dom0less-build.c @@ -XXX,XX +XXX,XX @@ static int __init construct_domU(struct kernel_info *kinfo, rangeset_destroy(kinfo->xen_reg_assigned); + d->console->input_allowed = true; + return rc; } diff --git a/xen/drivers/char/console.c b/xen/drivers/char/console.c index XXXXXXX..XXXXXXX 100644 --- a/xen/drivers/char/console.c +++ b/xen/drivers/char/console.c @@ -XXX,XX +XXX,XX @@ static void __serial_rx(char c) if ( !d ) return; - if ( is_hardware_domain(d) ) +#ifdef CONFIG_SBSA_VUART_CONSOLE + /* Prioritize vpl011 if enabled for this domain */ + if ( d->arch.vpl011.base_addr ) + { + /* Deliver input to the emulated UART. */ + rc = vpl011_rx_char_xen(d, c); + } + else +#endif { /* - * Deliver input to the hardware domain buffer, unless it is + * Deliver input to the focus domain buffer, unless it is * already full. */ nrspin_lock_irq(&console_lock); @@ -XXX,XX +XXX,XX @@ static void __serial_rx(char c) */ send_global_virq(VIRQ_CONSOLE); } -#ifdef CONFIG_SBSA_VUART_CONSOLE - else - /* Deliver input to the emulated UART. */ - rc = vpl011_rx_char_xen(d, c); -#endif if ( consoled_is_enabled() ) /* Deliver input to the PV shim console. */ -- 2.25.1 Hi all, This short patch series enable the usage of console_io hypercalls for dom0less guests. Cheers, Stefano There can be concurrent reads and writes to the console_rx variable so it is prudent to use ACCESS_ONCE. Signed-off-by: Stefano Stabellini <stefano.stabellini@amd.com> --- xen/drivers/char/console.c | 17 +++++++++-------- 1 file changed, 9 insertions(+), 8 deletions(-) diff --git a/xen/drivers/char/console.c b/xen/drivers/char/console.c index XXXXXXX..XXXXXXX 100644 --- a/xen/drivers/char/console.c +++ b/xen/drivers/char/console.c @@ -XXX,XX +XXX,XX @@ static unsigned int __read_mostly console_rx = 0; struct domain *console_get_domain(void) { struct domain *d; + unsigned int rx = ACCESS_ONCE(console_rx); - if ( console_rx == 0 ) - return NULL; + if ( rx == 0 ) + return NULL; - d = rcu_lock_domain_by_id(console_rx - 1); + d = rcu_lock_domain_by_id(rx - 1); if ( !d ) return NULL; @@ -XXX,XX +XXX,XX @@ void console_put_domain(struct domain *d) static void console_switch_input(void) { - unsigned int next_rx = console_rx; + unsigned int next_rx = ACCESS_ONCE(console_rx); /* * Rotate among Xen, dom0 and boot-time created domUs while skipping @@ -XXX,XX +XXX,XX @@ static void console_switch_input(void) if ( next_rx++ >= max_console_rx ) { - console_rx = 0; + ACCESS_ONCE(console_rx) = 0; printk("*** Serial input to Xen"); break; } @@ -XXX,XX +XXX,XX @@ static void console_switch_input(void) rcu_unlock_domain(d); - console_rx = next_rx; + ACCESS_ONCE(console_rx) = next_rx; printk("*** Serial input to DOM%u", domid); break; } @@ -XXX,XX +XXX,XX @@ static void __serial_rx(char c) struct domain *d; int rc = 0; - if ( console_rx == 0 ) + if ( ACCESS_ONCE(console_rx) == 0 ) return handle_keypress(c, false); d = console_get_domain(); @@ -XXX,XX +XXX,XX @@ void __init console_endboot(void) * a useful 'how to switch' message. */ if ( opt_conswitch[1] == 'x' ) - console_rx = max_console_rx; + ACCESS_ONCE(console_rx) = max_console_rx; register_keyhandler('w', conring_dump_keyhandler, "synchronously dump console ring buffer (dmesg)", 0); -- 2.25.1 Today only hwdom can bind VIRQ_CONSOLE. This patch changes the virq from global to VIRQ_DOMAIN to allow other domains to bind to it. Note that Linux silently falls back to polling when binding fails, which masks the issue. Signed-off-by: Jason Andryuk <jason.andryuk@amd.com> Signed-off-by: Stefano Stabellini <stefano.stabellini@amd.com> --- xen/common/event_channel.c | 1 + xen/drivers/char/console.c | 2 +- 2 files changed, 2 insertions(+), 1 deletion(-) diff --git a/xen/common/event_channel.c b/xen/common/event_channel.c index XXXXXXX..XXXXXXX 100644 --- a/xen/common/event_channel.c +++ b/xen/common/event_channel.c @@ -XXX,XX +XXX,XX @@ static enum virq_type get_virq_type(unsigned int virq) return VIRQ_VCPU; case VIRQ_ARGO: + case VIRQ_CONSOLE: return VIRQ_DOMAIN; case VIRQ_ARCH_0 ... VIRQ_ARCH_7: diff --git a/xen/drivers/char/console.c b/xen/drivers/char/console.c index XXXXXXX..XXXXXXX 100644 --- a/xen/drivers/char/console.c +++ b/xen/drivers/char/console.c @@ -XXX,XX +XXX,XX @@ static void __serial_rx(char c) * Always notify the hardware domain: prevents receive path from * getting stuck. */ - send_global_virq(VIRQ_CONSOLE); + send_guest_domain_virq(d, VIRQ_CONSOLE); } #ifdef CONFIG_SBSA_VUART_CONSOLE else -- 2.25.1 Allow multiple dom0less domains to use the console_io hypercalls to print to the console. Handle them in a similar way to vpl011: only the domain which has focus can read from the console. All domains can write to the console but the ones without focus have a prefix. In this case the prefix is applied by using guest_printk instead of printk or console_puts which is what the original code was already doing. When switching focus using Ctrl-AAA, discard any unread data in the input buffer. Input is read quickly and the user would be aware of it being slow or stuck as they use Ctrl-AAA to switch focus domain. In that situation, it is to be expected that the unread input is lost. The domain writes are buffered when the domain is not in focus. Push out the buffer after the domain enters focus on the first guest write. Locking updates: - Guard every mutation of serial_rx_cons/prod with console_lock and discard unread input under the lock when switching focus (including when returning to Xen) so that cross-domain reads can't see stale data - Require is_focus_domain() callers to hold console_lock, and take that lock around the entire CONSOLEIO_read loop, re-checking focus after each chunk so a focus change simply stops further copies without duplicating or leaking input - Hold cons->lock while flushing buffered writes in the focus path so the direct-write fast path does not race buffered guests or HVM debug output Signed-off-by: Stefano Stabellini <stefano.stabellini@amd.com> --- Changes in v9: - use irqsave/irqrestore in console_switch_input - update in-code comment - blank lines - local copy of serial_rx_ring --- xen/drivers/char/console.c | 78 +++++++++++++++++++++++++++++++++----- 1 file changed, 68 insertions(+), 10 deletions(-) diff --git a/xen/drivers/char/console.c b/xen/drivers/char/console.c index XXXXXXX..XXXXXXX 100644 --- a/xen/drivers/char/console.c +++ b/xen/drivers/char/console.c @@ -XXX,XX +XXX,XX @@ void console_put_domain(struct domain *d) rcu_unlock_domain(d); } +static bool is_focus_domain(const struct domain *d) +{ + ASSERT(rspin_is_locked(&console_lock)); + return d != NULL && d->domain_id == console_rx - 1; +} + static void console_switch_input(void) { unsigned int next_rx = ACCESS_ONCE(console_rx); @@ -XXX,XX +XXX,XX @@ static void console_switch_input(void) { domid_t domid; struct domain *d; + unsigned long flags; if ( next_rx++ >= max_console_rx ) { + nrspin_lock_irqsave(&console_lock, flags); ACCESS_ONCE(console_rx) = 0; + serial_rx_cons = serial_rx_prod; + nrspin_unlock_irqrestore(&console_lock, flags); printk("*** Serial input to Xen"); break; } @@ -XXX,XX +XXX,XX @@ static void console_switch_input(void) rcu_unlock_domain(d); + nrspin_lock_irqsave(&console_lock, flags); ACCESS_ONCE(console_rx) = next_rx; + /* Don't let the next dom read the previous dom's unread data. */ + serial_rx_cons = serial_rx_prod; + nrspin_unlock_irqrestore(&console_lock, flags); printk("*** Serial input to DOM%u", domid); break; } @@ -XXX,XX +XXX,XX @@ static void __serial_rx(char c) if ( is_hardware_domain(d) ) { + unsigned long flags; + /* - * Deliver input to the hardware domain buffer, unless it is + * Deliver input to the focus domain buffer, unless it is * already full. */ + nrspin_lock_irqsave(&console_lock, flags); if ( (serial_rx_prod - serial_rx_cons) != SERIAL_RX_SIZE ) serial_rx_ring[SERIAL_RX_MASK(serial_rx_prod++)] = c; + nrspin_unlock_irqrestore(&console_lock, flags); /* * Always notify the hardware domain: prevents receive path from @@ -XXX,XX +XXX,XX @@ static long guest_console_write(XEN_GUEST_HANDLE_PARAM(char) buffer, unsigned int flags = opt_console_to_ring ? CONSOLE_ALL : CONSOLE_DEFAULT; struct domain *cd = current->domain; + struct domain_console *cons = cd->console; while ( count > 0 ) { @@ -XXX,XX +XXX,XX @@ static long guest_console_write(XEN_GUEST_HANDLE_PARAM(char) buffer, if ( copy_from_guest(kbuf, buffer, kcount) ) return -EFAULT; - if ( is_hardware_domain(cd) ) + /* + * Take both cons->lock and console_lock: + * - cons->lock protects cons->buf and cons->idx + * - console_lock protects console_send() and is_focus_domain() + * checks + * + * The order must be respected. guest_printk() takes the + * console_lock and it is called with cons->lock held. It is + * important that cons->lock is taken first. + */ + spin_lock(&cons->lock); + nrspin_lock_irq(&console_lock); + if ( is_focus_domain(cd) ) { + if ( cons->idx ) + { + console_send(cons->buf, cons->idx, flags); + cons->idx = 0; + } + spin_unlock(&cons->lock); + /* Use direct console output as it could be interactive */ - nrspin_lock_irq(&console_lock); console_send(kbuf, kcount, flags); nrspin_unlock_irq(&console_lock); } else { char *kin = kbuf, *kout = kbuf, c; - struct domain_console *cons = cd->console; + + nrspin_unlock_irq(&console_lock); /* Strip non-printable characters */ do @@ -XXX,XX +XXX,XX @@ static long guest_console_write(XEN_GUEST_HANDLE_PARAM(char) buffer, } while ( --kcount > 0 ); *kout = '\0'; - spin_lock(&cons->lock); kcount = kin - kbuf; if ( c != '\n' && (cons->idx + (kout - kbuf) < (ARRAY_SIZE(cons->buf) - 1)) ) @@ -XXX,XX +XXX,XX @@ long do_console_io( { long rc; unsigned int idx, len; + char kbuf[SERIAL_RX_SIZE]; rc = xsm_console_io(XSM_OTHER, current->domain, cmd); if ( rc ) @@ -XXX,XX +XXX,XX @@ long do_console_io( break; rc = 0; + nrspin_lock_irq(&console_lock); + if ( !is_focus_domain(current->domain) ) + count = 0; while ( (serial_rx_cons != serial_rx_prod) && (rc < count) ) { idx = SERIAL_RX_MASK(serial_rx_cons); @@ -XXX,XX +XXX,XX @@ long do_console_io( len = SERIAL_RX_SIZE - idx; if ( (rc + len) > count ) len = count - rc; - if ( copy_to_guest_offset(buffer, rc, &serial_rx_ring[idx], len) ) - { - rc = -EFAULT; - break; - } + memcpy(kbuf, &serial_rx_ring[idx], len); + + nrspin_unlock_irq(&console_lock); + + if ( copy_to_guest_offset(buffer, rc, kbuf, len) ) + return -EFAULT; rc += len; + + /* + * First get console_lock again, then check is_focus_domain(). + * It is possible that we switched focus domain during + * copy_to_guest_offset(). In that case, serial_rx_cons and + * serial_rx_prod have been reset so it would be wrong to + * update serial_rx_cons here. Get out of the loop instead. + * + * rc is updated regardless to provide the correct return + * value to the VM as the data has been copied. + */ + nrspin_lock_irq(&console_lock); + if ( !is_focus_domain(current->domain) ) + break; + serial_rx_cons += len; } + nrspin_unlock_irq(&console_lock); break; default: rc = -ENOSYS; -- 2.25.1 Enable dom0less guests on ARM to use console_io hypercalls: - set input_allow = true for dom0less domains - update the in-code comment in console.c - prioritize the VUART check to retain the same behavior as today Signed-off-by: Stefano Stabellini <stefano.stabellini@amd.com> --- xen/common/device-tree/dom0less-build.c | 2 ++ xen/drivers/char/console.c | 16 ++++++++++------ 2 files changed, 12 insertions(+), 6 deletions(-) diff --git a/xen/common/device-tree/dom0less-build.c b/xen/common/device-tree/dom0less-build.c index XXXXXXX..XXXXXXX 100644 --- a/xen/common/device-tree/dom0less-build.c +++ b/xen/common/device-tree/dom0less-build.c @@ -XXX,XX +XXX,XX @@ static int __init construct_domU(struct kernel_info *kinfo, rangeset_destroy(kinfo->xen_reg_assigned); + d->console->input_allowed = true; + return rc; } diff --git a/xen/drivers/char/console.c b/xen/drivers/char/console.c index XXXXXXX..XXXXXXX 100644 --- a/xen/drivers/char/console.c +++ b/xen/drivers/char/console.c @@ -XXX,XX +XXX,XX @@ static void __serial_rx(char c) if ( ACCESS_ONCE(console_rx) == 0 ) return handle_keypress(c, false); + /* Includes an is_focus_domain() check. */ d = console_get_domain(); if ( !d ) return; - if ( is_hardware_domain(d) ) +#ifdef CONFIG_SBSA_VUART_CONSOLE + /* Prioritize vpl011 if enabled for this domain */ + if ( d->arch.vpl011.base_addr ) + { + /* Deliver input to the emulated UART. */ + rc = vpl011_rx_char_xen(d, c); + } + else +#endif { unsigned long flags; @@ -XXX,XX +XXX,XX @@ static void __serial_rx(char c) */ send_guest_domain_virq(d, VIRQ_CONSOLE); } -#ifdef CONFIG_SBSA_VUART_CONSOLE - else - /* Deliver input to the emulated UART. */ - rc = vpl011_rx_char_xen(d, c); -#endif if ( consoled_is_enabled() ) /* Deliver input to the PV shim console. */ -- 2.25.1