diff mbox series

virtio_console: Use an atomic to allocate virtual console numbers

Message ID 20221114080752.1900699-1-clg@kaod.org
State New
Headers
Series virtio_console: Use an atomic to allocate virtual console numbers |

Commit Message

Cédric Le Goater Nov. 14, 2022, 8:07 a.m. UTC 
  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(-)

Comments

Greg KH Nov. 14, 2022, 8:57 a.m. UTC | #1 
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?

And why is this never decremented?

and finally, why not use the value that created the "vportN" number
instead?

thanks,

greg k-h
Cédric Le Goater Nov. 14, 2022, 4:03 p.m. UTC | #2 
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 ...

> 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,
possibly using 'max_nr_ports' to reduce the port width. 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.

Thanks,
C.

> 
> thanks,
> 
> greg k-h
Greg KH Nov. 14, 2022, 4:18 p.m. UTC | #3 
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?

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

thanks,

greg k-h
Cédric Le Goater Nov. 14, 2022, 5 p.m. UTC | #4 
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.
diff mbox series

Patch

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);
 
 	port->cons.hvc = hvc_alloc(port->cons.vtermno, 0, &hv_ops, PAGE_SIZE);
 	if (IS_ERR(port->cons.hvc)) {
@@ -1255,7 +1256,6 @@  static int init_port_console(struct port *port)
 		return ret;
 	}
 	spin_lock_irq(&pdrvdata_lock);
-	pdrvdata.next_vtermno++;
 	list_add_tail(&port->cons.list, &pdrvdata.consoles);
 	spin_unlock_irq(&pdrvdata_lock);
 	port->guest_connected = true;