Re: [PATCH] virtio: add the queue number check

[Yang Zhong]

On Fri, Jan 03, 2020 at 10:18:58PM +0100, Paolo Bonzini wrote:
> Il ven 3 gen 2020, 16:08 Yang Zhong <address@hidden> ha scritto:
> 
> >   I also tried virtio-blk device like below:
> >   https://patchwork.kernel.org/cover/10873193/
> >
> >   The virtio-blk can work with this changes, but vhost-user-blk device
> >   failed with this kernel patch.
> >
> >   in vhost_virtqueue_start() function, below operation to check if the
> >   desc addr set by guest kernel. This will ignore the extra vqs.
> >     a = virtio_queue_get_desc_addr(vdev, idx);
> >     if (a == 0) {
> >         /* Queue might not be ready for start */
> >         return 0;
> >     }
> >
> >   If guest kernel add min(cpu,num_vqs), do we need add same check in
> >   realize function of vhost-user-blk device?
> >
> 
> No. If virtio-blk works, the bug is in vhost-user-blk; if virtio-blk needs
> no check in cpu count, vhost-user-blk also doesn't.
> 
> You need to check first if the bug is in QEMU or the vhost-user-blk server.
> 
  Paolo and all,

  It seems i had root cause this issue, let me list what's i found. Any
  issue please correct me, thanks!

  I found there are 2 issues in DPDK and seabios by debugging this
  issue.

  (1). Seabios issue
  In init_virtio_blk() function, which set VIRTIO_CONFIG_S_DRIVER_OK
  status to qemu vhost-user-blk device.

  // the related code
  ......
  status |= VIRTIO_CONFIG_S_DRIVER_OK;
  vp_set_status(&vdrive->vp, status);
  ......

  I think there is no need for seabios to set VIRTIO_CONFIG_S_DRIVER_OK
  status to qemu vhost-user-blk device. In fact, this time, the guest
  virtio-blk module is not started. Once seabios set this DRIVER_OK
  status to qemu vhost user device, the vhost-user-blk will call
  vhost_user_blk_start().
  
  static void vhost_user_blk_set_status(VirtIODevice *vdev, uint8_t status)
  {
    VHostUserBlk *s = VHOST_USER_BLK(vdev);
    bool should_start = virtio_device_started(vdev, status);
    int ret;

    if (!vdev->vm_running) {
        should_start = false;
    }

    if (!s->connected) {
        return;
    }

    if (s->dev.started == should_start) {
        return;
    }

    if (should_start) {
        ret = vhost_user_blk_start(vdev);
        if (ret < 0) {
            error_report("vhost-user-blk: vhost start failed: %s",
                         strerror(-ret));
            qemu_chr_fe_disconnect(&s->chardev);
        }
    } else {
        vhost_user_blk_stop(vdev);
    }
    }

    In fact, this time vhost_user_blk_start almost do nothing because
    the real guest virtio-blk driver still not started yet. This time,
    there is only one vq can be used(this vq should be inited in seabios).

    When the guest virtio-blk driver really start and complet the
    probe(), the guest virtio-blk driver will set
    VIRTIO_CONFIG_S_DRIVER_OK to vhost-user-blk device again. This
    time, this driver will allocate RIGHT queue num according to
    MIN(vcpu, num_vqs).

    So, i think set VIRTIO_CONFIG_S_DRIVER_OK status should be removed from
    seabios, and guest driver should do this.

    I removed this status seting in the seabios, and test verified this.
    guest virtio-blk driver can be normally initialized by virtio-blk or
    vhost-user-blk device from qemu.

    (2). DPDK issue
     DPDK does not know the real queue number used by guest virtio-blk
     driver and it only know the queue number from vhost-user-blk
     commond line. Once the guest virtio-blk driver change the queue
     number according to MIN(vcpu, num_vqs), DPDK still use previous
     queue number and it think virtio is never ready by
     virtio_is_ready() function.

     There are two methods to fix this issue,

     one is add VHOST_USER_SET_QUEUE_NUM to vhost user protocol.
     vhost-user-blk device can call this to inform SPDK the real
     queue number. vhost-user-device can use below method to get
     the real queue number(to check if vring.desc is NON-NULL)
     for (i = 0; i < s->dev.nvqs; i++) {
        VirtQueue *kick_vq = virtio_get_queue(vdev, i);

        if (!virtio_queue_get_desc_addr(vdev, i)) {
            continue;
        }
     }
    
     Current vhost-user protocol only support GET_QUEUE_NUM(get max
     queue num), we can add SET_QUEUE_NUM.

     or DPDK can get the real queue number by checking if the vring.desc
     is NON-NULL.

     By the way, vhost SCSI device has the same issue with
     vhost-user-blk device. 

     Yang

> Paolo