Re: [PATCH 09/28] virtio: console: struct ports for multiple ports per device.

[Amit Shah]

On (Mon) Nov 30 2009 [12:39:52], Rusty Russell wrote:
> On Sat, 28 Nov 2009 05:20:32 pm Amit Shah wrote:
> > @@ -196,7 +216,9 @@ static void virtcons_apply_config(struct virtio_device 
> > *dev)
> > dev->config->get(dev,
> >  offsetof(struct virtio_console_config, rows),
> >  &ws.ws_row, sizeof(u16));
> > -   hvc_resize(port->hvc, ws);
> > +   /* This is the pre-multiport style: we use control messages
> > +* these days which specify the port.  So this means port 0. */
> > +   hvc_resize(find_port_by_vtermno(0)->hvc, ws);
> > }
> 
> We end up doing this in a couple of places; perhaps we should have something
> like:
>   /*
>* Before multiple console support, we always had a single console.
>* Code paths without those features can use this.
>*/
>   static struct port *old_style_unique_console(void)
>   {
>   return find_port_by_vtermno(0);
>   }

(Again) in a later patch, I rename the function to resize_console() and
call that one from the config interrupt as well as from the
hvc notifier.

> > err = -ENOMEM;
> > -   port = add_port(pdrvdata.next_vtermno);
> > +   port = kzalloc(sizeof *port, GFP_KERNEL);
> > if (!port)
> > goto fail;
> 
> I still dislike kzalloc.  I think I've said this before :)

I remember seeing it in another thread I think (or was it in a blog post
about you liking the ability to run it through valgrind?).

The init function becomes a bit longer in that case, since we'll have to
init all the known state. But I'm fine with that.

However, there's one exception: when allocating buffers, I'd prefer to
do a kzalloc() since the buffers are sent across the guest/host boundary
and we don't want to leak data in either direction.

(Just mentioning this again since it's the last comment: I'll respin the
series once you have a chance to review the other patches too.)

Also: if you think the approach is OK and give an ACK for the approach,
I can also proceed in parallel with the host bits for QEMU.

Thanks, Rusty!

Amit
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linux-foundation.org/mailman/listinfo/virtualization

Re: [PATCH 09/28] virtio: console: struct ports for multiple ports per device.

[Rusty Russell]

On Sat, 28 Nov 2009 05:20:32 pm Amit Shah wrote:
> @@ -196,7 +216,9 @@ static void virtcons_apply_config(struct virtio_device 
> *dev)
>   dev->config->get(dev,
>offsetof(struct virtio_console_config, rows),
>&ws.ws_row, sizeof(u16));
> - hvc_resize(port->hvc, ws);
> + /* This is the pre-multiport style: we use control messages
> +  * these days which specify the port.  So this means port 0. */
> + hvc_resize(find_port_by_vtermno(0)->hvc, ws);
>   }

We end up doing this in a couple of places; perhaps we should have something
like:
/*
 * Before multiple console support, we always had a single console.
 * Code paths without those features can use this.
 */
static struct port *old_style_unique_console(void)
{
return find_port_by_vtermno(0);
}

>   err = -ENOMEM;
> - port = add_port(pdrvdata.next_vtermno);
> + port = kzalloc(sizeof *port, GFP_KERNEL);
>   if (!port)
>   goto fail;

I still dislike kzalloc.  I think I've said this before :)

Thanks,
Rusty.
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linux-foundation.org/mailman/listinfo/virtualization

[PATCH 09/28] virtio: console: struct ports for multiple ports per device.

[Amit Shah]

Rather than assume a single port, add a 'struct ports_device' which
stores data related to all the ports for that device.

Currently, there's only one port and is hooked up with hvc, but that
will change.

Signed-off-by: Amit Shah 
---
 drivers/char/virtio_console.c |  154 +++-
 1 files changed, 88 insertions(+), 66 deletions(-)

diff --git a/drivers/char/virtio_console.c b/drivers/char/virtio_console.c
index 98a5249..d4e2327 100644
--- a/drivers/char/virtio_console.c
+++ b/drivers/char/virtio_console.c
@@ -51,9 +51,20 @@ static struct ports_driver_data pdrvdata;
 
 DEFINE_SPINLOCK(pdrvdata_lock);
 
