On Mon, Aug 22, 2016 at 4:26 PM, Michael S. Tsirkin <mst@xxxxxxxxxx> 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@xxxxxxxxxx>
>
> 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
--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
