[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