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

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

[Maximilian Immanuel Brandtner]

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.
While QEMU doesn't currently support resizing consoles, TinyEMU
does[1](and they implement resizing according to the spec). CrosVM has
prototypes for virtio console resizing, but no user[2]. With the JSLinux
kernel[3] and alpine config[4] resizing the kernel works. It strongly
seems that they patch the issue downstream as patching their kernel to
use the upstream ordering breaks resizing.

[0] https://docs.oasis-open.org/virtio/virtio/v1.2/virtio-v1.2.pdf
[1] https://bellard.org/tinyemu/
[2] https://github.com/search?q=repo%3Agoogle/crosvm%20virtio_console_resize&type=code
[3] https://bellard.org/jslinux/kernel-x86.bin
[4] https://bellard.org/jslinux/alpine-x86.cfg

Fixes: 8345adbf96fc1 ("virtio: console: Accept console size along with resize control message")
Signed-off-by: Maximilian Immanuel Brandtner <maxbr@linux.ibm.com>
Cc: stable@vger.kernel.org # v2.6.35+
---
 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 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))
-- 
2.48.1

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

[Amit Shah]

On Tue, 2025-02-25 at 10:21 +0100, Maximilian Immanuel Brandtner wrote:
> 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.

> 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

> 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).

But note that all this only affects devices that implement multiport
support, and have multiple console ports on a single device.  I don't
recall there are any implementations using such a configuration.

... which all leads me to ask if you've actually seen a
misconfiguration happen when trying to resize consoles which led to
this patch.

		Amit

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

[Maximilian Immanuel Brandtner]

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

I didn't know about this patch-set, however as of right now QEMU does
not set VIRTIO_CONSOLE_F_SIZE, never uses VIRTIO_CONSOLE_RESIZE, and
resizing is never mentioned in hw/char/virtio-console.c or
hw/char/virtio-serial-bus.c. Suffice to say I don't see any indicating
of resize currently being used under QEMU. Perhaps QEMU supported
resizing at one point, but not anymore. If you disagree please send me
where the resizing logic can currently be found in the QEMU source
code. I at least was unable to find it.

> 
> > 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).
> 
> But note that all this only affects devices that implement multiport
> support, and have multiple console ports on a single device.  I don't
> recall there are any implementations using such a configuration.
> 
> ... which all leads me to ask if you've actually seen a
> misconfiguration happen when trying to resize consoles which led to
> this patch.
> 
>  Amit

I'm working on implementing console resizing for virtio in QEMU and
Libvirt. As SIGWINCH is raised on the virsh frontend the new console
size needs to be transfered to QEMU (in my RFC patch via QOM, which
then causes QEMU to trigger a virtio control msg in the chr_resize
function of the virtio-console chardev). (The patch-set should make its
way unto the QEMU mailing-list soon). The way I implemented it QEMU
sends a resize control message where the control message has the
following format:

```
struct {
    le32 id;    // port->id
    le16 event; // VIRTIO_CONSOLE_RESIZE
    le16 value; // 0
    le16 cols;  // ws.ws_col
    le16 rows;  // ws.ws_row
}
```

This strongly seems to me to be in accordance with the spec[0]. It
resulted in the rows and cols being switched after a resize event. I
was able to track the issue down to this part of the kernel. Applying
the patch I sent upstream, fixed the issue.
As of right now I only implemented resize for multiport (because in the
virtio spec I was only able to find references to resizing as a control
message which requires multiport. In your email you claimed that config
space resizing exists as well. I was only able to find references to
resizing as a control message in the spec. I would love to see what
part of the spec you are refering to specifically, as it would allow me
to implement resizing without multiport as well). 
It seems to me that either the spec or the kernel implementation has to
change. If you prefer changing the spec that would be fine by me as
well, however there seems to be no implementation that uses the linux
ordering and Alpine seems to patch their kernel to use the spec
ordering instead (as described in the initial email)(this was really
Niklas Schnelle's finding so for further questions I would refer to
him).

[0]
https://docs.oasis-open.org/virtio/virtio/v1.2/csd01/virtio-v1.2-csd01.html#x1-2980006

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

[Niklas Schnelle]

