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

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

[Amit Shah]

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.


		Amit

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

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

[Andy Bui]

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

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

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

[Amit Shah]

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

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

[Andy Bui]

On Wed, Nov 23, 2022 at 11:46:54AM +0100, Amit Shah wrote:
> 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.

I'm assuming you're talking about the is_console_port() check here (apologies
I'm not too familiar with the old virtio-serial), in which case I have no problem
with.

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

Yes, while developing a hypervisor solution I found that console port setup
was unsuccessful when the control messages were involved. I found out later on
after reading the qemu and linux source that a value of 1 was needed for all
VIRTIO_CONSOLE_CONSOLE_PORT events, which wasn't exactly specified in the
spec.

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