virtio_console: Add support for multiple ports for generic guest and host communication
in [Kernel]
|
Prev: [2.6.31] Memory leak in load_module()?
Next: virtio_console: Add support for multiple ports for generic guest and host communication
From: Amit Shah on 1 Oct 2009 04:50 On (Thu) Oct 01 2009 [10:17:29], Christian Borntraeger wrote: > Am Mittwoch 30 September 2009 19:13:21 schrieb Amit Shah: > > On (Tue) Sep 29 2009 [15:31:23], Christian Borntraeger wrote: > > > Am Dienstag 29 September 2009 15:09:50 schrieb Amit Shah: > > > > Great, thanks. However I was thinking of moving this init to the > > > > probe() routine instead of in the init_conosle routine just because > > > > multiple consoles can be added and we don't want to init this each > > > > time.. just once in probe is fine. > > > > > > If you have new patch CC me and I can give it a spin. > > > > Hey Christian, > > > > I have a new patch that changes a few things: > > - moves the put_char fix to probe instead of doing it in > > init_port_console(), which gets called on each console port found. > > - uses port->id instead of a static hvc_vtermno to pass on a value to > > hvc_alloc(). Motivation explained within comments in the code. > > - A few other changes that introduce and make use of port->vcon instead > > of accessing the static virtconsole directly -- aimed at easing future > > fix to have multiple virtio-console devices. > > > > It would be great if you could test this. > > The basic stuff seems to work, but when I did a console resize I get: .... > One of the config->get() related changes is probably wrong. I see it. The following should fix it. Amit diff --git a/drivers/char/virtio_console.c b/drivers/char/virtio_console.c index 30361c1..b2de2df 100644 --- a/drivers/char/virtio_console.c +++ b/drivers/char/virtio_console.c @@ -822,9 +822,10 @@ static void tx_intr(struct virtqueue *vq) static void config_intr(struct virtio_device *vdev) { - /* Handle port hot-add */ - schedule_work(&virtconsole.config_work); - + if (use_multiport()) { + /* Handle port hot-add */ + schedule_work(&virtconsole.config_work); + } /* Handle console size changes */ virtcons_apply_config(vdev); } -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo(a)vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
From: Amit Shah on 1 Oct 2009 06:10 On (Thu) Oct 01 2009 [11:00:48], Christian Borntraeger wrote: > Am Mittwoch 30 September 2009 19:13:21 schrieb Amit Shah: > > - uses port->id instead of a static hvc_vtermno to pass on a value to > > hvc_alloc(). Motivation explained within comments in the code. > > [...] > > + * The first argument of hvc_alloc() is the virtual console > > + * number. hvc_alloc() checks if a console with the same value > > + * was used in hvc_instantiate(). We may not end up passing > > + * the same value here (we use the value 0 in > > + * hvc_instantiate() but the console port may not be at id > > + * 0). This isn't a problem, though. Nothing in hvc really > > + * depends on these numbers and since this number is passed on > > + * to us when cons_get/put_chars() is called, it's preferable > > + * to pass on the port->id so that we can get the port struct > > + * via get_port_from_id(). > [...] > > + port->hvc = hvc_alloc(port->id, 0, &virtio_cons, PAGE_SIZE); > > I am not sure if this is going to be ok. > > When I change the old code (without your patch) to use vtermno=0 in > hvc_instantiate and vtermno=1 in hvc_alloc I get > "Warning: unable to open an initial console." > > It seems to me that we have to pass the same values. OK; that's bad enough. The following diff reverts that patch and adds a new get_port_from_vtermno() that works from hvc callbacks. What do you think? (Applies on top of the latest patch I sent.) Amit diff --git a/drivers/char/virtio_console.c b/drivers/char/virtio_console.c index b2de2df..a8ba5aa 100644 --- a/drivers/char/virtio_console.c +++ b/drivers/char/virtio_console.c @@ -105,12 +105,42 @@ struct virtio_console_port { /* The 'id' to identify the port with the Host */ u32 id; + /* + * If this port is a console port, this number identifies the + * number that we used to register with hvc in + * hvc_instantiate() and hvc_alloc(). + */ + u32 vtermno; + /* Is the host device open */ bool host_connected; }; static struct virtio_console_struct virtconsole; +/* + * This is used to keep track of the number of hvc consoles spawned. + * This number is given as first argument to hvc_alloc(). We could as + * well pass on the minor number of the char device but 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. + * + * With this int, we just assume the first console being initialised + * was the first one that got used as the initial console. + */ +static unsigned int hvc_vtermno; + +static struct virtio_console_port *get_port_from_vtermno(u32 vtermno) +{ + struct virtio_console_port *port; + + list_for_each_entry(port, &virtconsole.port_head, next) { + if (port->vtermno == vtermno) + return port; + } + return NULL; +} + static struct virtio_console_port *get_port_from_devt(dev_t devt) { struct virtio_console_port *port; @@ -457,7 +487,7 @@ static int cons_put_chars(u32 vtermno, const char *buf, int count) { struct virtio_console_port *port; - port = get_port_from_id(vtermno); + port = get_port_from_vtermno(vtermno); if (!port) return 0; @@ -478,7 +508,7 @@ static int cons_get_chars(u32 vtermno, char *buf, int count) /* If we don't have an input queue yet, we can't get input. */ BUG_ON(!virtconsole.in_vq); - port = get_port_from_id(vtermno); + port = get_port_from_vtermno(vtermno); if (!port) return 0; @@ -566,32 +596,23 @@ int init_port_console(struct virtio_console_port *port) * hvc_alloc(). * * The first argument of hvc_alloc() is the virtual console - * number. hvc_alloc() checks if a console with the same value - * was used in hvc_instantiate(). We may not end up passing - * the same value here (we use the value 0 in - * hvc_instantiate() but the console port may not be at id - * 0). This isn't a problem, though. Nothing in hvc really - * depends on these numbers and since this number is passed on - * to us when cons_get/put_chars() is called, it's preferable - * to pass on the port->id so that we can get the port struct - * via get_port_from_id(). - * - * The second argument is the parameter for the notification - * mechanism (like irq number). We currently leave this as - * zero, virtqueues have implicit notifications. + * number. The second argument is the parameter for the + * notification mechanism (like irq number). We currently + * leave this as zero, virtqueues have implicit notifications. * * The third argument is a "struct hv_ops" containing the * put_chars() get_chars(), notifier_add() and notifier_del() * pointers. The final argument is the output buffer size: we * can do any size, so we put PAGE_SIZE here. */ - port->hvc = hvc_alloc(port->id, 0, &virtio_cons, PAGE_SIZE); + port->hvc = hvc_alloc(hvc_vtermno, 0, &virtio_cons, PAGE_SIZE); if (IS_ERR(port->hvc)) { ret = PTR_ERR(port->hvc); pr_err("%s: Could not alloc hvc for virtio console port, ret = %d\n", __func__, ret); port->hvc = NULL; } + port->vtermno = hvc_vtermno++; return ret; } -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo(a)vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
From: Amit Shah on 1 Oct 2009 06:50 On (Thu) Oct 01 2009 [12:28:30], Christian Borntraeger wrote: > Am Mittwoch 30 September 2009 19:13:21 schrieben Sie: > > On (Tue) Sep 29 2009 [15:31:23], Christian Borntraeger wrote: > > > Am Dienstag 29 September 2009 15:09:50 schrieb Amit Shah: > > > > Great, thanks. However I was thinking of moving this init to the > > > > probe() routine instead of in the init_conosle routine just because > > > > multiple consoles can be added and we don't want to init this each > > > > time.. just once in probe is fine. > > > > > > If you have new patch CC me and I can give it a spin. > > > > Hey Christian, > > > > I have a new patch that changes a few things: > > - moves the put_char fix to probe instead of doing it in > > init_port_console(), which gets called on each console port found. > > - uses port->id instead of a static hvc_vtermno to pass on a value to > > hvc_alloc(). Motivation explained within comments in the code. > > - A few other changes that introduce and make use of port->vcon instead > > of accessing the static virtconsole directly -- aimed at easing future > > fix to have multiple virtio-console devices. > > > > It would be great if you could test this. > > With the latest git kernel + your patch I sometmes get a completely frozen > console. In the dump there is > > <3>virtio_console virtio0: output:id 68 is not a head! > > Seems that I can reproduce it with large amounts of output (find / for example) > Without your patch everything is fine. Do you mean the entire patch? Or just the last snippet that I sent? I'm testing things using a few days old linux-next; I guess you're seeing this with Linus' git tree? I'll try that. Amit -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo(a)vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
From: Amit Shah on 1 Oct 2009 08:00 On (Thu) Oct 01 2009 [12:28:30], Christian Borntraeger wrote: > Am Mittwoch 30 September 2009 19:13:21 schrieben Sie: > > On (Tue) Sep 29 2009 [15:31:23], Christian Borntraeger wrote: > > > Am Dienstag 29 September 2009 15:09:50 schrieb Amit Shah: > > > > Great, thanks. However I was thinking of moving this init to the > > > > probe() routine instead of in the init_conosle routine just because > > > > multiple consoles can be added and we don't want to init this each > > > > time.. just once in probe is fine. > > > > > > If you have new patch CC me and I can give it a spin. > > > > Hey Christian, > > > > I have a new patch that changes a few things: > > - moves the put_char fix to probe instead of doing it in > > init_port_console(), which gets called on each console port found. > > - uses port->id instead of a static hvc_vtermno to pass on a value to > > hvc_alloc(). Motivation explained within comments in the code. > > - A few other changes that introduce and make use of port->vcon instead > > of accessing the static virtconsole directly -- aimed at easing future > > fix to have multiple virtio-console devices. > > > > It would be great if you could test this. > > With the latest git kernel + your patch I sometmes get a completely frozen > console. In the dump there is > > <3>virtio_console virtio0: output:id 68 is not a head! > > Seems that I can reproduce it with large amounts of output (find / for example) > Without your patch everything is fine. I spawned two ports; one doing 'find /' and the other transferring a 800M file from the host to the guest, both running at the same time. This is on top of the latest Linus tree + my patches. It works fine. Can you give me any additional info that can help me nail this down? Amit -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo(a)vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
From: Amit Shah on 1 Oct 2009 10:00
On (Thu) Oct 01 2009 [14:04:37], Christian Borntraeger wrote: > Am Donnerstag 01 Oktober 2009 13:58:01 schrieben Sie: > > I spawned two ports; one doing 'find /' and the other transferring a > > 800M file from the host to the guest, both running at the same time. > > This is on top of the latest Linus tree + my patches. It works fine. > > > > Can you give me any additional info that can help me nail this down? > > This is just a wild guess: My guest has 8 cpus and virtio requires that users do > a proper locking. But as far as I see you use schedule_work which should be > single threaded. > Or maybe this is an existing bug that is just exposed by your patch - dont know. I think there might be two bugs. One is the readbuf_head list needs some locks around its operations. The readbuf_head list is a per-port list where buffers received from the host for a port are stored. They're taken off that list when userspace issues read() requests. The other bug is the virtio one that you saw. I'll try to see how that one gets triggered. Thanks, Amit -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo(a)vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/ |