[PATCHv6 2/3] mm: export use_mm/unuse_mm to modules

[Michael S. Tsirkin]

vhost net module wants to do copy to/from user from a kernel thread,
which needs use_mm. Export it to modules.

Acked-by: Andrea Arcangeli <aarc...@redhat.com>
Signed-off-by: Michael S. Tsirkin <m...@redhat.com>
---
mm/mmu_context.c | 3 +++
1 files changed, 3 insertions(+), 0 deletions(-)

diff --git a/mm/mmu_context.c b/mm/mmu_context.c
index ded9081..0777654 100644
--- a/mm/mmu_context.c
+++ b/mm/mmu_context.c
@@ -5,6 +5,7 @@

#include <linux/mm.h>
#include <linux/mmu_context.h>
+#include <linux/module.h>
#include <linux/sched.h>

#include <asm/mmu_context.h>
@@ -37,6 +38,7 @@ void use_mm(struct mm_struct *mm)
if (active_mm != mm)
mmdrop(active_mm);
}
+EXPORT_SYMBOL_GPL(use_mm);

/*
* unuse_mm
@@ -56,3 +58,4 @@ void unuse_mm(struct mm_struct *mm)
enter_lazy_tlb(mm, tsk);
task_unlock(tsk);
}
+EXPORT_SYMBOL_GPL(unuse_mm);
--
1.6.5.rc2

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majo...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

[Michael S. Tsirkin]

Tun device looks similar to a packet socket
in that both pass complete frames from/to userspace.

This patch fills in enough fields in the socket underlying tun driver
to support sendmsg/recvmsg operations, and message flags
MSG_TRUNC and MSG_DONTWAIT, and exports access to this socket
to modules. Regular read/write behaviour is unchanged.

This way, code using raw sockets to inject packets
into a physical device, can support injecting
packets into host network stack almost without modification.

First user of this interface will be vhost virtualization
accelerator.

Signed-off-by: Michael S. Tsirkin <m...@redhat.com>
---

This was posted to netdev separately as well.
Assuming it gets acked, and assuming it's okay with davem,
I think it makes sense to merge this patch through Rusty's
tree because vhost is the first user of the new interface.
Posted here for completeness.

drivers/net/tun.c | 101 +++++++++++++++++++++++++++++++++++++++---------
include/linux/if_tun.h | 14 +++++++
2 files changed, 96 insertions(+), 19 deletions(-)

diff --git a/drivers/net/tun.c b/drivers/net/tun.c
index 4fdfa2a..18f8876 100644
--- a/drivers/net/tun.c
+++ b/drivers/net/tun.c
@@ -144,6 +144,7 @@ static int tun_attach(struct tun_struct *tun, struct file *file)
err = 0;
tfile->tun = tun;
tun->tfile = tfile;
+ tun->socket.file = file;
dev_hold(tun->dev);
sock_hold(tun->socket.sk);
atomic_inc(&tfile->count);
@@ -158,6 +159,7 @@ static void __tun_detach(struct tun_struct *tun)
/* Detach from net device */
netif_tx_lock_bh(tun->dev);
tun->tfile = NULL;
+ tun->socket.file = NULL;
netif_tx_unlock_bh(tun->dev);

/* Drop read queue */
@@ -387,7 +389,8 @@ static netdev_tx_t tun_net_xmit(struct sk_buff *skb, struct net_device *dev)
/* Notify and wake up reader process */
if (tun->flags & TUN_FASYNC)
kill_fasync(&tun->fasync, SIGIO, POLL_IN);
- wake_up_interruptible(&tun->socket.wait);
+ wake_up_interruptible_poll(&tun->socket.wait, POLLIN |
+ POLLRDNORM | POLLRDBAND);
return NETDEV_TX_OK;

