diff mbox

[2/2] vhost: disentangle vring endianness stuff from the core code

Message ID 20160113170947.23705.95216.stgit@bahia.huguette.org (mailing list archive)
State New, archived
Headers show

Commit Message

Greg Kurz Jan. 13, 2016, 5:09 p.m. UTC 
The way vring endianness is being handled currently obfuscates
the code in vhost_init_used().

This patch tries to fix that by doing the following:
- move the the code that adjusts endianness to a dedicated helper
- export this helper so that backends explicitely call it

No behaviour change.

Signed-off-by: Greg Kurz <gkurz@linux.vnet.ibm.com>
---
 drivers/vhost/net.c   |    3 +++
 drivers/vhost/scsi.c  |    3 +++
 drivers/vhost/test.c  |    2 ++
 drivers/vhost/vhost.c |   16 +++++++++++-----
 drivers/vhost/vhost.h |    1 +
 5 files changed, 20 insertions(+), 5 deletions(-)


--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Comments

Cornelia Huck Jan. 21, 2016, 9:56 a.m. UTC | #1 
On Wed, 13 Jan 2016 18:09:47 +0100
Greg Kurz <gkurz@linux.vnet.ibm.com> wrote:

> The way vring endianness is being handled currently obfuscates
> the code in vhost_init_used().
> 
> This patch tries to fix that by doing the following:
> - move the the code that adjusts endianness to a dedicated helper
> - export this helper so that backends explicitely call it
> 
> No behaviour change.
> 
> Signed-off-by: Greg Kurz <gkurz@linux.vnet.ibm.com>
> ---
>  drivers/vhost/net.c   |    3 +++
>  drivers/vhost/scsi.c  |    3 +++
>  drivers/vhost/test.c  |    2 ++
>  drivers/vhost/vhost.c |   16 +++++++++++-----
>  drivers/vhost/vhost.h |    1 +
>  5 files changed, 20 insertions(+), 5 deletions(-)

Reviewed-by: Cornelia Huck <cornelia.huck@de.ibm.com>

--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Michael S. Tsirkin Feb. 10, 2016, 11:48 a.m. UTC | #2 
On Wed, Jan 13, 2016 at 06:09:47PM +0100, Greg Kurz wrote:
> The way vring endianness is being handled currently obfuscates
> the code in vhost_init_used().
> 
> This patch tries to fix that by doing the following:
> - move the the code that adjusts endianness to a dedicated helper
> - export this helper so that backends explicitely call it
> 
> No behaviour change.
> 
> Signed-off-by: Greg Kurz <gkurz@linux.vnet.ibm.com>
> ---
>  drivers/vhost/net.c   |    3 +++
>  drivers/vhost/scsi.c  |    3 +++
>  drivers/vhost/test.c  |    2 ++
>  drivers/vhost/vhost.c |   16 +++++++++++-----
>  drivers/vhost/vhost.h |    1 +
>  5 files changed, 20 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/vhost/net.c b/drivers/vhost/net.c
> index 9eda69e40678..df01c939cd00 100644
> --- a/drivers/vhost/net.c
> +++ b/drivers/vhost/net.c
> @@ -917,6 +917,9 @@ static long vhost_net_set_backend(struct vhost_net *n, unsigned index, int fd)
>  
>  		vhost_net_disable_vq(n, vq);
>  		vq->private_data = sock;
> +
> +		vhost_adjust_vring_endian(vq);
> +
>  		r = vhost_init_used(vq);
>  		if (r)
>  			goto err_used;


This is in fact a bug in existing code: if vhost_init_used
fails, it preferably should not have side-effects.
It's best to update it last thing.

