Message-ID: <92132076726e8c61253f0c7ba64501aff1da5d76.camel@linux.ibm.com>
Date: Wed, 05 Mar 2025 13:33:59 +0100
From: Maximilian Immanuel Brandtner <maxbr@...ux.ibm.com>
To: Niklas Schnelle <schnelle@...ux.ibm.com>, Amit Shah <amit@...nel.org>,
        linux-kernel@...r.kernel.org, virtualization@...ts.linux.dev
Cc: arnd@...db.de, gregkh@...uxfoundation.org, brueckner@...ux.ibm.com,
        pasic@...ux.ibm.com
Subject: Re: [PATCH] virtio: console: Make resizing compliant with virtio
 spec

On Wed, 2025-03-05 at 13:15 +0100, Niklas Schnelle wrote:
> On Wed, 2025-03-05 at 13:13 +0100, Niklas Schnelle wrote:
> > 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.
> > 
> 
> Forgot to note, the idea that Alpine was patched came because the
> kernel used in TinyEMU has '-dirty' in the local version so we were
> wondering if it was patched for this. But seeing the
> config_work_handler() it's probably just using that.
> 
> Thanks,
> Niklas

In that case it might be better to change the spec to reflect the
kernel implementation of resize control messages.

Max

Powered by blists - more mailing lists