drop:
@@ -743,7 +746,7 @@ static __inline__ ssize_t tun_put_user(struct tun_struct *tun,
len = min_t(int, skb->len, len);

skb_copy_datagram_const_iovec(skb, 0, iv, total, len);
- total += len;
+ total += skb->len;

tun->dev->stats.tx_packets++;
tun->dev->stats.tx_bytes += len;
@@ -751,34 +754,23 @@ static __inline__ ssize_t tun_put_user(struct tun_struct *tun,
return total;
}

-static ssize_t tun_chr_aio_read(struct kiocb *iocb, const struct iovec *iv,
- unsigned long count, loff_t pos)
+static ssize_t tun_do_read(struct tun_struct *tun,
+ struct kiocb *iocb, const struct iovec *iv,
+ ssize_t len, int noblock)
{
- struct file *file = iocb->ki_filp;
- struct tun_file *tfile = file->private_data;
- struct tun_struct *tun = __tun_get(tfile);
DECLARE_WAITQUEUE(wait, current);
struct sk_buff *skb;
- ssize_t len, ret = 0;
-
- if (!tun)
- return -EBADFD;
+ ssize_t ret = 0;

DBG(KERN_INFO "%s: tun_chr_read\n", tun->dev->name);

- len = iov_length(iv, count);
- if (len < 0) {
- ret = -EINVAL;
- goto out;
- }
-
add_wait_queue(&tun->socket.wait, &wait);
while (len) {
current->state = TASK_INTERRUPTIBLE;

/* Read frames from the queue */
if (!(skb=skb_dequeue(&tun->socket.sk->sk_receive_queue))) {
- if (file->f_flags & O_NONBLOCK) {
+ if (noblock) {
ret = -EAGAIN;
break;
}
@@ -805,6 +797,27 @@ static ssize_t tun_chr_aio_read(struct kiocb *iocb, const struct iovec *iv,
current->state = TASK_RUNNING;
remove_wait_queue(&tun->socket.wait, &wait);

+ return ret;
+}
+
+static ssize_t tun_chr_aio_read(struct kiocb *iocb, const struct iovec *iv,
+ unsigned long count, loff_t pos)
+{
+ struct file *file = iocb->ki_filp;
+ struct tun_file *tfile = file->private_data;
+ struct tun_struct *tun = __tun_get(tfile);
+ ssize_t len, ret;
+
+ if (!tun)
+ return -EBADFD;
+ len = iov_length(iv, count);
+ if (len < 0) {
+ ret = -EINVAL;
+ goto out;
+ }
+
+ ret = tun_do_read(tun, iocb, iv, len, file->f_flags & O_NONBLOCK);
+ ret = min_t(ssize_t, ret, len);
out:
tun_put(tun);
return ret;
@@ -847,7 +860,8 @@ static void tun_sock_write_space(struct sock *sk)
return;

if (sk->sk_sleep && waitqueue_active(sk->sk_sleep))
- wake_up_interruptible_sync(sk->sk_sleep);
+ wake_up_interruptible_sync_poll(sk->sk_sleep, POLLOUT |
+ POLLWRNORM | POLLWRBAND);

tun = container_of(sk, struct tun_sock, sk)->tun;
kill_fasync(&tun->fasync, SIGIO, POLL_OUT);
@@ -858,6 +872,37 @@ static void tun_sock_destruct(struct sock *sk)
free_netdev(container_of(sk, struct tun_sock, sk)->tun->dev);
}

+static int tun_sendmsg(struct kiocb *iocb, struct socket *sock,
+ struct msghdr *m, size_t total_len)
+{
+ struct tun_struct *tun = container_of(sock, struct tun_struct, socket);
+ return tun_get_user(tun, m->msg_iov, total_len,
+ m->msg_flags & MSG_DONTWAIT);
+}
+
+static int tun_recvmsg(struct kiocb *iocb, struct socket *sock,
+ struct msghdr *m, size_t total_len,
+ int flags)
+{
+ struct tun_struct *tun = container_of(sock, struct tun_struct, socket);
+ int ret;
+ if (flags & ~(MSG_DONTWAIT|MSG_TRUNC))
+ return -EINVAL;
+ ret = tun_do_read(tun, iocb, m->msg_iov, total_len,
+ flags & MSG_DONTWAIT);
+ if (ret > total_len) {
+ m->msg_flags |= MSG_TRUNC;
+ ret = flags & MSG_TRUNC ? ret : total_len;
+ }
+ return ret;
+}
+
+/* Ops structure to mimic raw sockets with tun */
+static const struct proto_ops tun_socket_ops = {
+ .sendmsg = tun_sendmsg,
+ .recvmsg = tun_recvmsg,
+};
+
static struct proto tun_proto = {
.name = "tun",
.owner = THIS_MODULE,
@@ -986,6 +1031,7 @@ static int tun_set_iff(struct net *net, struct file *file, struct ifreq *ifr)
goto err_free_dev;

init_waitqueue_head(&tun->socket.wait);
+ tun->socket.ops = &tun_socket_ops;
sock_init_data(&tun->socket, sk);
sk->sk_write_space = tun_sock_write_space;
sk->sk_sndbuf = INT_MAX;
@@ -1489,6 +1535,23 @@ static void tun_cleanup(void)
rtnl_link_unregister(&tun_link_ops);
}

+/* Get an underlying socket object from tun file. Returns error unless file is
+ * attached to a device. The returned object works like a packet socket, it
+ * can be used for sock_sendmsg/sock_recvmsg. The caller is responsible for
+ * holding a reference to the file for as long as the socket is in use. */
+struct socket *tun_get_socket(struct file *file)
+{
+ struct tun_struct *tun;
+ if (file->f_op != &tun_fops)
+ return ERR_PTR(-EINVAL);
+ tun = tun_get(file);
+ if (!tun)
+ return ERR_PTR(-EBADFD);
+ tun_put(tun);
+ return &tun->socket;
+}
+EXPORT_SYMBOL_GPL(tun_get_socket);
+
module_init(tun_init);
module_exit(tun_cleanup);
MODULE_DESCRIPTION(DRV_DESCRIPTION);
diff --git a/include/linux/if_tun.h b/include/linux/if_tun.h
index 3f5fd52..404abe0 100644
--- a/include/linux/if_tun.h
+++ b/include/linux/if_tun.h
@@ -86,4 +86,18 @@ struct tun_filter {
__u8 addr[0][ETH_ALEN];
};

+#ifdef __KERNEL__
+#if defined(CONFIG_TUN) || defined(CONFIG_TUN_MODULE)
+struct socket *tun_get_socket(struct file *);
+#else
+#include <linux/err.h>
+#include <linux/errno.h>
+struct file;
+struct socket;
+static inline struct socket *tun_get_socket(struct file *f)
+{
+ return ERR_PTR(-EINVAL);
+}
+#endif /* CONFIG_TUN */
+#endif /* __KERNEL__ */
#endif /* __IF_TUN_H */

[Michael S. Tsirkin]

What it is: vhost net is a character device that can be used to reduce
the number of system calls involved in virtio networking.
Existing virtio net code is used in the guest without modification.

There's similarity with vringfd, with some differences and reduced scope
- uses eventfd for signalling
- structures can be moved around in memory at any time (good for
migration, bug work-arounds in userspace)
- write logging is supported (good for migration)
- support memory table and not just an offset (needed for kvm)

common virtio related code has been put in a separate file vhost.c and
can be made into a separate module if/when more backends appear. I used
Rusty's lguest.c as the source for developing this part : this supplied
me with witty comments I wouldn't be able to write myself.

What it is not: vhost net is not a bus, and not a generic new system
call. No assumptions are made on how guest performs hypercalls.
Userspace hypervisors are supported as well as kvm.

How it works: Basically, we connect virtio frontend (configured by
userspace) to a backend. The backend could be a network device, or a tap
device. Backend is also configured by userspace, including vlan/mac
etc.

Status: This works for me, and I haven't see any crashes.
Compared to userspace, people reported improved latency (as I save up to
4 system calls per packet), as well as better bandwidth and CPU
utilization.

Features that I plan to look at in the future:
- mergeable buffers
- zero copy
- scalability tuning: figure out the best threading model to use

Acked-by: Arnd Bergmann <ar...@arndb.de>

Signed-off-by: Michael S. Tsirkin <m...@redhat.com>
---

MAINTAINERS | 10 +
arch/x86/kvm/Kconfig | 1 +
drivers/Makefile | 1 +
drivers/vhost/Kconfig | 11 +
drivers/vhost/Makefile | 2 +
drivers/vhost/net.c | 634 +++++++++++++++++++++++++++++
drivers/vhost/vhost.c | 968 ++++++++++++++++++++++++++++++++++++++++++++
drivers/vhost/vhost.h | 158 +++++++
include/linux/Kbuild | 1 +
include/linux/miscdevice.h | 1 +
include/linux/vhost.h | 126 ++++++
11 files changed, 1913 insertions(+), 0 deletions(-)
create mode 100644 drivers/vhost/Kconfig
create mode 100644 drivers/vhost/Makefile
create mode 100644 drivers/vhost/net.c
create mode 100644 drivers/vhost/vhost.c
create mode 100644 drivers/vhost/vhost.h
create mode 100644 include/linux/vhost.h

diff --git a/MAINTAINERS b/MAINTAINERS
index 69e31aa..58b7a21 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -5605,6 +5605,15 @@ S: Maintained
F: Documentation/filesystems/vfat.txt
F: fs/fat/

+VIRTIO HOST (VHOST)
+M: "Michael S. Tsirkin" <m...@redhat.com>
+L: k...@vger.kernel.org
+L: virtual...@lists.osdl.org
+L: net...@vger.kernel.org
+S: Maintained
+F: drivers/vhost/
+F: include/linux/vhost.h
+
VIA RHINE NETWORK DRIVER
M: Roger Luethi <r...@hellgate.ch>
S: Maintained
diff --git a/arch/x86/kvm/Kconfig b/arch/x86/kvm/Kconfig
index b84e571..94f44d9 100644
--- a/arch/x86/kvm/Kconfig
+++ b/arch/x86/kvm/Kconfig
@@ -64,6 +64,7 @@ config KVM_AMD

# OK, it's a little counter-intuitive to do this, but it puts it neatly under
# the virtualization menu.
+source drivers/vhost/Kconfig
source drivers/lguest/Kconfig
source drivers/virtio/Kconfig

diff --git a/drivers/Makefile b/drivers/Makefile
index 6ee53c7..81e3659 100644
--- a/drivers/Makefile
+++ b/drivers/Makefile
@@ -106,6 +106,7 @@ obj-$(CONFIG_HID) += hid/
obj-$(CONFIG_PPC_PS3) += ps3/
obj-$(CONFIG_OF) += of/
obj-$(CONFIG_SSB) += ssb/
+obj-$(CONFIG_VHOST_NET) += vhost/
obj-$(CONFIG_VIRTIO) += virtio/
obj-$(CONFIG_VLYNQ) += vlynq/
obj-$(CONFIG_STAGING) += staging/
diff --git a/drivers/vhost/Kconfig b/drivers/vhost/Kconfig
new file mode 100644
index 0000000..d955406
--- /dev/null
+++ b/drivers/vhost/Kconfig
@@ -0,0 +1,11 @@
+config VHOST_NET
+ tristate "Host kernel accelerator for virtio net"
+ depends on NET && EVENTFD
+ ---help---
+ This kernel module can be loaded in host kernel to accelerate
+ guest networking with virtio_net. Not to be confused with virtio_net
+ module itself which needs to be loaded in guest kernel.
+
+ To compile this driver as a module, choose M here: the module will
+ be called vhost_net.
+
diff --git a/drivers/vhost/Makefile b/drivers/vhost/Makefile
new file mode 100644
index 0000000..72dd020
--- /dev/null
+++ b/drivers/vhost/Makefile
@@ -0,0 +1,2 @@
+obj-$(CONFIG_VHOST_NET) += vhost_net.o
+vhost_net-y := vhost.o net.o
diff --git a/drivers/vhost/net.c b/drivers/vhost/net.c
new file mode 100644
index 0000000..ecf86ec
--- /dev/null
+++ b/drivers/vhost/net.c
@@ -0,0 +1,634 @@
+/* Copyright (C) 2009 Red Hat, Inc.
+ * Author: Michael S. Tsirkin <m...@redhat.com>
+ *
+ * This work is licensed under the terms of the GNU GPL, version 2.
+ *
+ * virtio-net server in host kernel.
+ */
+
+#include <linux/compat.h>
+#include <linux/eventfd.h>
+#include <linux/vhost.h>
+#include <linux/virtio_net.h>
+#include <linux/mmu_context.h>
+#include <linux/miscdevice.h>
+#include <linux/module.h>
+#include <linux/mutex.h>
+#include <linux/workqueue.h>
+#include <linux/rcupdate.h>
+#include <linux/file.h>
+
+#include <linux/net.h>
+#include <linux/if_packet.h>
+#include <linux/if_arp.h>
+#include <linux/if_tun.h>
+
+#include <net/sock.h>
+
+#include "vhost.h"
+
+/* Max number of bytes transferred before requeueing the job.
+ * Using this limit prevents one virtqueue from starving others. */
+#define VHOST_NET_WEIGHT 0x80000
+
+enum {
+ VHOST_NET_VQ_RX = 0,
+ VHOST_NET_VQ_TX = 1,
+ VHOST_NET_VQ_MAX = 2,
+};
+
+enum vhost_net_poll_state {
+ VHOST_NET_POLL_DISABLED = 0,
+ VHOST_NET_POLL_STARTED = 1,
+ VHOST_NET_POLL_STOPPED = 2,
+};
+
+struct vhost_net {
+ struct vhost_dev dev;
+ struct vhost_virtqueue vqs[VHOST_NET_VQ_MAX];
+ struct vhost_poll poll[VHOST_NET_VQ_MAX];
+ /* Tells us whether we are polling a socket for TX.
+ * We only do this when socket buffer fills up.
+ * Protected by tx vq lock. */
+ enum vhost_net_poll_state tx_poll_state;
+};
+
+/* Pop first len bytes from iovec. Return number of segments used. */
+static int move_iovec_hdr(struct iovec *from, struct iovec *to,
+ size_t len, int iov_count)
+{
+ int seg = 0;
+ size_t size;
+ while (len && seg < iov_count) {
+ size = min(from->iov_len, len);
+ to->iov_base = from->iov_base;
+ to->iov_len = size;
+ from->iov_len -= size;
+ from->iov_base += size;
+ len -= size;
+ ++from;
+ ++to;
+ ++seg;
+ }
+ return seg;
+}
+
+/* Caller must have TX VQ lock */
+static void tx_poll_stop(struct vhost_net *net)
+{
+ if (likely(net->tx_poll_state != VHOST_NET_POLL_STARTED))
+ return;
+ vhost_poll_stop(net->poll + VHOST_NET_VQ_TX);
+ net->tx_poll_state = VHOST_NET_POLL_STOPPED;
+}
+
+/* Caller must have TX VQ lock */
+static void tx_poll_start(struct vhost_net *net, struct socket *sock)
+{
+ if (unlikely(net->tx_poll_state != VHOST_NET_POLL_STOPPED))
+ return;
+ vhost_poll_start(net->poll + VHOST_NET_VQ_TX, sock->file);
+ net->tx_poll_state = VHOST_NET_POLL_STARTED;
+}
+
+/* Expects to be always run from workqueue - which acts as
+ * read-size critical section for our kind of RCU. */
+static void handle_tx(struct vhost_net *net)
+{
+ struct vhost_virtqueue *vq = &net->dev.vqs[VHOST_NET_VQ_TX];
+ unsigned head, out, in, s;
+ struct msghdr msg = {
+ .msg_name = NULL,
+ .msg_namelen = 0,
+ .msg_control = NULL,
+ .msg_controllen = 0,
+ .msg_iov = vq->iov,
+ .msg_flags = MSG_DONTWAIT,
+ };
+ size_t len, total_len = 0;
+ int err, wmem;
+ size_t hdr_size;
+ struct socket *sock = rcu_dereference(vq->private_data);
+ if (!sock)
+ return;
+
+ wmem = atomic_read(&sock->sk->sk_wmem_alloc);
+ if (wmem >= sock->sk->sk_sndbuf)
+ return;
+
+ use_mm(net->dev.mm);
+ mutex_lock(&vq->mutex);
+ vhost_no_notify(vq);
+
+ if (wmem < sock->sk->sk_sndbuf * 2)
+ tx_poll_stop(net);
+ hdr_size = vq->hdr_size;
+
+ for (;;) {
+ head = vhost_get_vq_desc(&net->dev, vq, vq->iov, &out, &in,
+ NULL, NULL);
+ /* Nothing new? Wait for eventfd to tell us they refilled. */
+ if (head == vq->num) {
+ wmem = atomic_read(&sock->sk->sk_wmem_alloc);
+ if (wmem >= sock->sk->sk_sndbuf * 3 / 4) {
+ tx_poll_start(net, sock);
+ set_bit(SOCK_ASYNC_NOSPACE, &sock->flags);
+ break;
+ }
+ if (vhost_notify(vq)) {
+ continue;
+ }
+ break;
+ }
+ if (in) {
+ vq_err(vq, "Unexpected descriptor format for TX: "
+ "out %d, int %d\n", out, in);
+ break;
+ }
+ /* Skip header. TODO: support TSO. */
+ s = move_iovec_hdr(vq->iov, vq->hdr, hdr_size, out);
+ msg.msg_iovlen = out;
+ len = iov_length(vq->iov, out);
+ /* Sanity check */
+ if (!len) {
+ vq_err(vq, "Unexpected header len for TX: "
+ "%ld expected %zd\n",
+ iov_length(vq->hdr, s), hdr_size);
+ break;
+ }
+ /* TODO: Check specific error and bomb out unless ENOBUFS? */
+ err = sock->ops->sendmsg(NULL, sock, &msg, len);
+ if (unlikely(err < 0)) {
+ vhost_discard_vq_desc(vq);
+ tx_poll_start(net, sock);
+ break;
+ }
+ if (err != len)
+ pr_err("Truncated TX packet: "
+ " len %d != %zd\n", err, len);
+ vhost_add_used_and_trigger(&net->dev, vq, head, 0);
+ total_len += len;
+ if (unlikely(total_len >= VHOST_NET_WEIGHT)) {
+ vhost_poll_queue(&vq->poll);
+ break;
+ }
+ }
+
+ mutex_unlock(&vq->mutex);
+ unuse_mm(net->dev.mm);
+}
+
+/* Expects to be always run from workqueue - which acts as
+ * read-size critical section for our kind of RCU. */
+static void handle_rx(struct vhost_net *net)
+{
+ struct vhost_virtqueue *vq = &net->dev.vqs[VHOST_NET_VQ_RX];
+ unsigned head, out, in, log, s;
+ struct vhost_log *vq_log;
+ struct msghdr msg = {
+ .msg_name = NULL,
+ .msg_namelen = 0,
+ .msg_control = NULL, /* FIXME: get and handle RX aux data. */
+ .msg_controllen = 0,
+ .msg_iov = vq->iov,
+ .msg_flags = MSG_DONTWAIT,
+ };
+
+ struct virtio_net_hdr hdr = {
+ .flags = 0,
+ .gso_type = VIRTIO_NET_HDR_GSO_NONE
+ };
+
+ size_t len, total_len = 0;
+ int err;
+ size_t hdr_size;
+ struct socket *sock = rcu_dereference(vq->private_data);
+ if (!sock || skb_queue_empty(&sock->sk->sk_receive_queue))
+ return;
+
+ use_mm(net->dev.mm);
+ mutex_lock(&vq->mutex);
+ vhost_no_notify(vq);
+ hdr_size = vq->hdr_size;
+
+ vq_log = unlikely(vhost_has_feature(&net->dev, VHOST_F_LOG_ALL)) ?
+ vq->log : NULL;
+
+ for (;;) {
+ head = vhost_get_vq_desc(&net->dev, vq, vq->iov, &out, &in,
+ vq_log, &log);
+ /* OK, now we need to know about added descriptors. */
+ if (head == vq->num && vhost_notify(vq))
+ /* They could have slipped one in as we were doing that:
+ * check again. */
+ continue;
+ /* Nothing new? Wait for eventfd to tell us they refilled. */
+ if (head == vq->num)
+ break;
+ /* We don't need to be notified again. */
+ vhost_no_notify(vq);
+ if (out) {
+ vq_err(vq, "Unexpected descriptor format for RX: "
+ "out %d, int %d\n",
+ out, in);
+ break;
+ }
+ /* Skip header. TODO: support TSO/mergeable rx buffers. */
+ s = move_iovec_hdr(vq->iov, vq->hdr, hdr_size, in);
+ msg.msg_iovlen = in;
+ len = iov_length(vq->iov, in);
+ /* Sanity check */
+ if (!len) {
+ vq_err(vq, "Unexpected header len for RX: "
+ "%zd expected %zd\n",
+ iov_length(vq->hdr, s), hdr_size);
+ break;
+ }
+ err = sock->ops->recvmsg(NULL, sock, &msg,
+ len, MSG_DONTWAIT | MSG_TRUNC);
+ /* TODO: Check specific error and bomb out unless EAGAIN? */
+ if (err < 0) {
+ vhost_discard_vq_desc(vq);
+ break;
+ }
+ /* TODO: Should check and handle checksum. */
+ if (err > len) {
+ pr_err("Discarded truncated rx packet: "
+ " len %d > %zd\n", err, len);
+ vhost_discard_vq_desc(vq);
+ continue;
+ }
+ len = err;
+ err = memcpy_toiovec(vq->hdr, (unsigned char *)&hdr, hdr_size);
+ if (err) {
+ vq_err(vq, "Unable to write vnet_hdr at addr %p: %d\n",
+ vq->iov->iov_base, err);
+ break;
+ }
+ len += hdr_size;
+ vhost_add_used_and_trigger(&net->dev, vq, head, len);
+ if (unlikely(vq_log))
+ vhost_log_write(vq, vq_log, log, len);
+ total_len += len;
+ if (unlikely(total_len >= VHOST_NET_WEIGHT)) {
+ vhost_poll_queue(&vq->poll);
+ break;
+ }
+ }
+
+ mutex_unlock(&vq->mutex);
+ unuse_mm(net->dev.mm);
+}
+
+static void handle_tx_kick(struct work_struct *work)
+{
+ struct vhost_virtqueue *vq;
+ struct vhost_net *net;
+ vq = container_of(work, struct vhost_virtqueue, poll.work);
+ net = container_of(vq->dev, struct vhost_net, dev);
+ handle_tx(net);
+}
+
+static void handle_rx_kick(struct work_struct *work)
+{
+ struct vhost_virtqueue *vq;
+ struct vhost_net *net;
+ vq = container_of(work, struct vhost_virtqueue, poll.work);
+ net = container_of(vq->dev, struct vhost_net, dev);
+ handle_rx(net);
+}
+
+static void handle_tx_net(struct work_struct *work)
+{
+ struct vhost_net *net;
+ net = container_of(work, struct vhost_net, poll[VHOST_NET_VQ_TX].work);
+ handle_tx(net);
+}
+
+static void handle_rx_net(struct work_struct *work)
+{
+ struct vhost_net *net;
+ net = container_of(work, struct vhost_net, poll[VHOST_NET_VQ_RX].work);
+ handle_rx(net);
+}
+
+static int vhost_net_open(struct inode *inode, struct file *f)
+{
+ struct vhost_net *n = kzalloc(sizeof *n, GFP_KERNEL);
+ int r;
+ if (!n)
+ return -ENOMEM;
+ f->private_data = n;
+ n->vqs[VHOST_NET_VQ_TX].handle_kick = handle_tx_kick;
+ n->vqs[VHOST_NET_VQ_RX].handle_kick = handle_rx_kick;
+ r = vhost_dev_init(&n->dev, n->vqs, VHOST_NET_VQ_MAX);
+ if (r < 0) {
+ kfree(n);
+ return r;
+ }
+
+ vhost_poll_init(n->poll + VHOST_NET_VQ_TX, handle_tx_net, POLLOUT);
+ vhost_poll_init(n->poll + VHOST_NET_VQ_RX, handle_rx_net, POLLIN);
+ n->tx_poll_state = VHOST_NET_POLL_DISABLED;
+ return 0;
+}
+
+static void vhost_net_disable_vq(struct vhost_net *n, int index)
+{
+ if (!n->vqs[index].private_data)
+ return;
+ if (index == VHOST_NET_VQ_TX) {
+ tx_poll_stop(n);
+ n->tx_poll_state = VHOST_NET_POLL_DISABLED;
+ } else
+ vhost_poll_stop(n->poll + VHOST_NET_VQ_RX);
+}
+
+static void vhost_net_enable_vq(struct vhost_net *n, int index)
+{
+ struct socket *sock = n->vqs[index].private_data;
+ if (!sock)
+ return;
+ if (index == VHOST_NET_VQ_TX) {
+ n->tx_poll_state = VHOST_NET_POLL_STOPPED;
+ tx_poll_start(n, sock);
+ } else
+ vhost_poll_start(n->poll + VHOST_NET_VQ_RX, sock->file);
+}
+
+static struct socket *vhost_net_stop_vq(struct vhost_net *n, int index)
+{
+ struct socket *sock;
+
+ mutex_lock(&n->vqs[index].mutex);
+ sock = n->vqs[index].private_data;
+ vhost_net_disable_vq(n, index);
+ rcu_assign_pointer(n->vqs[index].private_data, NULL);
+ mutex_unlock(&n->vqs[index].mutex);
+ return sock;
+}
+
+static void vhost_net_stop(struct vhost_net *n, struct socket **tx_sock,
+ struct socket **rx_sock)
+{
+ *tx_sock = vhost_net_stop_vq(n, VHOST_NET_VQ_TX);
+ *rx_sock = vhost_net_stop_vq(n, VHOST_NET_VQ_RX);
+}
+
+static void vhost_net_flush_vq(struct vhost_net *n, int index)
+{
+ vhost_poll_flush(n->poll + index);
+ vhost_poll_flush(&n->dev.vqs[index].poll);
+}
+
+static void vhost_net_flush(struct vhost_net *n)
+{
+ vhost_net_flush_vq(n, VHOST_NET_VQ_TX);
+ vhost_net_flush_vq(n, VHOST_NET_VQ_RX);
+}
+
+static int vhost_net_release(struct inode *inode, struct file *f)
+{
+ struct vhost_net *n = f->private_data;
+ struct socket *tx_sock;
+ struct socket *rx_sock;
+
+ vhost_net_stop(n, &tx_sock, &rx_sock);
+ vhost_net_flush(n);
+ vhost_dev_cleanup(&n->dev);
+ if (tx_sock)
+ fput(tx_sock->file);
+ if (rx_sock)
+ fput(rx_sock->file);
+ /* We do an extra flush before freeing memory,
+ * since jobs can re-queue themselves. */
+ vhost_net_flush(n);
+ kfree(n);
+ return 0;
+}
+
+static struct socket *get_raw_socket(int fd)
+{
+ struct {
+ struct sockaddr_ll sa;
+ char buf[MAX_ADDR_LEN];
+ } uaddr;
+ int uaddr_len = sizeof uaddr, r;
+ struct socket *sock = sockfd_lookup(fd, &r);
+ if (!sock)
+ return ERR_PTR(-ENOTSOCK);
+
+ /* Parameter checking */
+ if (sock->sk->sk_type != SOCK_RAW) {
+ r = -ESOCKTNOSUPPORT;
+ goto err;
+ }
+
+ r = sock->ops->getname(sock, (struct sockaddr *)&uaddr.sa,
+ &uaddr_len, 0);
+ if (r)
+ goto err;
+
+ if (uaddr.sa.sll_family != AF_PACKET) {
+ r = -EPFNOSUPPORT;
+ goto err;
+ }
+ return sock;
+err:
+ fput(sock->file);
+ return ERR_PTR(r);
+}
+
+static struct socket *get_tun_socket(int fd)
+{
+ struct file *file = fget(fd);
+ struct socket *sock;
+ if (!file)
+ return ERR_PTR(-EBADF);
+ sock = tun_get_socket(file);
+ if (IS_ERR(sock))
+ fput(file);
+ return sock;
+}
+
+static struct socket *get_socket(int fd)
+{
+ struct socket *sock;
+ if (fd == -1)
+ return NULL;
+ sock = get_raw_socket(fd);
+ if (!IS_ERR(sock))
+ return sock;
+ sock = get_tun_socket(fd);
+ if (!IS_ERR(sock))
+ return sock;
+ return ERR_PTR(-ENOTSOCK);
+}
+
+static long vhost_net_set_backend(struct vhost_net *n, unsigned index, int fd)
+{
+ struct socket *sock, *oldsock = NULL;
+ struct vhost_virtqueue *vq;
+ int r;
+
+ mutex_lock(&n->dev.mutex);
+ r = vhost_dev_check_owner(&n->dev);
+ if (r)
+ goto done;
+
+ if (index >= VHOST_NET_VQ_MAX) {
+ r = -ENOBUFS;
+ goto done;
+ }
+ vq = n->vqs + index;
+ mutex_lock(&vq->mutex);
+ sock = get_socket(fd);
+ if (IS_ERR(sock)) {
+ r = PTR_ERR(sock);
+ goto done;
+ }
+
+ /* start polling new socket */
+ oldsock = vq->private_data;
+ if (sock == oldsock)
+ goto done;
+
+ vhost_net_disable_vq(n, index);
+ rcu_assign_pointer(vq->private_data, sock);
+ vhost_net_enable_vq(n, index);
+ mutex_unlock(&vq->mutex);
+done:
+ mutex_unlock(&n->dev.mutex);
+ if (oldsock) {
+ vhost_net_flush_vq(n, index);
+ fput(oldsock->file);
+ }
+ return r;
+}
+
+static long vhost_net_reset_owner(struct vhost_net *n)
+{
+ struct socket *tx_sock = NULL;
+ struct socket *rx_sock = NULL;
+ long r;
+ mutex_lock(&n->dev.mutex);
+ r = vhost_dev_check_owner(&n->dev);
+ if (r)
+ goto done;
+ vhost_net_stop(n, &tx_sock, &rx_sock);
+ vhost_net_flush(n);
+ r = vhost_dev_reset_owner(&n->dev);
+done:
+ mutex_unlock(&n->dev.mutex);
+ if (tx_sock)
+ fput(tx_sock->file);
+ if (rx_sock)
+ fput(rx_sock->file);
+ return r;
+}
+
+static void vhost_net_set_features(struct vhost_net *n, u64 features)
+{
+ size_t hdr_size = features & (1 << VHOST_NET_F_VIRTIO_NET_HDR) ?
+ sizeof(struct virtio_net_hdr) : 0;
+ int i;
+ mutex_unlock(&n->dev.mutex);
+ n->dev.acked_features = features;
+ smp_wmb();
+ for (i = 0; i < VHOST_NET_VQ_MAX; ++i) {
+ mutex_lock(&n->vqs[i].mutex);
+ n->vqs[i].hdr_size = hdr_size;
+ mutex_unlock(&n->vqs[i].mutex);
+ }
+ mutex_unlock(&n->dev.mutex);
+ vhost_net_flush(n);
+}
+
+static long vhost_net_ioctl(struct file *f, unsigned int ioctl,
+ unsigned long arg)
+{
+ struct vhost_net *n = f->private_data;
+ void __user *argp = (void __user *)arg;
+ u32 __user *featurep = argp;
+ struct vhost_vring_file backend;
+ u64 features;
+ int r;
+ switch (ioctl) {
+ case VHOST_NET_SET_BACKEND:
+ r = copy_from_user(&backend, argp, sizeof backend);
+ if (r < 0)
+ return r;
+ return vhost_net_set_backend(n, backend.index, backend.fd);
+ case VHOST_GET_FEATURES:
+ features = VHOST_FEATURES;
+ return put_user(features, featurep);
+ case VHOST_ACK_FEATURES:
+ r = get_user(features, featurep);
+ /* No features for now */
+ if (r < 0)
+ return r;
+ if (features & ~VHOST_FEATURES)
+ return -EOPNOTSUPP;
+ vhost_net_set_features(n, features);
+ return 0;
+ case VHOST_RESET_OWNER:
+ return vhost_net_reset_owner(n);
+ default:
+ r = vhost_dev_ioctl(&n->dev, ioctl, arg);
+ vhost_net_flush(n);
+ return r;
+ }
+}
+
+#ifdef CONFIG_COMPAT
+static long vhost_net_compat_ioctl(struct file *f, unsigned int ioctl,
+ unsigned long arg)
+{
+ return vhost_net_ioctl(f, ioctl, (unsigned long)compat_ptr(arg));
+}
+#endif
+
+const static struct file_operations vhost_net_fops = {
+ .owner = THIS_MODULE,
+ .release = vhost_net_release,
+ .unlocked_ioctl = vhost_net_ioctl,
+#ifdef CONFIG_COMPAT
+ .compat_ioctl = vhost_net_compat_ioctl,
+#endif
+ .open = vhost_net_open,
+};
+
+static struct miscdevice vhost_net_misc = {
+ VHOST_NET_MINOR,
+ "vhost-net",
+ &vhost_net_fops,
+};
+
+int vhost_net_init(void)
+{
+ int r = vhost_init();
+ if (r)
+ goto err_init;
+ r = misc_register(&vhost_net_misc);
+ if (r)
+ goto err_reg;
+ return 0;
+err_reg:
+ vhost_cleanup();
+err_init:
+ return r;
+
+}
+module_init(vhost_net_init);
+
+void vhost_net_exit(void)
+{
+ misc_deregister(&vhost_net_misc);
+ vhost_cleanup();
+}
+module_exit(vhost_net_exit);
+
+MODULE_VERSION("0.0.1");
+MODULE_LICENSE("GPL v2");
+MODULE_AUTHOR("Michael S. Tsirkin");
+MODULE_DESCRIPTION("Host kernel accelerator for virtio net");
diff --git a/drivers/vhost/vhost.c b/drivers/vhost/vhost.c
new file mode 100644
index 0000000..6db2e63
--- /dev/null
+++ b/drivers/vhost/vhost.c
@@ -0,0 +1,968 @@
+/* Copyright (C) 2009 Red Hat, Inc.
+ * Copyright (C) 2006 Rusty Russell IBM Corporation
+ *
+ * Author: Michael S. Tsirkin <m...@redhat.com>
+ *
+ * Inspiration, some code, and most witty comments come from
+ * Documentation/lguest/lguest.c, by Rusty Russell
+ *
+ * This work is licensed under the terms of the GNU GPL, version 2.
+ *
+ * Generic code for virtio server in host kernel.
+ */
+
+#include <linux/eventfd.h>
+#include <linux/vhost.h>
+#include <linux/virtio_net.h>
+#include <linux/mm.h>
+#include <linux/miscdevice.h>
+#include <linux/mutex.h>
+#include <linux/workqueue.h>
+#include <linux/rcupdate.h>
+#include <linux/poll.h>
+#include <linux/file.h>
+#include <linux/highmem.h>
+
+#include <linux/net.h>
+#include <linux/if_packet.h>
+#include <linux/if_arp.h>
+
+#include <net/sock.h>
+
+#include "vhost.h"
+
+enum {
+ VHOST_MEMORY_MAX_NREGIONS = 64,
+ VHOST_MEMORY_F_LOG = 0x1,
+};
+
+static struct workqueue_struct *vhost_workqueue;
+
+static void vhost_poll_func(struct file *file, wait_queue_head_t *wqh,
+ poll_table *pt)
+{
+ struct vhost_poll *poll;
+ poll = container_of(pt, struct vhost_poll, table);
+
+ poll->wqh = wqh;
+ add_wait_queue(wqh, &poll->wait);
+}
+
+static int vhost_poll_wakeup(wait_queue_t *wait, unsigned mode, int sync,
+ void *key)
+{
+ struct vhost_poll *poll;
+ poll = container_of(wait, struct vhost_poll, wait);
+ if (!((unsigned long)key & poll->mask))
+ return 0;
+
+ queue_work(vhost_workqueue, &poll->work);
+ return 0;
+}
+
+/* Init poll structure */
+void vhost_poll_init(struct vhost_poll *poll, work_func_t func,
+ unsigned long mask)
+{
+ INIT_WORK(&poll->work, func);
+ init_waitqueue_func_entry(&poll->wait, vhost_poll_wakeup);
+ init_poll_funcptr(&poll->table, vhost_poll_func);
+ poll->mask = mask;
+}
+
+/* Start polling a file. We add ourselves to file's wait queue. The caller must
+ * keep a reference to a file until after vhost_poll_stop is called. */
+void vhost_poll_start(struct vhost_poll *poll, struct file *file)
+{
+ unsigned long mask;
+ mask = file->f_op->poll(file, &poll->table);
+ if (mask)
+ vhost_poll_wakeup(&poll->wait, 0, 0, (void *)mask);
+}
+
+/* Stop polling a file. After this function returns, it becomes safe to drop the
+ * file reference. You must also flush afterwards. */
+void vhost_poll_stop(struct vhost_poll *poll)
+{
+ remove_wait_queue(poll->wqh, &poll->wait);
+}
+
+/* Flush any work that has been scheduled. When calling this, don't hold any
+ * locks that are also used by the callback. */
+void vhost_poll_flush(struct vhost_poll *poll)
+{
+ flush_work(&poll->work);
+}
+
+void vhost_poll_queue(struct vhost_poll *poll)
+{
+ queue_work(vhost_workqueue, &poll->work);
+}
+
+long vhost_dev_init(struct vhost_dev *dev,
+ struct vhost_virtqueue *vqs, int nvqs)
+{
+ int i;
+ dev->vqs = vqs;
+ dev->nvqs = nvqs;
+ mutex_init(&dev->mutex);
+
+ for (i = 0; i < dev->nvqs; ++i) {
+ dev->vqs[i].dev = dev;
+ mutex_init(&dev->vqs[i].mutex);
+ if (dev->vqs[i].handle_kick)
+ vhost_poll_init(&dev->vqs[i].poll,
+ dev->vqs[i].handle_kick,
+ POLLIN);
+ }
+ return 0;
+}
+
+/* Caller should have device mutex */
+long vhost_dev_check_owner(struct vhost_dev *dev)
+{
+ /* Are you the owner? If not, I don't think you mean to do that */
+ return dev->mm == current->mm ? 0 : -EPERM;
+}
+
+/* Caller should have device mutex */
+static long vhost_dev_set_owner(struct vhost_dev *dev)
+{
+ /* Is there an owner already? */
+ if (dev->mm)
+ return -EBUSY;
+ /* No owner, become one */
+ dev->mm = get_task_mm(current);
+ return 0;
+}
+
+/* Caller should have device mutex */
+long vhost_dev_reset_owner(struct vhost_dev *dev)
+{
+ struct vhost_memory *memory;
+
+ /* Restore memory to default 1:1 mapping. */
+ memory = kzalloc(offsetof(struct vhost_memory, regions) +
+ 2 * sizeof *memory->regions, GFP_KERNEL);
+ if (!memory)
+ return -ENOMEM;
+
+ vhost_dev_cleanup(dev);
+
+ memory->nregions = 2;
+ memory->regions[0].guest_phys_addr = 1;
+ memory->regions[0].userspace_addr = 1;
+ memory->regions[0].memory_size = ~0ULL;
+ memory->regions[1].guest_phys_addr = 0;
+ memory->regions[1].userspace_addr = 0;
+ memory->regions[1].memory_size = 1;
+ dev->memory = memory;
+ return 0;
+}
+
+/* Caller should have device mutex */
+void vhost_dev_cleanup(struct vhost_dev *dev)
+{
+ int i;
+ for (i = 0; i < dev->nvqs; ++i) {
+ if (dev->vqs[i].kick && dev->vqs[i].handle_kick) {
+ vhost_poll_stop(&dev->vqs[i].poll);
+ vhost_poll_flush(&dev->vqs[i].poll);
+ }
+ if (dev->vqs[i].error_ctx)
+ eventfd_ctx_put(dev->vqs[i].error_ctx);
+ if (dev->vqs[i].error)
+ fput(dev->vqs[i].error);
+ if (dev->vqs[i].kick)
+ fput(dev->vqs[i].kick);
+ if (dev->vqs[i].call_ctx)
+ eventfd_ctx_put(dev->vqs[i].call_ctx);
+ if (dev->vqs[i].call)
+ fput(dev->vqs[i].call);
+ dev->vqs[i].error_ctx = NULL;
+ dev->vqs[i].error = NULL;
+ dev->vqs[i].kick = NULL;
+ dev->vqs[i].call_ctx = NULL;
+ dev->vqs[i].call = NULL;
+ }
+ if (dev->log_ctx)
+ eventfd_ctx_put(dev->log_ctx);
+ dev->log_ctx = NULL;
+ if (dev->log_file)
+ fput(dev->log_file);
+ dev->log_file = NULL;
+ /* No one will access memory at this point */
+ kfree(dev->memory);
+ dev->memory = NULL;
+ if (dev->mm)
+ mmput(dev->mm);
+ dev->mm = NULL;
+}
+
+static long vhost_set_memory(struct vhost_dev *d, struct vhost_memory __user *m)
+{
+ struct vhost_memory mem, *newmem, *oldmem;
+ unsigned long size = offsetof(struct vhost_memory, regions);
+ long r;
+ r = copy_from_user(&mem, m, size);
+ if (r)
+ return r;
+ if (mem.padding)
+ return -EOPNOTSUPP;
+ if (mem.nregions > VHOST_MEMORY_MAX_NREGIONS)
+ return -E2BIG;
+ newmem = kmalloc(size + mem.nregions * sizeof *m->regions, GFP_KERNEL);
+ if (!newmem)
+ return -ENOMEM;
+
+ memcpy(newmem, &mem, size);
+ r = copy_from_user(newmem->regions, m->regions,
+ mem.nregions * sizeof *m->regions);
+ if (r) {
+ kfree(newmem);
+ return r;
+ }
+ oldmem = d->memory;
+ rcu_assign_pointer(d->memory, newmem);
+ synchronize_rcu();
+ kfree(oldmem);
+ return 0;
+}
+
+static int init_used(struct vhost_virtqueue *vq)
+{
+ int r = put_user(vq->used_flags, &vq->used->flags);
+ if (r)
+ return r;
+ return get_user(vq->last_used_idx, &vq->used->idx);
+}
+
+static long vhost_set_vring(struct vhost_dev *d, int ioctl, void __user *argp)
+{
+ struct file *eventfp, *filep = NULL,
+ *pollstart = NULL, *pollstop = NULL;
+ struct eventfd_ctx *ctx = NULL;
+ u32 __user *idxp = argp;
+ struct vhost_virtqueue *vq;
+ struct vhost_vring_state s;
+ struct vhost_vring_file f;
+ struct vhost_vring_addr a;
+ u32 idx;
+ long r;
+
+ r = get_user(idx, idxp);
+ if (r < 0)
+ return r;
+ if (idx > d->nvqs)
+ return -ENOBUFS;
+
+ vq = d->vqs + idx;
+
+ mutex_lock(&vq->mutex);
+
+ switch (ioctl) {
+ case VHOST_SET_VRING_NUM:
+ r = copy_from_user(&s, argp, sizeof s);
+ if (r < 0)
+ break;
+ if (s.num > 0xffff) {
+ r = -EINVAL;
+ break;
+ }
+ vq->num = s.num;
+ break;
+ case VHOST_SET_VRING_BASE:
+ r = copy_from_user(&s, argp, sizeof s);
+ if (r < 0)
+ break;
+ if (s.num > 0xffff) {
+ r = -EINVAL;
+ break;
+ }
+ vq->avail_idx = vq->last_avail_idx = s.num;
+ break;
+ case VHOST_GET_VRING_BASE:
+ s.index = idx;
+ s.num = vq->last_avail_idx;
+ r = copy_to_user(argp, &s, sizeof s);
+ break;
+ case VHOST_SET_VRING_DESC:
+ r = copy_from_user(&a, argp, sizeof a);
+ if (r < 0)
+ break;
+ if (a.padding) {
+ r = -EOPNOTSUPP;
+ break;
+ }
+ if ((u64)(unsigned long)a.user_addr != a.user_addr) {
+ r = -EFAULT;
+ break;
+ }
+ vq->desc = (void __user *)(unsigned long)a.user_addr;
+ break;
+ case VHOST_SET_VRING_AVAIL:
+ r = copy_from_user(&a, argp, sizeof a);
+ if (r < 0)
+ break;
+ if (a.padding) {
+ r = -EOPNOTSUPP;
+ break;
+ }
+ if ((u64)(unsigned long)a.user_addr != a.user_addr) {
+ r = -EFAULT;
+ break;
+ }
+ if (a.user_addr & (sizeof *vq->avail->ring - 1)) {
+ r = -EINVAL;
+ break;
+ }
+ vq->avail = (void __user *)(unsigned long)a.user_addr;
+ /* Forget the cached index value. */
+ vq->avail_idx = vq->last_avail_idx;
+ break;
+ case VHOST_SET_VRING_USED:
+ r = copy_from_user(&a, argp, sizeof a);
+ if (r < 0)
+ break;
+ if (a.padding) {
+ r = -EOPNOTSUPP;
+ break;
+ }
+ if ((u64)(unsigned long)a.user_addr != a.user_addr) {
+ r = -EFAULT;
+ break;
+ }
+ if (a.user_addr & (sizeof *vq->used->ring - 1)) {
+ r = -EINVAL;
+ break;
+ }
+ vq->used = (void __user *)(unsigned long)a.user_addr;
+ r = init_used(vq);
+ if (r)
+ break;
+ break;
+ case VHOST_SET_VRING_LOG:
+ r = copy_from_user(&a, argp, sizeof a);
+ if (r < 0)
+ break;
+ if (a.padding) {
+ r = -EOPNOTSUPP;
+ break;
+ }
+ if (a.user_addr == VHOST_VRING_LOG_DISABLE) {
+ vq->log_used = false;
+ break;
+ }
+ if (a.user_addr & (sizeof *vq->used->ring - 1)) {
+ r = -EINVAL;
+ break;
+ }
+ vq->log_used = true;
+ vq->log_addr = a.user_addr;
+ break;
+ case VHOST_SET_VRING_KICK:
+ r = copy_from_user(&f, argp, sizeof f);
+ if (r < 0)
+ break;
+ eventfp = f.fd == -1 ? NULL : eventfd_fget(f.fd);
+ if (IS_ERR(eventfp))
+ return PTR_ERR(eventfp);
+ if (eventfp != vq->kick) {
+ pollstop = filep = vq->kick;
+ pollstart = vq->kick = eventfp;
+ } else
+ filep = eventfp;
+ break;
+ case VHOST_SET_VRING_CALL:
+ r = copy_from_user(&f, argp, sizeof f);
+ if (r < 0)
+ break;
+ eventfp = f.fd == -1 ? NULL : eventfd_fget(f.fd);
+ if (IS_ERR(eventfp))
+ return PTR_ERR(eventfp);
+ if (eventfp != vq->call) {
+ filep = vq->call;
+ ctx = vq->call_ctx;
+ vq->call = eventfp;
+ vq->call_ctx = eventfp ?
+ eventfd_ctx_fileget(eventfp) : NULL;
+ } else
+ filep = eventfp;
+ break;
+ case VHOST_SET_VRING_ERR:
+ r = copy_from_user(&f, argp, sizeof f);
+ if (r < 0)
+ break;
+ eventfp = f.fd == -1 ? NULL : eventfd_fget(f.fd);
+ if (IS_ERR(eventfp))
+ return PTR_ERR(eventfp);
+ if (eventfp != vq->error) {
+ filep = vq->error;
+ vq->error = eventfp;
+ ctx = vq->error_ctx;
+ vq->error_ctx = eventfp ?
+ eventfd_ctx_fileget(eventfp) : NULL;
+ } else
+ filep = eventfp;
+ break;
+ default:
+ r = -ENOIOCTLCMD;
+ }
+
+ if (pollstop && vq->handle_kick)
+ vhost_poll_stop(&vq->poll);
+
+ if (ctx)
+ eventfd_ctx_put(ctx);
+ if (filep)
+ fput(filep);
+
+ if (pollstart && vq->handle_kick)
+ vhost_poll_start(&vq->poll, vq->kick);
+
+ mutex_unlock(&vq->mutex);
+
+ if (pollstop && vq->handle_kick)
+ vhost_poll_flush(&vq->poll);
+ return r;
+}
+
+long vhost_dev_ioctl(struct vhost_dev *d, unsigned int ioctl, unsigned long arg)
+{
+ void __user *argp = (void __user *)arg;
+ struct file *eventfp, *filep = NULL;
+ struct eventfd_ctx *ctx = NULL;
+ u64 p;
+ long r;
+ int i, fd;
+
+ mutex_lock(&d->mutex);
+ /* If you are not the owner, you can become one */
+ if (ioctl == VHOST_SET_OWNER) {
+ r = vhost_dev_set_owner(d);
+ goto done;
+ }
+
+ /* You must be the owner to do anything else */
+ r = vhost_dev_check_owner(d);
+ if (r)
+ goto done;
+
+ switch (ioctl) {
+ case VHOST_SET_MEM_TABLE:
+ r = vhost_set_memory(d, argp);
+ break;
+ case VHOST_SET_LOG_BASE:
+ r = get_user(p, (u64 __user *)argp);
+ if (r < 0)
+ break;
+ if ((u64)(unsigned long)p != p) {
+ r = -EFAULT;
+ break;
+ }
+ for (i = 0; i < d->nvqs; ++i) {
+ mutex_lock(&d->vqs[i].mutex);
+ d->vqs[i].log_base = (void __user *)(unsigned long)p;
+ mutex_unlock(&d->vqs[i].mutex);
+ }
+ break;
+ case VHOST_SET_LOG_FD:
+ r = get_user(fd, (int __user *)argp);
+ if (r < 0)
+ break;
+ eventfp = fd == -1 ? NULL : eventfd_fget(fd);
+ if (IS_ERR(eventfp)) {
+ r = PTR_ERR(eventfp);
+ break;
+ }
+ if (eventfp != d->log_file) {
+ filep = d->log_file;
+ ctx = d->log_ctx;
+ d->log_ctx = eventfp ?
+ eventfd_ctx_fileget(eventfp) : NULL;
+ } else
+ filep = eventfp;
+ for (i = 0; i < d->nvqs; ++i) {
+ mutex_lock(&d->vqs[i].mutex);
+ d->vqs[i].log_ctx = d->log_ctx;
+ mutex_unlock(&d->vqs[i].mutex);
+ }
+ if (ctx)
+ eventfd_ctx_put(ctx);
+ if (filep)
+ fput(filep);
+ break;
+ default:
+ r = vhost_set_vring(d, ioctl, argp);
+ break;
+ }
+done:
+ mutex_unlock(&d->mutex);
+ return r;
+}
+
+static const struct vhost_memory_region *find_region(struct vhost_memory *mem,
+ __u64 addr, __u32 len)
+{
+ struct vhost_memory_region *reg;
+ int i;
+ /* linear search is not brilliant, but we really have on the order of 6
+ * regions in practice */
+ for (i = 0; i < mem->nregions; ++i) {
+ reg = mem->regions + i;
+ if (reg->guest_phys_addr <= addr &&
+ reg->guest_phys_addr + reg->memory_size - 1 >= addr)
+ return reg;
+ }
+ return NULL;
+}
+
+/* TODO: This is really inefficient. We need something like get_user()
+ * (instruction directly accesses the data, with an exception table entry
+ * returning -EFAULT). See Documentation/x86/exception-tables.txt.
+ */
+static int set_bit_to_user(int nr, void __user *addr)
+{
+ unsigned long log = (unsigned long)addr;
+ struct page *page;
+ void *base;
+ int bit = nr + (log % PAGE_SIZE) * 8;
+ int r;
+ r = get_user_pages_fast(log, 1, 1, &page);
+ if (r)
+ return r;
+ base = kmap_atomic(page, KM_USER0);
+ set_bit(bit, base);
+ kunmap_atomic(base, KM_USERO);
+ set_page_dirty_lock(page);
+ put_page(page);
+ return 0;
+}
+
+static int log_write(void __user *log_base,
+ u64 write_address, u64 write_length)
+{
+ int r;
+ if (!write_length)
+ return 0;
+ write_address /= VHOST_PAGE_SIZE;
+ for (;;) {
+ u64 base = (u64)log_base;
+ u64 log = base + write_address / 8;
+ int bit = write_address % 8;
+ if ((u64)(unsigned long)log != log)
+ return -EFAULT;
+ r = set_bit_to_user(bit, (void __user *)log);
+ if (r < 0)
+ return r;
+ if (write_length <= VHOST_PAGE_SIZE)
+ break;
+ write_length -= VHOST_PAGE_SIZE;
+ write_address += VHOST_PAGE_SIZE;
+ }
+ return r;
+}
+
+int vhost_log_write(struct vhost_virtqueue *vq, struct vhost_log *log,
+ unsigned int log_num, u64 len)
+{
+ int i, r;
+
+ /* Make sure data written is seen before log. */
+ wmb();
+ for (i = 0; i < log_num; ++i) {
+ u64 l = min(log[i].len, len);
+ r = log_write(vq->log_base, log[i].addr, l);
+ if (r < 0)
+ return r;
+ len -= l;
+ if (!len)
+ return 0;
+ }
+ if (vq->log_ctx)
+ eventfd_signal(vq->log_ctx, 1);
+ /* Length written exceeds what we have stored. This is a bug. */
+ BUG();
+ return 0;
+}
+
+int translate_desc(struct vhost_dev *dev, u64 addr, u32 len,
+ struct iovec iov[], int iov_size)
+{
+ const struct vhost_memory_region *reg;
+ struct vhost_memory *mem;
+ struct iovec *_iov;
+ u64 s = 0;
+ int ret = 0;
+
+ rcu_read_lock();
+
+ mem = rcu_dereference(dev->memory);
+ while ((u64)len > s) {
+ u64 size;
+ if (ret >= iov_size) {
+ ret = -ENOBUFS;
+ break;
+ }
+ reg = find_region(mem, addr, len);
+ if (!reg) {
+ ret = -EFAULT;
+ break;
+ }
+ _iov = iov + ret;
+ size = reg->memory_size - addr + reg->guest_phys_addr;
+ _iov->iov_len = min((u64)len, size);
+ _iov->iov_base = (void *)
+ (reg->userspace_addr + addr - reg->guest_phys_addr);
+ s += size;
+ addr += size;
+ ++ret;
+ }
+
+ rcu_read_unlock();

+ return ret;
+}
+

+/* Each buffer in the virtqueues is actually a chain of descriptors. This
+ * function returns the next descriptor in the chain,
+ * or -1 if we're at the end. */
+static unsigned next_desc(struct vring_desc *desc)
+{
+ unsigned int next;
+
+ /* If this descriptor says it doesn't chain, we're done. */
+ if (!(desc->flags & VRING_DESC_F_NEXT))
+ return -1;
+
+ /* Check they're not leading us off end of descriptors. */
+ next = desc->next;
+ /* Make sure compiler knows to grab that: we don't want it changing! */
+ /* We will use the result as an index in an array, so most
+ * architectures only need a compiler barrier here. */
+ read_barrier_depends();
+
+ return next;
+}
+
+static unsigned get_indirect(struct vhost_dev *dev, struct vhost_virtqueue *vq,
+ struct iovec iov[],
+ unsigned int *out_num, unsigned int *in_num,
+ struct vhost_log *log, unsigned int *log_num,
+ struct vring_desc *indirect)
+{
+ struct vring_desc desc;
+ unsigned int i = 0, count, found = 0;
+ int ret;
+
+ /* Sanity check */
+ if (indirect->len % sizeof desc) {
+ vq_err(vq, "Invalid length in indirect descriptor: "
+ "len 0x%llx not multiple of 0x%zx\n",
+ (unsigned long long)indirect->len,
+ sizeof desc);
+ return -EINVAL;
+ }
+
+ ret = translate_desc(dev, indirect->addr, indirect->len, vq->indirect,
+ ARRAY_SIZE(vq->indirect));
+ if (ret < 0) {
+ vq_err(vq, "Translation failure %d in indirect.\n", ret);

+ return ret;
+ }
+

+ /* We will use the result as an address to read from, so most
+ * architectures only need a compiler barrier here. */
+ read_barrier_depends();
+
+ count = indirect->len / sizeof desc;
+ /* Buffers are chained via a 16 bit next field, so
+ * we can have at most 2^16 of these. */
+ if (count > 0x10000) {
+ vq_err(vq, "Indirect buffer length too big: %d\n",
+ indirect->len);
+ return -E2BIG;
+ }
+
+ do {
+ unsigned iov_count = *in_num + *out_num;
+ if (++found > count) {
+ vq_err(vq, "Loop detected: last one at %u "
+ "indirect size %u\n",
+ i, count);
+ return -EINVAL;
+ }
+ if (memcpy_fromiovec((unsigned char *)&desc, vq->indirect,
+ sizeof desc)) {;
+ vq_err(vq, "Failed indirect descriptor: idx %d, %zx\n",
+ i, (size_t)indirect->addr + i * sizeof desc);
+ return -EINVAL;
+ }
+ if (desc.flags & VRING_DESC_F_INDIRECT) {
+ vq_err(vq, "Nested indirect descriptor: idx %d, %zx\n",
+ i, (size_t)indirect->addr + i * sizeof desc);
+ return -EINVAL;
+ }
+
+ ret = translate_desc(dev, desc.addr, desc.len, iov + iov_count,
+ VHOST_NET_MAX_SG - iov_count);
+ if (ret < 0) {
+ vq_err(vq, "Translation failure %d indirect idx %d\n",
+ ret, i);
+ return ret;
+ }
+ /* If this is an input descriptor, increment that count. */
+ if (desc.flags & VRING_DESC_F_WRITE) {
+ *in_num += ret;
+ if (unlikely(log)) {
+ log[*log_num].addr = desc.addr;
+ log[*log_num].len = desc.len;
+ ++*log_num;
+ }
+ } else {
+ /* If it's an output descriptor, they're all supposed
+ * to come before any input descriptors. */
+ if (*in_num) {
+ vq_err(vq, "Indirect descriptor "
+ "has out after in: idx %d\n", i);
+ return -EINVAL;
+ }
+ *out_num += ret;
+ }
+ } while ((i = next_desc(&desc)) != -1);
+ return 0;
+}
+
+/* This looks in the virtqueue and for the first available buffer, and converts
+ * it to an iovec for convenient access. Since descriptors consist of some
+ * number of output then some number of input descriptors, it's actually two
+ * iovecs, but we pack them into one and note how many of each there were.
+ *
+ * This function returns the descriptor number found, or vq->num (which
+ * is never a valid descriptor number) if none was found. */
+unsigned vhost_get_vq_desc(struct vhost_dev *dev, struct vhost_virtqueue *vq,
+ struct iovec iov[],
+ unsigned int *out_num, unsigned int *in_num,
+ struct vhost_log *log, unsigned int *log_num)
+{
+ struct vring_desc desc;
+ unsigned int i, head, found = 0;
+ u16 last_avail_idx;
+ int ret;
+
+ /* Check it isn't doing very strange things with descriptor numbers. */
+ last_avail_idx = vq->last_avail_idx;
+ if (get_user(vq->avail_idx, &vq->avail->idx)) {
+ vq_err(vq, "Failed to access avail idx at %p\n",
+ &vq->avail->idx);
+ return vq->num;
+ }
+
+ if ((u16)(vq->avail_idx - last_avail_idx) > vq->num) {
+ vq_err(vq, "Guest moved used index from %u to %u",
+ last_avail_idx, vq->avail_idx);
+ return vq->num;
+ }
+
+ /* If there's nothing new since last we looked, return invalid. */
+ if (vq->avail_idx == last_avail_idx)
+ return vq->num;
+
+ /* Only get avail ring entries after they have been exposed by guest. */
+ rmb();
+
+ /* Grab the next descriptor number they're advertising, and increment
+ * the index we've seen. */
+ if (get_user(head, &vq->avail->ring[last_avail_idx % vq->num])) {
+ vq_err(vq, "Failed to read head: idx %d address %p\n",
+ last_avail_idx,
+ &vq->avail->ring[last_avail_idx % vq->num]);
+ return vq->num;
+ }
+
+ /* If their number is silly, that's an error. */
+ if (head >= vq->num) {
+ vq_err(vq, "Guest says index %u > %u is available",
+ head, vq->num);
+ return vq->num;
+ }
+
+ /* When we start there are none of either input nor output. */
+ *out_num = *in_num = 0;
+ if (unlikely(log))
+ *log_num = 0;
+
+ i = head;
+ do {
+ unsigned iov_count = *in_num + *out_num;
+ if (i >= vq->num) {
+ vq_err(vq, "Desc index is %u > %u, head = %u",
+ i, vq->num, head);
+ return vq->num;
+ }
+ if (++found > vq->num) {
+ vq_err(vq, "Loop detected: last one at %u "
+ "vq size %u head %u\n",
+ i, vq->num, head);
+ return vq->num;
+ }
+ ret = copy_from_user(&desc, vq->desc + i, sizeof desc);
+ if (ret) {
+ vq_err(vq, "Failed to get descriptor: idx %d addr %p\n",
+ i, vq->desc + i);
+ return vq->num;
+ }
+ if (desc.flags & VRING_DESC_F_INDIRECT) {
+ ret = get_indirect(dev, vq, iov, out_num, in_num,
+ log, log_num, &desc);
+ if (ret < 0) {
+ vq_err(vq, "Failure detected "
+ "in indirect descriptor at idx %d\n", i);
+ return vq->num;
+ }
+ continue;
+ }
+
+ ret = translate_desc(dev, desc.addr, desc.len, iov + iov_count,
+ VHOST_NET_MAX_SG - iov_count);
+ if (ret < 0) {
+ vq_err(vq, "Translation failure %d descriptor idx %d\n",
+ ret, i);
+ return vq->num;
+ }
+ if (desc.flags & VRING_DESC_F_WRITE) {
+ /* If this is an input descriptor, increment that count. */
+ *in_num += ret;
+ if (unlikely(log)) {
+ log[*log_num].addr = desc.addr;
+ log[*log_num].len = desc.len;
+ ++*log_num;
+ }
+ } else {
+ /* If it's an output descriptor, they're all supposed
+ * to come before any input descriptors. */
+ if (*in_num) {
+ vq_err(vq, "Descriptor has out after in: "
+ "idx %d\n", i);
+ return vq->num;
+ }
+ *out_num += ret;
+ }
+ } while ((i = next_desc(&desc)) != -1);
+
+ /* On success, increment avail index. */
+ vq->last_avail_idx++;
+ return head;
+}
+
+/* Reverse the effect of vhost_get_vq_desc. Useful for error handling. */
+void vhost_discard_vq_desc(struct vhost_virtqueue *vq)
+{
+ vq->last_avail_idx--;
+}
+
+/* After we've used one of their buffers, we tell them about it. We'll then
+ * want to send them an interrupt, using vq->call. */
+int vhost_add_used(struct vhost_virtqueue *vq,
+ unsigned int head, int len)
+{
+ struct vring_used_elem *used;
+
+ /* The virtqueue contains a ring of used buffers. Get a pointer to the
+ * next entry in that used ring. */
+ used = &vq->used->ring[vq->last_used_idx % vq->num];
+ if (put_user(head, &used->id)) {
+ vq_err(vq, "Failed to write used id");
+ return -EFAULT;
+ }
+ if (put_user(len, &used->len)) {
+ vq_err(vq, "Failed to write used len");
+ return -EFAULT;
+ }
+ /* Make sure buffer is written before we update index. */
+ wmb();
+ if (put_user(vq->last_used_idx + 1, &vq->used->idx)) {
+ vq_err(vq, "Failed to increment used idx");
+ return -EFAULT;
+ }
+ if (unlikely(vq->log_used)) {
+ /* Make sure data is seen before log. */
+ wmb();
+ log_write(vq->log_base, vq->log_addr + sizeof *vq->used->ring
+ * (vq->last_used_idx % vq->num), sizeof *vq->used->ring);
+ log_write(vq->log_base, vq->log_addr, sizeof *vq->used->ring);
+ if (vq->log_ctx)
+ eventfd_signal(vq->log_ctx, 1);
+ }
+ vq->last_used_idx++;
+ return 0;
+}
+
+/* This actually sends the interrupt for this virtqueue */
+void vhost_trigger_irq(struct vhost_dev *dev, struct vhost_virtqueue *vq)
+{
+ __u16 flags = 0;
+ if (get_user(flags, &vq->avail->flags)) {
+ vq_err(vq, "Failed to get flags");
+ return;
+ }
+
+ /* If they don't want an interrupt, don't send one, unless empty. */
+ if ((flags & VRING_AVAIL_F_NO_INTERRUPT) &&
+ (vq->avail_idx != vq->last_avail_idx ||
+ !vhost_has_feature(dev, VIRTIO_F_NOTIFY_ON_EMPTY)))
+ return;
+
+ /* Send the Guest an interrupt tell them we used something up. */
+ if (vq->call_ctx)
+ eventfd_signal(vq->call_ctx, 1);
+}
+
+/* And here's the combo meal deal. Supersize me! */
+void vhost_add_used_and_trigger(struct vhost_dev *dev,
+ struct vhost_virtqueue *vq,
+ unsigned int head, int len)
+{
+ vhost_add_used(vq, head, len);
+ vhost_trigger_irq(dev, vq);
+}
+
+/* OK, now we need to know about added descriptors. */
+bool vhost_notify(struct vhost_virtqueue *vq)
+{
+ int r;
+ if (!(vq->used_flags & VRING_USED_F_NO_NOTIFY))
+ return false;
+ vq->used_flags &= ~VRING_USED_F_NO_NOTIFY;
+ r = put_user(vq->used_flags, &vq->used->flags);
+ if (r)
+ vq_err(vq, "Failed to disable notification: %d\n", r);
+ /* They could have slipped one in as we were doing that: make
+ * sure it's written, tell caller it needs to check again. */
+ mb();
+ return true;
+}
+
+/* We don't need to be notified again. */
+void vhost_no_notify(struct vhost_virtqueue *vq)
+{
+ int r;
+ if (vq->used_flags & VRING_USED_F_NO_NOTIFY)
+ return;
+ vq->used_flags |= VRING_USED_F_NO_NOTIFY;
+ r = put_user(vq->used_flags, &vq->used->flags);
+ if (r)
+ vq_err(vq, "Failed to enable notification: %d\n", r);
+}
+
+int vhost_init(void)
+{
+ vhost_workqueue = create_singlethread_workqueue("vhost");
+ if (!vhost_workqueue)
+ return -ENOMEM;
+ return 0;
+}
+
+void vhost_cleanup(void)
+{
+ destroy_workqueue(vhost_workqueue);
+}
diff --git a/drivers/vhost/vhost.h b/drivers/vhost/vhost.h
new file mode 100644
index 0000000..ae66b5e
--- /dev/null
+++ b/drivers/vhost/vhost.h
@@ -0,0 +1,158 @@
+#ifndef _VHOST_H
+#define _VHOST_H
+
+#include <linux/eventfd.h>
+#include <linux/vhost.h>
+#include <linux/mm.h>
+#include <linux/mutex.h>
+#include <linux/workqueue.h>
+#include <linux/poll.h>
+#include <linux/file.h>
+#include <linux/skbuff.h>
+#include <linux/uio.h>
+#include <linux/virtio_config.h>
+#include <linux/virtio_ring.h>
+
+struct vhost_device;
+
+enum {
+ VHOST_NET_MAX_SG = MAX_SKB_FRAGS + 2,
+};
+
+/* Poll a file (eventfd or socket) */
+/* Note: there's nothing vhost specific about this structure. */
+struct vhost_poll {
+ poll_table table;
+ wait_queue_head_t *wqh;
+ wait_queue_t wait;
+ /* struct which will handle all actual work. */
+ struct work_struct work;
+ unsigned long mask;
+};
+
+void vhost_poll_init(struct vhost_poll *poll, work_func_t func,
+ unsigned long mask);
+void vhost_poll_start(struct vhost_poll *poll, struct file *file);
+void vhost_poll_stop(struct vhost_poll *poll);
+void vhost_poll_flush(struct vhost_poll *poll);
+void vhost_poll_queue(struct vhost_poll *poll);
+
+struct vhost_log {
+ u64 addr;
+ u64 len;
+};
+
+/* The virtqueue structure describes a queue attached to a device. */
+struct vhost_virtqueue {
+ struct vhost_dev *dev;
+
+ /* The actual ring of buffers. */
+ struct mutex mutex;
+ unsigned int num;
+ struct vring_desc __user *desc;
+ struct vring_avail __user *avail;
+ struct vring_used __user *used;
+ struct file *kick;
+ struct file *call;
+ struct file *error;
+ struct eventfd_ctx *call_ctx;
+ struct eventfd_ctx *error_ctx;
+ struct eventfd_ctx *log_ctx;
+
+ struct vhost_poll poll;
+
+ /* The routine to call when the Guest pings us, or timeout. */
+ work_func_t handle_kick;
+
+ /* Last available index we saw. */
+ u16 last_avail_idx;
+
+ /* Caches available index value from user. */
+ u16 avail_idx;
+
+ /* Last index we used. */
+ u16 last_used_idx;
+
+ /* Used flags */
+ u16 used_flags;
+
+ /* Log writes to used structure. */
+ bool log_used;
+ u64 log_addr;
+
+ struct iovec indirect[VHOST_NET_MAX_SG];
+ struct iovec iov[VHOST_NET_MAX_SG];
+ struct iovec hdr[VHOST_NET_MAX_SG];
+ size_t hdr_size;
+ /* We use a kind of RCU to access private pointer.
+ * All readers access it from workqueue, which makes it possible to
+ * flush the workqueue instead of synchronize_rcu. Therefore readers do
+ * not need to call rcu_read_lock/rcu_read_unlock: the beginning of
+ * work item execution acts instead of rcu_read_lock() and the end of
+ * work item execution acts instead of rcu_read_lock().
+ * Writers use virtqueue mutex. */
+ void *private_data;
+ /* Log write descriptors */
+ void __user *log_base;
+ struct vhost_log log[VHOST_NET_MAX_SG];
+};
+
+struct vhost_dev {
+ /* Readers use RCU to access memory table pointer
+ * log base pointer and features.
+ * Writers use mutex below.*/
+ struct vhost_memory *memory;
+ struct mm_struct *mm;
+ struct mutex mutex;
+ unsigned acked_features;
+ struct vhost_virtqueue *vqs;
+ int nvqs;
+ struct file *log_file;
+ struct eventfd_ctx *log_ctx;
+};
+
+long vhost_dev_init(struct vhost_dev *, struct vhost_virtqueue *vqs, int nvqs);
+long vhost_dev_check_owner(struct vhost_dev *);
+long vhost_dev_reset_owner(struct vhost_dev *);
+void vhost_dev_cleanup(struct vhost_dev *);
+long vhost_dev_ioctl(struct vhost_dev *, unsigned int ioctl, unsigned long arg);
+
+unsigned vhost_get_vq_desc(struct vhost_dev *, struct vhost_virtqueue *,
+ struct iovec iov[],
+ unsigned int *out_num, unsigned int *in_num,
+ struct vhost_log *log, unsigned int *log_num);
+void vhost_discard_vq_desc(struct vhost_virtqueue *);
+
+int vhost_add_used(struct vhost_virtqueue *, unsigned int head, int len);
+void vhost_trigger_irq(struct vhost_dev *, struct vhost_virtqueue *);
+void vhost_add_used_and_trigger(struct vhost_dev *, struct vhost_virtqueue *,
+ unsigned int head, int len);
+void vhost_no_notify(struct vhost_virtqueue *);
+bool vhost_notify(struct vhost_virtqueue *);
+
+int vhost_log_write(struct vhost_virtqueue *vq, struct vhost_log *log,
+ unsigned int log_num, u64 len);
+
+int vhost_init(void);
+void vhost_cleanup(void);
+
+#define vq_err(vq, fmt, ...) do { \
+ pr_debug(pr_fmt(fmt), ##__VA_ARGS__); \
+ if ((vq)->error_ctx) \
+ eventfd_signal((vq)->error_ctx, 1);\
+ } while (0)
+
+enum {
+ VHOST_FEATURES = (1 << VIRTIO_F_NOTIFY_ON_EMPTY) |
+ (1 << VIRTIO_RING_F_INDIRECT_DESC) |
+ (1 << VHOST_F_LOG_ALL) |
+ (1 << VHOST_NET_F_VIRTIO_NET_HDR),
+};
+
+static inline int vhost_has_feature(struct vhost_dev *dev, int bit)
+{
+ unsigned acked_features = rcu_dereference(dev->acked_features);
+ return acked_features & (1 << bit);
+}
+
+#endif
diff --git a/include/linux/Kbuild b/include/linux/Kbuild
index 3f384d4..b6335c6 100644
--- a/include/linux/Kbuild
+++ b/include/linux/Kbuild
@@ -361,6 +361,7 @@ unifdef-y += uio.h
unifdef-y += unistd.h
unifdef-y += usbdevice_fs.h
unifdef-y += utsname.h
+unifdef-y += vhost.h
unifdef-y += videodev2.h
unifdef-y += videodev.h
unifdef-y += virtio_config.h
diff --git a/include/linux/miscdevice.h b/include/linux/miscdevice.h
index adaf3c1..8b5f7cc 100644
--- a/include/linux/miscdevice.h
+++ b/include/linux/miscdevice.h
@@ -30,6 +30,7 @@
#define HPET_MINOR 228
#define FUSE_MINOR 229
#define KVM_MINOR 232
+#define VHOST_NET_MINOR 233
#define MISC_DYNAMIC_MINOR 255

struct device;
diff --git a/include/linux/vhost.h b/include/linux/vhost.h
new file mode 100644
index 0000000..f66142f
--- /dev/null
+++ b/include/linux/vhost.h
@@ -0,0 +1,126 @@
+#ifndef _LINUX_VHOST_H
+#define _LINUX_VHOST_H
+/* Userspace interface for in-kernel virtio accelerators. */
+
+/* vhost is used to reduce the number of system calls involved in virtio.
+ *
+ * Existing virtio net code is used in the guest without modification.
+ *
+ * This header includes interface used by userspace hypervisor for
+ * device configuration.
+ */
+
+#include <linux/types.h>
+#include <linux/compiler.h>
+#include <linux/ioctl.h>
+#include <linux/virtio_config.h>
+#include <linux/virtio_ring.h>
+
+struct vhost_vring_state {
+ unsigned int index;
+ unsigned int num;
+};
+
+struct vhost_vring_file {
+ unsigned int index;
+ int fd; /* Pass -1 to unbind from file. */
+
+};
+
+struct vhost_vring_addr {
+ unsigned int index;
+ unsigned int padding;
+ __u64 user_addr;
+};
+
+struct vhost_memory_region {
+ __u64 guest_phys_addr;
+ __u64 memory_size; /* bytes */
+ __u64 userspace_addr;
+ __u64 flags_padding; /* No flags are currently specified. */
+};
+
+/* All region addresses and sizes must be 4K aligned. */
+#define VHOST_PAGE_SIZE 0x1000
+
+struct vhost_memory {
+ __u32 nregions;
+ __u32 padding;
+ struct vhost_memory_region regions[0];
+};
+
+/* ioctls */
+
+#define VHOST_VIRTIO 0xAF
+
+/* Features bitmask for forward compatibility. Transport bits are used for
+ * vhost specific features. */
+#define VHOST_GET_FEATURES _IOR(VHOST_VIRTIO, 0x00, __u64)
+#define VHOST_ACK_FEATURES _IOW(VHOST_VIRTIO, 0x00, __u64)
+
+/* Set current process as the (exclusive) owner of this file descriptor. This
+ * must be called before any other vhost command. Further calls to
+ * VHOST_OWNER_SET fail until VHOST_OWNER_RESET is called. */
+#define VHOST_SET_OWNER _IO(VHOST_VIRTIO, 0x01)
+/* Give up ownership, and reset the device to default values.
+ * Allows subsequent call to VHOST_OWNER_SET to succeed. */
+#define VHOST_RESET_OWNER _IO(VHOST_VIRTIO, 0x02)
+
+/* Set up/modify memory layout */
+#define VHOST_SET_MEM_TABLE _IOW(VHOST_VIRTIO, 0x03, struct vhost_memory)
+
+/* Write logging setup. */
+/* Memory writes can optionally be logged by setting bit at an offset
+ * (calculated from the physical address) from specified log base.
+ * The bit is set using an atomic 32 bit operation. */
+/* Set base address for logging. */
+#define VHOST_SET_LOG_BASE _IOW(VHOST_VIRTIO, 0x04, __u64)
+/* Specify an eventfd file descriptor to signal on log write. */
+#define VHOST_SET_LOG_FD _IOW(VHOST_VIRTIO, 0x07, int)
+
+/* Ring setup. These parameters can not be modified while ring is running
+ * (bound to a device). */
+/* Set number of descriptors in ring */
+#define VHOST_SET_VRING_NUM _IOW(VHOST_VIRTIO, 0x10, struct vhost_vring_state)
+/* Start of array of descriptors (virtually contiguous) */
+#define VHOST_SET_VRING_DESC _IOW(VHOST_VIRTIO, 0x11, struct vhost_vring_addr)
+/* Used structure address. Must be 32 bit aligned */
+#define VHOST_SET_VRING_USED _IOW(VHOST_VIRTIO, 0x12, struct vhost_vring_addr)
+/* Available structure address. Must be 16 bit aligned */
+#define VHOST_SET_VRING_AVAIL _IOW(VHOST_VIRTIO, 0x13, struct vhost_vring_addr)
+/* Base value where queue looks for available descriptors */
+#define VHOST_SET_VRING_BASE _IOW(VHOST_VIRTIO, 0x14, struct vhost_vring_state)
+/* Get accessor: reads index, writes value in num */
+#define VHOST_GET_VRING_BASE _IOWR(VHOST_VIRTIO, 0x14, struct vhost_vring_state)
+
+/* Logging support. Can be modified while ring is running. */
+/* Log writes to used structure, at offset calculated from specified address.
+ * Address must be 32 bit aligned. Pass 0x1 to disable logging. */
+#define VHOST_SET_VRING_LOG _IOW(VHOST_VIRTIO, 0x18, struct vhost_vring_addr)
+#define VHOST_VRING_LOG_DISABLE (0x1)
+
+/* The following ioctls use eventfd file descriptors to signal and poll
+ * for events. */
+
+/* Set eventfd to poll for added buffers */
+#define VHOST_SET_VRING_KICK _IOW(VHOST_VIRTIO, 0x20, struct vhost_vring_file)
+/* Set eventfd to signal when buffers have beed used */
+#define VHOST_SET_VRING_CALL _IOW(VHOST_VIRTIO, 0x21, struct vhost_vring_file)
+/* Set eventfd to signal an error */
+#define VHOST_SET_VRING_ERR _IOW(VHOST_VIRTIO, 0x22, struct vhost_vring_file)
+
+/* VHOST_NET specific defines */
+
+/* Attach virtio net ring to a raw socket, or tap device.
+ * The socket must be already bound to an ethernet device, this device will be
+ * used for transmit. Pass fd -1 to unbind from the socket and the transmit
+ * device. This can be used to stop the ring (e.g. for migration). */
+#define VHOST_NET_SET_BACKEND _IOW(VHOST_VIRTIO, 0x30, struct vhost_vring_file)
+
+/* Feature bits */
+/* Log all write descriptors. Can be changed while device is active. */
+#define VHOST_F_LOG_ALL 26
+/* vhost-net should add virtio_net_hdr for RX, and strip for TX packets. */
+#define VHOST_NET_F_VIRTIO_NET_HDR 27
+
+#endif

[Daniel Walker]

Random style issues below .. Part of this is just stuff checkpatch
found.

On Tue, 2009-11-03 at 00:29 +0200, Michael S. Tsirkin wrote:

> +static int move_iovec_hdr(struct iovec *from, struct iovec *to,
> + size_t len, int iov_count)
> +{
> + int seg = 0;
> + size_t size;
> + while (len && seg < iov_count) {
> + size = min(from->iov_len, len);
> + to->iov_base = from->iov_base;
> + to->iov_len = size;
> + from->iov_len -= size;
> + from->iov_base += size;
> + len -= size;
> + ++from;
> + ++to;
> + ++seg;
> + }
> + return seg;
> +}

The code above isn't properly indented .. You need to use tabs.

Need tabs for the line above.

> + "len 0x%llx not multiple of 0x%zx\n",
> + (unsigned long long)indirect->len,
> + sizeof desc);
> + return -EINVAL;
> + }
> +
> + ret = translate_desc(dev, indirect->addr, indirect->len, vq->indirect,
> + ARRAY_SIZE(vq->indirect));
> + if (ret < 0) {
> + vq_err(vq, "Translation failure %d in indirect.\n", ret);
> + return ret;
> + }
> +
> + /* We will use the result as an address to read from, so most
> + * architectures only need a compiler barrier here. */
> + read_barrier_depends();
> +
> + count = indirect->len / sizeof desc;
> + /* Buffers are chained via a 16 bit next field, so
> + * we can have at most 2^16 of these. */
> + if (count > 0x10000) {

Could you wrap that constant in a macro?

Line above needs more tabs.

Daniel

[Michael S. Tsirkin]

On Mon, Nov 02, 2009 at 04:05:58PM -0800, Daniel Walker wrote:
>
> Random style issues below .. Part of this is just stuff checkpatch
> found.

Thanks very much, I'll fix these.

[Arnd Bergmann]

On Monday 02 November 2009, Michael S. Tsirkin wrote:
> Tun device looks similar to a packet socket
> in that both pass complete frames from/to userspace.
>
> This patch fills in enough fields in the socket underlying tun driver
> to support sendmsg/recvmsg operations, and message flags
> MSG_TRUNC and MSG_DONTWAIT, and exports access to this socket
> to modules. Regular read/write behaviour is unchanged.
>
> This way, code using raw sockets to inject packets
> into a physical device, can support injecting
> packets into host network stack almost without modification.
>
> First user of this interface will be vhost virtualization
> accelerator.

You mentioned before that you wanted to export the socket
using some ioctl function returning an open file descriptor,
which seemed to be a cleaner approach than this one.

What was your reason for changing?

> index 3f5fd52..404abe0 100644
> --- a/include/linux/if_tun.h
> +++ b/include/linux/if_tun.h
> @@ -86,4 +86,18 @@ struct tun_filter {
> __u8 addr[0][ETH_ALEN];
> };
>
> +#ifdef __KERNEL__
> +#if defined(CONFIG_TUN) || defined(CONFIG_TUN_MODULE)
> +struct socket *tun_get_socket(struct file *);
> +#else
> +#include <linux/err.h>
> +#include <linux/errno.h>
> +struct file;
> +struct socket;
> +static inline struct socket *tun_get_socket(struct file *f)
> +{
> + return ERR_PTR(-EINVAL);
> +}
> +#endif /* CONFIG_TUN */
> +#endif /* __KERNEL__ */
> #endif /* __IF_TUN_H */

Is this a leftover from testing? Exporting the function for !__KERNEL__
seems pointless.

Arnd <><

[Michael S. Tsirkin]

On Tue, Nov 03, 2009 at 01:12:33PM +0100, Arnd Bergmann wrote:
> On Monday 02 November 2009, Michael S. Tsirkin wrote:
> > Tun device looks similar to a packet socket
> > in that both pass complete frames from/to userspace.
> >
> > This patch fills in enough fields in the socket underlying tun driver
> > to support sendmsg/recvmsg operations, and message flags
> > MSG_TRUNC and MSG_DONTWAIT, and exports access to this socket
> > to modules. Regular read/write behaviour is unchanged.
> >
> > This way, code using raw sockets to inject packets
> > into a physical device, can support injecting
> > packets into host network stack almost without modification.
> >
> > First user of this interface will be vhost virtualization
> > accelerator.
>
> You mentioned before that you wanted to export the socket
> using some ioctl function returning an open file descriptor,
> which seemed to be a cleaner approach than this one.

Note that a similar feature can be implemented on top of tun_get_socket,
as seen from patch below.

> What was your reason for changing?

It turns out socket structure is really bound to specific a file, so we
can not have 2 files referencing the same socket. Instead, as I say
above, it's possible to make sendmsg/recvmsg work on tap file directly.

For vhost, the advantage of such a feature over using tun_get_socket
directly would be that vhost module won't depend on tun module then. I
have implemented this (patch below), but decided to go with the simple
thing first. Since no userspace-visible changes are involved, let's do
this by small steps: it will be easier to figure out when vhost
is upstream.

---

Note: patch below aplies on top of patch tun: export underlying socket.
It is not intended for merge yet.

net: convert tun device to socket

Add callback to file_ops to retrieve socket from
file structure. Use this to make tun character device
accept sendmsg/recvmsg calls.

Signed-off-by: Michael S. Tsirkin <m...@redhat.com>

diff --git a/drivers/net/tun.c b/drivers/net/tun.c
index b58095a..53e1806 100644
--- a/drivers/net/tun.c
+++ b/drivers/net/tun.c
@@ -1405,7 +1405,8 @@ static const struct file_operations tun_fops = {
.unlocked_ioctl = tun_chr_ioctl,
.open = tun_chr_open,
.release = tun_chr_close,
- .fasync = tun_chr_fasync
+ .fasync = tun_chr_fasync,
+ .get_socket = tun_get_socket,
};

static struct miscdevice tun_miscdev = {
diff --git a/include/linux/fs.h b/include/linux/fs.h
index 2620a8c..f2b381f 100644
--- a/include/linux/fs.h
+++ b/include/linux/fs.h
@@ -1506,6 +1506,9 @@ struct file_operations {
ssize_t (*splice_write)(struct pipe_inode_info *, struct file *, loff_t *, size_t, unsigned int);
ssize_t (*splice_read)(struct file *, loff_t *, struct pipe_inode_info *, size_t, unsigned int);
int (*setlease)(struct file *, long, struct file_lock **);
+#ifdef CONFIG_NET
+ struct socket *(*get_socket)(struct file *file);
+#endif
};

struct inode_operations {
diff --git a/net/socket.c b/net/socket.c
index 9dff31c..700efcb 100644
--- a/net/socket.c
+++ b/net/socket.c
@@ -119,6 +119,11 @@ static ssize_t sock_splice_read(struct file *file, loff_t *ppos,
struct pipe_inode_info *pipe, size_t len,
unsigned int flags);

+static struct socket *sock_get_socket(struct file *file)
+{
+ return file->private_data; /* set in sock_map_fd */
+}
+
/*
* Socket files have a set of 'special' operations as well as the generic file ones. These don't appear
* in the operation structures but are done directly via the socketcall() multiplexor.
@@ -141,6 +146,7 @@ static const struct file_operations socket_file_ops = {
.sendpage = sock_sendpage,
.splice_write = generic_splice_sendpage,
.splice_read = sock_splice_read,
+ .get_socket = sock_get_socket,
};

/*
@@ -416,8 +422,8 @@ int sock_map_fd(struct socket *sock, int flags)

static struct socket *sock_from_file(struct file *file, int *err)
{
- if (file->f_op == &socket_file_ops)
- return file->private_data; /* set in sock_map_fd */
+ if (file->f_op->get_socket)
+ return file->f_op->get_socket(file);

*err = -ENOTSOCK;
return NULL;

--
MST

[Arnd Bergmann]

On Tuesday 03 November 2009, Michael S. Tsirkin wrote:
> > What was your reason for changing?
>
> It turns out socket structure is really bound to specific a file, so we
> can not have 2 files referencing the same socket. Instead, as I say
> above, it's possible to make sendmsg/recvmsg work on tap file directly.

Ah, I see.

> I have implemented this (patch below), but decided to go with the simple
> thing first. Since no userspace-visible changes are involved, let's do
> this by small steps: it will be easier to figure out when vhost
> is upstream.

This may even make it easier for me to do the same with macvtap
if I resume work on that.

> @@ -416,8 +422,8 @@ int sock_map_fd(struct socket *sock, int flags)
>
> static struct socket *sock_from_file(struct file *file, int *err)
> {
> - if (file->f_op == &socket_file_ops)
> - return file->private_data; /* set in sock_map_fd */
> + if (file->f_op->get_socket)
> + return file->f_op->get_socket(file);
>
> *err = -ENOTSOCK;

Or maybe do both (socket_file_ops and get_socket), to avoid an indirect
function call.

Arnd <><

[Michael S. Tsirkin]

Tun device looks similar to a packet socket
in that both pass complete frames from/to userspace.

This patch fills in enough fields in the socket underlying tun driver
to support sendmsg/recvmsg operations, and message flags
MSG_TRUNC and MSG_DONTWAIT, and exports access to this socket
to modules. Regular read/write behaviour is unchanged.

This way, code using raw sockets to inject packets
into a physical device, can support injecting
packets into host network stack almost without modification.

First user of this interface will be vhost virtualization
accelerator.

Acked-by: Herbert Xu <her...@gondor.apana.org.au>

Signed-off-by: Michael S. Tsirkin <m...@redhat.com>

---

This was posted to netdev separately as well.

Assuming it's okay with davem, I think it makes sense to merge this

patch through Rusty's tree because vhost is the first user of the new
interface. Posted here for completeness.

drivers/net/tun.c | 101 +++++++++++++++++++++++++++++++++++++++---------
include/linux/if_tun.h | 14 +++++++
2 files changed, 96 insertions(+), 19 deletions(-)

diff --git a/drivers/net/tun.c b/drivers/net/tun.c
index 4fdfa2a..18f8876 100644
--- a/drivers/net/tun.c
+++ b/drivers/net/tun.c

+ return ret;
+}
+

+static ssize_t tun_chr_aio_read(struct kiocb *iocb, const struct iovec *iv,
+ unsigned long count, loff_t pos)

+{

+ struct file *file = iocb->ki_filp;
+ struct tun_file *tfile = file->private_data;
+ struct tun_struct *tun = __tun_get(tfile);
+ ssize_t len, ret;
+
+ if (!tun)
+ return -EBADFD;
+ len = iov_length(iv, count);
+ if (len < 0) {
+ ret = -EINVAL;
+ goto out;

+ }
+

+ return ret;
+}
+

index 3f5fd52..404abe0 100644
--- a/include/linux/if_tun.h
+++ b/include/linux/if_tun.h
@@ -86,4 +86,18 @@ struct tun_filter {
__u8 addr[0][ETH_ALEN];
};

+#ifdef __KERNEL__
+#if defined(CONFIG_TUN) || defined(CONFIG_TUN_MODULE)
+struct socket *tun_get_socket(struct file *);
+#else
+#include <linux/err.h>
+#include <linux/errno.h>
+struct file;
+struct socket;
+static inline struct socket *tun_get_socket(struct file *f)
+{
+ return ERR_PTR(-EINVAL);
+}
+#endif /* CONFIG_TUN */
+#endif /* __KERNEL__ */
#endif /* __IF_TUN_H */

--
1.6.5.2.143.g8cc62

[Michael S. Tsirkin]

vhost net module wants to do copy to/from user from a kernel thread,
which needs use_mm. Export it to modules.

Acked-by: Andrea Arcangeli <aarc...@redhat.com>

Signed-off-by: Michael S. Tsirkin <m...@redhat.com>
---

mm/mmu_context.c | 3 +++
1 files changed, 3 insertions(+), 0 deletions(-)

diff --git a/mm/mmu_context.c b/mm/mmu_context.c
index ded9081..0777654 100644
--- a/mm/mmu_context.c
+++ b/mm/mmu_context.c
@@ -5,6 +5,7 @@

#include <linux/mm.h>
#include <linux/mmu_context.h>
+#include <linux/module.h>
#include <linux/sched.h>

#include <asm/mmu_context.h>
@@ -37,6 +38,7 @@ void use_mm(struct mm_struct *mm)
if (active_mm != mm)
mmdrop(active_mm);
}
+EXPORT_SYMBOL_GPL(use_mm);

/*
* unuse_mm
@@ -56,3 +58,4 @@ void unuse_mm(struct mm_struct *mm)
enter_lazy_tlb(mm, tsk);
task_unlock(tsk);
}
+EXPORT_SYMBOL_GPL(unuse_mm);

[Michael S. Tsirkin]

Acked-by: Arnd Bergmann <ar...@arndb.de>

Signed-off-by: Michael S. Tsirkin <m...@redhat.com>
---

MAINTAINERS | 9 +

arch/x86/kvm/Kconfig | 1 +
drivers/Makefile | 1 +
drivers/vhost/Kconfig | 11 +
drivers/vhost/Makefile | 2 +

drivers/vhost/net.c | 633 +++++++++++++++++++++++++++++
drivers/vhost/vhost.c | 970 ++++++++++++++++++++++++++++++++++++++++++++

drivers/vhost/vhost.h | 158 +++++++
include/linux/Kbuild | 1 +
include/linux/miscdevice.h | 1 +
include/linux/vhost.h | 126 ++++++
11 files changed, 1913 insertions(+), 0 deletions(-)
create mode 100644 drivers/vhost/Kconfig
create mode 100644 drivers/vhost/Makefile
create mode 100644 drivers/vhost/net.c
create mode 100644 drivers/vhost/vhost.c
create mode 100644 drivers/vhost/vhost.h
create mode 100644 include/linux/vhost.h

diff --git a/MAINTAINERS b/MAINTAINERS
index 8824115..980a69b 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -5619,6 +5619,15 @@ S: Maintained

new file mode 100644

index 0000000..d955406
--- /dev/null
+++ b/drivers/vhost/Kconfig
@@ -0,0 +1,11 @@
+config VHOST_NET
+ tristate "Host kernel accelerator for virtio net"
+ depends on NET && EVENTFD
+ ---help---
+ This kernel module can be loaded in host kernel to accelerate
+ guest networking with virtio_net. Not to be confused with virtio_net
+ module itself which needs to be loaded in guest kernel.
+
+ To compile this driver as a module, choose M here: the module will
+ be called vhost_net.
+
diff --git a/drivers/vhost/Makefile b/drivers/vhost/Makefile

new file mode 100644

index 0000000..72dd020
--- /dev/null
+++ b/drivers/vhost/Makefile
@@ -0,0 +1,2 @@
+obj-$(CONFIG_VHOST_NET) += vhost_net.o
+vhost_net-y := vhost.o net.o
diff --git a/drivers/vhost/net.c b/drivers/vhost/net.c

new file mode 100644
index 0000000..4af3b98
--- /dev/null
+++ b/drivers/vhost/net.c
@@ -0,0 +1,633 @@

+/* Copyright (C) 2009 Red Hat, Inc.

+ * Author: Michael S. Tsirkin <m...@redhat.com>
+ *

+ * This work is licensed under the terms of the GNU GPL, version 2.
+ *

+ * virtio-net server in host kernel.
+ */
+
+#include <linux/compat.h>

+#include <linux/eventfd.h>
+#include <linux/vhost.h>
+#include <linux/virtio_net.h>

+#include <linux/mmu_context.h>
+#include <linux/miscdevice.h>
+#include <linux/module.h>

+#include <linux/mutex.h>
+#include <linux/workqueue.h>
+#include <linux/rcupdate.h>

+#include <linux/file.h>

+
+#include <linux/net.h>
+#include <linux/if_packet.h>
+#include <linux/if_arp.h>

+#include <linux/if_tun.h>

+
+#include <net/sock.h>
+
+#include "vhost.h"
+

+static int move_iovec_hdr(struct iovec *from, struct iovec *to,
+ size_t len, int iov_count)
+{
+ int seg = 0;
+ size_t size;
+ while (len && seg < iov_count) {
+ size = min(from->iov_len, len);
+ to->iov_base = from->iov_base;
+ to->iov_len = size;
+ from->iov_len -= size;
+ from->iov_base += size;
+ len -= size;
+ ++from;
+ ++to;
+ ++seg;
+ }
+ return seg;
+}

+{

index 0000000..9e2ecc3
--- /dev/null
+++ b/drivers/vhost/vhost.c
@@ -0,0 +1,970 @@

+ kunmap_atomic(base, KM_USER0);

+ return ret;
+}
+

+ "len 0x%llx not multiple of 0x%zx\n",
+ (unsigned long long)indirect->len,
+ sizeof desc);
+ return -EINVAL;

+ }
+

+ ret = translate_desc(dev, indirect->addr, indirect->len, vq->indirect,
+ ARRAY_SIZE(vq->indirect));
+ if (ret < 0) {
+ vq_err(vq, "Translation failure %d in indirect.\n", ret);

+ return ret;
+ }
+

+ /* We will use the result as an address to read from, so most
+ * architectures only need a compiler barrier here. */
+ read_barrier_depends();
+
+ count = indirect->len / sizeof desc;
+ /* Buffers are chained via a 16 bit next field, so
+ * we can have at most 2^16 of these. */

+ if (count > USHORT_MAX + 1) {

+ vq_err(vq, "Indirect buffer length too big: %d\n",
+ indirect->len);
+ return -E2BIG;
+ }
+
+ do {
+ unsigned iov_count = *in_num + *out_num;
+ if (++found > count) {
+ vq_err(vq, "Loop detected: last one at %u "
+ "indirect size %u\n",
+ i, count);
+ return -EINVAL;
+ }
+ if (memcpy_fromiovec((unsigned char *)&desc, vq->indirect,
+ sizeof desc)) {
+ vq_err(vq, "Failed indirect descriptor: idx %d, %zx\n",
+ i, (size_t)indirect->addr + i * sizeof desc);
+ return -EINVAL;
+ }
+ if (desc.flags & VRING_DESC_F_INDIRECT) {
+ vq_err(vq, "Nested indirect descriptor: idx %d, %zx\n",
+ i, (size_t)indirect->addr + i * sizeof desc);
+ return -EINVAL;

+ }
+

+ ret = translate_desc(dev, desc.addr, desc.len, iov + iov_count,
+ VHOST_NET_MAX_SG - iov_count);
+ if (ret < 0) {
+ vq_err(vq, "Translation failure %d indirect idx %d\n",
+ ret, i);

+ return ret;
+ }

+ }
+

+ ret = translate_desc(dev, desc.addr, desc.len, iov + iov_count,
+ VHOST_NET_MAX_SG - iov_count);
+ if (ret < 0) {
+ vq_err(vq, "Translation failure %d descriptor idx %d\n",
+ ret, i);
+ return vq->num;
+ }
+ if (desc.flags & VRING_DESC_F_WRITE) {
+ /* If this is an input descriptor,

+ * increment that count. */

+ log_write(vq->log_base, vq->log_addr + sizeof *vq->used->ring *
+ (vq->last_used_idx % vq->num),
+ sizeof *vq->used->ring);

index 0000000..d3900de

new file mode 100644

index 0000000..f66142f
--- /dev/null
+++ b/include/linux/vhost.h
@@ -0,0 +1,126 @@
+#ifndef _LINUX_VHOST_H
+#define _LINUX_VHOST_H
+/* Userspace interface for in-kernel virtio accelerators. */
+
+/* vhost is used to reduce the number of system calls involved in virtio.
+ *
+ * Existing virtio net code is used in the guest without modification.

+ *

+ * This header includes interface used by userspace hypervisor for
+ * device configuration.
+ */
+
+#include <linux/types.h>
+#include <linux/compiler.h>
+#include <linux/ioctl.h>

+#include <linux/virtio_config.h>
+#include <linux/virtio_ring.h>
+

+struct vhost_vring_state {
+ unsigned int index;

+ unsigned int num;

[Gregory Haskins]

Michael S. Tsirkin wrote:
> vhost net module wants to do copy to/from user from a kernel thread,
> which needs use_mm. Export it to modules.
>
> Acked-by: Andrea Arcangeli <aarc...@redhat.com>
> Signed-off-by: Michael S. Tsirkin <m...@redhat.com>

I need this too:

Acked-by: Gregory Haskins <ghas...@novell.com>

[Eric Dumazet]

Michael S. Tsirkin a �crit :

using rcu_dereference() and mutex_lock() at the same time seems wrong, I suspect
that your use of RCU is not correct.

1) rcu_dereference() should be done inside a read_rcu_lock() section, and
we are not allowed to sleep in such a section.
(Quoting Documentation/RCU/whatisRCU.txt :
It is illegal to block while in an RCU read-side critical section, )

2) mutex_lock() can sleep (ie block)

[Gregory Haskins]

Eric Dumazet wrote:
> Michael S. Tsirkin a écrit :

Michael,
I warned you that this needed better documentation ;)

Eric,
I think I flagged this once before, but Michael convinced me that it
was indeed "ok", if but perhaps a bit unconventional. I will try to
find the thread.

Kind Regards,
-Greg

[Gregory Haskins]

Here it is:

http://lkml.org/lkml/2009/8/12/173

Kind Regards,
-Greg

[Eric Dumazet]

Gregory Haskins a �crit :

> Gregory Haskins wrote:
>> Eric Dumazet wrote:

>>> Michael S. Tsirkin a �crit :

Yes, this doesnt convince me at all, and could be a precedent for a wrong RCU use.
People wanting to use RCU do a grep on kernel sources to find how to correctly
use RCU.

Michael, please use existing locking/barrier mechanisms, and not pretend to use RCU.

Some automatic tools might barf later.

For example, we could add a debugging facility to check that rcu_dereference() is used
in an appropriate context, ie conflict with existing mutex_lock() debugging facility.

[Gregory Haskins]

Eric Dumazet wrote:
> Gregory Haskins a écrit :

>> Gregory Haskins wrote:
>>> Eric Dumazet wrote:

>>>> Michael S. Tsirkin a écrit :

>>>> using rcu_dereference() and mutex_lock() at the same time seems wrong, I suspect
>>>> that your use of RCU is not correct.
>>>>
>>>> 1) rcu_dereference() should be done inside a read_rcu_lock() section, and
>>>> we are not allowed to sleep in such a section.
>>>> (Quoting Documentation/RCU/whatisRCU.txt :
>>>> It is illegal to block while in an RCU read-side critical section, )
>>>>
>>>> 2) mutex_lock() can sleep (ie block)
>>>>
>>> Michael,
>>> I warned you that this needed better documentation ;)
>>>
>>> Eric,
>>> I think I flagged this once before, but Michael convinced me that it
>>> was indeed "ok", if but perhaps a bit unconventional. I will try to
>>> find the thread.
>>>
>>> Kind Regards,
>>> -Greg
>>>
>> Here it is:
>>
>> http://lkml.org/lkml/2009/8/12/173
>>
>
> Yes, this doesnt convince me at all, and could be a precedent for a wrong RCU use.
> People wanting to use RCU do a grep on kernel sources to find how to correctly
> use RCU.
>
> Michael, please use existing locking/barrier mechanisms, and not pretend to use RCU.

