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 <m...@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
