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