Linux Virtualization - Apr 2013 - [PATCHv2 virtio-next] remoteproc: Add support for host virtio rings (vringh)

Implement the vringh callback functions in order
to manage host virito rings and handle kicks.
This allows virtio device to request host-virtio-rings.

Signed-off-by: Sjur Br?ndeland <sjur.brandeland at stericsson.com>
---

Hi Ohad and Rusty,

This v2 version is simpler, more readable and verbose (+50 lines)
compared to the previous patch.

This patch probably should to go via Rusty's tree due to the
vringh dependencies. Ohad could you please review this and let
us know what you think.

Thanks,
Sjur

 drivers/remoteproc/remoteproc_virtio.c |  208 +++++++++++++++++++++++++++++++-
 include/linux/remoteproc.h             |   22 ++++
 2 files changed, 225 insertions(+), 5 deletions(-)

diff --git a/drivers/remoteproc/remoteproc_virtio.c
b/drivers/remoteproc/remoteproc_virtio.c
index afed9b7..d01bec4 100644
--- a/drivers/remoteproc/remoteproc_virtio.c
+++ b/drivers/remoteproc/remoteproc_virtio.c
@@ -41,6 +41,18 @@ static void rproc_virtio_notify(struct virtqueue *vq)
 	rproc->ops->kick(rproc, notifyid);
 }
 
+/* kick the remote processor, and let it know which vring to poke at */
+static void rproc_virtio_vringh_notify(struct vringh *vrh)
+{
+	struct rproc_vring *rvring = vringh_to_rvring(vrh);
+	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);
+}
+
 /**
  * rproc_vq_interrupt() - tell remoteproc that a virtqueue is interrupted
  * @rproc: handle to the remote processor
@@ -60,10 +72,18 @@ 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->rvringh && rvring->rvringh->vringh_cb) {
+		rvring->rvringh->vringh_cb(&rvring->rvdev->vdev,
+						&rvring->rvringh->vrh);
+		return IRQ_HANDLED;
+	} else if (rvring->vq) {
+		return vring_interrupt(0, rvring->vq);
+	} else {
+		return IRQ_NONE;
+	}
 }
 EXPORT_SYMBOL(rproc_vq_interrupt);
 
@@ -78,7 +98,7 @@ static struct virtqueue *rp_find_vq(struct virtio_device
*vdev,
 	struct rproc_vring *rvring;
 	struct virtqueue *vq;
 	void *addr;
-	int len, size, ret;
+	int len, size, ret, i;
 
 	/* we're temporarily limited to two virtqueues per rvdev */
 	if (id >= ARRAY_SIZE(rvdev->vring))
@@ -87,11 +107,26 @@ static struct virtqueue *rp_find_vq(struct virtio_device
*vdev,
 	if (!name)
 		return NULL;
 
-	ret = rproc_alloc_vring(rvdev, id);
+	/* Find available vring for a new vq */
+	for (i = id; i < ARRAY_SIZE(rvdev->vring); i++) {
+		rvring = &rvdev->vring[i];
+
+		/* Calling find_vqs twice is bad */
+		if (rvring->vq)
+			return ERR_PTR(-EINVAL);
+
+		/* Use vring not already in use */
+		if (!rvring->rvringh)
+			break;
+	}
+
+	if (i == ARRAY_SIZE(rvdev->vring))
+		return ERR_PTR(-ENODEV);
+
+	ret = rproc_alloc_vring(rvdev, i);
 	if (ret)
 		return ERR_PTR(ret);
 
-	rvring = &rvdev->vring[id];
 	addr = rvring->va;
 	len = rvring->len;
 
@@ -222,6 +257,168 @@ static void rproc_virtio_finalize_features(struct
virtio_device *vdev)
 	rvdev->gfeatures = vdev->features[0];
 }
 
