Message-ID: <67efe42ea8c10120f13f14838f7a3d883184ecf5.camel@linux.ibm.com>
Date: Wed, 05 Mar 2025 10:53:58 +0100
From: Maximilian Immanuel Brandtner <maxbr@...ux.ibm.com>
To: 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,
        schnelle@...ux.ibm.com, pasic@...ux.ibm.com
Subject: Re: [PATCH] virtio: console: Make resizing compliant with virtio
 spec

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

[0]
https://docs.oasis-open.org/virtio/virtio/v1.2/csd01/virtio-v1.2-csd01.html#x1-2980006

Powered by blists - more mailing lists