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