On Tue, 28 Jul 2009 03:34:33 am Amit Shah wrote:
> We expose multiple char devices ("ports") for simple communication
> between the host userspace and guest.
Hi Amit,
OK, seems like it's time for some serious review. Below.
> +config VIRTIO_SERIAL
> + tristate "Virtio serial"
> + select VIRTIO
> + select VIRTIO_RING
> + help
> + Virtio serial device driver for simple guest and host communication
depends on VIRTIO
Do not "select VIRTIO_RING" -- this code doesn't explicitly rely on it.
> +struct virtio_serial_struct {
> + struct work_struct rx_work;
> + struct work_struct tx_work;
> + struct work_struct queue_work;
> + struct work_struct config_work;
> +
> + struct list_head port_head;
> +
> + struct virtio_device *vdev;
> + struct class *class;
> + struct virtqueue *in_vq, *out_vq;
> +
> + struct virtio_serial_config *config;
> +};
> +
> +/* This struct holds individual buffers received for each port */
> +struct virtio_serial_port_buffer {
> + struct list_head list;
> +
> + unsigned int len; /* length of the buffer */
> + unsigned int offset; /* offset in the buf from which to consume data */
> +
> + char *buf;
> +};
> +
> +/* This struct is put in each buffer that gets passed to userspace and
> + * vice-versa
> + */
> +struct virtio_serial_id {
> + u32 id; /* Port number */
> +};
> +
> +struct virtio_serial_port {
> + /* Next port in the list */
> + struct list_head next;
> +
> + /* Buffer management */
> + struct virtio_serial_port_buffer read_buf;
> + struct list_head readbuf_head;
> + struct completion have_data;
> +
> + /* Each port associates with a separate char device */
> + struct cdev cdev;
> + struct device *dev;
> +};
> +
> +static struct virtio_serial_struct virtserial;
> +
> +static int major = 60; /* from the experimental range */
This will obviously need to change before it goes in.
> +
> +static struct virtio_serial_port *get_port_from_id(u32 id)
> +{
> + struct virtio_serial_port *port;
> + struct list_head *ptr;
> +
> + list_for_each(ptr, &virtserial.port_head) {
> + port = list_entry(ptr, struct virtio_serial_port, next);
list_for_each_entry, same with the others.
> +static int get_id_from_port(struct virtio_serial_port *port)
> +{
> + struct virtio_serial_port *match;
> + struct list_head *ptr;
> +
> + list_for_each(ptr, &virtserial.port_head) {
> + match = list_entry(ptr, struct virtio_serial_port, next);
> +
> + if (match == port)
> + return MINOR(port->dev->devt);
> + }
> + return VIRTIO_SERIAL_BAD_ID;
> +}
Why does this exist? Seems weird to loop, given you have the pointer already?
> +
> +static struct virtio_serial_port *get_port_from_buf(char *buf)
> +{
> + u32 id;
> +
> + memcpy(&id, buf, sizeof(id));
> +
> + return get_port_from_id(id);
> +}
> +
> +
> +static ssize_t virtserial_read(struct file *filp, char __user *ubuf,
> + size_t count, loff_t *offp)
> +{
> + struct list_head *ptr, *ptr2;
> + struct virtio_serial_port *port;
> + struct virtio_serial_port_buffer *buf;
> + ssize_t ret;
> +
> + port = filp->private_data;
> +
> + ret = -EINTR;
> + if (list_empty(&port->readbuf_head)) {
> + if (filp->f_flags & O_NONBLOCK)
> + return -EAGAIN;
> +
> + if (wait_for_completion_interruptible(&port->have_data) < 0)
> + return ret;
> + }
I don't think this code works. What protects from two simultaneous readers?
Who resets the completion? It's more normal to use a waitqueue and
wait_event_interruptible().
> + list_for_each_safe(ptr, ptr2, &port->readbuf_head) {
> + buf = list_entry(ptr, struct virtio_serial_port_buffer, list);
> +
> + /* FIXME: other buffers further in this list might
> + * have data too
> + */
> + if (count > buf->len - buf->offset)
> + count = buf->len - buf->offset;
> +
> + ret = copy_to_user(ubuf, buf->buf + buf->offset, count);
> +
> + /* Return the number of bytes actually copied */
> + ret = count - ret;
> +
> + buf->offset += ret;
> +
> + if (buf->len - buf->offset == 0) {
> + list_del(&buf->list);
> + kfree(buf->buf);
> + kfree(buf);
> + }
> + /* FIXME: if there's more data requested and more data
> + * available, return it.
> + */
> + break;
There's nothing wrong with short reads here.
> + }
> + return ret;
This can return -EINTR; if you hadn't assigned ret = -EINTR unconditionally
above, gcc would have given you a warning about that path :)
> +}
> +
> +/* For data that exceeds PAGE_SIZE in size we should send it all in
> + * one sg to not unnecessarily split up the data. Also some (all?)
> + * vnc clients don't consume split data.
> + *
> + * If we are to keep PAGE_SIZE sized buffers, we then have to stack
> + * multiple of those in one virtio request. virtio-ring returns to us
> + * just one pointer for all the buffers. So use this struct to
> + * allocate the bufs in so that freeing this up later is easier.
> + */
> +struct vbuf {
> + char **bufs;
> + struct scatterlist *sg;
> + unsigned int nent;
> +};
> +
> +static ssize_t virtserial_write(struct file *filp, const char __user *ubuf,
> + size_t count, loff_t *offp)
> +{
> + struct virtqueue *out_vq;
> + struct virtio_serial_port *port;
> + struct virtio_serial_id id;
> + struct vbuf *vbuf;
> + size_t offset, size;
> + ssize_t ret;
> + int i, id_len;
> +
> + port = filp->private_data;
> + id.id = get_id_from_port(port);
> + out_vq = virtserial.out_vq;
> +
> + id_len = sizeof(id);
> +
> + ret = -EFBIG;
> + vbuf = kzalloc(sizeof(struct vbuf), GFP_KERNEL);
> + if (!vbuf)
> + return ret;
-ENOMEM is normal here.
> +
> + /* Max. number of buffers clubbed together in one message */
> + vbuf->nent = (count + id_len + PAGE_SIZE - 1) / PAGE_SIZE;
> +
> + vbuf->bufs = kzalloc(vbuf->nent, GFP_KERNEL);
> + if (!vbuf->bufs)
> + goto free_vbuf;
> +
> + vbuf->sg = kzalloc(vbuf->nent, GFP_KERNEL);
> + if (!vbuf->sg)
> + goto free_bufs;
> + sg_init_table(vbuf->sg, vbuf->nent);
> +
> + i = 0; /* vbuf->bufs[i] */
> + offset = 0; /* offset in the user buffer */
> + while (count - offset) {
> + size = min(count - offset + id_len, PAGE_SIZE);
> + vbuf->bufs[i] = kzalloc(size, GFP_KERNEL);
> + if (!vbuf->bufs[i]) {
> + ret = -EFBIG;
> + goto free_buffers;
> + }
> + if (id_len) {
> + memcpy(vbuf->bufs[i], &id, id_len);
> + size -= id_len;
> + }
> + ret = copy_from_user(vbuf->bufs[i] + id_len, ubuf + offset, size);
> + offset += size - ret;
> +
> + sg_set_buf(&vbuf->sg[i], vbuf->bufs[i], size - ret + id_len);
> + id_len = 0; /* Pass the port id only in the first buffer */
> + i++;
> + }
> + if (out_vq->vq_ops->add_buf(out_vq, vbuf->sg, i, 0, vbuf)) {
> + /* XXX: We can't send the buffer. Report failure */
> + ret = 0;
> + }
> + /* Tell Host to go! */
> + out_vq->vq_ops->kick(out_vq);
> +
> + /* We're expected to return the amount of data we wrote */
> + return offset;
> +free_buffers:
> + while (i--)
> + kfree(vbuf->bufs[i]);
> + kfree(vbuf->sg);
> +free_bufs:
> + kfree(vbuf->bufs);
> +free_vbuf:
> + kfree(vbuf);
> + return ret;
> +}
> +
> +static long virtserial_ioctl(struct file *filp, unsigned int ioctl,
> + unsigned long arg)
> +{
> + struct virtio_serial_port *port;
> + long ret;
> +
> + port = filp->private_data;
> +
> + ret = -EINVAL;
> + switch (ioctl) {
> + default:
> + break;
> + }
> + return ret;
> +}
I thought -ENOTTY was normal for invalid ioctls? In which case, just don't
implement this function.
> +
> +static int virtserial_release(struct inode *inode, struct file *filp)
> +{
> + filp->private_data = NULL;
> + return 0;
> +}
This seems redundant.
> +
> +static int virtserial_open(struct inode *inode, struct file *filp)
> +{
> + struct cdev *cdev = inode->i_cdev;
> + struct virtio_serial_port *port;
> +
> + port = container_of(cdev, struct virtio_serial_port, cdev);
> +
> + filp->private_data = port;
> + return 0;
> +}
> +
> +static unsigned int virtserial_poll(struct file *filp, poll_table *wait)
> +{
> + pr_notice("%s\n", __func__);
> + return 0;
> +}
And you're going to want to implement this, too.
> +
> +static const struct file_operations virtserial_fops = {
> + .owner = THIS_MODULE,
> + .open = virtserial_open,
> + .read = virtserial_read,
> + .write = virtserial_write,
> + .compat_ioctl = virtserial_ioctl,
> + .unlocked_ioctl = virtserial_ioctl,
> + .poll = virtserial_poll,
> + .release = virtserial_release,
> +};
> +
> +static void virtio_serial_queue_work_handler(struct work_struct *work)
> +{
> + struct scatterlist sg[1];
> + struct virtqueue *vq;
> + char *buf;
> +
> + vq = virtserial.in_vq;
> + while (1) {
> + buf = kzalloc(PAGE_SIZE, GFP_KERNEL);
> + if (!buf)
> + break;
> +
> + sg_init_one(sg, buf, PAGE_SIZE);
> +
> + if (vq->vq_ops->add_buf(vq, sg, 0, 1, buf) < 0) {
> + kfree(buf);
> + break;
> + }
> + }
> + vq->vq_ops->kick(vq);
> +}
> +
> +static void virtio_serial_rx_work_handler(struct work_struct *work)
> +{
> + struct virtio_serial_port *port = NULL;
> + struct virtio_serial_port_buffer *buf;
> + struct virtqueue *vq;
> + char *tmpbuf;
> + unsigned int tmplen;
> +
> + vq = virtserial.in_vq;
> + while ((tmpbuf = vq->vq_ops->get_buf(vq, &tmplen))) {
> + port = get_port_from_buf(tmpbuf);
> + if (!port) {
> + /* No valid index at start of
> + * buffer. Drop it.
> + */
> + pr_debug("%s: invalid index in buffer, %c %d\n",
> + __func__, tmpbuf[0], tmpbuf[0]);
> + break;
leak?
> + }
> + buf = kzalloc(sizeof(struct virtio_serial_port_buffer),
> + GFP_KERNEL);
> + if (!buf)
> + break;
> +
> + buf->buf = tmpbuf;
> + buf->len = tmplen;
> + buf->offset = sizeof(struct virtio_serial_id);
> + list_add_tail(&buf->list, &port->readbuf_head);
> +
> + complete(&port->have_data);
> + }
> + /* Allocate buffers for all the ones that got used up */
> + schedule_work(&virtserial.queue_work);
> +}
Why do the allocation in a separate workqueue?
> +
> +static void virtio_serial_tx_work_handler(struct work_struct *work)
> +{
> + struct virtqueue *vq;
> + struct vbuf *vbuf;
> + unsigned int tmplen;
> + int i;
> +
> + vq = virtserial.out_vq;
> + while ((vbuf = vq->vq_ops->get_buf(vq, &tmplen))) {
> + for (i = 0; i < vbuf->nent; i++) {
> + kfree(vbuf->bufs[i]);
> + }
> + kfree(vbuf->bufs);
> + kfree(vbuf->sg);
> + kfree(vbuf);
> + }
> +}
> +
> +static void rx_intr(struct virtqueue *vq)
> +{
> + schedule_work(&virtserial.rx_work);
> +}
> +
> +static void tx_intr(struct virtqueue *vq)
> +{
> + schedule_work(&virtserial.tx_work);
> +}
> +
> +static void config_intr(struct virtio_device *vdev)
> +{
> + schedule_work(&virtserial.config_work);
> +}
> +
> +static u32 virtserial_get_hot_add_port(struct virtio_serial_config *config)
> +{
> + u32 i;
> + u32 port_nr;
> +
> + for (i = 0; i < virtserial.config->max_nr_ports / 32; i++) {
> + port_nr = ffs(config->ports_map[i] ^ virtserial.config->ports_map[i]);
> + if (port_nr)
> + break;
> + }
> + if (unlikely(!port_nr))
> + return VIRTIO_SERIAL_BAD_ID;
> +
> + /* We used ffs above */
> + port_nr--;
> +
> + /* FIXME: Do this only when add_port is successful */
> + virtserial.config->ports_map[i] |= 1U << port_nr;
> +
> + port_nr += i * 32;
> + return port_nr;
> +}
> +
> +static u32 virtserial_find_next_port(u32 *map, int *map_i)
> +{
> + u32 port_nr;
> +
> + while (1) {
> + port_nr = ffs(*map);
> + if (port_nr)
> + break;
> +
> + if (unlikely(*map_i >= virtserial.config->max_nr_ports / 32))
> + return VIRTIO_SERIAL_BAD_ID;
> + ++*map_i;
> + *map = virtserial.config->ports_map[*map_i];
> + }
> + /* We used ffs above */
> + port_nr--;
> +
> + /* FIXME: Do this only when add_port is successful / reset bit
> + * in config space if add_port was unsuccessful
> + */
> + *map &= ~(1U << port_nr);
> +
> + port_nr += *map_i * 32;
> + return port_nr;
> +}
> +
> +static int virtserial_add_port(u32 port_nr)
> +{
> + struct virtio_serial_port *port;
> + dev_t devt;
> + int ret;
> +
> + port = kzalloc(sizeof(struct virtio_serial_port), GFP_KERNEL);
> + if (!port)
> + return -ENOMEM;
> +
> + devt = MKDEV(major, port_nr);
> + cdev_init(&port->cdev, &virtserial_fops);
> +
> + ret = register_chrdev_region(devt, 1, "virtio-serial");
> + if (ret < 0) {
> + pr_err("%s: error registering chrdev region, ret = %d\n",
> + __func__, ret);
> + goto free_cdev;
> + }
> + ret = cdev_add(&port->cdev, devt, 1);
> + if (ret < 0) {
> + pr_err("%s: error adding cdev, ret = %d\n", __func__, ret);
> + goto free_cdev;
> + }
> + port->dev = device_create(virtserial.class, NULL, devt, NULL,
> + "vmch%u", port_nr);
> + if (IS_ERR(port->dev)) {
> + ret = PTR_ERR(port->dev);
> + pr_err("%s: Error creating device, ret = %d\n", __func__, ret);
> + goto free_cdev;
> + }
> + INIT_LIST_HEAD(&port->readbuf_head);
> + init_completion(&port->have_data);
> +
> + list_add_tail(&port->next, &virtserial.port_head);
> +
> + pr_info("virtio-serial port found at id %u\n", port_nr);
> +
> + return 0;
> +free_cdev:
> + unregister_chrdev(major, "virtio-serial");
> + return ret;
> +}
> +
> +static __u32 get_ports_map_size(__u32 max_ports)
> +{
> + return sizeof(__u32) * ((max_ports + 31) / 32);
> +}
The __ versions are for user-visible headers only.
> +
> +static void virtio_serial_config_work_handler(struct work_struct *work)
> +{
> + struct virtio_serial_config *virtserconf;
> + struct virtio_device *vdev = virtserial.vdev;
> + u32 i, port_nr;
> + int ret;
> +
> + virtserconf = kmalloc(sizeof(struct virtio_serial_config) +
> + get_ports_map_size(virtserial.config->max_nr_ports),
> + GFP_KERNEL);
> + vdev->config->get(vdev,
> + offsetof(struct virtio_serial_config, nr_active_ports),
> + &virtserconf->nr_active_ports,
> + sizeof(virtserconf->nr_active_ports));
> + vdev->config->get(vdev,
> + offsetof(struct virtio_serial_config, ports_map),
> + virtserconf->ports_map,
> + get_ports_map_size(virtserial.config->max_nr_ports));
> +
> + /* Hot-add ports */
> + for (i = virtserial.config->nr_active_ports;
> + i < virtserconf->nr_active_ports; i++) {
> + port_nr = virtserial_get_hot_add_port(virtserconf);
> + if (port_nr == VIRTIO_SERIAL_BAD_ID)
> + continue;
> + ret = virtserial_add_port(port_nr);
> + if (!ret)
> + virtserial.config->nr_active_ports++;
> + }
> + kfree(virtserconf);
> +}
> +
> +static int virtserial_probe(struct virtio_device *vdev)
> +{
> + struct virtqueue *vqs[3];
3?
> + const char *vq_names[] = { "input", "output" };
> + vq_callback_t *vq_callbacks[] = { rx_intr, tx_intr };
> + u32 i, map;
> + int ret, map_i;
> + u32 max_nr_ports;
> +
> + vdev->config->get(vdev, offsetof(struct virtio_serial_config,
> + max_nr_ports),
> + &max_nr_ports,
> + sizeof(max_nr_ports));
> + virtserial.config = kmalloc(sizeof(struct virtio_serial_config)
> + + get_ports_map_size(max_nr_ports),
> + GFP_KERNEL);
kmalloc not checked.
> + virtserial.config->max_nr_ports = max_nr_ports;
> +
> + vdev->config->get(vdev, offsetof(struct virtio_serial_config,
> + nr_active_ports),
> + &virtserial.config->nr_active_ports,
> + sizeof(virtserial.config->nr_active_ports));
> + vdev->config->get(vdev,
> + offsetof(struct virtio_serial_config, ports_map),
> + virtserial.config->ports_map,
> + get_ports_map_size(max_nr_ports));
> +
> + virtserial.vdev = vdev;
> +
> + ret = vdev->config->find_vqs(vdev, 2, vqs, vq_callbacks, vq_names);
> + if (ret)
> + goto fail;
> +
> + virtserial.in_vq = vqs[0];
> + virtserial.out_vq = vqs[1];
> +
> + INIT_LIST_HEAD(&virtserial.port_head);
> +
> + map_i = 0;
> + map = virtserial.config->ports_map[map_i];
> + for (i = 0; i < virtserial.config->nr_active_ports; i++) {
> + __u32 port_nr;
> +
> + port_nr = virtserial_find_next_port(&map, &map_i);
> + if (unlikely(port_nr == VIRTIO_SERIAL_BAD_ID))
> + continue;
> +
> + virtserial_add_port(port_nr);
> + }
> + INIT_WORK(&virtserial.rx_work, &virtio_serial_rx_work_handler);
> + INIT_WORK(&virtserial.tx_work, &virtio_serial_tx_work_handler);
> + INIT_WORK(&virtserial.queue_work, &virtio_serial_queue_work_handler);
> + INIT_WORK(&virtserial.config_work, &virtio_serial_config_work_handler);
> +
> + /* Allocate pages to fill the receive queue */
> + schedule_work(&virtserial.queue_work);
> +
> + return 0;
> +fail:
> + return ret;
> +}
> +
> +
> +static void virtserial_remove_port_data(struct virtio_serial_port *port)
> +{
> + struct list_head *ptr, *ptr2;
> +
> + device_destroy(virtserial.class, port->dev->devt);
> + unregister_chrdev_region(port->dev->devt, 1);
> + cdev_del(&port->cdev);
> +
> + /* Remove the buffers in which we have unconsumed data */
> + list_for_each_safe(ptr, ptr2, &port->readbuf_head) {
> + struct virtio_serial_port_buffer *buf;
> +
> + buf = list_entry(ptr, struct virtio_serial_port_buffer, list);
> +
> + list_del(&buf->list);
> + kfree(buf->buf);
> + kfree(buf);
> + }
> +}
> +
> +static void virtserial_remove(struct virtio_device *vdev)
> +{
> + struct list_head *ptr, *ptr2;
> + char *buf;
> + int len;
> +
> + unregister_chrdev(major, "virtio-serial");
> + class_destroy(virtserial.class);
> +
> + cancel_work_sync(&virtserial.rx_work);
> +
> + /* Free up the unused buffers in the receive queue */
> + while ((buf = virtserial.in_vq->vq_ops->get_buf(virtserial.in_vq, &len)))
> + kfree(buf);
This won't quite work. get_buf gets *used* buffers. You need to track
buffers yourself and delete them after del_vqs.
> + vdev->config->del_vqs(vdev);
> +
> + list_for_each_safe(ptr, ptr2, &virtserial.port_head) {
> + struct virtio_serial_port *port;
> +
> + port = list_entry(ptr, struct virtio_serial_port, next);
> +
> + list_del(&port->next);
> + virtserial_remove_port_data(port);
> + kfree(port);
> + }
> + kfree(virtserial.config);
> +}
> +
> +static struct virtio_device_id id_table[] = {
> + { VIRTIO_ID_SERIAL, VIRTIO_DEV_ANY_ID },
> + { 0 },
> +};
> +
> +static struct virtio_driver virtio_serial = {
> + // .feature_table = features,
> + // .feature_table_size = ARRAY_SIZE(features),
> + .driver.name = KBUILD_MODNAME,
> + .driver.owner = THIS_MODULE,
> + .id_table = id_table,
> + .probe = virtserial_probe,
> + .remove = virtserial_remove,
> + .config_changed = config_intr,
> +};
> +
> +static int __init init(void)
> +{
> + int ret;
> +
> + virtserial.class = class_create(THIS_MODULE, "virtio-serial");
> + if (IS_ERR(virtserial.class)) {
> + pr_err("Error creating virtio-serial class\n");
> + ret = PTR_ERR(virtserial.class);
> + return ret;
> + }
> + ret = register_virtio_driver(&virtio_serial);
> + if (ret) {
> + class_destroy(virtserial.class);
> + return ret;
> + }
> + return 0;
> +}
> +
> +static void __exit fini(void)
> +{
> + unregister_virtio_driver(&virtio_serial);
> +}
> +module_init(init);
> +module_exit(fini);
> +
> +MODULE_DEVICE_TABLE(virtio, id_table);
> +MODULE_DESCRIPTION("Virtio serial driver");
> +MODULE_LICENSE("GPL");
> diff --git a/include/linux/virtio_serial.h b/include/linux/virtio_serial.h
> new file mode 100644
> index 0000000..025dcf1
> --- /dev/null
> +++ b/include/linux/virtio_serial.h
> @@ -0,0 +1,27 @@
> +#ifndef _LINUX_VIRTIO_SERIAL_H
> +#define _LINUX_VIRTIO_SERIAL_H
> +#include <linux/types.h>
> +#include <linux/virtio_config.h>
> +
> +/* Guest kernel - Host interface */
> +
> +/* The ID for virtio serial */
> +#define VIRTIO_ID_SERIAL 7
> +
> +#define VIRTIO_SERIAL_BAD_ID (~(u32)0)
> +
> +struct virtio_serial_config {
> + __u32 max_nr_ports;
> + __u32 nr_active_ports;
> + __u32 ports_map[0 /* (max_nr_ports + 31) / 32 */];
> +};
> +
> +#ifdef __KERNEL__
> +
> +/* Guest kernel - Guest userspace interface */
> +
> +/* IOCTL-related */
> +#define VIRTIO_SERIAL_IO 0xAF
??
Cheers,
Rusty.
--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