+/* Helper function that creates and initializes the host virtio ring */
+static struct vringh *rproc_create_new_vringh(struct rproc_vring *rvring,
+					unsigned int index,
+					vrh_callback_t callback)
+{
+	struct rproc_vringh *rvrh = NULL;
+	struct rproc_vdev *rvdev = rvring->rvdev;
+	int err;
+
+	rvrh = kzalloc(sizeof(*rvrh), GFP_KERNEL);
+	err = -ENOMEM;
+	if (!rvrh)
+		goto err;
+
+	/* initialize the host virtio ring */
+	rvrh->vringh_cb = callback;
+	rvrh->vrh.notify = rproc_virtio_vringh_notify;
+	memset(rvring->va, 0, vring_size(rvring->len, rvring->align));
+	vring_init(&rvrh->vrh.vring, rvring->len, rvring->va,
rvring->align);
+
+	/*
+	 * Create the new vring host, and tell we're not interested in
+	 * the 'weak' smp barriers, since we're talking with a real
device.
+	 */
+	err = vringh_init_kern(&rvrh->vrh,
+				rproc_virtio_get_features(&rvdev->vdev),
+				rvring->len,
+				false,
+				rvrh->vrh.vring.desc,
+				rvrh->vrh.vring.avail,
+				rvrh->vrh.vring.used);
+	if (err) {
+		dev_err(&rvdev->rproc->dev,
+			"vringh_init_kern failed\n");
+		goto err;
+	}
+
+	rvring->rvringh = rvrh;
+	return &rvrh->vrh;
+err:
+	kfree(rvrh);
+	return ERR_PTR(err);
+}
+
+static struct vringh *rp_find_vrh(struct virtio_device *vdev,
+				unsigned id,
+				vrh_callback_t callback)
+{
+	struct rproc_vdev *rvdev = vdev_to_rvdev(vdev);
+	struct rproc *rproc = vdev_to_rproc(vdev);
+	struct device *dev = &rproc->dev;
+	struct rproc_vring *rvring;
+	struct vringh *vrh;
+	void *addr;
+	int len, size, ret, i;
+
+	/* we're temporarily limited to two virtqueues per rvdev */
+	if (id >= ARRAY_SIZE(rvdev->vring))
+		return ERR_PTR(-EINVAL);
+
+	/* Find available slot for a new host vring */
+	for (i = id; i < ARRAY_SIZE(rvdev->vring); i++) {
+		rvring = &rvdev->vring[i];
+
+		/* Calling find_vrhs twice is bad */
+		if (rvring->rvringh)
+			return ERR_PTR(-EINVAL);
+
+		/* Use vring not already in use */
+		if (!rvring->vq)
+			break;
+	}
+
+	if (i == ARRAY_SIZE(rvdev->vring))
+		return ERR_PTR(-ENODEV);
+
+	ret = rproc_alloc_vring(rvdev, i);
+	if (ret)
+		return ERR_PTR(ret);
+
+	addr = rvring->va;
+	len = rvring->len;
+
+	/* zero vring */
+	size = vring_size(len, rvring->align);
+	memset(addr, 0, size);
+
+	dev_dbg(dev, "vring%d: va %p qsz %d notifyid %d\n",
+					id, addr, len, rvring->notifyid);
+
+	/*
+	 * Create the new vringh, and tell virtio we're not interested in
+	 * the 'weak' smp barriers, since we're talking with a real
device.
+	 */
+	vrh = rproc_create_new_vringh(rvring, id, callback);
+	if (!vrh) {
+		rproc_free_vring(rvring);
+		return ERR_PTR(-ENOMEM);
+	}
+
+	return vrh;
+}
+
+static void __rproc_virtio_del_vrhs(struct virtio_device *vdev)
+{
+	struct rproc_vdev *rvdev = vdev_to_rvdev(vdev);
+	int i, num_of_vrings = ARRAY_SIZE(rvdev->vring);
+
+	for (i = 0; i < num_of_vrings; i++) {
+		struct rproc_vring *rvring = &rvdev->vring[i];
+		if (!rvring->rvringh)
+			continue;
+		kfree(rvring->rvringh);
+		rvring->rvringh = NULL;
+		rproc_free_vring(rvring);
+	}
+}
+
+static void rproc_virtio_del_vrhs(struct virtio_device *vdev)
+{
+	struct rproc *rproc = vdev_to_rproc(vdev);
+
+	/* power down the remote processor before deleting vqs */
+	rproc_shutdown(rproc);
+
+	__rproc_virtio_del_vrhs(vdev);
+}
+
+static int rproc_virtio_find_vrhs(struct virtio_device *vdev, unsigned nhvrs,
+			 struct vringh *vrhs[],
+			 vrh_callback_t *callbacks[])
+{
+	struct rproc *rproc = vdev_to_rproc(vdev);
+	int i, ret;
+
+	for (i = 0; i < nhvrs; ++i) {
+		vrhs[i] = rp_find_vrh(vdev, i, callbacks[i]);
+		if (IS_ERR(vrhs[i])) {
+			ret = PTR_ERR(vrhs[i]);
+			goto error;
+		}
+	}
+
+	/* now that the vqs are all set, boot the remote processor */
+	ret = rproc_boot(rproc);
+	if (ret) {
+		dev_err(&rproc->dev, "rproc_boot() failed %d\n", ret);
+		goto error;
+	}
+
+	return 0;
+
+error:
+	__rproc_virtio_del_vrhs(vdev);
+	return ret;
+}
+
+static struct vringh_config_ops rproc_virtio_vringh_ops = {
+	.find_vrhs	= rproc_virtio_find_vrhs,
+	.del_vrhs	= rproc_virtio_del_vrhs,
+};
+
 static const struct virtio_config_ops rproc_virtio_config_ops = {
 	.get_features	= rproc_virtio_get_features,
 	.finalize_features = rproc_virtio_finalize_features,
@@ -270,6 +467,7 @@ int rproc_add_virtio_dev(struct rproc_vdev *rvdev, int id)
 
 	vdev->id.device	= id,
 	vdev->config = &rproc_virtio_config_ops,
+	vdev->vringh_config = &rproc_virtio_vringh_ops;
 	vdev->dev.parent = dev;
 	vdev->dev.release = rproc_vdev_release;
 
diff --git a/include/linux/remoteproc.h b/include/linux/remoteproc.h
index faf3332..be191bc 100644
--- a/include/linux/remoteproc.h
+++ b/include/linux/remoteproc.h
@@ -39,6 +39,7 @@
 #include <linux/klist.h>
 #include <linux/mutex.h>
 #include <linux/virtio.h>
+#include <linux/vringh.h>
 #include <linux/completion.h>
 #include <linux/idr.h>
 
@@ -435,6 +436,19 @@ struct rproc {
 #define RVDEV_NUM_VRINGS 2
 
 /**
+ * struct rproc_vringh - remoteproc host vring
+ * @vrh: Host side virtio ring
+ * @rvring: Virtio ring associated with the device
+ * @vringh_cb: Callback notifying virtio driver about new buffers
+ */
+struct rproc_vring;
+struct rproc_vringh {
+	struct vringh vrh;
+	struct rproc_vring *rvring;
+	vrh_callback_t *vringh_cb;
+};
+
+/**
  * struct rproc_vring - remoteproc vring state
  * @va:	virtual address
  * @dma: dma address
@@ -444,6 +458,7 @@ struct rproc {
  * @notifyid: rproc-specific unique vring index
  * @rvdev: remote vdev
  * @vq: the virtqueue of this vring
+ * @rvringh: the reversed host-side vring
  */
 struct rproc_vring {
 	void *va;
@@ -454,6 +469,7 @@ struct rproc_vring {
 	int notifyid;
 	struct rproc_vdev *rvdev;
 	struct virtqueue *vq;
+	struct rproc_vringh *rvringh;
 };
 
 /**
@@ -497,4 +513,10 @@ static inline struct rproc *vdev_to_rproc(struct
virtio_device *vdev)
 	return rvdev->rproc;
 }
 
+static inline struct rproc_vring *vringh_to_rvring(struct vringh *vrh)
+{
+	struct rproc_vringh *rvrh = container_of(vrh, struct rproc_vringh, vrh);
+	return rvrh->rvring;
+}
+
 #endif /* REMOTEPROC_H */
-- 
1.7.9.5

Hi Sjur and Rusty,

On Wed, Apr 10, 2013 at 12:39 AM, Sjur Br?ndeland
<sjur.brandeland at stericsson.com> wrote:
> Implement the vringh callback functions in order
> to manage host virito rings and handle kicks.
> This allows virtio device to request host-virtio-rings.
>
> Signed-off-by: Sjur Br?ndeland <sjur.brandeland at stericsson.com>
Thanks for pushing this.

I have a few observations which I'd like to discuss with Rusty and you.

Rusty - will you be willing to consider a few patches to virtio which
will considerably simplify kernel users of vringh (such as this one) ?

The main motivation is to reduce code duplication and complexity.
Right now the code handling vringh is awfully similar to the one
handling regular vrings, with a few asymmetries:
- We need to call and maintain the vrh_callback_t pointer directly,
instead of relying on vring_interrupt()
- We have some vringh creation boilerplate code of our own very
similar to what vring_new_virtqueue is doing for regular vrings
- It seems that a driver which has both regular rings and host rings
(as caif does) will have to "find_vqs" them twice - once for the
regular rings and one for the host rings - where each of those
invocations will use its own set of indices. To simplify drivers it
might be nice if we had a single find_rings function which would
enumerate both regular and host rings using a single indices axis. It
may presumably take two nvqs params, one for the regular rings and the
other for the host rings, and we can arbitrarily decide it always
creates the regular rings first.

Stuff which will be nice to change along these lines:

- maintain the vrh_callback_t pointer in struct vringh, similarly to
what struct virtqueue does today for callbacks of regular rings
- when kicked, just call vring_interrupt, and let it demultiplex the
event to the appropriate ring (whether it is regular or host ring)
- try to push code from rproc_create_new_vringh into virtio, following
the lines of vring_new_virtqueue and regular rings
- try to merge the regular and host rings versions of the
find_vqs/del_vqs boilerplate code to avoid duplication

I guess this all depends on how such patches will look like and
whether we can reach an elegant implementation. I'm also aware that
host rings are being used by user space too, and we must not break
anything.

Thanks,
Ohad.