On Wed, 2025-03-05 at 10:53 +0100, Maximilian Immanuel Brandtner wrote:
> On Mon, 2025-03-03 at 12:54 +0100, Amit Shah wrote:
> > On Tue, 2025-02-25 at 10:21 +0100, Maximilian Immanuel Brandtner
> > wrote:
> > > 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.
> > 
> > > 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
> 
> I didn't know about this patch-set, however as of right now QEMU does
> not set VIRTIO_CONSOLE_F_SIZE, never uses VIRTIO_CONSOLE_RESIZE, and
> resizing is never mentioned in hw/char/virtio-console.c or
> hw/char/virtio-serial-bus.c. Suffice to say I don't see any indicating
> of resize currently being used under QEMU. Perhaps QEMU supported
> resizing at one point, but not anymore. If you disagree please send me
> where the resizing logic can currently be found in the QEMU source
> code. I at least was unable to find it.
> 
> > 
> > > 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).
> > 
> > But note that all this only affects devices that implement multiport
> > support, and have multiple console ports on a single device.  I don't
> > recall there are any implementations using such a configuration.
> > 
> > ... which all leads me to ask if you've actually seen a
> > misconfiguration happen when trying to resize consoles which led to
> > this patch.
> > 
> >  Amit
> 
> I'm working on implementing console resizing for virtio in QEMU and
> Libvirt. As SIGWINCH is raised on the virsh frontend the new console
> size needs to be transfered to QEMU (in my RFC patch via QOM, which
> then causes QEMU to trigger a virtio control msg in the chr_resize
> function of the virtio-console chardev). (The patch-set should make its
> way unto the QEMU mailing-list soon). The way I implemented it QEMU
> sends a resize control message where the control message has the
> following format:
> 
> ```
> struct {
>     le32 id;    // port->id
>     le16 event; // VIRTIO_CONSOLE_RESIZE
>     le16 value; // 0
>     le16 cols;  // ws.ws_col
>     le16 rows;  // ws.ws_row
> }
> ```
> 
> This strongly seems to me to be in accordance with the spec[0]. It
> resulted in the rows and cols being switched after a resize event. I
> was able to track the issue down to this part of the kernel. Applying
> the patch I sent upstream, fixed the issue.
> As of right now I only implemented resize for multiport (because in the
> virtio spec I was only able to find references to resizing as a control
> message which requires multiport. In your email you claimed that config
> space resizing exists as well. I was only able to find references to
> resizing as a control message in the spec. I would love to see what
> part of the spec you are refering to specifically, as it would allow me
> to implement resizing without multiport as well). 
> It seems to me that either the spec or the kernel implementation has to
> change. If you prefer changing the spec that would be fine by me as
> well, however there seems to be no implementation that uses the linux
> ordering and Alpine seems to patch their kernel to use the spec
> ordering instead (as described in the initial email)(this was really
> Niklas Schnelle's finding so for further questions I would refer to
> him).

I don't think this was patched in the (official) alpine kernel. What
happened is that I tested TinyEMU[0] with the kernel + initrd from the
JSLinux site and that has working console resizing. In the TinyEMU code
this is implemented in TinyEMU/virtio.c:virtio_console_resize_event():

void virtio_console_resize_event(VIRTIODevice *s, int width, int height)
{
    /* indicate the console size */
    put_le16(s->config_space + 0, width);
    put_le16(s->config_space + 2, height);

    virtio_config_change_notify(s);
}

On second look this indeed seems to use the config space. It writes
first the width then height which matches config_work_handler(). But
like Maximilian I could only find the VIRTIO_CONSOLE_RESIZE message
mechanism in the spec, also with width (cols) then height (rows) but
not matching the kernel struct changed by this patch.

Thanks,
Niklas

[0] https://bellard.org/tinyemu/

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

[Niklas Schnelle]

