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

[Ladi Prosek <lprosek@redhat.com>]

On Mon, Aug 22, 2016 at 4:26 PM, Michael S. Tsirkin <mst@redhat.com> wrote:
> 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.

Will do.

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

Windows is pretty much identical to Linux when it comes to ring routines.
    virtio_ring: shadow available ring flags & index
has already been ported to virtio-win and I'll do the same for this
patch if it gets merged.

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