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