On Wed, 2025-03-05 at 13:13 +0100, Niklas Schnelle wrote:
> On Wed, 2025-03-05 at 10:53 +0100, Maximilian Immanuel Brandtner wrote:
> > On Mon, 2025-03-03 at 12:54 +0100, Amit Shah wrote:
> > > On Tue, 2025-02-25 at 10:21 +0100, Maximilian Immanuel Brandtner
> > > wrote:
> > > > 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.
> > > 
> > > > 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
> > 
> > I didn't know about this patch-set, however as of right now QEMU does
> > not set VIRTIO_CONSOLE_F_SIZE, never uses VIRTIO_CONSOLE_RESIZE, and
> > resizing is never mentioned in hw/char/virtio-console.c or
> > hw/char/virtio-serial-bus.c. Suffice to say I don't see any indicating
> > of resize currently being used under QEMU. Perhaps QEMU supported
> > resizing at one point, but not anymore. If you disagree please send me
> > where the resizing logic can currently be found in the QEMU source
> > code. I at least was unable to find it.
> > 
> > > 
> > > > 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).
> > > 
> > > But note that all this only affects devices that implement multiport
> > > support, and have multiple console ports on a single device.  I don't
> > > recall there are any implementations using such a configuration.
> > > 
> > > ... which all leads me to ask if you've actually seen a
> > > misconfiguration happen when trying to resize consoles which led to
> > > this patch.
> > > 
> > >  Amit
> > 
> > I'm working on implementing console resizing for virtio in QEMU and
> > Libvirt. As SIGWINCH is raised on the virsh frontend the new console
> > size needs to be transfered to QEMU (in my RFC patch via QOM, which
> > then causes QEMU to trigger a virtio control msg in the chr_resize
> > function of the virtio-console chardev). (The patch-set should make its
> > way unto the QEMU mailing-list soon). The way I implemented it QEMU
> > sends a resize control message where the control message has the
> > following format:
> > 
> > ```
> > struct {
> >     le32 id;    // port->id
> >     le16 event; // VIRTIO_CONSOLE_RESIZE
> >     le16 value; // 0
> >     le16 cols;  // ws.ws_col
> >     le16 rows;  // ws.ws_row
> > }
> > ```
> > 
> > This strongly seems to me to be in accordance with the spec[0]. It
> > resulted in the rows and cols being switched after a resize event. I
> > was able to track the issue down to this part of the kernel. Applying
> > the patch I sent upstream, fixed the issue.
> > As of right now I only implemented resize for multiport (because in the
> > virtio spec I was only able to find references to resizing as a control
> > message which requires multiport. In your email you claimed that config
> > space resizing exists as well. I was only able to find references to
> > resizing as a control message in the spec. I would love to see what
> > part of the spec you are refering to specifically, as it would allow me
> > to implement resizing without multiport as well). 
> > It seems to me that either the spec or the kernel implementation has to
> > change. If you prefer changing the spec that would be fine by me as
> > well, however there seems to be no implementation that uses the linux
> > ordering and Alpine seems to patch their kernel to use the spec
> > ordering instead (as described in the initial email)(this was really
> > Niklas Schnelle's finding so for further questions I would refer to
> > him).
> 
> I don't think this was patched in the (official) alpine kernel. What
> happened is that I tested TinyEMU[0] with the kernel + initrd from the
> JSLinux site and that has working console resizing. In the TinyEMU code
> this is implemented in TinyEMU/virtio.c:virtio_console_resize_event():
> 
> void virtio_console_resize_event(VIRTIODevice *s, int width, int height)
> {
>     /* indicate the console size */
>     put_le16(s->config_space + 0, width);
>     put_le16(s->config_space + 2, height);
> 
>     virtio_config_change_notify(s);
> }
> 
> On second look this indeed seems to use the config space. It writes
> first the width then height which matches config_work_handler(). But
> like Maximilian I could only find the VIRTIO_CONSOLE_RESIZE message
> mechanism in the spec, also with width (cols) then height (rows) but
> not matching the kernel struct changed by this patch.
> 

Forgot to note, the idea that Alpine was patched came because the
kernel used in TinyEMU has '-dirty' in the local version so we were
wondering if it was patched for this. But seeing the
config_work_handler() it's probably just using that.

Thanks,
Niklas

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

[Maximilian Immanuel Brandtner]

