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
> >
