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

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

On Wed, Mar 19, 2025 at 02:00:44PM +0100, Maximilian Immanuel Brandtner wrote:
> On Tue, 2025-03-18 at 15:25 +0100, Amit Shah wrote:
> > On Tue, 2025-03-18 at 11:07 +0100, Maximilian Immanuel Brandtner
> > wrote:
> > > 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; 
> > > };
> > > ```
> > 
> > Indeed.
> > 
> > 
> > > 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.
> > 
> > You're right.
> > 
> > I was mistaken in my earlier reply - I had missed this
> > virtio_console_resize definition in the spec.  So indeed there's a
> > discrepancy in Linux kernel and the spec's ordering for the control
> > message.
> > 
> > OK, that needs fixing someplace.  Perhaps in the kernel (like your
> > orig. patch), but with an accurate commit message.
> 
> So should I send a patch v2 or should the spec be changed instead? Or
> would you like to first await the opinion of the spec maintainers?
> 
> The mail I initially sent to the virtio mailing list seems to have
> fallen on deaf ears. I now added Michael Tsirkin to this thread so that
> things might get going.


If we can fix the driver to fit the spec, that's best.
We generally try to avoid changing the spec just because
drivers are buggy.

> > 
> > Like I said, I don't think anyone is using this control message to
> > change console sizes.  I don't even think anyone's using multiple
> > console ports on the same device.
> > 
> > > 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?
> > 
> > See section 5.3.4 that describes `struct virtio_console_config` and
> > this note:
> > 
> > ```
> >     If the VIRTIO_CONSOLE_F_SIZE feature is negotiated, the driver
> > can
> > read the console dimensions from cols and rows. 
> > ```
> > 
> > 		Amit