> diff --git a/drivers/vhost/scsi.c b/drivers/vhost/scsi.c
> index 29cfc57d496e..5a8363bfcb74 100644
> --- a/drivers/vhost/scsi.c
> +++ b/drivers/vhost/scsi.c
> @@ -1274,6 +1274,9 @@ vhost_scsi_set_endpoint(struct vhost_scsi *vs,
>  			vq = &vs->vqs[i].vq;
>  			mutex_lock(&vq->mutex);
>  			vq->private_data = vs_tpg;
> +
> +			vhost_adjust_vring_endian(vq);
> +
>  			vhost_init_used(vq);
>  			mutex_unlock(&vq->mutex);
>  		}
> diff --git a/drivers/vhost/test.c b/drivers/vhost/test.c
> index f2882ac98726..75e3e0e9f5a8 100644
> --- a/drivers/vhost/test.c
> +++ b/drivers/vhost/test.c
> @@ -196,6 +196,8 @@ static long vhost_test_run(struct vhost_test *n, int test)
>  		oldpriv = vq->private_data;
>  		vq->private_data = priv;
>  
> +		vhost_adjust_vring_endian(vq);
> +
>  		r = vhost_init_used(&n->vqs[index]);
>  
>  		mutex_unlock(&vq->mutex);
> diff --git a/drivers/vhost/vhost.c b/drivers/vhost/vhost.c
> index e02e06755ab7..b0a00340309e 100644
> --- a/drivers/vhost/vhost.c
> +++ b/drivers/vhost/vhost.c
> @@ -123,6 +123,15 @@ static void vhost_disable_is_le(struct vhost_virtqueue *vq)
>  	vq->is_le = virtio_legacy_is_little_endian();
>  }
>  
> +void vhost_adjust_vring_endian(struct vhost_virtqueue *vq)
> +{
> +	if (!vq->private_data)
> +		vhost_disable_is_le(vq);
> +	else
> +		vhost_enable_is_le(vq);
> +}
> +EXPORT_SYMBOL_GPL(vhost_adjust_vring_endian);
> +
>  static void vhost_poll_func(struct file *file, wait_queue_head_t *wqh,
>  			    poll_table *pt)
>  {

I'd prefer "vhost_update_is_le" here. "endian" might also mean
"user_be". But see below pls.


> @@ -1166,12 +1175,9 @@ int vhost_init_used(struct vhost_virtqueue *vq)
>  {
>  	__virtio16 last_used_idx;
>  	int r;
> -	if (!vq->private_data) {
> -		vhost_disable_is_le(vq);
> -		return 0;
> -	}
>  
> -	vhost_enable_is_le(vq);
> +	if (!vq->private_data)
> +		return 0;
>  
>  	r = vhost_update_used_flags(vq);
>  	if (r)

Looking at how callers use this, maybe we should just rename init_used
to vhost_vq_init_access. The _used suffix was a hint that we
access the vq used ring. But maybe what callers care about is
that it must be called after access_ok.

> diff --git a/drivers/vhost/vhost.h b/drivers/vhost/vhost.h
> index d3f767448a72..88d86f45f756 100644
> --- a/drivers/vhost/vhost.h
> +++ b/drivers/vhost/vhost.h
> @@ -162,6 +162,7 @@ bool vhost_enable_notify(struct vhost_dev *, struct vhost_virtqueue *);
>  
>  int vhost_log_write(struct vhost_virtqueue *vq, struct vhost_log *log,
>  		    unsigned int log_num, u64 len);
> +void vhost_adjust_vring_endian(struct vhost_virtqueue *vq);
>  
>  #define vq_err(vq, fmt, ...) do {                                  \
>  		pr_debug(pr_fmt(fmt), ##__VA_ARGS__);       \
--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Greg Kurz Feb. 10, 2016, 1:08 p.m. UTC | #3 
On Wed, 10 Feb 2016 13:48:09 +0200
"Michael S. Tsirkin" <mst@redhat.com> wrote:

> On Wed, Jan 13, 2016 at 06:09:47PM +0100, Greg Kurz wrote:
> > The way vring endianness is being handled currently obfuscates
> > the code in vhost_init_used().
> > 
> > This patch tries to fix that by doing the following:
> > - move the the code that adjusts endianness to a dedicated helper
> > - export this helper so that backends explicitely call it
> > 
> > No behaviour change.
> > 
> > Signed-off-by: Greg Kurz <gkurz@linux.vnet.ibm.com>
> > ---
> >  drivers/vhost/net.c   |    3 +++
> >  drivers/vhost/scsi.c  |    3 +++
> >  drivers/vhost/test.c  |    2 ++
> >  drivers/vhost/vhost.c |   16 +++++++++++-----
> >  drivers/vhost/vhost.h |    1 +
> >  5 files changed, 20 insertions(+), 5 deletions(-)
> > 
> > diff --git a/drivers/vhost/net.c b/drivers/vhost/net.c
> > index 9eda69e40678..df01c939cd00 100644
> > --- a/drivers/vhost/net.c
> > +++ b/drivers/vhost/net.c
> > @@ -917,6 +917,9 @@ static long vhost_net_set_backend(struct vhost_net *n, unsigned index, int fd)
> >  
> >  		vhost_net_disable_vq(n, vq);
> >  		vq->private_data = sock;
> > +
> > +		vhost_adjust_vring_endian(vq);
> > +
> >  		r = vhost_init_used(vq);
> >  		if (r)
> >  			goto err_used;  
> 
> 
> This is in fact a bug in existing code: if vhost_init_used
> fails, it preferably should not have side-effects.
> It's best to update it last thing.
> 

I'm afraid we can't because the following path needs the
vring endianness:

vhost_init_used()->vhost_update_used_flags()->cpu_to_vhost16()

But you are right, there is a bug: we should rollback if vhost_init_used()
fails. Something like below:

 err_used:
        vq->private_data = oldsock;
        vhost_net_enable_vq(n, vq);
+       vhost_adjust_vring_endian(vq);
        if (ubufs)
                vhost_net_ubuf_put_wait_and_free(ubufs);
 err_ubufs:

> > diff --git a/drivers/vhost/scsi.c b/drivers/vhost/scsi.c
> > index 29cfc57d496e..5a8363bfcb74 100644
> > --- a/drivers/vhost/scsi.c
> > +++ b/drivers/vhost/scsi.c
> > @@ -1274,6 +1274,9 @@ vhost_scsi_set_endpoint(struct vhost_scsi *vs,
> >  			vq = &vs->vqs[i].vq;
> >  			mutex_lock(&vq->mutex);
> >  			vq->private_data = vs_tpg;
> > +
> > +			vhost_adjust_vring_endian(vq);
> > +
> >  			vhost_init_used(vq);
> >  			mutex_unlock(&vq->mutex);
> >  		}
> > diff --git a/drivers/vhost/test.c b/drivers/vhost/test.c
> > index f2882ac98726..75e3e0e9f5a8 100644
> > --- a/drivers/vhost/test.c
> > +++ b/drivers/vhost/test.c
> > @@ -196,6 +196,8 @@ static long vhost_test_run(struct vhost_test *n, int test)
> >  		oldpriv = vq->private_data;
> >  		vq->private_data = priv;
> >  
> > +		vhost_adjust_vring_endian(vq);
> > +
> >  		r = vhost_init_used(&n->vqs[index]);
> >  
> >  		mutex_unlock(&vq->mutex);
> > diff --git a/drivers/vhost/vhost.c b/drivers/vhost/vhost.c
> > index e02e06755ab7..b0a00340309e 100644
> > --- a/drivers/vhost/vhost.c
> > +++ b/drivers/vhost/vhost.c
> > @@ -123,6 +123,15 @@ static void vhost_disable_is_le(struct vhost_virtqueue *vq)
> >  	vq->is_le = virtio_legacy_is_little_endian();
> >  }
> >  
> > +void vhost_adjust_vring_endian(struct vhost_virtqueue *vq)
> > +{
> > +	if (!vq->private_data)
> > +		vhost_disable_is_le(vq);
> > +	else
> > +		vhost_enable_is_le(vq);
> > +}
> > +EXPORT_SYMBOL_GPL(vhost_adjust_vring_endian);
> > +
> >  static void vhost_poll_func(struct file *file, wait_queue_head_t *wqh,
> >  			    poll_table *pt)
> >  {  
> 
> I'd prefer "vhost_update_is_le" here. "endian" might also mean
> "user_be". But see below pls.
> 

Ok, I will rename if this patch survives the review.

> 
> > @@ -1166,12 +1175,9 @@ int vhost_init_used(struct vhost_virtqueue *vq)
> >  {
> >  	__virtio16 last_used_idx;
> >  	int r;
> > -	if (!vq->private_data) {
> > -		vhost_disable_is_le(vq);
> > -		return 0;
> > -	}
> >  
> > -	vhost_enable_is_le(vq);
> > +	if (!vq->private_data)
> > +		return 0;
> >  
> >  	r = vhost_update_used_flags(vq);
> >  	if (r)  
> 
> Looking at how callers use this, maybe we should just rename init_used
> to vhost_vq_init_access. The _used suffix was a hint that we
> access the vq used ring. But maybe what callers care about is
> that it must be called after access_ok.
> 

And so we would keep the current logic where it is up to the core
code to update is_le... that is basically getting rid of this patch :)

So IIUC you're asking for:
- fix the side-effect in vhost_init_used()
- rename vhost_init_used() to vhost_vq_init_access()

> > diff --git a/drivers/vhost/vhost.h b/drivers/vhost/vhost.h
> > index d3f767448a72..88d86f45f756 100644
> > --- a/drivers/vhost/vhost.h
> > +++ b/drivers/vhost/vhost.h
> > @@ -162,6 +162,7 @@ bool vhost_enable_notify(struct vhost_dev *, struct vhost_virtqueue *);
> >  
> >  int vhost_log_write(struct vhost_virtqueue *vq, struct vhost_log *log,
> >  		    unsigned int log_num, u64 len);
> > +void vhost_adjust_vring_endian(struct vhost_virtqueue *vq);
> >  
> >  #define vq_err(vq, fmt, ...) do {                                  \
> >  		pr_debug(pr_fmt(fmt), ##__VA_ARGS__);       \  
> 

--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Cornelia Huck Feb. 10, 2016, 1:23 p.m. UTC | #4 
On Wed, 10 Feb 2016 14:08:43 +0100
Greg Kurz <gkurz@linux.vnet.ibm.com> wrote:

> But you are right, there is a bug: we should rollback if vhost_init_used()
> fails. Something like below:
> 
>  err_used:
>         vq->private_data = oldsock;
>         vhost_net_enable_vq(n, vq);
> +       vhost_adjust_vring_endian(vq);

Shouldn't we switch back before we reenable? Or have I lost myself in
this maze here again?

>         if (ubufs)
>                 vhost_net_ubuf_put_wait_and_free(ubufs);
>  err_ubufs:

--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Greg Kurz Feb. 10, 2016, 1:40 p.m. UTC | #5 
On Wed, 10 Feb 2016 14:23:33 +0100
Cornelia Huck <cornelia.huck@de.ibm.com> wrote:

> On Wed, 10 Feb 2016 14:08:43 +0100
> Greg Kurz <gkurz@linux.vnet.ibm.com> wrote:
> 
> > But you are right, there is a bug: we should rollback if vhost_init_used()
> > fails. Something like below:
> > 
> >  err_used:
> >         vq->private_data = oldsock;
> >         vhost_net_enable_vq(n, vq);
> > +       vhost_adjust_vring_endian(vq);  
> 
> Shouldn't we switch back before we reenable? Or have I lost myself in
> this maze here again?
> 

I haven't spotted any path under vhost_net_enable_vq() that needs
the vring endianness, but indeed it looks safer to switch back
before a worker thread may be woken up.

> >         if (ubufs)
> >                 vhost_net_ubuf_put_wait_and_free(ubufs);
> >  err_ubufs:  

--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
diff mbox

Patch

diff --git a/drivers/vhost/net.c b/drivers/vhost/net.c
index 9eda69e40678..df01c939cd00 100644
--- a/drivers/vhost/net.c
+++ b/drivers/vhost/net.c
@@ -917,6 +917,9 @@  static long vhost_net_set_backend(struct vhost_net *n, unsigned index, int fd)
 
 		vhost_net_disable_vq(n, vq);
 		vq->private_data = sock;
+
+		vhost_adjust_vring_endian(vq);
+
 		r = vhost_init_used(vq);
 		if (r)
 			goto err_used;
diff --git a/drivers/vhost/scsi.c b/drivers/vhost/scsi.c
index 29cfc57d496e..5a8363bfcb74 100644
--- a/drivers/vhost/scsi.c
+++ b/drivers/vhost/scsi.c
@@ -1274,6 +1274,9 @@  vhost_scsi_set_endpoint(struct vhost_scsi *vs,
 			vq = &vs->vqs[i].vq;
 			mutex_lock(&vq->mutex);
 			vq->private_data = vs_tpg;
+
+			vhost_adjust_vring_endian(vq);
+
 			vhost_init_used(vq);
 			mutex_unlock(&vq->mutex);
 		}
diff --git a/drivers/vhost/test.c b/drivers/vhost/test.c
index f2882ac98726..75e3e0e9f5a8 100644
--- a/drivers/vhost/test.c
+++ b/drivers/vhost/test.c
@@ -196,6 +196,8 @@  static long vhost_test_run(struct vhost_test *n, int test)
 		oldpriv = vq->private_data;
 		vq->private_data = priv;
 
+		vhost_adjust_vring_endian(vq);
+
 		r = vhost_init_used(&n->vqs[index]);
 
 		mutex_unlock(&vq->mutex);
diff --git a/drivers/vhost/vhost.c b/drivers/vhost/vhost.c
index e02e06755ab7..b0a00340309e 100644
--- a/drivers/vhost/vhost.c
+++ b/drivers/vhost/vhost.c
@@ -123,6 +123,15 @@  static void vhost_disable_is_le(struct vhost_virtqueue *vq)
 	vq->is_le = virtio_legacy_is_little_endian();
 }
 
+void vhost_adjust_vring_endian(struct vhost_virtqueue *vq)
+{
+	if (!vq->private_data)
+		vhost_disable_is_le(vq);
+	else
+		vhost_enable_is_le(vq);
+}
+EXPORT_SYMBOL_GPL(vhost_adjust_vring_endian);
+
 static void vhost_poll_func(struct file *file, wait_queue_head_t *wqh,
 			    poll_table *pt)
 {
@@ -1166,12 +1175,9 @@  int vhost_init_used(struct vhost_virtqueue *vq)
 {
 	__virtio16 last_used_idx;
 	int r;
-	if (!vq->private_data) {
-		vhost_disable_is_le(vq);
-		return 0;
-	}
 
-	vhost_enable_is_le(vq);
+	if (!vq->private_data)
+		return 0;
 
 	r = vhost_update_used_flags(vq);
 	if (r)
diff --git a/drivers/vhost/vhost.h b/drivers/vhost/vhost.h
index d3f767448a72..88d86f45f756 100644
--- a/drivers/vhost/vhost.h
+++ b/drivers/vhost/vhost.h
@@ -162,6 +162,7 @@  bool vhost_enable_notify(struct vhost_dev *, struct vhost_virtqueue *);
 
 int vhost_log_write(struct vhost_virtqueue *vq, struct vhost_log *log,
 		    unsigned int log_num, u64 len);
+void vhost_adjust_vring_endian(struct vhost_virtqueue *vq);
 
 #define vq_err(vq, fmt, ...) do {                                  \
 		pr_debug(pr_fmt(fmt), ##__VA_ARGS__);       \