On Mon, 27 Nov 2017 23:25:28 +0530 (IST)
P J P <ppan...@redhat.com> wrote:
> +-- On Mon, 27 Nov 2017, Cornelia Huck wrote --+
> |The check for align is not really needed, as virtio-1 disallows setting
> align
> |anyway.
>
> disallows...?
See the check in virtio_queue_set_align(). Moreover, the calculation
that breaks virtqueue setup for align == 0 is only called for the
legacy setup, IOW not for this virtio-1 only function.
>
> | Checking for !desc is wrong (why shouldn't a driver be able to unset a
> | descriptor table?)
>
> +-- On Mon, 27 Nov 2017, Stefan Hajnoczi wrote --+
> | > + if (!vdev->vq[n].vring.num || !desc || !vdev->vq[n].vring.align) {
> | ...
> | vdev->vq[n].vring.desc = desc;
> |
> | Why !desc?
>
> virtio_queue_set_rings
> virtio_init_region_cache
> VirtQueue *vq = &vdev->vq[n];
> ...
> addr = vq->vring.desc;
> if (!addr) {
> return;
> }
>
> These checks seem to be repeating all over. As mentioned earlier, could these
> be collated in one place, maybe virtio_queue_get_num()?
>
> int virtio_queue_get_num(VirtIODevice *vdev, int n)
> {
> VirtQueue *vq = &vdev->vq[n];
>
> if (!vq->.vring.num
> || !vq->vring.desc
> || !vq->vring.align) {
> return 0; /* vq not set */
> }
>
> return vdev->vq[n].vring.num;
> }
This is conflating different things:
- vq does not exist (num == 0)
- vq is not setup by the guest (desc == 0)
- vq has no valid alignment (which is only relevant for legacy)
