Patchwork [WIP,0/4] vhost-scsi: new device supporting the tcm_vhost Linux kernel module

login
register
mail settings
Submitter Michael S. Tsirkin
Date Jan. 31, 2013, 11:12 a.m.
Message ID <20130131111228.GB520@redhat.com>
Download mbox | patch
Permalink /patch/217160/
State New
Headers show

Comments

Michael S. Tsirkin - Jan. 31, 2013, 11:12 a.m.
On Wed, Jan 30, 2013 at 05:41:22PM +0100, Paolo Bonzini wrote:
> Ok, so here is my attempt at a vhost-scsi device.  I'm creating an
> entirely separate device, with the common parts of virtio-scsi and
> vhost-scsi (actually little more than the initialization) grouped into
> a VirtIOSCSICommon type.  The device is used simply like "-device
> vhost-scsi-pci,wwpn=WWPN", with all configuration done in configfs
> beforehand.
> 
> As expected, using a separate device finds a snag: vhost-scsi is passing
> force=false to vhost_dev_init, and the BIOS does not use MSI-X so it
> will actually use the non-vhost implementation which is wrong.  I fixed
> this by passing force=true; I'm not sure what that would break, but I
> figured "not much" since the BIOS polls and does not rely on interrupts.
> 
> That makes vhost start, but it still doesn't work for me with a 3.7.2
> kernel on the host.  Even Nick's patches hang the guest as soon as vhost
> starts, and I get the same behavior with mine.  (Of course with my
> patches the BIOS hangs and you never reach Linux; but try a BIOS without
> virtio-scsi support, and you'll see Linux hanging in the same way).
> 
> Here is my configuration:
> 
>   cd /sys/kernel/config/target
>   mkdir -p core/fileio_0/fileio
>   echo 'fd_dev_name=/home/pbonzini/test.img,fd_dev_size=5905580032' > core/fileio_0/fileio/control 
>   echo 1 > core/fileio_0/fileio/enable
>   mkdir -p vhost/naa.600140554cf3a18e/tpgt_0/lun/lun_0
>   cd vhost/naa.600140554cf3a18e/tpgt_0
>   ln -sf ../../../../../core/fileio_0/fileio/ lun/lun_0/virtual_scsi_port
>   echo naa.60014053226f0388 > nexus
> 
> Nick's patches are run with "-vhost-scsi id=vs,tpgt=0,wwpn=naa.600140554cf3a18e
> -device virtio-scsi-pci,vhost-scsi=vs".  Perhaps I'm doing something wrong.
> 
> Another small bug I found is an ordering problem between
> VHOST_SET_VRING_KICK and VHOST_SCSI_SET_ENDPOINT.  Starting the vq
> causes a "vhost_scsi_handle_vq endpoint not set" error in dmesg.
> Because of this I added the first two patches, which let me do
> VHOST_SCSI_SET_ENDPOINT before VHOST_SET_VRING_KICK but after setting
> up the vring.
> 
> Unfortunately, this is not enough to fix the hang.  And anyway, it's
> probably simpler to avoid the two patches and remove this test from the
> tcm_vhost.c vhost_scsi_set/clear_endpoint functions:
> 
>         mutex_lock(&vs->dev.mutex);
>         /* Verify that ring has been setup correctly. */
>         for (index = 0; index < vs->dev.nvqs; ++index) {
>                 /* Verify that ring has been setup correctly. */
>                 if (!vhost_vq_access_ok(&vs->vqs[index])) {
>                         mutex_unlock(&vs->dev.mutex);
>                         return -EFAULT;
>                 }
>         }
>         mutex_unlock(&vs->dev.mutex);

Well userspace should initialize the kick eventfd to 0,
it seems to init it to 1 which is why we get the error.
But I think the only issue is pr_err: vhost-net already
ignores such a kick with no backend. So let's just
remove it, preferably for 3.8.

--->
tcm_vhost: fix pr_err on early kick

It's OK to get kick before backend is set or after
it is cleared, we can just ignore it.

Signed-off-by: Michael S. Tsirkin <mst@redhat.com>

---
Nicholas A. Bellinger - Jan. 31, 2013, 9:10 p.m.
On Thu, 2013-01-31 at 13:12 +0200, Michael S. Tsirkin wrote:
> On Wed, Jan 30, 2013 at 05:41:22PM +0100, Paolo Bonzini wrote:

<SNIP>

> > Another small bug I found is an ordering problem between
> > VHOST_SET_VRING_KICK and VHOST_SCSI_SET_ENDPOINT.  Starting the vq
> > causes a "vhost_scsi_handle_vq endpoint not set" error in dmesg.
> > Because of this I added the first two patches, which let me do
> > VHOST_SCSI_SET_ENDPOINT before VHOST_SET_VRING_KICK but after setting
> > up the vring.
> > 
> > Unfortunately, this is not enough to fix the hang.  And anyway, it's
> > probably simpler to avoid the two patches and remove this test from the
> > tcm_vhost.c vhost_scsi_set/clear_endpoint functions:
> > 
> >         mutex_lock(&vs->dev.mutex);
> >         /* Verify that ring has been setup correctly. */
> >         for (index = 0; index < vs->dev.nvqs; ++index) {
> >                 /* Verify that ring has been setup correctly. */
> >                 if (!vhost_vq_access_ok(&vs->vqs[index])) {
> >                         mutex_unlock(&vs->dev.mutex);
> >                         return -EFAULT;
> >                 }
> >         }
> >         mutex_unlock(&vs->dev.mutex);
> 
> Well userspace should initialize the kick eventfd to 0,
> it seems to init it to 1 which is why we get the error.
> But I think the only issue is pr_err: vhost-net already
> ignores such a kick with no backend. So let's just
> remove it, preferably for 3.8.
> 

That's fine with me.  Applying to target-pending/master, and will
include in the next PULL request for v3.8.0.

Thanks,

--nab

> --->
> tcm_vhost: fix pr_err on early kick
> 
> It's OK to get kick before backend is set or after
> it is cleared, we can just ignore it.
> 
> Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> 
> ---
> 
> diff --git a/drivers/vhost/tcm_vhost.c b/drivers/vhost/tcm_vhost.c
> index b20df5c..22321cf 100644
> --- a/drivers/vhost/tcm_vhost.c
> +++ b/drivers/vhost/tcm_vhost.c
> @@ -575,10 +575,8 @@ static void vhost_scsi_handle_vq(struct vhost_scsi *vs)
>  
>  	/* Must use ioctl VHOST_SCSI_SET_ENDPOINT */
>  	tv_tpg = vs->vs_tpg;
> -	if (unlikely(!tv_tpg)) {
> -		pr_err("%s endpoint not set\n", __func__);
> +	if (unlikely(!tv_tpg))
>  		return;
> -	}
>  
>  	mutex_lock(&vq->mutex);
>  	vhost_disable_notify(&vs->dev, vq);

Patch

diff --git a/drivers/vhost/tcm_vhost.c b/drivers/vhost/tcm_vhost.c
index b20df5c..22321cf 100644
--- a/drivers/vhost/tcm_vhost.c
+++ b/drivers/vhost/tcm_vhost.c
@@ -575,10 +575,8 @@  static void vhost_scsi_handle_vq(struct vhost_scsi *vs)
 
 	/* Must use ioctl VHOST_SCSI_SET_ENDPOINT */
 	tv_tpg = vs->vs_tpg;
-	if (unlikely(!tv_tpg)) {
-		pr_err("%s endpoint not set\n", __func__);
+	if (unlikely(!tv_tpg))
 		return;
-	}
 
 	mutex_lock(&vq->mutex);
 	vhost_disable_notify(&vs->dev, vq);