[PATCH 1/1] virtio_console: fix missing byte order handling for cols and rows

[PATCH 1/1] virtio_console: fix missing byte order handling for cols and rows

[Halil Pasic]

As per virtio spec the fields cols and rows are specified as little
endian. Although there is no legacy interface requirement that would
state that cols and rows need to be handled as native endian when legacy
interface is used, unlike for the fields of the adjacent struct
virtio_console_control, I decided to err on the side of caution based
on some non-conclusive virtio spec repo archaeology and opt for using
virtio16_to_cpu() much like for virtio_console_control.event. Strictly
by the letter of the spec virtio_le_to_cpu() would have been sufficient.
But when the legacy interface is not used, it boils down to the same.

And when using the legacy interface, the device formatting these as
little endian when the guest is big endian would surprise me more than
it using guest native byte order (which would make it compatible with
the current implementation). Nevertheless somebody trying to implement
the spec following it to the letter could end up forcing little endian
byte order when the legacy interface is in use. So IMHO this ultimately
needs a judgement call by the maintainers.

Fixes: 8345adbf96fc1 ("virtio: console: Accept console size along with resize control message")
Signed-off-by: Halil Pasic <pasic@linux.ibm.com>
Cc: stable@vger.kernel.org # v2.6.35+
---

@Michael: I think it would be nice to add a clarification on the byte
order to be used for cols and rows when the legacy interface is used to
the spec, regardless of what we decide the right byte order is. If
it is native endian that shall be stated much like it is stated for
virtio_console_control. If it is little endian, I would like to add
a sentence that states that unlike for the fields of virtio_console_control
the byte order of the fields of struct virtio_console_resize is little
endian also when the legacy interface is used.

@Maximilian: Would you mind giving this a spin with your implementation
on the device side of things in QEMU?
---
 drivers/char/virtio_console.c | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

diff --git a/drivers/char/virtio_console.c b/drivers/char/virtio_console.c
index 18f92dd44d45..fc698e2b1da1 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;
+			__virtio16 rows;
+			__virtio16 cols;
 		} size;
 
 		if (!is_console_port(port))
