On 29.01.2026 23:08, Stefano Stabellini wrote:
> --- a/xen/drivers/char/console.c
> +++ b/xen/drivers/char/console.c
> @@ -518,11 +518,16 @@ static unsigned int __read_mostly console_rx = 0;
> struct domain *console_get_domain(void)
> {
> struct domain *d;
> + unsigned int rx;
>
> - if ( console_rx == 0 )
> + nrspin_lock(&console_lock);
> + rx = console_rx;
> + nrspin_unlock(&console_lock);
Did you test this in a debug build, and it didn't blow up? The console lock
is an IRQ-safe one, so I'd expect check_lock() to complain that you do not
disable IRQs here. At the same time I don't think you can rely on IRQs being
off upon entry into the function.
Anyway - is locking here really needed? Wouldn't suitable use of ACCESS_ONCE()
(also elsewhere) do? (Such a switchover likely could be a separate, prereq
patch.)
Further, if already you add locking on the read sides, what about ...
> @@ -540,6 +545,12 @@ 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;
... this read?
> @@ -555,7 +566,10 @@ static void console_switch_input(void)
>
> if ( next_rx++ >= max_console_rx )
> {
> + nrspin_lock_irq(&console_lock);
As indicated earlier, you can't know IRQ state in anything down the call
tree from serial_rx().
> @@ -742,17 +765,36 @@ 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);
While guest_printk() does matter here, it taking (down the call tree)
console_lock isn't alone relevant. It being called with cons->lock held
is, .
> @@ -816,22 +857,39 @@ 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);
> len = serial_rx_prod - serial_rx_cons;
> + nrspin_unlock_irq(&console_lock);
Can we please have blank lines on both sides of this?
> if ( (idx + len) > SERIAL_RX_SIZE )
> len = SERIAL_RX_SIZE - idx;
> if ( (rc + len) > count )
> len = count - rc;
> if ( copy_to_guest_offset(buffer, rc, &serial_rx_ring[idx], len)
> )
Have I perhaps talked you into moving the unlock too early? serial_rx_ring[]
accesses look like they need to be with the lock still held. Or, to avoid
calling copy_to_guest_offset() with the lock held, a local copy would need
making.
Jan
