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

login
register
mail settings
Submitter Asias He
Date Feb. 1, 2013, 3:46 a.m.
Message ID <510B3A94.4000409@redhat.com>
Download mbox | patch
Permalink /patch/217339/
State New
Headers show

Comments

Asias He - Feb. 1, 2013, 3:46 a.m.
On 01/31/2013 07:12 PM, Michael S. Tsirkin wrote:
> 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.

With following patch, the kick before backend is set is gone.

> 
> --->
> 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);
>
Michael S. Tsirkin - Feb. 3, 2013, 12:38 p.m.
On Fri, Feb 01, 2013 at 11:46:28AM +0800, Asias He wrote:
> On 01/31/2013 07:12 PM, Michael S. Tsirkin wrote:
> > 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.
> 
> With following patch, the kick before backend is set is gone.
> 
> --- a/hw/virtio-pci.c
> +++ b/hw/virtio-pci.c
> @@ -169,7 +169,7 @@ static int
> virtio_pci_set_host_notifier_internal(VirtIOPCIProxy *proxy,
>      int r = 0;
> 
>      if (assign) {
> -        r = event_notifier_init(notifier, false);
> +        r = event_notifier_init(notifier, 1);
> 

Hmm, not the reverse?
It's also int so should be 0 not false.


> > 
> > --->
> > 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);
> > 
> 
> 
> -- 
> Asias
Asias He - Feb. 5, 2013, 9:23 a.m.
On 02/03/2013 08:38 PM, Michael S. Tsirkin wrote:
> On Fri, Feb 01, 2013 at 11:46:28AM +0800, Asias He wrote:
>> On 01/31/2013 07:12 PM, Michael S. Tsirkin wrote:
>>> 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.
>>
>> With following patch, the kick before backend is set is gone.
>>
>> --- a/hw/virtio-pci.c
>> +++ b/hw/virtio-pci.c
>> @@ -169,7 +169,7 @@ static int
>> virtio_pci_set_host_notifier_internal(VirtIOPCIProxy *proxy,
>>      int r = 0;
>>
>>      if (assign) {
>> -        r = event_notifier_init(notifier, false);
>> +        r = event_notifier_init(notifier, 1);
>>
> 
> Hmm, not the reverse?

I pasted the diff in the revert commit.

> It's also int so should be 0 not false.

Yes. it should be int.

'git grep event_notifier_init' shows there are other places where false
is used.

> 
>>>
>>> --->
>>> 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);
>>>
>>
>>
>> -- 
>> Asias
Michael S. Tsirkin - Feb. 5, 2013, 11:56 a.m.
On Tue, Feb 05, 2013 at 05:23:03PM +0800, Asias He wrote:
> On 02/03/2013 08:38 PM, Michael S. Tsirkin wrote:
> > On Fri, Feb 01, 2013 at 11:46:28AM +0800, Asias He wrote:
> >> On 01/31/2013 07:12 PM, Michael S. Tsirkin wrote:
> >>> 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.
> >>
> >> With following patch, the kick before backend is set is gone.
> >>
> >> --- a/hw/virtio-pci.c
> >> +++ b/hw/virtio-pci.c
> >> @@ -169,7 +169,7 @@ static int
> >> virtio_pci_set_host_notifier_internal(VirtIOPCIProxy *proxy,
> >>      int r = 0;
> >>
> >>      if (assign) {
> >> -        r = event_notifier_init(notifier, false);
> >> +        r = event_notifier_init(notifier, 1);
> >>
> > 
> > Hmm, not the reverse?
> 
> I pasted the diff in the revert commit.
> 
> > It's also int so should be 0 not false.
> 
> Yes. it should be int.
> 
> 'git grep event_notifier_init' shows there are other places where false
> is used.

Thanks, I'll fix them up.

> > 
> >>>
> >>> --->
> >>> 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);
> >>>
> >>
> >>
> >> -- 
> >> Asias
> 
> 
> -- 
> Asias

Patch

--- a/hw/virtio-pci.c
+++ b/hw/virtio-pci.c
@@ -169,7 +169,7 @@  static int
virtio_pci_set_host_notifier_internal(VirtIOPCIProxy *proxy,
     int r = 0;

     if (assign) {
-        r = event_notifier_init(notifier, false);
+        r = event_notifier_init(notifier, 1);