Thread (23 messages) 23 messages, 5 authors, 2025-03-20

Re: [PATCH] virtio: console: Make resizing compliant with virtio spec

From: Maximilian Immanuel Brandtner <hidden>
Date: 2025-03-20 07:23:56
Also in: lkml

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:
quoted
On Mon, 2025-03-03 at 12:54 +0100, Amit Shah wrote:
quoted
On Tue, 2025-02-25 at 10:21 +0100, Maximilian Immanuel Brandtner
wrote:
quoted
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.
quoted
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
quoted
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.

quoted
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.
Would the following do?

virtio: console: Make resize control event handling compliant with spec

According to section 5.3.6.2 of the virtio spec a control buffer with
the event VIRTIO_CONSOLE_RESIZE is followed by a virtio_console_resize
struct containing 2 little endian 16bit integers cols,rows. The kernel
implementation assumes native endianness (which results in mangled
values on big endian architectures) and swaps the ordering of columns
and rows. This patch fixes these discrepancies between kernel and spec.
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.
quoted
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
  
Keyboard shortcuts
hback out one level
jnext message in thread
kprevious message in thread
ldrill in
Escclose help / fold thread tree
?toggle this help