On Wed, 2025-03-05 at 13:15 +0100, Niklas Schnelle wrote:
> On Wed, 2025-03-05 at 13:13 +0100, Niklas Schnelle wrote:
> > On Wed, 2025-03-05 at 10:53 +0100, Maximilian Immanuel Brandtner
> > wrote:
> > > On Mon, 2025-03-03 at 12:54 +0100, Amit Shah wrote:
> > > > On Tue, 2025-02-25 at 10:21 +0100, Maximilian Immanuel
> > > > Brandtner
> > > > wrote:
> > > > > 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.
> > > > 
> > > > > 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
> > > 
> > > I didn't know about this patch-set, however as of right now QEMU
> > > does
> > > not set VIRTIO_CONSOLE_F_SIZE, never uses VIRTIO_CONSOLE_RESIZE,
> > > and
> > > resizing is never mentioned in hw/char/virtio-console.c or
> > > hw/char/virtio-serial-bus.c. Suffice to say I don't see any
> > > indicating
> > > of resize currently being used under QEMU. Perhaps QEMU supported
> > > resizing at one point, but not anymore. If you disagree please
> > > send me
> > > where the resizing logic can currently be found in the QEMU
> > > source
> > > code. I at least was unable to find it.
> > > 
> > > > 
> > > > > 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).
> > > > 
> > > > But note that all this only affects devices that implement
> > > > multiport
> > > > support, and have multiple console ports on a single device.  I
> > > > don't
> > > > recall there are any implementations using such a
> > > > configuration.
> > > > 
> > > > ... which all leads me to ask if you've actually seen a
> > > > misconfiguration happen when trying to resize consoles which
> > > > led to
> > > > this patch.
> > > > 
> > > >  Amit
> > > 
> > > I'm working on implementing console resizing for virtio in QEMU
> > > and
> > > Libvirt. As SIGWINCH is raised on the virsh frontend the new
> > > console
> > > size needs to be transfered to QEMU (in my RFC patch via QOM,
> > > which
> > > then causes QEMU to trigger a virtio control msg in the
> > > chr_resize
> > > function of the virtio-console chardev). (The patch-set should
> > > make its
> > > way unto the QEMU mailing-list soon). The way I implemented it
> > > QEMU
> > > sends a resize control message where the control message has the
> > > following format:
> > > 
> > > ```
> > > struct {
> > >     le32 id;    // port->id
> > >     le16 event; // VIRTIO_CONSOLE_RESIZE
> > >     le16 value; // 0
> > >     le16 cols;  // ws.ws_col
> > >     le16 rows;  // ws.ws_row
> > > }
> > > ```
> > > 
> > > This strongly seems to me to be in accordance with the spec[0].
> > > It
> > > resulted in the rows and cols being switched after a resize
> > > event. I
> > > was able to track the issue down to this part of the kernel.
> > > Applying
> > > the patch I sent upstream, fixed the issue.
> > > As of right now I only implemented resize for multiport (because
> > > in the
> > > virtio spec I was only able to find references to resizing as a
> > > control
> > > message which requires multiport. In your email you claimed that
> > > config
> > > space resizing exists as well. I was only able to find references
> > > to
> > > resizing as a control message in the spec. I would love to see
> > > what
> > > part of the spec you are refering to specifically, as it would
> > > allow me
> > > to implement resizing without multiport as well). 
> > > It seems to me that either the spec or the kernel implementation
> > > has to
> > > change. If you prefer changing the spec that would be fine by me
> > > as
> > > well, however there seems to be no implementation that uses the
> > > linux
> > > ordering and Alpine seems to patch their kernel to use the spec
> > > ordering instead (as described in the initial email)(this was
> > > really
> > > Niklas Schnelle's finding so for further questions I would refer
> > > to
> > > him).
> > 
> > I don't think this was patched in the (official) alpine kernel.
> > What
> > happened is that I tested TinyEMU[0] with the kernel + initrd from
> > the
> > JSLinux site and that has working console resizing. In the TinyEMU
> > code
> > this is implemented in
> > TinyEMU/virtio.c:virtio_console_resize_event():
> > 
> > void virtio_console_resize_event(VIRTIODevice *s, int width, int
> > height)
> > {
> >     /* indicate the console size */
> >     put_le16(s->config_space + 0, width);
> >     put_le16(s->config_space + 2, height);
> > 
> >     virtio_config_change_notify(s);
> > }
> > 
> > On second look this indeed seems to use the config space. It writes
> > first the width then height which matches config_work_handler().
> > But
> > like Maximilian I could only find the VIRTIO_CONSOLE_RESIZE message
> > mechanism in the spec, also with width (cols) then height (rows)
> > but
> > not matching the kernel struct changed by this patch.
> > 
> 
> Forgot to note, the idea that Alpine was patched came because the
> kernel used in TinyEMU has '-dirty' in the local version so we were
> wondering if it was patched for this. But seeing the
> config_work_handler() it's probably just using that.
> 
> Thanks,
> Niklas

In that case it might be better to change the spec to reflect the
kernel implementation of resize control messages.

Max

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

[Maximilian Immanuel Brandtner]

