Re: [PATCH RESEND] virtio_console: read size from config space during device init

["Michael S. Tsirkin" <mst@redhat.com>]

On Thu, Jun 11, 2026 at 08:57:57AM +0200, Filip Hejsek wrote:
> On Wed, 2026-06-10 at 03:04 -0400, Michael S. Tsirkin wrote:
> > On Mon, Feb 23, 2026 at 06:37:02PM +0100, Filip Hejsek wrote:
> > > Previously, the size was only read upon receiving the config interrupt.
> > > This interrupt is sent when the size changes. However, we also need to
> > > read the initial size.
> > > 
> > > Also make sure to only read the size from config if F_SIZE is enabled.
> > > 
> > > Fixes: 9778829cffd4 ("virtio: console: Store each console's size in the console structure")
> > > Signed-off-by: Filip Hejsek <filip.hejsek@gmail.com>
> > > ---
> > > This is a resend of [1], which hasn't received any response.
> > > 
> > > I found this bug while developing patches for QEMU that add virtio
> > > console resize support. If you want to test this, you can get my QEMU
> > > patches from [2]. You will need to disable multiport
> > > using `-device virtio-serial,max_ports=1`.
> > > 
> > > [1]: https://lore.kernel.org/all/20251224-virtio-console-fix-v1-1-69d0349692dc@gmail.com/
> > > [2]: https://lore.kernel.org/all/20250921-console-resize-v5-0-89e3c6727060@gmail.com/
> > > 
> > > I'll also repeat my questions from the previous submission here. These are
> > > things that confused me when I was trying to understand the surrounding code,
> > > but should in no way prevent merging this patch.
> > > 
> > >   - Why does use_multiport use __virtio_test_bit instead of
> > >     virtio_has_feature?
> > > 
> > >   - The VIRTIO_CONSOLE_RESIZE handler sets irq_requested to 1, which I
> > >     think makes no sense?
> > > ---
> > >  drivers/char/virtio_console.c | 52 ++++++++++++++++++++++++++-----------------
> > >  1 file changed, 31 insertions(+), 21 deletions(-)
> > > 
> > > diff --git a/drivers/char/virtio_console.c b/drivers/char/virtio_console.c
> > > index 088182e54d..c355f6d392 100644
> > > --- a/drivers/char/virtio_console.c
> > > +++ b/drivers/char/virtio_console.c
> > > @@ -1771,32 +1771,40 @@ static void config_intr(struct virtio_device *vdev)
> > >  		schedule_work(&portdev->config_work);
> > >  }
> > >  
> > > -static void config_work_handler(struct work_struct *work)
> > > +static void update_size_from_config(struct ports_device *portdev)
> > >  {
> > > -	struct ports_device *portdev;
> > > +	struct virtio_device *vdev;
> > > +	struct port *port;
> > > +	u16 rows, cols;
> > >  
> > > -	portdev = container_of(work, struct ports_device, config_work);
> > > -	if (!use_multiport(portdev)) {
> > > -		struct virtio_device *vdev;
> > > -		struct port *port;
> > > -		u16 rows, cols;
> > > +	vdev = portdev->vdev;
> > >  
> > > -		vdev = portdev->vdev;
> > > -		virtio_cread(vdev, struct virtio_console_config, cols, &cols);
> > > -		virtio_cread(vdev, struct virtio_console_config, rows, &rows);
> > > +	/*
> > > +	 * We'll use this way of resizing only for legacy support.
> > > +	 * For multiport devices, use control messages to indicate
> > > +	 * console size changes so that it can be done per-port.
> > > +	 *
> > > +	 * Don't test F_SIZE at all if we're rproc: not a valid feature.
> > > +	 */
> > > +	if (is_rproc_serial(vdev) ||
> > 
> > Wait a second. Why is there this rproc test here?
> > Was not in the original code and commit log says nothing about it.
> > 
> 
> Previously, this code was in config_work_handler(), which was never
> called for rproc_serial (it's scheduled from config_intr(), which is
> the config_changed handler only for virtio_console).
> 
> Now update_size_from_config() is called unconditionally from
> virtcons_probe(), so it will be called for rproc_serial too, which
> doesn't have the F_SIZE feature.

So why not test it? What does "not a valid feature" mean?
I dislike transport code leaking into devices.


> > 
> > > +	    use_multiport(portdev) ||
> > > +	    !virtio_has_feature(vdev, VIRTIO_CONSOLE_F_SIZE))
> > > +		return;
> > >  
> > > -		port = find_port_by_id(portdev, 0);
> > > -		set_console_size(port, rows, cols);
> > > +	virtio_cread(vdev, struct virtio_console_config, cols, &cols);
> > > +	virtio_cread(vdev, struct virtio_console_config, rows, &rows);
> > >  
> > > -		/*
> > > -		 * We'll use this way of resizing only for legacy
> > > -		 * support.  For newer userspace
> > > -		 * (VIRTIO_CONSOLE_F_MULTPORT+), use control messages
> > > -		 * to indicate console size changes so that it can be
> > > -		 * done per-port.
> > > -		 */
> > > -		resize_console(port);
> > > -	}
> > > +	port = find_port_by_id(portdev, 0);
> > > +	set_console_size(port, rows, cols);
> > > +	resize_console(port);
> > > +}
> > > +
> > > +static void config_work_handler(struct work_struct *work)
> > > +{
> > > +	struct ports_device *portdev;
> > > +
> > > +	portdev = container_of(work, struct ports_device, config_work);
> > > +	update_size_from_config(portdev);
> > >  }
> > >  
> > >  static int init_vqs(struct ports_device *portdev)
> > > @@ -2054,6 +2062,8 @@ static int virtcons_probe(struct virtio_device *vdev)
> > >  	__send_control_msg(portdev, VIRTIO_CONSOLE_BAD_ID,
> > >  			   VIRTIO_CONSOLE_DEVICE_READY, 1);
> > >  
> > > +	update_size_from_config(portdev);
> > > +
> > >  	return 0;
> > >  
> > >  free_chrdev:
> > > 
> > > ---
> > > base-commit: b927546677c876e26eba308550207c2ddf812a43
> > > change-id: 20251224-virtio-console-fix-3d46980ef569
> > > 
> > > Best regards,
> > > -- 
> > > Filip Hejsek <filip.hejsek@gmail.com>