On Wed, 28 Sep 2022 17:16:15 +0200
dinhngoc.tu@xxxxxxx wrote:
Hi,
> From: Tu Dinh Ngoc <dinhngoc.tu@xxxxxxx>
>
> VIRTIO_RING_F_EVENT_IDX is a bit position value, but
> virtio_init_device_vq populates vq->use_event_idx by ANDing this value
> directly to vdev->features.
Indeed, it's used correctly in other parts of the code (virtio/net.c,
virtio/scsi.c), so we need to shift.
Just one thing below, but good catch!
>
> Fix the check for this flag in virtio_init_device_vq.
> ---
> virtio/core.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/virtio/core.c b/virtio/core.c
> index f432421..32e6a7a 100644
> --- a/virtio/core.c
> +++ b/virtio/core.c
> @@ -165,7 +165,7 @@ void virtio_init_device_vq(struct kvm *kvm, struct virtio_device *vdev,
> struct vring_addr *addr = &vq->vring_addr;
>
> vq->endian = vdev->endian;
> - vq->use_event_idx = (vdev->features & VIRTIO_RING_F_EVENT_IDX);
> + vq->use_event_idx = (vdev->features & (1 << VIRTIO_RING_F_EVENT_IDX));
Can you make this "1UL << ..." instead? Shifting signed values is quite
fragile.
Cheers,
Andre
> vq->enabled = true;
>
> if (addr->legacy) {
