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

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

[Maximilian Immanuel Brandtner]

According to section 5.3.6.2 (Multiport Device Operation) of the virtio
spec(version 1.2) a control buffer with the event VIRTIO_CONSOLE_RESIZE
is followed by a virtio_console_resize struct containing cols then rows.
The kernel implements this the wrong way around (rows then cols) resulting
in the two values being swapped.

Signed-off-by: Maximilian Immanuel Brandtner <maxbr@linux.ibm.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 21de774996ad..38af3029da39 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 {
-			__virtio16 rows;
 			__virtio16 cols;
+			__virtio16 rows;
 		} size;
 
 		if (!is_console_port(port))
-- 
2.48.1

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

[Daniel Verkamp]

On Mon, Mar 24, 2025 at 7:43 AM Maximilian Immanuel Brandtner
<maxbr@linux.ibm.com> wrote:
>
> According to section 5.3.6.2 (Multiport Device Operation) of the virtio
> spec(version 1.2) a control buffer with the event VIRTIO_CONSOLE_RESIZE
> is followed by a virtio_console_resize struct containing cols then rows.
> The kernel implements this the wrong way around (rows then cols) resulting
> in the two values being swapped.
>
> Signed-off-by: Maximilian Immanuel Brandtner <maxbr@linux.ibm.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 21de774996ad..38af3029da39 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 {
> -                       __virtio16 rows;
>                         __virtio16 cols;
> +                       __virtio16 rows;
>                 } size;

The order of the fields after the patch matches the spec, so from that
perspective, looks fine:
Reviewed-by: Daniel Verkamp <dverkamp@chromium.org>

Since the driver code has been using the wrong order since support for
this message was added in 2010, but there is no support for sending
this message in the current qemu device implementation, I wondered
what device code was used to test this when it was originally added. I
dug up what I assume is the corresponding qemu device change from the
same era, which sends the VIRTIO_CONSOLE_RESIZE message using the
rows, cols order that matches the kernel driver (and differs from the
spec):

