| Message ID | 510B3A94.4000409@redhat.com |
|---|---|
| State | New |
| Headers | show |
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
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
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
--- 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);