Re: [PATCH] drivers/virtio: delayed configuration descriptor flags

[Jason Wang]

On Tue, Apr 9, 2024 at 1:27 AM ni.liqiang <ni_liqiang@xxxxxxx> wrote:
>
> In our testing of the virtio hardware accelerator, we found that
> configuring the flags of the descriptor after addr and len,
> as implemented in DPDK, seems to be more friendly to the hardware.
>
> In our Virtio hardware implementation tests, using the default
> open-source code, the hardware's bulk reads ensure performance
> but correctness is compromised. If we refer to the implementation code
> of DPDK, placing the flags configuration of the descriptor
> after addr and len, virtio backend can function properly based on
> our hardware accelerator.
>
> I am somewhat puzzled by this. From a software process perspective,
> it seems that there should be no difference whether
> the flags configuration of the descriptor is before or after addr and len.
> However, this is not the case according to experimental test results.
> We would like to know if such a change in the configuration order
> is reasonable and acceptable?

Harmless but a hint that there's a bug in your hardware?

More below

>
> Thanks.
>
> Signed-off-by: ni.liqiang <ni_liqiang@xxxxxxx>
> Reviewed-by: jin.qi <jin.qi@xxxxxxxxxx>
> Tested-by: jin.qi <jin.qi@xxxxxxxxxx>
> Cc: ni.liqiang <ni.liqiang@xxxxxxxxxx>
> ---
>  drivers/virtio/virtio_ring.c | 9 +++++----
>  1 file changed, 5 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/virtio/virtio_ring.c b/drivers/virtio/virtio_ring.c
> index 6f7e5010a673..bea2c2fb084e 100644
> --- a/drivers/virtio/virtio_ring.c
> +++ b/drivers/virtio/virtio_ring.c
> @@ -1472,15 +1472,16 @@ static inline int virtqueue_add_packed(struct virtqueue *_vq,
>                         flags = cpu_to_le16(vq->packed.avail_used_flags |
>                                     (++c == total_sg ? 0 : VRING_DESC_F_NEXT) |
>                                     (n < out_sgs ? 0 : VRING_DESC_F_WRITE));
> -                       if (i == head)
> -                               head_flags = flags;
> -                       else
> -                               desc[i].flags = flags;
>
>                         desc[i].addr = cpu_to_le64(addr);
>                         desc[i].len = cpu_to_le32(sg->length);
>                         desc[i].id = cpu_to_le16(id);
>
> +                       if (i == head)
> +                               head_flags = flags;
> +                       else
> +                               desc[i].flags = flags;
> +

The head_flags are not updated at this time, so descriptors are not
available, the device should not start to read the chain:

        /*
         * A driver MUST NOT make the first descriptor in the list
         * available before all subsequent descriptors comprising
         * the list are made available.
         */
        virtio_wmb(vq->weak_barriers);
        vq->packed.vring.desc[head].flags = head_flags;
        vq->num_added += descs_used;

It looks like your device does speculation reading on the descriptors
that are not available?

Thanks

>                         if (unlikely(vq->use_dma_api)) {
>                                 vq->packed.desc_extra[curr].addr = addr;
>                                 vq->packed.desc_extra[curr].len = sg->length;
> --
> 2.34.1
>
>