On Wed, 2025-03-05 at 13:15 +0100, Niklas Schnelle wrote:
> On Wed, 2025-03-05 at 13:13 +0100, Niklas Schnelle wrote:
> > On Wed, 2025-03-05 at 10:53 +0100, Maximilian Immanuel Brandtner
> > wrote:
> > > On Mon, 2025-03-03 at 12:54 +0100, Amit Shah wrote:
> > > > On Tue, 2025-02-25 at 10:21 +0100, Maximilian Immanuel
> > > > Brandtner
> > > > wrote:
> > > > > 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.
> > > > 
> > > > > 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
> > > 
> > > I didn't know about this patch-set, however as of right now QEMU
> > > does
> > > not set VIRTIO_CONSOLE_F_SIZE, never uses VIRTIO_CONSOLE_RESIZE,
> > > and
> > > resizing is never mentioned in hw/char/virtio-console.c or
> > > hw/char/virtio-serial-bus.c. Suffice to say I don't see any
> > > indicating
> > > of resize currently being used under QEMU. Perhaps QEMU supported
> > > resizing at one point, but not anymore. If you disagree please
> > > send me
> > > where the resizing logic can currently be found in the QEMU
> > > source
> > > code. I at least was unable to find it.
> > > 
> > > > 
> > > > > 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).
> > > > 
> > > > But note that all this only affects devices that implement
> > > > multiport
> > > > support, and have multiple console ports on a single device.  I
> > > > don't
> > > > recall there are any implementations using such a
> > > > configuration.
> > > > 
> > > > ... which all leads me to ask if you've actually seen a
> > > > misconfiguration happen when trying to resize consoles which
> > > > led to
> > > > this patch.
> > > > 
> > > >  Amit
> > > 
> > > I'm working on implementing console resizing for virtio in QEMU
> > > and
> > > Libvirt. As SIGWINCH is raised on the virsh frontend the new
> > > console
> > > size needs to be transfered to QEMU (in my RFC patch via QOM,
> > > which
> > > then causes QEMU to trigger a virtio control msg in the
> > > chr_resize
> > > function of the virtio-console chardev). (The patch-set should
> > > make its
> > > way unto the QEMU mailing-list soon). The way I implemented it
> > > QEMU
> > > sends a resize control message where the control message has the
> > > following format:
> > > 
> > > ```
> > > struct {
> > >     le32 id;    // port->id
> > >     le16 event; // VIRTIO_CONSOLE_RESIZE
> > >     le16 value; // 0
> > >     le16 cols;  // ws.ws_col
> > >     le16 rows;  // ws.ws_row
> > > }
> > > ```
> > > 
> > > This strongly seems to me to be in accordance with the spec[0].
> > > It
> > > resulted in the rows and cols being switched after a resize
> > > event. I
> > > was able to track the issue down to this part of the kernel.
> > > Applying
> > > the patch I sent upstream, fixed the issue.
> > > As of right now I only implemented resize for multiport (because
> > > in the
> > > virtio spec I was only able to find references to resizing as a
> > > control
> > > message which requires multiport. In your email you claimed that
> > > config
> > > space resizing exists as well. I was only able to find references
> > > to
> > > resizing as a control message in the spec. I would love to see
> > > what
> > > part of the spec you are refering to specifically, as it would
> > > allow me
> > > to implement resizing without multiport as well). 
> > > It seems to me that either the spec or the kernel implementation
> > > has to
> > > change. If you prefer changing the spec that would be fine by me
> > > as
> > > well, however there seems to be no implementation that uses the
> > > linux
> > > ordering and Alpine seems to patch their kernel to use the spec
> > > ordering instead (as described in the initial email)(this was
> > > really
> > > Niklas Schnelle's finding so for further questions I would refer
> > > to
> > > him).
> > 
> > I don't think this was patched in the (official) alpine kernel.
> > What
> > happened is that I tested TinyEMU[0] with the kernel + initrd from
> > the
> > JSLinux site and that has working console resizing. In the TinyEMU
> > code
> > this is implemented in
> > TinyEMU/virtio.c:virtio_console_resize_event():
> > 
> > void virtio_console_resize_event(VIRTIODevice *s, int width, int
> > height)
> > {
> >     /* indicate the console size */
> >     put_le16(s->config_space + 0, width);
> >     put_le16(s->config_space + 2, height);
> > 
> >     virtio_config_change_notify(s);
> > }
> > 
> > On second look this indeed seems to use the config space. It writes
> > first the width then height which matches config_work_handler().
> > But
> > like Maximilian I could only find the VIRTIO_CONSOLE_RESIZE message
> > mechanism in the spec, also with width (cols) then height (rows)
> > but
> > not matching the kernel struct changed by this patch.
> > 
> 
> Forgot to note, the idea that Alpine was patched came because the
> kernel used in TinyEMU has '-dirty' in the local version so we were
> wondering if it was patched for this. But seeing the
> config_work_handler() it's probably just using that.
> 
> Thanks,
> Niklas

Just to make sure that everyone here is one the same page there is
indeed a difference between the ordering of the control resize message
and the kernel implementation; however as this bug has been around for
~15 years the spec should be changed instead, right?

I would like to get a clear ACK of the issue, as I would like to
reference this discussion when creating a bug-report on the virtio-spec
github.

Thanks,
Max

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

[Amit Shah]

On Mon, 2025-03-10 at 14:04 +0100, Maximilian Immanuel Brandtner wrote:


[...]

> Just to make sure that everyone here is one the same page there is
> indeed a difference between the ordering of the control resize
> message
> and the kernel implementation; however as this bug has been around
> for
> ~15 years the spec should be changed instead, right?
> 
> I would like to get a clear ACK of the issue, as I would like to
> reference this discussion when creating a bug-report on the virtio-
> spec
> github.

I'm afraid you haven't understood the difference between a control
message for an individual port, and the config space for the entire
device.  Please re-read my post earlier in the thread, and follow the
code.  There's no divergence in the implementation and the spec, and
there's nothing to fix.  If anything, there may be a chance to add to
the spec the order for the control message - though I don't think
there's a strong need to.

		Amit

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

[Maximilian Immanuel Brandtner]

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

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.

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?

> 
> But note that all this only affects devices that implement multiport
> support, and have multiple console ports on a single device.  I don't
> recall there are any implementations using such a configuration.
> 
> ... which all leads me to ask if you've actually seen a
> misconfiguration happen when trying to resize consoles which led to
> this patch.
> 
> 		Amit

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

[Amit Shah]

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


> 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.

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.

> 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

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

[Maximilian Immanuel Brandtner]

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

