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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [day] [month] [year] [list]

Message-ID: <c6782614-1201-4796-b82c-cf8a69c3c8bf@nvidia.com>
Date: Mon, 8 Apr 2024 18:41:03 +0000
From: Chaitanya Kulkarni <chaitanyak@...dia.com>
To: ni.liqiang <ni_liqiang@....com>
CC: "jin . qi" <jin.qi@....com.cn>, Xuan Zhuo <xuanzhuo@...ux.alibaba.com>,
	"ni . liqiang" <ni.liqiang@....com.cn>, "virtualization@...ts.linux.dev"
	<virtualization@...ts.linux.dev>, "linux-kernel@...r.kernel.org"
	<linux-kernel@...r.kernel.org>, "Michael S. Tsirkin" <mst@...hat.com>, Jason
 Wang <jasowang@...hat.com>
Subject: Re: [PATCH] drivers/virtio: delayed configuration descriptor flags

On 4/8/24 10:02, ni.liqiang 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.
please describe in detail "friendly to the hardware" means ..

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

you are not specifying how the correctness is compromised ..
again what the "properly" mean here ? what is the exact failure that
you are seeing ? please document ..

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

where are the experimental results ? any particular reason those
results are not documented here ?

>
> Thanks.
>
> Signed-off-by: ni.liqiang <ni_liqiang@....com>
> Reviewed-by: jin.qi <jin.qi@....com.cn>
> Tested-by: jin.qi <jin.qi@....com.cn>
> Cc: ni.liqiang <ni.liqiang@....com.cn>
> ---
>   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;
> +
>   			if (unlikely(vq->use_dma_api)) {
>   				vq->packed.desc_extra[curr].addr = addr;
>   				vq->packed.desc_extra[curr].len = sg->length;

-ck