https://lore.kernel.org/qemu-devel/1273092505-22783-1-git-send-email-amit.shah@redhat.com/
("[Qemu-devel] [PATCH] virtio-serial: Send per-console port resize
notifications to guest", May 6, 2010)

However, I don't believe that patch ever made it into an actual qemu
release, so it's probably not a compatibility concern. (If there are
any other device implementations that use the kernel driver order
rather than the spec order, then maybe this would need more
consideration, but I don't personally know of any.)

Thanks,
-- Daniel

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

[Michael S. Tsirkin]

On Mon, Mar 24, 2025 at 12:53:29PM -0700, Daniel Verkamp wrote:
> On Mon, Mar 24, 2025 at 7:43 AM Maximilian Immanuel Brandtner
> <maxbr@linux.ibm.com> wrote:
> >
> > According to section 5.3.6.2 (Multiport Device Operation) of the virtio
> > spec(version 1.2) a control buffer with the event VIRTIO_CONSOLE_RESIZE
> > is followed by a virtio_console_resize struct containing cols then rows.
> > The kernel implements this the wrong way around (rows then cols) resulting
> > in the two values being swapped.
> >
> > Signed-off-by: Maximilian Immanuel Brandtner <maxbr@linux.ibm.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 21de774996ad..38af3029da39 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 {
> > -                       __virtio16 rows;
> >                         __virtio16 cols;
> > +                       __virtio16 rows;
> >                 } size;
> 
> The order of the fields after the patch matches the spec, so from that
> perspective, looks fine:
> Reviewed-by: Daniel Verkamp <dverkamp@chromium.org>
> 
> Since the driver code has been using the wrong order since support for
> this message was added in 2010, but there is no support for sending
> this message in the current qemu device implementation, I wondered
> what device code was used to test this when it was originally added. I
> dug up what I assume is the corresponding qemu device change from the
> same era, which sends the VIRTIO_CONSOLE_RESIZE message using the
> rows, cols order that matches the kernel driver (and differs from the
> spec):
> 
> https://lore.kernel.org/qemu-devel/1273092505-22783-1-git-send-email-amit.shah@redhat.com/
> ("[Qemu-devel] [PATCH] virtio-serial: Send per-console port resize
> notifications to guest", May 6, 2010)
> 
> However, I don't believe that patch ever made it into an actual qemu
> release, so it's probably not a compatibility concern. (If there are
> any other device implementations that use the kernel driver order
> rather than the spec order, then maybe this would need more
> consideration, but I don't personally know of any.)
> 
> Thanks,
> -- Daniel

I agree. Cc author of the qemu patch for confirmation.

-- 
MST

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

[Michael S. Tsirkin]

Filip Hejsek pointed out the following:

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 the qemu-devel 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.

While commit 5326ab737a47278dbd16ed3ee7380b26c7056ddd in Linux made it
match the spec, no one seems to have implemented it yet host side. It
seems better to just drop the change (it was only in 2 releases so far),
going back to the status quo.

Change the spec to match.

Reported-by: Filip Hejsek <filip.hejsek@gmail.com>
Cc: "Maximilian Immanuel Brandtner" <maxbr@linux.ibm.com>
Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
---
 device-types/console/description.tex | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/device-types/console/description.tex b/device-types/console/description.tex
index 40a2ba4..c18dc08 100644
--- a/device-types/console/description.tex
+++ b/device-types/console/description.tex
@@ -59,8 +59,8 @@ \subsection{Device configuration layout}\label{sec:Device Types / Console Device
 
 \begin{lstlisting}
 struct virtio_console_config {
-        le16 cols;
         le16 rows;
+        le16 cols;
         le32 max_nr_ports;
         le32 emerg_wr;
 };
-- 
MST

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

[Matias Ezequiel Vara Larsen]

On Thu, Sep 18, 2025 at 12:18:47PM -0400, Michael S. Tsirkin wrote:
> Filip Hejsek pointed out the following:
> 
> 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 the qemu-devel 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.
> 
> While commit 5326ab737a47278dbd16ed3ee7380b26c7056ddd in Linux made it
> match the spec, no one seems to have implemented it yet host side. It
> seems better to just drop the change (it was only in 2 releases so far),
> going back to the status quo.
> 
> Change the spec to match.
> 
> Reported-by: Filip Hejsek <filip.hejsek@gmail.com>
> Cc: "Maximilian Immanuel Brandtner" <maxbr@linux.ibm.com>
> Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> ---
>  device-types/console/description.tex | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 

Reviewed-by: Matias Ezequiel Vara Larsen <mvaralar@redhat.com>

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

[Michael S. Tsirkin]

On Mon, Oct 13, 2025 at 02:00:22PM +0900, Dominique Martinet wrote:
> Michael S. Tsirkin wrote on Thu, Sep 18, 2025 at 12:18:47PM -0400:
> > While commit 5326ab737a47278dbd16ed3ee7380b26c7056ddd in Linux made it
> > match the spec, no one seems to have implemented it yet host side. It
> > seems better to just drop the change (it was only in 2 releases so far),
> > going back to the status quo.
> 
> FWIW, there are other users of the spec e.g. bhyve, which implemented it
> "right" (cols, rows as per the spec -- since 2016):
> https://github.com/freebsd/freebsd-src/blob/main/usr.sbin/bhyve/pci_virtio_console.c#L148
> 
> or crossvm (also cols,rows)
> https://github.com/google/crosvm/blob/main/devices/src/virtio/device_constants.rs#L293
> 


Indeed, thanks for bringing this to our attention.


> I didn't look to see if there are others (probably some of the other
> rust hypervisor crowd?); I'm not involved in any of them but given there
> are other implementations I'd personally think it makes more sense to
> continue the work of "righting" linux (that is backport the order
> fix)...?

Yes, this is what we normally do. The reason I wanted to do it
differently here is because I did not realize we have implementations.

> I don't have any beef either way as long as the qemu patches aren't
> forgotten (thank you again Filip!); hopefully this can help reach any
> kind of decision.
> 
> If this was discussed somewhere else I'd appreciate being pointed to
> it :) I "only" found the recent qemu patches, an attempt to revert the
> fix in linux that was held back, and the stble backport thread where the
> issue was raised but not really discussed either way.
> 
> Thanks,
> -- 
> Dominique Martinet | Asmadeus


OK I guess ... let's get the ball rolling with backporting the Linux
fix?

-- 
MST