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

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

[Sjur Brændeland]

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@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

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

Re: [PATCHv2 virtio-next] remoteproc: Add support for host virtio rings (vringh)

[Ohad Ben-Cohen]

Hi Sjur and Rusty,

On Wed, Apr 10, 2013 at 12:39 AM, Sjur Brændeland
<sjur.brandeland@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@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.

Re: [PATCHv2 virtio-next] remoteproc: Add support for host virtio rings (vringh)

[Rusty Russell]

Ohad Ben-Cohen <ohad@wizery.com> writes:
> Hi Sjur and Rusty,
> 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.

Oh, we can break everything :)

I was concentrating purely on the mechanics of the virtqueue, mainly
because vhost has special needs wrt tracking changes.  vhost doesn't use
vringh yet because my patches are slightly suboptimal (I stick with the
vhost API, just replace the guts with vringh).  Michael has a
simplification of vhost-net pending, which will make altering this much
easier.

But CAIF isn't the right thing to optimize for, either.  It's weird to
have both host and guest rings at the same time, and I don't see other
users doing that (ie. vhost_net and tcm_vhost).  But if we can make it
easier for you without overly uglifying vringh, that'd be great.

And yes, I'd like a core of code which I can license liberally to
include or ship alongside the spec.  But I think we can manage that.

Cheers,
Rusty.

Re: [PATCHv2 virtio-next] remoteproc: Add support for host virtio rings (vringh)

[Sjur Brændeland]

Hi Ohad,

On Mon, Apr 22, 2013 at 3:08 PM, Ohad Ben-Cohen <ohad@wizery.com> wrote:
> Hi Sjur and Rusty,
>
> On Wed, Apr 10, 2013 at 12:39 AM, Sjur Brændeland
> <sjur.brandeland@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@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.

Quite frankly I would *really* like to see vringh support in remoteproc
included in 3.10, and I am afraid your suggestion will cause us to miss
the merge window once again.

How about including my simple, but verbose patch in virtio-next for 3.10.
Then you could post patches that refactors remoteproc and vringh
making the code more elegant and less verbose on top of my patch.

Doing this iteratively would also make it easy to verify your changes,
and show how your patches reduces overall code size and provides
a more elegant solution.

Thanks,
Sjur
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Re: [PATCHv2 virtio-next] remoteproc: Add support for host virtio rings (vringh)

[Ohad Ben-Cohen]

On Tue, Apr 23, 2013 at 6:46 AM, Rusty Russell <rusty@rustcorp.com.au> wrote:
> Oh, we can break everything :)
>
> I was concentrating purely on the mechanics of the virtqueue, mainly
> because vhost has special needs wrt tracking changes.  vhost doesn't use
> vringh yet because my patches are slightly suboptimal (I stick with the
> vhost API, just replace the guts with vringh).  Michael has a
> simplification of vhost-net pending, which will make altering this much
> easier.
>
> But CAIF isn't the right thing to optimize for, either.  It's weird to
> have both host and guest rings at the same time, and I don't see other
> users doing that (ie. vhost_net and tcm_vhost).  But if we can make it
> easier for you without overly uglifying vringh, that'd be great.

Thanks.

Today with one application processor talking to one or several remote
cores we live well with guest rings, but future SoCs seems to be
having an increasing number of on-chip cores which all talk to each
other. Managing this matrix of communications with guest rings is
somewhat cumbersome - it requires deciding, for every two cores, who
is "the guest" and who is "the host". As the number of edges in this
graph increases, this would be increasingly harder to develop, set up
and debug.

In such environments it makes sense to have, for each pair of on-chip
cores, 1 guest and 1 host ring. This way each core will maintain its
own TX buffers and send a buffer across whenever it has a pending
outbound message. This also works well with systems where each core
has its own memory which only it can write to, and from which others
could only read.

So I expect additional users for this paradigm CAIF has adopted -
probably rpmsg at the very least - which makes it even more appealing
to clean up nicely. Last year I discussed this at least with Loic
(STE) and Suman (TI) and both companies were actively developing this
for their future SoCs - I'm cc'ing both in case there are any updates.

Thanks,
Ohad.

Re: [PATCHv2 virtio-next] remoteproc: Add support for host virtio rings (vringh)

[Ohad Ben-Cohen]

Hi Sjur,

