[v2] virtio_console: Introduce an ID allocator for virtual console numbers
Commit Message
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.
Signed-off-by: Cédric Le Goater <clg@kaod.org>
---
drivers/char/virtio_console.c | 25 ++++++++++---------------
1 file changed, 10 insertions(+), 15 deletions(-)
Comments
On 14/11/2022 18.38, 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.
Sounds like a good idea!
> @@ -1244,8 +1236,11 @@ 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;
> + ret = ida_alloc_range(&vtermno_ida, 1, ~0, GFP_KERNEL);
Just cosmetics: I think you could use ida_alloc_min() instead.
> + if (ret < 0)
> + return ret;
>
> + port->cons.vtermno = ret;
> port->cons.hvc = hvc_alloc(port->cons.vtermno, 0, &hv_ops, PAGE_SIZE);
> if (IS_ERR(port->cons.hvc)) {
> ret = PTR_ERR(port->cons.hvc);
What if this if (IS_ERR()) error happens? The code seems to return early in
this case - shouldn't the ID be freed again in this case?
Thomas
On 11/22/22 12:12, Thomas Huth wrote:
> On 14/11/2022 18.38, 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.
>
> Sounds like a good idea!
>
>> @@ -1244,8 +1236,11 @@ 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;
>> + ret = ida_alloc_range(&vtermno_ida, 1, ~0, GFP_KERNEL);
>
> Just cosmetics: I think you could use ida_alloc_min() instead.
>
>> + if (ret < 0)
>> + return ret;
>> + port->cons.vtermno = ret;
>> port->cons.hvc = hvc_alloc(port->cons.vtermno, 0, &hv_ops, PAGE_SIZE);
>> if (IS_ERR(port->cons.hvc)) {
>> ret = PTR_ERR(port->cons.hvc);
>
> What if this if (IS_ERR()) error happens? The code seems to return early in this case - shouldn't the ID be freed again in this case?
Oh yes. I forgot that.
Thanks,
C.
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...
Amit
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.
On Wed, 2022-11-23 at 11:17 +0100, Cédric Le Goater wrote:
> 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 ?
No, you're right about this being kernel-internal -- just that it's
used with hvc instead of qemu like I mentioned.
I think this is the right change; just want to confirm hvc didn't get
confused.
>
> 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.
I think the bug is that we don't increment the vtermno today in all
places that we should; but this patch solves it too - I don't mind
adding the extra ida bits.
@@ -13,6 +13,7 @@
#include <linux/fs.h>
#include <linux/splice.h>
#include <linux/pagemap.h>
+#include <linux/idr.h>
#include <linux/init.h>
#include <linux/list.h>
#include <linux/poll.h>
@@ -48,22 +49,11 @@ struct ports_driver_data {
/* List of all the devices we're handling */
struct list_head portdevs;
- /*
- * This is used to keep track of the number of hvc consoles
- * spawned by this driver. This number is given as the first
- * argument to hvc_alloc(). To correctly map an initial
- * console spawned via hvc_instantiate to the console being
- * hooked up via hvc_alloc, we need to pass the same vtermno.
- *
- * We also just assume the first console being initialised was
- * the first one that got used as the initial console.
- */
- unsigned int 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;
static DEFINE_SPINLOCK(pdrvdata_lock);
static DECLARE_COMPLETION(early_console_added);
@@ -89,6 +79,8 @@ struct console {
u32 vtermno;
};
+static DEFINE_IDA(vtermno_ida);
+
struct port_buffer {
char *buf;
@@ -1244,8 +1236,11 @@ 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;
+ ret = ida_alloc_range(&vtermno_ida, 1, ~0, GFP_KERNEL);
+ if (ret < 0)
+ return ret;
+ port->cons.vtermno = ret;
port->cons.hvc = hvc_alloc(port->cons.vtermno, 0, &hv_ops, PAGE_SIZE);
if (IS_ERR(port->cons.hvc)) {
ret = PTR_ERR(port->cons.hvc);
@@ -1255,7 +1250,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;
@@ -1532,6 +1526,7 @@ static void unplug_port(struct port *port)
list_del(&port->cons.list);
spin_unlock_irq(&pdrvdata_lock);
hvc_remove(port->cons.hvc);
+ ida_free(&vtermno_ida, port->cons.vtermno);
}
remove_port_data(port);