-struct port {
+/*
+ * This is a per-device struct that stores data common to all the
+ * ports for that device (vdev->priv).
+ */
+struct ports_device {
struct virtqueue *in_vq, *out_vq;
struct virtio_device *vdev;
+};
+
+/* This struct holds the per-port data */
+struct port {
+   /* Pointer to the parent virtio_console device */
+   struct ports_device *portdev;
+
/* This is our input buffer, and how much data is left in it. */
char *inbuf;
unsigned int used_len, offset;
@@ -100,6 +111,7 @@ static int put_chars(u32 vtermno, const char *buf, int 
count)
 {
struct scatterlist sg[1];
struct port *port;
+   struct virtqueue *out_vq;
unsigned int len;
 
port = find_port_by_vtermno(vtermno);
@@ -109,14 +121,15 @@ static int put_chars(u32 vtermno, const char *buf, int 
count)
if (unlikely(early_put_chars))
return early_put_chars(vtermno, buf, count);
 
+   out_vq = port->portdev->out_vq;
/* This is a convenient routine to initialize a single-elem sg list */
sg_init_one(sg, buf, count);
 
/* This shouldn't fail: if it does, we lose chars. */
-   if (port->out_vq->vq_ops->add_buf(port->out_vq, sg, 1, 0, port) >= 0) {
+   if (out_vq->vq_ops->add_buf(out_vq, sg, 1, 0, port) >= 0) {
/* Tell Host to go! */
-   port->out_vq->vq_ops->kick(port->out_vq);
-   while (!port->out_vq->vq_ops->get_buf(port->out_vq, &len))
+   out_vq->vq_ops->kick(out_vq);
+   while (!out_vq->vq_ops->get_buf(out_vq, &len))
cpu_relax();
}
 
@@ -130,13 +143,16 @@ static int put_chars(u32 vtermno, const char *buf, int 
count)
  */
 static void add_inbuf(struct port *port)
 {
+   struct virtqueue *in_vq;
struct scatterlist sg[1];
+
sg_init_one(sg, port->inbuf, PAGE_SIZE);
 
+   in_vq = port->portdev->in_vq;
/* Should always be able to add one buffer to an empty queue. */
-   if (port->in_vq->vq_ops->add_buf(port->in_vq, sg, 0, 1, port) < 0)
+   if (in_vq->vq_ops->add_buf(in_vq, sg, 0, 1, port) < 0)
BUG();
-   port->in_vq->vq_ops->kick(port->in_vq);
+   in_vq->vq_ops->kick(in_vq);
 }
 
 /*
@@ -150,18 +166,23 @@ static void add_inbuf(struct port *port)
 static int get_chars(u32 vtermno, char *buf, int count)
 {
struct port *port;
+   struct virtqueue *in_vq;
 
port = find_port_by_vtermno(vtermno);
if (!port)
return 0;
 
+   in_vq = port->portdev->in_vq;
/* If we don't have an input queue yet, we can't get input. */
-   BUG_ON(!port->in_vq);
+   BUG_ON(!in_vq);
 
/* No more in buffer?  See if they've (re)used it. */
if (port->offset == port->used_len) {
-   if (!port->in_vq->vq_ops->get_buf(port->in_vq, &port->used_len))
+   unsigned int len;
+
+   if (!in_vq->vq_ops->get_buf(in_vq, &len))
return 0;
+   port->used_len = len;
port->offset = 0;
}
 
@@ -186,7 +207,6 @@ static int get_chars(u32 vtermno, char *buf, int count)
  */
 static void virtcons_apply_config(struct virtio_device *dev)
 {
-   struct port *port = dev->priv;
struct winsize ws;
 
if (virtio_has_feature(dev, VIRTIO_CONSOLE_F_SIZE)) {
@@ -196,7 +216,9 @@ static void virtcons_apply_config(struct virtio_device *dev)
dev->config->get(dev,
 offsetof(struct virtio_console_config, rows),
 &ws.ws_row, sizeof(u16));
-   hvc_resize(port->hvc, ws);
+   /* This is the pre-multiport style: we use control messages
+* these days which specify the port.  So this means port 0. */
+   hvc_resize(find_port_by_vtermno(0)->hvc, ws);
}
 }
 
@@ -210,7 +232,7 @@ static int notifier_add_vio(struct hvc_struct *hp, int data)
return -EINVAL;
 
hp->irq_requested = 1;
-   virtcons_apply_config(port->vdev);
+   virtcons_apply_config(port->portdev->vdev);
 
return 0;
 }
@@ -222,9 +244,13 @@ static void notifier_del_vio(struct hvc_struct *hp, int 
data)
 
 static void hvc_handle_input(struct virtqueue *vq)
 {
-   str