Re: [Qemu-devel] [PATCH v3 1/2] virtio: check VirtQueue Vring object is set

[Cornelia Huck]

On Mon, 27 Nov 2017 23:25:28 +0530 (IST)
P J P <address@hidden> 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)