The way I understand VIRTIO_CONSOLE_F_SIZE, it has to be negotiated for
any resize events to be sent (including resize control messages) (at
least as of right now it is necessary to enable this feature to sent
resize control messages). Just above in section 5.3.4 cols and rows are
mentioned in the struct virtio_console_config for
VIRTIO_CONSOLE_F_EMERG_WRITE. Were you referring to that? In that case
would it be better for the hypervisor to resize via an emergency write
(aka as a config message) or over multiport (aka as a control event).
It seems to me that VIRTIO_CONSOLE_F_EMERG_WRITE is meant for debugging
and early writes rather than regular events, but I could be mistaken.

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

[Amit Shah]

On Wed, 2025-03-19 at 09:54 +0100, Maximilian Immanuel Brandtner wrote:
> 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:
> > > On Mon, 2025-03-03 at 12:54 +0100, Amit Shah wrote:
> > > > On Tue, 2025-02-25 at 10:21 +0100, Maximilian Immanuel
> > > > Brandtner
> > > > wrote:
> > > > > 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.
> > > > 
> > > > > 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
> > > > 
> > > > > 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.
> > 
> > 
> > > 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.
> > 
> > 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.
> > 
> > > 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
> 
> The way I understand VIRTIO_CONSOLE_F_SIZE, it has to be negotiated
> for
> any resize events to be sent (including resize control messages) (at
> least as of right now it is necessary to enable this feature to sent
> resize control messages).

That's right, but virtio feature negotiation just means that the host
and guest figure out what the other endpoint supports.  Console
resizing wasn't part of the orig virtio console implementation, and was
added later.

For the resizing case, it's one-way communication from the host to the
guest: the host sets row/col values and notifies the guest.  If the
guest is old and does not have the F_SIZE feature, it will just ignore
it.  If the host doesn't have that feature, well, it can't send the
resize messages.

The commit to resize the console was added way back in 2008:

https://web.git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/drivers/char/virtio_console.c?id=c29834584ea4eafccf2f62a0b8a32e64f792044c

This line:

