Re: [PATCH] virtio: console: Make resizing compliant with virtio spec

[Maximilian Immanuel Brandtner]

On Mon, 2025-03-03 at 12:54 +0100, Amit Shah wrote:
> On Tue, 2025-02-25 at 10:21 +0100, Maximilian Immanuel Brandtner
> wrote:
> > According to the virtio spec[0] the virtio console resize struct
> > defines
> > cols before rows. In the kernel implementation it is the other way
> > around
> > resulting in the two properties being switched.
> 
> Not true, see below.
> 
> > While QEMU doesn't currently support resizing consoles, TinyEMU
> 
> QEMU does support console resizing - just that it uses the classical
> way of doing it: via the config space, and not via a control message
> (yet).
> 
> https://web.git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/char/virtio_console.c#n1787
> 
> https://lists.nongnu.org/archive/html/qemu-devel/2010-05/msg00031.html
> 
> > diff --git a/drivers/char/virtio_console.c
> > b/drivers/char/virtio_console.c
> > index 24442485e73e..9668e89873cf 100644
> > --- a/drivers/char/virtio_console.c
> > +++ b/drivers/char/virtio_console.c
> > @@ -1579,8 +1579,8 @@ static void handle_control_message(struct
> > virtio_device *vdev,
> >  		break;
> >  	case VIRTIO_CONSOLE_RESIZE: {
> >  		struct {
> > -			__u16 rows;
> >  			__u16 cols;
> > +			__u16 rows;
> >  		} size;
> >  
> >  		if (!is_console_port(port))
> 
> This VIRTIO_CONSOLE_RESIZE message is a control message, as opposed
> to
> the config space row/col values that is documented in the spec.
> 
> Maybe more context will be helpful:
> 
> Initially, virtio_console was just a way to create one hvc console
> port
> over the virtio transport.  The size of that console port could be
> changed by changing the size parameters in the virtio device's
> configuration space.  Those are the values documented in the spec. 
> These are read via virtio_cread(), and do not have a struct
> representation.
> 
> When the MULTIPORT feature was added to the virtio_console.c driver,
> more than one console port could be associated with the single
> device.
> Eg. we could have hvc0, hvc1, hvc2 all as part of the same device. 
> With this, the single config space value for row/col could not be
> used
> for the "extra" hvc1/hvc2 devices -- so a new VIRTIO_CONSOLE_RESIZE
> control message was added that conveys each console's dimensions.
> 
> Your patch is trying to change the control message, and not the
> config
> space.
> 
> Now - the lack of the 'struct size' definition for the control
> message
> in the spec is unfortunate, but that can be easily added -- and I
> prefer we add it based on this Linux implementation (ie. first rows,
> then cols).

Under section 5.3.6.2 multiport device operation for
VIRTIO_CONSOLE_RESIZE the spec says the following

```
Sent by the device to indicate a console size change. value is unused.
The buffer is followed by the number of columns and rows:

struct virtio_console_resize { 
        le16 cols; 
        le16 rows; 
};
```

It would be extremely surprising to me if the section `multiport device
operation` does not document resize for multiport control messages, but
rather config messages, especially as VIRTIO_CONSOLE_RESIZE is
documented as a virtio_console_control event.

In fact as far as I can tell this is the only part of the spec that
documents resize. I would be legitimately interested in resizing
without multiport and I would genuinely like to find out about how it
could be used. In what section of the documentation could I find it?

> 
> But note that all this only affects devices that implement multiport
> support, and have multiple console ports on a single device.  I don't
> recall there are any implementations using such a configuration.
> 
> ... which all leads me to ask if you've actually seen a
> misconfiguration happen when trying to resize consoles which led to
> this patch.
> 
> 		Amit