Yes, I would tend to agree with you. In fact, I think I suggested that
a normal barrier should be used instead of abusing rcu_dereference().

But as far as his code is concerned, I think it technically works
properly, and that was my main point. Also note that the usage
rcu_dereference+mutex_lock() are not necessarily broken, per se: it
could be an srcu-based critical section created by the caller, for
instance. It would be perfectly legal to sleep on the mutex if that
were the case.

To me, the bigger issue is that the rcu_dereference() without any
apparent hint of a corresponding RSCS is simply confusing as a reviewer.
smp_rmb() (or whatever is proper in this case) is probably more
appropriate.

Kind Regards,
-Greg

[Michael S. Tsirkin]

This use is correct. See comment in vhost.h This use of RCU has been
acked by Paul E. McKenney (pau...@linux.vnet.ibm.com) as well.
There are many ways to use RCU not all of which involve read_rcu_lock.

[Michael S. Tsirkin]

Paul, you acked this previously. Should I add you acked-by line so
people calm down? If you would rather I replace
rcu_dereference/rcu_assign_pointer with rmb/wmb, I can do this.
Or maybe patch Documentation to explain this RCU usage?

--
MST

[Eric Dumazet]

Michael S. Tsirkin a �crit :
>

> Paul, you acked this previously. Should I add you acked-by line so
> people calm down? If you would rather I replace
> rcu_dereference/rcu_assign_pointer with rmb/wmb, I can do this.
> Or maybe patch Documentation to explain this RCU usage?
>

