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

[Niklas Schnelle <schnelle@linux.ibm.com>]

On Wed, 2025-03-05 at 10:53 +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
> 
> I didn't know about this patch-set, however as of right now QEMU does
> not set VIRTIO_CONSOLE_F_SIZE, never uses VIRTIO_CONSOLE_RESIZE, and
> resizing is never mentioned in hw/char/virtio-console.c or
> hw/char/virtio-serial-bus.c. Suffice to say I don't see any indicating
> of resize currently being used under QEMU. Perhaps QEMU supported
> resizing at one point, but not anymore. If you disagree please send me
> where the resizing logic can currently be found in the QEMU source
> code. I at least was unable to find it.
> 
> > 
> > > 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).
> > 
> > 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
> 
> I'm working on implementing console resizing for virtio in QEMU and
> Libvirt. As SIGWINCH is raised on the virsh frontend the new console
> size needs to be transfered to QEMU (in my RFC patch via QOM, which
> then causes QEMU to trigger a virtio control msg in the chr_resize
> function of the virtio-console chardev). (The patch-set should make its
> way unto the QEMU mailing-list soon). The way I implemented it QEMU
> sends a resize control message where the control message has the
> following format:
> 
> ```
> struct {
>     le32 id;    // port->id
>     le16 event; // VIRTIO_CONSOLE_RESIZE
>     le16 value; // 0
>     le16 cols;  // ws.ws_col
>     le16 rows;  // ws.ws_row
> }
> ```
> 
> This strongly seems to me to be in accordance with the spec[0]. It
> resulted in the rows and cols being switched after a resize event. I
> was able to track the issue down to this part of the kernel. Applying
> the patch I sent upstream, fixed the issue.
> As of right now I only implemented resize for multiport (because in the
> virtio spec I was only able to find references to resizing as a control
> message which requires multiport. In your email you claimed that config
> space resizing exists as well. I was only able to find references to
> resizing as a control message in the spec. I would love to see what
> part of the spec you are refering to specifically, as it would allow me
> to implement resizing without multiport as well). 
> It seems to me that either the spec or the kernel implementation has to
> change. If you prefer changing the spec that would be fine by me as
> well, however there seems to be no implementation that uses the linux
> ordering and Alpine seems to patch their kernel to use the spec
> ordering instead (as described in the initial email)(this was really
> Niklas Schnelle's finding so for further questions I would refer to
> him).

I don't think this was patched in the (official) alpine kernel. What
happened is that I tested TinyEMU[0] with the kernel + initrd from the
JSLinux site and that has working console resizing. In the TinyEMU code
this is implemented in TinyEMU/virtio.c:virtio_console_resize_event():

void virtio_console_resize_event(VIRTIODevice *s, int width, int height)
{
    /* indicate the console size */
    put_le16(s->config_space + 0, width);
    put_le16(s->config_space + 2, height);

    virtio_config_change_notify(s);
}

On second look this indeed seems to use the config space. It writes
first the width then height which matches config_work_handler(). But
like Maximilian I could only find the VIRTIO_CONSOLE_RESIZE message
mechanism in the spec, also with width (cols) then height (rows) but
not matching the kernel struct changed by this patch.

Thanks,
Niklas

[0] https://bellard.org/tinyemu/