On Wed, Mar 19, 2025 at 02:00:44PM +0100, Maximilian Immanuel Brandtner wrote: > On Tue, 2025-03-18 at 15:25 +0100, Amit Shah wrote: > > On Tue, 2025-03-18 at 11:07 +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 > > > > > > > > > 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). > > > > > > Under section 5.3.6.2 multiport device operation for > > > VIRTIO_CONSOLE_RESIZE the spec says the following > > > > > > ``` > > > Sent by the device to indicate a console size change. value is > > > unused. > > > The buffer is followed by the number of columns and rows: > > > > > > struct virtio_console_resize { > > > le16 cols; > > > le16 rows; > > > }; > > > ``` > > > > Indeed. > > > > > > > It would be extremely surprising to me if the section `multiport > > > device > > > operation` does not document resize for multiport control messages, > > > but > > > rather config messages, especially as VIRTIO_CONSOLE_RESIZE is > > > documented as a virtio_console_control event. > > > > You're right. > > > > I was mistaken in my earlier reply - I had missed this > > virtio_console_resize definition in the spec. So indeed there's a > > discrepancy in Linux kernel and the spec's ordering for the control > > message. > > > > OK, that needs fixing someplace. Perhaps in the kernel (like your > > orig. patch), but with an accurate commit message. > > So should I send a patch v2 or should the spec be changed instead? Or > would you like to first await the opinion of the spec maintainers? > > The mail I initially sent to the virtio mailing list seems to have > fallen on deaf ears. I now added Michael Tsirkin to this thread so that > things might get going.
If we can fix the driver to fit the spec, that's best. We generally try to avoid changing the spec just because drivers are buggy. > > > > Like I said, I don't think anyone is using this control message to > > change console sizes. I don't even think anyone's using multiple > > console ports on the same device. > > > > > In fact as far as I can tell this is the only part of the spec that > > > documents resize. I would be legitimately interested in resizing > > > without multiport and I would genuinely like to find out about how > > > it > > > could be used. In what section of the documentation could I find > > > it? > > > > See section 5.3.4 that describes `struct virtio_console_config` and > > this note: > > > > ``` > > If the VIRTIO_CONSOLE_F_SIZE feature is negotiated, the driver > > can > > read the console dimensions from cols and rows. > > ``` > > > > Amit