So you believe I am over-reacting to this dubious use of RCU ?

RCU documentation is already very complex, we dont need to add yet another
subtle use, and makes it less readable.

It seems you use 'RCU api' in drivers/vhost/net.c as convenient macros :

#define rcu_dereference(p) ({ \
typeof(p) _________p1 = ACCESS_ONCE(p); \
smp_read_barrier_depends(); \
(_________p1); \
})

#define rcu_assign_pointer(p, v) \
({ \
if (!__builtin_constant_p(v) || \
((v) != NULL)) \
smp_wmb(); \
(p) = (v); \
})

There are plenty regular uses of smp_wmb() in kernel, not related to Read Copy Update,
there is nothing wrong to use barriers with appropriate comments.

(And you already use mb(), wmb(), rmb(), smp_wmb() in your patch)

BTW there is at least one locking bug in vhost_net_set_features()

Apparently, mutex_unlock() doesnt trigger a fault if mutex is not locked
by current thread... even with DEBUG_MUTEXES / DEBUG_LOCK_ALLOC

static void vhost_net_set_features(struct vhost_net *n, u64 features)

{

size_t hdr_size = features & (1 << VHOST_NET_F_VIRTIO_NET_HDR) ?

sizeof(struct virtio_net_hdr) : 0;
int i;
<<!>> mutex_unlock(&n->dev.mutex);
n->dev.acked_features = features;
smp_wmb();

for (i = 0; i < VHOST_NET_VQ_MAX; ++i) {

mutex_lock(&n->vqs[i].mutex);

n->vqs[i].hdr_size = hdr_size;

mutex_unlock(&n->vqs[i].mutex);
}
mutex_unlock(&n->dev.mutex);
vhost_net_flush(n);

[Paul E. McKenney]

On Tue, Nov 03, 2009 at 01:14:06PM -0500, Gregory Haskins wrote:
> Gregory Haskins wrote:
> > Eric Dumazet wrote:

> >> Michael S. Tsirkin a �crit :

What was happening in that case was that the rcu_dereference()
was being used in a workqueue item. The role of rcu_read_lock()
was taken on be the start of execution of the workqueue item, of
rcu_read_unlock() by the end of execution of the workqueue item, and
of synchronize_rcu() by flush_workqueue(). This does work, at least
assuming that flush_workqueue() operates as advertised, which it appears
to at first glance.

The above code looks somewhat different, however -- I don't see
handle_tx() being executed in the context of a work queue. Instead
it appears to be in an interrupt handler.

So what is the story? Using synchronize_irq() or some such?

Thanx, Paul

[Andi Kleen]

"Michael S. Tsirkin" <m...@redhat.com> writes:

Haven't really read the whole thing, just noticed something at a glance.

I haven't gone over all this code in detail, but that isolated reference count
use looks suspicious. What prevents the mm from going away before
you increment, if it's not the current one?

-Andi

--
a...@linux.intel.com -- Speaking for myself only.

[Michael S. Tsirkin]

On Tue, Nov 03, 2009 at 10:11:12PM +0100, Eric Dumazet wrote:
> Michael S. Tsirkin a �crit :
> >
> > Paul, you acked this previously. Should I add you acked-by line so
> > people calm down? If you would rather I replace
> > rcu_dereference/rcu_assign_pointer with rmb/wmb, I can do this.
> > Or maybe patch Documentation to explain this RCU usage?
> >
>
> So you believe I am over-reacting to this dubious use of RCU ?
>
> RCU documentation is already very complex, we dont need to add yet another
> subtle use, and makes it less readable.
>
> It seems you use 'RCU api' in drivers/vhost/net.c as convenient macros :
>
> #define rcu_dereference(p) ({ \
> typeof(p) _________p1 = ACCESS_ONCE(p); \
> smp_read_barrier_depends(); \
> (_________p1); \
> })
>
> #define rcu_assign_pointer(p, v) \
> ({ \
> if (!__builtin_constant_p(v) || \
> ((v) != NULL)) \
> smp_wmb(); \
> (p) = (v); \
> })
>
>
> There are plenty regular uses of smp_wmb() in kernel, not related to Read Copy Update,
> there is nothing wrong to use barriers with appropriate comments.

Well, what I do has classic RCU characteristics: readers do not take
locks, writers take a lock and flush after update. This is why I believe
rcu_dereference and rcu_assign_pointer are more appropriate here than
open-coding barriers.

Before deciding whether it's a good idea to open-code barriers
instead, I would like to hear Paul's opinion.

>
> (And you already use mb(), wmb(), rmb(), smp_wmb() in your patch)

Yes, virtio guest pretty much forces this, there's no way to share
a lock with the guest.

> BTW there is at least one locking bug in vhost_net_set_features()
>
> Apparently, mutex_unlock() doesnt trigger a fault if mutex is not locked
> by current thread... even with DEBUG_MUTEXES / DEBUG_LOCK_ALLOC
>
>
> static void vhost_net_set_features(struct vhost_net *n, u64 features)
> {
> size_t hdr_size = features & (1 << VHOST_NET_F_VIRTIO_NET_HDR) ?
> sizeof(struct virtio_net_hdr) : 0;
> int i;
> <<!>> mutex_unlock(&n->dev.mutex);
> n->dev.acked_features = features;
> smp_wmb();
> for (i = 0; i < VHOST_NET_VQ_MAX; ++i) {
> mutex_lock(&n->vqs[i].mutex);
> n->vqs[i].hdr_size = hdr_size;
> mutex_unlock(&n->vqs[i].mutex);
> }
> mutex_unlock(&n->dev.mutex);
> vhost_net_flush(n);
> }

Thanks very much for spotting this! Will fix.

--
MST

[Michael S. Tsirkin]

No, there has been no change (I won't be able to use a mutex in an
interrupt handler, will I?). handle_tx is still called in the context
of a work queue: either from handle_tx_kick or from handle_tx_net which
are work queue items.

Can you ack this usage please?

--
MST

[Michael S. Tsirkin]

We take a reference to it before we start any virtqueues,
and stop all virtqueues before we drop the reference:

/* Caller should have device mutex */

static long vhost_dev_set_owner(struct vhost_dev *dev)
{

/* Is there an owner already? */

if (dev->mm)
return -EBUSY;

/* No owner, become one */

dev->mm = get_task_mm(current);
return 0;
}

And
vhost_dev_cleanup:
....

if (dev->mm)
mmput(dev->mm);
dev->mm = NULL;
}

Fine?

[Andi Kleen]

> Fine?

I cannot say -- are there paths that could drop the device beforehand?
(as in do you hold a reference to it?)

[Michael S. Tsirkin]

On Wed, Nov 04, 2009 at 01:59:57PM +0100, Andi Kleen wrote:
> > Fine?
>
> I cannot say -- are there paths that could drop the device beforehand?

Do you mean drop the mm reference?

> (as in do you hold a reference to it?)

By design I think I always have a reference to mm before I use it.

This works like this:
ioctl SET_OWNER - calls get_task_mm, I think this gets a reference to mm
ioctl SET_BACKEND - checks that SET_OWNER was run, starts virtqueue
ioctl RESET_OWNER - stops virtqueues, drops the reference to mm
file close - stops virtqueues, if we still have it then drops mm

This is why I think I can call use_mm/unuse_mm while virtqueue is running,
safely.
Makes sense?

[Andi Kleen]

On Wed, Nov 04, 2009 at 03:08:28PM +0200, Michael S. Tsirkin wrote:
> On Wed, Nov 04, 2009 at 01:59:57PM +0100, Andi Kleen wrote:
> > > Fine?
> >
> > I cannot say -- are there paths that could drop the device beforehand?
>
> Do you mean drop the mm reference?

No the reference to the device, which owns the mm for you.

>
> > (as in do you hold a reference to it?)
>
> By design I think I always have a reference to mm before I use it.
>
> This works like this:
> ioctl SET_OWNER - calls get_task_mm, I think this gets a reference to mm
> ioctl SET_BACKEND - checks that SET_OWNER was run, starts virtqueue
> ioctl RESET_OWNER - stops virtqueues, drops the reference to mm
> file close - stops virtqueues, if we still have it then drops mm
>
> This is why I think I can call use_mm/unuse_mm while virtqueue is running,
> safely.
> Makes sense?

Do you protect against another thread doing RESET_OWNER in parallel while
RESET_OWNER runs?

[Michael S. Tsirkin]

On Wed, Nov 04, 2009 at 02:15:33PM +0100, Andi Kleen wrote:
> On Wed, Nov 04, 2009 at 03:08:28PM +0200, Michael S. Tsirkin wrote:
> > On Wed, Nov 04, 2009 at 01:59:57PM +0100, Andi Kleen wrote:
> > > > Fine?
> > >
> > > I cannot say -- are there paths that could drop the device beforehand?
> >
> > Do you mean drop the mm reference?
>
> No the reference to the device, which owns the mm for you.

The device is created when file is open and destroyed
when file is closed. So I think the fs code handles the
reference counting for me: it won't call file cleanup
callback while some userspace process has the file open.
Right?

> >
> > > (as in do you hold a reference to it?)
> >
> > By design I think I always have a reference to mm before I use it.
> >
> > This works like this:
> > ioctl SET_OWNER - calls get_task_mm, I think this gets a reference to mm
> > ioctl SET_BACKEND - checks that SET_OWNER was run, starts virtqueue
> > ioctl RESET_OWNER - stops virtqueues, drops the reference to mm
> > file close - stops virtqueues, if we still have it then drops mm
> >
> > This is why I think I can call use_mm/unuse_mm while virtqueue is running,
> > safely.
> > Makes sense?
>
> Do you protect against another thread doing RESET_OWNER in parallel while
> RESET_OWNER runs?

Yes, I have a mutex in the device for that. Same with SET_BACKEND.

[Andi Kleen]

On Wed, Nov 04, 2009 at 03:17:36PM +0200, Michael S. Tsirkin wrote:
> On Wed, Nov 04, 2009 at 02:15:33PM +0100, Andi Kleen wrote:
> > On Wed, Nov 04, 2009 at 03:08:28PM +0200, Michael S. Tsirkin wrote:
> > > On Wed, Nov 04, 2009 at 01:59:57PM +0100, Andi Kleen wrote:
> > > > > Fine?
> > > >
> > > > I cannot say -- are there paths that could drop the device beforehand?
> > >
> > > Do you mean drop the mm reference?
> >
> > No the reference to the device, which owns the mm for you.
>
> The device is created when file is open and destroyed
> when file is closed. So I think the fs code handles the
> reference counting for me: it won't call file cleanup
> callback while some userspace process has the file open.
> Right?

Yes.

But the semantics when someone inherits such a fd through exec
or through file descriptor passing would be surely "interesting"
You would still do IO on the old VM.

I guess it would be a good way to confuse memory accounting schemes
or administrators @)

It would be all saner if this was all a single atomic step.

[David Miller]

From: "Michael S. Tsirkin" <m...@redhat.com>
Date: Tue, 3 Nov 2009 19:24:00 +0200

> Assuming it's okay with davem, I think it makes sense to merge this
> patch through Rusty's tree because vhost is the first user of the new
> interface. Posted here for completeness.

I'm fine with that, please add my:

Acked-by: David S. Miller <da...@davemloft.net>

[Michael S. Tsirkin]

