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

[Yang Zhong]

On Fri, Jan 31, 2020 at 05:22:47PM +0100, Paolo Bonzini wrote:
> I have just found this email... sorry for the delay.
> 
> On 10/01/20 07:10, Yang Zhong wrote:
> >> 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.
> >
> >   (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.
> 
> It does so because it cannot know how it will be used.  It could be used
> by the guest boot loader to load a kernel, for example.  SeaBIOS sets
> DRIVER_OK because it has loaded a driver for the disk; that's exactly
> what DRIVER_OK signals.
> 
  Yes, the virtio_blk driver in seabios is mainly for virtio format
  images. I did not consider this. Thanks Paolo!
  
  Yang
> 
> >     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).
> 
> Doesn't it first reset the status to 0?
> 
  I checked the virtio_blk drivers in seabios and guest kernel, all do
  the reset before driver probe(). thanks!

  Yang

> >     (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.
> 
> What is virtio_is_ready()?  The virtio device should not wait for all
> the queues to be set.  A device is ready when it sets DRIVER_OK, and
> that's it.
> 
  In spdk/dpdk/lib/librte_vhost/vhost_user.c
  static int
  virtio_is_ready(struct virtio_net *dev)
  {
      struct vhost_virtqueue *vq;
      uint32_t i;

      if (dev->nr_vring == 0)
          return 0;

      for (i = 0; i < dev->nr_vring; i++) {
          vq = dev->virtqueue[i];

          if (!vq_is_ready(dev, vq))
              return 0;
      }

      RTE_LOG(INFO, VHOST_CONFIG,
          "virtio is now ready for processing.\n");
      return 1;
  }

  static bool
  vq_is_ready(struct virtio_net *dev, struct vhost_virtqueue *vq)
  {
      bool rings_ok;

      if (!vq)
          return false;

      if (vq_is_packed(dev))
          rings_ok = !!vq->desc_packed;
      else
          rings_ok = vq->desc && vq->avail && vq->used;

      return rings_ok &&
             vq->kickfd != VIRTIO_UNINITIALIZED_EVENTFD &&
             vq->callfd != VIRTIO_UNINITIALIZED_EVENTFD;
  }

  The dev->nr_vring is equal to the queue number set in qemu cmdline, 
  which maybe different with guest virtio_blk driver by MIN(vcpu, num_vqs).
  If like this scenario, the virtio is NEVER ready from
  virtio_is_ready() because DPDK still check the left
  queues(i.e num_vqs-vcpu), which are not initialized by the guest
  driver.
  
  Yang
  
> >      or DPDK can get the real queue number by checking if the vring.desc
> >      is NON-NULL.
> 
> Note that there is no requirement that the driver initializes a
> consecutive number of virtqueues.  It is acceptable for it to initialize
> virtqueues 0, 1 and 57.  It seems like the bug is in DPDK, possibly more
> than one...
> 
  Yes, it seems the bug is in DPDK side. But one strange issue here, the
  virtio_net driver do the same thing to get real num_vqs by MIN(..),
  the DPDK with virtio_net can avoid this kind of issue(Sorry, i did not
  fully dig out the virtio_net and DPDK code and only hope to get the
  best solution to fix this issue). Anyway, i will firstly check this from 
  DPDK side. Thanks for your great help!

  Yang

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