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

[Maximilian Immanuel Brandtner]

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.

> 
> 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