On Wed, Nov 04, 2009 at 02:37:28PM +0100, Andi Kleen wrote:
> On Wed, Nov 04, 2009 at 03:17:36PM +0200, Michael S. Tsirkin wrote:
> > On Wed, Nov 04, 2009 at 02:15:33PM +0100, Andi Kleen wrote:
> > > On Wed, Nov 04, 2009 at 03:08:28PM +0200, Michael S. Tsirkin wrote:
> > > > On Wed, Nov 04, 2009 at 01:59:57PM +0100, Andi Kleen wrote:
> > > > > > Fine?
> > > > >
> > > > > I cannot say -- are there paths that could drop the device beforehand?
> > > >
> > > > Do you mean drop the mm reference?
> > >
> > > No the reference to the device, which owns the mm for you.
> >
> > The device is created when file is open and destroyed
> > when file is closed. So I think the fs code handles the
> > reference counting for me: it won't call file cleanup
> > callback while some userspace process has the file open.
> > Right?
>
> Yes.
>
> But the semantics when someone inherits such a fd through exec
> or through file descriptor passing would be surely "interesting"
> You would still do IO on the old VM.
>
> I guess it would be a good way to confuse memory accounting schemes
> or administrators @)
> It would be all saner if this was all a single atomic step.
>
> -Andi

I have this atomic actually. A child process will first thing
do SET_OWNER: this is required before any other operation.

SET_OWNER atomically (under mutex) does two things:
- check that there is no other owner
- get mm and set current process as owner

I hope this addresses your concern?

--
MST

[Michael S. Tsirkin]

Tun device looks similar to a packet socket
in that both pass complete frames from/to userspace.

This patch fills in enough fields in the socket underlying tun driver
to support sendmsg/recvmsg operations, and message flags
MSG_TRUNC and MSG_DONTWAIT, and exports access to this socket
to modules. Regular read/write behaviour is unchanged.

This way, code using raw sockets to inject packets
into a physical device, can support injecting
packets into host network stack almost without modification.

First user of this interface will be vhost virtualization
accelerator.

Signed-off-by: Michael S. Tsirkin <m...@redhat.com>
Acked-by: Herbert Xu <her...@gondor.apana.org.au>

Acked-by: David S. Miller <da...@davemloft.net>

---

DaveM agreed that this will go in through Rusty's tree together
with vhost patch, because vhost is the first user of the new
interface.

drivers/net/tun.c | 101 +++++++++++++++++++++++++++++++++++++++---------

+ return ret;
+}
+

+static ssize_t tun_chr_aio_read(struct kiocb *iocb, const struct iovec *iv,
+ unsigned long count, loff_t pos)

+{

+ struct file *file = iocb->ki_filp;
+ struct tun_file *tfile = file->private_data;
+ struct tun_struct *tun = __tun_get(tfile);
+ ssize_t len, ret;
+
+ if (!tun)
+ return -EBADFD;
+ len = iov_length(iv, count);
+ if (len < 0) {
+ ret = -EINVAL;
+ goto out;

+ }
+

+ return ret;
+}
+

--

[Michael S. Tsirkin]

vhost net module wants to do copy to/from user from a kernel thread,
which needs use_mm. Export it to modules.

Acked-by: Andrea Arcangeli <aarc...@redhat.com>

Signed-off-by: Michael S. Tsirkin <m...@redhat.com>

---
mm/mmu_context.c | 3 +++
1 files changed, 3 insertions(+), 0 deletions(-)

diff --git a/mm/mmu_context.c b/mm/mmu_context.c
index ded9081..0777654 100644
--- a/mm/mmu_context.c
+++ b/mm/mmu_context.c
@@ -5,6 +5,7 @@

#include <linux/mm.h>
#include <linux/mmu_context.h>
+#include <linux/module.h>
#include <linux/sched.h>

#include <asm/mmu_context.h>
@@ -37,6 +38,7 @@ void use_mm(struct mm_struct *mm)
if (active_mm != mm)
mmdrop(active_mm);
}
+EXPORT_SYMBOL_GPL(use_mm);

/*
* unuse_mm
@@ -56,3 +58,4 @@ void unuse_mm(struct mm_struct *mm)
enter_lazy_tlb(mm, tsk);
task_unlock(tsk);
}
+EXPORT_SYMBOL_GPL(unuse_mm);

[Michael S. Tsirkin]

Note on RCU usage (this is also documented in vhost.h, near
private_pointer which is the value protected by this variant of RCU):
what is happening is that the rcu_dereference() is being used
in a workqueue item. The role of rcu_read_lock() is taken on by the

start of execution of the workqueue item, of rcu_read_unlock() by the
end of execution of the workqueue item, and of synchronize_rcu() by

flush_workqueue()/flush_work().

Acked-by: Arnd Bergmann <ar...@arndb.de>
Cc: "Paul E. McKenney" <pau...@linux.vnet.ibm.com>

Signed-off-by: Michael S. Tsirkin <m...@redhat.com>
---

Paul, I'm Cc you so that you can verify that nothing significant changed
in vhost/net.c since you approved it the previous time. Could you
please Ack use of rcu_dereference/rcu_assign_pointer there? Eric
Dumazet proposed that I open-code these, he suggested that this is an
abuse of these macros and not enough like RCU. What is your opinion?

index 0000000..9f409f4

--- /dev/null
+++ b/drivers/vhost/Kconfig
@@ -0,0 +1,11 @@
+config VHOST_NET

+ tristate "Host kernel accelerator for virtio net (EXPERIMENTAL)"
+ depends on NET && EVENTFD && EXPERIMENTAL

+ ---help---
+ This kernel module can be loaded in host kernel to accelerate
+ guest networking with virtio_net. Not to be confused with virtio_net
+ module itself which needs to be loaded in guest kernel.
+
+ To compile this driver as a module, choose M here: the module will
+ be called vhost_net.
+
diff --git a/drivers/vhost/Makefile b/drivers/vhost/Makefile
new file mode 100644
index 0000000..72dd020
--- /dev/null
+++ b/drivers/vhost/Makefile
@@ -0,0 +1,2 @@
+obj-$(CONFIG_VHOST_NET) += vhost_net.o
+vhost_net-y := vhost.o net.o
diff --git a/drivers/vhost/net.c b/drivers/vhost/net.c
new file mode 100644

index 0000000..aeafaf3

+ mutex_lock(&vq->mutex);
+ vhost_no_notify(vq);
+

+ if (wmem < sock->sk->sk_sndbuf * 2)
+ tx_poll_stop(net);
+ hdr_size = vq->hdr_size;
+
+ for (;;) {
+ head = vhost_get_vq_desc(&net->dev, vq, vq->iov, &out, &in,
+ NULL, NULL);
+ /* Nothing new? Wait for eventfd to tell us they refilled. */
+ if (head == vq->num) {

+ wmem = atomic_read(&sock->sk->sk_wmem_alloc);

+ if (wmem >= sock->sk->sk_sndbuf * 3 / 4) {
+ tx_poll_start(net, sock);
+ set_bit(SOCK_ASYNC_NOSPACE, &sock->flags);
+ break;
+ }
+ if (vhost_notify(vq))
+ continue;
+ break;
+ }
+ if (in) {
+ vq_err(vq, "Unexpected descriptor format for TX: "
+ "out %d, int %d\n", out, in);
+ break;
+ }
+ /* Skip header. TODO: support TSO. */
+ s = move_iovec_hdr(vq->iov, vq->hdr, hdr_size, out);
+ msg.msg_iovlen = out;
+ len = iov_length(vq->iov, out);
+ /* Sanity check */
+ if (!len) {
+ vq_err(vq, "Unexpected header len for TX: "

+ "%zd expected %zd\n",
+ iov_length(vq->hdr, s), hdr_size);
+ break;
+ }

+ /* TODO: Check specific error and bomb out unless ENOBUFS? */
+ err = sock->ops->sendmsg(NULL, sock, &msg, len);
+ if (unlikely(err < 0)) {
+ vhost_discard_vq_desc(vq);
+ tx_poll_start(net, sock);
+ break;
+ }
+ if (err != len)
+ pr_err("Truncated TX packet: "
+ " len %d != %zd\n", err, len);
+ vhost_add_used_and_trigger(&net->dev, vq, head, 0);
+ total_len += len;
+ if (unlikely(total_len >= VHOST_NET_WEIGHT)) {
+ vhost_poll_queue(&vq->poll);
+ break;
+ }
+ }
+
+ mutex_unlock(&vq->mutex);
+ unuse_mm(net->dev.mm);
+}
+

+/* Expects to be always run from workqueue - which acts as
+ * read-size critical section for our kind of RCU. */

+static void handle_rx(struct vhost_net *net)
+{

+ struct vhost_virtqueue *vq = &net->dev.vqs[VHOST_NET_VQ_RX];
+ unsigned head, out, in, log, s;
+ struct vhost_log *vq_log;

+ struct msghdr msg = {
+ .msg_name = NULL,
+ .msg_namelen = 0,

+ .msg_control = NULL, /* FIXME: get and handle RX aux data. */

+ .msg_controllen = 0,
+ .msg_iov = vq->iov,
+ .msg_flags = MSG_DONTWAIT,
+ };
+

+ struct virtio_net_hdr hdr = {
+ .flags = 0,
+ .gso_type = VIRTIO_NET_HDR_GSO_NONE
+ };

+
+ size_t len, total_len = 0;

+ int err;

+ size_t hdr_size;
+ struct socket *sock = rcu_dereference(vq->private_data);

+ if (!sock || skb_queue_empty(&sock->sk->sk_receive_queue))

+ return;
+
+ use_mm(net->dev.mm);

+ mutex_lock(&vq->mutex);
+ vhost_no_notify(vq);

+{

+ struct vhost_virtqueue *vq;
+ struct vhost_net *net;
+ vq = container_of(work, struct vhost_virtqueue, poll.work);
+ net = container_of(vq->dev, struct vhost_net, dev);
+ handle_tx(net);
+}
+
+static void handle_rx_kick(struct work_struct *work)

+{

+ struct socket *sock = n->vqs[index].private_data;

+ if (!sock)
+ return;

+ struct socket *sock = sockfd_lookup(fd, &r);
+ if (!sock)

+ return ERR_PTR(-ENOTSOCK);
+
+ /* Parameter checking */
+ if (sock->sk->sk_type != SOCK_RAW) {
+ r = -ESOCKTNOSUPPORT;
+ goto err;
+ }
+
+ r = sock->ops->getname(sock, (struct sockaddr *)&uaddr.sa,
+ &uaddr_len, 0);
+ if (r)
+ goto err;
+
+ if (uaddr.sa.sll_family != AF_PACKET) {
+ r = -EPFNOSUPPORT;
+ goto err;
+ }
+ return sock;
+err:
+ fput(sock->file);
+ return ERR_PTR(r);
+}
+
+static struct socket *get_tun_socket(int fd)

+{

+ mutex_lock(&n->dev.mutex);

index 0000000..985f7f8

+ r = copy_from_user(&p, argp, sizeof p);

+ u64 base = (u64)(unsigned long)log_base;

+ u64 log = base + write_address / 8;
+ int bit = write_address % 8;
+ if ((u64)(unsigned long)log != log)
+ return -EFAULT;

+ r = set_bit_to_user(bit, (void __user *)(unsigned long)log);

+ _iov->iov_base = (void *)(unsigned long)

+ (reg->userspace_addr + addr - reg->guest_phys_addr);
+ s += size;
+ addr += size;
+ ++ret;
+ }
+
+ rcu_read_unlock();

+ return ret;
+}
+

+ }
+

+ ret = translate_desc(dev, indirect->addr, indirect->len, vq->indirect,
+ ARRAY_SIZE(vq->indirect));
+ if (ret < 0) {
+ vq_err(vq, "Translation failure %d in indirect.\n", ret);

+ return ret;
+ }
+

+ }
+

+ ret = translate_desc(dev, desc.addr, desc.len, iov + iov_count,
+ VHOST_NET_MAX_SG - iov_count);
+ if (ret < 0) {
+ vq_err(vq, "Translation failure %d indirect idx %d\n",
+ ret, i);

+ return ret;
+ }

+ }
+

[Michael S. Tsirkin]

Andrea, since you looked at this design at the early stages,
maybe you can provide feedback on the following question:

vhost has an ioctl to do get_task_mm and store the mm in per-file device
structure. mmput is called when file is closed. vhost is careful not
to reference the mm after is has been put. There is also an atomic
mutual exclusion mechanism to ensure that vhost does not allow one
process to access another's mm, even if they share a vhost file
descriptor. But, this still means that mm structure can outlive the
task if the file descriptor is shared with another process.

Other drivers, such as kvm, have the same property.

Do you think this is OK?

Thanks!

[Paul E. McKenney]

Ah, my mistake -- I was looking at 2.6.31 rather than latest git with
your patches.

> Can you ack this usage please?

I thought I had done so in my paragraph above, but if you would like
something a bit more formal...

I, Paul E. McKenney, maintainer of the RCU implmentation
embodied in the Linux kernel and co-inventor of RCU, being of
sound mind and body, notwithstanding the wear and tear inherent
in my numerous decades sojourn on this planet, hereby declare
that the following usage of work queues constitutes a valid
RCU implementation:

1. Execution of a full workqueue item being substituted
for a conventional RCU read-side critical section, so
that the start of execution of the function specified to
INIT_WORK() corresponds to rcu_read_lock(), and the end of
this self-same function corresponds to rcu_read_unlock().

2. Execution of flush_workqueue() being substituted for
the conventional synchronize_rcu().

The kernel developer availing himself or herself of this
declaration must observe the following caveats:

a. The function specified to INIT_WORK() may only be
invoked via the workqueue mechanism. Invoking said
function directly renders this declaration null
and void, as it prevents the flush_workqueue() function
from delivering the fundamental guarantee inherent in RCU.

b. At some point in the future, said developer may be
required to apply some gcc attribute or sparse annotation
to the function passed to INIT_WORK(). Beyond that
point, failure to comply will render this declaration
null and void, as such failure would render inoperative
some potential RCU-validation tools, as duly noted by
Eric Dumazet.

c. This declaration in no way relieves the developer of
the responsibility to use this and other synchronization
mechanisms correctly, again, as duly noted by Eric
Dumazet.

(Sorry, but, as always, I could not resist!)

Thanx, Paul

[Michael S. Tsirkin]

On Wed, Nov 04, 2009 at 09:25:42AM -0800, Paul E. McKenney wrote:
> (Sorry, but, as always, I could not resist!)
>
> Thanx, Paul

Thanks Paul!
Jonathan: are you reading this?
Another one for your quotes of the week collection :)

--
MST

[Michael S. Tsirkin]

On Wed, Nov 04, 2009 at 07:09:05PM +0100, Arnd Bergmann wrote:

> On Tuesday 03 November 2009, Arnd Bergmann wrote:
> > > index 3f5fd52..404abe0 100644
> > > --- a/include/linux/if_tun.h
> > > +++ b/include/linux/if_tun.h
> > > @@ -86,4 +86,18 @@ struct tun_filter {
> > > __u8 addr[0][ETH_ALEN];
> > > };
> > >
> > > +#ifdef __KERNEL__
> > > +#if defined(CONFIG_TUN) || defined(CONFIG_TUN_MODULE)
> > > +struct socket *tun_get_socket(struct file *);
> > > +#else
> > > +#include <linux/err.h>
> > > +#include <linux/errno.h>
> > > +struct file;
> > > +struct socket;
> > > +static inline struct socket *tun_get_socket(struct file *f)
> > > +{
> > > + return ERR_PTR(-EINVAL);
> > > +}
> > > +#endif /* CONFIG_TUN */
> > > +#endif /* __KERNEL__ */
> > > #endif /* __IF_TUN_H */
> >

> > Is this a leftover from testing? Exporting the function for !__KERNEL__
> > seems pointless.
> >
>
> Michael, you didn't reply on this comment and the code is still there in v8.
> Do you actually need this? What for?
>
> Arnd <><

Sorry, missed the question. If you look closely it is not exported for
!__KERNEL__ at all. The stub is for when CONFIG_TUN is undefined.
Maybe I'll add a comment near #else, even though this is a bit strange
since the #if is just 2 lines above it.

[Gregory Haskins]

Michael S. Tsirkin wrote:
> On Wed, Nov 04, 2009 at 09:25:42AM -0800, Paul E. McKenney wrote:
>> (Sorry, but, as always, I could not resist!)
>>
>> Thanx, Paul
>
> Thanks Paul!
> Jonathan: are you reading this?
> Another one for your quotes of the week collection :)
>

I second that.

[Arnd Bergmann]

On Wednesday 04 November 2009, Michael S. Tsirkin wrote:
> >
> > Michael, you didn't reply on this comment and the code is still there in v8.
> > Do you actually need this? What for?
> >
> > Arnd <><
>
> Sorry, missed the question. If you look closely it is not exported for
> !__KERNEL__ at all. The stub is for when CONFIG_TUN is undefined.
> Maybe I'll add a comment near #else, even though this is a bit strange
> since the #if is just 2 lines above it.

Ah right, I'm just blind.

Don't bother changing it then, the code looks good as it is.

Arnd <><

[Eric Dumazet]

Paul E. McKenney a �crit :

>
> (Sorry, but, as always, I could not resist!)

Yesssss :)

Thanks Paul for this masterpiece of diplomatic "Acked-by" ;)

[Arnd Bergmann]

On Tuesday 03 November 2009, Arnd Bergmann wrote:
> > index 3f5fd52..404abe0 100644
> > --- a/include/linux/if_tun.h
> > +++ b/include/linux/if_tun.h
> > @@ -86,4 +86,18 @@ struct tun_filter {
> > __u8 addr[0][ETH_ALEN];
> > };
> >
> > +#ifdef __KERNEL__
> > +#if defined(CONFIG_TUN) || defined(CONFIG_TUN_MODULE)
> > +struct socket *tun_get_socket(struct file *);
> > +#else
> > +#include <linux/err.h>
> > +#include <linux/errno.h>
> > +struct file;
> > +struct socket;
> > +static inline struct socket *tun_get_socket(struct file *f)
> > +{
> > + return ERR_PTR(-EINVAL);
> > +}
> > +#endif /* CONFIG_TUN */
> > +#endif /* __KERNEL__ */
> > #endif /* __IF_TUN_H */
>
> Is this a leftover from testing? Exporting the function for !__KERNEL__
> seems pointless.
>

Michael, you didn't reply on this comment and the code is still there in v8.

Do you actually need this? What for?

Arnd <><

[Rusty Russell]

On Thu, 5 Nov 2009 02:27:24 am Michael S. Tsirkin wrote:
> What it is: vhost net is a character device that can be used to reduce
> the number of system calls involved in virtio networking.

Hi Michael,

Now everyone else has finally kicked all the tires and it seems to pass,
I've done a fairly complete review. Generally, it's really nice; just one
bug and a few minor suggestions for polishing.

