Re: [PATCH v2] virtio_console: Introduce an ID allocator for virtual console numbers

["Cédric Le Goater" <clg@kaod.org>]

Hello Amit,

On 11/22/22 18:03, Amit Shah wrote:
> On Mon, 2022-11-14 at 18:38 +0100, Cédric Le Goater wrote:
>> When a virtio console port is initialized, it is registered as an hvc
>> console using a virtual console number. If a KVM guest is started with
>> multiple virtio console devices, the same vtermno (or virtual console
>> number) can be used to allocate different hvc consoles, which leads to
>> various communication problems later on.
>>
>> This is also reported in debugfs :
>>
>>    # grep vtermno /sys/kernel/debug/virtio-ports/*
>>    /sys/kernel/debug/virtio-ports/vport1p1:console_vtermno: 1
>>    /sys/kernel/debug/virtio-ports/vport2p1:console_vtermno: 1
>>    /sys/kernel/debug/virtio-ports/vport3p1:console_vtermno: 2
>>    /sys/kernel/debug/virtio-ports/vport4p1:console_vtermno: 3
>>
>> Replace the next_vtermno global with an ID allocator and start the
>> allocation at 1 as it is today. Also recycle IDs when a console port
>> is removed.
> 
> When the original virtio_console module was written, it didn't have
> support for multiple ports to be used this way.  So the oddity you're
> seeing is left there deliberately: VMMs should not be instantiating
> console ports this way.
> 
> I don't know if we should take in this change, but can you walk through
> all combinations of new/old guest and new/old hypervisor and ensure
> nothing's going to break -- and confirm with the spec this is still OK
> to do?  It may not be a goal to still ensure launches of a new guest on
> a very old (say) Centos5 guest still works -- but that was the point of
> maintaining backward compat...

'next_vtermno' was introduced by d8a02bd58ab6 ("virtio: console:
remove global var") to differentiate the underlying kernel hvc console
associated with each virtio console port. Some drivers, like XEN,
simply use a magic/cookie number for instance.

This number is not related to the virtio specs. It is not exposed to
QEMU nor the guest (a part from debugfs). It's an internal identifier
related to the implementation in the kernel. I don't understand how
this could break compatibility. The change even keeps the allocated
range the same in case some assumption is made on vtermno 0. Am I
missing something ?

In the virtio console driver case, we could also generate a unique
number from the tuple { virtio device index, virtio console port }.
The ID allocator approach is simpler.

Thanks,

C.