Messages in this thread | | | Date | Mon, 14 Nov 2022 18:00:13 +0100 | Subject | Re: [PATCH] virtio_console: Use an atomic to allocate virtual console numbers | From | Cédric Le Goater <> |
| |
On 11/14/22 17:18, Greg Kroah-Hartman wrote: > On Mon, Nov 14, 2022 at 05:03:40PM +0100, Cédric Le Goater wrote: >> On 11/14/22 09:57, Greg Kroah-Hartman wrote: >>> On Mon, Nov 14, 2022 at 09:07:52AM +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 >>>> >>>> Fix the issue with an atomic variable and start the first console >>>> number at 1 as it is today. >>>> >>>> Signed-off-by: Cédric Le Goater <clg@kaod.org> >>>> --- >>>> drivers/char/virtio_console.c | 8 ++++---- >>>> 1 file changed, 4 insertions(+), 4 deletions(-) >>>> >>>> diff --git a/drivers/char/virtio_console.c b/drivers/char/virtio_console.c >>>> index 9fa3c76a267f..253574f41e57 100644 >>>> --- a/drivers/char/virtio_console.c >>>> +++ b/drivers/char/virtio_console.c >>>> @@ -58,12 +58,13 @@ struct ports_driver_data { >>>> * We also just assume the first console being initialised was >>>> * the first one that got used as the initial console. >>>> */ >>>> - unsigned int next_vtermno; >>>> + atomic_t next_vtermno; >>>> /* All the console devices handled by this driver */ >>>> struct list_head consoles; >>>> }; >>>> -static struct ports_driver_data pdrvdata = { .next_vtermno = 1}; >>>> + >>>> +static struct ports_driver_data pdrvdata = { .next_vtermno = ATOMIC_INIT(0) }; >>>> static DEFINE_SPINLOCK(pdrvdata_lock); >>>> static DECLARE_COMPLETION(early_console_added); >>>> @@ -1244,7 +1245,7 @@ static int init_port_console(struct port *port) >>>> * pointers. The final argument is the output buffer size: we >>>> * can do any size, so we put PAGE_SIZE here. >>>> */ >>>> - port->cons.vtermno = pdrvdata.next_vtermno; >>>> + port->cons.vtermno = atomic_inc_return(&pdrvdata.next_vtermno); >>> >>> Why not use a normal ida/idr structure here? >> >> yes that works. >> >>> And why is this never decremented? >> >> The driver would then need to track the id allocation ... > > That's what an ida/idr does. > >>> and finally, why not use the value that created the "vportN" number >>> instead? >> >> yes. we could also encode the tuple (vdev->index, port) using a bitmask, > > No need for that, you already have a unique number in the name above, > why not use that? > >> possibly using 'max_nr_ports' to reduce the port width. > > Why is that an issue? Maybe I am confused as to what this magic > "vtermno" is here. Who uses it and why is the vportN number not > sufficient?
A virtio console device can have multiple ports each being a /dev/hvcX exposed in the guest OS. The "vportN" prefix identifies the virtio device :
# grep vtermno /sys/kernel/debug/virtio-ports/* /sys/kernel/debug/virtio-ports/vport1p0:console_vtermno: 2 /sys/kernel/debug/virtio-ports/vport1p1:console_vtermno: 3 /sys/kernel/debug/virtio-ports/vport1p2:console_vtermno: 4 /sys/kernel/debug/virtio-ports/vport1p3:console_vtermno: 5 /sys/kernel/debug/virtio-ports/vport2p0:console_vtermno: 1 /sys/kernel/debug/virtio-ports/vport2p1:console_vtermno: 6 /sys/kernel/debug/virtio-ports/vport2p2:console_vtermno: 7 /sys/kernel/debug/virtio-ports/vport2p3:console_vtermno: 8
and "pX" the port within in the device. The naming is a bit confusing.
>> VIRTCONS_MAX_PORTS >> seems a bit big for this device and QEMU sets the #ports to 31. >> >> An ida might be simpler. One drawback is that an id can be reused for a >> different device/port tuple in case of an (unlikely) unplug/plug sequence. > > What's wrong with that? We do not have persistent device names from > within the kernel.
Let's go with the ida then.
Thanks, C.
| |