@@ -1588,7 +1588,8 @@ static void handle_control_message(struct virtio_device *vdev,
 
 		memcpy(&size, buf->buf + buf->offset + sizeof(*cpkt),
 		       sizeof(size));
-		set_console_size(port, size.rows, size.cols);
+		set_console_size(port, virtio16_to_cpu(vdev, size.rows),
+				 virtio16_to_cpu(vdev, size.cols));
 
 		port->cons.hvc->irq_requested = 1;
 		resize_console(port);

base-commit: b3ee1e4609512dfff642a96b34d7e5dfcdc92d05
-- 
2.45.2

Re: [PATCH 1/1] virtio_console: fix missing byte order handling for cols and rows

[Maximilian Immanuel Brandtner]

On Sat, 2025-03-22 at 01:29 +0100, Halil Pasic wrote:
> As per virtio spec the fields cols and rows are specified as little
> endian. Although there is no legacy interface requirement that would
> state that cols and rows need to be handled as native endian when
> legacy
> interface is used, unlike for the fields of the adjacent struct
> virtio_console_control, I decided to err on the side of caution based
> on some non-conclusive virtio spec repo archaeology and opt for using
> virtio16_to_cpu() much like for virtio_console_control.event.
> Strictly
> by the letter of the spec virtio_le_to_cpu() would have been
> sufficient.
> But when the legacy interface is not used, it boils down to the same.
> 
> And when using the legacy interface, the device formatting these as
> little endian when the guest is big endian would surprise me more
> than
> it using guest native byte order (which would make it compatible with
> the current implementation). Nevertheless somebody trying to
> implement
> the spec following it to the letter could end up forcing little
> endian
> byte order when the legacy interface is in use. So IMHO this
> ultimately
> needs a judgement call by the maintainers.
> 
> Fixes: 8345adbf96fc1 ("virtio: console: Accept console size along
> with resize control message")
> Signed-off-by: Halil Pasic <pasic@linux.ibm.com>
> Cc: stable@vger.kernel.org # v2.6.35+
> ---
> 
> @Michael: I think it would be nice to add a clarification on the byte
> order to be used for cols and rows when the legacy interface is used
> to
> the spec, regardless of what we decide the right byte order is. If
> it is native endian that shall be stated much like it is stated for
> virtio_console_control. If it is little endian, I would like to add
> a sentence that states that unlike for the fields of
> virtio_console_control
> the byte order of the fields of struct virtio_console_resize is
> little
> endian also when the legacy interface is used.
> 
> @Maximilian: Would you mind giving this a spin with your
> implementation
> on the device side of things in QEMU?
> ---
>  drivers/char/virtio_console.c | 7 ++++---
>  1 file changed, 4 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/char/virtio_console.c
> b/drivers/char/virtio_console.c
> index 18f92dd44d45..fc698e2b1da1 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;
> +			__virtio16 rows;
> +			__virtio16 cols;
>  		} size;
>  
>  		if (!is_console_port(port))
> @@ -1588,7 +1588,8 @@ static void handle_control_message(struct
> virtio_device *vdev,
>  
>  		memcpy(&size, buf->buf + buf->offset +
> sizeof(*cpkt),
>  		       sizeof(size));
> -		set_console_size(port, size.rows, size.cols);
> +		set_console_size(port, virtio16_to_cpu(vdev,
> size.rows),
> +				 virtio16_to_cpu(vdev, size.cols));
>  
>  		port->cons.hvc->irq_requested = 1;
>  		resize_console(port);
> 
> base-commit: b3ee1e4609512dfff642a96b34d7e5dfcdc92d05

It took me a while to recompile the kernel, but now that it has
compiled it works! Unfortunately, images don't lend themselves well to
mailing lists, but here is tmux running at 18x55(you'll just have to
trust me that it's over a virtio serial console)

                           │top - 12:54:04 up 4 min,  1
~                          │Tasks: 222 total,   1 runni
~                          │%Cpu(s):  0.0 us,  0.0 sy, 
~                          │MiB Mem :  15987.2 total,  
~                          │MiB Swap:   8192.0 total,  
~                          │
~                          │    PID USER      PR  NI 
~                          │      1 root      20   0 
~                          │      2 root      20   0 
~                          │      3 root      20   0 
~                          │      4 root       0 -20 
~                          │      5 root       0 -20 
~                          │      6 root       0 -20 
~                          │      7 root       0 -20 
~                          │      8 root       0 -20 
[No Name]     0,0-1     All│      9 root      20   0 
                           │     10 root       0 -20 
[0] 0:zsh*                     "fedora" 12:53 24-Mar-25

Cheers,
Max

Re: [PATCH 1/1] virtio_console: fix missing byte order handling for cols and rows

[Halil Pasic]

On Sat, 22 Mar 2025 01:29:54 +0100
Halil Pasic <pasic@linux.ibm.com> wrote:

> As per virtio spec the fields cols and rows are specified as little
> endian. 
[..]

@Amit: Any feedback?

> 
> Fixes: 8345adbf96fc1 ("virtio: console: Accept console size along with resize control message")
> Signed-off-by: Halil Pasic <pasic@linux.ibm.com>
> Cc: stable@vger.kernel.org # v2.6.35+
> ---
> 
> @Michael: I think it would be nice to add a clarification on the byte
> order to be used for cols and rows when the legacy interface is used to
> the spec, regardless of what we decide the right byte order is. If
> it is native endian that shall be stated much like it is stated for
> virtio_console_control. If it is little endian, I would like to add
> a sentence that states that unlike for the fields of virtio_console_control
> the byte order of the fields of struct virtio_console_resize is little
> endian also when the legacy interface is used.

@MST: any opinion on that?

[..]

Re: [PATCH 1/1] virtio_console: fix missing byte order handling for cols and rows

[Michael S. Tsirkin]

On Wed, Apr 02, 2025 at 05:26:59PM +0200, Halil Pasic wrote:
> On Sat, 22 Mar 2025 01:29:54 +0100
> Halil Pasic <pasic@linux.ibm.com> wrote:
> 
> > As per virtio spec the fields cols and rows are specified as little
> > endian. 
> [..]
> 
> @Amit: Any feedback?
> 
> > 
> > Fixes: 8345adbf96fc1 ("virtio: console: Accept console size along with resize control message")
> > Signed-off-by: Halil Pasic <pasic@linux.ibm.com>
> > Cc: stable@vger.kernel.org # v2.6.35+
> > ---
> > 
> > @Michael: I think it would be nice to add a clarification on the byte
> > order to be used for cols and rows when the legacy interface is used to
> > the spec, regardless of what we decide the right byte order is. If
> > it is native endian that shall be stated much like it is stated for
> > virtio_console_control. If it is little endian, I would like to add
> > a sentence that states that unlike for the fields of virtio_console_control
> > the byte order of the fields of struct virtio_console_resize is little
> > endian also when the legacy interface is used.
> 
> @MST: any opinion on that?
> 
> [..]


native endian for legacy. Yes extending the spec to say this is right.

-- 
MST

Re: [PATCH 1/1] virtio_console: fix missing byte order handling for cols and rows

[Amit Shah]

On Sat, 2025-03-22 at 01:29 +0100, Halil Pasic wrote:
> As per virtio spec the fields cols and rows are specified as little
> endian. Although there is no legacy interface requirement that would
> state that cols and rows need to be handled as native endian when
> legacy
> interface is used, unlike for the fields of the adjacent struct
> virtio_console_control, I decided to err on the side of caution based
> on some non-conclusive virtio spec repo archaeology and opt for using
> virtio16_to_cpu() much like for virtio_console_control.event.
> Strictly
> by the letter of the spec virtio_le_to_cpu() would have been
> sufficient.
> But when the legacy interface is not used, it boils down to the same.
> 
> And when using the legacy interface, the device formatting these as
> little endian when the guest is big endian would surprise me more
> than
> it using guest native byte order (which would make it compatible with
> the current implementation). Nevertheless somebody trying to
> implement
> the spec following it to the letter could end up forcing little
> endian
> byte order when the legacy interface is in use. So IMHO this
> ultimately
> needs a judgement call by the maintainers.

The patch looks fine to me, but can you reword this message to say what
you chose and why (and not have the bit about judgment call by
maintainers in there)?  If it sounds right, it'll be acked and merged.
If not, we'll work to ensure it's all good -- so the judgment calls
happen on the list, rather than mentioning this way in the commit.

Thanks,

		Amit

Re: [PATCH 1/1] virtio_console: fix missing byte order handling for cols and rows

[Halil Pasic]

On Wed, 16 Apr 2025 15:49:40 +0200
Amit Shah <amit@kernel.org> wrote:

> On Sat, 2025-03-22 at 01:29 +0100, Halil Pasic wrote:
> > As per virtio spec the fields cols and rows are specified as little
> > endian. Although there is no legacy interface requirement that would
> > state that cols and rows need to be handled as native endian when
> > legacy
> > interface is used, unlike for the fields of the adjacent struct
> > virtio_console_control, I decided to err on the side of caution based
> > on some non-conclusive virtio spec repo archaeology and opt for using
> > virtio16_to_cpu() much like for virtio_console_control.event.
> > Strictly
> > by the letter of the spec virtio_le_to_cpu() would have been
> > sufficient.
> > But when the legacy interface is not used, it boils down to the same.
> > 
> > And when using the legacy interface, the device formatting these as
> > little endian when the guest is big endian would surprise me more
> > than
> > it using guest native byte order (which would make it compatible with
> > the current implementation). Nevertheless somebody trying to
> > implement
> > the spec following it to the letter could end up forcing little
> > endian
> > byte order when the legacy interface is in use. So IMHO this
> > ultimately
> > needs a judgement call by the maintainers.  
> 
> The patch looks fine to me, but can you reword this message to say what
> you chose and why (and not have the bit about judgment call by
> maintainers in there)?  If it sounds right, it'll be acked and merged.
> If not, we'll work to ensure it's all good -- so the judgment calls
> happen on the list, rather than mentioning this way in the commit.
> 

Sorry for the late response! I was vacationing last week.

Would you be so kind to propose a more fortunate wording? My intention
was actually to say what did I choose (I choose virtio16_to_cpu() over
virtio_le_to_cpu()) and why (if we go strictly by the words of the spec
virtio_le_to_cpu() would be right and  virtio16_to_cpu() would be wrong,
but  assumed that we forgot to put the right words into the spec it is
the other way around; and MST confirmed that indeed those words need
to be added to the spec).

The part on the judgment call is because, for me there is no way to
tell if those words are missing from the spec because of intention or
because of omission.

Regards,
Halil