Re: [PATCH v10 3/5] xen/console: add locking for serial_rx ring buffer access

[Jan Beulich <jbeulich@xxxxxxxx>]

On 05.02.2026 00:37, Stefano Stabellini wrote:
> Guard every mutation of serial_rx_cons/prod with console_lock so that
> cross-domain reads can't see stale data:

Cross-domain reads become a thing the earliest in the next patch, though?
You say something along these lines ...

> - In console_switch_input(): protect console_rx assignment with the lock
>   using irqsave/irqrestore variants since this can be called from
>   interrupt context
> 
> - In __serial_rx(): protect the ring buffer write operation when
>   delivering input to the hardware domain
> 
> - In do_console_io() CONSOLEIO_read: hold the lock around the entire
>   read loop, using a local buffer copy to avoid holding the lock during
>   copy_to_guest_offset()
> 
> This is preparatory work for allowing multiple domains to use the
> console_io hypercalls where proper synchronization is required.

... here, but I think that initial part also wants slightly re-phrasing.
At the very least insert "in the future".

> --- a/xen/drivers/char/console.c
> +++ b/xen/drivers/char/console.c
> @@ -553,10 +553,13 @@ 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;
> +            nrspin_unlock_irqrestore(&console_lock, flags);
>              printk("*** Serial input to Xen");
>              break;
>          }
> @@ -576,7 +579,9 @@ static void console_switch_input(void)
>  
>              rcu_unlock_domain(d);
>  
> +            nrspin_lock_irqsave(&console_lock, flags);
>              ACCESS_ONCE(console_rx) = next_rx;
> +            nrspin_unlock_irqrestore(&console_lock, flags);
>              printk("*** Serial input to DOM%u", domid);
>              break;
>          }

In __serial_rx() and do_console_io() you guard more than the mere updating.
As said before, with this arrangement of locking next_rx can in principle
be stale by the time you use it for storing into console_rx. This
arrangement may be okay, but would then need commenting upon.

> @@ -796,6 +805,7 @@ long do_console_io(
>  {
>      long rc;
>      unsigned int idx, len;
> +    char kbuf[SERIAL_RX_SIZE];

Please can such live in the narrowest possible scope?

Jan