Re: [PATCH] virtio_ring: Make interrupt suppression spec compliant

["Michael S. Tsirkin" <mst@redhat.com>]

On Mon, Jun 06, 2016 at 11:51:34AM +0200, Ladi Prosek wrote:
> According to the spec, if the VIRTIO_RING_F_EVENT_IDX feature bit is
> negotiated the driver MUST set flags to 0. Not dirtying the available
> ring in virtqueue_disable_cb may also have a positive performance impact.
> 
> Writes to the used event field (vring_used_event) are still unconditional.
> 
> Signed-off-by: Ladi Prosek <lprosek@redhat.com>

Write it to match Linux coding style please:
	if (!vq->event)
		vq->vring.avail->flags = cpu_to_virtio16(_vq->vdev, vq->avail_flags_shadow);

Also, please Cc stable as this is a spec compliance issue.

Looks like older Linux guests will have to cherry-pick
    virtio_ring: shadow available ring flags & index
to fix this. Pls mention this when you resubmit.

And pls Cc virtio lists as per MAINTAINERS.

We should also list this as one of the differences between
virtio 1.0 and 0.9.X.

Also Cc windows driver devs to make sure windows drivers do not
have spec issues.


> ---
>  drivers/virtio/virtio_ring.c | 18 +++++++++++++-----
>  1 file changed, 13 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/virtio/virtio_ring.c b/drivers/virtio/virtio_ring.c
> index ca6bfdd..d6345e1 100644
> --- a/drivers/virtio/virtio_ring.c
> +++ b/drivers/virtio/virtio_ring.c
> @@ -718,7 +718,9 @@ void virtqueue_disable_cb(struct virtqueue *_vq)
>  
>  	if (!(vq->avail_flags_shadow & VRING_AVAIL_F_NO_INTERRUPT)) {
>  		vq->avail_flags_shadow |= VRING_AVAIL_F_NO_INTERRUPT;
> -		vq->vring.avail->flags = cpu_to_virtio16(_vq->vdev, vq->avail_flags_shadow);
> +		if (!vq->event) {
> +			vq->vring.avail->flags = cpu_to_virtio16(_vq->vdev, vq->avail_flags_shadow);
> +		}
>  	}
>  
>  }
> @@ -750,7 +752,9 @@ unsigned virtqueue_enable_cb_prepare(struct virtqueue *_vq)
>  	 * entry. Always do both to keep code simple. */
>  	if (vq->avail_flags_shadow & VRING_AVAIL_F_NO_INTERRUPT) {
>  		vq->avail_flags_shadow &= ~VRING_AVAIL_F_NO_INTERRUPT;
> -		vq->vring.avail->flags = cpu_to_virtio16(_vq->vdev, vq->avail_flags_shadow);
> +		if (!vq->event) {
> +			vq->vring.avail->flags = cpu_to_virtio16(_vq->vdev, vq->avail_flags_shadow);
> +		}
>  	}
>  	vring_used_event(&vq->vring) = cpu_to_virtio16(_vq->vdev, last_used_idx = vq->last_used_idx);
>  	END_USE(vq);
> @@ -818,10 +822,12 @@ bool virtqueue_enable_cb_delayed(struct virtqueue *_vq)
>  	 * more to do. */
>  	/* Depending on the VIRTIO_RING_F_USED_EVENT_IDX feature, we need to
>  	 * either clear the flags bit or point the event index at the next
> -	 * entry. Always do both to keep code simple. */
> +	 * entry. Always update the event index to keep code simple. */
>  	if (vq->avail_flags_shadow & VRING_AVAIL_F_NO_INTERRUPT) {
>  		vq->avail_flags_shadow &= ~VRING_AVAIL_F_NO_INTERRUPT;
> -		vq->vring.avail->flags = cpu_to_virtio16(_vq->vdev, vq->avail_flags_shadow);
> +		if (!vq->event) {
> +			vq->vring.avail->flags = cpu_to_virtio16(_vq->vdev, vq->avail_flags_shadow);
> +		}
>  	}
>  	/* TODO: tune this threshold */
>  	bufs = (u16)(vq->avail_idx_shadow - vq->last_used_idx) * 3 / 4;
> @@ -939,7 +945,9 @@ struct virtqueue *__vring_new_virtqueue(unsigned int index,
>  	/* No callback?  Tell other side not to bother us. */
>  	if (!callback) {
>  		vq->avail_flags_shadow |= VRING_AVAIL_F_NO_INTERRUPT;
> -		vq->vring.avail->flags = cpu_to_virtio16(vdev, vq->avail_flags_shadow);
> +		if (!vq->event) {
> +			vq->vring.avail->flags = cpu_to_virtio16(vdev, vq->avail_flags_shadow);
> +		}
>  	}
>  
>  	/* Put everything in free lists. */
> -- 
> 2.5.5