On Tue, 3 Feb 2026, Jan Beulich wrote:
> 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.)
I created a prereq patch which introduces ACCESS_ONCE everywhere for
console_rx
> 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().
I'll switch to the irqsave/restore versions in console_switch_input
>
> > @@ -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, .
I updated the in-code comment
>
> > @@ -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?
sure
> > 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.
I introduced a local copy