On Tue, Apr 23, 2013 at 11:32 AM, Sjur Brændeland <sjur@brendeland.net> wrote:
> Quite frankly I would *really* like to see vringh support in remoteproc
> included in 3.10, and I am afraid your suggestion will cause us to miss
> the merge window once again.
>
> How about including my simple, but verbose patch in virtio-next for 3.10.
> Then you could post patches that refactors remoteproc and vringh
> making the code more elegant and less verbose on top of my patch.
>
> Doing this iteratively would also make it easy to verify your changes,
> and show how your patches reduces overall code size and provides
> a more elegant solution.

I realize this is frustrating for you, and I'm sorry about that. You
did a great job and I'm sure the patch works great for you.

I don't feel the patch is ready for mainline merger at this point though.

There's a good chance that eventually not much will be left from this
patch as it is mostly a duplication of existing code with very little
changes, and I'm afraid that if we do merge it now it's going to be
much harder to change things after the fact and this will become a
maintainability burden for us.

If STE requires this functionality ASAP then this patch can surely be
applied out-of-tree - I doubt there's any product out there that is
built purely out of vanilla upstream kernel anyway. Please feel free
to blame me if there's any internal pressure on you because of this
delay.

I would also appreciate any help from you (or Dmitry) to try and
implement the changes I've outlined, as I'm not sure how much free
time I'm going to have which I can dedicate for this work. I'm happy
to offer any help to further elaborate or prototype this if needed.
Hopefully we can work together towards a satisfactory result.

Thanks!
Ohad.

Re: [PATCHv2 virtio-next] remoteproc: Add support for host virtio rings (vringh)

[Loic PALLARDY]

Hi Ohad, Rusty,

On 04/23/2013 01:02 PM, Ohad Ben-Cohen wrote:
> On Tue, Apr 23, 2013 at 6:46 AM, Rusty Russell<rusty@rustcorp.com.au>  wrote:
>> Oh, we can break everything :)
>>
>> I was concentrating purely on the mechanics of the virtqueue, mainly
>> because vhost has special needs wrt tracking changes.  vhost doesn't use
>> vringh yet because my patches are slightly suboptimal (I stick with the
>> vhost API, just replace the guts with vringh).  Michael has a
>> simplification of vhost-net pending, which will make altering this much
>> easier.
>>
>> But CAIF isn't the right thing to optimize for, either.  It's weird to
>> have both host and guest rings at the same time, and I don't see other
>> users doing that (ie. vhost_net and tcm_vhost).  But if we can make it
>> easier for you without overly uglifying vringh, that'd be great.
>
> Thanks.
>
> Today with one application processor talking to one or several remote
> cores we live well with guest rings, but future SoCs seems to be
> having an increasing number of on-chip cores which all talk to each
> other. Managing this matrix of communications with guest rings is
> somewhat cumbersome - it requires deciding, for every two cores, who
> is "the guest" and who is "the host". As the number of edges in this
> graph increases, this would be increasingly harder to develop, set up
> and debug.
>
> In such environments it makes sense to have, for each pair of on-chip
> cores, 1 guest and 1 host ring. This way each core will maintain its
> own TX buffers and send a buffer across whenever it has a pending
> outbound message. This also works well with systems where each core
> has its own memory which only it can write to, and from which others
> could only read.
>
> So I expect additional users for this paradigm CAIF has adopted -
> probably rpmsg at the very least - which makes it even more appealing
> to clean up nicely. Last year I discussed this at least with Loic
> (STE) and Suman (TI) and both companies were actively developing this
> for their future SoCs - I'm cc'ing both in case there are any updates.
>
Yes I confirm. In ST future SoCs, the different coprocessors should be 
able to exchange each other. In different use cases, AP could on or off.
This symmetric buffer management is key for future design.

Regards,
Loic

> Thanks,
> Ohad.

Re: [PATCHv2 virtio-next] remoteproc: Add support for host virtio rings (vringh)

[Rusty Russell]

