* [Qemu-devel] [PATCH] virtio_rpmsg: set DRIVER_OK before using device
@ 2015-03-07 19:06 ` Michael S. Tsirkin
0 siblings, 0 replies; 18+ messages in thread
From: Michael S. Tsirkin @ 2015-03-07 19:06 UTC (permalink / raw)
To: linux-kernel
Cc: Ohad Ben-Cohen, Rusty Russell, qemu-devel, kvm, virtualization
virtio spec requires that all drivers set DRIVER_OK
before using devices. While rpmsg isn't yet
included in the virtio 1 spec, previous spec versions
also required this.
virtio rpmsg violates this rule: is calls kick
before setting DRIVER_OK.
The fix isn't trivial since simply calling virtio_device_ready earlier
would mean we might get an interrupt in parallel with adding buffers.
Instead, split kick out to prepare+notify calls. prepare before
virtio_device_ready - when we know we won't get interrupts. notify right
afterwards.
Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
---
Note: compile-tested only.
drivers/rpmsg/virtio_rpmsg_bus.c | 17 ++++++++++++++++-
1 file changed, 16 insertions(+), 1 deletion(-)
diff --git a/drivers/rpmsg/virtio_rpmsg_bus.c b/drivers/rpmsg/virtio_rpmsg_bus.c
index 92f6af6..73354ee 100644
--- a/drivers/rpmsg/virtio_rpmsg_bus.c
+++ b/drivers/rpmsg/virtio_rpmsg_bus.c
@@ -951,6 +951,7 @@ static int rpmsg_probe(struct virtio_device *vdev)
void *bufs_va;
int err = 0, i;
size_t total_buf_space;
+ bool notify;
vrp = kzalloc(sizeof(*vrp), GFP_KERNEL);
if (!vrp)
@@ -1030,8 +1031,22 @@ static int rpmsg_probe(struct virtio_device *vdev)
}
}
+ /*
+ * Prepare to kick but don't notify yet - we can't do this before
+ * device is ready.
+ */
+ notify = virtqueue_kick_prepare(vrp->rvq);
+
+ /* From this point on, we can notify and get callbacks. */
+ virtio_device_ready(vdev);
+
/* tell the remote processor it can start sending messages */
- virtqueue_kick(vrp->rvq);
+ /*
+ * this might be concurrent with callbacks, but we are only
+ * doing notify, not a full kick here, so that's ok.
+ */
+ if (notify)
+ virtqueue_notify(vrp->rvq);
dev_info(&vdev->dev, "rpmsg host is online\n");
--
MST
^ permalink raw reply related [flat|nested] 18+ messages in thread
* [PATCH] virtio_rpmsg: set DRIVER_OK before using device
@ 2015-03-07 19:06 ` Michael S. Tsirkin
0 siblings, 0 replies; 18+ messages in thread
From: Michael S. Tsirkin @ 2015-03-07 19:06 UTC (permalink / raw)
To: linux-kernel; +Cc: qemu-devel, kvm, virtualization
virtio spec requires that all drivers set DRIVER_OK
before using devices. While rpmsg isn't yet
included in the virtio 1 spec, previous spec versions
also required this.
virtio rpmsg violates this rule: is calls kick
before setting DRIVER_OK.
The fix isn't trivial since simply calling virtio_device_ready earlier
would mean we might get an interrupt in parallel with adding buffers.
Instead, split kick out to prepare+notify calls. prepare before
virtio_device_ready - when we know we won't get interrupts. notify right
afterwards.
Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
---
Note: compile-tested only.
drivers/rpmsg/virtio_rpmsg_bus.c | 17 ++++++++++++++++-
1 file changed, 16 insertions(+), 1 deletion(-)
diff --git a/drivers/rpmsg/virtio_rpmsg_bus.c b/drivers/rpmsg/virtio_rpmsg_bus.c
index 92f6af6..73354ee 100644
--- a/drivers/rpmsg/virtio_rpmsg_bus.c
+++ b/drivers/rpmsg/virtio_rpmsg_bus.c
@@ -951,6 +951,7 @@ static int rpmsg_probe(struct virtio_device *vdev)
void *bufs_va;
int err = 0, i;
size_t total_buf_space;
+ bool notify;
vrp = kzalloc(sizeof(*vrp), GFP_KERNEL);
if (!vrp)
@@ -1030,8 +1031,22 @@ static int rpmsg_probe(struct virtio_device *vdev)
}
}
+ /*
+ * Prepare to kick but don't notify yet - we can't do this before
+ * device is ready.
+ */
+ notify = virtqueue_kick_prepare(vrp->rvq);
+
+ /* From this point on, we can notify and get callbacks. */
+ virtio_device_ready(vdev);
+
/* tell the remote processor it can start sending messages */
- virtqueue_kick(vrp->rvq);
+ /*
+ * this might be concurrent with callbacks, but we are only
+ * doing notify, not a full kick here, so that's ok.
+ */
+ if (notify)
+ virtqueue_notify(vrp->rvq);
dev_info(&vdev->dev, "rpmsg host is online\n");
--
MST
^ permalink raw reply related [flat|nested] 18+ messages in thread
* Re: [PATCH] virtio_rpmsg: set DRIVER_OK before using device
2015-03-07 19:06 ` Michael S. Tsirkin
@ 2015-03-09 7:09 ` Rusty Russell
-1 siblings, 0 replies; 18+ messages in thread
From: Rusty Russell @ 2015-03-09 7:09 UTC (permalink / raw)
To: Michael S. Tsirkin, linux-kernel
Cc: virtualization, kvm, qemu-devel, Ohad Ben-Cohen
"Michael S. Tsirkin" <mst@redhat.com> writes:
> virtio spec requires that all drivers set DRIVER_OK
> before using devices. While rpmsg isn't yet
> included in the virtio 1 spec, previous spec versions
> also required this.
>
> virtio rpmsg violates this rule: is calls kick
> before setting DRIVER_OK.
>
> The fix isn't trivial since simply calling virtio_device_ready earlier
> would mean we might get an interrupt in parallel with adding buffers.
>
> Instead, split kick out to prepare+notify calls. prepare before
> virtio_device_ready - when we know we won't get interrupts. notify right
> afterwards.
>
> Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
Applied. I'll wait for Ohad to ack before sending to Linus.
BTW I assume you have a version of qemu which warns on these kind of
failures? That'd be nice to have!
Thanks,
Rusty.
> ---
>
> Note: compile-tested only.
>
> drivers/rpmsg/virtio_rpmsg_bus.c | 17 ++++++++++++++++-
> 1 file changed, 16 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/rpmsg/virtio_rpmsg_bus.c b/drivers/rpmsg/virtio_rpmsg_bus.c
> index 92f6af6..73354ee 100644
> --- a/drivers/rpmsg/virtio_rpmsg_bus.c
> +++ b/drivers/rpmsg/virtio_rpmsg_bus.c
> @@ -951,6 +951,7 @@ static int rpmsg_probe(struct virtio_device *vdev)
> void *bufs_va;
> int err = 0, i;
> size_t total_buf_space;
> + bool notify;
>
> vrp = kzalloc(sizeof(*vrp), GFP_KERNEL);
> if (!vrp)
> @@ -1030,8 +1031,22 @@ static int rpmsg_probe(struct virtio_device *vdev)
> }
> }
>
> + /*
> + * Prepare to kick but don't notify yet - we can't do this before
> + * device is ready.
> + */
> + notify = virtqueue_kick_prepare(vrp->rvq);
> +
> + /* From this point on, we can notify and get callbacks. */
> + virtio_device_ready(vdev);
> +
> /* tell the remote processor it can start sending messages */
> - virtqueue_kick(vrp->rvq);
> + /*
> + * this might be concurrent with callbacks, but we are only
> + * doing notify, not a full kick here, so that's ok.
> + */
> + if (notify)
> + virtqueue_notify(vrp->rvq);
>
> dev_info(&vdev->dev, "rpmsg host is online\n");
>
> --
> MST
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [Qemu-devel] [PATCH] virtio_rpmsg: set DRIVER_OK before using device
@ 2015-03-09 7:09 ` Rusty Russell
0 siblings, 0 replies; 18+ messages in thread
From: Rusty Russell @ 2015-03-09 7:09 UTC (permalink / raw)
To: Michael S. Tsirkin, linux-kernel
Cc: Ohad Ben-Cohen, qemu-devel, kvm, virtualization
"Michael S. Tsirkin" <mst@redhat.com> writes:
> virtio spec requires that all drivers set DRIVER_OK
> before using devices. While rpmsg isn't yet
> included in the virtio 1 spec, previous spec versions
> also required this.
>
> virtio rpmsg violates this rule: is calls kick
> before setting DRIVER_OK.
>
> The fix isn't trivial since simply calling virtio_device_ready earlier
> would mean we might get an interrupt in parallel with adding buffers.
>
> Instead, split kick out to prepare+notify calls. prepare before
> virtio_device_ready - when we know we won't get interrupts. notify right
> afterwards.
>
> Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
Applied. I'll wait for Ohad to ack before sending to Linus.
BTW I assume you have a version of qemu which warns on these kind of
failures? That'd be nice to have!
Thanks,
Rusty.
> ---
>
> Note: compile-tested only.
>
> drivers/rpmsg/virtio_rpmsg_bus.c | 17 ++++++++++++++++-
> 1 file changed, 16 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/rpmsg/virtio_rpmsg_bus.c b/drivers/rpmsg/virtio_rpmsg_bus.c
> index 92f6af6..73354ee 100644
> --- a/drivers/rpmsg/virtio_rpmsg_bus.c
> +++ b/drivers/rpmsg/virtio_rpmsg_bus.c
> @@ -951,6 +951,7 @@ static int rpmsg_probe(struct virtio_device *vdev)
> void *bufs_va;
> int err = 0, i;
> size_t total_buf_space;
> + bool notify;
>
> vrp = kzalloc(sizeof(*vrp), GFP_KERNEL);
> if (!vrp)
> @@ -1030,8 +1031,22 @@ static int rpmsg_probe(struct virtio_device *vdev)
> }
> }
>
> + /*
> + * Prepare to kick but don't notify yet - we can't do this before
> + * device is ready.
> + */
> + notify = virtqueue_kick_prepare(vrp->rvq);
> +
> + /* From this point on, we can notify and get callbacks. */
> + virtio_device_ready(vdev);
> +
> /* tell the remote processor it can start sending messages */
> - virtqueue_kick(vrp->rvq);
> + /*
> + * this might be concurrent with callbacks, but we are only
> + * doing notify, not a full kick here, so that's ok.
> + */
> + if (notify)
> + virtqueue_notify(vrp->rvq);
>
> dev_info(&vdev->dev, "rpmsg host is online\n");
>
> --
> MST
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH] virtio_rpmsg: set DRIVER_OK before using device
2015-03-09 7:09 ` [Qemu-devel] " Rusty Russell
(?)
@ 2015-03-09 8:27 ` Michael S. Tsirkin
-1 siblings, 0 replies; 18+ messages in thread
From: Michael S. Tsirkin @ 2015-03-09 8:27 UTC (permalink / raw)
To: Rusty Russell
Cc: linux-kernel, virtualization, kvm, qemu-devel, Ohad Ben-Cohen
On Mon, Mar 09, 2015 at 05:39:20PM +1030, Rusty Russell wrote:
> "Michael S. Tsirkin" <mst@redhat.com> writes:
> > virtio spec requires that all drivers set DRIVER_OK
> > before using devices. While rpmsg isn't yet
> > included in the virtio 1 spec, previous spec versions
> > also required this.
> >
> > virtio rpmsg violates this rule: is calls kick
> > before setting DRIVER_OK.
> >
> > The fix isn't trivial since simply calling virtio_device_ready earlier
> > would mean we might get an interrupt in parallel with adding buffers.
> >
> > Instead, split kick out to prepare+notify calls. prepare before
> > virtio_device_ready - when we know we won't get interrupts. notify right
> > afterwards.
> >
> > Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
>
> Applied. I'll wait for Ohad to ack before sending to Linus.
>
> BTW I assume you have a version of qemu which warns on these kind of
> failures? That'd be nice to have!
>
> Thanks,
> Rusty.
Yes but it's hacky ATM - it's just a one-liner that checks
the status on kick and does fprintf(stderr).
I'm trying to rework the code so it'll actually
set the status automatically, but there are some difficulties there
when ioeventfd is used,
in particular we need a way to re-inject the kick
after status update.
> > ---
> >
> > Note: compile-tested only.
> >
> > drivers/rpmsg/virtio_rpmsg_bus.c | 17 ++++++++++++++++-
> > 1 file changed, 16 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/rpmsg/virtio_rpmsg_bus.c b/drivers/rpmsg/virtio_rpmsg_bus.c
> > index 92f6af6..73354ee 100644
> > --- a/drivers/rpmsg/virtio_rpmsg_bus.c
> > +++ b/drivers/rpmsg/virtio_rpmsg_bus.c
> > @@ -951,6 +951,7 @@ static int rpmsg_probe(struct virtio_device *vdev)
> > void *bufs_va;
> > int err = 0, i;
> > size_t total_buf_space;
> > + bool notify;
> >
> > vrp = kzalloc(sizeof(*vrp), GFP_KERNEL);
> > if (!vrp)
> > @@ -1030,8 +1031,22 @@ static int rpmsg_probe(struct virtio_device *vdev)
> > }
> > }
> >
> > + /*
> > + * Prepare to kick but don't notify yet - we can't do this before
> > + * device is ready.
> > + */
> > + notify = virtqueue_kick_prepare(vrp->rvq);
> > +
> > + /* From this point on, we can notify and get callbacks. */
> > + virtio_device_ready(vdev);
> > +
> > /* tell the remote processor it can start sending messages */
> > - virtqueue_kick(vrp->rvq);
> > + /*
> > + * this might be concurrent with callbacks, but we are only
> > + * doing notify, not a full kick here, so that's ok.
> > + */
> > + if (notify)
> > + virtqueue_notify(vrp->rvq);
> >
> > dev_info(&vdev->dev, "rpmsg host is online\n");
> >
> > --
> > MST
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [Qemu-devel] [PATCH] virtio_rpmsg: set DRIVER_OK before using device
@ 2015-03-09 8:27 ` Michael S. Tsirkin
0 siblings, 0 replies; 18+ messages in thread
From: Michael S. Tsirkin @ 2015-03-09 8:27 UTC (permalink / raw)
To: Rusty Russell
Cc: qemu-devel, Ohad Ben-Cohen, linux-kernel, kvm, virtualization
On Mon, Mar 09, 2015 at 05:39:20PM +1030, Rusty Russell wrote:
> "Michael S. Tsirkin" <mst@redhat.com> writes:
> > virtio spec requires that all drivers set DRIVER_OK
> > before using devices. While rpmsg isn't yet
> > included in the virtio 1 spec, previous spec versions
> > also required this.
> >
> > virtio rpmsg violates this rule: is calls kick
> > before setting DRIVER_OK.
> >
> > The fix isn't trivial since simply calling virtio_device_ready earlier
> > would mean we might get an interrupt in parallel with adding buffers.
> >
> > Instead, split kick out to prepare+notify calls. prepare before
> > virtio_device_ready - when we know we won't get interrupts. notify right
> > afterwards.
> >
> > Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
>
> Applied. I'll wait for Ohad to ack before sending to Linus.
>
> BTW I assume you have a version of qemu which warns on these kind of
> failures? That'd be nice to have!
>
> Thanks,
> Rusty.
Yes but it's hacky ATM - it's just a one-liner that checks
the status on kick and does fprintf(stderr).
I'm trying to rework the code so it'll actually
set the status automatically, but there are some difficulties there
when ioeventfd is used,
in particular we need a way to re-inject the kick
after status update.
> > ---
> >
> > Note: compile-tested only.
> >
> > drivers/rpmsg/virtio_rpmsg_bus.c | 17 ++++++++++++++++-
> > 1 file changed, 16 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/rpmsg/virtio_rpmsg_bus.c b/drivers/rpmsg/virtio_rpmsg_bus.c
> > index 92f6af6..73354ee 100644
> > --- a/drivers/rpmsg/virtio_rpmsg_bus.c
> > +++ b/drivers/rpmsg/virtio_rpmsg_bus.c
> > @@ -951,6 +951,7 @@ static int rpmsg_probe(struct virtio_device *vdev)
> > void *bufs_va;
> > int err = 0, i;
> > size_t total_buf_space;
> > + bool notify;
> >
> > vrp = kzalloc(sizeof(*vrp), GFP_KERNEL);
> > if (!vrp)
> > @@ -1030,8 +1031,22 @@ static int rpmsg_probe(struct virtio_device *vdev)
> > }
> > }
> >
> > + /*
> > + * Prepare to kick but don't notify yet - we can't do this before
> > + * device is ready.
> > + */
> > + notify = virtqueue_kick_prepare(vrp->rvq);
> > +
> > + /* From this point on, we can notify and get callbacks. */
> > + virtio_device_ready(vdev);
> > +
> > /* tell the remote processor it can start sending messages */
> > - virtqueue_kick(vrp->rvq);
> > + /*
> > + * this might be concurrent with callbacks, but we are only
> > + * doing notify, not a full kick here, so that's ok.
> > + */
> > + if (notify)
> > + virtqueue_notify(vrp->rvq);
> >
> > dev_info(&vdev->dev, "rpmsg host is online\n");
> >
> > --
> > MST
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH] virtio_rpmsg: set DRIVER_OK before using device
@ 2015-03-09 8:27 ` Michael S. Tsirkin
0 siblings, 0 replies; 18+ messages in thread
From: Michael S. Tsirkin @ 2015-03-09 8:27 UTC (permalink / raw)
To: Rusty Russell; +Cc: qemu-devel, linux-kernel, kvm, virtualization
On Mon, Mar 09, 2015 at 05:39:20PM +1030, Rusty Russell wrote:
> "Michael S. Tsirkin" <mst@redhat.com> writes:
> > virtio spec requires that all drivers set DRIVER_OK
> > before using devices. While rpmsg isn't yet
> > included in the virtio 1 spec, previous spec versions
> > also required this.
> >
> > virtio rpmsg violates this rule: is calls kick
> > before setting DRIVER_OK.
> >
> > The fix isn't trivial since simply calling virtio_device_ready earlier
> > would mean we might get an interrupt in parallel with adding buffers.
> >
> > Instead, split kick out to prepare+notify calls. prepare before
> > virtio_device_ready - when we know we won't get interrupts. notify right
> > afterwards.
> >
> > Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
>
> Applied. I'll wait for Ohad to ack before sending to Linus.
>
> BTW I assume you have a version of qemu which warns on these kind of
> failures? That'd be nice to have!
>
> Thanks,
> Rusty.
Yes but it's hacky ATM - it's just a one-liner that checks
the status on kick and does fprintf(stderr).
I'm trying to rework the code so it'll actually
set the status automatically, but there are some difficulties there
when ioeventfd is used,
in particular we need a way to re-inject the kick
after status update.
> > ---
> >
> > Note: compile-tested only.
> >
> > drivers/rpmsg/virtio_rpmsg_bus.c | 17 ++++++++++++++++-
> > 1 file changed, 16 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/rpmsg/virtio_rpmsg_bus.c b/drivers/rpmsg/virtio_rpmsg_bus.c
> > index 92f6af6..73354ee 100644
> > --- a/drivers/rpmsg/virtio_rpmsg_bus.c
> > +++ b/drivers/rpmsg/virtio_rpmsg_bus.c
> > @@ -951,6 +951,7 @@ static int rpmsg_probe(struct virtio_device *vdev)
> > void *bufs_va;
> > int err = 0, i;
> > size_t total_buf_space;
> > + bool notify;
> >
> > vrp = kzalloc(sizeof(*vrp), GFP_KERNEL);
> > if (!vrp)
> > @@ -1030,8 +1031,22 @@ static int rpmsg_probe(struct virtio_device *vdev)
> > }
> > }
> >
> > + /*
> > + * Prepare to kick but don't notify yet - we can't do this before
> > + * device is ready.
> > + */
> > + notify = virtqueue_kick_prepare(vrp->rvq);
> > +
> > + /* From this point on, we can notify and get callbacks. */
> > + virtio_device_ready(vdev);
> > +
> > /* tell the remote processor it can start sending messages */
> > - virtqueue_kick(vrp->rvq);
> > + /*
> > + * this might be concurrent with callbacks, but we are only
> > + * doing notify, not a full kick here, so that's ok.
> > + */
> > + if (notify)
> > + virtqueue_notify(vrp->rvq);
> >
> > dev_info(&vdev->dev, "rpmsg host is online\n");
> >
> > --
> > MST
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH] virtio_rpmsg: set DRIVER_OK before using device
2015-03-07 19:06 ` Michael S. Tsirkin
` (2 preceding siblings ...)
(?)
@ 2015-03-09 7:09 ` Rusty Russell
-1 siblings, 0 replies; 18+ messages in thread
From: Rusty Russell @ 2015-03-09 7:09 UTC (permalink / raw)
To: Michael S. Tsirkin, linux-kernel; +Cc: qemu-devel, kvm, virtualization
"Michael S. Tsirkin" <mst@redhat.com> writes:
> virtio spec requires that all drivers set DRIVER_OK
> before using devices. While rpmsg isn't yet
> included in the virtio 1 spec, previous spec versions
> also required this.
>
> virtio rpmsg violates this rule: is calls kick
> before setting DRIVER_OK.
>
> The fix isn't trivial since simply calling virtio_device_ready earlier
> would mean we might get an interrupt in parallel with adding buffers.
>
> Instead, split kick out to prepare+notify calls. prepare before
> virtio_device_ready - when we know we won't get interrupts. notify right
> afterwards.
>
> Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
Applied. I'll wait for Ohad to ack before sending to Linus.
BTW I assume you have a version of qemu which warns on these kind of
failures? That'd be nice to have!
Thanks,
Rusty.
> ---
>
> Note: compile-tested only.
>
> drivers/rpmsg/virtio_rpmsg_bus.c | 17 ++++++++++++++++-
> 1 file changed, 16 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/rpmsg/virtio_rpmsg_bus.c b/drivers/rpmsg/virtio_rpmsg_bus.c
> index 92f6af6..73354ee 100644
> --- a/drivers/rpmsg/virtio_rpmsg_bus.c
> +++ b/drivers/rpmsg/virtio_rpmsg_bus.c
> @@ -951,6 +951,7 @@ static int rpmsg_probe(struct virtio_device *vdev)
> void *bufs_va;
> int err = 0, i;
> size_t total_buf_space;
> + bool notify;
>
> vrp = kzalloc(sizeof(*vrp), GFP_KERNEL);
> if (!vrp)
> @@ -1030,8 +1031,22 @@ static int rpmsg_probe(struct virtio_device *vdev)
> }
> }
>
> + /*
> + * Prepare to kick but don't notify yet - we can't do this before
> + * device is ready.
> + */
> + notify = virtqueue_kick_prepare(vrp->rvq);
> +
> + /* From this point on, we can notify and get callbacks. */
> + virtio_device_ready(vdev);
> +
> /* tell the remote processor it can start sending messages */
> - virtqueue_kick(vrp->rvq);
> + /*
> + * this might be concurrent with callbacks, but we are only
> + * doing notify, not a full kick here, so that's ok.
> + */
> + if (notify)
> + virtqueue_notify(vrp->rvq);
>
> dev_info(&vdev->dev, "rpmsg host is online\n");
>
> --
> MST
^ permalink raw reply [flat|nested] 18+ messages in thread* Re: [PATCH] virtio_rpmsg: set DRIVER_OK before using device
2015-03-07 19:06 ` Michael S. Tsirkin
(?)
@ 2015-03-09 8:41 ` Michael S. Tsirkin
-1 siblings, 0 replies; 18+ messages in thread
From: Michael S. Tsirkin @ 2015-03-09 8:41 UTC (permalink / raw)
To: linux-kernel
Cc: virtualization, Rusty Russell, kvm, qemu-devel, Ohad Ben-Cohen
On Sat, Mar 07, 2015 at 08:06:56PM +0100, Michael S. Tsirkin wrote:
> virtio spec requires that all drivers set DRIVER_OK
> before using devices. While rpmsg isn't yet
> included in the virtio 1 spec, previous spec versions
> also required this.
>
> virtio rpmsg violates this rule: is calls kick
> before setting DRIVER_OK.
>
> The fix isn't trivial since simply calling virtio_device_ready earlier
> would mean we might get an interrupt in parallel with adding buffers.
>
> Instead, split kick out to prepare+notify calls. prepare before
> virtio_device_ready - when we know we won't get interrupts. notify right
> afterwards.
>
> Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> ---
Ohad, can you review and ack pls?
> Note: compile-tested only.
>
> drivers/rpmsg/virtio_rpmsg_bus.c | 17 ++++++++++++++++-
> 1 file changed, 16 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/rpmsg/virtio_rpmsg_bus.c b/drivers/rpmsg/virtio_rpmsg_bus.c
> index 92f6af6..73354ee 100644
> --- a/drivers/rpmsg/virtio_rpmsg_bus.c
> +++ b/drivers/rpmsg/virtio_rpmsg_bus.c
> @@ -951,6 +951,7 @@ static int rpmsg_probe(struct virtio_device *vdev)
> void *bufs_va;
> int err = 0, i;
> size_t total_buf_space;
> + bool notify;
>
> vrp = kzalloc(sizeof(*vrp), GFP_KERNEL);
> if (!vrp)
> @@ -1030,8 +1031,22 @@ static int rpmsg_probe(struct virtio_device *vdev)
> }
> }
>
> + /*
> + * Prepare to kick but don't notify yet - we can't do this before
> + * device is ready.
> + */
> + notify = virtqueue_kick_prepare(vrp->rvq);
> +
> + /* From this point on, we can notify and get callbacks. */
> + virtio_device_ready(vdev);
> +
> /* tell the remote processor it can start sending messages */
> - virtqueue_kick(vrp->rvq);
> + /*
> + * this might be concurrent with callbacks, but we are only
> + * doing notify, not a full kick here, so that's ok.
> + */
> + if (notify)
> + virtqueue_notify(vrp->rvq);
>
> dev_info(&vdev->dev, "rpmsg host is online\n");
>
> --
> MST
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [Qemu-devel] [PATCH] virtio_rpmsg: set DRIVER_OK before using device
@ 2015-03-09 8:41 ` Michael S. Tsirkin
0 siblings, 0 replies; 18+ messages in thread
From: Michael S. Tsirkin @ 2015-03-09 8:41 UTC (permalink / raw)
To: linux-kernel
Cc: Ohad Ben-Cohen, Rusty Russell, qemu-devel, kvm, virtualization
On Sat, Mar 07, 2015 at 08:06:56PM +0100, Michael S. Tsirkin wrote:
> virtio spec requires that all drivers set DRIVER_OK
> before using devices. While rpmsg isn't yet
> included in the virtio 1 spec, previous spec versions
> also required this.
>
> virtio rpmsg violates this rule: is calls kick
> before setting DRIVER_OK.
>
> The fix isn't trivial since simply calling virtio_device_ready earlier
> would mean we might get an interrupt in parallel with adding buffers.
>
> Instead, split kick out to prepare+notify calls. prepare before
> virtio_device_ready - when we know we won't get interrupts. notify right
> afterwards.
>
> Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> ---
Ohad, can you review and ack pls?
> Note: compile-tested only.
>
> drivers/rpmsg/virtio_rpmsg_bus.c | 17 ++++++++++++++++-
> 1 file changed, 16 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/rpmsg/virtio_rpmsg_bus.c b/drivers/rpmsg/virtio_rpmsg_bus.c
> index 92f6af6..73354ee 100644
> --- a/drivers/rpmsg/virtio_rpmsg_bus.c
> +++ b/drivers/rpmsg/virtio_rpmsg_bus.c
> @@ -951,6 +951,7 @@ static int rpmsg_probe(struct virtio_device *vdev)
> void *bufs_va;
> int err = 0, i;
> size_t total_buf_space;
> + bool notify;
>
> vrp = kzalloc(sizeof(*vrp), GFP_KERNEL);
> if (!vrp)
> @@ -1030,8 +1031,22 @@ static int rpmsg_probe(struct virtio_device *vdev)
> }
> }
>
> + /*
> + * Prepare to kick but don't notify yet - we can't do this before
> + * device is ready.
> + */
> + notify = virtqueue_kick_prepare(vrp->rvq);
> +
> + /* From this point on, we can notify and get callbacks. */
> + virtio_device_ready(vdev);
> +
> /* tell the remote processor it can start sending messages */
> - virtqueue_kick(vrp->rvq);
> + /*
> + * this might be concurrent with callbacks, but we are only
> + * doing notify, not a full kick here, so that's ok.
> + */
> + if (notify)
> + virtqueue_notify(vrp->rvq);
>
> dev_info(&vdev->dev, "rpmsg host is online\n");
>
> --
> MST
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH] virtio_rpmsg: set DRIVER_OK before using device
@ 2015-03-09 8:41 ` Michael S. Tsirkin
0 siblings, 0 replies; 18+ messages in thread
From: Michael S. Tsirkin @ 2015-03-09 8:41 UTC (permalink / raw)
To: linux-kernel; +Cc: qemu-devel, kvm, virtualization
On Sat, Mar 07, 2015 at 08:06:56PM +0100, Michael S. Tsirkin wrote:
> virtio spec requires that all drivers set DRIVER_OK
> before using devices. While rpmsg isn't yet
> included in the virtio 1 spec, previous spec versions
> also required this.
>
> virtio rpmsg violates this rule: is calls kick
> before setting DRIVER_OK.
>
> The fix isn't trivial since simply calling virtio_device_ready earlier
> would mean we might get an interrupt in parallel with adding buffers.
>
> Instead, split kick out to prepare+notify calls. prepare before
> virtio_device_ready - when we know we won't get interrupts. notify right
> afterwards.
>
> Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> ---
Ohad, can you review and ack pls?
> Note: compile-tested only.
>
> drivers/rpmsg/virtio_rpmsg_bus.c | 17 ++++++++++++++++-
> 1 file changed, 16 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/rpmsg/virtio_rpmsg_bus.c b/drivers/rpmsg/virtio_rpmsg_bus.c
> index 92f6af6..73354ee 100644
> --- a/drivers/rpmsg/virtio_rpmsg_bus.c
> +++ b/drivers/rpmsg/virtio_rpmsg_bus.c
> @@ -951,6 +951,7 @@ static int rpmsg_probe(struct virtio_device *vdev)
> void *bufs_va;
> int err = 0, i;
> size_t total_buf_space;
> + bool notify;
>
> vrp = kzalloc(sizeof(*vrp), GFP_KERNEL);
> if (!vrp)
> @@ -1030,8 +1031,22 @@ static int rpmsg_probe(struct virtio_device *vdev)
> }
> }
>
> + /*
> + * Prepare to kick but don't notify yet - we can't do this before
> + * device is ready.
> + */
> + notify = virtqueue_kick_prepare(vrp->rvq);
> +
> + /* From this point on, we can notify and get callbacks. */
> + virtio_device_ready(vdev);
> +
> /* tell the remote processor it can start sending messages */
> - virtqueue_kick(vrp->rvq);
> + /*
> + * this might be concurrent with callbacks, but we are only
> + * doing notify, not a full kick here, so that's ok.
> + */
> + if (notify)
> + virtqueue_notify(vrp->rvq);
>
> dev_info(&vdev->dev, "rpmsg host is online\n");
>
> --
> MST
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH] virtio_rpmsg: set DRIVER_OK before using device
2015-03-09 8:41 ` Michael S. Tsirkin
(?)
@ 2015-03-11 12:45 ` Ohad Ben-Cohen
-1 siblings, 0 replies; 18+ messages in thread
From: Ohad Ben-Cohen @ 2015-03-11 12:45 UTC (permalink / raw)
To: Michael S. Tsirkin
Cc: linux-kernel, virtualization, Rusty Russell, kvm, qemu-devel
On Mon, Mar 9, 2015 at 10:41 AM, Michael S. Tsirkin <mst@redhat.com> wrote:
> On Sat, Mar 07, 2015 at 08:06:56PM +0100, Michael S. Tsirkin wrote:
>> virtio spec requires that all drivers set DRIVER_OK
>> before using devices. While rpmsg isn't yet
>> included in the virtio 1 spec, previous spec versions
>> also required this.
>>
>> virtio rpmsg violates this rule: is calls kick
>> before setting DRIVER_OK.
>>
>> The fix isn't trivial since simply calling virtio_device_ready earlier
>> would mean we might get an interrupt in parallel with adding buffers.
>>
>> Instead, split kick out to prepare+notify calls. prepare before
>> virtio_device_ready - when we know we won't get interrupts. notify right
>> afterwards.
>>
>> Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
>> ---
>
> Ohad, can you review and ack pls?
Sure,
Acked-by: Ohad Ben-Cohen <ohad@wizery.com>
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [Qemu-devel] [PATCH] virtio_rpmsg: set DRIVER_OK before using device
@ 2015-03-11 12:45 ` Ohad Ben-Cohen
0 siblings, 0 replies; 18+ messages in thread
From: Ohad Ben-Cohen @ 2015-03-11 12:45 UTC (permalink / raw)
To: Michael S. Tsirkin
Cc: qemu-devel, Rusty Russell, linux-kernel, kvm, virtualization
On Mon, Mar 9, 2015 at 10:41 AM, Michael S. Tsirkin <mst@redhat.com> wrote:
> On Sat, Mar 07, 2015 at 08:06:56PM +0100, Michael S. Tsirkin wrote:
>> virtio spec requires that all drivers set DRIVER_OK
>> before using devices. While rpmsg isn't yet
>> included in the virtio 1 spec, previous spec versions
>> also required this.
>>
>> virtio rpmsg violates this rule: is calls kick
>> before setting DRIVER_OK.
>>
>> The fix isn't trivial since simply calling virtio_device_ready earlier
>> would mean we might get an interrupt in parallel with adding buffers.
>>
>> Instead, split kick out to prepare+notify calls. prepare before
>> virtio_device_ready - when we know we won't get interrupts. notify right
>> afterwards.
>>
>> Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
>> ---
>
> Ohad, can you review and ack pls?
Sure,
Acked-by: Ohad Ben-Cohen <ohad@wizery.com>
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH] virtio_rpmsg: set DRIVER_OK before using device
@ 2015-03-11 12:45 ` Ohad Ben-Cohen
0 siblings, 0 replies; 18+ messages in thread
From: Ohad Ben-Cohen @ 2015-03-11 12:45 UTC (permalink / raw)
To: Michael S. Tsirkin; +Cc: qemu-devel, linux-kernel, kvm, virtualization
On Mon, Mar 9, 2015 at 10:41 AM, Michael S. Tsirkin <mst@redhat.com> wrote:
> On Sat, Mar 07, 2015 at 08:06:56PM +0100, Michael S. Tsirkin wrote:
>> virtio spec requires that all drivers set DRIVER_OK
>> before using devices. While rpmsg isn't yet
>> included in the virtio 1 spec, previous spec versions
>> also required this.
>>
>> virtio rpmsg violates this rule: is calls kick
>> before setting DRIVER_OK.
>>
>> The fix isn't trivial since simply calling virtio_device_ready earlier
>> would mean we might get an interrupt in parallel with adding buffers.
>>
>> Instead, split kick out to prepare+notify calls. prepare before
>> virtio_device_ready - when we know we won't get interrupts. notify right
>> afterwards.
>>
>> Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
>> ---
>
> Ohad, can you review and ack pls?
Sure,
Acked-by: Ohad Ben-Cohen <ohad@wizery.com>
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH] virtio_rpmsg: set DRIVER_OK before using device
2015-03-11 12:45 ` Ohad Ben-Cohen
(?)
@ 2015-03-12 1:05 ` Rusty Russell
-1 siblings, 0 replies; 18+ messages in thread
From: Rusty Russell @ 2015-03-12 1:05 UTC (permalink / raw)
To: Ohad Ben-Cohen, Michael S. Tsirkin
Cc: linux-kernel, virtualization, kvm, qemu-devel
Ohad Ben-Cohen <ohad@wizery.com> writes:
> On Mon, Mar 9, 2015 at 10:41 AM, Michael S. Tsirkin <mst@redhat.com> wrote:
>> On Sat, Mar 07, 2015 at 08:06:56PM +0100, Michael S. Tsirkin wrote:
>>> virtio spec requires that all drivers set DRIVER_OK
>>> before using devices. While rpmsg isn't yet
>>> included in the virtio 1 spec, previous spec versions
>>> also required this.
>>>
>>> virtio rpmsg violates this rule: is calls kick
>>> before setting DRIVER_OK.
>>>
>>> The fix isn't trivial since simply calling virtio_device_ready earlier
>>> would mean we might get an interrupt in parallel with adding buffers.
>>>
>>> Instead, split kick out to prepare+notify calls. prepare before
>>> virtio_device_ready - when we know we won't get interrupts. notify right
>>> afterwards.
>>>
>>> Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
>>> ---
>>
>> Ohad, can you review and ack pls?
>
> Sure,
>
> Acked-by: Ohad Ben-Cohen <ohad@wizery.com>
Applied.
Thanks,
Rusty.
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [Qemu-devel] [PATCH] virtio_rpmsg: set DRIVER_OK before using device
@ 2015-03-12 1:05 ` Rusty Russell
0 siblings, 0 replies; 18+ messages in thread
From: Rusty Russell @ 2015-03-12 1:05 UTC (permalink / raw)
To: Ohad Ben-Cohen, Michael S. Tsirkin
Cc: qemu-devel, linux-kernel, kvm, virtualization
Ohad Ben-Cohen <ohad@wizery.com> writes:
> On Mon, Mar 9, 2015 at 10:41 AM, Michael S. Tsirkin <mst@redhat.com> wrote:
>> On Sat, Mar 07, 2015 at 08:06:56PM +0100, Michael S. Tsirkin wrote:
>>> virtio spec requires that all drivers set DRIVER_OK
>>> before using devices. While rpmsg isn't yet
>>> included in the virtio 1 spec, previous spec versions
>>> also required this.
>>>
>>> virtio rpmsg violates this rule: is calls kick
>>> before setting DRIVER_OK.
>>>
>>> The fix isn't trivial since simply calling virtio_device_ready earlier
>>> would mean we might get an interrupt in parallel with adding buffers.
>>>
>>> Instead, split kick out to prepare+notify calls. prepare before
>>> virtio_device_ready - when we know we won't get interrupts. notify right
>>> afterwards.
>>>
>>> Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
>>> ---
>>
>> Ohad, can you review and ack pls?
>
> Sure,
>
> Acked-by: Ohad Ben-Cohen <ohad@wizery.com>
Applied.
Thanks,
Rusty.
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH] virtio_rpmsg: set DRIVER_OK before using device
@ 2015-03-12 1:05 ` Rusty Russell
0 siblings, 0 replies; 18+ messages in thread
From: Rusty Russell @ 2015-03-12 1:05 UTC (permalink / raw)
To: Ohad Ben-Cohen, Michael S. Tsirkin
Cc: qemu-devel, linux-kernel, kvm, virtualization
Ohad Ben-Cohen <ohad@wizery.com> writes:
> On Mon, Mar 9, 2015 at 10:41 AM, Michael S. Tsirkin <mst@redhat.com> wrote:
>> On Sat, Mar 07, 2015 at 08:06:56PM +0100, Michael S. Tsirkin wrote:
>>> virtio spec requires that all drivers set DRIVER_OK
>>> before using devices. While rpmsg isn't yet
>>> included in the virtio 1 spec, previous spec versions
>>> also required this.
>>>
>>> virtio rpmsg violates this rule: is calls kick
>>> before setting DRIVER_OK.
>>>
>>> The fix isn't trivial since simply calling virtio_device_ready earlier
>>> would mean we might get an interrupt in parallel with adding buffers.
>>>
>>> Instead, split kick out to prepare+notify calls. prepare before
>>> virtio_device_ready - when we know we won't get interrupts. notify right
>>> afterwards.
>>>
>>> Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
>>> ---
>>
>> Ohad, can you review and ack pls?
>
> Sure,
>
> Acked-by: Ohad Ben-Cohen <ohad@wizery.com>
Applied.
Thanks,
Rusty.
^ permalink raw reply [flat|nested] 18+ messages in thread