Re: [PATCH] virtio: console: Make resizing compliant with virtio spec
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 b
Re: [PATCH] virtio: console: Make resizing compliant with virtio spec
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" 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
On Thu, 20 Mar 2025 15:09:57 +0100 Halil Pasic 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
On Thu, 2025-03-20 at 15:19 +0100, Halil Pasic wrote: > On Thu, 20 Mar 2025 15:09:57 +0100 > Halil Pasic 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.
Re: [PATCH] virtio: console: Make resizing compliant with virtio spec
On Thu, 20 Mar 2025 12:53:43 +0100 Maximilian Immanuel Brandtner 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 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
On Thu, 2025-03-20 at 11:41 +0100, Halil Pasic wrote: > On Thu, 20 Mar 2025 08:12:23 +0100 > Maximilian Immanuel Brandtner 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
On Thu, 20 Mar 2025 08:12:23 +0100 Maximilian Immanuel Brandtner 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
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 res
Re: [PATCH] virtio: console: Make resizing compliant with virtio spec
On Wed, 2025-03-19 at 18:13 +0100, Halil Pasic wrote: > On Wed, 19 Mar 2025 11:00:06 -0400 > "Michael S. Tsirkin" 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
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
Re: [PATCH] virtio: console: Make resizing compliant with virtio spec
On Wed, 19 Mar 2025 11:00:06 -0400 "Michael S. Tsirkin" 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
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 th
Re: [PATCH] virtio: console: Make resizing compliant with virtio spec
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 b
Re: [PATCH] virtio: console: Make resizing compliant with virtio spec
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
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
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
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:
> > >
> > > ```
Re: [PATCH] virtio: console: Make resizing compliant with virtio spec
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:
> > >
> > > ```
Re: [PATCH] virtio: console: Make resizing compliant with virtio spec
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 mu
Re: [PATCH] virtio: console: Make resizing compliant with virtio spec
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
Re: [PATCH] virtio: console: Make resizing compliant with virtio spec
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 seem
Re: [PATCH] virtio: console: Make resizing compliant with virtio spec
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
