[PATCHv2 vringh 0/3] Introduce CAIF Virtio driver

[PATCHv2 vringh 0/3] Introduce CAIF Virtio driver

[sjur.brandeland]

From: Sjur Brændeland <sjur.brandeland@stericsson.com>

This driver depends on Rusty's new host virtio ring implementation,
so this patch-set is based on the vringh branch in Rusty's git.

Changes since V1:
- Use the new iov helper functions, and simplify iov handling.
  However this triggers compile warnings, as it takes struct iov
  while kernel api uses struct kiov
- Introduced the module_virtio_driver macro
- Pass NULL as wiov to vringh_getdesc_kern() 

Regards,
Sjur

Sjur Brændeland (2):
  remoteproc: Add support for vringh (Host vrings)
  virtio: Add module driver macro for virtio drivers.

Vikram ARV (1):
  caif_virtio: Introduce caif over virtio

 drivers/net/caif/Kconfig               |    8 +
 drivers/net/caif/Makefile              |    3 +
 drivers/net/caif/caif_virtio.c         |  547 ++++++++++++++++++++++++++++++++
 drivers/remoteproc/Kconfig             |    3 +
 drivers/remoteproc/remoteproc_virtio.c |  127 +++++++-
 include/linux/remoteproc.h             |   14 +
 include/linux/virtio.h                 |    9 +
 include/linux/virtio_caif.h            |   24 ++
 include/uapi/linux/virtio_ids.h        |    1 +
 9 files changed, 729 insertions(+), 7 deletions(-)
 create mode 100644 drivers/net/caif/caif_virtio.c
 create mode 100644 include/linux/virtio_caif.h

-- 
1.7.5.4

[PATCHv2 vringh 1/3] remoteproc: Add support for vringh (Host vrings)

[sjur.brandeland]

From: Sjur Brændeland <sjur.brandeland@stericsson.com>

Add functions for creating, deleting and kicking host-side virtio rings.

The host ring is not integrated with virtiqueues and cannot be managed
through virtio-config. Remoteproc must export functions for handling the
host-side virtio rings.

The functions rproc_virtio_get_vringh(), rproc_virtio_del_vringh(),
rproc_virtio_kick_vringh() are added to remoteproc_virtio.c. The
existing functions rproc_vq_interrupt() and rproc_virtio_find_vqs()
are updated to handle the new vhost rings.

Signed-off-by: Sjur Brændeland <sjur.brandeland@stericsson.com>
cc: Ohad Ben-Cohen <ohad@wizery.com>
cc: Rusty Russell <rusty@rustcorp.com.au>
cc: Ido Yariv <ido@wizery.com>
cc: Erwan Yvin <erwan.yvin@stericsson.com>
---
 drivers/remoteproc/Kconfig             |    3 +
 drivers/remoteproc/remoteproc_virtio.c |  127 ++++++++++++++++++++++++++++++--
 include/linux/remoteproc.h             |   14 ++++
 3 files changed, 137 insertions(+), 7 deletions(-)

diff --git a/drivers/remoteproc/Kconfig b/drivers/remoteproc/Kconfig
index 96ce101..c7d1d36 100644
--- a/drivers/remoteproc/Kconfig
+++ b/drivers/remoteproc/Kconfig
@@ -7,6 +7,9 @@ config REMOTEPROC
 	depends on HAS_DMA
 	select FW_CONFIG
 	select VIRTIO
+	select VHOST_RING
+
+source drivers/vhost/Kconfig
 
 config OMAP_REMOTEPROC
 	tristate "OMAP remoteproc support"
diff --git a/drivers/remoteproc/remoteproc_virtio.c b/drivers/remoteproc/remoteproc_virtio.c
index 9e198e5..fa7bf7b 100644
--- a/drivers/remoteproc/remoteproc_virtio.c
+++ b/drivers/remoteproc/remoteproc_virtio.c
@@ -23,6 +23,7 @@
 #include <linux/virtio_config.h>
 #include <linux/virtio_ids.h>
 #include <linux/virtio_ring.h>
+#include <linux/vringh.h>
 #include <linux/err.h>
 #include <linux/kref.h>
 #include <linux/slab.h>
@@ -60,10 +61,15 @@ irqreturn_t rproc_vq_interrupt(struct rproc *rproc, int notifyid)
 	dev_dbg(&rproc->dev, "vq index %d is interrupted\n", notifyid);
 
 	rvring = idr_find(&rproc->notifyids, notifyid);
-	if (!rvring || !rvring->vq)
+	if (!rvring)
 		return IRQ_NONE;
 
-	return vring_interrupt(0, rvring->vq);
+	if (rvring->vringh && rvring->vringh_cb)
+		return rvring->vringh_cb(&rvring->rvdev->vdev, rvring->vringh);
+	else if (rvring->vq)
+		return vring_interrupt(0, rvring->vq);
+	else
+		return IRQ_NONE;
 }
 EXPORT_SYMBOL(rproc_vq_interrupt);
 