Ohad Ben-Cohen <ohad@wizery.com> writes:
> On Tue, Apr 23, 2013 at 6:46 AM, Rusty Russell <rusty@rustcorp.com.au> wrote:
>> Oh, we can break everything :)
>>
>> I was concentrating purely on the mechanics of the virtqueue, mainly
>> because vhost has special needs wrt tracking changes.  vhost doesn't use
>> vringh yet because my patches are slightly suboptimal (I stick with the
>> vhost API, just replace the guts with vringh).  Michael has a
>> simplification of vhost-net pending, which will make altering this much
>> easier.
>>
>> But CAIF isn't the right thing to optimize for, either.  It's weird to
>> have both host and guest rings at the same time, and I don't see other
>> users doing that (ie. vhost_net and tcm_vhost).  But if we can make it
>> easier for you without overly uglifying vringh, that'd be great.
>
> Thanks.
>
> Today with one application processor talking to one or several remote
> cores we live well with guest rings, but future SoCs seems to be
> having an increasing number of on-chip cores which all talk to each
> other. Managing this matrix of communications with guest rings is
> somewhat cumbersome - it requires deciding, for every two cores, who
> is "the guest" and who is "the host". As the number of edges in this
> graph increases, this would be increasingly harder to develop, set up
> and debug.
>
> In such environments it makes sense to have, for each pair of on-chip
> cores, 1 guest and 1 host ring. This way each core will maintain its
> own TX buffers and send a buffer across whenever it has a pending
> outbound message. This also works well with systems where each core
> has its own memory which only it can write to, and from which others
> could only read.
>
> So I expect additional users for this paradigm CAIF has adopted -
> probably rpmsg at the very least - which makes it even more appealing
> to clean up nicely. Last year I discussed this at least with Loic
> (STE) and Suman (TI) and both companies were actively developing this
> for their future SoCs - I'm cc'ing both in case there are any updates.

Perhaps we should add a new struct virtio_pair (?), with associated ops,
which works on top of the existing stuff.  That may be too many levels
of abstraction, but it feels right.

I don't mind Sjur's current simple code; there's nothing magical about
upstream APIs, and I'm happy to merge it now and add an abstraction
later.  It's a pretty common practice, but it's your call.

Cheers,
Rusty.

Re: [PATCHv2 virtio-next] remoteproc: Add support for host virtio rings (vringh)

[Ohad Ben-Cohen]

On Mon, Apr 29, 2013 at 3:33 AM, Rusty Russell <rusty@rustcorp.com.au> wrote:
> Ohad Ben-Cohen <ohad@wizery.com> writes:
>> On Tue, Apr 23, 2013 at 6:46 AM, Rusty Russell <rusty@rustcorp.com.au> wrote:
>>> Oh, we can break everything :)
>>>
>>> I was concentrating purely on the mechanics of the virtqueue, mainly
>>> because vhost has special needs wrt tracking changes.  vhost doesn't use
>>> vringh yet because my patches are slightly suboptimal (I stick with the
>>> vhost API, just replace the guts with vringh).  Michael has a
>>> simplification of vhost-net pending, which will make altering this much
>>> easier.
>>>
>>> But CAIF isn't the right thing to optimize for, either.  It's weird to
>>> have both host and guest rings at the same time, and I don't see other
>>> users doing that (ie. vhost_net and tcm_vhost).  But if we can make it
>>> easier for you without overly uglifying vringh, that'd be great.
>>
>> Thanks.
>>
>> Today with one application processor talking to one or several remote
>> cores we live well with guest rings, but future SoCs seems to be
>> having an increasing number of on-chip cores which all talk to each
>> other. Managing this matrix of communications with guest rings is
>> somewhat cumbersome - it requires deciding, for every two cores, who
>> is "the guest" and who is "the host". As the number of edges in this
>> graph increases, this would be increasingly harder to develop, set up
>> and debug.
>>
>> In such environments it makes sense to have, for each pair of on-chip
>> cores, 1 guest and 1 host ring. This way each core will maintain its
>> own TX buffers and send a buffer across whenever it has a pending
>> outbound message. This also works well with systems where each core
>> has its own memory which only it can write to, and from which others
>> could only read.
>>
>> So I expect additional users for this paradigm CAIF has adopted -
>> probably rpmsg at the very least - which makes it even more appealing
>> to clean up nicely. Last year I discussed this at least with Loic
>> (STE) and Suman (TI) and both companies were actively developing this
>> for their future SoCs - I'm cc'ing both in case there are any updates.
>
> Perhaps we should add a new struct virtio_pair (?), with associated ops,
> which works on top of the existing stuff.  That may be too many levels
> of abstraction, but it feels right.

Interesting idea!

> I don't mind Sjur's current simple code; there's nothing magical about
> upstream APIs, and I'm happy to merge it now and add an abstraction
> later.  It's a pretty common practice, but it's your call.

I agree the code is simple but the problem is that it's basically a
duplication of existing code (see remoteproc_virtio.c) with very
little changes. Since I personally have little time myself to work on
it (unfortunately), I'm concerned that once merged there will be
little incentive for anyone to work on it and eliminate the code
duplication.

Thanks!
Ohad.