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
