Virtualization - Nov 2022 - [PATCH] virtio: console: remove check for cpkt value when nominating console port

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?

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.

Thanks and lmk your thoughts,
Andy

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