> +/* Caller must have TX VQ lock */
> +static void tx_poll_stop(struct vhost_net *net)
> +{
> + if (likely(net->tx_poll_state != VHOST_NET_POLL_STARTED))
> + return;

likely? Really?

> + for (;;) {
> + head = vhost_get_vq_desc(&net->dev, vq, vq->iov, &out, &in,
> + NULL, NULL);

Danger! You need an arg to vhost_get_vq_desc to tell it the max desc size
you can handle. Otherwise, it's only limited by ring size, and a malicious
guest can overflow you here, and below:

> + /* Skip header. TODO: support TSO. */
> + s = move_iovec_hdr(vq->iov, vq->hdr, hdr_size, out);

...

> +
> + use_mm(net->dev.mm);
> + mutex_lock(&vq->mutex);
> + vhost_no_notify(vq);

I prefer a name like "vhost_disable_notify()".

> + /* OK, now we need to know about added descriptors. */
> + if (head == vq->num && vhost_notify(vq))
> + /* They could have slipped one in as we were doing that:
> + * check again. */
> + continue;
> + /* Nothing new? Wait for eventfd to tell us they refilled. */
> + if (head == vq->num)
> + break;
> + /* We don't need to be notified again. */
> + vhost_no_notify(vq);

Similarly, vhost_enable_notify. This one is particularly misleading since
it doesn't actually notify anything!

In particular, this code would be neater as:

if (head == vq->num) {

if (vhost_enable_notify(vq)) {
/* Try again, they could have slipped one in. */
continue;
}
/* Nothing more to do. */
break;
}
vhost_disable_notify(vq);

Now, AFAICT vhost_notify()/enable_notify() would be better rewritten to
return true only when there's more pending. Saves a loop around here most
of the time. Also, the vhost_no_notify/vhost_disable_notify() can be moved
out of the loop entirely. (It could be under an if (unlikely(enabled)), not
sure if it's worth it).

> + len = err;
> + err = memcpy_toiovec(vq->hdr, (unsigned char *)&hdr, hdr_size);

That unsigned char * arg to memcpy_toiovec is annoying. A patch might be
nice, separate from this effort.

> +static int vhost_net_open(struct inode *inode, struct file *f)
> +{
> + struct vhost_net *n = kzalloc(sizeof *n, GFP_KERNEL);
> + int r;
> + if (!n)
> + return -ENOMEM;
> + f->private_data = n;
> + n->vqs[VHOST_NET_VQ_TX].handle_kick = handle_tx_kick;
> + n->vqs[VHOST_NET_VQ_RX].handle_kick = handle_rx_kick;

I have a personal dislike of calloc for structures. In userspace, it's
because valgrind can't spot uninitialized fields. These days a similar
argument applies in the kernel, because we have KMEMCHECK now. If someone
adds a field to the struct and forgets to initialize it, we can spot it.

> +static void vhost_net_enable_vq(struct vhost_net *n, int index)
> +{
> + struct socket *sock = n->vqs[index].private_data;

OK, I can't help but this that presenting the vqs as an array doesn't buy
us very much. Esp. if you change vhost_dev_init to take a NULL-terminated
varargs. I think readability would improve. It means passing a vq around
rather than an index.

Not completely sure it'll be a win tho.

> +static long vhost_net_set_backend(struct vhost_net *n, unsigned index, int fd)
> +{
> + struct socket *sock, *oldsock = NULL;

...

> + sock = get_socket(fd);
> + if (IS_ERR(sock)) {
> + r = PTR_ERR(sock);
> + goto done;
> + }
> +
> + /* start polling new socket */
> + oldsock = vq->private_data;

...

> +done:
> + mutex_unlock(&n->dev.mutex);
> + if (oldsock) {
> + vhost_net_flush_vq(n, index);
> + fput(oldsock->file);

I dislike this style; I prefer multiple different goto points, one for when
oldsock is set, and one for when it's not.

That way, gcc warns us about uninitialized variables if we get it wrong.

> +static long vhost_net_reset_owner(struct vhost_net *n)
> +{
> + struct socket *tx_sock = NULL;
> + struct socket *rx_sock = NULL;
> + long r;

This should be called "err", since that's what it is.

> +static void vhost_net_set_features(struct vhost_net *n, u64 features)
> +{
> + size_t hdr_size = features & (1 << VHOST_NET_F_VIRTIO_NET_HDR) ?
> + sizeof(struct virtio_net_hdr) : 0;
> + int i;
> + mutex_lock(&n->dev.mutex);
> + n->dev.acked_features = features;

Why is this called "acked_features"? Not just "features"? I expected
to see code which exposed these back to userspace, and didn't.

> + case VHOST_GET_FEATURES:
> + features = VHOST_FEATURES;
> + return put_user(features, featurep);
> + case VHOST_ACK_FEATURES:
> + r = get_user(features, featurep);
> + /* No features for now */
> + if (r < 0)
> + return r;
> + if (features & ~VHOST_FEATURES)
> + return -EOPNOTSUPP;
> + vhost_net_set_features(n, features);

OK, from the userspace POV it's "get features" then "ack features". But
I think "VHOST_SET_FEATURES" is more consistent, despite this usage.

> + switch (ioctl) {
> + case VHOST_SET_VRING_NUM:

I haven't looked at your userspace implementation, but does a generic
VHOST_SET_VRING_STATE & VHOST_GET_VRING_STATE with a struct make more
sense? It'd be simpler here, but not sure if it'd be simpler to use?

(Not the fd-setting ioctls of course)

> + case VHOST_SET_VRING_LOG:
> + r = copy_from_user(&a, argp, sizeof a);
> + if (r < 0)
> + break;
> + if (a.padding) {
> + r = -EOPNOTSUPP;
> + break;
> + }
> + if (a.user_addr == VHOST_VRING_LOG_DISABLE) {
> + vq->log_used = false;
> + break;
> + }
> + if (a.user_addr & (sizeof *vq->used->ring - 1)) {
> + r = -EINVAL;
> + break;
> + }
> + vq->log_used = true;
> + vq->log_addr = a.user_addr;
> + break;

For future reference, this is *exactly* the kind of thing which would have
been nice as a followup patch. Easy to separate, easy to review, not critical
to the core.

> +/* TODO: This is really inefficient. We need something like get_user()
> + * (instruction directly accesses the data, with an exception table entry
> + * returning -EFAULT). See Documentation/x86/exception-tables.txt.
> + */
> +static int set_bit_to_user(int nr, void __user *addr)
> +{

I guess we won't be dealing with many contiguous pages, otherwise we could
get a cheap speedup making this set_bits_to_user(int nr, int num_bits...).

> +/* Each buffer in the virtqueues is actually a chain of descriptors. This
> + * function returns the next descriptor in the chain,
> + * or -1 if we're at the end. */
> +static unsigned next_desc(struct vring_desc *desc)
> +{
> + unsigned int next;
> +
> + /* If this descriptor says it doesn't chain, we're done. */
> + if (!(desc->flags & VRING_DESC_F_NEXT))
> + return -1;

Hmm, prefer s/-1/-1U/ in comment, here, and below. Clarifies a bit.

> +/* After we've used one of their buffers, we tell them about it. We'll then
> + * want to send them an interrupt, using vq->call. */

This comment has too much cut & paste:
... want to notify the guest, using the eventfd */

> +/* This actually sends the interrupt for this virtqueue */
> +void vhost_trigger_irq(struct vhost_dev *dev, struct vhost_virtqueue *vq)
> +{

Rename vhost_notify_eventfd() or something, and fix comments?

> +enum {
> + VHOST_NET_MAX_SG = MAX_SKB_FRAGS + 2,

+2? Believable, but is it correct?

> +/* Poll a file (eventfd or socket) */
> +/* Note: there's nothing vhost specific about this structure. */
> +struct vhost_poll {

This comment really helped while reading the code. Kudos!

Thanks!
Rusty.

[Rusty Russell]

On Thu, 5 Nov 2009 03:55:42 am Paul E. McKenney wrote:
> On Wed, Nov 04, 2009 at 01:57:29PM +0200, Michael S. Tsirkin wrote:
> > Can you ack this usage please?
>
> I thought I had done so in my paragraph above, but if you would like
> something a bit more formal...

<snip verbose super-ack with qualifications>

That's great guys. And yes, this is a kind of read-copy-update. And no,
there's nothing wrong with it.

But it's still nasty to use half an API. If it were a few places I would
have open-coded it with a comment, or wrapped it. As it is, I don't think
that would be a win.

Cheers,
Rusty.

[Paul E. McKenney]

On Fri, Nov 06, 2009 at 03:31:20PM +1030, Rusty Russell wrote:
> On Thu, 5 Nov 2009 03:55:42 am Paul E. McKenney wrote:
> > On Wed, Nov 04, 2009 at 01:57:29PM +0200, Michael S. Tsirkin wrote:
> > > Can you ack this usage please?
> >
> > I thought I had done so in my paragraph above, but if you would like
> > something a bit more formal...
>
> <snip verbose super-ack with qualifications>
>
> That's great guys. And yes, this is a kind of read-copy-update. And no,
> there's nothing wrong with it.
>
> But it's still nasty to use half an API. If it were a few places I would
> have open-coded it with a comment, or wrapped it. As it is, I don't think
> that would be a win.

So would it help to have a rcu_read_lock_workqueue() and
rcu_read_unlock_workqueue() that checked nesting and whether they were
actually running in the context of a workqueue item? Or did you have
something else in mind? Or am I misjudging the level of sarcasm in
your reply? ;-)

Thanx, Paul

[Rusty Russell]

On Sat, 7 Nov 2009 03:00:07 am Paul E. McKenney wrote:
> On Fri, Nov 06, 2009 at 03:31:20PM +1030, Rusty Russell wrote:
> > But it's still nasty to use half an API. If it were a few places I would
> > have open-coded it with a comment, or wrapped it. As it is, I don't think
> > that would be a win.
>
> So would it help to have a rcu_read_lock_workqueue() and
> rcu_read_unlock_workqueue() that checked nesting and whether they were
> actually running in the context of a workqueue item? Or did you have
> something else in mind? Or am I misjudging the level of sarcasm in
> your reply? ;-)

You read correctly. If we get a second user, creating an API makes sense.

Thanks,
Rusty.

[Michael S. Tsirkin]

On Fri, Nov 06, 2009 at 03:29:17PM +1030, Rusty Russell wrote:
> On Thu, 5 Nov 2009 02:27:24 am Michael S. Tsirkin wrote:
> > What it is: vhost net is a character device that can be used to reduce
> > the number of system calls involved in virtio networking.
>
> Hi Michael,
>
> Now everyone else has finally kicked all the tires and it seems to pass,
> I've done a fairly complete review. Generally, it's really nice; just one
> bug and a few minor suggestions for polishing.

Thanks for the review! I'll add more polishing and repost.
Answers to some questions below.

> > +/* Caller must have TX VQ lock */
> > +static void tx_poll_stop(struct vhost_net *net)
> > +{
> > + if (likely(net->tx_poll_state != VHOST_NET_POLL_STARTED))
> > + return;
>
> likely? Really?

Hmm ... yes. tx poll stop is called on each packet (as long as we do not
fill up 1/2 backend queue), the first call will stop polling
the rest checks state and does nothing.

This is because we normally do not care when the message has left the
queue in backend device: we tell backend to send it and forget. We only
start polling when backend tx queue fills up.

Makes sense?

> > + for (;;) {
> > + head = vhost_get_vq_desc(&net->dev, vq, vq->iov, &out, &in,
> > + NULL, NULL);
>
> Danger! You need an arg to vhost_get_vq_desc to tell it the max desc size
> you can handle. Otherwise, it's only limited by ring size, and a malicious
> guest can overflow you here, and below:

In fact, I think this is not a bug. This happens to work correctly
(even with malicious guests) because vhost_get_vq_desc is hard-coded to
check VHOST_NET_MAX_SG, so in fact no overflow is possible. I agree
that it's mich nicer to pass iovec size to vhost_get_vq_desc.

>
> > + /* Skip header. TODO: support TSO. */
> > + s = move_iovec_hdr(vq->iov, vq->hdr, hdr_size, out);
> ...
> > +
> > + use_mm(net->dev.mm);
> > + mutex_lock(&vq->mutex);
> > + vhost_no_notify(vq);
>
> I prefer a name like "vhost_disable_notify()".

Good idea.

> > + /* OK, now we need to know about added descriptors. */
> > + if (head == vq->num && vhost_notify(vq))
> > + /* They could have slipped one in as we were doing that:
> > + * check again. */
> > + continue;
> > + /* Nothing new? Wait for eventfd to tell us they refilled. */
> > + if (head == vq->num)
> > + break;
> > + /* We don't need to be notified again. */
> > + vhost_no_notify(vq);
>
> Similarly, vhost_enable_notify. This one is particularly misleading since
> it doesn't actually notify anything!

Good idea.

>
> In particular, this code would be neater as:
>
> if (head == vq->num) {
> if (vhost_enable_notify(vq)) {
> /* Try again, they could have slipped one in. */
> continue;
> }
> /* Nothing more to do. */
> break;
> }
> vhost_disable_notify(vq);
>
> Now, AFAICT vhost_notify()/enable_notify() would be better rewritten to
> return true only when there's more pending. Saves a loop around here most
> of the time.

OKay, I'll look into this. It kind of annoys me that we would do
get_user for the same value twice: once in vhost_enable_notify and once
in vhost_get_vq_desc. OTOH, all the loop does is call vhost_get_vq_desc
again.

> Also, the vhost_no_notify/vhost_disable_notify() can be moved
> out of the loop entirely.

I don't think it can, if we enabled notification and then see more
descriptors in queue, we want to disable notification again. But it can
be

> (It could be under an if (unlikely(enabled)), not
> sure if it's worth it).

if (unlikely(vhost_enable_notify(vq))) {
/* Try again, they have slipped one in. */
vhost_disable_notify(vq);
continue;
}

>
> > + len = err;
> > + err = memcpy_toiovec(vq->hdr, (unsigned char *)&hdr, hdr_size);
>
> That unsigned char * arg to memcpy_toiovec is annoying. A patch might be
> nice, separate from this effort.

Sounds good.

> > +static int vhost_net_open(struct inode *inode, struct file *f)
> > +{
> > + struct vhost_net *n = kzalloc(sizeof *n, GFP_KERNEL);
> > + int r;
> > + if (!n)
> > + return -ENOMEM;
> > + f->private_data = n;
> > + n->vqs[VHOST_NET_VQ_TX].handle_kick = handle_tx_kick;
> > + n->vqs[VHOST_NET_VQ_RX].handle_kick = handle_rx_kick;
>
> I have a personal dislike of calloc for structures.

You mean zalloc?

> In userspace, it's because valgrind can't spot uninitialized fields.
> These days a similar argument applies in the kernel, because we have
> KMEMCHECK now. If someone adds a field to the struct and forgets to
> initialize it, we can spot it.

OK.

> > +static void vhost_net_enable_vq(struct vhost_net *n, int index)
> > +{
> > + struct socket *sock = n->vqs[index].private_data;
>
> OK, I can't help but this that presenting the vqs as an array doesn't buy
> us very much. Esp. if you change vhost_dev_init to take a NULL-terminated
> varargs. I think readability would improve. It means passing a vq around
> rather than an index.
>
> Not completely sure it'll be a win tho.

Hmm, varargs sounds a bit complex. But I agree readability for
vhost_net_enable_vq and friends would benefit from passing a vq around
rather than an index. I'll try it out and do it if it's a win.

> > +static long vhost_net_set_backend(struct vhost_net *n, unsigned index, int fd)
> > +{
> > + struct socket *sock, *oldsock = NULL;
> ...
> > + sock = get_socket(fd);
> > + if (IS_ERR(sock)) {
> > + r = PTR_ERR(sock);
> > + goto done;
> > + }
> > +
> > + /* start polling new socket */
> > + oldsock = vq->private_data;
> ...
> > +done:
> > + mutex_unlock(&n->dev.mutex);
> > + if (oldsock) {
> > + vhost_net_flush_vq(n, index);
> > + fput(oldsock->file);
>
> I dislike this style; I prefer multiple different goto points, one for when
> oldsock is set, and one for when it's not.
>
> That way, gcc warns us about uninitialized variables if we get it wrong.

OK.

> > +static long vhost_net_reset_owner(struct vhost_net *n)
> > +{
> > + struct socket *tx_sock = NULL;
> > + struct socket *rx_sock = NULL;
> > + long r;
>
> This should be called "err", since that's what it is.

OK.

> > +static void vhost_net_set_features(struct vhost_net *n, u64 features)
> > +{
> > + size_t hdr_size = features & (1 << VHOST_NET_F_VIRTIO_NET_HDR) ?
> > + sizeof(struct virtio_net_hdr) : 0;
> > + int i;
> > + mutex_lock(&n->dev.mutex);
> > + n->dev.acked_features = features;
>
> Why is this called "acked_features"? Not just "features"? I expected
> to see code which exposed these back to userspace, and didn't.

Not sure how do you mean. Userspace sets them, why
does it want to get them exposed back?

> > + case VHOST_GET_FEATURES:
> > + features = VHOST_FEATURES;
> > + return put_user(features, featurep);
> > + case VHOST_ACK_FEATURES:
> > + r = get_user(features, featurep);
> > + /* No features for now */
> > + if (r < 0)
> > + return r;
> > + if (features & ~VHOST_FEATURES)
> > + return -EOPNOTSUPP;
> > + vhost_net_set_features(n, features);
>
> OK, from the userspace POV it's "get features" then "ack features". But
> I think "VHOST_SET_FEATURES" is more consistent, despite this usage.

OK.

> > + switch (ioctl) {
> > + case VHOST_SET_VRING_NUM:
>
> I haven't looked at your userspace implementation, but does a generic
> VHOST_SET_VRING_STATE & VHOST_GET_VRING_STATE with a struct make more
> sense? It'd be simpler here,

Not by much though, right?

> but not sure if it'd be simpler to use?

The problem is with VHOST_SET_VRING_BASE as well. I want it to be
separate because I want to make it possible to relocate e.g. used ring
to another address while ring is running. This would be a good debugging
tool (you look at kernel's used ring, check descriptor, then update
guest's used ring) and also possibly an extra way to do migration. And
it's nicer to have vring size separate as well, because it is
initialized by host and never changed, right?

We could merge DESC, AVAIL, USED, and it will reduce the amount of code
in userspace. With both base, size and fds separate, it seemed a bit
more symmetrical to have desc/avail/used separate as well.
What's your opinion?

> (Not the fd-setting ioctls of course)
>
> > + case VHOST_SET_VRING_LOG:
> > + r = copy_from_user(&a, argp, sizeof a);
> > + if (r < 0)
> > + break;
> > + if (a.padding) {
> > + r = -EOPNOTSUPP;
> > + break;
> > + }
> > + if (a.user_addr == VHOST_VRING_LOG_DISABLE) {
> > + vq->log_used = false;
> > + break;
> > + }
> > + if (a.user_addr & (sizeof *vq->used->ring - 1)) {
> > + r = -EINVAL;
> > + break;
> > + }
> > + vq->log_used = true;
> > + vq->log_addr = a.user_addr;
> > + break;
>
> For future reference, this is *exactly* the kind of thing which would have
> been nice as a followup patch. Easy to separate, easy to review, not critical
> to the core.

Yes. It's not too late to split it out though: should I do it yet?

> > +/* TODO: This is really inefficient. We need something like get_user()
> > + * (instruction directly accesses the data, with an exception table entry
> > + * returning -EFAULT). See Documentation/x86/exception-tables.txt.
> > + */
> > +static int set_bit_to_user(int nr, void __user *addr)
> > +{
>
> I guess we won't be dealing with many contiguous pages, otherwise we could
> get a cheap speedup making this set_bits_to_user(int nr, int num_bits...).

No idea. Let's keep it as simple as possible for now?

> > +/* Each buffer in the virtqueues is actually a chain of descriptors. This
> > + * function returns the next descriptor in the chain,
> > + * or -1 if we're at the end. */
> > +static unsigned next_desc(struct vring_desc *desc)
> > +{
> > + unsigned int next;
> > +
> > + /* If this descriptor says it doesn't chain, we're done. */
> > + if (!(desc->flags & VRING_DESC_F_NEXT))
> > + return -1;
>
> Hmm, prefer s/-1/-1U/ in comment, here, and below. Clarifies a bit.

Good idea.

> > +/* After we've used one of their buffers, we tell them about it. We'll then
> > + * want to send them an interrupt, using vq->call. */
>
> This comment has too much cut & paste:

I tried to cut and paste as many comments as possible, this
made it easy to audit the code by comparing it with lguest,
and made them much more witty. But yes, this is definitely going
overboard as we do not have vq->call at all :)

> ... want to notify the guest, using the eventfd */
>
> > +/* This actually sends the interrupt for this virtqueue */
> > +void vhost_trigger_irq(struct vhost_dev *dev, struct vhost_virtqueue *vq)
> > +{
>
> Rename vhost_notify_eventfd() or something, and fix comments?

Sounds good. Since I'm renaming vhost_notify to vhost_enable_notify,
this one can just become vhost_notify.

> > +enum {
> > + VHOST_NET_MAX_SG = MAX_SKB_FRAGS + 2,
>
> +2? Believable, but is it correct?

+ 1 is for skb head, + 1 is for virtio net header.
I'll add a comment.

[Paul E. McKenney]

On Sun, Nov 08, 2009 at 02:39:59PM +1030, Rusty Russell wrote:
> On Sat, 7 Nov 2009 03:00:07 am Paul E. McKenney wrote:
> > On Fri, Nov 06, 2009 at 03:31:20PM +1030, Rusty Russell wrote:
> > > But it's still nasty to use half an API. If it were a few places I would
> > > have open-coded it with a comment, or wrapped it. As it is, I don't think
> > > that would be a win.
> >
> > So would it help to have a rcu_read_lock_workqueue() and
> > rcu_read_unlock_workqueue() that checked nesting and whether they were
> > actually running in the context of a workqueue item? Or did you have
> > something else in mind? Or am I misjudging the level of sarcasm in
> > your reply? ;-)
>
> You read correctly. If we get a second user, creating an API makes sense.

Makes sense to me as well. Which does provide some time to come up with
a primitive designed to answer the question "Am I currently executing in
the context of a workqueue item?". ;-)

Thanx, Paul

[Rusty Russell]

On Sun, 8 Nov 2009 10:05:16 pm Michael S. Tsirkin wrote:
> On Fri, Nov 06, 2009 at 03:29:17PM +1030, Rusty Russell wrote:
> > > +/* Caller must have TX VQ lock */
> > > +static void tx_poll_stop(struct vhost_net *net)
> > > +{
> > > + if (likely(net->tx_poll_state != VHOST_NET_POLL_STARTED))
> > > + return;
> >
> > likely? Really?
>
> Hmm ... yes. tx poll stop is called on each packet (as long as we do not
> fill up 1/2 backend queue), the first call will stop polling
> the rest checks state and does nothing.
>
> This is because we normally do not care when the message has left the
> queue in backend device: we tell backend to send it and forget. We only
> start polling when backend tx queue fills up.

OK, good.

> > > +static void vhost_net_set_features(struct vhost_net *n, u64 features)
> > > +{
> > > + size_t hdr_size = features & (1 << VHOST_NET_F_VIRTIO_NET_HDR) ?
> > > + sizeof(struct virtio_net_hdr) : 0;
> > > + int i;
> > > + mutex_lock(&n->dev.mutex);
> > > + n->dev.acked_features = features;
> >
> > Why is this called "acked_features"? Not just "features"? I expected
> > to see code which exposed these back to userspace, and didn't.
>
> Not sure how do you mean. Userspace sets them, why
> does it want to get them exposed back?

There's something about the 'acked' which rubs me the wrong way.
"enabled_features" is perhaps a better term than "acked_features"; "acked"
seems more a user point-of-view, "enabled" seems more driver POV?

set_features matches your ioctl names, but it sounds like a fn name :(

It's marginal. And 'features' is shorter than both.

> > > + switch (ioctl) {
> > > + case VHOST_SET_VRING_NUM:
> >
> > I haven't looked at your userspace implementation, but does a generic
> > VHOST_SET_VRING_STATE & VHOST_GET_VRING_STATE with a struct make more
> > sense? It'd be simpler here,
>
> Not by much though, right?
>
> > but not sure if it'd be simpler to use?
>
> The problem is with VHOST_SET_VRING_BASE as well. I want it to be
> separate because I want to make it possible to relocate e.g. used ring
> to another address while ring is running. This would be a good debugging
> tool (you look at kernel's used ring, check descriptor, then update
> guest's used ring) and also possibly an extra way to do migration. And
> it's nicer to have vring size separate as well, because it is
> initialized by host and never changed, right?

Actually, this looks wrong to me:

+ case VHOST_SET_VRING_BASE:
...

+ vq->avail_idx = vq->last_avail_idx = s.num;

The last_avail_idx is part of the state of the driver. It needs to be saved
and restored over susp/resume. The only reason it's not in the ring itself
is because I figured the other side doesn't need to see it (which is true, but
missed debugging opportunities as well as man-in-the-middle issues like this
one). I had a patch which put this field at the end of the ring, I might
resurrect it to avoid this problem. This is backwards compatible with all
implementations. See patch at end.

I would drop avail_idx altogether: get_user is basically free, and simplifies
a lot. As most state is in the ring, all you need is an ioctl to save/restore
the last_avail_idx.

> We could merge DESC, AVAIL, USED, and it will reduce the amount of code
> in userspace. With both base, size and fds separate, it seemed a bit
> more symmetrical to have desc/avail/used separate as well.
> What's your opinion?

Well, DESC, AVAIL, and USED could easily be turned into SET/GET_LAYOUT.

> > For future reference, this is *exactly* the kind of thing which would have
> > been nice as a followup patch. Easy to separate, easy to review, not critical
> > to the core.
>
> Yes. It's not too late to split it out though: should I do it yet?

Only if you're feeling enthused. It's lightly reviewed now.

Cheers,
Rusty.

virtio: put last_used and last_avail index into ring itself.

Generally, the other end of the virtio ring doesn't need to see where
you're up to in consuming the ring. However, to completely understand
what's going on from the outside, this information must be exposed.
For example, if you want to save and restore a virtio_ring, but you're
not the consumer because the kernel is using it directly.

Fortunately, we have room to expand: the ring is always a whole number
of pages and there's hundreds of bytes of padding after the avail ring
and the used ring, whatever the number of descriptors (which must be a
power of 2).

We add a feature bit so the guest can tell the host that it's writing
out the current value there, if it wants to use that.

Signed-off-by: Rusty Russell <ru...@rustcorp.com.au>
---
drivers/virtio/virtio_ring.c | 23 +++++++++++++++--------
include/linux/virtio_ring.h | 12 +++++++++++-
2 files changed, 26 insertions(+), 9 deletions(-)

diff --git a/drivers/virtio/virtio_ring.c b/drivers/virtio/virtio_ring.c
--- a/drivers/virtio/virtio_ring.c
+++ b/drivers/virtio/virtio_ring.c
@@ -71,9 +71,6 @@ struct vring_virtqueue
/* Number we've added since last sync. */
unsigned int num_added;

- /* Last used index we've seen. */
- u16 last_used_idx;
-
/* How to notify other side. FIXME: commonalize hcalls! */
void (*notify)(struct virtqueue *vq);

@@ -278,12 +275,13 @@ static void detach_buf(struct vring_virt

static inline bool more_used(const struct vring_virtqueue *vq)
{
- return vq->last_used_idx != vq->vring.used->idx;
+ return vring_last_used(&vq->vring) != vq->vring.used->idx;
}

static void *vring_get_buf(struct virtqueue *_vq, unsigned int *len)
{
struct vring_virtqueue *vq = to_vvq(_vq);
+ struct vring_used_elem *u;
void *ret;
unsigned int i;

@@ -300,8 +298,11 @@ static void *vring_get_buf(struct virtqu
return NULL;
}

- i = vq->vring.used->ring[vq->last_used_idx%vq->vring.num].id;
- *len = vq->vring.used->ring[vq->last_used_idx%vq->vring.num].len;
+ u = &vq->vring.used->ring[vring_last_used(&vq->vring) % vq->vring.num];
+ i = u->id;
+ *len = u->len;
+ /* Make sure we don't reload i after doing checks. */
+ rmb();

if (unlikely(i >= vq->vring.num)) {
BAD_RING(vq, "id %u out of range\n", i);
@@ -315,7 +316,8 @@ static void *vring_get_buf(struct virtqu
/* detach_buf clears data, so grab it now. */
ret = vq->data[i];
detach_buf(vq, i);
- vq->last_used_idx++;
+ vring_last_used(&vq->vring)++;
+
END_USE(vq);
return ret;
}
@@ -402,7 +404,6 @@ struct virtqueue *vring_new_virtqueue(un
vq->vq.name = name;
vq->notify = notify;
vq->broken = false;
- vq->last_used_idx = 0;
vq->num_added = 0;
list_add_tail(&vq->vq.list, &vdev->vqs);
#ifdef DEBUG
@@ -413,6 +414,10 @@ struct virtqueue *vring_new_virtqueue(un

vq->indirect = virtio_has_feature(vdev, VIRTIO_RING_F_INDIRECT_DESC);

+ /* We publish indices whether they offer it or not: if not, it's junk
+ * space anyway. But calling this acknowledges the feature. */
+ virtio_has_feature(vdev, VIRTIO_RING_F_PUBLISH_INDICES);
+
/* No callback? Tell other side not to bother us. */
if (!callback)
vq->vring.avail->flags |= VRING_AVAIL_F_NO_INTERRUPT;
@@ -443,6 +448,8 @@ void vring_transport_features(struct vir
switch (i) {
case VIRTIO_RING_F_INDIRECT_DESC:
break;
+ case VIRTIO_RING_F_PUBLISH_INDICES:
+ break;
default:
/* We don't understand this bit. */
clear_bit(i, vdev->features);
diff --git a/include/linux/virtio_ring.h b/include/linux/virtio_ring.h
--- a/include/linux/virtio_ring.h
+++ b/include/linux/virtio_ring.h
@@ -29,6 +29,9 @@
/* We support indirect buffer descriptors */
#define VIRTIO_RING_F_INDIRECT_DESC 28

+/* We publish our last-seen used index at the end of the avail ring. */
+#define VIRTIO_RING_F_PUBLISH_INDICES 29
+
/* Virtio ring descriptors: 16 bytes. These can chain together via "next". */
struct vring_desc
{
@@ -87,6 +90,7 @@ struct vring {
* __u16 avail_flags;
* __u16 avail_idx;
* __u16 available[num];
+ * __u16 last_used_idx;
*
* // Padding to the next align boundary.
* char pad[];
@@ -95,6 +99,7 @@ struct vring {
* __u16 used_flags;
* __u16 used_idx;
* struct vring_used_elem used[num];
+ * __u16 last_avail_idx;
* };
*/
static inline void vring_init(struct vring *vr, unsigned int num, void *p,
@@ -111,9 +116,14 @@ static inline unsigned vring_size(unsign
{
return ((sizeof(struct vring_desc) * num + sizeof(__u16) * (2 + num)
+ align - 1) & ~(align - 1))
- + sizeof(__u16) * 2 + sizeof(struct vring_used_elem) * num;
+ + sizeof(__u16) * 2 + sizeof(struct vring_used_elem) * num + 2;
}

+/* We publish the last-seen used index at the end of the available ring, and
+ * vice-versa. These are at the end for backwards compatibility. */
+#define vring_last_used(vr) ((vr)->avail->ring[(vr)->num])
+#define vring_last_avail(vr) (*(__u16 *)&(vr)->used->ring[(vr)->num])
+
#ifdef __KERNEL__
#include <linux/irqreturn.h>
struct virtio_device;

[Michael S. Tsirkin]

On Mon, Nov 9, 2009 at 8:17 AM, Rusty Russell <ru...@rustcorp.com.au> wrote:
>> > > +static void vhost_net_set_features(struct vhost_net *n, u64 features)
>> > > +{
>> > > + size_t hdr_size = features & (1 << VHOST_NET_F_VIRTIO_NET_HDR) ?
>> > > + � � � � sizeof(struct virtio_net_hdr) : 0;
>> > > + int i;
>> > > + mutex_lock(&n->dev.mutex);
>> > > + n->dev.acked_features = features;
>> >
>> > Why is this called "acked_features"? �Not just "features"? �I expected
>> > to see code which exposed these back to userspace, and didn't.
>>
>> Not sure how do you mean. Userspace sets them, why
>> does it want to get them exposed back?
>
> There's something about the 'acked' which rubs me the wrong way.
> "enabled_features" is perhaps a better term than "acked_features"; "acked"
> seems more a user point-of-view, "enabled" seems more driver POV?
>

Hmm. Are you happy with the ioctl name? If yes I think being consistent
with that is important.

> set_features matches your ioctl names, but it sounds like a fn name :(
>
> It's marginal. �And 'features' is shorter than both.

I started with this but I was always getting confused whether this
includes all features or just acked features. I'll go with
enabled_features.

>
>> > > + switch (ioctl) {
>> > > + case VHOST_SET_VRING_NUM:
>> >
>> > I haven't looked at your userspace implementation, but does a generic
>> > VHOST_SET_VRING_STATE & VHOST_GET_VRING_STATE with a struct make more
>> > sense? �It'd be simpler here,
>>
>> Not by much though, right?
>>
>> > but not sure if it'd be simpler to use?
>>
>> The problem is with VHOST_SET_VRING_BASE as well. I want it to be
>> separate because I want to make it possible to relocate e.g. used ring
>> to another address while ring is running. This would be a good debugging
>> tool (you look at kernel's used ring, check descriptor, then update
>> guest's used ring) and also possibly an extra way to do migration. �And
>> it's nicer to have vring size separate as well, because it is
>> initialized by host and never changed, right?
>
> Actually, this looks wrong to me:
>
> + � � � case VHOST_SET_VRING_BASE:
> ...
> + � � � � � � � vq->avail_idx = vq->last_avail_idx = s.num;
>
> The last_avail_idx is part of the state of the driver. �It needs to be saved
> and restored over susp/resume.

Exactly. That's what VHOST_GET/SET_VRING_BASE does. avail_idx is just a
cached value for notify on empty, so what this does is clear the value.
What exactly do you refer to when you say "this looks wrong"?
This could trigger an extra notification if I ever called
trigger_irq without get first. As I don't, it in fact has no effect.

> �The only reason it's not in the ring itself

> is because I figured the other side doesn't need to see it (which is true, but
> missed debugging opportunities as well as man-in-the-middle issues like this
> one). �I had a patch which put this field at the end of the ring, I might
> resurrect it to avoid this problem. �This is backwards compatible with all
> implementations. �See patch at end.

Yes, I remember that patch. There seems to be little point though, at
this stage.

>
> I would drop avail_idx altogether: get_user is basically free, and simplifies
> a lot. �As most state is in the ring, all you need is an ioctl to save/restore
> the last_avail_idx.

avail_idx is there for notify on empty: I had this thought that it's
better to leave the avail cache line alone when we are triggering
interrupt to avoid bouncing it around if guest is updating it meanwhile
on another CPU, and I think my testing showed that it helped
performance, but could be a mistake. You don't believe this can help?

>
>> We could merge DESC, AVAIL, USED, and it will reduce the amount of code
>> in userspace. With both base, size and fds separate, it seemed a bit
>> more symmetrical to have desc/avail/used separate as well.
>> What's your opinion?
>
> Well, DESC, AVAIL, and USED could easily be turned into SET/GET_LAYOUT.

Will do.

>
>> > For future reference, this is *exactly* the kind of thing which would have
>> > been nice as a followup patch. �Easy to separate, easy to review, not critical
>> > to the core.
>>
>> Yes. It's not too late to split it out though: should I do it yet?
>
> Only if you're feeling enthused. �It's lightly reviewed now.

Not really :) I'll keep this in mind for the future.
Thanks!

>
> Cheers,
> Rusty.

[Michael S. Tsirkin]

Hmm. Are you happy with the ioctl name? If yes I think being consistent
with that is important.

> It's marginal. And 'features' is shorter than both.

I started with this but I was always getting confused whether this

includes all features or just acked features. I'll go with
enabled_features.

> > > > + switch (ioctl) {

> > > > + case VHOST_SET_VRING_NUM:
> > >
> > > I haven't looked at your userspace implementation, but does a generic
> > > VHOST_SET_VRING_STATE & VHOST_GET_VRING_STATE with a struct make more
> > > sense? It'd be simpler here,
> >
> > Not by much though, right?
> >
> > > but not sure if it'd be simpler to use?
> >
> > The problem is with VHOST_SET_VRING_BASE as well. I want it to be
> > separate because I want to make it possible to relocate e.g. used ring
> > to another address while ring is running. This would be a good debugging
> > tool (you look at kernel's used ring, check descriptor, then update
> > guest's used ring) and also possibly an extra way to do migration. And
> > it's nicer to have vring size separate as well, because it is
> > initialized by host and never changed, right?
>
> Actually, this looks wrong to me:
>
> + case VHOST_SET_VRING_BASE:
> ...
> + vq->avail_idx = vq->last_avail_idx = s.num;
>
> The last_avail_idx is part of the state of the driver. It needs to be saved
> and restored over susp/resume.

Exactly. That's what VHOST_GET/SET_VRING_BASE does. avail_idx is just a

cached value for notify on empty, so what this does is clear the value.
What exactly do you refer to when you say "this looks wrong"?
This could trigger an extra notification if I ever called
trigger_irq without get first. As I don't, it in fact has no effect.

> The only reason it's not in the ring itself

> is because I figured the other side doesn't need to see it (which is true, but
> missed debugging opportunities as well as man-in-the-middle issues like this
> one). I had a patch which put this field at the end of the ring, I might
> resurrect it to avoid this problem. This is backwards compatible with all
> implementations. See patch at end.

Yes, I remember that patch. There seems to be little point though, at
this stage.

>

> I would drop avail_idx altogether: get_user is basically free, and
> simplifies a lot. As most state is in the ring, all you need is an
> ioctl to save/restore the last_avail_idx.

avail_idx is there for notify on empty: I had this thought that it's

better to leave the avail cache line alone when we are triggering
interrupt to avoid bouncing it around if guest is updating it meanwhile
on another CPU, and I think my testing showed that it helped
performance, but could be a mistake. You don't believe this can help?

> > We could merge DESC, AVAIL, USED, and it will reduce the amount of code

> > in userspace. With both base, size and fds separate, it seemed a bit
> > more symmetrical to have desc/avail/used separate as well.
> > What's your opinion?
>
> Well, DESC, AVAIL, and USED could easily be turned into SET/GET_LAYOUT.

OK, I'll do this.

> > > For future reference, this is *exactly* the kind of thing which would have
> > > been nice as a followup patch. Easy to separate, easy to review, not critical
> > > to the core.
> >
> > Yes. It's not too late to split it out though: should I do it yet?
>
> Only if you're feeling enthused. It's lightly reviewed now.

Not really :) I'll keep this in mind for the future.
Thanks!

> Cheers,

[Michael S. Tsirkin]

On Mon, Nov 09, 2009 at 04:47:29PM +1030, Rusty Russell wrote:

> Actually, this looks wrong to me:
>
> + case VHOST_SET_VRING_BASE:
> ...
> + vq->avail_idx = vq->last_avail_idx = s.num;
>
> The last_avail_idx is part of the state of the driver. It needs to be saved
> and restored over susp/resume. The only reason it's not in the ring itself
> is because I figured the other side doesn't need to see it (which is true, but
> missed debugging opportunities as well as man-in-the-middle issues like this
> one). I had a patch which put this field at the end of the ring, I might
> resurrect it to avoid this problem. This is backwards compatible with all
> implementations. See patch at end.
>
> I would drop avail_idx altogether: get_user is basically free, and simplifies
> a lot. As most state is in the ring, all you need is an ioctl to save/restore
> the last_avail_idx.

I remembered another reason for caching head in avail_idx. Basically,
avail index could change between when I poll for descriptors and when I
want to notify guest.

So we could have:
- poll descriptors until empty
- notify
detects not empty so does not notify

And the way to solve it would be to return flag from
notify telling us to restart the polling loop.

But, this will be more code, on data path, than
what happens today where I simply keep state
from descriptor polling and use that to notify.

I also suspect that somehow this race in practice can not create
deadlocks ... but I prefer to avoid it, these things are very tricky: if
I see an empty ring, and stop processing descriptors, I want to trigger
notify on empty.

So if we want to avoid keeping "empty" state, IMO the best way would be
to pass a flag to vhost_signal that tells it that ring is empty.
Makes sense?

--
MST

[Michael S. Tsirkin]

Tun device looks similar to a packet socket
in that both pass complete frames from/to userspace.

This patch fills in enough fields in the socket underlying tun driver
to support sendmsg/recvmsg operations, and message flags
MSG_TRUNC and MSG_DONTWAIT, and exports access to this socket
to modules. Regular read/write behaviour is unchanged.

This way, code using raw sockets to inject packets
into a physical device, can support injecting
packets into host network stack almost without modification.

First user of this interface will be vhost virtualization
accelerator.

Signed-off-by: Michael S. Tsirkin <m...@redhat.com>

Acked-by: Herbert Xu <her...@gondor.apana.org.au>
Acked-by: David S. Miller <da...@davemloft.net>
---

+ return ret;
+}
+

+static ssize_t tun_chr_aio_read(struct kiocb *iocb, const struct iovec *iv,
+ unsigned long count, loff_t pos)

+{

+ struct file *file = iocb->ki_filp;
+ struct tun_file *tfile = file->private_data;
+ struct tun_struct *tun = __tun_get(tfile);
+ ssize_t len, ret;
+
+ if (!tun)
+ return -EBADFD;
+ len = iov_length(iv, count);
+ if (len < 0) {
+ ret = -EINVAL;
+ goto out;

+ }
+

+ return ret;
+}
+

index 3f5fd52..404abe0 100644
--- a/include/linux/if_tun.h
+++ b/include/linux/if_tun.h
@@ -86,4 +86,18 @@ struct tun_filter {
__u8 addr[0][ETH_ALEN];
};

+#ifdef __KERNEL__
+#if defined(CONFIG_TUN) || defined(CONFIG_TUN_MODULE)
+struct socket *tun_get_socket(struct file *);
+#else
+#include <linux/err.h>
+#include <linux/errno.h>
+struct file;
+struct socket;
+static inline struct socket *tun_get_socket(struct file *f)
+{
+ return ERR_PTR(-EINVAL);
+}
+#endif /* CONFIG_TUN */
+#endif /* __KERNEL__ */
#endif /* __IF_TUN_H */

--
1.6.5.2.143.g8cc62

[Michael S. Tsirkin]

vhost net module wants to do copy to/from user from a kernel thread,
which needs use_mm. Export it to modules.

Acked-by: Andrea Arcangeli <aarc...@redhat.com>
Acked-by: Andrew Morton <ak...@linux-foundation.org>

Signed-off-by: Michael S. Tsirkin <m...@redhat.com>

---
mm/mmu_context.c | 3 +++
1 files changed, 3 insertions(+), 0 deletions(-)

diff --git a/mm/mmu_context.c b/mm/mmu_context.c
index ded9081..0777654 100644
--- a/mm/mmu_context.c
+++ b/mm/mmu_context.c
@@ -5,6 +5,7 @@

#include <linux/mm.h>
#include <linux/mmu_context.h>
+#include <linux/module.h>
#include <linux/sched.h>

#include <asm/mmu_context.h>
@@ -37,6 +38,7 @@ void use_mm(struct mm_struct *mm)
if (active_mm != mm)
mmdrop(active_mm);
}
+EXPORT_SYMBOL_GPL(use_mm);

/*
* unuse_mm
@@ -56,3 +58,4 @@ void unuse_mm(struct mm_struct *mm)
enter_lazy_tlb(mm, tsk);
task_unlock(tsk);
}
+EXPORT_SYMBOL_GPL(unuse_mm);

[Michael S. Tsirkin]

What it is: vhost net is a character device that can be used to reduce
the number of system calls involved in virtio networking.

what is happening is that the rcu_dereference() is being used in a
workqueue item. The role of rcu_read_lock() is taken on by the start of

execution of the workqueue item, of rcu_read_unlock() by the end of
execution of the workqueue item, and of synchronize_rcu() by

flush_workqueue()/flush_work(). In the future we might need to apply

some gcc attribute or sparse annotation to the function passed to

INIT_WORK(). Paul's ack below is for this RCU usage.

Acked-by: Arnd Bergmann <ar...@arndb.de>
Acked-by: "Paul E. McKenney" <pau...@linux.vnet.ibm.com>

Signed-off-by: Michael S. Tsirkin <m...@redhat.com>
---

MAINTAINERS | 9 +
arch/x86/kvm/Kconfig | 1 +
drivers/Makefile | 1 +
drivers/vhost/Kconfig | 11 +
drivers/vhost/Makefile | 2 +

drivers/vhost/net.c | 648 +++++++++++++++++++++++++++++
drivers/vhost/vhost.c | 965 ++++++++++++++++++++++++++++++++++++++++++++
drivers/vhost/vhost.h | 159 ++++++++

include/linux/Kbuild | 1 +
include/linux/miscdevice.h | 1 +

include/linux/vhost.h | 130 ++++++
11 files changed, 1928 insertions(+), 0 deletions(-)

create mode 100644 drivers/vhost/Kconfig
create mode 100644 drivers/vhost/Makefile
create mode 100644 drivers/vhost/net.c
create mode 100644 drivers/vhost/vhost.c
create mode 100644 drivers/vhost/vhost.h
create mode 100644 include/linux/vhost.h

diff --git a/MAINTAINERS b/MAINTAINERS
index a1a2ace..7d4bfa2 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -5636,6 +5636,15 @@ S: Maintained

index 0000000..22d5fef
--- /dev/null
+++ b/drivers/vhost/net.c
@@ -0,0 +1,648 @@

+/* Caller must have TX VQ lock */
+static void tx_poll_stop(struct vhost_net *net)
+{
+ if (likely(net->tx_poll_state != VHOST_NET_POLL_STARTED))
+ return;

+ vhost_poll_stop(net->poll + VHOST_NET_VQ_TX);
+ net->tx_poll_state = VHOST_NET_POLL_STOPPED;
+}
+

+/* Caller must have TX VQ lock */

+static void tx_poll_start(struct vhost_net *net, struct socket *sock)
+{
+ if (unlikely(net->tx_poll_state != VHOST_NET_POLL_STOPPED))
+ return;
+ vhost_poll_start(net->poll + VHOST_NET_VQ_TX, sock->file);
+ net->tx_poll_state = VHOST_NET_POLL_STARTED;
+}
+
+/* Expects to be always run from workqueue - which acts as
+ * read-size critical section for our kind of RCU. */

+static void handle_tx(struct vhost_net *net)
+{
+ struct vhost_virtqueue *vq = &net->dev.vqs[VHOST_NET_VQ_TX];
+ unsigned head, out, in, s;
+ struct msghdr msg = {
+ .msg_name = NULL,
+ .msg_namelen = 0,
+ .msg_control = NULL,
+ .msg_controllen = 0,
+ .msg_iov = vq->iov,
+ .msg_flags = MSG_DONTWAIT,
+ };
+ size_t len, total_len = 0;
+ int err, wmem;
+ size_t hdr_size;
+ struct socket *sock = rcu_dereference(vq->private_data);
+ if (!sock)
+ return;
+
+ wmem = atomic_read(&sock->sk->sk_wmem_alloc);
+ if (wmem >= sock->sk->sk_sndbuf)
+ return;

+
+ use_mm(net->dev.mm);
+ mutex_lock(&vq->mutex);

+ vhost_disable_notify(vq);

+
+ if (wmem < sock->sk->sk_sndbuf * 2)
+ tx_poll_stop(net);
+ hdr_size = vq->hdr_size;
+

+ for (;;) {
+ head = vhost_get_vq_desc(&net->dev, vq, vq->iov,

+ ARRAY_SIZE(vq->iov),
+ &out, &in,
+ NULL, NULL);

+ /* Nothing new? Wait for eventfd to tell us they refilled. */
+ if (head == vq->num) {

+ wmem = atomic_read(&sock->sk->sk_wmem_alloc);

+ if (wmem >= sock->sk->sk_sndbuf * 3 / 4) {
+ tx_poll_start(net, sock);
+ set_bit(SOCK_ASYNC_NOSPACE, &sock->flags);

+ break;
+ }
+ if (unlikely(vhost_enable_notify(vq))) {
+ vhost_disable_notify(vq);
+ continue;
+ }
+ break;
+ }

+ if (in) {
+ vq_err(vq, "Unexpected descriptor format for TX: "
+ "out %d, int %d\n", out, in);
+ break;
+ }

+ /* Skip header. TODO: support TSO. */
+ s = move_iovec_hdr(vq->iov, vq->hdr, hdr_size, out);

+ msg.msg_iovlen = out;
+ len = iov_length(vq->iov, out);
+ /* Sanity check */
+ if (!len) {
+ vq_err(vq, "Unexpected header len for TX: "
+ "%zd expected %zd\n",
+ iov_length(vq->hdr, s), hdr_size);
+ break;
+ }
+ /* TODO: Check specific error and bomb out unless ENOBUFS? */
+ err = sock->ops->sendmsg(NULL, sock, &msg, len);
+ if (unlikely(err < 0)) {
+ vhost_discard_vq_desc(vq);
+ tx_poll_start(net, sock);

+ break;
+ }

+ if (err != len)
+ pr_err("Truncated TX packet: "
+ " len %d != %zd\n", err, len);

+ vhost_add_used_and_signal(&net->dev, vq, head, 0);

+ total_len += len;
+ if (unlikely(total_len >= VHOST_NET_WEIGHT)) {
+ vhost_poll_queue(&vq->poll);
+ break;
+ }
+ }
+
+ mutex_unlock(&vq->mutex);
+ unuse_mm(net->dev.mm);
+}
+
+/* Expects to be always run from workqueue - which acts as
+ * read-size critical section for our kind of RCU. */

+static void handle_rx(struct vhost_net *net)
+{

+ struct vhost_virtqueue *vq = &net->dev.vqs[VHOST_NET_VQ_RX];
+ unsigned head, out, in, log, s;
+ struct vhost_log *vq_log;

+ struct msghdr msg = {
+ .msg_name = NULL,
+ .msg_namelen = 0,

+ .msg_control = NULL, /* FIXME: get and handle RX aux data. */

+ .msg_controllen = 0,
+ .msg_iov = vq->iov,
+ .msg_flags = MSG_DONTWAIT,
+ };
+

+ struct virtio_net_hdr hdr = {
+ .flags = 0,
+ .gso_type = VIRTIO_NET_HDR_GSO_NONE
+ };

+
+ size_t len, total_len = 0;

+ int err;
+ size_t hdr_size;
+ struct socket *sock = rcu_dereference(vq->private_data);
+ if (!sock || skb_queue_empty(&sock->sk->sk_receive_queue))
+ return;

+
+ use_mm(net->dev.mm);
+ mutex_lock(&vq->mutex);

+ vhost_disable_notify(vq);

+ hdr_size = vq->hdr_size;
+
+ vq_log = unlikely(vhost_has_feature(&net->dev, VHOST_F_LOG_ALL)) ?
+ vq->log : NULL;
+

+ for (;;) {
+ head = vhost_get_vq_desc(&net->dev, vq, vq->iov,

+ ARRAY_SIZE(vq->iov),
+ &out, &in,
+ vq_log, &log);

+ /* OK, now we need to know about added descriptors. */

+ if (head == vq->num) {
+ if (unlikely(vhost_enable_notify(vq))) {
+ /* They have slipped one in as we were
+ * doing that: check again. */
+ vhost_disable_notify(vq);
+ continue;
+ }

+ /* Nothing new? Wait for eventfd to tell us

+ * they refilled. */
+ break;
+ }

+ /* We don't need to be notified again. */

+ len = err;
+ err = memcpy_toiovec(vq->hdr, (unsigned char *)&hdr, hdr_size);

+ if (err) {
+ vq_err(vq, "Unable to write vnet_hdr at addr %p: %d\n",
+ vq->iov->iov_base, err);
+ break;
+ }
+ len += hdr_size;

+ vhost_add_used_and_signal(&net->dev, vq, head, len);

+ if (unlikely(vq_log))
+ vhost_log_write(vq, vq_log, log, len);
+ total_len += len;
+ if (unlikely(total_len >= VHOST_NET_WEIGHT)) {
+ vhost_poll_queue(&vq->poll);
+ break;
+ }
+ }
+
+ mutex_unlock(&vq->mutex);
+ unuse_mm(net->dev.mm);
+}
+
+static void handle_tx_kick(struct work_struct *work)

+{

+ struct vhost_virtqueue *vq;
+ struct vhost_net *net;
+ vq = container_of(work, struct vhost_virtqueue, poll.work);
+ net = container_of(vq->dev, struct vhost_net, dev);
+ handle_tx(net);
+}
+
+static void handle_rx_kick(struct work_struct *work)

+{

+ struct vhost_virtqueue *vq;
+ struct vhost_net *net;
+ vq = container_of(work, struct vhost_virtqueue, poll.work);
+ net = container_of(vq->dev, struct vhost_net, dev);
+ handle_rx(net);
+}
+
+static void handle_tx_net(struct work_struct *work)
+{
+ struct vhost_net *net;
+ net = container_of(work, struct vhost_net, poll[VHOST_NET_VQ_TX].work);
+ handle_tx(net);
+}
+
+static void handle_rx_net(struct work_struct *work)
+{
+ struct vhost_net *net;
+ net = container_of(work, struct vhost_net, poll[VHOST_NET_VQ_RX].work);
+ handle_rx(net);
+}
+

+static int vhost_net_open(struct inode *inode, struct file *f)
+{

+ struct vhost_net *n = kmalloc(sizeof *n, GFP_KERNEL);

+ int r;
+ if (!n)
+ return -ENOMEM;
+ f->private_data = n;
+ n->vqs[VHOST_NET_VQ_TX].handle_kick = handle_tx_kick;
+ n->vqs[VHOST_NET_VQ_RX].handle_kick = handle_rx_kick;

+ r = vhost_dev_init(&n->dev, n->vqs, VHOST_NET_VQ_MAX);

+ if (r < 0) {

+ kfree(n);
+ return r;
+ }
+
+ vhost_poll_init(n->poll + VHOST_NET_VQ_TX, handle_tx_net, POLLOUT);
+ vhost_poll_init(n->poll + VHOST_NET_VQ_RX, handle_rx_net, POLLIN);
+ n->tx_poll_state = VHOST_NET_POLL_DISABLED;
+ return 0;
+}
+
+static void vhost_net_disable_vq(struct vhost_net *n,

+ struct vhost_virtqueue *vq)
+{
+ if (!vq->private_data)
+ return;
+ if (vq == n->vqs + VHOST_NET_VQ_TX) {

+ tx_poll_stop(n);
+ n->tx_poll_state = VHOST_NET_POLL_DISABLED;
+ } else
+ vhost_poll_stop(n->poll + VHOST_NET_VQ_RX);
+}
+

+static void vhost_net_enable_vq(struct vhost_net *n,

+ struct vhost_virtqueue *vq)
+{
+ struct socket *sock = vq->private_data;

+ if (!sock)
+ return;

+ if (vq == n->vqs + VHOST_NET_VQ_TX) {

+ n->tx_poll_state = VHOST_NET_POLL_STOPPED;
+ tx_poll_start(n, sock);
+ } else
+ vhost_poll_start(n->poll + VHOST_NET_VQ_RX, sock->file);
+}
+
+static struct socket *vhost_net_stop_vq(struct vhost_net *n,

+ struct vhost_virtqueue *vq)
+{
+ struct socket *sock;
+
+ mutex_lock(&vq->mutex);
+ sock = vq->private_data;
+ vhost_net_disable_vq(n, vq);
+ rcu_assign_pointer(vq->private_data, NULL);
+ mutex_unlock(&vq->mutex);

+ return sock;
+}
+
+static void vhost_net_stop(struct vhost_net *n, struct socket **tx_sock,
+ struct socket **rx_sock)
+{

+ *tx_sock = vhost_net_stop_vq(n, n->vqs + VHOST_NET_VQ_TX);
+ *rx_sock = vhost_net_stop_vq(n, n->vqs + VHOST_NET_VQ_RX);
+}
+
+static void vhost_net_flush_vq(struct vhost_net *n, int index)
+{

+ vhost_poll_flush(n->poll + index);
+ vhost_poll_flush(&n->dev.vqs[index].poll);
+}
+
+static void vhost_net_flush(struct vhost_net *n)
+{
+ vhost_net_flush_vq(n, VHOST_NET_VQ_TX);
+ vhost_net_flush_vq(n, VHOST_NET_VQ_RX);
+}
+

+static int vhost_net_release(struct inode *inode, struct file *f)
+{

+ struct socket *sock = sockfd_lookup(fd, &r);
+ if (!sock)

+ return ERR_PTR(-ENOTSOCK);
+
+ /* Parameter checking */
+ if (sock->sk->sk_type != SOCK_RAW) {
+ r = -ESOCKTNOSUPPORT;
+ goto err;
+ }
+
+ r = sock->ops->getname(sock, (struct sockaddr *)&uaddr.sa,
+ &uaddr_len, 0);
+ if (r)
+ goto err;
+
+ if (uaddr.sa.sll_family != AF_PACKET) {
+ r = -EPFNOSUPPORT;
+ goto err;
+ }
+ return sock;
+err:
+ fput(sock->file);
+ return ERR_PTR(r);
+}
+
+static struct socket *get_tun_socket(int fd)

+{

+ struct file *file = fget(fd);
+ struct socket *sock;
+ if (!file)
+ return ERR_PTR(-EBADF);
+ sock = tun_get_socket(file);
+ if (IS_ERR(sock))
+ fput(file);
+ return sock;
+}
+

+static struct socket *get_socket(int fd)
+{

+ struct socket *sock;
+ if (fd == -1)
+ return NULL;
+ sock = get_raw_socket(fd);
+ if (!IS_ERR(sock))
+ return sock;
+ sock = get_tun_socket(fd);
+ if (!IS_ERR(sock))
+ return sock;
+ return ERR_PTR(-ENOTSOCK);
+}
+

+static long vhost_net_set_backend(struct vhost_net *n, unsigned index, int fd)
+{

+ struct socket *sock, *oldsock;

+ struct vhost_virtqueue *vq;
+ int r;
+
+ mutex_lock(&n->dev.mutex);
+ r = vhost_dev_check_owner(&n->dev);
+ if (r)

+ goto err;

+
+ if (index >= VHOST_NET_VQ_MAX) {
+ r = -ENOBUFS;

+ goto err;

+ }
+ vq = n->vqs + index;
+ mutex_lock(&vq->mutex);

+ sock = get_socket(fd);
+ if (IS_ERR(sock)) {
+ r = PTR_ERR(sock);

+ goto err;

+ }
+
+ /* start polling new socket */
+ oldsock = vq->private_data;

+ if (sock == oldsock)

+ goto done;
+
+ vhost_net_disable_vq(n, vq);
+ rcu_assign_pointer(vq->private_data, sock);
+ vhost_net_enable_vq(n, vq);
+ mutex_unlock(&vq->mutex);

+done:
+ mutex_unlock(&n->dev.mutex);
+ if (oldsock) {
+ vhost_net_flush_vq(n, index);
+ fput(oldsock->file);

+ }
+ return r;
+err:
+ mutex_unlock(&n->dev.mutex);

+ return r;
+}
+

+static long vhost_net_reset_owner(struct vhost_net *n)
+{
+ struct socket *tx_sock = NULL;
+ struct socket *rx_sock = NULL;

+ long err;
+ mutex_lock(&n->dev.mutex);
+ err = vhost_dev_check_owner(&n->dev);
+ if (err)

+ goto done;
+ vhost_net_stop(n, &tx_sock, &rx_sock);
+ vhost_net_flush(n);

+ err = vhost_dev_reset_owner(&n->dev);
+done:
+ mutex_unlock(&n->dev.mutex);

+ if (tx_sock)
+ fput(tx_sock->file);
+ if (rx_sock)
+ fput(rx_sock->file);

+ return err;
+}
+

+static void vhost_net_set_features(struct vhost_net *n, u64 features)
+{
+ size_t hdr_size = features & (1 << VHOST_NET_F_VIRTIO_NET_HDR) ?
+ sizeof(struct virtio_net_hdr) : 0;
+ int i;
+ mutex_lock(&n->dev.mutex);
+ n->dev.acked_features = features;

+ smp_wmb();
+ for (i = 0; i < VHOST_NET_VQ_MAX; ++i) {
+ mutex_lock(&n->vqs[i].mutex);
+ n->vqs[i].hdr_size = hdr_size;
+ mutex_unlock(&n->vqs[i].mutex);
+ }
+ mutex_unlock(&n->dev.mutex);
+ vhost_net_flush(n);
+}
+
+static long vhost_net_ioctl(struct file *f, unsigned int ioctl,
+ unsigned long arg)

+{

+ struct vhost_net *n = f->private_data;
+ void __user *argp = (void __user *)arg;
+ u32 __user *featurep = argp;
+ struct vhost_vring_file backend;
+ u64 features;
+ int r;

+ switch (ioctl) {

+ case VHOST_NET_SET_BACKEND:
+ r = copy_from_user(&backend, argp, sizeof backend);

+ if (r < 0)
+ return r;

+ return vhost_net_set_backend(n, backend.index, backend.fd);

+ case VHOST_GET_FEATURES:
+ features = VHOST_FEATURES;
+ return put_user(features, featurep);

+ case VHOST_SET_FEATURES:

+ r = get_user(features, featurep);
+ /* No features for now */
+ if (r < 0)
+ return r;
+ if (features & ~VHOST_FEATURES)
+ return -EOPNOTSUPP;
+ vhost_net_set_features(n, features);

index 0000000..97233d5
--- /dev/null
+++ b/drivers/vhost/vhost.c
@@ -0,0 +1,965 @@

+static void vhost_vq_reset(struct vhost_dev *dev,
+ struct vhost_virtqueue *vq)
+{
+ vq->num = 1;
+ vq->desc = NULL;
+ vq->avail = NULL;
+ vq->used = NULL;
+ vq->last_avail_idx = 0;
+ vq->avail_idx = 0;
+ vq->last_used_idx = 0;
+ vq->used_flags = 0;
+ vq->used_flags = 0;

+ vq->log_used = false;

+ vq->log_addr = -1ull;
+ vq->hdr_size = 0;
+ vq->private_data = NULL;
+ vq->log_base = NULL;
+ vq->error_ctx = NULL;
+ vq->error = NULL;
+ vq->kick = NULL;
+ vq->call_ctx = NULL;
+ vq->call = NULL;

+}
+
+long vhost_dev_init(struct vhost_dev *dev,
+ struct vhost_virtqueue *vqs, int nvqs)
+{
+ int i;
+ dev->vqs = vqs;
+ dev->nvqs = nvqs;
+ mutex_init(&dev->mutex);

+ dev->log_ctx = NULL;

+ dev->log_file = NULL;

+ dev->memory = NULL;

+ dev->mm = NULL;
+

+ for (i = 0; i < dev->nvqs; ++i) {
+ dev->vqs[i].dev = dev;
+ mutex_init(&dev->vqs[i].mutex);

+ vhost_vq_reset(dev, dev->vqs + i);

+ memory = kmalloc(offsetof(struct vhost_memory, regions) +

+ vhost_vq_reset(dev, dev->vqs + i);

+ }
+ if (dev->log_ctx)
+ eventfd_ctx_put(dev->log_ctx);
+ dev->log_ctx = NULL;
+ if (dev->log_file)
+ fput(dev->log_file);
+ dev->log_file = NULL;
+ /* No one will access memory at this point */
+ kfree(dev->memory);
+ dev->memory = NULL;
+ if (dev->mm)
+ mmput(dev->mm);
+ dev->mm = NULL;
+}
+
+static long vhost_set_memory(struct vhost_dev *d, struct vhost_memory __user *m)
+{
+ struct vhost_memory mem, *newmem, *oldmem;
+ unsigned long size = offsetof(struct vhost_memory, regions);
+ long r;
+ r = copy_from_user(&mem, m, size);
+ if (r)

+ return r;

+ if (mem.padding)
+ return -EOPNOTSUPP;
+ if (mem.nregions > VHOST_MEMORY_MAX_NREGIONS)
+ return -E2BIG;
+ newmem = kmalloc(size + mem.nregions * sizeof *m->regions, GFP_KERNEL);
+ if (!newmem)
+ return -ENOMEM;
+
+ memcpy(newmem, &mem, size);
+ r = copy_from_user(newmem->regions, m->regions,
+ mem.nregions * sizeof *m->regions);
+ if (r) {
+ kfree(newmem);
+ return r;
+ }
+ oldmem = d->memory;
+ rcu_assign_pointer(d->memory, newmem);
+ synchronize_rcu();
+ kfree(oldmem);
+ return 0;
+}
+

+static int init_used(struct vhost_virtqueue *vq,
+ struct vring_used __user *used)
+{
+ int r = put_user(vq->used_flags, &used->flags);

+ if (r)
+ return r;

+ return get_user(vq->last_used_idx, &used->idx);

+}
+
+static long vhost_set_vring(struct vhost_dev *d, int ioctl, void __user *argp)
+{
+ struct file *eventfp, *filep = NULL,
+ *pollstart = NULL, *pollstop = NULL;
+ struct eventfd_ctx *ctx = NULL;
+ u32 __user *idxp = argp;
+ struct vhost_virtqueue *vq;
+ struct vhost_vring_state s;
+ struct vhost_vring_file f;
+ struct vhost_vring_addr a;
+ u32 idx;
+ long r;
+
+ r = get_user(idx, idxp);

+ if (r < 0)
+ return r;

+ if (idx > d->nvqs)
+ return -ENOBUFS;
+
+ vq = d->vqs + idx;
+
+ mutex_lock(&vq->mutex);
+

+ switch (ioctl) {
+ case VHOST_SET_VRING_NUM:

+ r = copy_from_user(&s, argp, sizeof s);

+ if (r < 0)
+ break;

+ if (!s.num || s.num > 0xffff || (s.num & (s.num - 1))) {

+ r = -EINVAL;
+ break;
+ }

+ vq->num = s.num;
+ break;
+ case VHOST_SET_VRING_BASE:
+ r = copy_from_user(&s, argp, sizeof s);

+ if (r < 0)
+ break;

+ if (s.num > 0xffff) {

+ r = -EINVAL;
+ break;
+ }

+ vq->last_avail_idx = s.num;

+ /* Forget the cached index value. */

+ vq->avail_idx = vq->last_avail_idx;

+ break;
+ case VHOST_GET_VRING_BASE:
+ s.index = idx;
+ s.num = vq->last_avail_idx;
+ r = copy_to_user(argp, &s, sizeof s);
+ break;

+ case VHOST_SET_VRING_ADDR:

+ r = copy_from_user(&a, argp, sizeof a);
+ if (r < 0)
+ break;

+ if (a.flags & ~(0x1 << VHOST_VRING_F_LOG)) {

+ r = -EOPNOTSUPP;
+ break;
+ }

+ if ((u64)(unsigned long)a.desc_user_addr != a.desc_user_addr ||
+ (u64)(unsigned long)a.used_user_addr != a.used_user_addr ||
+ (u64)(unsigned long)a.avail_user_addr != a.avail_user_addr) {
+ r = -EFAULT;
+ break;
+ }
+ if ((a.avail_user_addr & (sizeof *vq->avail->ring - 1)) ||
+ (a.used_user_addr & (sizeof *vq->used->ring - 1)) ||
+ (a.log_guest_addr & (sizeof *vq->used->ring - 1))) {

+ r = -EINVAL;
+ break;
+ }

+ r = init_used(vq, (struct vring_used __user *)a.used_user_addr);

+ if (r)
+ break;

+ vq->log_used = !!(a.flags & (0x1 << VHOST_VRING_F_LOG));
+ vq->desc = (void __user *)(unsigned long)a.desc_user_addr;
+ vq->avail = (void __user *)(unsigned long)a.avail_user_addr;
+ vq->log_addr = a.log_guest_addr;
+ vq->used = (void __user *)(unsigned long)a.used_user_addr;

+ break;
+ case VHOST_SET_VRING_KICK:
+ r = copy_from_user(&f, argp, sizeof f);

+ if (r < 0)
+ break;

+ eventfp = f.fd == -1 ? NULL : eventfd_fget(f.fd);
+ if (IS_ERR(eventfp))
+ return PTR_ERR(eventfp);
+ if (eventfp != vq->kick) {
+ pollstop = filep = vq->kick;
+ pollstart = vq->kick = eventfp;
+ } else
+ filep = eventfp;
+ break;
+ case VHOST_SET_VRING_CALL:
+ r = copy_from_user(&f, argp, sizeof f);

+ if (r < 0)
+ break;

+ eventfp = f.fd == -1 ? NULL : eventfd_fget(f.fd);
+ if (IS_ERR(eventfp))
+ return PTR_ERR(eventfp);
+ if (eventfp != vq->call) {
+ filep = vq->call;
+ ctx = vq->call_ctx;
+ vq->call = eventfp;
+ vq->call_ctx = eventfp ?
+ eventfd_ctx_fileget(eventfp) : NULL;
+ } else
+ filep = eventfp;
+ break;
+ case VHOST_SET_VRING_ERR:
+ r = copy_from_user(&f, argp, sizeof f);

+ if (r < 0)
+ break;

+ goto done;
+ }
+

+ /* You must be the owner to do anything else */
+ r = vhost_dev_check_owner(d);
+ if (r)
+ goto done;
+

+ switch (ioctl) {

+ case VHOST_SET_MEM_TABLE:
+ r = vhost_set_memory(d, argp);
+ break;
+ case VHOST_SET_LOG_BASE:
+ r = copy_from_user(&p, argp, sizeof p);

+ if (r < 0)
+ break;

+ if ((u64)(unsigned long)p != p) {
+ r = -EFAULT;
+ break;
+ }
+ for (i = 0; i < d->nvqs; ++i) {
+ mutex_lock(&d->vqs[i].mutex);
+ d->vqs[i].log_base = (void __user *)(unsigned long)p;
+ mutex_unlock(&d->vqs[i].mutex);
+ }
+ break;
+ case VHOST_SET_LOG_FD:
+ r = get_user(fd, (int __user *)argp);

+ if (r < 0)
+ break;

+ eventfp = fd == -1 ? NULL : eventfd_fget(fd);
+ if (IS_ERR(eventfp)) {
+ r = PTR_ERR(eventfp);

+ break;
+ }

+/* TODO: This is really inefficient. We need something like get_user()
+ * (instruction directly accesses the data, with an exception table entry
+ * returning -EFAULT). See Documentation/x86/exception-tables.txt.
+ */
+static int set_bit_to_user(int nr, void __user *addr)
+{

+ unsigned long log = (unsigned long)addr;
+ struct page *page;
+ void *base;
+ int bit = nr + (log % PAGE_SIZE) * 8;
+ int r;
+ r = get_user_pages_fast(log, 1, 1, &page);
+ if (r)
+ return r;
+ base = kmap_atomic(page, KM_USER0);
+ set_bit(bit, base);
+ kunmap_atomic(base, KM_USER0);
+ set_page_dirty_lock(page);
+ put_page(page);
+ return 0;
+}
+
+static int log_write(void __user *log_base,
+ u64 write_address, u64 write_length)
+{

+ int r;

+ if (!write_length)
+ return 0;
+ write_address /= VHOST_PAGE_SIZE;

+ for (;;) {

+ u64 base = (u64)(unsigned long)log_base;
+ u64 log = base + write_address / 8;
+ int bit = write_address % 8;
+ if ((u64)(unsigned long)log != log)
+ return -EFAULT;
+ r = set_bit_to_user(bit, (void __user *)(unsigned long)log);

+ if (r < 0)
+ return r;

+ if (write_length <= VHOST_PAGE_SIZE)
+ break;
+ write_length -= VHOST_PAGE_SIZE;
+ write_address += VHOST_PAGE_SIZE;
+ }
+ return r;
+}
+
+int vhost_log_write(struct vhost_virtqueue *vq, struct vhost_log *log,
+ unsigned int log_num, u64 len)
+{
+ int i, r;
+
+ /* Make sure data written is seen before log. */
+ wmb();
+ for (i = 0; i < log_num; ++i) {
+ u64 l = min(log[i].len, len);
+ r = log_write(vq->log_base, log[i].addr, l);

+ if (r < 0)
+ return r;

+ return ret;
+}
+

+/* Each buffer in the virtqueues is actually a chain of descriptors. This
+ * function returns the next descriptor in the chain,

+ * or -1U if we're at the end. */

+static unsigned next_desc(struct vring_desc *desc)
+{
+ unsigned int next;
+
+ /* If this descriptor says it doesn't chain, we're done. */
+ if (!(desc->flags & VRING_DESC_F_NEXT))

+ return -1U;

+
+ /* Check they're not leading us off end of descriptors. */
+ next = desc->next;
+ /* Make sure compiler knows to grab that: we don't want it changing! */
+ /* We will use the result as an index in an array, so most
+ * architectures only need a compiler barrier here. */
+ read_barrier_depends();
+
+ return next;
+}
+
+static unsigned get_indirect(struct vhost_dev *dev, struct vhost_virtqueue *vq,

+ struct iovec iov[], unsigned int iov_size,

+ unsigned int *out_num, unsigned int *in_num,
+ struct vhost_log *log, unsigned int *log_num,
+ struct vring_desc *indirect)
+{
+ struct vring_desc desc;
+ unsigned int i = 0, count, found = 0;
+ int ret;
+
+ /* Sanity check */
+ if (indirect->len % sizeof desc) {
+ vq_err(vq, "Invalid length in indirect descriptor: "
+ "len 0x%llx not multiple of 0x%zx\n",
+ (unsigned long long)indirect->len,
+ sizeof desc);
+ return -EINVAL;

+ }
+

+ ret = translate_desc(dev, indirect->addr, indirect->len, vq->indirect,
+ ARRAY_SIZE(vq->indirect));
+ if (ret < 0) {
+ vq_err(vq, "Translation failure %d in indirect.\n", ret);

+ return ret;
+ }
+

+ }
+

+ ret = translate_desc(dev, desc.addr, desc.len, iov + iov_count,

+ iov_size - iov_count);

+ if (ret < 0) {
+ vq_err(vq, "Translation failure %d indirect idx %d\n",
+ ret, i);

+ return ret;
+ }

+ struct iovec iov[], unsigned int iov_size,

+ ret = get_indirect(dev, vq, iov, iov_size,
+ out_num, in_num,

+ log, log_num, &desc);
+ if (ret < 0) {
+ vq_err(vq, "Failure detected "
+ "in indirect descriptor at idx %d\n", i);
+ return vq->num;
+ }
+ continue;

+ }
+

+ ret = translate_desc(dev, desc.addr, desc.len, iov + iov_count,

+ iov_size - iov_count);

+/* After we've used one of their buffers, we tell them about it. We'll then

+ * want to notify the guest, using eventfd. */
+int vhost_add_used(struct vhost_virtqueue *vq, unsigned int head, int len)

+/* This actually signals the guest, using eventfd. */
+void vhost_signal(struct vhost_dev *dev, struct vhost_virtqueue *vq)
+{

+ __u16 flags = 0;
+ if (get_user(flags, &vq->avail->flags)) {
+ vq_err(vq, "Failed to get flags");
+ return;
+ }
+

+ /* If they don't want an interrupt, don't signal, unless empty. */

+ if ((flags & VRING_AVAIL_F_NO_INTERRUPT) &&
+ (vq->avail_idx != vq->last_avail_idx ||
+ !vhost_has_feature(dev, VIRTIO_F_NOTIFY_ON_EMPTY)))
+ return;
+

+ /* Signal the Guest tell them we used something up. */

+ if (vq->call_ctx)
+ eventfd_signal(vq->call_ctx, 1);
+}
+
+/* And here's the combo meal deal. Supersize me! */

+void vhost_add_used_and_signal(struct vhost_dev *dev,

+ struct vhost_virtqueue *vq,
+ unsigned int head, int len)
+{
+ vhost_add_used(vq, head, len);

+ vhost_signal(dev, vq);
+}
+
+/* OK, now we need to know about added descriptors. */
+bool vhost_enable_notify(struct vhost_virtqueue *vq)
+{
+ u16 avail_idx;
+ int r;

+ if (!(vq->used_flags & VRING_USED_F_NO_NOTIFY))
+ return false;
+ vq->used_flags &= ~VRING_USED_F_NO_NOTIFY;
+ r = put_user(vq->used_flags, &vq->used->flags);
+ if (r) {

+ vq_err(vq, "Failed to enable notification at %p: %d\n",
+ &vq->used->flags, r);
+ return false;
+ }

+ /* They could have slipped one in as we were doing that: make

+ * sure it's written, then check again. */
+ mb();
+ r = get_user(avail_idx, &vq->avail->idx);
+ if (r) {
+ vq_err(vq, "Failed to check avail idx at %p: %d\n",
+ &vq->avail->idx, r);
+ return false;
+ }
+
+ return avail_idx != vq->last_avail_idx;
+}
+
+/* We don't need to be notified again. */
+void vhost_disable_notify(struct vhost_virtqueue *vq)
+{
+ int r;

+ if (vq->used_flags & VRING_USED_F_NO_NOTIFY)
+ return;
+ vq->used_flags |= VRING_USED_F_NO_NOTIFY;
+ r = put_user(vq->used_flags, &vq->used->flags);
+ if (r)

+ vq_err(vq, "Failed to enable notification at %p: %d\n",
+ &vq->used->flags, r);

+}
+
+int vhost_init(void)
+{
+ vhost_workqueue = create_singlethread_workqueue("vhost");
+ if (!vhost_workqueue)
+ return -ENOMEM;
+ return 0;
+}
+
+void vhost_cleanup(void)
+{
+ destroy_workqueue(vhost_workqueue);
+}
diff --git a/drivers/vhost/vhost.h b/drivers/vhost/vhost.h
new file mode 100644

index 0000000..d1f0453
--- /dev/null
+++ b/drivers/vhost/vhost.h
@@ -0,0 +1,159 @@

+#ifndef _VHOST_H
+#define _VHOST_H
+
+#include <linux/eventfd.h>
+#include <linux/vhost.h>
+#include <linux/mm.h>
+#include <linux/mutex.h>
+#include <linux/workqueue.h>
+#include <linux/poll.h>
+#include <linux/file.h>
+#include <linux/skbuff.h>
+#include <linux/uio.h>
+#include <linux/virtio_config.h>
+#include <linux/virtio_ring.h>
+
+struct vhost_device;
+
+enum {

+ /* Enough place for all fragments, head, and virtio net header. */

+ VHOST_NET_MAX_SG = MAX_SKB_FRAGS + 2,

+};
+

+/* Poll a file (eventfd or socket) */
+/* Note: there's nothing vhost specific about this structure. */
+struct vhost_poll {

+ struct iovec iov[], unsigned int iov_count,

+ unsigned int *out_num, unsigned int *in_num,
+ struct vhost_log *log, unsigned int *log_num);
+void vhost_discard_vq_desc(struct vhost_virtqueue *);
+
+int vhost_add_used(struct vhost_virtqueue *, unsigned int head, int len);

+void vhost_signal(struct vhost_dev *, struct vhost_virtqueue *);
+void vhost_add_used_and_signal(struct vhost_dev *, struct vhost_virtqueue *,

+ unsigned int head, int len);

+void vhost_disable_notify(struct vhost_virtqueue *);
+bool vhost_enable_notify(struct vhost_virtqueue *);

index 1feed71..e210194 100644

--- a/include/linux/Kbuild
+++ b/include/linux/Kbuild
@@ -361,6 +361,7 @@ unifdef-y += uio.h
unifdef-y += unistd.h
unifdef-y += usbdevice_fs.h
unifdef-y += utsname.h
+unifdef-y += vhost.h
unifdef-y += videodev2.h
unifdef-y += videodev.h
unifdef-y += virtio_config.h
diff --git a/include/linux/miscdevice.h b/include/linux/miscdevice.h
index adaf3c1..8b5f7cc 100644
--- a/include/linux/miscdevice.h
+++ b/include/linux/miscdevice.h
@@ -30,6 +30,7 @@
#define HPET_MINOR 228
#define FUSE_MINOR 229
#define KVM_MINOR 232
+#define VHOST_NET_MINOR 233
#define MISC_DYNAMIC_MINOR 255

struct device;
diff --git a/include/linux/vhost.h b/include/linux/vhost.h
new file mode 100644

index 0000000..e847f1e
--- /dev/null
+++ b/include/linux/vhost.h
@@ -0,0 +1,130 @@

+ /* Option flags. */
+ unsigned int flags;
+ /* Flag values: */
+ /* Whether log address is valid. If set enables logging. */
+#define VHOST_VRING_F_LOG 0
+
+ /* Start of array of descriptors (virtually contiguous) */
+ __u64 desc_user_addr;
+ /* Used structure address. Must be 32 bit aligned */
+ __u64 used_user_addr;
+ /* Available structure address. Must be 16 bit aligned */
+ __u64 avail_user_addr;
+ /* Logging support. */
+ /* Log writes to used structure, at offset calculated from specified
+ * address. Address must be 32 bit aligned. */
+ __u64 log_guest_addr;

+#define VHOST_SET_FEATURES _IOW(VHOST_VIRTIO, 0x00, __u64)

+
+/* Set current process as the (exclusive) owner of this file descriptor. This
+ * must be called before any other vhost command. Further calls to
+ * VHOST_OWNER_SET fail until VHOST_OWNER_RESET is called. */
+#define VHOST_SET_OWNER _IO(VHOST_VIRTIO, 0x01)
+/* Give up ownership, and reset the device to default values.
+ * Allows subsequent call to VHOST_OWNER_SET to succeed. */
+#define VHOST_RESET_OWNER _IO(VHOST_VIRTIO, 0x02)
+
+/* Set up/modify memory layout */
+#define VHOST_SET_MEM_TABLE _IOW(VHOST_VIRTIO, 0x03, struct vhost_memory)
+
+/* Write logging setup. */
+/* Memory writes can optionally be logged by setting bit at an offset
+ * (calculated from the physical address) from specified log base.
+ * The bit is set using an atomic 32 bit operation. */
+/* Set base address for logging. */
+#define VHOST_SET_LOG_BASE _IOW(VHOST_VIRTIO, 0x04, __u64)
+/* Specify an eventfd file descriptor to signal on log write. */
+#define VHOST_SET_LOG_FD _IOW(VHOST_VIRTIO, 0x07, int)
+

+/* Ring setup. */
+/* Set number of descriptors in ring. This parameter can not
+ * be modified while ring is running (bound to a device). */

+#define VHOST_SET_VRING_NUM _IOW(VHOST_VIRTIO, 0x10, struct vhost_vring_state)

+/* Set addresses for the ring. */
+#define VHOST_SET_VRING_ADDR _IOW(VHOST_VIRTIO, 0x11, struct vhost_vring_addr)

+/* Base value where queue looks for available descriptors */

+#define VHOST_SET_VRING_BASE _IOW(VHOST_VIRTIO, 0x12, struct vhost_vring_state)

+/* Get accessor: reads index, writes value in num */

+#define VHOST_GET_VRING_BASE _IOWR(VHOST_VIRTIO, 0x12, struct vhost_vring_state)

[Rusty Russell]

On Mon, 9 Nov 2009 05:40:32 pm Michael S. Tsirkin wrote:
> On Mon, Nov 9, 2009 at 8:17 AM, Rusty Russell <ru...@rustcorp.com.au> wrote:
> > There's something about the 'acked' which rubs me the wrong way.
> > "enabled_features" is perhaps a better term than "acked_features"; "acked"
> > seems more a user point-of-view, "enabled" seems more driver POV?
>
> Hmm. Are you happy with the ioctl name? If yes I think being consistent
> with that is important.

I think in my original comments I noted that I preferred GET / SET, rather
than GET/ACK.

> > Actually, this looks wrong to me:
> >
> > + case VHOST_SET_VRING_BASE:
> > ...
> > + vq->avail_idx = vq->last_avail_idx = s.num;
> >
> > The last_avail_idx is part of the state of the driver. It needs to be saved
> > and restored over susp/resume.
>
>
> Exactly. That's what VHOST_GET/SET_VRING_BASE does. avail_idx is just a
> cached value for notify on empty, so what this does is clear the value.

Ah, you actually refresh it every time anyway. Hmm, could you do my poor
brain a favor and either just get_user it in vhost_trigger_irq(), or call
it 'cached_avail_idx' or something?

> > The only reason it's not in the ring itself
> > is because I figured the other side doesn't need to see it (which is true, but
> > missed debugging opportunities as well as man-in-the-middle issues like this
> > one). I had a patch which put this field at the end of the ring, I might
> > resurrect it to avoid this problem. This is backwards compatible with all
> > implementations. See patch at end.
>
> Yes, I remember that patch. There seems to be little point though, at
> this stage.

Well, it avoids this ioctl, by exposing all the state. We may well need it
later, to expand the ring in other ways.

> > I would drop avail_idx altogether: get_user is basically free, and simplifies
> > a lot. As most state is in the ring, all you need is an ioctl to save/restore
> > the last_avail_idx.
>
> avail_idx is there for notify on empty: I had this thought that it's
> better to leave the avail cache line alone when we are triggering
> interrupt to avoid bouncing it around if guest is updating it meanwhile
> on another CPU, and I think my testing showed that it helped
> performance, but could be a mistake. You don't believe this can help?

I believe it could help, but this is YA case where it would have been nice to
have a dumb basic patch and this as a patch on top. But I am going to ask
you to re-run that measurement, see if it stacks up (because it's an
interesting lesson if it does..)

Thanks!

[Rusty Russell]

On Tue, 10 Nov 2009 03:52:30 am Michael S. Tsirkin wrote:
> What it is: vhost net is a character device that can be used to reduce
> the number of system calls involved in virtio networking.
> Existing virtio net code is used in the guest without modification.

Thanks, applied. Will be in tomorrow's linux-next.

Cheers,
Rusty.

[Rusty Russell]

One fix:

vhost: fix TUN=m VHOST_NET=y

drivers/built-in.o: In function `get_tun_socket':
net.c:(.text+0x15436e): undefined reference to `tun_get_socket'

Signed-off-by: Rusty Russell <ru...@rustcorp.com.au>
---

drivers/vhost/Kconfig | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/vhost/Kconfig b/drivers/vhost/Kconfig
--- a/drivers/vhost/Kconfig
+++ b/drivers/vhost/Kconfig
@@ -1,6 +1,6 @@
config VHOST_NET

tristate "Host kernel accelerator for virtio net (EXPERIMENTAL)"

- depends on NET && EVENTFD && EXPERIMENTAL
+ depends on NET && EVENTFD && TUN && EXPERIMENTAL
---help---

This kernel module can be loaded in host kernel to accelerate

guest networking with virtio_net. Not to be confused with virtio_net

[Michael S. Tsirkin]

On Tue, Nov 10, 2009 at 01:49:09PM +1030, Rusty Russell wrote:
> One fix:
>
> vhost: fix TUN=m VHOST_NET=y
>
> drivers/built-in.o: In function `get_tun_socket':
> net.c:(.text+0x15436e): undefined reference to `tun_get_socket'
>
> Signed-off-by: Rusty Russell <ru...@rustcorp.com.au>
> ---
> drivers/vhost/Kconfig | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/vhost/Kconfig b/drivers/vhost/Kconfig
> --- a/drivers/vhost/Kconfig
> +++ b/drivers/vhost/Kconfig
> @@ -1,6 +1,6 @@
> config VHOST_NET
> tristate "Host kernel accelerator for virtio net (EXPERIMENTAL)"
> - depends on NET && EVENTFD && EXPERIMENTAL
> + depends on NET && EVENTFD && TUN && EXPERIMENTAL
> ---help---
> This kernel module can be loaded in host kernel to accelerate
> guest networking with virtio_net. Not to be confused with virtio_net

In fact, vhost can be built with TUN=n VHOST_NET=y as well
(tun_get_socket is stubbed out in that case).
So I think this is better (it looks strange
until you realize that for tristate variables
boolean logic math does not apply):

--->

From: Michael S. Tsirkin <m...@redhat.com>
Subject: vhost: fix TUN=m VHOST_NET=y

drivers/built-in.o: In function `get_tun_socket':
net.c:(.text+0x15436e): undefined reference to `tun_get_socket'

If tun is a module, vhost must be a module, too.
If tun is built-in or disabled, vhost can be built-in.

Signed-off-by: Michael S. Tsirkin <m...@redhat.com>

---

diff --git a/drivers/vhost/Kconfig b/drivers/vhost/Kconfig
index 9f409f4..9e93553 100644

--- a/drivers/vhost/Kconfig
+++ b/drivers/vhost/Kconfig
@@ -1,6 +1,6 @@
config VHOST_NET
tristate "Host kernel accelerator for virtio net (EXPERIMENTAL)"
- depends on NET && EVENTFD && EXPERIMENTAL

+ depends on NET && EVENTFD && (TUN || !TUN) && EXPERIMENTAL

---help---
This kernel module can be loaded in host kernel to accelerate
guest networking with virtio_net. Not to be confused with virtio_net

--
MST

[Rusty Russell]

On Tue, 10 Nov 2009 10:06:37 pm Michael S. Tsirkin wrote:
> If tun is a module, vhost must be a module, too.
> If tun is built-in or disabled, vhost can be built-in.

I really like the brainbending :) Keeps readers on their toes...

Applied,
Rusty.

[Xin, Xiaohui]

Michael,
From the http://www.linux-kvm.org/page/VhostNet, we can see netperf with TCP_STREAM can get more than 4GMb/s for the receive side, and more than 5GMb/s for the send side.
Is it the result from the raw socket or through tap?
I want to duplicate such performance with vhost on my side. I can only get more than 1GMb/s with following conditions:
1) disabled the GRO feature in the host 10G NIC driver
2) vi->big_packet in guest is false
3) MTU is 1500.
4) raw socket, not the tap
5) using your vhost git tree

Is that the reasonable result with such conditions or maybe I have made some silly mistakes somewhere I don't know yet?
May you kindly describe your test environment/conditions in detail to have much better performance in your website (I really need the performance)?

And I have tested the tun support with vhost now, and may you share your /home/mst/ifup script here?

Thanks
Xiaohui

To unsubscribe from this list: send the line "unsubscribe kvm" in

[Michael S. Tsirkin]

On Wed, Nov 18, 2009 at 01:42:43PM +0800, Xin, Xiaohui wrote:
> Michael,
> >From the http://www.linux-kvm.org/page/VhostNet, we can see netperf with TCP_STREAM can get more than 4GMb/s for the receive side, and more than 5GMb/s for the send side.
> Is it the result from the raw socket or through tap?
> I want to duplicate such performance with vhost on my side. I can only get more than 1GMb/s with following conditions:
> 1) disabled the GRO feature in the host 10G NIC driver
> 2) vi->big_packet in guest is false
> 3) MTU is 1500.
> 4) raw socket, not the tap
> 5) using your vhost git tree
>
> Is that the reasonable result with such conditions or maybe I have made some silly mistakes somewhere I don't know yet?
> May you kindly describe your test environment/conditions in detail to have much better performance in your website (I really need the performance)?
>

> Thanks
> Xiaohui

These results where sent by Shirley Ma (Cc'd).
I think they were with tap, host-to-guest/guest-to-host

> And I have tested the tun support with vhost now, and may you share your /home/mst/ifup script here?
>

These are usually pretty simple, mine looks like this:

#!/bin/sh -x
/sbin/ifconfig tap0 0.0.0.0 up
brctl addif br0 tap0

--
MST

[Shirley Ma]

On Sun, 2009-11-22 at 12:35 +0200, Michael S. Tsirkin wrote:
> These results where sent by Shirley Ma (Cc'd).
> I think they were with tap, host-to-guest/guest-to-host

Yes, you are right.

Shirley

[Michael S. Tsirkin]

> virtio: put last_used and last_avail index into ring itself.
>
> Generally, the other end of the virtio ring doesn't need to see where
> you're up to in consuming the ring. However, to completely understand
> what's going on from the outside, this information must be exposed.
> For example, if you want to save and restore a virtio_ring, but you're
> not the consumer because the kernel is using it directly.
>
> Fortunately, we have room to expand: the ring is always a whole number
> of pages and there's hundreds of bytes of padding after the avail ring
> and the used ring, whatever the number of descriptors (which must be a
> power of 2).
>
> We add a feature bit so the guest can tell the host that it's writing
> out the current value there, if it wants to use that.
>
> Signed-off-by: Rusty Russell <ru...@rustcorp.com.au>

I've been looking at this patch some more (more on why
later), and I wonder: would it be better to add some
alignment to the last used index address, so that
if we later add more stuff at the tail, it all
fits in a single cache line?

We use a new feature bit anyway, so layout change should not be
a problem.

Since I raised the question of caches: for used ring,
the ring is not aligned to 64 bit, so on CPUs with 64 bit
or larger cache lines, used entries will often cross
cache line boundaries. Am I right and might it
have been better to align ring entries to cache line boundaries?

What do you think?

[Rusty Russell]

On Wed, 5 May 2010 03:52:36 am Michael S. Tsirkin wrote:
> > virtio: put last_used and last_avail index into ring itself.
> >
> > Generally, the other end of the virtio ring doesn't need to see where
> > you're up to in consuming the ring. However, to completely understand
> > what's going on from the outside, this information must be exposed.
> > For example, if you want to save and restore a virtio_ring, but you're
> > not the consumer because the kernel is using it directly.
> >
> > Fortunately, we have room to expand: the ring is always a whole number
> > of pages and there's hundreds of bytes of padding after the avail ring
> > and the used ring, whatever the number of descriptors (which must be a
> > power of 2).
> >
> > We add a feature bit so the guest can tell the host that it's writing
> > out the current value there, if it wants to use that.
> >
> > Signed-off-by: Rusty Russell <ru...@rustcorp.com.au>
>
> I've been looking at this patch some more (more on why
> later), and I wonder: would it be better to add some
> alignment to the last used index address, so that
> if we later add more stuff at the tail, it all
> fits in a single cache line?

In theory, but not in practice. We don't have many rings, so the
difference between 1 and 2 cache lines is not very much.

> We use a new feature bit anyway, so layout change should not be
> a problem.
>
> Since I raised the question of caches: for used ring,
> the ring is not aligned to 64 bit, so on CPUs with 64 bit
> or larger cache lines, used entries will often cross
> cache line boundaries. Am I right and might it
> have been better to align ring entries to cache line boundaries?
>
> What do you think?

I think everyone is settled on 128 byte cache lines for the forseeable
future, so it's not really an issue.

Cheers,
Rusty.

[Michael S. Tsirkin]

On Thu, May 06, 2010 at 10:22:12AM +0930, Rusty Russell wrote:
> On Wed, 5 May 2010 03:52:36 am Michael S. Tsirkin wrote:
> > > virtio: put last_used and last_avail index into ring itself.
> > >
> > > Generally, the other end of the virtio ring doesn't need to see where
> > > you're up to in consuming the ring. However, to completely understand
> > > what's going on from the outside, this information must be exposed.
> > > For example, if you want to save and restore a virtio_ring, but you're
> > > not the consumer because the kernel is using it directly.
> > >
> > > Fortunately, we have room to expand: the ring is always a whole number
> > > of pages and there's hundreds of bytes of padding after the avail ring
> > > and the used ring, whatever the number of descriptors (which must be a
> > > power of 2).
> > >
> > > We add a feature bit so the guest can tell the host that it's writing
> > > out the current value there, if it wants to use that.
> > >
> > > Signed-off-by: Rusty Russell <ru...@rustcorp.com.au>
> >
> > I've been looking at this patch some more (more on why
> > later), and I wonder: would it be better to add some
> > alignment to the last used index address, so that
> > if we later add more stuff at the tail, it all
> > fits in a single cache line?
>
> In theory, but not in practice. We don't have many rings, so the
> difference between 1 and 2 cache lines is not very much.

Fair enough.

> > We use a new feature bit anyway, so layout change should not be
> > a problem.
> >
> > Since I raised the question of caches: for used ring,
> > the ring is not aligned to 64 bit, so on CPUs with 64 bit
> > or larger cache lines, used entries will often cross
> > cache line boundaries. Am I right and might it
> > have been better to align ring entries to cache line boundaries?
> >
> > What do you think?
>
> I think everyone is settled on 128 byte cache lines for the forseeable
> future, so it's not really an issue.
>
> Cheers,
> Rusty.

You mean with 64 bit descriptors we will be bouncing a cache line
between host and guest, anyway?

--
MST

[Rusty Russell]

On Thu, 6 May 2010 03:57:55 pm Michael S. Tsirkin wrote:
> On Thu, May 06, 2010 at 10:22:12AM +0930, Rusty Russell wrote:
> > On Wed, 5 May 2010 03:52:36 am Michael S. Tsirkin wrote:
> > > What do you think?
> >
> > I think everyone is settled on 128 byte cache lines for the forseeable
> > future, so it's not really an issue.
>

> You mean with 64 bit descriptors we will be bouncing a cache line
> between host and guest, anyway?

I'm confused by this entire thread.

Descriptors are 16 bytes. They are at the start, so presumably aligned to
cache boundaries.

Available ring follows that at 2 bytes per entry, so it's also packed nicely
into cachelines.

Then there's padding to page boundary. That puts us on a cacheline again
for the used ring; also 2 bytes per entry.

I don't see how any change in layout could be more cache friendly?
Rusty.

[Michael S. Tsirkin]

On Fri, May 07, 2010 at 12:35:39PM +0930, Rusty Russell wrote:
> On Thu, 6 May 2010 03:57:55 pm Michael S. Tsirkin wrote:
> > On Thu, May 06, 2010 at 10:22:12AM +0930, Rusty Russell wrote:
> > > On Wed, 5 May 2010 03:52:36 am Michael S. Tsirkin wrote:
> > > > What do you think?
> > >
> > > I think everyone is settled on 128 byte cache lines for the forseeable
> > > future, so it's not really an issue.
> >
> > You mean with 64 bit descriptors we will be bouncing a cache line
> > between host and guest, anyway?
>
> I'm confused by this entire thread.
>
> Descriptors are 16 bytes. They are at the start, so presumably aligned to
> cache boundaries.
>
> Available ring follows that at 2 bytes per entry, so it's also packed nicely
> into cachelines.
>
> Then there's padding to page boundary. That puts us on a cacheline again
> for the used ring; also 2 bytes per entry.
>

Hmm, is used ring really 2 bytes per entry?

/* u32 is used here for ids for padding reasons. */
struct vring_used_elem {
/* Index of start of used descriptor chain. */
__u32 id;
/* Total length of the descriptor chain which was used (written to) */
__u32 len;
};

struct vring_used {
__u16 flags;
__u16 idx;
struct vring_used_elem ring[];
};

> I don't see how any change in layout could be more cache friendly?
> Rusty.

I thought that used ring has 8 bytes per entry, and that struct
vring_used is aligned at page boundary, this
would mean that ring element is at offset 4 bytes from page boundary.
Thus with cacheline size 128 bytes, each 4th element crosses
a cacheline boundary. If we had a 4 byte padding after idx, each
used element would always be completely within a single cacheline.

What am I missing?
--
MST

[Rusty Russell]

On Sun, 9 May 2010 06:27:33 pm Michael S. Tsirkin wrote:
> On Fri, May 07, 2010 at 12:35:39PM +0930, Rusty Russell wrote:
> > Then there's padding to page boundary. That puts us on a cacheline again
> > for the used ring; also 2 bytes per entry.
> >
>
> Hmm, is used ring really 2 bytes per entry?

Err, no, I am an idiot.

> /* u32 is used here for ids for padding reasons. */
> struct vring_used_elem {
> /* Index of start of used descriptor chain. */
> __u32 id;
> /* Total length of the descriptor chain which was used (written to) */
> __u32 len;
> };
>
> struct vring_used {
> __u16 flags;
> __u16 idx;
> struct vring_used_elem ring[];
> };

OK, now I get it. Sorry, I was focussed on the avail ring.

> I thought that used ring has 8 bytes per entry, and that struct
> vring_used is aligned at page boundary, this
> would mean that ring element is at offset 4 bytes from page boundary.
> Thus with cacheline size 128 bytes, each 4th element crosses
> a cacheline boundary. If we had a 4 byte padding after idx, each
> used element would always be completely within a single cacheline.

I think the numbers are: every 16th entry hits two cachelines. So currently
the first 15 entries are "free" (assuming we hit the idx cacheline anyway),
then 1 in 16 cost 2 cachelines. That makes the aligned version win when
N > 240.

But, we access the array linearly. So the extra cacheline cost is in fact
amortized. I doubt it could be measured, but maybe vring_get_buf() should
prefetch? While you're there, we could use an & rather than a mod on the
calculation, which may actually be measurable :)

Cheers,
Rusty.