@@ -149,14 +155,21 @@ static int rproc_virtio_find_vqs(struct virtio_device *vdev, unsigned nvqs,
 		       const char *names[])
 {
 	struct rproc *rproc = vdev_to_rproc(vdev);
-	int i, ret;
+	struct rproc_vdev *rvdev = vdev_to_rvdev(vdev);
+	int rng, id, ret, nrings = ARRAY_SIZE(rvdev->vring);
+
+	for (id = 0, rng = 0; rng < nrings; ++rng) {
+		struct rproc_vring *rvring = &rvdev->vring[rng];
+		/* Skip the host side rings */
+		if (rvring->vringh)
+			continue;
 
-	for (i = 0; i < nvqs; ++i) {
-		vqs[i] = rp_find_vq(vdev, i, callbacks[i], names[i]);
-		if (IS_ERR(vqs[i])) {
-			ret = PTR_ERR(vqs[i]);
+		vqs[id] = rp_find_vq(vdev, rng, callbacks[id], names[id]);
+		if (IS_ERR(vqs[id])) {
+			ret = PTR_ERR(vqs[id]);
 			goto error;
 		}
+		++id;
 	}
 
 	/* now that the vqs are all set, boot the remote processor */
@@ -173,6 +186,106 @@ error:
 	return ret;
 }
 
+/**
+ * rproc_virtio_new_vringh() - create a reversed virtio ring.
+ * @vdev: the virtio device
+ * @index: the virtio ring index
+ * @cb: callback for the reversed virtio ring
+ *
+ * This function should be called by the virtio-driver
+ * before calling find_vqs(). It returns a struct vringh for
+ * accessing the virtio ring.
+ *
+ * Return: struct vhost, or NULL upon error.
+ */
+struct vringh *
+rproc_virtio_new_vringh(struct virtio_device *vdev, unsigned index,
+			irqreturn_t (*cb)(struct virtio_device *vdev,
+					  struct vringh *vring))
+{
+	struct rproc_vdev *rvdev = vdev_to_rvdev(vdev);
+	struct rproc_vring *rvring;
+	struct vringh *vrh;
+	int err;
+
+	if (index > ARRAY_SIZE(rvdev->vring)) {
+		dev_err(&rvdev->vdev.dev, "bad vring index: %d\n", index);
+		return NULL;
+	}
+
+	vrh = kzalloc(sizeof(*vrh), GFP_KERNEL);
+	if (!vrh)
+		return NULL;
+
+	err = rproc_alloc_vring(rvdev, index);
+	if (err)
+		goto free_vring;
+
+
+	rvring = &rvdev->vring[index];
+	/* zero vring */
+	memset(rvring->va, 0, vring_size(rvring->len, rvring->align));
+	vring_init(&vrh->vring, rvring->len, rvring->va, rvring->align);
+
+	rvring->vringh_cb = cb;
+	rvring->vringh = vrh;
+
+	err = vringh_init_kern(vrh,
+			       rvdev->dfeatures,
+			       rvring->len,
+			       false,
+			       vrh->vring.desc,
+			       vrh->vring.avail,
+			       vrh->vring.used);
+	if (!err)
+		return vrh;
+
+	dev_err(&vdev->dev, "failed to create vhost: %d\n", err);
+	rproc_free_vring(rvring);
+free_vring:
+	kfree(vrh);
+	return NULL;
+}
+EXPORT_SYMBOL(rproc_virtio_new_vringh);
+
+/**
+ * rproc_virtio_del_vringh() - release a reversed virtio ring.
+ * @vdev: the virtio device
+ * @index: the virtio ring index
+ *
+ * This function release the reversed virtio ring.
+ */
+void rproc_virtio_del_vringh(struct virtio_device *vdev, unsigned index)
+{
+	struct rproc_vdev *rvdev = vdev_to_rvdev(vdev);
+	struct rproc_vring *rvring = &rvdev->vring[index];
+	kfree(rvring->vringh);
+	rproc_free_vring(rvring);
+	rvring->vringh_cb = NULL;
+	rvring->vringh = NULL;
+}
+EXPORT_SYMBOL(rproc_virtio_del_vringh);
+
+/**
+ * rproc_virtio_kick_vringh() - kick the remote processor.
+ * @vdev: the virtio device
+ * @index: the virtio ring index
+ *
+ * kick the remote processor, and let it know which vring to poke at
+ */
+void rproc_virtio_kick_vringh(struct virtio_device *vdev, unsigned index)
+{
+	struct rproc_vdev *rvdev = vdev_to_rvdev(vdev);
+	struct rproc_vring *rvring = &rvdev->vring[index];
+	struct rproc *rproc = rvring->rvdev->rproc;
+	int notifyid = rvring->notifyid;
+
+	dev_dbg(&rproc->dev, "kicking vq index: %d\n", notifyid);
+
+	rproc->ops->kick(rproc, notifyid);
+}
+EXPORT_SYMBOL(rproc_virtio_kick_vringh);
+
 /*
  * We don't support yet real virtio status semantics.
  *
diff --git a/include/linux/remoteproc.h b/include/linux/remoteproc.h
index faf3332..414a1fd 100644
--- a/include/linux/remoteproc.h
+++ b/include/linux/remoteproc.h
@@ -39,7 +39,9 @@
 #include <linux/klist.h>
 #include <linux/mutex.h>
 #include <linux/virtio.h>
+#include <linux/vringh.h>
 #include <linux/completion.h>
+#include <linux/interrupt.h>
 #include <linux/idr.h>
 
 /**
@@ -444,6 +446,8 @@ struct rproc {
  * @notifyid: rproc-specific unique vring index
  * @rvdev: remote vdev
  * @vq: the virtqueue of this vring
+ * @vringh_cb: callback used when device has kicked
+ * @vringh: the reversed host-side vring
  */
 struct rproc_vring {
 	void *va;
@@ -454,6 +458,9 @@ struct rproc_vring {
 	int notifyid;
 	struct rproc_vdev *rvdev;
 	struct virtqueue *vq;
+	irqreturn_t (*vringh_cb)(struct virtio_device *vdev,
+				 struct vringh *vring);
+	struct vringh *vringh;
 };
 
 /**
@@ -485,6 +492,13 @@ int rproc_boot(struct rproc *rproc);
 void rproc_shutdown(struct rproc *rproc);
 void rproc_report_crash(struct rproc *rproc, enum rproc_crash_type type);
 
+struct vringh *
+rproc_virtio_new_vringh(struct virtio_device *vdev, unsigned index,
+			irqreturn_t (*cb)(struct virtio_device *vdev,
+					  struct vringh *vring));
+void rproc_virtio_del_vringh(struct virtio_device *vdev, unsigned index);
+void rproc_virtio_kick_vringh(struct virtio_device *vdev, unsigned index);
+
 static inline struct rproc_vdev *vdev_to_rvdev(struct virtio_device *vdev)
 {
 	return container_of(vdev, struct rproc_vdev, vdev);
-- 
1.7.5.4

[PATCHv2 vringh 2/3] virtio: Add module driver macro for virtio drivers.

[sjur.brandeland]

From: Sjur Brændeland <sjur.brandeland@stericsson.com>

Add helper macro for drivers that don't do anything
special in module init/exit.

Signed-off-by: Sjur Brændeland <sjur.brandeland@stericsson.com>
---
 include/linux/virtio.h |    9 +++++++++
 1 files changed, 9 insertions(+), 0 deletions(-)

diff --git a/include/linux/virtio.h b/include/linux/virtio.h
index cf8adb1..00ccc40 100644
--- a/include/linux/virtio.h
+++ b/include/linux/virtio.h
@@ -126,4 +126,13 @@ static inline struct virtio_driver *drv_to_virtio(struct device_driver *drv)
 
 int register_virtio_driver(struct virtio_driver *drv);
 void unregister_virtio_driver(struct virtio_driver *drv);
+
+/* module_virtio_driver() - Helper macro for drivers that don't do
+ * anything special in module init/exit.  This eliminates a lot of
+ * boilerplate.  Each module may only use this macro once, and
+ * calling it replaces module_init() and module_exit()
+ */
+#define module_virtio_driver(__virtio_driver) \
+	module_driver(__virtio_driver, register_virtio_driver, \
+			unregister_virtio_driver)
 #endif /* _LINUX_VIRTIO_H */
-- 
1.7.5.4

[PATCHv2 vringh 3/3] caif_virtio: Introduce caif over virtio

[sjur.brandeland]

From: Vikram ARV <vikram.arv@stericsson.com>

Add the the Virtio shared memory driver for STE Modems.
caif_virtio is implemented utilizing the virtio framework
for data transport and is managed with the remoteproc frameworks.

The Virtio queue is used for transmitting data to the modem, and
the new vringh implementation is receiving data over the vring.

Signed-off-by: Vikram ARV <vikram.arv@stericsson.com>
Signed-off-by: Sjur Brændeland <sjur.brandeland@stericsson.com>

cc: David S. Miller <davem@davemloft.net>
cc: Ohad Ben-Cohen <ohad@wizery.com>
cc: Rusty Russell <rusty@rustcorp.com.au>
cc: Ido Yariv <ido@wizery.com>
cc: Erwan Yvin <erwan.yvin@stericsson.com>
---
As mentioned earlier this patch-set will go via Rusty's git.

Changes since V1: 
- update to new vringh API, 
- use module_virtio_driver macro
- call tasklet_kill from cfv_remove().

Thanks,
Sjur

 drivers/net/caif/Kconfig        |    8 +
 drivers/net/caif/Makefile       |    3 +
 drivers/net/caif/caif_virtio.c  |  547 +++++++++++++++++++++++++++++++++++++++
 include/linux/virtio_caif.h     |   24 ++
 include/uapi/linux/virtio_ids.h |    1 +
 5 files changed, 583 insertions(+), 0 deletions(-)
 create mode 100644 drivers/net/caif/caif_virtio.c
 create mode 100644 include/linux/virtio_caif.h

diff --git a/drivers/net/caif/Kconfig b/drivers/net/caif/Kconfig
index abf4d7a..a8b67e9 100644
--- a/drivers/net/caif/Kconfig
+++ b/drivers/net/caif/Kconfig
@@ -47,3 +47,11 @@ config CAIF_HSI
        The caif low level driver for CAIF over HSI.
        Be aware that if you enable this then you also need to
        enable a low-level HSI driver.
+
+config CAIF_VIRTIO
+	tristate "CAIF virtio transport driver"
+	depends on CAIF
+	depends on REMOTEPROC
+	default n
+	---help---
+	The caif driver for CAIF over Virtio.
diff --git a/drivers/net/caif/Makefile b/drivers/net/caif/Makefile
index 91dff86..d9ee26a 100644
--- a/drivers/net/caif/Makefile
+++ b/drivers/net/caif/Makefile
@@ -13,3 +13,6 @@ obj-$(CONFIG_CAIF_SHM) += caif_shm.o
 
 # HSI interface
 obj-$(CONFIG_CAIF_HSI) += caif_hsi.o
+
+# Virtio interface
+obj-$(CONFIG_CAIF_VIRTIO) += caif_virtio.o
diff --git a/drivers/net/caif/caif_virtio.c b/drivers/net/caif/caif_virtio.c
new file mode 100644
index 0000000..d4f339c
--- /dev/null
+++ b/drivers/net/caif/caif_virtio.c
@@ -0,0 +1,547 @@
+/*
+ * Copyright (C) ST-Ericsson AB 2012
+ * Contact: Sjur Brendeland / sjur.brandeland@stericsson.com
+ * Authors: Vicram Arv / vikram.arv@stericsson.com,
+ *	    Dmitry Tarnyagin / dmitry.tarnyagin@stericsson.com
+ *	    Sjur Brendeland / sjur.brandeland@stericsson.com
+ * License terms: GNU General Public License (GPL) version 2
+ */
+
+#include <linux/module.h>
+#include <linux/virtio.h>
+#include <linux/virtio_ids.h>
+#include <linux/virtio_config.h>
+#include <linux/dma-mapping.h>
+#include <linux/netdevice.h>
+#include <linux/if_arp.h>
+#include <linux/spinlock.h>
+#include <linux/virtio_caif.h>
+#include <linux/virtio_ring.h>
+#include <linux/vringh.h>
+#include <linux/remoteproc.h>
+#include <net/caif/caif_dev.h>
+
+MODULE_LICENSE("GPL v2");
+MODULE_AUTHOR("Vicram Arv <vikram.arv@stericsson.com>");
+MODULE_AUTHOR("Sjur Brendeland <sjur.brandeland@stericsson.com");
+MODULE_DESCRIPTION("Virtio CAIF Driver");
+
+/* Virtio Ring used in receive direction */
+#define RX_RING_INDEX 0
+
+#define CFV_DEFAULT_QUOTA 32
+
+/* struct cfv_napi_contxt - NAPI context info
+ * @riov: IOV holding data read from the ring. Note that riov may
+ *	  still hold data when cfv_rx_poll() returns.
+ * @head: Last descriptor ID we received from vringh_getdesc_kern.
+ *	  We use this to put descriptor back on the used ring. USHRT_MAX is
+ *	  used to indicate invalid head-id.
+ */
+struct cfv_napi_context {
+	struct vringh_kiov riov;
+	unsigned short head;
+};
+
+/* struct cfv_info - Caif Virtio control structure
+ * @cfdev:	caif common header
+ * @vdev:	Associated virtio device
+ * @vq_rx:	rx/downlink virtqueue
+ * @vq_tx:	tx/uplink virtqueue
+ * @ndev:	associated netdevice
+ * @queued_tx:	number of buffers queued in the tx virtqueue
+ * @watermark_tx: indicates number of buffers the tx queue
+ *		should shrink to to unblock datapath
+ * @tx_lock:	protects vq_tx to allow concurrent senders
+ * @tx_hr:	transmit headroom
+ * @rx_hr:	receive headroom
+ * @tx_tr:	transmit tailroom
+ * @rx_tr:	receive tailroom
+ * @mtu:	transmit max size
+ * @mru:	receive max size
+ */
+struct cfv_info {
+	struct caif_dev_common cfdev;
+	struct virtio_device *vdev;
+	struct vringh *vr_rx;
+	struct virtqueue *vq_tx;
+	struct net_device *ndev;
+	unsigned int queued_tx;
+	unsigned int watermark_tx;
+	/* Protect access to vq_tx */
+	spinlock_t tx_lock;
+	struct tasklet_struct tx_release_tasklet;
+	struct napi_struct napi;
+	struct cfv_napi_context ctx;
+	/* Copied from Virtio config space */
+	u16 tx_hr;
+	u16 rx_hr;
+	u16 tx_tr;
+	u16 rx_tr;
+	u32 mtu;
+	u32 mru;
+};
+
+/* struct token_info - maintains Transmit buffer data handle
+ * @size:	size of transmit buffer
+ * @dma_handle: handle to allocated dma device memory area
+ * @vaddr:	virtual address mapping to allocated memory area
+ */
+struct token_info {
+	size_t size;
+	u8 *vaddr;
+	dma_addr_t dma_handle;
+};
+
+/* Default if virtio config space is unavailable */
+#define CFV_DEF_MTU_SIZE 4096
+#define CFV_DEF_HEADROOM 32
+#define CFV_DEF_TAILROOM 32
+
+/* Require IP header to be 4-byte aligned. */
+#define IP_HDR_ALIGN 4
+
+static void cfv_release_cb(struct virtqueue *vq_tx)
+{
+	struct cfv_info *cfv = vq_tx->vdev->priv;
+	tasklet_schedule(&cfv->tx_release_tasklet);
+}
+
+/* This is invoked whenever the remote processor completed processing
+ * a TX msg we just sent it, and the buffer is put back to the used ring.
+ */
+static void cfv_release_used_buf(struct virtqueue *vq_tx)
+{
+	struct cfv_info *cfv = vq_tx->vdev->priv;
+	unsigned long flags;
+
+	BUG_ON(vq_tx != cfv->vq_tx);
+	WARN_ON_ONCE(irqs_disabled());
+
+	for (;;) {
+		unsigned int len;
+		struct token_info *buf_info;
+
+		/* Get used buffer from used ring to recycle used descriptors */
+		spin_lock_irqsave(&cfv->tx_lock, flags);
+		buf_info = virtqueue_get_buf(vq_tx, &len);
+
+		if (!buf_info)
+			goto out;
+
+		BUG_ON(!cfv->queued_tx);
+		if (--cfv->queued_tx <= cfv->watermark_tx) {
+			cfv->watermark_tx = 0;
+			netif_tx_wake_all_queues(cfv->ndev);
+		}
+		spin_unlock_irqrestore(&cfv->tx_lock, flags);
+
+		dma_free_coherent(vq_tx->vdev->dev.parent->parent,
+				  buf_info->size, buf_info->vaddr,
+				  buf_info->dma_handle);
+		kfree(buf_info);
+	}
+	return;
+out:
+	spin_unlock_irqrestore(&cfv->tx_lock, flags);
+}
+
+static struct sk_buff *cfv_alloc_and_copy_skb(int *err,
+					      struct cfv_info *cfv,
+					      u8 *frm, u32 frm_len)
+{
+	struct sk_buff *skb;
+	u32 cfpkt_len, pad_len;
+
+	*err = 0;
+	/* Verify that packet size with down-link header and mtu size */
+	if (frm_len > cfv->mru || frm_len <= cfv->rx_hr + cfv->rx_tr) {
+		netdev_err(cfv->ndev,
+			   "Invalid frmlen:%u  mtu:%u hr:%d tr:%d\n",
+			   frm_len, cfv->mru,  cfv->rx_hr,
+			   cfv->rx_tr);
+		*err = -EPROTO;
+		return NULL;
+	}
+
+	cfpkt_len = frm_len - (cfv->rx_hr + cfv->rx_tr);
+
+	pad_len = (unsigned long)(frm + cfv->rx_hr) & (IP_HDR_ALIGN - 1);
+
+	skb = netdev_alloc_skb(cfv->ndev, frm_len + pad_len);
+	if (!skb) {
+		*err = -ENOMEM;
+		return NULL;
+	}
+	/* Reserve space for headers. */
+	skb_reserve(skb, cfv->rx_hr + pad_len);
+
+	memcpy(skb_put(skb, cfpkt_len), frm + cfv->rx_hr, cfpkt_len);
+	return skb;
+}
+
+static int cfv_rx_poll(struct napi_struct *napi, int quota)
+{
+	struct cfv_info *cfv = container_of(napi, struct cfv_info, napi);
+	int rxcnt = 0;
+	int err = 0;
+	void *buf;
+	struct sk_buff *skb;
+	struct vringh_kiov *riov = &cfv->ctx.riov;
+
+	do {
+		skb = NULL;
+		if (riov->i == riov->used) {
+			if (cfv->ctx.head != USHRT_MAX) {
+				vringh_complete_kern(cfv->vr_rx,
+						     cfv->ctx.head,
+						     0);
+				cfv->ctx.head = USHRT_MAX;
+			}
+
+			err = vringh_getdesc_kern(
+				cfv->vr_rx,
+				riov,
+				NULL,
+				&cfv->ctx.head,
+				GFP_ATOMIC);
+
+			if (err <= 0)
+				goto out;
+
+		}
+
+		buf = phys_to_virt((unsigned long) riov->iov[riov->i].iov_base);
+		/* TODO: Add check on valid buffer address */
+
+		skb = cfv_alloc_and_copy_skb(&err, cfv, buf,
+					     riov->iov[riov->i].iov_len);
+		if (unlikely(err))
+			goto out;
+
+		/* Push received packet up the stack. */
+		skb->protocol = htons(ETH_P_CAIF);
+		skb_reset_mac_header(skb);
+		skb->dev = cfv->ndev;
+		err = netif_receive_skb(skb);
+		if (unlikely(err)) {
+			++cfv->ndev->stats.rx_dropped;
+		} else {
+			++cfv->ndev->stats.rx_packets;
+			cfv->ndev->stats.rx_bytes += skb->len;
+		}
+
+		++riov->i;
+		++rxcnt;
+	} while (rxcnt < quota);
+
+	return rxcnt;
+
+out:
+	switch (err) {
+	case 0:
+		/* Empty ring, enable notifications and stop NAPI polling */
+		if (!vringh_notify_enable_kern(cfv->vr_rx))
+			napi_complete(napi);
+
+		return rxcnt;
+
+	case -ENOMEM:
+		dev_kfree_skb(skb);
+		/* Stop NAPI poll on OOM, we hope to be polled later */
+		napi_complete(napi);
+		vringh_notify_enable_kern(cfv->vr_rx);
+		break;
+
+	default:
+		/* We're doomed, any modem fault is fatal */
+		netdev_warn(cfv->ndev, "Bad ring, disable device\n");
+		cfv->ndev->stats.rx_dropped = riov->used - riov->i;
+		napi_complete(napi);
+		vringh_notify_disable_kern(cfv->vr_rx);
+		netif_carrier_off(cfv->ndev);
+		break;
+	}
+
+	return rxcnt;
+}
+
+static irqreturn_t cfv_recv(struct virtio_device *vdev, struct vringh *vr_rx)
+{
+	struct cfv_info *cfv = vdev->priv;
+
+	vringh_notify_disable_kern(cfv->vr_rx);
+	napi_schedule(&cfv->napi);
+	return IRQ_HANDLED;
+}
+
+static int cfv_netdev_open(struct net_device *netdev)
+{
+	struct cfv_info *cfv = netdev_priv(netdev);
+
+	netif_carrier_on(netdev);
+	napi_enable(&cfv->napi);
+	return 0;
+}
+
+static int cfv_netdev_close(struct net_device *netdev)
+{
+	struct cfv_info *cfv = netdev_priv(netdev);
+
+	netif_carrier_off(netdev);
+	napi_disable(&cfv->napi);
+	return 0;
+}
+
+static struct token_info *cfv_alloc_and_copy_to_dmabuf(struct cfv_info *cfv,
+						       struct sk_buff *skb,
+						       struct scatterlist *sg)
+{
+	struct caif_payload_info *info = (void *)&skb->cb;
+	struct token_info *buf_info = NULL;
+	u8 pad_len, hdr_ofs;
+
+	if (unlikely(cfv->tx_hr + skb->len + cfv->tx_tr > cfv->mtu)) {
+		netdev_warn(cfv->ndev, "Invalid packet len (%d > %d)\n",
+			    cfv->tx_hr + skb->len + cfv->tx_tr, cfv->mtu);
+		goto err;
+	}
+
+	buf_info = kmalloc(sizeof(struct token_info), GFP_ATOMIC);
+	if (unlikely(!buf_info))
+		goto err;
+
+	/* Make the IP header aligned in tbe buffer */
+	hdr_ofs = cfv->tx_hr + info->hdr_len;
+	pad_len = hdr_ofs & (IP_HDR_ALIGN - 1);
+	buf_info->size = cfv->tx_hr + skb->len + cfv->tx_tr + pad_len;
+
+	if (WARN_ON_ONCE(!cfv->vdev->dev.parent))
+		goto err;
+
+	/* allocate coherent memory for the buffers */
+	buf_info->vaddr =
+		dma_alloc_coherent(cfv->vdev->dev.parent->parent,
+				   buf_info->size, &buf_info->dma_handle,
+				   GFP_ATOMIC);
+	if (unlikely(!buf_info->vaddr)) {
+		netdev_warn(cfv->ndev,
+			    "Out of DMA memory (alloc %zu bytes)\n",
+			    buf_info->size);
+		goto err;
+	}
+
+	/* copy skbuf contents to send buffer */
+	skb_copy_bits(skb, 0, buf_info->vaddr + cfv->tx_hr + pad_len, skb->len);
+	sg_init_one(sg, buf_info->vaddr + pad_len,
+		    skb->len + cfv->tx_hr + cfv->rx_hr);
+
+	return buf_info;
+err:
+	kfree(buf_info);
+	return NULL;
+}
+
+/* This is invoked whenever the host processor application has sent
+ * up-link data. Send it in the TX VQ avail ring.
+ *
+ * CAIF Virtio sends does not use linked descriptors in the tx direction.
+ */
+static int cfv_netdev_tx(struct sk_buff *skb, struct net_device *netdev)
+{
+	struct cfv_info *cfv = netdev_priv(netdev);
+	struct token_info *buf_info;
+	struct scatterlist sg;
+	unsigned long flags;
+	int ret;
+
+	buf_info = cfv_alloc_and_copy_to_dmabuf(cfv, skb, &sg);
+
+	spin_lock_irqsave(&cfv->tx_lock, flags);
+	if (unlikely(!buf_info))
+		goto flow_off;
+
+	/* Add buffer to avail ring. Flow control below should ensure
+	 * that this always succeedes
+	 */
+	ret = virtqueue_add_buf(cfv->vq_tx, &sg, 1, 0,
+				buf_info, GFP_ATOMIC);
+
+	if (unlikely(WARN_ON(ret < 0))) {
+		kfree(buf_info);
+		goto flow_off;
+	}
+
+
+	/* update netdev statistics */
+	cfv->queued_tx++;
+	cfv->ndev->stats.tx_packets++;
+	cfv->ndev->stats.tx_bytes += skb->len;
+
+	/* tell the remote processor it has a pending message to read */
+	virtqueue_kick(cfv->vq_tx);
+
+	/* Flow-off check takes into account number of cpus to make sure
+	 * virtqueue will not be overfilled in any possible smp conditions.
+	 *
+	 * Flow-on is triggered when sufficient buffers are freed
+	 */
+	if (ret <= num_present_cpus()) {
+flow_off:
+		cfv->watermark_tx = cfv->queued_tx >> 1;
+		netif_tx_stop_all_queues(netdev);
+	}
+
+	spin_unlock_irqrestore(&cfv->tx_lock, flags);
+
+	dev_kfree_skb(skb);
+	tasklet_schedule(&cfv->tx_release_tasklet);
+	return NETDEV_TX_OK;
+}
+
+static void cfv_tx_release_tasklet(unsigned long drv)
+{
+	struct cfv_info *cfv = (struct cfv_info *)drv;
+	cfv_release_used_buf(cfv->vq_tx);
+}
+
+static const struct net_device_ops cfv_netdev_ops = {
+	.ndo_open = cfv_netdev_open,
+	.ndo_stop = cfv_netdev_close,
+	.ndo_start_xmit = cfv_netdev_tx,
+};
+
+static void cfv_netdev_setup(struct net_device *netdev)
+{
+	netdev->netdev_ops = &cfv_netdev_ops;
+	netdev->type = ARPHRD_CAIF;
+	netdev->tx_queue_len = 100;
+	netdev->flags = IFF_POINTOPOINT | IFF_NOARP;
+	netdev->mtu = CFV_DEF_MTU_SIZE;
+	netdev->destructor = free_netdev;
+}
+
+static int cfv_probe(struct virtio_device *vdev)
+{
+	vq_callback_t *vq_cbs = cfv_release_cb;
+	const char *names =  "output";
+	const char *cfv_netdev_name = "cfvrt";
+	struct net_device *netdev;
+	struct virtqueue *vqs;
+
+	struct cfv_info *cfv;
+	int err = 0;
+
+	netdev = alloc_netdev(sizeof(struct cfv_info), cfv_netdev_name,
+			      cfv_netdev_setup);
+	if (!netdev)
+		return -ENOMEM;
+
+	cfv = netdev_priv(netdev);
+	cfv->vdev = vdev;
+	cfv->ndev = netdev;
+
+	spin_lock_init(&cfv->tx_lock);
+
+	cfv->vr_rx = rproc_virtio_new_vringh(vdev, RX_RING_INDEX, cfv_recv);
+	if (!cfv->vr_rx)
+		goto free_cfv;
+
+	/* Get the TX (uplink) virtque */
+	err = vdev->config->find_vqs(vdev, 1, &vqs, &vq_cbs, &names);
+	if (err)
+		goto free_cfv;
+
+	cfv->vq_tx = vqs;
+
+#define GET_VIRTIO_CONFIG_OPS(_v, _var, _f) \
+	((_v)->config->get(_v, offsetof(struct virtio_caif_transf_config, _f), \
+			   &_var, \
+			   FIELD_SIZEOF(struct virtio_caif_transf_config, _f)))
+
+	if (vdev->config->get) {
+		GET_VIRTIO_CONFIG_OPS(vdev, cfv->tx_hr, headroom);
+		GET_VIRTIO_CONFIG_OPS(vdev, cfv->rx_hr, headroom);
+		GET_VIRTIO_CONFIG_OPS(vdev, cfv->tx_tr, tailroom);
+		GET_VIRTIO_CONFIG_OPS(vdev, cfv->rx_tr, tailroom);
+		GET_VIRTIO_CONFIG_OPS(vdev, cfv->mtu, mtu);
+		GET_VIRTIO_CONFIG_OPS(vdev, cfv->mru, mtu);
+	} else {
+		cfv->tx_hr = CFV_DEF_HEADROOM;
+		cfv->rx_hr = CFV_DEF_HEADROOM;
+		cfv->tx_tr = CFV_DEF_TAILROOM;
+		cfv->rx_tr = CFV_DEF_TAILROOM;
+		cfv->mtu = CFV_DEF_MTU_SIZE;
+		cfv->mru = CFV_DEF_MTU_SIZE;
+	}
+
+	netdev->needed_headroom = cfv->tx_hr;
+	netdev->needed_tailroom = cfv->tx_tr;
+
+	/* Subtract needed tailroom from MTU to ensure enough room */
+	netdev->mtu = cfv->mtu - cfv->tx_tr;
+
+	vdev->priv = cfv;
+
+	vringh_iov_init(&cfv->ctx.riov, NULL, 0);
+	cfv->ctx.head = USHRT_MAX;
+
+	netif_napi_add(netdev, &cfv->napi, cfv_rx_poll, CFV_DEFAULT_QUOTA);
+	tasklet_init(&cfv->tx_release_tasklet,
+		     cfv_tx_release_tasklet,
+		     (unsigned long)cfv);
+
+	netif_carrier_off(netdev);
+
+	/* register Netdev */
+	err = register_netdev(netdev);
+	if (err) {
+		dev_err(&vdev->dev, "Unable to register netdev (%d)\n", err);
+		goto vqs_del;
+	}
+
+	/* tell the remote processor it can start sending messages */
+	rproc_virtio_kick_vringh(vdev, RX_RING_INDEX);
+
+	return 0;
+
+vqs_del:
+	vdev->config->del_vqs(cfv->vdev);
+free_cfv:
+	free_netdev(netdev);
+	return err;
+}
+
+static void cfv_remove(struct virtio_device *vdev)
+{
+	struct cfv_info *cfv = vdev->priv;
+
+	tasklet_kill(&cfv->tx_release_tasklet);
+	vringh_iov_cleanup(&cfv->ctx.riov);
+	vdev->config->reset(vdev);
+	rproc_virtio_del_vringh(vdev, RX_RING_INDEX);
+	cfv->vr_rx = NULL;
+	vdev->config->del_vqs(cfv->vdev);
+	unregister_netdev(cfv->ndev);
+}
+
+static struct virtio_device_id id_table[] = {
+	{ VIRTIO_ID_CAIF, VIRTIO_DEV_ANY_ID },
+	{ 0 },
+};
+
+static unsigned int features[] = {
+};
+
+static struct virtio_driver caif_virtio_driver = {
+	.feature_table		= features,
+	.feature_table_size	= ARRAY_SIZE(features),
+	.driver.name		= KBUILD_MODNAME,
+	.driver.owner		= THIS_MODULE,
+	.id_table		= id_table,
+	.probe			= cfv_probe,
+	.remove			= cfv_remove,
+};
+
+module_virtio_driver(caif_virtio_driver);
+MODULE_DEVICE_TABLE(virtio, id_table);
diff --git a/include/linux/virtio_caif.h b/include/linux/virtio_caif.h
new file mode 100644
index 0000000..5d2d312
--- /dev/null
+++ b/include/linux/virtio_caif.h
@@ -0,0 +1,24 @@
+/*
+ * Copyright (C) ST-Ericsson AB 2012
+ * Author: Sjur Brændeland <sjur.brandeland@stericsson.com>
+ *
+ * This header is BSD licensed so
+ * anyone can use the definitions to implement compatible remote processors
+ */
+
+#ifndef VIRTIO_CAIF_H
+#define VIRTIO_CAIF_H
+
+#include <linux/types.h>
+struct virtio_caif_transf_config {
+	u16 headroom;
+	u16 tailroom;
+	u32 mtu;
+	u8 reserved[4];
+};
+
+struct virtio_caif_config {
+	struct virtio_caif_transf_config uplink, downlink;
+	u8 reserved[8];
+};
+#endif
diff --git a/include/uapi/linux/virtio_ids.h b/include/uapi/linux/virtio_ids.h
index a7630d0..284fc3a 100644
--- a/include/uapi/linux/virtio_ids.h
+++ b/include/uapi/linux/virtio_ids.h
@@ -38,5 +38,6 @@
 #define VIRTIO_ID_SCSI		8 /* virtio scsi */
 #define VIRTIO_ID_9P		9 /* 9p virtio console */
 #define VIRTIO_ID_RPROC_SERIAL 11 /* virtio remoteproc serial link */
+#define VIRTIO_ID_CAIF	       12 /* Virtio caif */
 
 #endif /* _LINUX_VIRTIO_IDS_H */
-- 
1.7.5.4

Re: [PATCHv2 vringh 3/3] caif_virtio: Introduce caif over virtio

[Rusty Russell]

sjur.brandeland@stericsson.com writes:
> From: Vikram ARV <vikram.arv@stericsson.com>
>
> Add the the Virtio shared memory driver for STE Modems.
> caif_virtio is implemented utilizing the virtio framework
> for data transport and is managed with the remoteproc frameworks.
>
> The Virtio queue is used for transmitting data to the modem, and
> the new vringh implementation is receiving data over the vring.
>
> Signed-off-by: Vikram ARV <vikram.arv@stericsson.com>
> Signed-off-by: Sjur Brændeland <sjur.brandeland@stericsson.com>

I have applied 1 and 3 (2 is already in virtio-next); they're in my
pending-rebases branch behind the vringh patches which are pending
a final review.

Here's the warning fix patch I applied:

diff --git a/drivers/net/caif/caif_virtio.c b/drivers/net/caif/caif_virtio.c
index d4f339c..df832e7 100644
--- a/drivers/net/caif/caif_virtio.c
+++ b/drivers/net/caif/caif_virtio.c
@@ -483,7 +483,7 @@ static int cfv_probe(struct virtio_device *vdev)
 
 	vdev->priv = cfv;
 
-	vringh_iov_init(&cfv->ctx.riov, NULL, 0);
+	vringh_kiov_init(&cfv->ctx.riov, NULL, 0);
 	cfv->ctx.head = USHRT_MAX;
 
 	netif_napi_add(netdev, &cfv->napi, cfv_rx_poll, CFV_DEFAULT_QUOTA);
@@ -517,7 +517,7 @@ static void cfv_remove(struct virtio_device *vdev)
 	struct cfv_info *cfv = vdev->priv;
 
 	tasklet_kill(&cfv->tx_release_tasklet);
-	vringh_iov_cleanup(&cfv->ctx.riov);
+	vringh_kiov_cleanup(&cfv->ctx.riov);
 	vdev->config->reset(vdev);
 	rproc_virtio_del_vringh(vdev, RX_RING_INDEX);
 	cfv->vr_rx = NULL;

Cheers,
Rusty.

Re: [PATCHv2 vringh 1/3] remoteproc: Add support for vringh (Host vrings)

[Ohad Ben-Cohen]

Hi Sjur,

On Tue, Feb 12, 2013 at 1:49 PM,  <sjur.brandeland@stericsson.com> wrote:
> From: Sjur Brændeland <sjur.brandeland@stericsson.com>
>
> Add functions for creating, deleting and kicking host-side virtio rings.
>
> The host ring is not integrated with virtiqueues and cannot be managed
> through virtio-config.

Is that an inherent design/issue of vringh or just a description of
the current vringh code ?

> Remoteproc must export functions for handling the host-side virtio rings.

Have you considered exporting this via virtio instead ?

> The functions rproc_virtio_get_vringh(), rproc_virtio_del_vringh(),
> rproc_virtio_kick_vringh() are added to remoteproc_virtio.c.

I wonder if this is the way we want things to work.

Following this design, virtio drivers that use these rproc_* functions
will be coupled with the remoteproc framework.

One issue with this is what happens if, e.g., a VIRTIO_ID_CAIF vdev is
added by other than remoteproc (e.g. by virtio_pci or virtio_mmio).
Not sure how probable this really is, and whether there's anything
that prevents this, but things will go awry if this happens.

But maybe the important aspect to consider is whether we really want
to couple virtio drivers (such as the upcoming caif one) with the
remoteproc framework.

If you'll take a look at the rpmsg virtio driver, there's nothing
there which couples it with remoteproc. It's just a standard virtio
driver, that can be easily used with traditional virtio hosts as well.

This is possible of course thanks to the abstraction provided by
virtio: remoteproc only implements a set of callbacks which virtio
invokes when needed.

Do we not want to follow a similar design scheme with vringh ?

I have some other questions as well but maybe it's better to discuss
first the bigger picture.

Thanks!
Ohad.

Re: [PATCHv2 vringh 1/3] remoteproc: Add support for vringh (Host vrings)

[Sjur Brændeland]

On Wed, Feb 20, 2013 at 5:05 PM, Ohad Ben-Cohen <ohad@wizery.com> wrote:
> Hi Sjur,
>
> On Tue, Feb 12, 2013 at 1:49 PM,  <sjur.brandeland@stericsson.com> wrote:
>> From: Sjur Brændeland <sjur.brandeland@stericsson.com>
>>
>> Add functions for creating, deleting and kicking host-side virtio rings.
>>
>> The host ring is not integrated with virtiqueues and cannot be managed
>> through virtio-config.
>
> Is that an inherent design/issue of vringh or just a description of
> the current vringh code ?
>
>> Remoteproc must export functions for handling the host-side virtio rings.
>
> Have you considered exporting this via virtio instead ?

Rusty should comment on this...
I asked Rusty the same question a while a go, see
http://lkml.org/lkml/2013/1/11/559
AFAIK, using the vringh API directly is a deliberate design choice.

[Sjur:]
>> How do you see the in-kernel API for this? I would like to see
>> something similar to my previous patches, where we extend
>> the virtqueue API. E.g. something like this:
>> struct virtqueue *vring_new_virtqueueh(...)...

[Rusty:]
>I was just going to create _kernel variants of all the _user helpers,
>and let you drive it directly like that.
>
>If we get a second in-kernel user, we create wrappers (I'd prefer not to
>overload struct virtqueue though).


>> The functions rproc_virtio_get_vringh(), rproc_virtio_del_vringh(),
>> rproc_virtio_kick_vringh() are added to remoteproc_virtio.c.
>
> I wonder if this is the way we want things to work.
>
> Following this design, virtio drivers that use these rproc_* functions
> will be coupled with the remoteproc framework.
>
> One issue with this is what happens if, e.g., a VIRTIO_ID_CAIF vdev is
> added by other than remoteproc (e.g. by virtio_pci or virtio_mmio).
> Not sure how probable this really is, and whether there's anything
> that prevents this, but things will go awry if this happens.

Yes, if you insert a "malicious device" like that you can make it crash,
but wouldn't most drivers do if you try to register a malicious device...?

If we really want to protect from this, we could perhaps validate the vdev
pointer in function rproc_virtio_new_vringh() by looking through the vdevs
of the registered rprocs.

> But maybe the important aspect to consider is whether we really want
> to couple virtio drivers (such as the upcoming caif one) with the
> remoteproc framework.

I'm not sure this is an issue for the CAIF driver. It would be very nice
if someone else could make use of it, but right now cannot see the CAIF
driver being used outside the remoteproc framework. This driver is
designed specifically to work with the STE-modem using the CAIF
protocol over a shared memory interface.

> If you'll take a look at the rpmsg virtio driver, there's nothing
> there which couples it with remoteproc. It's just a standard virtio
> driver, that can be easily used with traditional virtio hosts as well.
>
> This is possible of course thanks to the abstraction provided by
> virtio: remoteproc only implements a set of callbacks which virtio
> invokes when needed.

Yes, and generalizing the use of virtio devices in remoteproc
has been useful. It has enabled me to let remoteproc manage both
virtio_serial and virtio_caif devices :-)

>
> Do we not want to follow a similar design scheme with vringh ?

I know some of my colleagues has been working on symmetric vring
for rpmsg. Last I heard from them they were going to use the same
approach I've done for CAIF, by "reversing" the direction of the rings.
AFAIK this means that the current API I have proposed will work for
them as well.

If some other driver is showing up using the vringh kernel API where
the current API don't fit, I guess it's time to create some abstractions
and wrappers... But I hope the current approach will do for now?

> I have some other questions as well but maybe it's better to discuss
> first the bigger picture.

OK, but please don't hesitate to address this. I'm still aiming for this
to go into 3.9.

Regards,
Sjur

Re: [PATCHv2 vringh 1/3] remoteproc: Add support for vringh (Host vrings)

[Rusty Russell]

Ohad Ben-Cohen <ohad@wizery.com> writes:
> Hi Sjur,
>
> On Tue, Feb 12, 2013 at 1:49 PM,  <sjur.brandeland@stericsson.com> wrote:
>> From: Sjur Brændeland <sjur.brandeland@stericsson.com>
>>
>> Add functions for creating, deleting and kicking host-side virtio rings.
>>
>> The host ring is not integrated with virtiqueues and cannot be managed
>> through virtio-config.
>
> Is that an inherent design/issue of vringh or just a description of
> the current vringh code ?

It's by design.  The producer (virtqueue) and consumer (vringh) are two
sides of the same coin, but they do different things.

virtqueue is a slightly higher level abstraction which assumes a
virtio_device, because every user so far has had one.  vringh doesn't,
because it's also aimed to underlie vhost.c which doesn't really have
one.

> This is possible of course thanks to the abstraction provided by
> virtio: remoteproc only implements a set of callbacks which virtio
> invokes when needed.
>
> Do we not want to follow a similar design scheme with vringh ?

Hmm... I clearly jumped the gun, assuming consensus was already reached.
I have put these patches *back* into pending-rebases, and they will not
be merged this merge window.

Cheers,
Rusty.

Re: [PATCHv2 vringh 1/3] remoteproc: Add support for vringh (Host vrings)

[Ohad Ben-Cohen]

On Thu, Feb 21, 2013 at 1:01 AM, Sjur Brændeland
<sjur.brandeland@stericsson.com> wrote:
> [Sjur:]
>>> How do you see the in-kernel API for this? I would like to see
>>> something similar to my previous patches, where we extend
>>> the virtqueue API. E.g. something like this:
>>> struct virtqueue *vring_new_virtqueueh(...)...
>
> [Rusty:]
>>I was just going to create _kernel variants of all the _user helpers,
>>and let you drive it directly like that.
>>
>>If we get a second in-kernel user, we create wrappers (I'd prefer not to
>>overload struct virtqueue though).

The wrappers suggestion sounds good.

One of the things we truly enjoyed about virtio is how easy it was to
reuse its drivers for communication with a remote processor.

It'd be great if we can stick to this design and keep vringh virtio
drivers decoupled from the low level implementation they are developed
with (specifically caif and remoteproc in this case).

> Yes, if you insert a "malicious device" like that you can make it crash,
> but wouldn't most drivers do if you try to register a malicious device...?
>
> If we really want to protect from this, we could perhaps validate the vdev
> pointer in function rproc_virtio_new_vringh() by looking through the vdevs
> of the registered rprocs.

It sounds like we can instead just avoid this altogether if we follow
Rusty's wrappers suggestion.

The result would probably look better, and I suspect it might be very
minimal code.

> If some other driver is showing up using the vringh kernel API where
> the current API don't fit, I guess it's time to create some abstractions
> and wrappers... But I hope the current approach will do for now?

Unless I'm missing something here, adding wrappers should be straight
forward and quick. Coupling a virtio driver with remoteproc doesn't
look great to me, probably because I appreciate so much the elegance
of virtio and how easy we could have reused its vanilla drivers with a
use case it wasn't originally designed to support.

>> I have some other questions as well but maybe it's better to discuss
>> first the bigger picture.
>
> OK, but please don't hesitate to address this. I'm still aiming for this
> to go into 3.9.

I was wondering - can you please explain your motivation for using
vringh in caif ?

We have internally discussed supporting multiple remote processors
talking to each other using rpmsg, and in that scenario using vringh
can considerably simplifies the solution (no need to decide in advance
which side is the "guest" and which is the "host"). Was this the
general incentive in using vringh in caif too or something else?

Thanks,
Ohad.

Re: [PATCHv2 vringh 1/3] remoteproc: Add support for vringh (Host vrings)

[Ohad Ben-Cohen]

On Thu, Feb 21, 2013 at 8:37 AM, Rusty Russell <rusty@rustcorp.com.au> wrote:
> Hmm... I clearly jumped the gun, assuming consensus was already reached.
> I have put these patches *back* into pending-rebases, and they will not
> be merged this merge window.

Thanks.

What do you think about creating some virtio-level wrappers for the
vringh handlers?

I don't think we're going to stop with caif as the only vringh user,
and it could be nice if we follow the virtio spirit of decoupling the
drivers from the low level implementation. It sure did prove itself
when the remoteproc use cases started showing up, and it's neat.

Thanks,
Ohad.

Re: [PATCHv2 vringh 1/3] remoteproc: Add support for vringh (Host vrings)

[Sjur Brændeland]

Hi Ohad,

> I was wondering - can you please explain your motivation for using
> vringh in caif ?
>
> We have internally discussed supporting multiple remote processors
> talking to each other using rpmsg, and in that scenario using vringh
> can considerably simplifies the solution (no need to decide in advance
> which side is the "guest" and which is the "host"). Was this the
> general incentive in using vringh in caif too or something else?

The motivation for using vringh was to avoid copying buffers
when sending data from the modem to the host. By using
vringh the modem can transfer datagrams to host only by
transfering buffer pointers via the rings.

We use a carveout to declare a memory area that is passed to the
modem internal memory allocator. This way we get both modem
and host to work on the shared memory region.

For TX traffic we use normal virtio queues for the same reason.
TX buffers can be handled without copying as well.

We are seeing a nice performance gain on the modem side after
implementing this.

Host size zero-copy is more tricky. It's hard to handle ownership of buffers,
if the modem crashes. But we might look into this in the future as well.

Regards,
Sjur

Re: [PATCHv2 vringh 1/3] remoteproc: Add support for vringh (Host vrings)

[Ohad Ben-Cohen]

Hi Sjur,

On Thu, Feb 21, 2013 at 7:28 PM, Sjur Brændeland <sjurbren@gmail.com> wrote:
> The motivation for using vringh was to avoid copying buffers
> when sending data from the modem to the host.

I may be missing something here, but why do you need vringh for that?

With rpmsg (which uses two regular vrings) both ends send huge amount
of data without copying it - we just send the pointers across.

Thanks,
Ohad.

Re: [PATCHv2 vringh 1/3] remoteproc: Add support for vringh (Host vrings)

[Sjur Brændeland]

Hi Ohad,

On Thu, Feb 21, 2013 at 6:55 PM, Ohad Ben-Cohen <ohad@wizery.com> wrote:
> On Thu, Feb 21, 2013 at 7:28 PM, Sjur Brændeland <sjurbren@gmail.com> wrote:
>> The motivation for using vringh was to avoid copying buffers
>> when sending data from the modem to the host.
>
> I may be missing something here, but why do you need vringh for that?
>
> With rpmsg (which uses two regular vrings) both ends send huge amount
> of data without copying it - we just send the pointers across.

OK, We did carefully consider using the normal vrings, but concluded it was
not doable. I'll try to give you some of the background that I can
recall from top of my head.
(I can dig out more if you're still not convinced :)

The modem we're integrating with is a complex beast (multi mode modem
LTE, HSPA+, etc).
When a packet is received deep down in the radio stack, it allocates packet
buffers using the internal slab allocator.The packet may contain
anything from control, voice,
or IP packets. If the packet contains IP-payload it will travel to a
different asymmetric CPU that
handles the IPC towards the modem. If the packet is not a IP-packet it will be
processed internally and eventually freed.

What we have done to integrate virtio is to inject the carveout into the
the modem internal slab-allocator.

Using the reversed ring allows us to introduce zero-copy for virtio
with virtually no
impact on the radio-stack, only a small change on the modem slab allocator.
It supports dynamic size buffer allocation, and modem internal
messaging using data
allocated from the shared region. It also allows modem to manage it's
own memory,
without any dependency on host side allocators.


Thanks,
Sjur

Re: [PATCHv2 vringh 1/3] remoteproc: Add support for vringh (Host vrings)

[Rusty Russell]

Ohad Ben-Cohen <ohad@wizery.com> writes:
> On Thu, Feb 21, 2013 at 8:37 AM, Rusty Russell <rusty@rustcorp.com.au> wrote:
>> Hmm... I clearly jumped the gun, assuming consensus was already reached.
>> I have put these patches *back* into pending-rebases, and they will not
>> be merged this merge window.
>
> Thanks.
>
> What do you think about creating some virtio-level wrappers for the
> vringh handlers?
>
> I don't think we're going to stop with caif as the only vringh user,
> and it could be nice if we follow the virtio spirit of decoupling the
> drivers from the low level implementation. It sure did prove itself
> when the remoteproc use cases started showing up, and it's neat.

The problem space is a bit different.  My immediate concern is getting
vhost (and thus vhost_net/blk) to use vringh: I wanted to unify the
in-userspace and in-kernelspace ring implementations.  We don't have
that issue in virtqueue.c.

vhost is (will be) the higher abstraction for in-userspace rings,
perhaps we want an equivalent for in-kernelspace rings.  I'm happy to
look at patches, but I don't immediately see what it would look like...

Thanks,
Rusty.

Re: [PATCHv2 vringh 1/3] remoteproc: Add support for vringh (Host vrings)

[Ohad Ben-Cohen]

On Thu, Feb 21, 2013 at 9:36 PM, Sjur Brændeland <sjurbren@gmail.com> wrote:
> OK, We did carefully consider using the normal vrings, but concluded it was
> not doable. I'll try to give you some of the background that I can
> recall from top of my head.
> (I can dig out more if you're still not convinced :)
>
> The modem we're integrating with is a complex beast (multi mode modem
> LTE, HSPA+, etc).
> When a packet is received deep down in the radio stack, it allocates packet
> buffers using the internal slab allocator.The packet may contain
> anything from control, voice,
> or IP packets. If the packet contains IP-payload it will travel to a
> different asymmetric CPU that
> handles the IPC towards the modem. If the packet is not a IP-packet it will be
> processed internally and eventually freed.
>
> What we have done to integrate virtio is to inject the carveout into the
> the modem internal slab-allocator.
>
> Using the reversed ring allows us to introduce zero-copy for virtio
> with virtually no
> impact on the radio-stack, only a small change on the modem slab allocator.
> It supports dynamic size buffer allocation, and modem internal
> messaging using data
> allocated from the shared region. It also allows modem to manage it's
> own memory,
> without any dependency on host side allocators.

Thanks for the description, Sjur.

It sounds like this mostly simplifies your modem-side code, especially
if it talks to other cores as well. My impression is that persistently
sticking to guest vrings is also possible, but it makes things
cumbersome.

We will most probably adopt vringh in rpmsg too when a real multicore
use case shows up.

Ohad.