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

[Amit Shah]

On Wed, 2025-03-19 at 09:54 +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.
> > 
> > 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
> 
> The way I understand VIRTIO_CONSOLE_F_SIZE, it has to be negotiated
> for
> any resize events to be sent (including resize control messages) (at
> least as of right now it is necessary to enable this feature to sent
> resize control messages).

That's right, but virtio feature negotiation just means that the host
and guest figure out what the other endpoint supports.  Console
resizing wasn't part of the orig virtio console implementation, and was
added later.

For the resizing case, it's one-way communication from the host to the
guest: the host sets row/col values and notifies the guest.  If the
guest is old and does not have the F_SIZE feature, it will just ignore
it.  If the host doesn't have that feature, well, it can't send the
resize messages.

The commit to resize the console was added way back in 2008:

https://web.git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/drivers/char/virtio_console.c?id=c29834584ea4eafccf2f62a0b8a32e64f792044c

This line:

+	if (virtio_has_feature(dev, VIRTIO_CONSOLE_F_SIZE)) {

is the negotiation part -- if the host device has that feature, read
the values from the config space and apply them.

		Amit