+	if (virtio_has_feature(dev, VIRTIO_CONSOLE_F_SIZE)) {

is the negotiation part -- if the host device has that feature, read
the values from the config space and apply them.

		Amit

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

[Maximilian Immanuel Brandtner]

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

So should I send a patch v2 or should the spec be changed instead? Or
would you like to first await the opinion of the spec maintainers?

The mail I initially sent to the virtio mailing list seems to have
fallen on deaf ears. I now added Michael Tsirkin to this thread so that
things might get going.

> 
> 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.
> 
> > 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

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

[Michael S. Tsirkin]

On Wed, Mar 19, 2025 at 02:00:44PM +0100, Maximilian Immanuel Brandtner wrote:
> 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:
> > > On Mon, 2025-03-03 at 12:54 +0100, Amit Shah wrote:
> > > > On Tue, 2025-02-25 at 10:21 +0100, Maximilian Immanuel Brandtner
> > > > wrote:
> > > > > 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.
> > > > 
> > > > > 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
> > > > 
> > > > > 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.
> > 
> > 
> > > 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.
> 
> So should I send a patch v2 or should the spec be changed instead? Or
> would you like to first await the opinion of the spec maintainers?
> 
> The mail I initially sent to the virtio mailing list seems to have
> fallen on deaf ears. I now added Michael Tsirkin to this thread so that
> things might get going.


If we can fix the driver to fit the spec, that's best.
We generally try to avoid changing the spec just because
drivers are buggy.

> > 
> > 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.
> > 
> > > 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

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

[Halil Pasic]

On Wed, 19 Mar 2025 11:00:06 -0400
"Michael S. Tsirkin" <mst@redhat.com> wrote:

> > > 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.  
> > 
> > So should I send a patch v2 or should the spec be changed instead? Or
> > would you like to first await the opinion of the spec maintainers?
> > 
> > The mail I initially sent to the virtio mailing list seems to have
> > fallen on deaf ears. I now added Michael Tsirkin to this thread so that
> > things might get going.  
> 
> 
> If we can fix the driver to fit the spec, that's best.
> We generally try to avoid changing the spec just because
> drivers are buggy.

I think the call if fixing the driver is possible needs to be made by
the maintainers of the driver. Fixing the driver IMHO implies that
if this is seeing any usage in the wild where it properly works a
fix on the driver side would imply a function regression. But any
implementers should have complained. So IMHO it is not unreasonable to
assume that this is not seeing any usage in the wild.

And people would still have the opportunity to catch the regression
during testing and complain about it.

I agree with Michael, changing the spec because of a buggy
implementation should rather be the exception than the rule. And AFAIK
it is not like we have declared something a reference implementation,
so in that sense the implementation in Linux is just one implementation.

I suppose making it runtime configurable via module parameter is an
overkill at this point.

So based no what we know I'm slightly in favor of let us just fix it
in Linux and see if anybody complains.

Another thing I noticed during looking at this. AFAICT Linux does not
seem to handle endiannes here. And AFAIU the message is supposed to hold
le16 that is little endian u16! Maximilian, is this in your opinion
something we need to fix as well? Or am I just missing the conversion?

Regards,
Halil

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

[Michael S. Tsirkin]

On Wed, Mar 19, 2025 at 06:13:08PM +0100, Halil Pasic wrote:
> On Wed, 19 Mar 2025 11:00:06 -0400
> "Michael S. Tsirkin" <mst@redhat.com> wrote:
> 
> > > > 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.  
> > > 
> > > So should I send a patch v2 or should the spec be changed instead? Or
> > > would you like to first await the opinion of the spec maintainers?
> > > 
> > > The mail I initially sent to the virtio mailing list seems to have
> > > fallen on deaf ears. I now added Michael Tsirkin to this thread so that
> > > things might get going.  
> > 
> > 
> > If we can fix the driver to fit the spec, that's best.
> > We generally try to avoid changing the spec just because
> > drivers are buggy.
> 
> I think the call if fixing the driver is possible needs to be made by
> the maintainers of the driver. Fixing the driver IMHO implies that
> if this is seeing any usage in the wild where it properly works a
> fix on the driver side would imply a function regression. But any
> implementers should have complained. So IMHO it is not unreasonable to
> assume that this is not seeing any usage in the wild.
> 
> And people would still have the opportunity to catch the regression
> during testing and complain about it.
> 
> I agree with Michael, changing the spec because of a buggy
> implementation should rather be the exception than the rule. And AFAIK
> it is not like we have declared something a reference implementation,
> so in that sense the implementation in Linux is just one implementation.
> 
> I suppose making it runtime configurable via module parameter is an
> overkill at this point.
> 
> So based no what we know I'm slightly in favor of let us just fix it
> in Linux and see if anybody complains.
> 
> Another thing I noticed during looking at this. AFAICT Linux does not
> seem to handle endiannes here. And AFAIU the message is supposed to hold
> le16 that is little endian u16! Maximilian, is this in your opinion
> something we need to fix as well? Or am I just missing the conversion?
> 
> Regards,
> Halil

Agreed, still, please do a bit of research on open source hypervisors
at least (rust-vmm?) and include the info which ones you checked.

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

[Maximilian Immanuel Brandtner]

On Wed, 2025-03-19 at 18:13 +0100, Halil Pasic wrote:
> On Wed, 19 Mar 2025 11:00:06 -0400
> "Michael S. Tsirkin" <mst@redhat.com> wrote:
> 
> > > > 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.  
> > > 
> > > So should I send a patch v2 or should the spec be changed
> > > instead? Or
> > > would you like to first await the opinion of the spec
> > > maintainers?
> > > 
> > > The mail I initially sent to the virtio mailing list seems to
> > > have
> > > fallen on deaf ears. I now added Michael Tsirkin to this thread
> > > so that
> > > things might get going.  
> > 
> > 
> > If we can fix the driver to fit the spec, that's best.
> > We generally try to avoid changing the spec just because
> > drivers are buggy.
> 
> I think the call if fixing the driver is possible needs to be made by
> the maintainers of the driver. Fixing the driver IMHO implies that
> if this is seeing any usage in the wild where it properly works a
> fix on the driver side would imply a function regression. But any
> implementers should have complained. So IMHO it is not unreasonable
> to
> assume that this is not seeing any usage in the wild.
> 
> And people would still have the opportunity to catch the regression
> during testing and complain about it.
> 
> I agree with Michael, changing the spec because of a buggy
> implementation should rather be the exception than the rule. And
> AFAIK
> it is not like we have declared something a reference implementation,
> so in that sense the implementation in Linux is just one
> implementation.
> 
> I suppose making it runtime configurable via module parameter is an
> overkill at this point.
> 
> So based no what we know I'm slightly in favor of let us just fix it
> in Linux and see if anybody complains.
> 
> Another thing I noticed during looking at this. AFAICT Linux does not
> seem to handle endiannes here. And AFAIU the message is supposed to
> hold
> le16 that is little endian u16! Maximilian, is this in your opinion
> something we need to fix as well? Or am I just missing the
> conversion?
> 
> Regards,
> Halil

Thanks, I didn't notice that, as I only tested this feature on x86. I
double checked struct virtio_console_config as it also defines cols
before rows, but there the kernel follows the spec (including
endianness).
I'll send a patch v2 shortly.

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

[Maximilian Immanuel Brandtner]

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

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

[Halil Pasic]

On Thu, 20 Mar 2025 08:12:23 +0100
Maximilian Immanuel Brandtner <maxbr@linux.ibm.com> wrote:

> > Another thing I noticed during looking at this. AFAICT Linux does not
> > seem to handle endiannes here. And AFAIU the message is supposed to
> > hold
> > le16 that is little endian u16! Maximilian, is this in your opinion
> > something we need to fix as well? Or am I just missing the
> > conversion?
> > 
> > Regards,
> > Halil  
> 
> Thanks, I didn't notice that, as I only tested this feature on x86. I
> double checked struct virtio_console_config as it also defines cols
> before rows, but there the kernel follows the spec (including
> endianness).
> I'll send a patch v2 shortly.

I can send a fix for the endiannes issue. It should be a separate
patch anyway.

Regards,
Halil

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

[Maximilian Immanuel Brandtner]

On Thu, 2025-03-20 at 11:41 +0100, Halil Pasic wrote:
> On Thu, 20 Mar 2025 08:12:23 +0100
> Maximilian Immanuel Brandtner <maxbr@linux.ibm.com> wrote:
> 
> > > Another thing I noticed during looking at this. AFAICT Linux does
> > > not
> > > seem to handle endiannes here. And AFAIU the message is supposed
> > > to
> > > hold
> > > le16 that is little endian u16! Maximilian, is this in your
> > > opinion
> > > something we need to fix as well? Or am I just missing the
> > > conversion?
> > > 
> > > Regards,
> > > Halil  
> > 
> > Thanks, I didn't notice that, as I only tested this feature on x86.
> > I
> > double checked struct virtio_console_config as it also defines cols
> > before rows, but there the kernel follows the spec (including
> > endianness).
> > I'll send a patch v2 shortly.
> 
> I can send a fix for the endiannes issue. It should be a separate
> patch anyway.
> 
> Regards,
> Halil

I already implemented it in my patch v2 (just waiting for Amit to
confirm the new commit message). But if you want to split it you can
create a seperate patch for it as well (I don't really mind either
way).

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

[Halil Pasic]

On Thu, 20 Mar 2025 12:53:43 +0100
Maximilian Immanuel Brandtner <maxbr@linux.ibm.com> wrote:

> On Thu, 2025-03-20 at 11:41 +0100, Halil Pasic wrote:
> > On Thu, 20 Mar 2025 08:12:23 +0100
> > Maximilian Immanuel Brandtner <maxbr@linux.ibm.com> wrote:
> >   
> > > > Another thing I noticed during looking at this. AFAICT Linux does
> > > > not
> > > > seem to handle endiannes here. And AFAIU the message is supposed
> > > > to
> > > > hold
> > > > le16 that is little endian u16! Maximilian, is this in your
> > > > opinion
> > > > something we need to fix as well? Or am I just missing the
> > > > conversion?
> > > > 
> > > > Regards,
> > > > Halil    
> > > 
> > > Thanks, I didn't notice that, as I only tested this feature on x86.
> > > I
> > > double checked struct virtio_console_config as it also defines cols
> > > before rows, but there the kernel follows the spec (including
> > > endianness).
> > > I'll send a patch v2 shortly.  
> > 
> > I can send a fix for the endiannes issue. It should be a separate
> > patch anyway.
> > 
> > Regards,
> > Halil  
> 
> I already implemented it in my patch v2 (just waiting for Amit to
> confirm the new commit message). But if you want to split it you can
> create a seperate patch for it as well (I don't really mind either
> way).
> 

It is conceptually a different bug and warrants a patch and a commit
message of its own. At least IMHO.

Regards,
Halil

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

[Halil Pasic]

On Thu, 20 Mar 2025 15:09:57 +0100
Halil Pasic <pasic@linux.ibm.com> wrote:

> > I already implemented it in my patch v2 (just waiting for Amit to
> > confirm the new commit message). But if you want to split it you can
> > create a seperate patch for it as well (I don't really mind either
> > way).
> >   

Your v2 has not been posted yet, or? I can't find it in my Inbox.

I understand that you have confirmed that the byte order handling is
needed but missing, right?

> 
> It is conceptually a different bug and warrants a patch and a commit
> message of its own. At least IMHO.

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

[Maximilian Immanuel Brandtner]

On Thu, 2025-03-20 at 15:19 +0100, Halil Pasic wrote:
> On Thu, 20 Mar 2025 15:09:57 +0100
> Halil Pasic <pasic@linux.ibm.com> wrote:
> 
> > > I already implemented it in my patch v2 (just waiting for Amit to
> > > confirm the new commit message). But if you want to split it you
> > > can
> > > create a seperate patch for it as well (I don't really mind
> > > either
> > > way).
> > >   
> 
> Your v2 has not been posted yet, or? I can't find it in my Inbox.
> 
> I understand that you have confirmed that the byte order handling is
> needed but missing, right?

Yes, I wanted to first hear back from Amit whether he liked the new
commit message, but seeing as he hasn't yet replied I'll just post it
now. I've confirmed that the endianness handling is necessary, but not
implementated. I've looked all the way down into hvc_console() up until
tty_do_resize(), but there are no endianess adjustments up until that
point aka I'm pretty certain the endianness is broken.

I'll post my v2 without the endianness fix then

> 
> > 
> > It is conceptually a different bug and warrants a patch and a
> > commit
> > message of its own. At least IMHO.