[PATCH] Revert "virtio_console: fix order of fields cols and rows"

[PATCH] Revert "virtio_console: fix order of fields cols and rows"

[Michael S. Tsirkin]

This reverts commit 5326ab737a47278dbd16ed3ee7380b26c7056ddd.

The problem is that for a long time, the
Linux kernel used a different field order from what was specified in the
virtio spec. The kernel implementation was apparently merged around 2010,
while the virtio spec came in 2014, so when a previous version of this
patch series was being discussed here on this mailing list in 2020, it
was decided that QEMU should match the Linux implementation, and ideally,
the virtio spec should be changed.

There are about 15 years' worth
of kernel versions with the swapped field order, including the kernel
currently shipped in Debian stable. The effects of the swapped dimensions
can sometimes be quite annoying - e.g. if you have a terminal with
24 rows, this will be interpreted as 24 columns, and your shell may limit
line editing to this small space, most of which will be taken by your
prompt.

NB: the command structures really should move to the UAPI header so it
is easier to notice when a change is breaking the guest/host ABI.

Reported-by: Filip Hejsek <filip.hejsek@gmail.com>
Fixes: 5326ab737a47 ("virtio_console: fix order of fields cols and rows")
Cc: "Maximilian Immanuel Brandtner" <maxbr@linux.ibm.com>
Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
---
 drivers/char/virtio_console.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/char/virtio_console.c b/drivers/char/virtio_console.c
