Re: [PATCH] virtio: console: remove check for cpkt value when nominating console port

From: Amit Shah <amit@infradead.org>
To: Andy Bui <andy.bui2001@gmail.com>
Cc: virtualization@lists.linux-foundation.org, amit@kernel.org
Subject: Re: [PATCH] virtio: console: remove check for cpkt value when nominating console port
Date: Wed, 23 Nov 2022 11:46:54 +0100	[thread overview]
Message-ID: <639e5da00eff26d9fe4be8bed8bc3c91ee3b72a3.camel@infradead.org> (raw)
In-Reply-To: <Y3MD2vRn+8r+wp61@DESKTOP-UNJJAT8.localdomain>

On Tue, 2022-11-15 at 14:13 +1100, Andy Bui wrote:
> On Mon, Nov 14, 2022 at 09:43:48AM -0800, Amit Shah wrote:
> > 
> > On Sat, 2022-11-12 at 23:45 +1100, Andy Bui wrote:
> > > The virtIO spec does not specify a need for a value when nominating a
> > > port as a console port, yet the virtio_console driver requires the value
> > > to be 1.
> > > 
> > > Besides being a check that's not specified by the virtIO spec, I don't
> > > see anywhere else in the kernel the value is used when the corresponding
> > > event is VIRTIO_CONSOLE_CONSOLE_PORT. As an example QEMU also currently
> > > only passes in value=1 when nominating a console port.
> > 
> > The original virtio-console driver just had the one port, and it was,
> > as the name suggests, was a console port.  When we converted this
> > console driver to a generic serial driver, the first port was kept as
> > the console port to not break old drivers or hypervisors.  I'm afraid
> > we'll have to keep this bit of backward compatibility forever.
> > 
> 
> I think I might be misunderstanding the exact usage of the cpkt value.
> Is error'ing and not continuing with the console port nomination the intended
> behaviour when value != 1? I'm guessing that there are some hypervisors out
> there that set value to something not 1 and we don't want those ports to be
> the console ports?

I've not looked at the code in quite a bit - please bear with me as I
refresh my memory.

I think the backward compat concern I have here is that a really old
hypervisor - one that doesn't do virtio-serial-ports yet, will not have
any other messages sent from the host, but the original console setup
messages.

The is_serial_port() check looks for whether hvc is initialized.  It's
possible hvc may not have been initialized when some control message
comes in.

> If this is correct I think a warning should be emitted here? Not setting the
> value to 1 can exhibit undefined behaviour (at least when interpreting the
> virtIO spec) and it would help hypervisor developers.

Did you actually run into some issue here?

		Amit
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization