[PATCH virtio-next] caif_virtio: Remove bouncing email addresses
Remove our (soon to be) bouncing email addresses,
and update Dmitri's address.
Dmitry will take over as maintainer for CAIF from now on.
Cc: Vikram Arv
Cc: Dmitry Tarnyagin
Cc: Dmitry Tarnyagin
Signed-off-by: Sjur Brændeland
---
From end of April I will no longer work for ST-Ericsson.
But I might stick around as a hobbyist for a short while...
Thanks,
Sjur
drivers/net/caif/caif_virtio.c | 10 +-
1 files changed, 5 insertions(+), 5 deletions(-)
diff --git a/drivers/net/caif/caif_virtio.c b/drivers/net/caif/caif_virtio.c
index 0e3bede..b9ed128 100644
--- a/drivers/net/caif/caif_virtio.c
+++ b/drivers/net/caif/caif_virtio.c
@@ -1,8 +1,8 @@
/*
* Copyright (C) ST-Ericsson AB 2013
- * Authors: Vicram Arv / vikram@stericsson.com,
- * Dmitry Tarnyagin / dmitry.tarnya...@stericsson.com
- * Sjur Brendeland / sjur.brandel...@stericsson.com
+ * Authors: Vicram Arv
+ * Dmitry Tarnyagin
+ * Sjur Brendeland
* License terms: GNU General Public License (GPL) version 2
*/
#include
@@ -23,8 +23,8 @@
#include
MODULE_LICENSE("GPL v2");
-MODULE_AUTHOR("Vicram Arv ");
-MODULE_AUTHOR("Sjur Brendeland ");
+MODULE_AUTHOR("Vicram Arv");
+MODULE_AUTHOR("Sjur Brendeland");
MODULE_DESCRIPTION("Virtio CAIF Driver");
/* NAPI schedule quota */
--
1.7.5.4
___
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)
Hi Ohad, On Mon, Apr 22, 2013 at 3:08 PM, Ohad Ben-Cohen wrote: > Hi Sjur and Rusty, > > On Wed, Apr 10, 2013 at 12:39 AM, Sjur Brændeland > 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 > > 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
[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
---
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),
+
Re: [PATCH 2/2] remoteproc: Add support for host virtio rings (vringh)
Hi Ohad, On Fri, Mar 15, 2013 at 10:29 AM, Erwan Yvin wrote: > From: Erwan Yvin > > Implement the vringh callback functions in order > to manage host virtio rings and handle kicks. > This allows virtio device to request host-virtio-rings. > > Signed-off-by: Erwan Yvin Any chance that you could review this in time for 3.10 merge window? This is the last missing piece for host vring support. The patches would have to be Acked/Reviewed by and should go via Rusty's tree. Thanks, Sjur ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH 03/22] virtio_config: make transports implement accessors.
Hi Rusty,
On Sun, Mar 24, 2013 at 5:24 AM, Rusty Russell wrote:
>
> Sjur Brændeland writes:
> > On Thu, Mar 21, 2013 at 9:29 AM, Rusty Russell
> > wrote:
> > Would it be possible to make this simpler and less verbose somehow?
> > At least three virtio devices: virtio_pci_legacy.c, virtio_mmio.c and
> > soon remoteproc_virtio.c will duplicate variants of the code above.
> >
> > What if set8/get8 was mandatory, and the 16,32,64 variants where optional,
> > and then virtio_creadX() virtio_cwriteX did the magic to make things work?
>
> But this is a case where simpler and less verbose are opposites. These
> patches are really straightforward. Noone can forget to write or assign
> an accessor and have still work on x86 and fail for BE machines.
>
> So I like explicit accessors, set by the backend. But it doesn't have
> to be quite this ugly.
>
> How's this (completely untested!)
Looks really good to me. This way we avoid copying code around between
virtio devices.
BTW: If you are planning to merge this is for 3.10 we will get some
compile issues in linux-next when merging with my latest remoteproc patches.
But with the changes below it should be easy enough to fix.
Thanks,
Sjur
>
> diff --git a/drivers/virtio/virtio.c b/drivers/virtio/virtio.c
> index e342692..6ffa542 100644
> --- a/drivers/virtio/virtio.c
> +++ b/drivers/virtio/virtio.c
> @@ -219,6 +219,75 @@ out:
> }
> EXPORT_SYMBOL_GPL(register_virtio_device);
>
> +static void noconv_get(struct virtio_device *vdev, unsigned offset,
> + size_t len, void *p)
> +{
> + u8 *buf = p;
> + while (len) {
> + *buf = vdev->config->get8(vdev, offset);
> + buf++;
> + offset++;
> + len--;
> + }
> +}
> +
> +u16 virtio_config_get_noconv16(struct virtio_device *vdev, unsigned offset)
> +{
> + u16 v;
> + noconv_get(vdev, offset, sizeof(v), &v);
> + return v;
> +}
> +EXPORT_SYMBOL_GPL(virtio_config_get_noconv16);
> +
> +u32 virtio_config_get_noconv32(struct virtio_device *vdev, unsigned offset)
> +{
> + u32 v;
> + noconv_get(vdev, offset, sizeof(v), &v);
> + return v;
> +}
> +EXPORT_SYMBOL_GPL(virtio_config_get_noconv32);
> +
> +u64 virtio_config_get_noconv64(struct virtio_device *vdev, unsigned offset)
> +{
> + u64 v;
> + noconv_get(vdev, offset, sizeof(v), &v);
> + return v;
> +}
> +EXPORT_SYMBOL_GPL(virtio_config_get_noconv64);
> +
> +static void noconv_set(struct virtio_device *vdev, unsigned offset,
> + size_t len, const void *p)
> +{
> + const u8 *buf = p;
> + while (len) {
> + vdev->config->set8(vdev, offset, *buf);
> + buf++;
> + offset++;
> + len--;
> + }
> +}
> +
> +void virtio_config_set_noconv16(struct virtio_device *vdev,
> + unsigned offset, u16 v)
> +{
> + noconv_set(vdev, offset, sizeof(v), &v);
> +}
> +EXPORT_SYMBOL_GPL(virtio_config_set_noconv16);
> +
> +void virtio_config_set_noconv32(struct virtio_device *vdev,
> + unsigned offset, u32 v)
> +{
> + noconv_set(vdev, offset, sizeof(v), &v);
> +}
> +EXPORT_SYMBOL_GPL(virtio_config_set_noconv32);
> +
> +void virtio_config_set_noconv64(struct virtio_device *vdev,
> + unsigned offset, u64 v)
> +{
> + noconv_set(vdev, offset, sizeof(v), &v);
> +}
> +EXPORT_SYMBOL_GPL(virtio_config_set_noconv64);
> +
> void unregister_virtio_device(struct virtio_device *dev)
> {
> int index = dev->index; /* save for after device release */
> diff --git a/include/linux/virtio_config.h b/include/linux/virtio_config.h
> index 9dfe116..9a9aaa0 100644
> --- a/include/linux/virtio_config.h
> +++ b/include/linux/virtio_config.h
> @@ -286,4 +286,23 @@ static inline void virtio_cwrite64(struct virtio_device
> *vdev,
> _r; \
> })
>
> +/* Helpers for non-endian converting transports. */
> +u16 virtio_config_get_noconv16(struct virtio_device *vdev, unsigned offset);
> +u32 virtio_config_get_noconv32(struct virtio_device *vdev, unsigned offset);
> +u64 virtio_config_get_noconv64(struct virtio_device *vdev, unsigned offset);
> +void virtio_config_set_noconv16(struct virtio_device *vdev,
> + unsigned offset, u16 v);
> +void virtio_config_set_noconv32(struct virtio_device *vdev,
> + unsigned offset, u32 v);
> +v
[PATCH virtio-next 2/2] caif_virtio: Check that vringh_config is not null
Check that vringh_config is not NULL before using it. Signed-off-by: Sjur Brændeland --- Hi, This patch applies to Rusty's virtio-next branch. Thanks, Sjur drivers/net/caif/caif_virtio.c |4 1 file changed, 4 insertions(+) diff --git a/drivers/net/caif/caif_virtio.c b/drivers/net/caif/caif_virtio.c index fb80765..316b184 100644 --- a/drivers/net/caif/caif_virtio.c +++ b/drivers/net/caif/caif_virtio.c @@ -670,6 +670,10 @@ static int cfv_probe(struct virtio_device *vdev) spin_lock_init(&cfv->tx_lock); /* Get the RX virtio ring. This is a "host side vring". */ + err = -ENODEV; + if (!vdev->vringh_config || !vdev->vringh_config->find_vrhs) + goto err; + err = vdev->vringh_config->find_vrhs(vdev, 1, &cfv->vr_rx, &vrh_cbs); if (err) goto err; -- 1.7.9.5 ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
[PATCH virtio-next 1/2] caif_virtio: Use vringh_notify_enable correctly
Check on the correct return value from
vringh_notify_enable_kern(). It returns false if
more packets are available, not true.
Signed-off-by: Sjur Brændeland
---
Hi,
This patch applies to Rusty's virtio-next branch.
Thanks,
Sjur
drivers/net/caif/caif_virtio.c |2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/net/caif/caif_virtio.c b/drivers/net/caif/caif_virtio.c
index f6caa1e..fb80765 100644
--- a/drivers/net/caif/caif_virtio.c
+++ b/drivers/net/caif/caif_virtio.c
@@ -318,7 +318,7 @@ exit:
/* Really out of patckets? (stolen from virtio_net)*/
napi_complete(napi);
- if (unlikely(vringh_notify_enable_kern(cfv->vr_rx)) &&
+ if (unlikely(!vringh_notify_enable_kern(cfv->vr_rx)) &&
napi_schedule_prep(napi)) {
vringh_notify_disable_kern(cfv->vr_rx);
__napi_schedule(napi);
--
1.7.9.5
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH 05/22] virtio: add support for 64 bit features.
On Thu, Mar 21, 2013 at 11:06 AM, Cornelia Huck
wrote:
> On Thu, 21 Mar 2013 18:59:26 +1030
> Rusty Russell wrote:
>
>> Change the u32 to a u64, and make sure to use 1ULL everywhere!
>>
>> Cc: Ohad Ben-Cohen
>> Cc: Brian Swetland
>> Cc: Cornelia Huck
>> Cc: Pawel Moll
>> Cc: Christian Borntraeger
>> Signed-off-by: Rusty Russell
>> ---
>> drivers/char/virtio_console.c |2 +-
>> drivers/lguest/lguest_device.c | 10 +-
>> drivers/remoteproc/remoteproc_virtio.c |6 +-
>> drivers/s390/kvm/kvm_virtio.c | 10 +-
>> drivers/virtio/virtio.c| 12 ++--
>> drivers/virtio/virtio_mmio.c | 14 +-
>> drivers/virtio/virtio_pci.c|5 ++---
>> drivers/virtio/virtio_ring.c |2 +-
>> include/linux/virtio.h |2 +-
>> include/linux/virtio_config.h |8
>> tools/virtio/linux/virtio.h|2 +-
>> tools/virtio/linux/virtio_config.h |2 +-
>> 12 files changed, 41 insertions(+), 34 deletions(-)
I guess you would need to update the feature bits in remoteproc as well?
e.g. something like:
diff --git a/include/linux/remoteproc.h b/include/linux/remoteproc.h
index faf3332..148a503 100644
--- a/include/linux/remoteproc.h
+++ b/include/linux/remoteproc.h
@@ -296,8 +296,8 @@ struct fw_rsc_vdev_vring {
struct fw_rsc_vdev {
u32 id;
u32 notifyid;
- u32 dfeatures;
- u32 gfeatures;
+ u64 dfeatures;
+ u64 gfeatures;
u32 config_len;
u8 status;
u8 num_of_vrings;
@@ -470,8 +470,8 @@ struct rproc_vdev {
struct rproc *rproc;
struct virtio_device vdev;
struct rproc_vring vring[RVDEV_NUM_VRINGS];
- unsigned long dfeatures;
- unsigned long gfeatures;
+ u64 dfeatures;
+ u64 gfeatures;
};
Thanks,
Sjur
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH 03/22] virtio_config: make transports implement accessors.
On Thu, Mar 21, 2013 at 9:29 AM, Rusty Russell wrote:
> All transports just pass through at the moment.
>
> Cc: Ohad Ben-Cohen
> Cc: Brian Swetland
> Cc: Cornelia Huck
> Cc: Pawel Moll
> Cc: Christian Borntraeger
> Signed-off-by: Rusty Russell
> ---
> drivers/lguest/lguest_device.c | 79
> ++--
> drivers/net/caif/caif_virtio.c |2 +-
> drivers/s390/kvm/kvm_virtio.c | 78 +--
> drivers/s390/kvm/virtio_ccw.c | 39 +++-
> drivers/virtio/virtio_mmio.c | 35 +-
> drivers/virtio/virtio_pci.c| 39 +---
> include/linux/virtio_config.h | 70 +--
> 7 files changed, 283 insertions(+), 59 deletions(-)
>
> --- a/drivers/virtio/virtio_pci.c
> +++ b/drivers/virtio/virtio_pci.c
> @@ -127,7 +127,7 @@ static void vp_finalize_features(struct virtio_device
> *vdev)
> iowrite32(vdev->features[0],
> vp_dev->ioaddr+VIRTIO_PCI_GUEST_FEATURES);
> }
>
> -/* virtio config->get() implementation */
> +/* Device config access: we use guest endian, as per spec. */
> static void vp_get(struct virtio_device *vdev, unsigned offset,
>void *buf, unsigned len)
> {
> @@ -141,8 +141,19 @@ static void vp_get(struct virtio_device *vdev, unsigned
> offset,
> ptr[i] = ioread8(ioaddr + i);
> }
>
> -/* the config->set() implementation. it's symmetric to the config->get()
> - * implementation */
> +#define VP_GETx(bits) \
> +static u##bits vp_get##bits(struct virtio_device *vdev, unsigned int offset)
> \
> +{ \
> + u##bits v; \
> + vp_get(vdev, offset, &v, sizeof(v));\
> + return v; \
> +}
> +
> +VP_GETx(8)
> +VP_GETx(16)
> +VP_GETx(32)
> +VP_GETx(64)
> +
> static void vp_set(struct virtio_device *vdev, unsigned offset,
>const void *buf, unsigned len)
> {
> @@ -156,6 +167,18 @@ static void vp_set(struct virtio_device *vdev, unsigned
> offset,
> iowrite8(ptr[i], ioaddr + i);
> }
>
> +#define VP_SETx(bits) \
> +static void vp_set##bits(struct virtio_device *vdev, unsigned int offset, \
> +u##bits v) \
> +{ \
> + vp_set(vdev, offset, &v, sizeof(v));\
> +}
> +
> +VP_SETx(8)
> +VP_SETx(16)
> +VP_SETx(32)
> +VP_SETx(64)
> +
> /* config->{get,set}_status() implementations */
> static u8 vp_get_status(struct virtio_device *vdev)
> {
> @@ -653,8 +676,14 @@ static int vp_set_vq_affinity(struct virtqueue *vq, int
> cpu)
> }
>
> static const struct virtio_config_ops virtio_pci_config_ops = {
> - .get= vp_get,
> - .set= vp_set,
> + .get8 = vp_get8,
> + .set8 = vp_set8,
> + .get16 = vp_get16,
> + .set16 = vp_set16,
> + .get32 = vp_get32,
> + .set32 = vp_set32,
> + .get64 = vp_get64,
> + .set64 = vp_set64,
> .get_status = vp_get_status,
> .set_status = vp_set_status,
> .reset = vp_reset,
Would it be possible to make this simpler and less verbose somehow?
At least three virtio devices: virtio_pci_legacy.c, virtio_mmio.c and
soon remoteproc_virtio.c will duplicate variants of the code above.
What if set8/get8 was mandatory, and the 16,32,64 variants where optional,
and then virtio_creadX() virtio_cwriteX did the magic to make things work?
Regards,
Sjur
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCHv2] virtio: Introduce vringh wrappers in virtio_config
On Mon, Mar 18, 2013 at 12:29 AM, Ohad Ben-Cohen wrote: > Hi Sjur, > > Sorry for the delay - just got back from you know where :) > > On Wed, Mar 13, 2013 at 12:12 PM, Sjur Brændeland wrote: >>> From: Sjur Brændeland >>> >>> Add wrappers for the host vrings to support loose >>> coupling between the virtio device and driver. >>> >>> A new struct vringh_config_ops with the functions >>> find_vrhs() and del_vrhs() is added to the virtio_device >>> struct. This enables virtio drivers to manage virtio >>> host rings without detailed knowledge of how the >>> vrings are created and deleted. >>> >>> The function vringh_notify() is added so vringh clients >>> can notify the other side that buffers are added to the >>> used-ring. >>> >>> Cc: Ohad Ben-Cohen >>> Cc: Rusty Russell >>> Signed-off-by: Sjur Brændeland >> >> As you have been pushing for this wrapper, could you please >> add your Ack or Reviewed tag if you are happy with this patch? > > I am happy with this patch: the caif virtio driver is now decoupled > from remoteproc which I think is great. Thanks for doing it! > > To provide a Reviewed-by tag though I need to review it from a virtio > point of view, which I can try to do after I'll finish going through > my backlog of pending patches. Let me finish that pile of patches I > have, and if you or Rusty will still want me to provide a Reviewed-by > tag here I'll do it. Hi Rusty, Do you have what you need to apply this patch? Please, let me know if any further changes are needed. The informal ack above from Ohad is good enough for me :-) Thanks, Sjur ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCHv2] virtio: Introduce vringh wrappers in virtio_config
Hi Ohad, On Fri, Mar 8, 2013 at 11:08 AM, wrote: > From: Sjur Brændeland > > Add wrappers for the host vrings to support loose > coupling between the virtio device and driver. > > A new struct vringh_config_ops with the functions > find_vrhs() and del_vrhs() is added to the virtio_device > struct. This enables virtio drivers to manage virtio > host rings without detailed knowledge of how the > vrings are created and deleted. > > The function vringh_notify() is added so vringh clients > can notify the other side that buffers are added to the > used-ring. > > Cc: Ohad Ben-Cohen > Cc: Rusty Russell > Signed-off-by: Sjur Brændeland As you have been pushing for this wrapper, could you please add your Ack or Reviewed tag if you are happy with this patch? Thanks, Sjur ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH] Revert "virtio_console: Initialize guest_connected=true for rproc_serial"
On Tue, Mar 12, 2013 at 8:34 AM, Amit Shah wrote: > On (Mon) 11 Mar 2013 [16:15:00], sjur.brandel...@stericsson.com wrote: >> From: Sjur Brćndeland >> >> This reverts commit 8078db789a92b10ff6e2d713231b5367e014c53b. >> >> The reverted patch caused opening of ports to fail for rproc_serial. >> In probe guest_connected was set to true, but port_fops_open() >> fails with -EMFILE if guest_connected already is true. > > OK, I missed that. Can you add a comment near the 2nd hunk mentioning > this? Ok, I ended up rewriting the whole comment here in my attempt to "mention this". Perhaps it's a bit over the top to write a short essay to explain two code lines, but anyway here it is. Let me know what you think: /* * Normally the port should not accept data when the port is * closed. For generic serial ports, the host won't (shouldn't) * send data till the guest is connected. But this condition * can be reached when a console port is not yet connected (no * tty is spawned) and the other side sends out data over the * vring, or when a remote devices start sending data before * the ports are opened. * * A generic serial port will discard data if not connected, * while console ports and rproc-serial ports accepts data at * any time. rproc-serial is initiated with guest_connect = false * because port_fops_open expects this. Console ports are * hooked up with an HVC console and is initialized with * guest_connected = true. */ Regards, Sjur ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Opposite return values in vringh_notify_enable_kern() and virtqueue_enable_cb().
Hi Rusty, The two similar functions in vringh and virtqueue for turning on interrupts has opposite return values if there are buffers available in the ring. I think it would be better if these two functions aligned the use of return values. Maybe it's just me, but I got the logic for re-scheduling NAPI wrong due to this. /** * vringh_notify_enable_kern - we want to know if something changes. * @vrh: the vring. * * This always enables notifications, but returns true if there are * now more buffers available in the vring. */ bool vringh_notify_enable_kern(struct vringh *vrh) /** * virtqueue_enable_cb - restart callbacks after disable_cb. * @vq: the struct virtqueue we're talking about. * * This re-enables callbacks; it returns "false" if there are pending * buffers in the queue, */ bool virtqueue_enable_cb(struct virtqueue *_vq) Regards. Sjur ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH vringh] virtio: Introduce vringh wrappers in virtio_config
On Wed, Mar 6, 2013 at 5:42 AM, Rusty Russell wrote:
...
>> @@ -70,6 +80,9 @@ struct virtio_config_ops {
>> void (*finalize_features)(struct virtio_device *vdev);
>> const char *(*bus_name)(struct virtio_device *vdev);
>> int (*set_vq_affinity)(struct virtqueue *vq, int cpu);
>> + int (*find_vrhs)(struct virtio_device *vdev, unsigned nhvrs,
>> + struct vringh *vrhs[], vrh_callback_t *callbacks[]);
>> + void (*del_vrhs)(struct virtio_device *vdev);
>> };
>>
>> /* If driver didn't advertise the feature, it will never appear. */
>
> It's weird that you conflate the host and guest ring sides in rpmsg, but
> that might make sense if they're really bound together.
The caif_virtio driver is using both host and guest ring sides, host side
rings in RX direction and guest side rings in TX direction. The reason
behind this is to enable zero-copy on the producer sides.
(For details see recent discussion with Ohad "Wrappers for vringh"
or the initial blurb when first introducing caif virtio:
https://lkml.org/lkml/2012/10/31/677)
> ... However, in
> general they are not: it's normal to be a guest or host, not both.
>
> This implies that you need a struct vringh_config, to put this in.
OK, should we move the definition of struct vringh_config into vringh.h
then, and add a vringh_config field in struct virtio_device?
>> diff --git a/include/linux/vringh.h b/include/linux/vringh.h
>> index ab41185..8156f51 100644
>> --- a/include/linux/vringh.h
>> +++ b/include/linux/vringh.h
>> @@ -50,6 +50,12 @@ struct vringh {
>>
>> /* The vring (note: it may contain user pointers!) */
>> struct vring vring;
>> +
>> + /* The function to call when buffers are available */
>> + void (*notify)(struct vringh *);
>> +
>> + /* A pointer for the vringh clients to use. */
>> + void *priv;
>> };
>
> Since the caller allocates the vringh, can it not use container_of()
> instead of a priv pointer?
Sure, no problem. I just need to add a new struct in remoteproc containing
both the vringh and a pointer to the internal ring data.
Regards,
Sjur
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH vringh] virtio: Introduce vringh wrappers in virtio_config
On Wed, Mar 6, 2013 at 1:37 PM, Sjur Brændeland wrote: > On Wed, Mar 6, 2013 at 1:16 PM, Ohad Ben-Cohen wrote: >> On Wed, Mar 6, 2013 at 12:50 PM, Sjur Brændeland wrote: >>> The caif_virtio driver is using both host and guest ring sides, host side >>> rings in RX direction and guest side rings in TX direction. The reason >>> behind this is to enable zero-copy on the producer sides. >>> (For details see recent discussion with Ohad "Wrappers for vringh" >> >> FWIW, I'm not convinced that host side rings are necessary for >> zero-copy - you could always just pass pointers to your huge data >> inside buffers posted by guest side rings. When I read this again I realized that I missed your point regarding passing pointers inside buffers completely. My rambling below argued against using the buffers provided in the available ring as a replacement of the modem internal allocator. Sorry for the confusion. I haven't really spent time thinking through the implication of the alternative of passing buffer pointers inside buffers posted by guest side rings. But it seems to me that there would be some issues related to returning consumed buffers back if you use the rings in this way. Thanks, Sjur > Well, to do that for CAIF you would have to integrate virtio buffer handling > deep into the lower layer of the modems 3GPP stack so that the 3GPP stack > could take buffers from the available ring. > > From the outside point of view, perhaps this sounds doable, but in reality you > wouldn't get any Network Signalling engineers implementing the 3GPP stack > to buy into that. > > Another issue is that our modem also support other interface technologies > such as USB and HSI. It simply doesn't make sense to enforce virtio rings > internally if virtio isn't otherwise used on the modem. > > Without host side rings we simply wouldn't be able to implement zero-copy > in the RX direction for the STE-modem. > > Regards, > Sjur > >> >> That said, I do believe that mixing guest side and host side rings may >> simplify the overall solution in some cases, especially in more >> complex multi core scenarios (where several cores all communicate with >> each other). ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH vringh] virtio: Introduce vringh wrappers in virtio_config
On Wed, Mar 6, 2013 at 1:16 PM, Ohad Ben-Cohen wrote: > On Wed, Mar 6, 2013 at 12:50 PM, Sjur Brændeland wrote: >> The caif_virtio driver is using both host and guest ring sides, host side >> rings in RX direction and guest side rings in TX direction. The reason >> behind this is to enable zero-copy on the producer sides. >> (For details see recent discussion with Ohad "Wrappers for vringh" > > FWIW, I'm not convinced that host side rings are necessary for > zero-copy - you could always just pass pointers to your huge data > inside buffers posted by guest side rings. Well, to do that for CAIF you would have to integrate virtio buffer handling deep into the lower layer of the modems 3GPP stack so that the 3GPP stack could take buffers from the available ring. From the outside point of view, perhaps this sounds doable, but in reality you wouldn't get any Network Signalling engineers implementing the 3GPP stack to buy into that. Another issue is that our modem also support other interface technologies such as USB and HSI. It simply doesn't make sense to enforce virtio rings internally if virtio isn't otherwise used on the modem. Without host side rings we simply wouldn't be able to implement zero-copy in the RX direction for the STE-modem. Regards, Sjur > > That said, I do believe that mixing guest side and host side rings may > simplify the overall solution in some cases, especially in more > complex multi core scenarios (where several cores all communicate with > each other). ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Wrappers for vringh (was Re: [PATCHv2 vringh 1/3] remoteproc: Add support for vringh (Host vrings))
On Fri, Feb 22, 2013 at 1:42 AM, Rusty Russell wrote:
> Ohad Ben-Cohen writes:
>> On Thu, Feb 21, 2013 at 8:37 AM, Rusty Russell wrote:
>> 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...
I'm not sure if the tight binding between vringh and remoteproc is
a big problem. I think the most obvious use-case for kernel vringh is
when they are instantiated from remoteproc.
But if we where to make wrappers, how about something like this?
diff --git a/include/linux/virtio_config.h b/include/linux/virtio_config.h
index 29b9104..ca257d8 100644
--- a/include/linux/virtio_config.h
+++ b/include/linux/virtio_config.h
@@ -53,0 +54,8 @@
+ * @find_vrh: find the host vrings and instantiate them
+ * vdev: the virtio_device
+ * nhvrs: the number of host vrings to find
+ * hvrs: on success, includes new host vrings
+ * callbacks: array of driver callbacks, for each host vring
+ * include a NULL entry for vqs that do not need a callback
+ * Returns 0 on success or error status
+ * @del_vrh: free the host vrings found by find_vrh().
@@ -55,0 +64 @@ typedef void vq_callback_t(struct virtqueue *);
+typedef void vrh_callback_t(struct virtio_device *, struct vringh *);
@@ -72,0 +82,4 @@ struct virtio_config_ops {
+ int (*find_vrh) (struct virtio_device *, unsigned nhvrs,
+struct vringh *vrhs[],
+vrh_callback_t *callbacks[]);
+ int (*del_vrhs)(struct virtio_device *);
diff --git a/include/linux/vringh.h b/include/linux/vringh.h
index 4c4c918..78aecc9 100644
--- a/include/linux/vringh.h
+++ b/include/linux/vringh.h
@@ -52,0 +53,3 @@ struct vringh {
+
+ /* The function to call when buffers are available */
+ void (*notify)(struct vringh *);
diff --git a/drivers/net/caif/caif_virtio.c b/drivers/net/caif/caif_virtio.c
index d4f339c..e657277 100644
--- a/drivers/net/caif/caif_virtio.c
+++ b/drivers/net/caif/caif_virtio.c
@@ -433,0 +434,2 @@ static int cfv_probe(struct virtio_device *vdev)
+ struct vringh *vrhs;
+ vrh_callback_t *vrh_cbs = cfv_recv;
@@ -446,2 +448,2 @@ static int cfv_probe(struct virtio_device *vdev)
- cfv->vr_rx = rproc_virtio_new_vringh(vdev, RX_RING_INDEX, cfv_recv);
- if (!cfv->vr_rx)
+ err = vdev->config->find_vrh(vdev, 1, &cfv->vr_rx, &vrh_cbs);
+ if (err)
@@ -504 +506 @@ static int cfv_probe(struct virtio_device *vdev)
- rproc_virtio_kick_vringh(vdev, RX_RING_INDEX);
+ cfv->vr_rx->notify(cfv->vr_rx);
@@ -522 +524 @@ static void cfv_remove(struct virtio_device *vdev)
- rproc_virtio_del_vringh(vdev, RX_RING_INDEX);
+ vdev->config->del_vrhs(cfv->vdev);
Thanks,
Sjur
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCHv2 vringh 1/3] remoteproc: Add support for vringh (Host vrings)
Hi Ohad, On Thu, Feb 21, 2013 at 6:55 PM, Ohad Ben-Cohen wrote: > On Thu, Feb 21, 2013 at 7:28 PM, Sjur Brændeland 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 ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCHv2 vringh 1/3] remoteproc: Add support for vringh (Host vrings)
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 ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCHv2 vringh 1/3] remoteproc: Add support for vringh (Host vrings)
On Wed, Feb 20, 2013 at 5:05 PM, Ohad Ben-Cohen wrote: > Hi Sjur, > > On Tue, Feb 12, 2013 at 1:49 PM, wrote: >> From: Sjur Brændeland >> >> 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 ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH vringh 2/2] caif_virtio: Introduce caif over virtio
> Sjur Brændeland writes: >> How about supporting struct vringh_kiov and struct kvec as well? >> I currently get the following complaints with my V2 patch-set: >> >> drivers/net/caif/caif_virtio.c:486:2: warning: passing argument 1 of >> ‘vringh_iov_init’ from incompatible pointer type [enabled by default] > > vringh_kiov_init()? Did I miss something else? I get a warning for my useof vringh_iov_cleanup() in addition to the one above. How about adding kiov variants of vringh_iov_cleanup() and vringh_iov_reset() as well? Thanks, Sjur ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH 1/6] virtio_host: host-side implementation of virtio rings.
Hi Rusty, On Wed, Feb 13, 2013 at 11:25 AM, Rusty Russell wrote: > Sjur Brændeland writes: >> On Mon, Feb 4, 2013 at 10:44 PM, Rusty Russell wrote: >>> Sjur Brændeland writes: >> ... >>>> I guess my vringh related stuff should go into your tree together with your >>>> vringh patches... >>>> Would you be willing to take this via your tree, provided that I get acks >>>> from the right people? >>> >>> Yes please. Now I have set up a test rig for vhost (unfortunately not >>> with 10Ge) I can make progress on vhost adaption. >>> >> >> I have one question, are you planning to include vringh in your >> virtio-next branch >> for the 3.9 merge window or some time later? > > If you're eager with the CAIF stuff, I'll merge it for this merge window > (we may still rework stuff, but I don't want to block you). > > Otherwise, it'll probably wait. Yes, I would prefer to get CAIF-virtio merged now unless it creates a a lot of trouble for you. But I'm not sure I'll manage to get ack from Ohad in time though. He said he would be busy for the next week. I need an ack from David Miller as well, I'll resend the patch to him if you decide to go ahead and merge this. Thanks, Sjur ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH vringh 2/2] caif_virtio: Introduce caif over virtio
Hi Rusty,
On Wed, Feb 13, 2013 at 11:16 AM, Rusty Russell wrote:
> Sjur BRENDELAND writes:
>>> > +static inline void ctx_prep_iov(struct cfv_napi_context *ctx)
>>> > +{
>>> > + if (ctx->riov.allocated) {
>>> > + kfree(ctx->riov.iov);
>>> > + ctx->riov.iov = NULL;
>>> > + ctx->riov.allocated = false;
>>> > + }
>>> > + ctx->riov.iov = NULL;
>>> > + ctx->riov.i = 0;
>>> > + ctx->riov.max = 0;
>>> > +}
>>>
>>> Hmm, we should probably make sure you don't have to do this: that if
>>> allocated once, you can simply reuse it by setting i = 0.
>>
>> Yes, I had problems getting the alloc/free of iov right. This is
>> perhaps the least intuitively part of the API. I maybe it's just me, but
>> I think some more helper functions and support from vringh in this
>> area would be great.
>
> Yes, I've neatened my git tree, an in particular added a commit which
> covers this explicitly, and one for the -EPROTO when we get unexpected
> r/w bufs. I've appended them below.
Great, thanks. This helps a lot. I picked up your patch-sett yesterday
so my V2 patch-set is using this already.
...
> +static inline void vringh_iov_init(struct vringh_iov *iov,
> + struct iovec *iovec, unsigned num)
> +{
> + iov->used = iov->i = 0;
> + iov->off = 0;
> + iov->max_num = num;
> + iov->iov = iovec;
> +}
How about supporting struct vringh_kiov and struct kvec as well?
I currently get the following complaints with my V2 patch-set:
drivers/net/caif/caif_virtio.c:486:2: warning: passing argument 1 of
‘vringh_iov_init’ from incompatible pointer type [enabled by default]
...
Thanks,
Sjur
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH 1/6] virtio_host: host-side implementation of virtio rings.
Hi Rusty, On Mon, Feb 4, 2013 at 10:44 PM, Rusty Russell wrote: > Sjur Brændeland writes: ... >> I guess my vringh related stuff should go into your tree together with your >> vringh patches... >> Would you be willing to take this via your tree, provided that I get acks >> from the right people? > > Yes please. Now I have set up a test rig for vhost (unfortunately not > with 10Ge) I can make progress on vhost adaption. > I have one question, are you planning to include vringh in your virtio-next branch for the 3.9 merge window or some time later? Regards, Sjur ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH] virtio_console: Let unconnected rproc device receive data.
On Tue, Feb 12, 2013 at 10:42 AM, Michael S. Tsirkin wrote: ... > > > > How about setting port->guest_connected = true in the init routines > > > > instead? Keeps this code path cleaner. > > > > > > Agree, I'll respin this patch. > > > > Hm, Rusty has already picked this up. > > I don't see it in virtio-next. Do you? Yes I do, see: http://git.kernel.org/?p=linux/kernel/git/rusty/linux.git;a=commitdiff;h=1f8051876a194d7f7fe7834d9853f240d6b4b9ab;hp=226364766f936d249e408de03821468c1bf11dda Regards, Sjur ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH] virtio_console: Let unconnected rproc device receive data.
Hi Amit, > > @@ -1763,8 +1763,11 @@ static void in_intr(struct virtqueue *vq) > > > > * tty is spawned) and the host sends out data to console > > > * ports. For generic serial ports, the host won't > > > * (shouldn't) send data till the guest is connected. > > > +* However a remote device might send data before the port is > > > +* connected. So don't remove data from a rproc_serial device. > > > */ > > > - if (!port->guest_connected) > > > + > > > + if (!port->guest_connected && !is_rproc_serial(port->portdev- > > >vdev)) > > > discard_port_data(port); > > > > How about setting port->guest_connected = true in the init routines > > instead? Keeps this code path cleaner. > > Agree, I'll respin this patch. Hm, Rusty has already picked this up. Do you still want me to do a respin, or should I leave it as is? Regards, Sjur ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH] virtio_console: Use virtio device index to generate port name
On Fri, Jan 25, 2013 at 10:20 AM, Amit Shah wrote: > On (Thu) 17 Jan 2013 [13:23:17], sjur.brandel...@stericsson.com wrote: > > From: Sjur Brændeland > > > > Use virtio device index for creating unique device port names. > > Current index allocation in virtio is based on a monotonically > > increasing variable "index". A better handling of this is to > > use device index which is allocated by ida. > > This should be fine. I'm wondering, though, if there's something else > you need from the naming scheme (esp. considering the next patch)? > This current patch solves the most critical issue - that the device name changes for every modem reboot. This is a show-stopper for me. So it would be great if this patch could go upstream. The other problem that device name is non-deterministic is currently only a theoretical problem as we have only one remoteproc device. Regards, Sjur ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH 1/6] virtio_host: host-side implementation of virtio rings.
Hi Rusty, On Thu, Jan 17, 2013 at 11:29 AM, Rusty Russell wrote: > Getting use of virtio rings correct is tricky, and a recent patch saw > an implementation of in-kernel rings (as separate from userspace). > > This patch attempts to abstract the business of dealing with the > virtio ring layout from the access (userspace or direct); to do this, > we use function pointers, which gcc inlines correctly. I have been using your patches for a while in my test setup without any issues with vringh. The only thing I'm missing is export of symbols. My current caif_virtio driver expects vringh to be a module exporting symbols. Is this something you are planning to add? I guess my vringh related stuff should go into your tree together with your vringh patches... Would you be willing to take this via your tree, provided that I get acks from the right people? Regards, Sjur ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [RFC] virtio_console: Add DRIVER and INTERFACE to uevent.
Hi Greg, On Tue, Jan 22, 2013 at 5:16 PM, Greg KH wrote: > On Tue, Jan 22, 2013 at 09:55:20AM +1030, Rusty Russell wrote: >> sjur.brandel...@stericsson.com writes: >> >> > From: Sjur Brændeland >> > >> > Add information so rproc-serial can be easily recogniced >> > from user space. Add the following information to uevent: >> > DRIVER=virtio_console|virtio_rproc_serial >> > INTERFACE=grand-parent/parent/name >> > >> > Signed-off-by: Sjur Brændeland >> > --- >> > Hi, >> > >> > I need some way to identify the major/minor number for >> > the rproc-serial device, given the udev event. >> > Review comments are welcomed. >> > >> > Thanks, >> > Sjur >> >> Hmm, I send all uevent questions to Greg KH (CC'd). > > The "INTERFACE" uevent variable is only for USB devices, so for anything > else to be sending userspace that variable, would confuse things > greatly. > > Sjur, what exactly are you trying to do here? What do you want > userspace to know, and what should it do with that information? I'm trying to help user-space make sense of the port-name generated by virtio_console. I'm using remoteproc and virtio_console for talking to a remote device (modem). At startup the port-device will be named vportp0. If we get a reboot of the remote device, the device name will change to vportp0. It is difficult for applications to guess the X or Y values in order to get hold of the right file after a reboot. If we at some point get more than one remote processor using virtio_console the port name may not even be deterministic. (X is currently just a monotonic counter). The current device path is: /sys/devices/platform/ste-modem/remoteproc0/virtio0/virtio-ports/vport0p0 $cat uevent MAJOR=254 MINOR=0 DEVNAME=vport0p0 So what I'm trying to do is to add information to udev events in order to be able create good device names and be able identify the remoteproc device. I should probably also mention that we're currently running this on Android. The initial stab at this was to add information to the udev event. Another approach could perhaps be to change the way port-names are generated by virtio_console... Thanks, Sjur ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH 1/6] virtio_host: host-side implementation of virtio rings.
On Mon, Jan 21, 2013 at 3:36 AM, Rusty Russell wrote: > Sjur Brændeland writes: >> On Thu, Jan 17, 2013 at 12:23 PM, Michael S. Tsirkin wrote: >>> in otgher words, we might need to split a single desc to multiple >>> iov entries. >> >> Splitting descriptors doesn't work for me. As described earlier, I >> receive one IP-frame for each descriptor. So I need to keep >> the original boundaries. > > Yes, but the _kern helpers don't do range checking or offsetting at the > moment, so they'll be preserved. > > If you want to do range checking, I can add that... I don't think I need that. Initially I can just check if the received address is readable. Regards, Sjur ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH 1/6] virtio_host: host-side implementation of virtio rings.
On Thu, Jan 17, 2013 at 12:23 PM, Michael S. Tsirkin wrote:
> On Thu, Jan 17, 2013 at 08:59:38PM +1030, Rusty Russell wrote:
...
>> +static inline int
>> +__vringh_iov(struct vringh *vrh, u16 i,
>> + struct vringh_iov *riov,
>> + struct vringh_iov *wiov,
>> + bool (*getrange)(u64 addr, struct vringh_range *r),
>> + gfp_t gfp,
>> + int (*getdesc)(struct vring_desc *dst, const struct vring_desc
>> *s))
>> +{
>> + int err, count = 0, up_next, desc_max;
>> + struct vring_desc desc, *descs;
>> + struct vringh_range range = { -1ULL, 0 };
>> +
>> + /* We start traversing vring's descriptor table. */
>> + descs = vrh->vring.desc;
>> + desc_max = vrh->vring.num;
>> + up_next = -1;
>> +
>> + riov->i = wiov->i = 0;
>> + for (;;) {
>> + void *addr;
>> + struct vringh_iov *iov;
>> +
>> + err = getdesc(&desc, &descs[i]);
>> + if (unlikely(err))
>> + goto fail;
>> +
>> + /* Make sure it's OK, and get offset. */
>> + if (!check_range(desc.addr, desc.len, &range, getrange)) {
>> + err = -EINVAL;
>> + goto fail;
>> + }
>> + addr = (void *)(long)desc.addr + range.offset;
>
> Should probably be (void *)(long)(desc.addr + range.offset).
> Otherwise we risk signed integer overflow.
>
>> +
>> + if (unlikely(desc.flags & VRING_DESC_F_INDIRECT)) {
>> + err = move_to_indirect(&up_next, &i, addr, &desc,
>> +&descs, &desc_max);
>> + if (err)
>> + goto fail;
>> + continue;
>> + }
>> +
>> + if (count++ == vrh->vring.num) {
>> + vringh_bad("Descriptor loop in %p", descs);
>> + err = -ELOOP;
>> + goto fail;
>> + }
>> +
>> + if (desc.flags & VRING_DESC_F_WRITE)
>> + iov = wiov;
>> + else {
>> + iov = riov;
>> + if (unlikely(wiov->i)) {
>> + vringh_bad("Readable desc %p after writable",
>> +&descs[i]);
>> + err = -EINVAL;
>> + goto fail;
>> + }
>> + }
>> +
>> + if (unlikely(iov->i == iov->max)) {
>> + err = resize_iovec(iov, gfp);
>> + if (err)
>> + goto fail;
>> + }
>> +
>> + iov->iov[iov->i].iov_base = (__force __user void *)addr;
>> + iov->iov[iov->i].iov_len = desc.len;
>
> The following comment from the previous version still applies:
> > This looks like it won't do the right thing if desc.len spans
> multiple
> > ranges. I don't know if this happens in practice but this is
> something
> > vhost supports ATM.
> in otgher words, we might need to split a single desc to multiple
> iov entries.
Splitting descriptors doesn't work for me. As described earlier, I
receive one IP-frame for each descriptor. So I need to keep
the original boundaries.
Regards,
Sjur
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [RFCv2 00/12] Introduce host-side virtio queue and CAIF Virtio.
On Fri, Jan 11, 2013 at 7:37 AM, Rusty Russell wrote: >virtio_host: host-side implementation of virtio rings (untested!) > >Getting use of virtio rings correct is tricky, and a recent patch saw >an implementation of in-kernel rings (as separate from userspace). 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(unsigned int index, unsigned int num, unsigned int vring_align, struct virtio_device *vdev, bool weak_barriers, void *pages, void (*notify)(struct virtqueue *), void (*callback)(struct virtqueue *), const char *name); int virtqueueh_get_iov(struct virtqueue *vqh, struct vringh_iov *riov, struct vringh_iov *wiov, gfp_t gfp); int virtqueueh_add_iov(struct virtqueue *vqh, struct vringh_iov *riov, struct vringh_iov *wiov); I guess implementation of the host-virtqueue should stay in drivers/virtio? Regards, Sjur ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [RFCv2 00/12] Introduce host-side virtio queue and CAIF Virtio.
On Fri, Jan 11, 2013 at 12:35 AM, Rusty Russell wrote: > Hi Sjur! > > OK, the Internet was no help here, how do you pronounce Sjur? > I'm guessing "shoor" rhyming with tour until I know better. Thank you for asking! This is pretty close yes. I usually tell people to pronounce it like "sure" for simplicity. But Google translate has a perfect Norwegian voice pronouncing my name: http://translate.google.com/#auto/no/sjur (Press the speaker button) While at the subject: Norwegian names can be a laugh: I have people named "Odd" and "Even" in my family, and I once had a friend named "Aashold". ... >> I would prefer not to split into pages at this point, but rather provide an >> iterator or the original list found in the descriptor to the client. ... > In other words, boundaries matter? > > While the sg-in-place hack is close to optimal for TCM_VHOST, neither > net not you can use it directly. I'll switch to an iovec (with a > similar use-caller-supplied-if-it-fits trick); they're smaller anyway. Great, I think iovec will be a good fit for my CAIF driver. Thanks, Sjur ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [RFCv2 00/12] Introduce host-side virtio queue and CAIF Virtio.
Hi Rusty,
On Thu, Jan 10, 2013 at 11:30 AM, Rusty Russell wrote:
...
>I now have some lightly-tested code (via a userspace harness).
Great - thank you for looking into this. I will start integrating this
with my patches
when you send out a proper patch.
...
> diff --git a/drivers/virtio/Kconfig b/drivers/virtio/Kconfig
> index 8d5bddb..fd95d3e 100644
> --- a/drivers/virtio/Kconfig
> +++ b/drivers/virtio/Kconfig
> @@ -5,6 +5,12 @@ config VIRTIO
> bus, such as CONFIG_VIRTIO_PCI, CONFIG_VIRTIO_MMIO, CONFIG_LGUEST,
> CONFIG_RPMSG or CONFIG_S390_GUEST.
>
> +config VHOST
> + tristate
Inclusion of drivers/virtio from drivers/Makefile depends on VIRTIO.
So I guess VHOST should select VIRTIO to ensure that
drivers/virtio/virtio_host.c
is part of the build.
> + ---help---
> + This option is selected by any driver which needs to access
> + the host side of a virtio ring.
> +
...
> +/* Returns vring->num if empty, -ve on error. */
> +static inline int __vringh_get_head(const struct vringh *vrh,
> + int (*getu16)(u16 *val, const u16 *p),
> + u16 *last_avail_idx)
> +{
> + u16 avail_idx, i, head;
> + int err;
> +
> + err = getu16(&avail_idx, &vrh->vring.avail->idx);
> + if (err) {
> + vringh_bad("Failed to access avail idx at %p",
> + &vrh->vring.avail->idx);
> + return err;
> + }
> +
> + err = getu16(last_avail_idx, &vring_avail_event(&vrh->vring));
> + if (err) {
> + vringh_bad("Failed to access last avail idx at %p",
> + &vring_avail_event(&vrh->vring));
> + return err;
> + }
> +
> + if (*last_avail_idx == avail_idx)
> + return vrh->vring.num;
> +
> + /* Only get avail ring entries after they have been exposed by guest.
> */
> + smp_rmb();
We are accessing memory shared with a remote device (modem), so we probably
need mandatory barriers here, e.g. something like the virtio_rmb
defined in virtio_ring.c.
> +
> + i = *last_avail_idx & (vrh->vring.num - 1);
> +
> + err = getu16(&head, &vrh->vring.avail->ring[i]);
> + if (err) {
> + vringh_bad("Failed to read head: idx %d address %p",
> + *last_avail_idx, &vrh->vring.avail->ring[i]);
> + return err;
> + }
> +
> + if (head >= vrh->vring.num) {
> + vringh_bad("Guest says index %u > %u is available",
> + head, vrh->vring.num);
> + return -EINVAL;
> + }
> + return head;
> +}
...
> +static inline int
> +__vringh_sg(struct vringh_acc *acc,
> + struct vringh_sg *vsg,
> + unsigned max,
> + u16 write_flag,
> + gfp_t gfp,
> + int (*getdesc)(struct vring_desc *dst, const struct vring_desc
> *s))
> +{
> + unsigned count = 0, num_descs = 0;
> + struct vringh_sg *orig_vsg = vsg;
> + int err;
> +
> + /* This sends end marker on sg[max-1], so we know when to chain. */
> + if (max)
> + sg_init_table(&vsg->sg, max);
> +
> + for (;;) {
> + /* Exhausted this descriptor? Read next. */
> + if (acc->desc.len == 0) {
> + if (!(acc->desc.flags & VRING_DESC_F_NEXT))
> + break;
> +
> + if (num_descs++ == acc->max) {
> + err = -ELOOP;
> + goto fail;
> + }
> +
> + if (acc->desc.next >= acc->max) {
> + vringh_bad("Chained index %u > %u",
> + acc->desc.next, acc->max);
> + err = -EINVAL;
> + goto fail;
> + }
> +
> + acc->idx = acc->desc.next;
> + err = getdesc(&acc->desc, acc->start + acc->idx);
> + if (unlikely(err))
> + goto fail;
> + }
> +
> + if (unlikely(!max)) {
> + vringh_bad("Unexpected %s descriptor",
> + write_flag ? "writable" : "readable");
> + return -EINVAL;
> + }
> +
> + /* No more readable/writable descriptors? */
> + if ((acc->desc.flags & VRING_DESC_F_WRITE) != write_flag) {
> + /* We should not have readable after writable */
> + if (write_flag) {
> + vringh_bad("Readable desc %p after writable",
> + acc->start + acc->idx);
> + err = -EINVAL;
> + goto fail;
> +
Re: [RFCv2 00/12] Introduce host-side virtio queue and CAIF Virtio.
On Fri, Dec 21, 2012 at 7:11 AM, Rusty Russell wrote: > "Michael S. Tsirkin" writes: > >> On Wed, Dec 05, 2012 at 03:36:58PM +0100, Sjur Brændeland wrote: >>> Feedback on this patch-set is appreciated, particularly on structure >>> and code-reuse between vhost.c and the host-side virtio-queue. >>> I'd also like some suggestions on how to handle the build configuration >>> better - currently there are some unnecessary build dependencies. >> >> Rusty seems to disagree but one of the concerns people have about vhost >> is security; so I value getting as much static checking as we can. This >> discards __user annotations so this doesn't work for me. > > Sometimes, when we generalize code, we lose some type safety. Callbacks > take void *, for example. And it happens *all the time* with const. We > don't create a set of parallel const-safe routines. > > Extracting common code where it can be shared provides better, not worse > security, because more people will read it. I've never audited the > vhost code, for example. > > We already have a 'struct vring', we should just use that. > > I meant to do more work on this, but I've run out of time :( I was > thinking of a drivers/virtio/virtio_ring.c, and in that we'd put the > wrappers for vhost_net/blk, and for CAIF, etc. We could at least have a > variant which did the __user etc thing, even if you have to pass in > getdesc(). > > getu16: get_user or assignment. > getdesc: copy (and check, translate) the descriptor. > getmem/putmem: memcpy or copy_to/from_user. > > (Completely untested, of course...) > > Thoughts? Any thoughts on this Michael? Regards, Sjur ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
[PATCHv9 2/2] virtio_console: Add support for remoteproc serial
Add a simple serial connection driver called
VIRTIO_ID_RPROC_SERIAL (11) for communicating with a
remote processor in an asymmetric multi-processing
configuration.
This implementation reuses the existing virtio_console
implementation, and adds support for DMA allocation
of data buffers and disables use of tty console and
the virtio control queue.
Signed-off-by: Sjur Brændeland
Acked-by: Amit Shah
---
drivers/char/virtio_console.c | 192 ++-
include/uapi/linux/virtio_ids.h |1 +
2 files changed, 170 insertions(+), 23 deletions(-)
diff --git a/drivers/char/virtio_console.c b/drivers/char/virtio_console.c
index 5482246..55a89a4 100644
--- a/drivers/char/virtio_console.c
+++ b/drivers/char/virtio_console.c
@@ -37,8 +37,12 @@
#include
#include
#include
+#include
+#include
#include "../tty/hvc/hvc_console.h"
+#define is_rproc_enabled IS_ENABLED(CONFIG_REMOTEPROC)
+
/*
* This is a global struct for storing common data for all the devices
* this driver handles.
@@ -112,6 +116,15 @@ struct port_buffer {
/* offset in the buf from which to consume data */
size_t offset;
+ /* DMA address of buffer */
+ dma_addr_t dma;
+
+ /* Device we got DMA memory from */
+ struct device *dev;
+
+ /* List of pending dma buffers to free */
+ struct list_head list;
+
/* If sgpages == 0 then buf is used */
unsigned int sgpages;
@@ -331,6 +344,11 @@ static bool is_console_port(struct port *port)
return false;
}
+static bool is_rproc_serial(const struct virtio_device *vdev)
+{
+ return is_rproc_enabled && vdev->id.device == VIRTIO_ID_RPROC_SERIAL;
+}
+
static inline bool use_multiport(struct ports_device *portdev)
{
/*
@@ -342,11 +360,13 @@ static inline bool use_multiport(struct ports_device
*portdev)
return portdev->vdev->features[0] & (1 << VIRTIO_CONSOLE_F_MULTIPORT);
}
-static void free_buf(struct port_buffer *buf)
+static DEFINE_SPINLOCK(dma_bufs_lock);
+static LIST_HEAD(pending_free_dma_bufs);
+
+static void free_buf(struct port_buffer *buf, bool can_sleep)
{
unsigned int i;
- kfree(buf->buf);
for (i = 0; i < buf->sgpages; i++) {
struct page *page = sg_page(&buf->sg[i]);
if (!page)
@@ -354,14 +374,57 @@ static void free_buf(struct port_buffer *buf)
put_page(page);
}
+ if (!buf->dev) {
+ kfree(buf->buf);
+ } else if (is_rproc_enabled) {
+ unsigned long flags;
+
+ /* dma_free_coherent requires interrupts to be enabled. */
+ if (!can_sleep) {
+ /* queue up dma-buffers to be freed later */
+ spin_lock_irqsave(&dma_bufs_lock, flags);
+ list_add_tail(&buf->list, &pending_free_dma_bufs);
+ spin_unlock_irqrestore(&dma_bufs_lock, flags);
+ return;
+ }
+ dma_free_coherent(buf->dev, buf->size, buf->buf, buf->dma);
+
+ /* Release device refcnt and allow it to be freed */
+ put_device(buf->dev);
+ }
+
kfree(buf);
}
+static void reclaim_dma_bufs(void)
+{
+ unsigned long flags;
+ struct port_buffer *buf, *tmp;
+ LIST_HEAD(tmp_list);
+
+ if (list_empty(&pending_free_dma_bufs))
+ return;
+
+ /* Create a copy of the pending_free_dma_bufs while holding the lock */
+ spin_lock_irqsave(&dma_bufs_lock, flags);
+ list_cut_position(&tmp_list, &pending_free_dma_bufs,
+ pending_free_dma_bufs.prev);
+ spin_unlock_irqrestore(&dma_bufs_lock, flags);
+
+ /* Release the dma buffers, without irqs enabled */
+ list_for_each_entry_safe(buf, tmp, &tmp_list, list) {
+ list_del(&buf->list);
+ free_buf(buf, true);
+ }
+}
+
static struct port_buffer *alloc_buf(struct virtqueue *vq, size_t buf_size,
int pages)
{
struct port_buffer *buf;
+ reclaim_dma_bufs();
+
/*
* Allocate buffer and the sg list. The sg list array is allocated
* directly after the port_buffer struct.
@@ -373,11 +436,34 @@ static struct port_buffer *alloc_buf(struct virtqueue
*vq, size_t buf_size,
buf->sgpages = pages;
if (pages > 0) {
+ buf->dev = NULL;
buf->buf = NULL;
return buf;
}
- buf->buf = kmalloc(buf_size, GFP_KERNEL);
+ if (is_rproc_serial(vq->vdev)) {
+ /*
+* Allocate DMA memory from ancestor. When a virtio
+* device is created by remoteproc, the DMA memory is
+* associated
[PATCHv9 1/2] virtio_console: Merge struct buffer_token into struct port_buffer
Refactoring the splice functionality by unifying the approach for
sending scatter-lists and regular buffers. This simplifies
buffer handling and reduces code size. Splice will now allocate
a port_buffer and send_buf() and free_buf() can always be used
for any buffer.
Signed-off-by: Sjur Brændeland
Acked-by: Amit Shah
---
drivers/char/virtio_console.c | 129 +
1 files changed, 53 insertions(+), 76 deletions(-)
diff --git a/drivers/char/virtio_console.c b/drivers/char/virtio_console.c
index db244b5..5482246 100644
--- a/drivers/char/virtio_console.c
+++ b/drivers/char/virtio_console.c
@@ -111,6 +111,12 @@ struct port_buffer {
size_t len;
/* offset in the buf from which to consume data */
size_t offset;
+
+ /* If sgpages == 0 then buf is used */
+ unsigned int sgpages;
+
+ /* sg is used if spages > 0. sg must be the last in is struct */
+ struct scatterlist sg[0];
};
/*
@@ -338,17 +344,39 @@ static inline bool use_multiport(struct ports_device
*portdev)
static void free_buf(struct port_buffer *buf)
{
+ unsigned int i;
+
kfree(buf->buf);
+ for (i = 0; i < buf->sgpages; i++) {
+ struct page *page = sg_page(&buf->sg[i]);
+ if (!page)
+ break;
+ put_page(page);
+ }
+
kfree(buf);
}
-static struct port_buffer *alloc_buf(size_t buf_size)
+static struct port_buffer *alloc_buf(struct virtqueue *vq, size_t buf_size,
+int pages)
{
struct port_buffer *buf;
- buf = kmalloc(sizeof(*buf), GFP_KERNEL);
+ /*
+* Allocate buffer and the sg list. The sg list array is allocated
+* directly after the port_buffer struct.
+*/
+ buf = kmalloc(sizeof(*buf) + sizeof(struct scatterlist) * pages,
+ GFP_KERNEL);
if (!buf)
goto fail;
+
+ buf->sgpages = pages;
+ if (pages > 0) {
+ buf->buf = NULL;
+ return buf;
+ }
+
buf->buf = kmalloc(buf_size, GFP_KERNEL);
if (!buf->buf)
goto free_buf;
@@ -478,52 +506,26 @@ static ssize_t send_control_msg(struct port *port,
unsigned int event,
return 0;
}
-struct buffer_token {
- union {
- void *buf;
- struct scatterlist *sg;
- } u;
- /* If sgpages == 0 then buf is used, else sg is used */
- unsigned int sgpages;
-};
-
-static void reclaim_sg_pages(struct scatterlist *sg, unsigned int nrpages)
-{
- int i;
- struct page *page;
-
- for (i = 0; i < nrpages; i++) {
- page = sg_page(&sg[i]);
- if (!page)
- break;
- put_page(page);
- }
- kfree(sg);
-}
/* Callers must take the port->outvq_lock */
static void reclaim_consumed_buffers(struct port *port)
{
- struct buffer_token *tok;
+ struct port_buffer *buf;
unsigned int len;
if (!port->portdev) {
/* Device has been unplugged. vqs are already gone. */
return;
}
- while ((tok = virtqueue_get_buf(port->out_vq, &len))) {
- if (tok->sgpages)
- reclaim_sg_pages(tok->u.sg, tok->sgpages);
- else
- kfree(tok->u.buf);
- kfree(tok);
+ while ((buf = virtqueue_get_buf(port->out_vq, &len))) {
+ free_buf(buf);
port->outvq_full = false;
}
}
static ssize_t __send_to_port(struct port *port, struct scatterlist *sg,
int nents, size_t in_count,
- struct buffer_token *tok, bool nonblock)
+ void *data, bool nonblock)
{
struct virtqueue *out_vq;
int err;
@@ -536,7 +538,7 @@ static ssize_t __send_to_port(struct port *port, struct
scatterlist *sg,
reclaim_consumed_buffers(port);
- err = virtqueue_add_buf(out_vq, sg, nents, 0, tok, GFP_ATOMIC);
+ err = virtqueue_add_buf(out_vq, sg, nents, 0, data, GFP_ATOMIC);
/* Tell Host to go! */
virtqueue_kick(out_vq);
@@ -574,37 +576,6 @@ done:
return in_count;
}
-static ssize_t send_buf(struct port *port, void *in_buf, size_t in_count,
- bool nonblock)
-{
- struct scatterlist sg[1];
- struct buffer_token *tok;
-
- tok = kmalloc(sizeof(*tok), GFP_ATOMIC);
- if (!tok)
- return -ENOMEM;
- tok->sgpages = 0;
- tok->u.buf = in_buf;
-
- sg_init_one(sg, in_buf, in_count);
-
- return __send_to_port(port, sg, 1, in_count, tok, nonblock);
-}
-
-static ssize_t send_pages(struct port *port, struct scatterlist *sg, int nents,
- size_t in_count,
[PATCHv9 0/2] virtio_console: Add rproc_serial driver
This patch-set introduces a new virtio type "rproc_serial" for communicating with remote processors over shared memory. The driver depends on the the remoteproc framework. As preparation for introducing "rproc_serial" I've done a refactoring of the transmit buffer handling. NOTE: These two patches are identical to first two patches of the V8 patch-set, but are rebased to virtio-next. Regards, Sjur Sjur Brændeland (2): virtio_console: Merge struct buffer_token into struct port_buffer virtio_console: Add support for remoteproc serial drivers/char/virtio_console.c | 317 +++ include/uapi/linux/virtio_ids.h |1 + 2 files changed, 221 insertions(+), 97 deletions(-) -- 1.7.5.4 ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
[PATCH] Introduce rproc serial device specification.
Signed-off-by: Sjur Brændeland --- >> Please pick the virtio spec from >> >> https://github.com/rustyrussell/virtio-spec >> >> and update the spec with info for remote-proc. > >Hi Sjur, > >I didn't hear back from you on this.. > Hi Amit Sorry for the delay. I've had this ready for a while waiting for go-ahead from our legal dept, and then it got stuck in my backlog. Anyway, here is a stab at describing the rproc serial device. Feedback is welcomed. Thanks, Sjur virtio-spec.lyx | 58 +++ 1 file changed, 58 insertions(+) diff --git a/virtio-spec.lyx b/virtio-spec.lyx index 83f2771..bbb5c75 100644 --- a/virtio-spec.lyx +++ b/virtio-spec.lyx @@ -9079,6 +9079,64 @@ For MMC devices (inquiry type 5) there would be some overlap between this \end_deeper \begin_layout Chapter* +Appendix J: Rproc Serial Device +\end_layout + +\begin_layout Standard +The Remoteproc Serial Device device is used when communicating with a remote + processor. + The remote processor may be a modem communicating over a shared memory + interface, or a CPU in an asymmetric multi-processing (AMP) configuration. + The Remoteproc Serial Device provides a simple device for data IO. + A device has one port and a single pair of virtqueues for input and output. + Empty buffers are placed in one virtqueue for receiving packets, and outgoing + packets are enqueued into another for transmission. +\end_layout + +\begin_layout Section* +Configuration +\end_layout + +\begin_layout Description +Subsystem +\begin_inset space ~ +\end_inset + +Device +\begin_inset space ~ +\end_inset + +ID 11 +\end_layout + +\begin_deeper +\begin_layout Description +Virtqueues 0:receiveq(port0). + 1:transmitq(port0), +\end_layout + +\begin_layout Section* +Device Initialization +\end_layout + +\begin_layout Enumerate +The receiveq for each port is populated with one or more receive buffers. +\end_layout + +\begin_layout Section* +Device Operation +\end_layout + +\begin_layout Enumerate +For output, a buffer containing the characters is placed in the port's transmitq. +\end_layout + +\begin_layout Enumerate +Inbound messages are placed in the receiveq and signalled by an interrupt. +\end_layout + +\end_deeper +\begin_layout Chapter* Appendix X: virtio-mmio \end_layout -- 1.7.9.5 ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
[RFCv2 12/12] caif_virtio: Introduce caif over virtio
From: Vikram ARV
caif_virtio is using the remoteproc and virtio framework
for communicating with the modem. The CAIF link layer
device is registered as a network device.
CAIF over virtio uses the virtio rings in both "directions",
and request a reversed virtio queue in the RX direction.
Signed-off-by: Vikram ARV
Signed-off-by: Sjur Brændeland
---
drivers/net/caif/Kconfig|8 +
drivers/net/caif/Makefile |3 +
drivers/net/caif/caif_virtio.c | 482 +++
include/linux/virtio_caif.h | 24 ++
include/uapi/linux/virtio_ids.h |1 +
5 files changed, 518 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..a1279d5 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 VIRTIO
+ default m
+ ---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 000..94efd21
--- /dev/null
+++ b/drivers/net/caif/caif_virtio.c
@@ -0,0 +1,482 @@
+/*
+ * Copyright (C) ST-Ericsson AB 2012
+ * Contact: Sjur Brendeland / sjur.brandel...@stericsson.com
+ * Authors: Vicram Arv / vikram@stericsson.com,
+ * Dmitry Tarnyagin / dmitry.tarnya...@stericsson.com
+ * Sjur Brendeland / sjur.brandel...@stericsson.com
+ * License terms: GNU General Public License (GPL) version 2
+ */
+
+#define pr_fmt(fmt) KBUILD_MODNAME ":%s/%d" fmt, __func__, __LINE__
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+
+MODULE_LICENSE("GPL v2");
+MODULE_AUTHOR("Vicram Arv ");
+MODULE_DESCRIPTION("Virtio CAIF Driver");
+
+/*
+ * 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 virtqueue *vq_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;
+ /* 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 0
+#define CFV_DEF_TAILROOM 0
+
+/* Require IP header to be 4-byte aligned. */
+#define IP_HDR_ALIGN 4
+
+/*
+ * virtqueue_next_desc - get next available or linked descriptor
+ * @_vq: the struct virtqueue we're talking about
+ * @desc: "current" descriptor.
+ * @head: on return it is filled by the descriptor index in case of
+ * available descriptor was returned, or -1 in case of linked
+ * descriptor.
+ *
+ * The function is to be used as an iterator through received descriptors.
+ */
+static struct vring_desc *virtqueue_next_desc(struct virtqueue *_vq,
+ struct vring_desc *desc,
+ int *head)
+{
+ struct vring_desc *next = virtqueue_next_linked_desc(_vq, desc);
+ BUG_ON(!_vq->reversed);
+
[RFCv2 11/12] remoteproc: Add support for host-virtqueues
Create a virtio-queue in reversed direction if requested
by the virtio device. This is done by calling the function
vring_new_host_virtqueue().
Signed-off-by: Sjur Brændeland
---
drivers/remoteproc/remoteproc_virtio.c | 15 ---
1 files changed, 12 insertions(+), 3 deletions(-)
diff --git a/drivers/remoteproc/remoteproc_virtio.c
b/drivers/remoteproc/remoteproc_virtio.c
index a825f67..5866b03 100644
--- a/drivers/remoteproc/remoteproc_virtio.c
+++ b/drivers/remoteproc/remoteproc_virtio.c
@@ -69,6 +69,7 @@ EXPORT_SYMBOL(rproc_vq_interrupt);
static struct virtqueue *rp_find_vq(struct virtio_device *vdev,
unsigned id,
+ bool revers,
void (*callback)(struct virtqueue *vq),
const char *name)
{
@@ -106,8 +107,14 @@ static struct virtqueue *rp_find_vq(struct virtio_device
*vdev,
* Create the new vq, and tell virtio we're not interested in
* the 'weak' smp barriers, since we're talking with a real device.
*/
- vq = vring_new_virtqueue(id, len, rvring->align, vdev, false, addr,
- rproc_virtio_notify, callback, name);
+ if (revers)
+ vq = vring_new_host_virtqueue(id, len, rvring->align, vdev,
+ false, addr, rproc_virtio_notify,
+ callback, name);
+ else
+ vq = vring_new_virtqueue(id, len, rvring->align, vdev, false,
+addr, rproc_virtio_notify, callback,
+name);
if (!vq) {
dev_err(dev, "vring_new_virtqueue %s failed\n", name);
rproc_free_vring(rvring);
@@ -145,9 +152,11 @@ static int rproc_virtio_find_vqs(struct virtio_device
*vdev, unsigned nvqs,
{
struct rproc *rproc = vdev_to_rproc(vdev);
int i, ret;
+ bool revers;
for (i = 0; i < nvqs; ++i) {
- vqs[i] = rp_find_vq(vdev, i, callbacks[i], names[i]);
+ revers = reversed ? reversed[i] : false;
+ vqs[i] = rp_find_vq(vdev, i, revers, callbacks[i], names[i]);
if (IS_ERR(vqs[i])) {
ret = PTR_ERR(vqs[i]);
goto error;
--
1.7.5.4
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
[RFCv2 10/12] virtio: Add argument reversed to function find_vqs()
Add argument 'reversed' to function find_vqs() in order to
allow virtio devices to request host-side virtio-queues.
The argument 'reversed' may be NULL if reversed
virtio-queues are not requested.
Signed-off-by: Sjur Brændeland
---
drivers/char/virtio_console.c |3 ++-
drivers/lguest/lguest_device.c |5 -
drivers/net/virtio_net.c |3 ++-
drivers/remoteproc/remoteproc_virtio.c |3 ++-
drivers/rpmsg/virtio_rpmsg_bus.c |2 +-
drivers/s390/kvm/kvm_virtio.c |5 -
drivers/scsi/virtio_scsi.c |2 +-
drivers/virtio/virtio_balloon.c|3 ++-
drivers/virtio/virtio_mmio.c |5 -
drivers/virtio/virtio_pci.c|3 ++-
include/linux/virtio_config.h |7 +--
11 files changed, 29 insertions(+), 12 deletions(-)
diff --git a/drivers/char/virtio_console.c b/drivers/char/virtio_console.c
index 4ad8aca..6c96ec7 100644
--- a/drivers/char/virtio_console.c
+++ b/drivers/char/virtio_console.c
@@ -1780,7 +1780,8 @@ static int init_vqs(struct ports_device *portdev)
/* Find the queues. */
err = portdev->vdev->config->find_vqs(portdev->vdev, nr_queues, vqs,
io_callbacks,
- (const char **)io_names);
+ (const char **)io_names,
+ NULL);
if (err)
goto free;
diff --git a/drivers/lguest/lguest_device.c b/drivers/lguest/lguest_device.c
index fc92ccb..724e084 100644
--- a/drivers/lguest/lguest_device.c
+++ b/drivers/lguest/lguest_device.c
@@ -369,7 +369,8 @@ static void lg_del_vqs(struct virtio_device *vdev)
static int lg_find_vqs(struct virtio_device *vdev, unsigned nvqs,
struct virtqueue *vqs[],
vq_callback_t *callbacks[],
- const char *names[])
+ const char *names[],
+ const bool reversed[])
{
struct lguest_device *ldev = to_lgdev(vdev);
int i;
@@ -379,6 +380,8 @@ static int lg_find_vqs(struct virtio_device *vdev, unsigned
nvqs,
return -ENOENT;
for (i = 0; i < nvqs; ++i) {
+ if (reversed || reversed[i])
+ goto error;
vqs[i] = lg_find_vq(vdev, i, callbacks[i], names[i]);
if (IS_ERR(vqs[i]))
goto error;
diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
index 6289891..0eb5b2b 100644
--- a/drivers/net/virtio_net.c
+++ b/drivers/net/virtio_net.c
@@ -1026,7 +1026,8 @@ static int init_vqs(struct virtnet_info *vi)
* and optionally control. */
nvqs = virtio_has_feature(vi->vdev, VIRTIO_NET_F_CTRL_VQ) ? 3 : 2;
- err = vi->vdev->config->find_vqs(vi->vdev, nvqs, vqs, callbacks, names);
+ err = vi->vdev->config->find_vqs(vi->vdev, nvqs, vqs, callbacks, names,
+NULL);
if (err)
return err;
diff --git a/drivers/remoteproc/remoteproc_virtio.c
b/drivers/remoteproc/remoteproc_virtio.c
index e7a4780..a825f67 100644
--- a/drivers/remoteproc/remoteproc_virtio.c
+++ b/drivers/remoteproc/remoteproc_virtio.c
@@ -140,7 +140,8 @@ static void rproc_virtio_del_vqs(struct virtio_device *vdev)
static int rproc_virtio_find_vqs(struct virtio_device *vdev, unsigned nvqs,
struct virtqueue *vqs[],
vq_callback_t *callbacks[],
- const char *names[])
+ const char *names[],
+ bool reversed[])
{
struct rproc *rproc = vdev_to_rproc(vdev);
int i, ret;
diff --git a/drivers/rpmsg/virtio_rpmsg_bus.c b/drivers/rpmsg/virtio_rpmsg_bus.c
index 027096f..3b2f727 100644
--- a/drivers/rpmsg/virtio_rpmsg_bus.c
+++ b/drivers/rpmsg/virtio_rpmsg_bus.c
@@ -946,7 +946,7 @@ static int rpmsg_probe(struct virtio_device *vdev)
init_waitqueue_head(&vrp->sendq);
/* We expect two virtqueues, rx and tx (and in this order) */
- err = vdev->config->find_vqs(vdev, 2, vqs, vq_cbs, names);
+ err = vdev->config->find_vqs(vdev, 2, vqs, vq_cbs, names, NULL);
if (err)
goto free_vrp;
diff --git a/drivers/s390/kvm/kvm_virtio.c b/drivers/s390/kvm/kvm_virtio.c
index 7dabef6..74cc7e8 100644
--- a/drivers/s390/kvm/kvm_virtio.c
+++ b/drivers/s390/kvm/kvm_virtio.c
@@ -246,7 +246,8 @@ static void kvm_del_vqs(struct virtio_device *vdev)
static int kvm_find_vqs(struct virtio_device *vdev, unsigned nvqs,
struct virtqueue *vqs[],
vq_callback_t *callbacks[],
- const char *names[])
+ const char *names[],
+ const bool reversed[])
{
[RFCv2 09/12] virtio-ring: Add BUG_ON checking on host/guest ring type
From: Sjur Brændeland
Add BUG_ON to ensure that the correct virtio queue type is
used for the virtqueue functions.
In addition the function virtqueue_kick_prepare() is changed so
that it always returns true if the virtio-ring is reversed.
Signed-off-by: Sjur Brændeland
---
drivers/virtio/virtio_ring.c | 11 +++
1 files changed, 11 insertions(+), 0 deletions(-)
diff --git a/drivers/virtio/virtio_ring.c b/drivers/virtio/virtio_ring.c
index 67f7bcd..a6f38c1 100644
--- a/drivers/virtio/virtio_ring.c
+++ b/drivers/virtio/virtio_ring.c
@@ -176,6 +176,7 @@ int virtqueue_add_buf(struct virtqueue *_vq,
START_USE(vq);
+ BUG_ON(vq->vq.reversed);
BUG_ON(data == NULL);
#ifdef DEBUG
@@ -282,6 +283,9 @@ bool virtqueue_kick_prepare(struct virtqueue *_vq)
u16 new, old;
bool needs_kick;
+ if (_vq->reversed)
+ return true;
+
START_USE(vq);
/* We need to expose available array entries before checking avail
* event. */
@@ -346,6 +350,7 @@ static void detach_buf(struct vring_virtqueue *vq, unsigned
int head)
{
unsigned int i;
+ BUG_ON(vq->vq.reversed);
/* Clear data ptr. */
vq->data[head] = NULL;
@@ -395,6 +400,7 @@ void *virtqueue_get_buf(struct virtqueue *_vq, unsigned int
*len)
unsigned int i;
u16 last_used;
+ BUG_ON(vq->vq.reversed);
START_USE(vq);
if (unlikely(vq->broken)) {
@@ -457,6 +463,7 @@ EXPORT_SYMBOL_GPL(virtqueue_get_buf);
void virtqueue_disable_cb(struct virtqueue *_vq)
{
struct vring_virtqueue *vq = to_vvq(_vq);
+ BUG_ON(vq->vq.reversed);
vq->vring.avail->flags |= VRING_AVAIL_F_NO_INTERRUPT;
}
@@ -477,6 +484,7 @@ bool virtqueue_enable_cb(struct virtqueue *_vq)
{
struct vring_virtqueue *vq = to_vvq(_vq);
+ BUG_ON(vq->vq.reversed);
START_USE(vq);
/* We optimistically turn back on interrupts, then check if there was
@@ -515,6 +523,7 @@ bool virtqueue_enable_cb_delayed(struct virtqueue *_vq)
struct vring_virtqueue *vq = to_vvq(_vq);
u16 bufs;
+ BUG_ON(vq->vq.reversed);
START_USE(vq);
/* We optimistically turn back on interrupts, then check if there was
@@ -551,6 +560,7 @@ void *virtqueue_detach_unused_buf(struct virtqueue *_vq)
unsigned int i;
void *buf;
+ BUG_ON(vq->vq.reversed);
START_USE(vq);
for (i = 0; i < vq->vring.num; i++) {
@@ -575,6 +585,7 @@ irqreturn_t __vring_interrupt(int irq, void *_vq)
{
struct vring_virtqueue *vq = to_vvq(_vq);
+ BUG_ON(vq->vq.reversed);
if (!more_used(vq)) {
pr_debug("virtqueue interrupt with no work for %p\n", vq);
return IRQ_NONE;
--
1.7.5.4
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
[RFCv2 08/12] virtio: Update vring_interrupt for host-side virtio queues
From: Sjur Brændeland
Add an inline function for vring_interrupt that can handle
host side virtio queues.
Signed-off-by: Sjur Brændeland
---
drivers/virtio/virtio_ring.c |4 ++--
include/linux/virtio_ring.h | 13 -
2 files changed, 14 insertions(+), 3 deletions(-)
diff --git a/drivers/virtio/virtio_ring.c b/drivers/virtio/virtio_ring.c
index ead47d7..67f7bcd 100644
--- a/drivers/virtio/virtio_ring.c
+++ b/drivers/virtio/virtio_ring.c
@@ -571,7 +571,7 @@ void *virtqueue_detach_unused_buf(struct virtqueue *_vq)
}
EXPORT_SYMBOL_GPL(virtqueue_detach_unused_buf);
-irqreturn_t vring_interrupt(int irq, void *_vq)
+irqreturn_t __vring_interrupt(int irq, void *_vq)
{
struct vring_virtqueue *vq = to_vvq(_vq);
@@ -589,7 +589,7 @@ irqreturn_t vring_interrupt(int irq, void *_vq)
return IRQ_HANDLED;
}
-EXPORT_SYMBOL_GPL(vring_interrupt);
+EXPORT_SYMBOL_GPL(__vring_interrupt);
struct virtqueue *vring_new_virtqueue(unsigned int index,
unsigned int num,
diff --git a/include/linux/virtio_ring.h b/include/linux/virtio_ring.h
index 01c0f59..d0aa046 100644
--- a/include/linux/virtio_ring.h
+++ b/include/linux/virtio_ring.h
@@ -33,7 +33,18 @@ void vring_del_virtqueue(struct virtqueue *vq);
/* Filter out transport-specific feature bits. */
void vring_transport_features(struct virtio_device *vdev);
-irqreturn_t vring_interrupt(int irq, void *_vq);
+irqreturn_t __vring_interrupt(int irq, void *_vq);
+static inline irqreturn_t vring_interrupt(int irq, void *_vq)
+{
+ struct virtqueue *vq = _vq;
+ if (!vq->callback)
+ return IRQ_HANDLED;
+ if (vq->reversed) {
+ vq->callback(vq);
+ return IRQ_HANDLED;
+ }
+ return __vring_interrupt(irq, vq);
+}
unsigned vring_next_desc(struct vring_desc *desc);
--
1.7.5.4
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
[RFCv2 07/12] virtio-ring: Add Host side virtio-ring implementation
Introduce a host-side virtio queue implementation. The function
vring_new_host_virtqueue(), virtqueue_add_buf_to_used()
virtqueue_next_avail_desc() are added to virtio_ring_host.c
Signed-off-by: Sjur Brændeland
---
drivers/virtio/virtio_ring_host.c | 195 +
include/linux/virtio.h|2 +
include/linux/virtio_ring.h | 23 +
3 files changed, 220 insertions(+), 0 deletions(-)
diff --git a/drivers/virtio/virtio_ring_host.c
b/drivers/virtio/virtio_ring_host.c
index 0750099..570e11e 100644
--- a/drivers/virtio/virtio_ring_host.c
+++ b/drivers/virtio/virtio_ring_host.c
@@ -19,10 +19,32 @@
#include
#include
#include
+#include
MODULE_LICENSE("GPL");
+struct vring_host_virtqueue {
+ struct virtqueue vq;
+
+ /* Actual memory layout for this queue */
+ struct vring_host vring;
+
+ /* Other side has made a mess, don't try any more. */
+ bool broken;
+};
+
+#define to_vvq(_vq) container_of(_vq, struct vring_host_virtqueue, vq)
+
+#define BAD_RING(_vq, fmt, args...)\
+ do {\
+ dev_err(&_vq->vq.vdev->dev, \
+ "%s:"fmt, (_vq)->vq.name, ##args); \
+ (_vq)->broken = true; \
+ } while (0)
+#define START_USE(vq)
+#define END_USE(vq)
+
static inline struct vring_used_elem *_vring_add_used(struct vring_host *vh,
u32 head, u32 len,
bool (*cpy)(void *dst,
@@ -148,3 +170,176 @@ unsigned vring_next_desc(struct vring_desc *desc)
return next;
}
EXPORT_SYMBOL(vring_next_desc);
+
+struct virtqueue *vring_new_host_virtqueue(unsigned int index,
+ unsigned int num,
+ unsigned int vring_align,
+ struct virtio_device *vdev,
+ bool weak_barriers,
+ void *pages,
+ void (*notify)(struct virtqueue *),
+ void (*callback)(struct virtqueue *),
+ const char *name)
+{
+ struct vring_host_virtqueue *vq;
+
+ /* We assume num is a power of 2. */
+ if (num & (num - 1)) {
+ dev_warn(&vdev->dev, "Bad virtqueue length %u\n", num);
+ return NULL;
+ }
+
+ vq = kmalloc(sizeof(*vq), GFP_KERNEL);
+ if (!vq)
+ return NULL;
+
+ vring_init(&vq->vring.vr, num, pages, vring_align);
+ vq->vq.callback = callback;
+ vq->vq.vdev = vdev;
+ vq->vq.name = name;
+ vq->vq.num_free = num;
+ vq->vq.index = index;
+ vq->vq.weak_barriers = weak_barriers;
+ vq->vq.notify = notify;
+ vq->broken = false;
+ vq->vq.reversed = true;
+ list_add_tail(&vq->vq.list, &vdev->vqs);
+ /* FIX: What about no callback, should we tell pair not to bother us? */
+ return &vq->vq;
+}
+EXPORT_SYMBOL_GPL(vring_new_host_virtqueue);
+
+static inline bool _kernel_cpy_to(void *dst, void *src, size_t s)
+{
+ memcpy(dst, src, s);
+ return true;
+}
+
+static inline bool _kernel_get(u16 *dst, u16 *src)
+{
+ *dst = *src;
+ return true;
+}
+
+static inline void _read_barrier(void)
+{
+ rmb();
+}
+
+/**
+ * virtqueue_next_avail_desc - get the next available descriptor
+ * @_vq: the struct virtqueue we're talking about
+ * @head: index of the descriptor in the ring
+ *
+ * Look for the next available descriptor in the available ring.
+ * Return NULL if nothing new in the available.
+ */
+struct vring_desc *virtqueue_next_avail_desc(struct virtqueue *_vq,
+ int *head)
+{
+ struct vring_host_virtqueue *vq = to_vvq(_vq);
+ struct vring_desc *desc = NULL;
+ int hd = -1;
+
+ BUG_ON(!vq->vq.reversed);
+ if (unlikely(vq->broken))
+ goto out;
+
+ START_USE(vq);
+ virtio_rmb(vq);
+
+ hd = _vring_avail_desc(&vq->vring, _kernel_get, _read_barrier);
+ if (unlikely(hd < 0)) {
+ BAD_RING(vq, "Bad available descriptor avail:%d last:%d\n",
+vq->vring.avail_idx, vq->vring.last_avail_idx);
+ goto out;
+ }
+ if (likely(hd >= vq->vring.vr.num))
+ goto out;
+
+ desc = &vq->vring.vr.desc[hd];
+ vq->vring.last_avail_idx++;
+out:
+ *head = hd;
+ END_USE(vq);
+ return desc;
+}
+EXPORT_SYMBOL(virtqueue_next_avail_desc);
+
+/*
+ * virtqueue_next_linked_desc - get n
[RFCv2 06/12] virtio_ring: Move SMP macros to virtio_ring.h
Move macros from virtio_ring.c to virtio_ring.h so that
they can be used outside the file.
Signed-off-by: Sjur Brændeland
---
drivers/virtio/virtio_ring.c | 21 -
include/linux/virtio_ring.h | 20
2 files changed, 20 insertions(+), 21 deletions(-)
diff --git a/drivers/virtio/virtio_ring.c b/drivers/virtio/virtio_ring.c
index 6aa76b4..ead47d7 100644
--- a/drivers/virtio/virtio_ring.c
+++ b/drivers/virtio/virtio_ring.c
@@ -24,27 +24,6 @@
#include
#include
-/* virtio guest is communicating with a virtual "device" that actually runs on
- * a host processor. Memory barriers are used to control SMP effects. */
-#ifdef CONFIG_SMP
-/* Where possible, use SMP barriers which are more lightweight than mandatory
- * barriers, because mandatory barriers control MMIO effects on accesses
- * through relaxed memory I/O windows (which virtio-pci does not use). */
-#define virtio_mb(vq) \
- do { if ((vq)->vq.weak_barriers) smp_mb(); else mb(); } while (0)
-#define virtio_rmb(vq) \
- do { if ((vq)->vq.weak_barriers) smp_rmb(); else rmb(); } while (0)
-#define virtio_wmb(vq) \
- do { if ((vq)->vq.weak_barriers) smp_wmb(); else wmb(); } while (0)
-#else
-/* We must force memory ordering even if guest is UP since host could be
- * running on another CPU, but SMP barriers are defined to barrier() in that
- * configuration. So fall back to mandatory barriers instead. */
-#define virtio_mb(vq) mb()
-#define virtio_rmb(vq) rmb()
-#define virtio_wmb(vq) wmb()
-#endif
-
#ifdef DEBUG
/* For development, we want to crash whenever the ring is screwed. */
#define BAD_RING(_vq, fmt, args...)\
diff --git a/include/linux/virtio_ring.h b/include/linux/virtio_ring.h
index 6c9b871..1a4023b 100644
--- a/include/linux/virtio_ring.h
+++ b/include/linux/virtio_ring.h
@@ -42,4 +42,24 @@ int vring_avail_desc_user(struct vring_host *vh);
struct vring_used_elem *vring_add_used_user(struct vring_host *vh,
unsigned int head, int len);
+/* virtio guest is communicating with a virtual "device" that actually runs on
+ * a host processor. Memory barriers are used to control SMP effects. */
+#ifdef CONFIG_SMP
+/* Where possible, use SMP barriers which are more lightweight than mandatory
+ * barriers, because mandatory barriers control MMIO effects on accesses
+ * through relaxed memory I/O windows (which virtio-pci does not use). */
+#define virtio_mb(vq) \
+ do { if ((vq)->vq.weak_barriers) smp_mb(); else mb(); } while (0)
+#define virtio_rmb(vq) \
+ do { if ((vq)->vq.weak_barriers) smp_rmb(); else rmb(); } while (0)
+#define virtio_wmb(vq) \
+ do { if ((vq)->vq.weak_barriers) smp_wmb(); else wmb(); } while (0)
+#else
+/* We must force memory ordering even if guest is UP since host could be
+ * running on another CPU, but SMP barriers are defined to barrier() in that
+ * configuration. So fall back to mandatory barriers instead. */
+#define virtio_mb(vq) mb()
+#define virtio_rmb(vq) rmb()
+#define virtio_wmb(vq) wmb()
+#endif
#endif /* _LINUX_VIRTIO_RING_H */
--
1.7.5.4
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
[RFCv2 05/12] virtio-ring: Refactor move attributes to struct virtqueue
Attributes 'weak_barriers' and 'notify' are moved
from struct vring_virtqueue to struct virtqueue.
Signed-off-by: Sjur Brændeland
---
drivers/virtio/virtio_ring.c | 20 +++-
include/linux/virtio.h |4
2 files changed, 11 insertions(+), 13 deletions(-)
diff --git a/drivers/virtio/virtio_ring.c b/drivers/virtio/virtio_ring.c
index ffd7e7d..6aa76b4 100644
--- a/drivers/virtio/virtio_ring.c
+++ b/drivers/virtio/virtio_ring.c
@@ -31,11 +31,11 @@
* barriers, because mandatory barriers control MMIO effects on accesses
* through relaxed memory I/O windows (which virtio-pci does not use). */
#define virtio_mb(vq) \
- do { if ((vq)->weak_barriers) smp_mb(); else mb(); } while(0)
+ do { if ((vq)->vq.weak_barriers) smp_mb(); else mb(); } while (0)
#define virtio_rmb(vq) \
- do { if ((vq)->weak_barriers) smp_rmb(); else rmb(); } while(0)
+ do { if ((vq)->vq.weak_barriers) smp_rmb(); else rmb(); } while (0)
#define virtio_wmb(vq) \
- do { if ((vq)->weak_barriers) smp_wmb(); else wmb(); } while(0)
+ do { if ((vq)->vq.weak_barriers) smp_wmb(); else wmb(); } while (0)
#else
/* We must force memory ordering even if guest is UP since host could be
* running on another CPU, but SMP barriers are defined to barrier() in that
@@ -81,9 +81,6 @@ struct vring_virtqueue
/* Actual memory layout for this queue */
struct vring vring;
- /* Can we use weak barriers? */
- bool weak_barriers;
-
/* Other side has made a mess, don't try any more. */
bool broken;
@@ -101,9 +98,6 @@ struct vring_virtqueue
/* Last used index we've seen. */
u16 last_used_idx;
- /* How to notify other side. FIXME: commonalize hcalls! */
- void (*notify)(struct virtqueue *vq);
-
#ifdef DEBUG
/* They're supposed to lock for us. */
unsigned int in_use;
@@ -236,7 +230,7 @@ int virtqueue_add_buf(struct virtqueue *_vq,
* there are outgoing parts to the buffer. Presumably the
* host should service the ring ASAP. */
if (out)
- vq->notify(&vq->vq);
+ vq->vq.notify(&vq->vq);
END_USE(vq);
return -ENOSPC;
}
@@ -348,7 +342,7 @@ void virtqueue_notify(struct virtqueue *_vq)
struct vring_virtqueue *vq = to_vvq(_vq);
/* Prod other side to tell it about changes. */
- vq->notify(_vq);
+ vq->vq.notify(_vq);
}
EXPORT_SYMBOL_GPL(virtqueue_notify);
@@ -647,8 +641,8 @@ struct virtqueue *vring_new_virtqueue(unsigned int index,
vq->vq.name = name;
vq->vq.num_free = num;
vq->vq.index = index;
- vq->notify = notify;
- vq->weak_barriers = weak_barriers;
+ vq->vq.notify = notify;
+ vq->vq.weak_barriers = weak_barriers;
vq->broken = false;
vq->last_used_idx = 0;
vq->num_added = 0;
diff --git a/include/linux/virtio.h b/include/linux/virtio.h
index 25fa1a6..f513ba8 100644
--- a/include/linux/virtio.h
+++ b/include/linux/virtio.h
@@ -13,11 +13,13 @@
* virtqueue - a queue to register buffers for sending or receiving.
* @list: the chain of virtqueues for this device
* @callback: the function to call when buffers are consumed (can be NULL).
+ * @notify: the function to notify the other side (can be NULL)
* @name: the name of this virtqueue (mainly for debugging)
* @vdev: the virtio device this queue was created for.
* @priv: a pointer for the virtqueue implementation to use.
* @index: the zero-based ordinal number for this queue.
* @num_free: number of elements we expect to be able to fit.
+ * @weak_barriers: indicate if we can use weak memory barriers.
*
* A note on @num_free: with indirect buffers, each buffer needs one
* element in the queue, otherwise a buffer will need one element per
@@ -26,11 +28,13 @@
struct virtqueue {
struct list_head list;
void (*callback)(struct virtqueue *vq);
+ void (*notify)(struct virtqueue *vq);
const char *name;
struct virtio_device *vdev;
unsigned int index;
unsigned int num_free;
void *priv;
+ bool weak_barriers;
};
int virtqueue_add_buf(struct virtqueue *vq,
--
1.7.5.4
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
[RFCv2 04/12] virtio-ring: Refactor out the functions accessing user memory
Isolate the access to user-memory in separate inline
functions. This open up for reuse from host-side virtioqueue
implementation accessing virtio-ring in kernel space.
Signed-off-by: Sjur Brændeland
---
drivers/virtio/virtio_ring_host.c | 81 ++---
1 files changed, 57 insertions(+), 24 deletions(-)
diff --git a/drivers/virtio/virtio_ring_host.c
b/drivers/virtio/virtio_ring_host.c
index 192b838..0750099 100644
--- a/drivers/virtio/virtio_ring_host.c
+++ b/drivers/virtio/virtio_ring_host.c
@@ -18,44 +18,45 @@
#include
#include
#include
+#include
MODULE_LICENSE("GPL");
-struct vring_used_elem *vring_add_used_user(struct vring_host *vh,
-unsigned int head, int len)
+
+static inline struct vring_used_elem *_vring_add_used(struct vring_host *vh,
+ u32 head, u32 len,
+ bool (*cpy)(void *dst,
+ void *src,
+ size_t s),
+ void (*wbarrier)(void))
{
struct vring_used_elem *used;
+ u16 last_used;
/* The virtqueue contains a ring of used buffers. Get a pointer to the
* next entry in that used ring. */
- used = &vh->vr.used->ring[vh->last_used_idx % vh->vr.num];
- if (__put_user(head, &used->id)) {
- pr_debug("Failed to write used id");
+ used = &vh->vr.used->ring[vh->last_used_idx & (vh->vr.num - 1)];
+ if (!cpy(&used->id, &head, sizeof(used->id)) ||
+ !cpy(&used->len, &len, sizeof(used->len)))
return NULL;
- }
- if (__put_user(len, &used->len)) {
- pr_debug("Failed to write used len");
+ wbarrier();
+ last_used = vh->last_used_idx + 1;
+ if (!cpy(&vh->vr.used->idx, &last_used, sizeof(vh->vr.used->idx)))
return NULL;
- }
- /* Make sure buffer is written before we update index. */
- smp_wmb();
- if (__put_user(vh->last_used_idx + 1, &vh->vr.used->idx)) {
- pr_debug("Failed to increment used idx");
- return NULL;
- }
- vh->last_used_idx++;
+ vh->last_used_idx = last_used;
return used;
}
-EXPORT_SYMBOL(vring_add_used_user);
-int vring_avail_desc_user(struct vring_host *vh)
+static inline int _vring_avail_desc(struct vring_host *vh,
+ bool (*get)(u16 *dst, u16 *src),
+ void (*read_barrier)(void))
{
- int head;
+ u16 head;
u16 last_avail_idx;
/* Check it isn't doing very strange things with descriptor numbers. */
last_avail_idx = vh->last_avail_idx;
- if (unlikely(__get_user(vh->avail_idx, &vh->vr.avail->idx))) {
+ if (unlikely(!get(&vh->avail_idx, &vh->vr.avail->idx))) {
pr_debug("Failed to access avail idx at %p\n",
&vh->vr.avail->idx);
return -EFAULT;
@@ -69,13 +70,12 @@ int vring_avail_desc_user(struct vring_host *vh)
return vh->vr.num;
/* Only get avail ring entries after they have been exposed by guest. */
- smp_rmb();
+ read_barrier();
/* Grab the next descriptor number they're advertising, and increment
* the index we've seen. */
- if (unlikely(__get_user(head,
- &vh->vr.avail->ring[last_avail_idx %
- vh->vr.num]))) {
+ if (unlikely(!get(&head, &vh->vr.avail->ring[last_avail_idx &
+(vh->vr.num - 1)]))) {
pr_debug("Failed to read head: idx %d address %p\n",
last_avail_idx,
&vh->vr.avail->ring[last_avail_idx %
@@ -92,6 +92,39 @@ int vring_avail_desc_user(struct vring_host *vh)
return head;
}
+
+static inline void smp_write_barrier(void)
+{
+ smp_wmb();
+}
+
+static inline void smp_read_barrier(void)
+{
+ smp_rmb();
+}
+
+static inline bool userspace_cpy_to(void *dst, void *src, size_t s)
+{
+ return __copy_to_user(dst, src, s) == 0;
+}
+
+static inline bool userspace_get(u16 *dst, u16 *src)
+{
+ return __get_user(*dst, src);
+}
+
+struct vring_used_elem *vring_add_used_user(struct vring_host *vh,
+ unsigned int head, int len)
+{
+ return _vring_add_used(vh, head, len, userspace_cpy_to,
+ s
[RFCv2 03/12] virtio-ring: Introduce file virtio_ring_host
Move host-side virtio ring functions to file virtio_ring_host.c
The functions vring_avail_desc_user(), vring_add_used_user() and
vring_next_desc() are moved from vhost.c to the new file
virtio_ring_host.c. (The functions are copied as is without any changes)
Signed-off-by: Sjur Brændeland
---
drivers/vhost/Kconfig |2 +
drivers/vhost/vhost.c | 92 -
drivers/virtio/Kconfig|3 +
drivers/virtio/Makefile |1 +
drivers/virtio/virtio_ring_host.c | 117 +
include/linux/virtio_ring.h |8 +++
6 files changed, 131 insertions(+), 92 deletions(-)
create mode 100644 drivers/virtio/virtio_ring_host.c
diff --git a/drivers/vhost/Kconfig b/drivers/vhost/Kconfig
index 202bba6..5bfdaa9 100644
--- a/drivers/vhost/Kconfig
+++ b/drivers/vhost/Kconfig
@@ -1,4 +1,6 @@
config VHOST_NET
+ select VIRTIO_RING_HOST
+ select VIRTIO
tristate "Host kernel accelerator for virtio net (EXPERIMENTAL)"
depends on NET && EVENTFD && (TUN || !TUN) && (MACVTAP || !MACVTAP) &&
EXPERIMENTAL
---help---
diff --git a/drivers/vhost/vhost.c b/drivers/vhost/vhost.c
index 5e91048..6634f0a 100644
--- a/drivers/vhost/vhost.c
+++ b/drivers/vhost/vhost.c
@@ -1097,27 +1097,6 @@ static int translate_desc(struct vhost_dev *dev, u64
addr, u32 len,
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. */
-unsigned vring_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 int 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,
@@ -1206,51 +1185,6 @@ static int get_indirect(struct vhost_dev *dev, struct
vhost_virtqueue *vq,
return 0;
}
-static int vring_avail_desc_user(struct vring_host *vh)
-{
- int head;
- u16 last_avail_idx;
-
- /* Check it isn't doing very strange things with descriptor numbers. */
- last_avail_idx = vh->last_avail_idx;
- if (unlikely(__get_user(vh->avail_idx, &vh->vr.avail->idx))) {
- pr_debug("Failed to access avail idx at %p\n",
- &vh->vr.avail->idx);
- return -EFAULT;
- }
-
- if (unlikely((u16)(vh->avail_idx - last_avail_idx) > vh->vr.num))
- return -EFAULT;
-
- /* If there's nothing new since last we looked, return invalid. */
- if (vh->avail_idx == last_avail_idx)
- return vh->vr.num;
-
- /* Only get avail ring entries after they have been exposed by guest. */
- smp_rmb();
-
- /* Grab the next descriptor number they're advertising, and increment
-* the index we've seen. */
- if (unlikely(__get_user(head,
- &vh->vr.avail->ring[last_avail_idx %
- vh->vr.num]))) {
- pr_debug("Failed to read head: idx %d address %p\n",
-last_avail_idx,
- &vh->vr.avail->ring[last_avail_idx %
- vh->vr.num]);
- return -EFAULT;
- }
-
- /* If their number is silly, that's an error. */
- if (unlikely(head >= vh->vr.num)) {
- pr_debug("Guest says index %u > %u is available",
- head, vh->vr.num);
- return -EINVAL;
- }
-
- return head;
-}
-
/* 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
@@ -1352,32 +1286,6 @@ void vhost_discard_vq_desc(struct vhost_virtqueue *vq,
int n)
vq->hst.last_avail_idx -= n;
}
-struct vring_used_elem *vring_add_used_user(struct vring_host *vh,
-unsigned int head, int len)
-{
- struct vring_used_elem *used;
-
- /* The virtqueue contains a ring of used buffers. Ge
[RFCv2 02/12] vhost: Isolate reusable vring related functions
Prepare for moving virtio ring code out to a separate
file by isolating vring related functions. The function
vring_add_used_user() and vring_avail_desc_user() that
are handling virtio rings from user space are prepared
to be moved out.
Signed-off-by: Sjur Brændeland
---
drivers/vhost/vhost.c | 119
1 files changed, 69 insertions(+), 50 deletions(-)
diff --git a/drivers/vhost/vhost.c b/drivers/vhost/vhost.c
index 0a676f1..5e91048 100644
--- a/drivers/vhost/vhost.c
+++ b/drivers/vhost/vhost.c
@@ -1100,7 +1100,7 @@ static int translate_desc(struct vhost_dev *dev, u64
addr, u32 len,
/* 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 vring_next_desc(struct vring_desc *desc)
{
unsigned int next;
@@ -1202,46 +1202,29 @@ static int get_indirect(struct vhost_dev *dev, struct
vhost_virtqueue *vq,
}
*out_num += ret;
}
- } while ((i = next_desc(&desc)) != -1);
+ } while ((i = vring_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. A negative code is
- * returned on error. */
-int vhost_get_vq_desc(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)
+static int vring_avail_desc_user(struct vring_host *vh)
{
- struct vring_desc desc;
- unsigned int i, head, found = 0;
+ int head;
u16 last_avail_idx;
- int ret;
/* Check it isn't doing very strange things with descriptor numbers. */
- last_avail_idx = vq->hst.last_avail_idx;
- if (unlikely(__get_user(vq->hst.avail_idx, &vq->hst.vr.avail->idx))) {
- vq_err(vq, "Failed to access avail idx at %p\n",
- &vq->hst.vr.avail->idx);
+ last_avail_idx = vh->last_avail_idx;
+ if (unlikely(__get_user(vh->avail_idx, &vh->vr.avail->idx))) {
+ pr_debug("Failed to access avail idx at %p\n",
+ &vh->vr.avail->idx);
return -EFAULT;
}
- if (unlikely((u16)(vq->hst.avail_idx -
- last_avail_idx) > vq->hst.vr.num)) {
- vq_err(vq, "Guest moved used index from %u to %u",
- last_avail_idx, vq->hst.avail_idx);
+ if (unlikely((u16)(vh->avail_idx - last_avail_idx) > vh->vr.num))
return -EFAULT;
- }
/* If there's nothing new since last we looked, return invalid. */
- if (vq->hst.avail_idx == last_avail_idx)
- return vq->hst.vr.num;
+ if (vh->avail_idx == last_avail_idx)
+ return vh->vr.num;
/* Only get avail ring entries after they have been exposed by guest. */
smp_rmb();
@@ -1249,22 +1232,46 @@ int vhost_get_vq_desc(struct vhost_dev *dev, struct
vhost_virtqueue *vq,
/* Grab the next descriptor number they're advertising, and increment
* the index we've seen. */
if (unlikely(__get_user(head,
- &vq->hst.vr.avail->ring[last_avail_idx %
- vq->hst.vr.num]))) {
- vq_err(vq, "Failed to read head: idx %d address %p\n",
- last_avail_idx,
- &vq->hst.vr.avail->ring[last_avail_idx %
- vq->hst.vr.num]);
+ &vh->vr.avail->ring[last_avail_idx %
+ vh->vr.num]))) {
+ pr_debug("Failed to read head: idx %d address %p\n",
+last_avail_idx,
+ &vh->vr.avail->ring[last_avail_idx %
+ vh->vr.num]);
return -EFAULT;
}
/* If their number is silly, that's an error. */
- if (unlikely(head >= vq->hst.vr.num)) {
- vq_err(vq, "Guest says index %u > %u is available",
-
[RFCv2 01/12] vhost: Use struct vring in vhost_virtqueue
Pull out common vring attributes from vhost_virtqueue to a new
struct vring_host. This allows for reuse of data definitions
between vhost and virtio queue when host-side virtio queue is
introduced. Also unsigned long is replaced with ulong a couple
of places.
Signed-off-by: Sjur Brændeland
---
drivers/vhost/net.c |4 +-
drivers/vhost/vhost.c | 213 +++
drivers/vhost/vhost.h | 14 +---
include/linux/virtio_ring.h | 13 +++
4 files changed, 130 insertions(+), 114 deletions(-)
diff --git a/drivers/vhost/net.c b/drivers/vhost/net.c
index 072cbba..8fc1869 100644
--- a/drivers/vhost/net.c
+++ b/drivers/vhost/net.c
@@ -182,7 +182,7 @@ static void handle_tx(struct vhost_net *net)
if (unlikely(head < 0))
break;
/* Nothing new? Wait for eventfd to tell us they refilled. */
- if (head == vq->num) {
+ if (head == vq->hst.vr.num) {
int num_pends;
wmem = atomic_read(&sock->sk->sk_wmem_alloc);
@@ -329,7 +329,7 @@ static int get_rx_bufs(struct vhost_virtqueue *vq,
d = vhost_get_vq_desc(vq->dev, vq, vq->iov + seg,
ARRAY_SIZE(vq->iov) - seg, &out,
&in, log, log_num);
- if (d == vq->num) {
+ if (d == vq->hst.vr.num) {
r = 0;
goto err;
}
diff --git a/drivers/vhost/vhost.c b/drivers/vhost/vhost.c
index 99ac2cb..0a676f1 100644
--- a/drivers/vhost/vhost.c
+++ b/drivers/vhost/vhost.c
@@ -12,6 +12,7 @@
*/
#include
+#include
#include
#include
#include
@@ -39,8 +40,10 @@ enum {
static unsigned vhost_zcopy_mask __read_mostly;
-#define vhost_used_event(vq) ((u16 __user *)&vq->avail->ring[vq->num])
-#define vhost_avail_event(vq) ((u16 __user *)&vq->used->ring[vq->num])
+#define vhost_used_event(vq) ((u16 __user *) \
+ &vq->hst.vr.avail->ring[vq->hst.vr.num])
+#define vhost_avail_event(vq) ((u16 __user *)\
+ &vq->hst.vr.used->ring[vq->hst.vr.num])
static void vhost_poll_func(struct file *file, wait_queue_head_t *wqh,
poll_table *pt)
@@ -57,7 +60,7 @@ static int vhost_poll_wakeup(wait_queue_t *wait, unsigned
mode, int sync,
{
struct vhost_poll *poll = container_of(wait, struct vhost_poll, wait);
- if (!((unsigned long)key & poll->mask))
+ if (!((ulong)key & poll->mask))
return 0;
vhost_poll_queue(poll);
@@ -75,7 +78,7 @@ void vhost_work_init(struct vhost_work *work, vhost_work_fn_t
fn)
/* Init poll structure */
void vhost_poll_init(struct vhost_poll *poll, vhost_work_fn_t fn,
-unsigned long mask, struct vhost_dev *dev)
+ulong mask, struct vhost_dev *dev)
{
init_waitqueue_func_entry(&poll->wait, vhost_poll_wakeup);
init_poll_funcptr(&poll->table, vhost_poll_func);
@@ -89,7 +92,7 @@ void vhost_poll_init(struct vhost_poll *poll, vhost_work_fn_t
fn,
* 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;
+ ulong mask;
mask = file->f_op->poll(file, &poll->table);
if (mask)
@@ -139,7 +142,7 @@ void vhost_poll_flush(struct vhost_poll *poll)
void vhost_work_queue(struct vhost_dev *dev, struct vhost_work *work)
{
- unsigned long flags;
+ ulong flags;
spin_lock_irqsave(&dev->work_lock, flags);
if (list_empty(&work->node)) {
@@ -158,13 +161,13 @@ void vhost_poll_queue(struct vhost_poll *poll)
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->hst.vr.num = 1;
+ vq->hst.vr.desc = NULL;
+ vq->hst.vr.avail = NULL;
+ vq->hst.vr.used = NULL;
+ vq->hst.last_avail_idx = 0;
+ vq->hst.avail_idx = 0;
+ vq->hst.last_used_idx = 0;
vq->signalled_used = 0;
vq->signalled_used_valid = false;
vq->used_flags = 0;
@@ -489,13 +492,13 @@ void vhost_dev_cleanup(struct vhost_dev *dev, bool locked)
dev->mm = NULL;
}
-static int log_access_ok(void __user *log_base, u64 addr, unsigned long sz)
+static int log_access_ok(void __user *log_base, u64 addr, ulong sz)
{
u64 a = addr / VHOST_PAGE_SIZE / 8;
/* Make sure 64 bit math will not overflow. */
- if (a > ULONG_MAX - (unsigned long)log_base ||
-
[RFCv2 00/12] Introduce host-side virtio queue and CAIF Virtio.
From: Sjur Brændeland This patch-set introduces a host-side virtqueue implementation and the CAIF Virtio Link layer. See http://lwn.net/Articles/522296/ for background info on CAIF over Virtio. After feedback from Rusty, I have re-factored vhost.c and pulled out common functionality and data definitions from drivers/vhost into drivers/virtio. Part of the challenge of doing this is that vhost.c is accessing the virtio rings from user-space, while CAIF uses kernel memory. In order to solve this issue, inline memory access functions are passed to some of the vring functions (as suggested by Rusty). I have added one argument to the function find_vqs(), in order to request host-side virtio-queue. (This needs some more work...) Feedback on this patch-set is appreciated, particularly on structure and code-reuse between vhost.c and the host-side virtio-queue. I'd also like some suggestions on how to handle the build configuration better - currently there are some unnecessary build dependencies. The patches are based on v3.7, and compile tested only. Rusty, if you want to review this later, after the merge window closes, let me know and I'll resend these patches later. Thanks, Sjur Sjur Brændeland (11): vhost: Use struct vring in vhost_virtqueue vhost: Isolate reusable vring related functions virtio-ring: Introduce file virtio_ring_host virtio-ring: Refactor out the functions accessing user memory virtio-ring: Refactor move attributes to struct virtqueue virtio_ring: Move SMP macros to virtio_ring.h virtio-ring: Add Host side virtio-ring implementation virtio: Update vring_interrupt for host-side virtio queues virtio-ring: Add BUG_ON checking on host/guest ring type virtio: Add argument reversed to function find_vqs() remoteproc: Add support for host-virtqueues Vikram ARV (1): caif_virtio: Introduce caif over virtio drivers/char/virtio_console.c |3 +- drivers/lguest/lguest_device.c |5 +- drivers/net/caif/Kconfig |8 + drivers/net/caif/Makefile |3 + drivers/net/caif/caif_virtio.c | 482 drivers/net/virtio_net.c |3 +- drivers/remoteproc/remoteproc_virtio.c | 18 +- drivers/rpmsg/virtio_rpmsg_bus.c |2 +- drivers/s390/kvm/kvm_virtio.c |5 +- drivers/scsi/virtio_scsi.c |2 +- drivers/vhost/Kconfig |2 + drivers/vhost/net.c|4 +- drivers/vhost/vhost.c | 272 +++--- drivers/vhost/vhost.h | 14 +- drivers/virtio/Kconfig |3 + drivers/virtio/Makefile|1 + drivers/virtio/virtio_balloon.c|3 +- drivers/virtio/virtio_mmio.c |5 +- drivers/virtio/virtio_pci.c|3 +- drivers/virtio/virtio_ring.c | 50 ++--- drivers/virtio/virtio_ring_host.c | 345 +++ include/linux/virtio.h |6 + include/linux/virtio_caif.h| 25 ++ include/linux/virtio_config.h |7 +- include/linux/virtio_ring.h| 77 +- include/uapi/linux/virtio_ids.h|1 + 26 files changed, 1120 insertions(+), 229 deletions(-) create mode 100644 drivers/net/caif/caif_virtio.c create mode 100644 drivers/virtio/virtio_ring_host.c create mode 100644 include/linux/virtio_caif.h -- 1.7.5.4 ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH] virtio: Don't access index after unregister.
On Thu, Nov 8, 2012 at 11:43 AM, Cornelia Huck wrote:
> Virtio wants to release used indices after the corresponding
> virtio device has been unregistered. However, virtio does not
> hold an extra reference, giving up its last reference with
> device_unregister(), making accessing dev->index afterwards
> invalid.
>
> I actually saw problems when testing my (not-yet-merged)
> virtio-ccw code:
>
> - device_add virtio-net,id=xxx
> -> creates device virtio with n>0
>
> - device_del xxx
> -> deletes virtio, but calls ida_simple_remove with an
>index of 0
>
> - device_add virtio-net,id=xxx
> -> tries to add virtio0, which is still in use...
>
> So let's save the index we want to release before calling
> device_unregister().
>
> Signed-off-by: Cornelia Huck
> ---
> drivers/virtio/virtio.c | 4 +++-
> 1 file changed, 3 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/virtio/virtio.c b/drivers/virtio/virtio.c
> index 1e8659c..809b0de 100644
> --- a/drivers/virtio/virtio.c
> +++ b/drivers/virtio/virtio.c
> @@ -225,8 +225,10 @@ EXPORT_SYMBOL_GPL(register_virtio_device);
>
> void unregister_virtio_device(struct virtio_device *dev)
> {
> + int index = dev->index; /* save for after device release */
> +
> device_unregister(&dev->dev);
> - ida_simple_remove(&virtio_index_ida, dev->index);
> + ida_simple_remove(&virtio_index_ida, index);
> }
> EXPORT_SYMBOL_GPL(unregister_virtio_device);
Acked-by: Sjur Brændeland
Great minds think alike! I discovered issues with this implementation
a while back and Michael suggested an identical patch:
https://lkml.org/lkml/2012/9/4/173
https://lkml.org/lkml/2012/9/7/105
The issue I ran into was that when virtio devices are created by remoteproc
the device memory might be freed when calling device_unregister(), and
the value of dev->index is then undefined. So this bug bites when
unregistering a Virtio devices from remoteproc with CONFIG_DEBUG_SLAB
enabled. However this bug is not triggered by virtio_pci as it implements a
non-standard device release-function that does not free the device memory.
Thanks,
Sjur
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH resend] virtio_console: Free buffers from out-queue upon close
>> > Note: This patch is compile tested only. I have done the removal >> > of buffers from out-queue in handle_control_message() >> > when host has acked the close request. This seems less >> > racy than doing it in the release function. >> >> This confuses me... why are we doing this in case >> VIRTIO_CONSOLE_PORT_OPEN:? >> >> We can't pull unconsumed buffers out of the ring when the other side may >> still access it, and this seems to be doing that. > > Yes -- and it's my fault; I asked Sjur to do that in the close fops > function. Thanks Amit :-), but this was really my bad. > We should only do this in the port remove case (unplug or device > remove) -- so the original patch, with just the WARN_ON removed is the > right way. > > I'll send the revised 3/3 patch for you. Thank you. Regards, Sjur ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [RFC virtio-next 0/4] Introduce CAIF Virtio and reversed Vrings
Hi Rusty,
> So, this adds another host-side virtqueue implementation.
>
> Can we combine them together conveniently? You pulled out more stuff
> into vring.h which is a start, but it's a bit overloaded.
> Perhaps we should separate the common fields into struct vring, and use
> it to build:
>
> struct vring_guest {
> struct vring vr;
> u16 last_used_idx;
> };
>
> struct vring_host {
> struct vring vr;
> u16 last_avail_idx;
> };
> I haven't looked closely at vhost to see what it wants, but I would
> think we could share more code.
I have played around with the code in vhost.c to explore your idea.
The main issue I run into is that vhost.c is accessing user data while my new
code does not. So I end up with some quirky code testing if the ring lives in
user memory or not. Another issue is sparse warnings when
accessing user memory.
With your suggested changes I end up sharing about 100 lines of code.
So in sum, I feel this add more complexity than what we gain by sharing.
Below is an initial draft of the re-usable code. I added "is_uaccess" to struct
virtio_ring in order to know if the ring lives in user memory.
Let me know what you think.
[snip]
int virtqueue_add_used(struct vring_host *vr, 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 = &vr->vring.used->ring[vr->last_used_idx % vr->vring.num];
if (vr->is_uaccess) {
if(unlikely(__put_user(head, &(*used)->id))) {
pr_debug("Failed to write used id");
return -EFAULT;
}
if (unlikely(__put_user(len, &(*used)->len))) {
pr_debug("Failed to write used len");
return -EFAULT;
}
smp_wmb();
if (__put_user(vr->last_used_idx + 1,
&vr->vring.used->idx)) {
pr_debug("Failed to increment used idx");
return -EFAULT;
}
} else {
(*used)->id = head;
(*used)->len = len;
smp_wmb();
vr->vring.used->idx = vr->last_used_idx + 1;
}
vr->last_used_idx++;
return 0;
}
/* 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. */
unsigned virtqueue_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 int virtqueue_next_avail_desc(struct vring_host *vr)
{
int head;
u16 last_avail_idx;
/* Check it isn't doing very strange things with descriptor numbers. */
last_avail_idx = vr->last_avail_idx;
if (vr->is_uaccess) {
if (__get_user(vr->avail_idx, &vr->vring.avail->idx)) {
pr_debug("Failed to access avail idx at %p\n",
&vr->vring.avail->idx);
return -EFAULT;
}
} else
vr->avail_idx = vr->vring.avail->idx;
if (unlikely((u16)(vr->avail_idx - last_avail_idx) > vr->vring.num)) {
pr_debug("Guest moved used index from %u to %u",
last_avail_idx, vr->avail_idx);
return -EFAULT;
}
/* If there's nothing new since last we looked, return invalid. */
if (vr->avail_idx == last_avail_idx)
return vr->vring.num;
/* Only get avail ring entries after they have been exposed by guest. */
smp_rmb();
/* Grab the next descriptor number they're advertising, and increment
* the index we've seen. */
if (vr->is_uaccess) {
if (unlikely(__get_user(head,
&vr->vring.avail->ring[last_avail_idx
% vr->vring.num]))) {
pr_debug("Failed to read head: idx %d address %p\n",
last_avail_idx,
&vr->vring.avail->ring[last_avail_idx %
vr->vring.num]);
return -EFAULT;
}
[RFC virtio-next 4/4] caif_virtio: Add CAIF over virtio
From: Sjur Brændeland
Add the CAIF Virtio Link layer, used for communicating with a
modem over shared memory. Virtio is used as the transport mechanism.
In the TX direction the virtio rings are used in the normal fashion,
sending data in the available ring. But in the rx direction the
the we have flipped the direction of the virtio ring, and
implemented the virtio access-function similar to what is found
in vhost.c.
CAIF also uses the virtio configuration space for getting
configuration parameters such as headroom, tailroom etc.
Signed-off-by: Vikram ARV
Signed-off-by: Sjur Brændeland
---
drivers/net/caif/Kconfig|9 +
drivers/net/caif/Makefile |3 +
drivers/net/caif/caif_virtio.c | 627 +++
include/uapi/linux/virtio_ids.h |1 +
4 files changed, 640 insertions(+)
create mode 100644 drivers/net/caif/caif_virtio.c
diff --git a/drivers/net/caif/Kconfig b/drivers/net/caif/Kconfig
index abf4d7a..a01f617 100644
--- a/drivers/net/caif/Kconfig
+++ b/drivers/net/caif/Kconfig
@@ -47,3 +47,12 @@ 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"
+ default n
+ depends on CAIF
+ depends on REMOTEPROC
+ select VIRTIO
+ ---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 000..e50940f
--- /dev/null
+++ b/drivers/net/caif/caif_virtio.c
@@ -0,0 +1,627 @@
+/*
+ * Copyright (C) ST-Ericsson AB 2012
+ * Contact: Sjur Brendeland / sjur.brandel...@stericsson.com
+ * Authors: Vicram Arv / vikram@stericsson.com,
+ * Dmitry Tarnyagin / dmitry.tarnya...@stericsson.com
+ * Sjur Brendeland / sjur.brandel...@stericsson.com
+ * License terms: GNU General Public License (GPL) version 2
+ */
+
+#define pr_fmt(fmt) KBUILD_MODNAME ":" fmt
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include "../drivers/virtio/vring.h"
+
+MODULE_LICENSE("GPL v2");
+MODULE_AUTHOR("Vicram Arv ");
+MODULE_DESCRIPTION("Virtio CAIF Driver");
+
+/*
+ * 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 virtqueue *vq_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;
+ /* 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 16
+#define CFV_DEF_TAILROOM 16
+
+/* Require IP header to be 4-byte aligned. */
+#define IP_HDR_ALIGN 4
+
+/*
+ * virtqueue_next_avail_desc - get the next available descriptor
+ * @_vq: the struct virtqueue we're talking about
+ * @head: index of the descriptor in the ring
+ *
+ * Look for the next available descriptor in the available ring.
+ * Return NULL if nothing new in the available.
+ */
+static struct vring_desc *virtqueue_next_avail_desc(struct virtqueue *_vq,
+ int *head)
+{
+ struct vring_virtqueue *vq = to_vvq(_vq);
+ u16 avail_idx, hd, last_avail_idx = vq->last_avail_idx;
+
+ STA
[RFC virtio-next 1/4] virtio: Move definitions to header file vring.h
From: Sjur Brændeland
Move the vring_virtqueue structure, memory barrier and debug
macros out from virtio_ring.c to the new header file vring.h.
This is done in order to allow other kernel modules to access the
virtio internal data-structures.
Signed-off-by: Sjur Brændeland
---
Tis patch triggers a couple of checkpatch warnings, but I've
chosen to do a clean copy and not do any corrections.
drivers/virtio/virtio_ring.c | 96 +
drivers/virtio/vring.h | 121 ++
2 files changed, 122 insertions(+), 95 deletions(-)
create mode 100644 drivers/virtio/vring.h
diff --git a/drivers/virtio/virtio_ring.c b/drivers/virtio/virtio_ring.c
index ffd7e7d..9027af6 100644
--- a/drivers/virtio/virtio_ring.c
+++ b/drivers/virtio/virtio_ring.c
@@ -23,101 +23,7 @@
#include
#include
#include
-
-/* virtio guest is communicating with a virtual "device" that actually runs on
- * a host processor. Memory barriers are used to control SMP effects. */
-#ifdef CONFIG_SMP
-/* Where possible, use SMP barriers which are more lightweight than mandatory
- * barriers, because mandatory barriers control MMIO effects on accesses
- * through relaxed memory I/O windows (which virtio-pci does not use). */
-#define virtio_mb(vq) \
- do { if ((vq)->weak_barriers) smp_mb(); else mb(); } while(0)
-#define virtio_rmb(vq) \
- do { if ((vq)->weak_barriers) smp_rmb(); else rmb(); } while(0)
-#define virtio_wmb(vq) \
- do { if ((vq)->weak_barriers) smp_wmb(); else wmb(); } while(0)
-#else
-/* We must force memory ordering even if guest is UP since host could be
- * running on another CPU, but SMP barriers are defined to barrier() in that
- * configuration. So fall back to mandatory barriers instead. */
-#define virtio_mb(vq) mb()
-#define virtio_rmb(vq) rmb()
-#define virtio_wmb(vq) wmb()
-#endif
-
-#ifdef DEBUG
-/* For development, we want to crash whenever the ring is screwed. */
-#define BAD_RING(_vq, fmt, args...)\
- do {\
- dev_err(&(_vq)->vq.vdev->dev, \
- "%s:"fmt, (_vq)->vq.name, ##args); \
- BUG(); \
- } while (0)
-/* Caller is supposed to guarantee no reentry. */
-#define START_USE(_vq) \
- do {\
- if ((_vq)->in_use) \
- panic("%s:in_use = %i\n", \
- (_vq)->vq.name, (_vq)->in_use); \
- (_vq)->in_use = __LINE__; \
- } while (0)
-#define END_USE(_vq) \
- do { BUG_ON(!(_vq)->in_use); (_vq)->in_use = 0; } while(0)
-#else
-#define BAD_RING(_vq, fmt, args...)\
- do {\
- dev_err(&_vq->vq.vdev->dev, \
- "%s:"fmt, (_vq)->vq.name, ##args); \
- (_vq)->broken = true; \
- } while (0)
-#define START_USE(vq)
-#define END_USE(vq)
-#endif
-
-struct vring_virtqueue
-{
- struct virtqueue vq;
-
- /* Actual memory layout for this queue */
- struct vring vring;
-
- /* Can we use weak barriers? */
- bool weak_barriers;
-
- /* Other side has made a mess, don't try any more. */
- bool broken;
-
- /* Host supports indirect buffers */
- bool indirect;
-
- /* Host publishes avail event idx */
- bool event;
-
- /* Head of free buffer list. */
- unsigned int free_head;
- /* 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);
-
-#ifdef DEBUG
- /* They're supposed to lock for us. */
- unsigned int in_use;
-
- /* Figure out if their kicks are too delayed. */
- bool last_add_time_valid;
- ktime_t last_add_time;
-#endif
-
- /* Tokens for callbacks. */
- void *data[];
-};
-
-#define to_vvq(_vq) container_of(_vq, struct vring_virtqueue, vq)
+#include "vring.h"
/* Set up an indirect table of descriptors and add it to the queue. */
static int vring_add_indirect(struct vring_virtqueue *vq,
diff --git a/drivers/virtio/vring.h b/drivers/virtio/vring.h
new file mode 100644
index 000..b997fc3
--- /dev/null
+++ b/drivers/virtio/vring.h
@@ -0,0 +1,121 @@
+/* Virtio ring implementation.
+ *
+ * Copyright 2007 Rusty Russell IBM Corporation
+ *
+ * This program is free soft
[RFC virtio-next 3/4] virtio_ring: Call callback function even when used ring is empty
From: Sjur Brændeland
Enable option to force call of callback function even if
used ring is empty. This is needed for reversed vring.
Add a helper function __vring_interrupt and add extra
boolean argument for forcing callback when interrupt is called.
The original vring_interrupt semantic and signature is
perserved.
Signed-off-by: Sjur Brændeland
---
drivers/remoteproc/remoteproc_virtio.c |2 +-
drivers/virtio/virtio_ring.c |6 +++---
include/linux/virtio_ring.h|8 +++-
3 files changed, 11 insertions(+), 5 deletions(-)
diff --git a/drivers/remoteproc/remoteproc_virtio.c
b/drivers/remoteproc/remoteproc_virtio.c
index e7a4780..ddde863 100644
--- a/drivers/remoteproc/remoteproc_virtio.c
+++ b/drivers/remoteproc/remoteproc_virtio.c
@@ -63,7 +63,7 @@ irqreturn_t rproc_vq_interrupt(struct rproc *rproc, int
notifyid)
if (!rvring || !rvring->vq)
return IRQ_NONE;
- return vring_interrupt(0, rvring->vq);
+ return __vring_interrupt(0, rvring->vq, true);
}
EXPORT_SYMBOL(rproc_vq_interrupt);
diff --git a/drivers/virtio/virtio_ring.c b/drivers/virtio/virtio_ring.c
index 9027af6..af85034 100644
--- a/drivers/virtio/virtio_ring.c
+++ b/drivers/virtio/virtio_ring.c
@@ -504,11 +504,11 @@ void *virtqueue_detach_unused_buf(struct virtqueue *_vq)
}
EXPORT_SYMBOL_GPL(virtqueue_detach_unused_buf);
-irqreturn_t vring_interrupt(int irq, void *_vq)
+irqreturn_t __vring_interrupt(int irq, void *_vq, bool force)
{
struct vring_virtqueue *vq = to_vvq(_vq);
- if (!more_used(vq)) {
+ if (!force && !more_used(vq)) {
pr_debug("virtqueue interrupt with no work for %p\n", vq);
return IRQ_NONE;
}
@@ -522,7 +522,7 @@ irqreturn_t vring_interrupt(int irq, void *_vq)
return IRQ_HANDLED;
}
-EXPORT_SYMBOL_GPL(vring_interrupt);
+EXPORT_SYMBOL_GPL(__vring_interrupt);
struct virtqueue *vring_new_virtqueue(unsigned int index,
unsigned int num,
diff --git a/include/linux/virtio_ring.h b/include/linux/virtio_ring.h
index 63c6ea1..ccb7915 100644
--- a/include/linux/virtio_ring.h
+++ b/include/linux/virtio_ring.h
@@ -20,5 +20,11 @@ void vring_del_virtqueue(struct virtqueue *vq);
/* Filter out transport-specific feature bits. */
void vring_transport_features(struct virtio_device *vdev);
-irqreturn_t vring_interrupt(int irq, void *_vq);
+irqreturn_t __vring_interrupt(int irq, void *_vq, bool force);
+
+static inline irqreturn_t vring_interrupt(int irq, void *_vq)
+{
+ return __vring_interrupt(irq, _vq, false);
+}
+
#endif /* _LINUX_VIRTIO_RING_H */
--
1.7.9.5
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
[RFC virtio-next 2/4] include/vring.h: Add support for reversed vritio rings.
From: Sjur Brændeland Add last avilable index to the vring_virtqueue structure, this is done to prepare for implementation of the reversed vring. Signed-off-by: Sjur Brændeland --- drivers/virtio/vring.h |3 +++ 1 file changed, 3 insertions(+) diff --git a/drivers/virtio/vring.h b/drivers/virtio/vring.h index b997fc3..3b53961 100644 --- a/drivers/virtio/vring.h +++ b/drivers/virtio/vring.h @@ -51,6 +51,9 @@ struct vring_virtqueue /* Last used index we've seen. */ u16 last_used_idx; + /* Last avail index seen. NOTE: Only used for reversed rings.*/ + u16 last_avail_idx; + /* How to notify other side. FIXME: commonalize hcalls! */ void (*notify)(struct virtqueue *vq); -- 1.7.9.5 ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
[RFC virtio-next 0/4] Introduce CAIF Virtio and reversed Vrings
This patch-set introduces the CAIF Virtio Link layer. The purpose is to communicate with a remote processor (a modem) over shared memory. Virtio is used as the transport mechanism, and the Remoteproc framework provides configuration and management of the Virtio rings and devices. The modem and Linux host may be on the same SoC, or connected over a shared memory interface such as LLI. Zero-Copy data transport on the modem is primary goal for CAIF Virtio. In order to achieve Zero-Copy the direction of the Virtio rings are flipped in the RX direction. So we have implemented the Virtio access-function similar to what is found in vhost.c. The connected LTE-modem is a multi-processor system with an advanced memory allocator on board. In order to provide zero-copy from the modem to the connected Linux host, the direction of the Virtio rings are reversed. This allows the modem to allocate data-buffers in RX direction and pass them to the Linux host, and recycled buffers to be sent back to the modem. The option of providing pre-allocated buffers in RX direction has been considered, but rejected. The allocation of data buffers happens deep down in the network signaling stack on the LTE modem before it is known what type of data is received. It may be data that is handled within the modem and never sent to the Linux host, or IP traffic going to the host. Pre-allocated Virtio buffers does not fit for this usage. Another issue is that the network traffic pattern may vary, resulting in variation of number and size of buffers allocated from the memory allocator. Dynamic allocation is needed in order to utilize memory properly. Due to this, we decided we had to implement "reversed" vrings. Reversed vrings allows us to minimize the impact on the current memory allocator and buffer handling on the modem. In order to implement reversed rings we have added functions for reading descriptors from the available-ring and adding descriptors to the used-ring. The internal data-structures in virtio_ring.c are moved into a new header file so the data-structures can be accessed by caif_virtio. The data buffers in TX direction are allocated using dma_alloc_coherent(). This allows memory to be allocated from the memory region shared between the Host and modem. In TX direction single linearized TX buffers are added to the vring. In RX direction linearized frames are also used, but multiple descriptors may be linked. This is done to allow maximum efficiency for the LTE modem. This patch set is not yet fully tested and does not handle all negative scenarios correctly. So at this stage we're primarily looking for review comments related to the structure of the Virtio code. There are several options on how to structure this, and feedback is welcomed. Thanks, Sjur Sjur Brændeland (4): virtio: Move definitions to header file vring.h include/vring.h: Add support for reversed vritio rings. virtio_ring: Call callback function even when used ring is empty caif_virtio: Add CAIF over virtio drivers/net/caif/Kconfig |9 + drivers/net/caif/Makefile |3 + drivers/net/caif/caif_virtio.c | 627 drivers/remoteproc/remoteproc_virtio.c |2 +- drivers/virtio/virtio_ring.c | 102 +- drivers/virtio/vring.h | 124 +++ include/linux/virtio_ring.h|8 +- include/uapi/linux/virtio_ids.h|1 + 8 files changed, 776 insertions(+), 100 deletions(-) create mode 100644 drivers/net/caif/caif_virtio.c create mode 100644 drivers/virtio/vring.h -- 1.7.9.5 ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
[PATCH v8 3/3] virtio_console: Remove buffers from out_vq at port removal
From: Sjur Brændeland
Remove buffers from the out-queue when a rproc_serial device
is removed. Rproc_serial communicates with remote processors
that may crash and leave buffers in the out-queue. But the
virtio_console device is not supposed to leave buffers in the
out-queue when a port is removed, so in this case throw a
warning.
Signed-off-by: Sjur Brændeland
---
drivers/char/virtio_console.c | 10 ++
1 file changed, 10 insertions(+)
diff --git a/drivers/char/virtio_console.c b/drivers/char/virtio_console.c
index 9ebadcb..3fa036a 100644
--- a/drivers/char/virtio_console.c
+++ b/drivers/char/virtio_console.c
@@ -1521,6 +1521,16 @@ static void remove_port_data(struct port *port)
/* Remove buffers we queued up for the Host to send us data in. */
while ((buf = virtqueue_detach_unused_buf(port->in_vq)))
free_buf(buf, true);
+
+ /*
+* Check the out-queue for buffers. For VIRTIO_CONSOLE it is a
+* bug if this happens. But for RPROC_SERIAL the remote processor
+* may have crashed, leaving buffers hanging in the out-queue.
+*/
+ while ((buf = virtqueue_detach_unused_buf(port->out_vq))) {
+ WARN_ON_ONCE(!is_rproc_serial(port->portdev->vdev));
+ free_buf(buf, true);
+ }
}
/*
--
1.7.9.5
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
[PATCH v8 2/3] virtio_console: Add support for remoteproc serial
From: Sjur Brændeland
Add a simple serial connection driver called
VIRTIO_ID_RPROC_SERIAL (11) for communicating with a
remote processor in an asymmetric multi-processing
configuration.
This implementation reuses the existing virtio_console
implementation, and adds support for DMA allocation
of data buffers and disables use of tty console and
the virtio control queue.
Signed-off-by: Sjur Brændeland
Acked-by: Amit Shah
---
drivers/char/virtio_console.c | 190 ++-
include/uapi/linux/virtio_ids.h |1 +
2 files changed, 169 insertions(+), 22 deletions(-)
diff --git a/drivers/char/virtio_console.c b/drivers/char/virtio_console.c
index b84390f..9ebadcb 100644
--- a/drivers/char/virtio_console.c
+++ b/drivers/char/virtio_console.c
@@ -37,8 +37,12 @@
#include
#include
#include
+#include
+#include
#include "../tty/hvc/hvc_console.h"
+#define is_rproc_enabled IS_ENABLED(CONFIG_REMOTEPROC)
+
/*
* This is a global struct for storing common data for all the devices
* this driver handles.
@@ -112,6 +116,15 @@ struct port_buffer {
/* offset in the buf from which to consume data */
size_t offset;
+ /* DMA address of buffer */
+ dma_addr_t dma;
+
+ /* Device we got DMA memory from */
+ struct device *dev;
+
+ /* List of pending dma buffers to free */
+ struct list_head list;
+
/* If sgpages == 0 then buf is used */
unsigned int sgpages;
@@ -331,6 +344,11 @@ static bool is_console_port(struct port *port)
return false;
}
+static bool is_rproc_serial(const struct virtio_device *vdev)
+{
+ return is_rproc_enabled && vdev->id.device == VIRTIO_ID_RPROC_SERIAL;
+}
+
static inline bool use_multiport(struct ports_device *portdev)
{
/*
@@ -342,11 +360,13 @@ static inline bool use_multiport(struct ports_device
*portdev)
return portdev->vdev->features[0] & (1 << VIRTIO_CONSOLE_F_MULTIPORT);
}
-static void free_buf(struct port_buffer *buf)
+static DEFINE_SPINLOCK(dma_bufs_lock);
+static LIST_HEAD(pending_free_dma_bufs);
+
+static void free_buf(struct port_buffer *buf, bool can_sleep)
{
unsigned int i;
- kfree(buf->buf);
for (i = 0; i < buf->sgpages; i++) {
struct page *page = sg_page(&buf->sg[i]);
if (!page)
@@ -354,14 +374,57 @@ static void free_buf(struct port_buffer *buf)
put_page(page);
}
+ if (!buf->dev) {
+ kfree(buf->buf);
+ } else if (is_rproc_enabled) {
+ unsigned long flags;
+
+ /* dma_free_coherent requires interrupts to be enabled. */
+ if (!can_sleep) {
+ /* queue up dma-buffers to be freed later */
+ spin_lock_irqsave(&dma_bufs_lock, flags);
+ list_add_tail(&buf->list, &pending_free_dma_bufs);
+ spin_unlock_irqrestore(&dma_bufs_lock, flags);
+ return;
+ }
+ dma_free_coherent(buf->dev, buf->size, buf->buf, buf->dma);
+
+ /* Release device refcnt and allow it to be freed */
+ put_device(buf->dev);
+ }
+
kfree(buf);
}
+static void reclaim_dma_bufs(void)
+{
+ unsigned long flags;
+ struct port_buffer *buf, *tmp;
+ LIST_HEAD(tmp_list);
+
+ if (list_empty(&pending_free_dma_bufs))
+ return;
+
+ /* Create a copy of the pending_free_dma_bufs while holding the lock */
+ spin_lock_irqsave(&dma_bufs_lock, flags);
+ list_cut_position(&tmp_list, &pending_free_dma_bufs,
+ pending_free_dma_bufs.prev);
+ spin_unlock_irqrestore(&dma_bufs_lock, flags);
+
+ /* Release the dma buffers, without irqs enabled */
+ list_for_each_entry_safe(buf, tmp, &tmp_list, list) {
+ list_del(&buf->list);
+ free_buf(buf, true);
+ }
+}
+
static struct port_buffer *alloc_buf(struct virtqueue *vq, size_t buf_size,
int pages)
{
struct port_buffer *buf;
+ reclaim_dma_bufs();
+
/*
* Allocate buffer and the sg list. The sg list array is allocated
* directly after the port_buffer struct.
@@ -373,11 +436,34 @@ static struct port_buffer *alloc_buf(struct virtqueue
*vq, size_t buf_size,
buf->sgpages = pages;
if (pages > 0) {
+ buf->dev = NULL;
buf->buf = NULL;
return buf;
}
- buf->buf = kmalloc(buf_size, GFP_KERNEL);
+ if (is_rproc_serial(vq->vdev)) {
+ /*
+* Allocate DMA memory from ancestor. When a virtio
+* device is created by remoteproc, the DMA memory is
+* associat
[PATCH v8 1/3] virtio_console: Merge struct buffer_token into struct port_buffer
From: Sjur Brændeland
Refactoring the splice functionality by unifying the approach for
sending scatter-lists and regular buffers. This simplifies
buffer handling and reduces code size. Splice will now allocate
a port_buffer and send_buf() and free_buf() can always be used
for any buffer.
Signed-off-by: Sjur Brændeland
Acked-by: Amit Shah
---
drivers/char/virtio_console.c | 129 +
1 file changed, 53 insertions(+), 76 deletions(-)
diff --git a/drivers/char/virtio_console.c b/drivers/char/virtio_console.c
index 4ad8aca..b84390f 100644
--- a/drivers/char/virtio_console.c
+++ b/drivers/char/virtio_console.c
@@ -111,6 +111,12 @@ struct port_buffer {
size_t len;
/* offset in the buf from which to consume data */
size_t offset;
+
+ /* If sgpages == 0 then buf is used */
+ unsigned int sgpages;
+
+ /* sg is used if spages > 0. sg must be the last in is struct */
+ struct scatterlist sg[0];
};
/*
@@ -338,17 +344,39 @@ static inline bool use_multiport(struct ports_device
*portdev)
static void free_buf(struct port_buffer *buf)
{
+ unsigned int i;
+
kfree(buf->buf);
+ for (i = 0; i < buf->sgpages; i++) {
+ struct page *page = sg_page(&buf->sg[i]);
+ if (!page)
+ break;
+ put_page(page);
+ }
+
kfree(buf);
}
-static struct port_buffer *alloc_buf(size_t buf_size)
+static struct port_buffer *alloc_buf(struct virtqueue *vq, size_t buf_size,
+int pages)
{
struct port_buffer *buf;
- buf = kmalloc(sizeof(*buf), GFP_KERNEL);
+ /*
+* Allocate buffer and the sg list. The sg list array is allocated
+* directly after the port_buffer struct.
+*/
+ buf = kmalloc(sizeof(*buf) + sizeof(struct scatterlist) * pages,
+ GFP_KERNEL);
if (!buf)
goto fail;
+
+ buf->sgpages = pages;
+ if (pages > 0) {
+ buf->buf = NULL;
+ return buf;
+ }
+
buf->buf = kmalloc(buf_size, GFP_KERNEL);
if (!buf->buf)
goto free_buf;
@@ -476,52 +504,26 @@ static ssize_t send_control_msg(struct port *port,
unsigned int event,
return 0;
}
-struct buffer_token {
- union {
- void *buf;
- struct scatterlist *sg;
- } u;
- /* If sgpages == 0 then buf is used, else sg is used */
- unsigned int sgpages;
-};
-
-static void reclaim_sg_pages(struct scatterlist *sg, unsigned int nrpages)
-{
- int i;
- struct page *page;
-
- for (i = 0; i < nrpages; i++) {
- page = sg_page(&sg[i]);
- if (!page)
- break;
- put_page(page);
- }
- kfree(sg);
-}
/* Callers must take the port->outvq_lock */
static void reclaim_consumed_buffers(struct port *port)
{
- struct buffer_token *tok;
+ struct port_buffer *buf;
unsigned int len;
if (!port->portdev) {
/* Device has been unplugged. vqs are already gone. */
return;
}
- while ((tok = virtqueue_get_buf(port->out_vq, &len))) {
- if (tok->sgpages)
- reclaim_sg_pages(tok->u.sg, tok->sgpages);
- else
- kfree(tok->u.buf);
- kfree(tok);
+ while ((buf = virtqueue_get_buf(port->out_vq, &len))) {
+ free_buf(buf);
port->outvq_full = false;
}
}
static ssize_t __send_to_port(struct port *port, struct scatterlist *sg,
int nents, size_t in_count,
- struct buffer_token *tok, bool nonblock)
+ void *data, bool nonblock)
{
struct virtqueue *out_vq;
int err;
@@ -534,7 +536,7 @@ static ssize_t __send_to_port(struct port *port, struct
scatterlist *sg,
reclaim_consumed_buffers(port);
- err = virtqueue_add_buf(out_vq, sg, nents, 0, tok, GFP_ATOMIC);
+ err = virtqueue_add_buf(out_vq, sg, nents, 0, data, GFP_ATOMIC);
/* Tell Host to go! */
virtqueue_kick(out_vq);
@@ -572,37 +574,6 @@ done:
return in_count;
}
-static ssize_t send_buf(struct port *port, void *in_buf, size_t in_count,
- bool nonblock)
-{
- struct scatterlist sg[1];
- struct buffer_token *tok;
-
- tok = kmalloc(sizeof(*tok), GFP_ATOMIC);
- if (!tok)
- return -ENOMEM;
- tok->sgpages = 0;
- tok->u.buf = in_buf;
-
- sg_init_one(sg, in_buf, in_count);
-
- return __send_to_port(port, sg, 1, in_count, tok, nonblock);
-}
-
-static ssize_t send_pages(struct port *port, struct scatterlist *sg, int nents,
-
[PATCHv8 0/3]virtio_console: Add rproc_serial driver
From: Sjur Brændeland This patch-set introduces a new virtio type "rproc_serial" for communicating with remote processors over shared memory. The driver depends on the the remoteproc framework. As preparation for introducing "rproc_serial" I've done a refactoring of the transmit buffer handling. Changes since v7: - Rebased to virtio-next branch. - Removed extra added lines. - Removed superfluous checks before calling reclaim_dma_bufs(). - Rusty raised some question regarding garbage collection of the out-vq, so I moved this into a separate patch. Thanks, Sjur Sjur Brændeland (3): virtio_console: Merge struct buffer_token into struct port_buffer virtio_console: Add support for remoteproc serial virtio_console: Remove buffers from out_vq at port removal drivers/char/virtio_console.c | 325 +++ include/uapi/linux/virtio_ids.h |1 + 2 files changed, 230 insertions(+), 96 deletions(-) -- 1.7.9.5 ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCHv7 4/4] virtio_console: Add support for remoteproc serial
Hi Rusty,
> The free-outside-interrupt issue is usually dealt with by offloading to
> a wq, but your variant works (and isn't too ugly).
Ok, thanks.
>> +static void reclaim_dma_bufs(void)
>> +{
>> + unsigned long flags;
>> + struct port_buffer *buf, *tmp;
>> + LIST_HEAD(tmp_list);
>> +
>> + if (list_empty(&pending_free_dma_bufs))
>> + return;
...
> Looks like this should be an easy noop even if !is_rproc_serial.
Ok, I'll drop the superfluous check before calling reclaim_dma_bufs().
>> @@ -1415,7 +1524,16 @@ static void remove_port_data(struct port *port)
>>
>> /* Remove buffers we queued up for the Host to send us data in. */
>> while ((buf = virtqueue_detach_unused_buf(port->in_vq)))
>> - free_buf(buf);
>> + free_buf(buf, true);
>> +
>> + /*
>> + * Remove buffers from out queue for rproc-serial. We cannot afford
>> + * to leak any DMA mem, so reclaim this memory even if this might be
>> + * racy for the remote processor.
>> + */
>> + if (is_rproc_serial(port->portdev->vdev))
>> + while ((buf = virtqueue_detach_unused_buf(port->out_vq)))
>> + free_buf(buf, true);
>> }
>
> This seems wrong; either this is needed even if !is_rproc_serial(), or
> it's not necessary as the out_vq is empty.
>
> Every path I can see has the device reset (in which case the queues
> should not be active), or we got a VIRTIO_CONSOLE_PORT_REMOVE event (in
> which case, the same).
>
> I think we can have non-blocking writes which could leave buffers in
> out_vq: Amit?
Hm, the remote device could potentially freeze whenever. So I think we
should handle the situation where buffer are stuck in the out-queue for
the rproc_serial device. But I'll move this piece of code into a new
follow-up patch so we can handle this issue separately.
Thanks,
Sjur
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [RFC 1/2] virtio_console: Add support for DMA memory allocation
Hi Michael.
>> Not just fail to work, the kernel will panic on the BUG_ON().
>> Remoteproc gets the virtio configuration from firmware loaded
>> from user space. So this type of problem might be triggered
>> for other virtio drivers as well.
>
> how?
...
>> Even if we fix this particular problem, the general problem
>> still exists: bogus virtio declarations in remoteproc's firmware
>> may cause BUG_ON().
>
> which BUG_ON exactly?
I am afraid I have been barking up the wrong tree here.
Please ignore my previous rambling about panics related
to device features.
I hit the BUG() in virtio_check_driver_offered_feature():
First I did not declare the feature because DMA was not set:
static unsigned int features[] = {
...
#if VIRTIO_CONSOLE_HAS_DMA
VIRTIO_CONSOLE_F_DMA_MEM,
#endif
};
and then in probe I checked if the feature was supported:
virtio_has_feature(vdev, VIRTIO_CONSOLE_F_DMA_MEM)
This triggered the BUG() in virtio_check_driver_offered_feature(),
because the driver was asking for a unknown-feature.
I can get avoid this by simply checking the devices feature bits directly
instead of using virtio_has_feature().
Regards,
Sjur
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [RFC 1/2] virtio_console: Add support for DMA memory allocation
Hi Ohad, >> A simplest thing to do is change dev id. rusty? > > For generic usage, this is correct. But my opinion is that fallback on > feature non-ack is quality-of-implementation issue: great to have, but > there are cases where you just want to fail with "you're too old". > > And in this case, an old system simply will never work. So it's a > question of how graceful the failure is. > > Can your userspace loader can refuse to proceed if the driver doesn't > ack the bits? If so, it's simpler than a whole new ID. Ohad: Are there any way we could avoid starting up the remote processor if mandatory virtio features (such as DMA memory) are not supported by the virtio-driver? Regards, Sjur ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [RFC 1/2] virtio_console: Add support for DMA memory allocation
Hi Michael.
>> If the device then asks for VIRTIO_CONSOLE_F_DMA_MEM
>> when DMA is not supported, virtio will do BUG_ON() from
>> virtio_check_driver_offered_feature().
>>
>> Is this acceptable or should we add a check in virtcons_probe()
>> and let the probing fail instead?
>>
>> E.g:
>> /* Refuse to bind if F_DMA_MEM request cannot be met */
>> if (!VIRTIO_CONSOLE_HAS_DMA &&
>> (vdev->config->get_features(vdev) & (1 <<
>> VIRTIO_CONSOLE_F_DMA_MEM))){
>> dev_err(&vdev->dev,
>> "DMA_MEM requested, but arch does not support DMA\n");
>> err = -EINVAL;
>> goto fail;
>> }
>>
>> Regards,
>> Sjur
>
> Failing probe would be cleaner. But there is still a problem:
> old driver will happily bind to that device and then
> fail to work, right?
Not just fail to work, the kernel will panic on the BUG_ON().
Remoteproc gets the virtio configuration from firmware loaded
from user space. So this type of problem might be triggered
for other virtio drivers as well.
> virtio pci has revision id for this, but remoteproc doesn't
> seem to have anything similar. Or did I miss it?
No there are currently no sanity check of
virtio type and feature bits in remoteproc.
One option may be to add this...
> If not -
> we probably need to use a different
> device id, and not a feature bit.
But if I create a new virtio console type, remoteproc
could still call the existing virtio_console with random
bad feature bits, causing kernel panic.
Even if we fix this particular problem, the general problem
still exists: bogus virtio declarations in remoteproc's firmware
may cause BUG_ON(). (Note the fundamental difference
between visualizations and remoteproc. For remoteproc
the virtio configuration comes from binaries loaded from
user space).
So maybe we should look for a more generic solution, e.g.
changing virtio probe functionality so that devices with
bad feature bits will not trigger BUG_ON(), but rather refuse
to bind the driver.
Regards,
Sjur
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [RFC 1/2] virtio_console: Add support for DMA memory allocation
> The driver certainly shouldn't offer VIRTIO_CONSOLE_F_DMA_MEM if you
> don't have DMA!
OK, so the feature table could be done like this:
static unsigned int features[] = {
VIRTIO_CONSOLE_F_SIZE,
VIRTIO_CONSOLE_F_MULTIPORT,
#if VIRTIO_CONSOLE_HAS_DMA
VIRTIO_CONSOLE_F_DMA_MEM,
#endif
}
If the device then asks for VIRTIO_CONSOLE_F_DMA_MEM
when DMA is not supported, virtio will do BUG_ON() from
virtio_check_driver_offered_feature().
Is this acceptable or should we add a check in virtcons_probe()
and let the probing fail instead?
E.g:
/* Refuse to bind if F_DMA_MEM request cannot be met */
if (!VIRTIO_CONSOLE_HAS_DMA &&
(vdev->config->get_features(vdev) & (1 <<
VIRTIO_CONSOLE_F_DMA_MEM))){
dev_err(&vdev->dev,
"DMA_MEM requested, but arch does not support DMA\n");
err = -EINVAL;
goto fail;
}
Regards,
Sjur
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [RFC 1/2] virtio_console: Add support for DMA memory allocation
Hi Michael, > Exactly. Though if we just fail load it will be much less code. > > Generally, using a feature bit for this is a bit of a problem though: > normally driver is expected to be able to simply ignore > a feature bit. In this case driver is required to > do something so a feature bit is not a good fit. > I am not sure what the right thing to do is. I see - so in order to avoid the binding between driver and device there are two options I guess. Either make virtio_dev_match() or virtcons_probe() fail. Neither of them seems like the obvious choice. Maybe adding a check for VIRTIO_CONSOLE_F_DMA_MEM match between device and driver in virtcons_probe() is the lesser evil? Regards, Sjur ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH] virtio: Don't access device data after unregistration.
Hi Michael,
>> >> Fix panic in virtio.c when CONFIG_DEBUG_SLAB is set.
>> >
>> > What's the root cause of the panic?
>>
>> I believe the cause of the panic is calling
>> ida_simple_remove(&virtio_index_ida, dev->index);
>> when the dev structure is "poisoned" after kfree.
>> It might be the "BUG_ON((int)id < 0)" that bites...
>>
>> >> Use device_del() and put_device() instead of
>> >> device_unregister(), and access device data before
>> >> calling put_device().
>>
>> > Why does this help? Does device_unregister free the
>> > device so dev->index access crashes?
>>
>> Yes, if device ref-count is one when calling unregister
>> the device is freed.
>
> Interesting. Where exactly?...
I was wrong here, the reason is not related to ref-count being
above one. The reason this issue do not show up in virtio_pci
is that the release function is a dummy:
[snip]
static void virtio_pci_release_dev(struct device *_d)
{
/*
* No need for a release method as we allocate/free
* all devices together with the pci devices.
* Provide an empty one to avoid getting a warning from core.
*/
}
The device structure uses a kref for reference counting the device.
In virtio_pci() the release function virtio_pci_release_dev()
will be called when the device is unregistered, but because the
release function is dummy, data isn't freed or reset at this point.
So for virtio devices created from virtio_pci my patch is not
currently needed.
However, empty release functions are not the preferred way, e.g look at
https://lkml.org/lkml/2012/4/3/301
[Greg K.H:]
> > > > +static void hsi_port_release(struct device *dev __maybe_unused)
> > > > +{
> > > > +}
> > >
> > > As per the documentation in the kernel tree, I get to mock you
> > > mercilessly for doing something as foolish as this. You are not smarter
> > > than the kernel and don't think that you got rid of the kernel warning
> > > properly by doing this. Do you think that I wrote that code for no good
> > > reason? The kernel was being nice and telling you what you did wrong,
> > > don't try to fake it out, it's smarter than you are here.
But remoteproc frees the device memory in the release function
rproc_vdev_release() and needs this patch.
Regards,
Sjur
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [RFC 1/2] virtio_console: Add support for DMA memory allocation
Hi Michael,
>> If an architecture do not support DMA you will get
>> a link error: "unknown symbol" for dma_alloc_coherent.
>
> Yes, it even seems intentional.
> But I have a question: can the device work without DMA?
The main dependency is actually on the dma-allocation.
In my case I do dma_declare_coherent_memory() with
the memory area shared with the remote device as argument.
Subsequent calls to dma_alloc_coherent() will then allocate
from this memory area.
> Does not VIRTIO_CONSOLE_F_DMA_MEM mean dma api is required?
Yes.dma_alloc_coherent to work.
> If yes you should just fail load.
Agree, I'll check on VIRTIO_CONSOLE_HAS_DMA before adding
VIRTIO_CONSOLE_F_DMA_MEM to the feature array.
> Also let's add a wrapper at top of file so ifdefs
> do not litter the code like this. For example:
>
> #ifdef CONFIG_HAS_DMA
> #define VIRTIO_CONSOLE_HAS_DMA (1)
> #else
> #define VIRTIO_CONSOLE_HAS_DMA (0)
> #endif
OK, good point. Perhaps we could even exploit gcc's
ability to remove dead code and do something like this:
if (VIRTIO_CONSOLE_HAS_DMA &&
virtio_has_feature(vdev, VIRTIO_CONSOLE_F_DMA_MEM)) {
...
This does not give any linker warnings when compiling for UML (no DMA).
Regards,
Sjur
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [RFC 1/2] virtio_console: Add support for DMA memory allocation
Hi Michael,
> How does access to descriptors work in this setup?
When the ring is setup by remoteproc the descriptors are
also allocated using dma_alloc_coherent().
>> -static void free_buf(struct port_buffer *buf)
>> +/* Allcoate data buffer from DMA memory if requested */
>
> typo
Thanks.
>> +static inline void *
>> +alloc_databuf(struct virtio_device *vdev, size_t size, dma_addr_t
>> *dma_handle,
>> +gfp_t flag)
>> {
>> - kfree(buf->buf);
>> +#ifdef CONFIG_HAS_DMA
>> + if (virtio_has_feature(vdev, VIRTIO_CONSOLE_F_DMA_MEM)) {
>> + struct device *dev = &vdev->dev;
>> + /*
>> + * Allocate DMA memory from ancestors. Finding the ancestor
>> + * is a bit quirky when DMA_MEMORY_INCLUDES_CHILDREN is not
>> + * implemented.
>> + */
>> + dev = dev->parent ? dev->parent : dev;
>> + dev = dev->parent ? dev->parent : dev;
>> + return dma_alloc_coherent(dev, size, dma_handle, flag);
>> + }
>> +#endif
>
> Are these ifdefs really needed? If DMA_MEM is set,
> can't we use dma_alloc_coherent
> unconditionally?
If an architecture do not support DMA you will get
a link error: "unknown symbol" for dma_alloc_coherent.
Regards,
Sjur
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH] virtio: Don't access device data after unregistration.
Hi Michael, >> Fix panic in virtio.c when CONFIG_DEBUG_SLAB is set. > > What's the root cause of the panic? I believe the cause of the panic is calling ida_simple_remove(&virtio_index_ida, dev->index); when the dev structure is "poisoned" after kfree. It might be the "BUG_ON((int)id < 0)" that bites... >> Use device_del() and put_device() instead of >> device_unregister(), and access device data before >> calling put_device(). > Why does this help? Does device_unregister free the > device so dev->index access crashes? Yes, if device ref-count is one when calling unregister the device is freed. > If yes virtio_pci_remove will crash too > as it accesses the device after the > call to unregister_virtio_device so the > fix won't be effective. I discovered this using the remoteproc framework. It might be that device is unregistered with ref-count greater than one normally, in that case this bug will not show up. Regards, Sjur ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