index 088182e54deb..216c5115637d 100644
--- a/drivers/char/virtio_console.c
+++ b/drivers/char/virtio_console.c
@@ -1576,8 +1576,8 @@ static void handle_control_message(struct virtio_device *vdev,
 		break;
 	case VIRTIO_CONSOLE_RESIZE: {
 		struct {
-			__virtio16 cols;
 			__virtio16 rows;
+			__virtio16 cols;
 		} size;
 
 		if (!is_console_port(port))
-- 
MST

[PATCH] Revert "virtio_console: fix order of fields cols and rows"

[Michael S. Tsirkin]

This reverts commit 5326ab737a47278dbd16ed3ee7380b26c7056ddd.

The problem is that for a long time, the
Linux kernel used a different field order from what was specified in the
virtio spec. The kernel implementation was apparently merged around 2010,
while the virtio spec came in 2014, so when a previous version of this
patch series was being discussed here on this mailing list in 2020, it
was decided that QEMU should match the Linux implementation, and ideally,
the virtio spec should be changed.

There are about 15 years' worth
of kernel versions with the swapped field order, including the kernel
currently shipped in Debian stable. The effects of the swapped dimensions
can sometimes be quite annoying - e.g. if you have a terminal with
24 rows, this will be interpreted as 24 columns, and your shell may limit
line editing to this small space, most of which will be taken by your
prompt.

It seems better to just drop the change (it was only in 2 releases so
far), going back to the status quo.

Reported-by: Filip Hejsek <filip.hejsek@gmail.com>
Fixes: 5326ab737a47 ("virtio_console: fix order of fields cols and rows")
Cc: "Maximilian Immanuel Brandtner" <maxbr@linux.ibm.com>
Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
---
 drivers/char/virtio_console.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/char/virtio_console.c b/drivers/char/virtio_console.c
index 088182e54deb..216c5115637d 100644
--- a/drivers/char/virtio_console.c
+++ b/drivers/char/virtio_console.c
@@ -1576,8 +1576,8 @@ static void handle_control_message(struct virtio_device *vdev,
 		break;
 	case VIRTIO_CONSOLE_RESIZE: {
 		struct {
-			__virtio16 cols;
 			__virtio16 rows;
+			__virtio16 cols;
 		} size;
 
 		if (!is_console_port(port))
-- 
MST

Re: [PATCH] Revert "virtio_console: fix order of fields cols and rows"

[Filip Hejsek]

On Fri, 2025-09-12 at 04:56 -0400, Michael S. Tsirkin wrote:
> when a previous version of this
> patch series was being discussed here on this mailing list in 2020, it
> was decided that QEMU should match the Linux implementation, and ideally,
> the virtio spec should be changed.

This wording has been copied verbatim from a cover letter I posted to
qemu-devel and doesn't make much sense for a Linux commit message.

Re: [PATCH] Revert "virtio_console: fix order of fields cols and rows"

[Michael S. Tsirkin]

On Fri, Sep 12, 2025 at 04:06:37PM +0200, Filip Hejsek wrote:
> On Fri, 2025-09-12 at 04:56 -0400, Michael S. Tsirkin wrote:
> > when a previous version of this
> > patch series was being discussed here on this mailing list in 2020, it
> > was decided that QEMU should match the Linux implementation, and ideally,
> > the virtio spec should be changed.
> 
> This wording has been copied verbatim from a cover letter I posted to
> qemu-devel and doesn't make much sense for a Linux commit message.

Will change to "to the qemu-devel mailing list". Thanks for pointing
this out!

Re: [PATCH] Revert "virtio_console: fix order of fields cols and rows"

[Maximilian Immanuel Brandtner]

On Fri, 2025-09-12 at 04:40 -0400, Michael S. Tsirkin wrote:
> This reverts commit 5326ab737a47278dbd16ed3ee7380b26c7056ddd.
> 
> The problem is that for a long time, the
> Linux kernel used a different field order from what was specified in
> the
> virtio spec. The kernel implementation was apparently merged around
> 2010,
> while the virtio spec came in 2014, so when a previous version of
> this
> patch series was being discussed here on this mailing list in 2020,
> it
> was decided that QEMU should match the Linux implementation, and
> ideally,
> the virtio spec should be changed.
> 
> There are about 15 years' worth
> of kernel versions with the swapped field order, including the kernel
> currently shipped in Debian stable. The effects of the swapped
> dimensions
> can sometimes be quite annoying - e.g. if you have a terminal with
> 24 rows, this will be interpreted as 24 columns, and your shell may
> limit
> line editing to this small space, most of which will be taken by your
> prompt.
> 
> NB: the command structures really should move to the UAPI header so
> it
> is easier to notice when a change is breaking the guest/host ABI.

As I already mentioned in the QEMU discussion, I proposed the fix,
because I was working on a similar implementation to bring resizing to
QEMU. Unfortunately, the patch-set was stuck in limbo for a while and
now that someone else has picked up the slack, I've descided that it's
better to contribute to the patch-set that is upstream instead of
sending a competing patch-set that does the same thing. Accordingly, I
no longer have any skin in the game of implementing resizing for virtio
console in QEMU as the other patch-set takes care of that task.

On a related note, during the initial discussion of this changing the
virtio spec was proposed as well (as can be read from the commit mgs),
however at the time on the viritio mailing list people were resistent
to the idea of changing the virtio spec to conform to the kernel
implementation.
I don't really care if this discrepancy is fixed one way or the other,
but it should most definitely be fixed.

Kind regards,
Max Brandtner

> 
> Reported-by: Filip Hejsek <filip.hejsek@gmail.com>
> Fixes: 5326ab737a47 ("virtio_console: fix order of fields cols and
> rows")
> Cc: "Maximilian Immanuel Brandtner" <maxbr@linux.ibm.com>
> Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> ---
>  drivers/char/virtio_console.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/char/virtio_console.c
> b/drivers/char/virtio_console.c
> index 088182e54deb..216c5115637d 100644
> --- a/drivers/char/virtio_console.c
> +++ b/drivers/char/virtio_console.c
> @@ -1576,8 +1576,8 @@ static void handle_control_message(struct
> virtio_device *vdev,
>  		break;
>  	case VIRTIO_CONSOLE_RESIZE: {
>  		struct {
> -			__virtio16 cols;
>  			__virtio16 rows;
> +			__virtio16 cols;
>  		} size;
>  
>  		if (!is_console_port(port))

Re: [PATCH] Revert "virtio_console: fix order of fields cols and rows"

[Filip Hejsek]

Hi,

On Mon, 2025-09-15 at 18:44 +0200, Maximilian Immanuel Brandtner wrote:
> [...]
> As I already mentioned in the QEMU discussion, I proposed the fix,
> because I was working on a similar implementation to bring resizing to
> QEMU.

I couldn't find any mention of your implementation on the QEMU ML.

> Unfortunately, the patch-set was stuck in limbo for a while and
> now that someone else has picked up the slack, I've descided that it's
> better to contribute to the patch-set that is upstream instead of
> sending a competing patch-set that does the same thing.

Would it be OK for me to take a look at your WIP patches? I would like
to see if you did anything differently.

Also, you mentioned before that you were also working on patches for
Libvirt. These will still be useful, because I won't be implementing
that part.

> [...]
> I don't really care if this discrepancy is fixed one way or the other,
> but it should most definitely be fixed.

I'm of the same opinion, but if it is fixed on the kernel side, then
(assuming no device implementation with the wrong order exists) I think
maybe the fix should be backported to all widely used kernels. It seems
that the patch hasn't been backported to the longterm kernels [1],
which I think Debian kernels are based on.

[1]: https://git.kernel.org/pub/scm/linux/kernel/git/stable/linux.git/log/drivers/char/virtio_console.c?h=v6.12.47

> Kind regards,
> Max Brandtner

Regards,
Filip Hejsek

Re: [PATCH] Revert "virtio_console: fix order of fields cols and rows"

[Maximilian Immanuel Brandtner]

On Mon, 2025-09-15 at 23:37 +0200, Filip Hejsek wrote:
> Hi,
> 
> On Mon, 2025-09-15 at 18:44 +0200, Maximilian Immanuel Brandtner
> wrote:
> > [...]
> > As I already mentioned in the QEMU discussion, I proposed the fix,
> > because I was working on a similar implementation to bring resizing
> > to
> > QEMU.
> 
> I couldn't find any mention of your implementation on the QEMU ML.
> 
> > Unfortunately, the patch-set was stuck in limbo for a while and
> > now that someone else has picked up the slack, I've descided that
> > it's
> > better to contribute to the patch-set that is upstream instead of
> > sending a competing patch-set that does the same thing.
> 
> Would it be OK for me to take a look at your WIP patches? I would
> like
> to see if you did anything differently.
> 

For the stuff I can get clearance for anyways. The tl:dr of what's
different about my patch-set is as follows:
- introduce a new IOResizeHandler instead of using a chr backend event
- dynamically add a QOM-property to every character device that
supports resizing (instead of the qmp-command that you have
implemented)
- guest chardev implementations

I also used a g_unix_signal_source instead of a signal notifier and it
worked during testing, but I prefer your solution

> Also, you mentioned before that you were also working on patches for
> Libvirt. These will still be useful, because I won't be implementing
> that part.
> 

I haven't properly implemented it for libvirt either. The libvirt patch
-set would have to be somewhat sizeable as well as virStream currently
doesn't implement a handle to perform hypervisor specific calls (which
a resize QMP call would be). Accordingly, the virStream implementation
would have to be changed on a deeper level which is why I just didn't
come around to it (I would also need to familiarize myself first with
libvirt more which I haven't done yet).

> > [...]
> > I don't really care if this discrepancy is fixed one way or the
> > other,
> > but it should most definitely be fixed.
> 
> I'm of the same opinion, but if it is fixed on the kernel side, then
> (assuming no device implementation with the wrong order exists) I
> think
> maybe the fix should be backported to all widely used kernels. It
> seems
> that the patch hasn't been backported to the longterm kernels [1],
> which I think Debian kernels are based on.
> 
> [1]:
> https://git.kernel.org/pub/scm/linux/kernel/git/stable/linux.git/log/drivers/char/virtio_console.c?h=v6.12.47
> 

Then I guess the patch-set should be backported

> > Kind regards,
> > Max Brandtner
> 
> Regards,
> Filip Hejsek