Re: [PATCH v2 3/4] vhost-user: introduce vhost-user API

[Laurent Vivier <lvivier@redhat.com>]

On 19/07/2024 23:29, Stefano Brivio wrote:
> On Fri, 12 Jul 2024 17:32:43 +0200
> Laurent Vivier <lvivier@redhat.com> wrote:
> 
>> Add vhost_user.c and vhost_user.h that define the functions needed
>> to implement vhost-user backend.
>>
>> Signed-off-by: Laurent Vivier <lvivier@redhat.com>
>> ---
>>   Makefile     |    4 +-
>>   iov.c        |    1 -
>>   vhost_user.c | 1267 ++++++++++++++++++++++++++++++++++++++++++++++++++
>>   vhost_user.h |  197 ++++++++
>>   virtio.c     |    5 -
>>   virtio.h     |    2 +-
>>   6 files changed, 1467 insertions(+), 9 deletions(-)
>>   create mode 100644 vhost_user.c
>>   create mode 100644 vhost_user.h
>>
...
>> +/**
>> + * vu_send() - Send a buffer to the front-end using the RX virtqueue
>> + * @vdev:	vhost-user device
>> + * @buf:	address of the buffer
>> + * @size:	size of the buffer
>> + *
>> + * Return: number of bytes sent, -1 if there is an error
>> + */
>> +/* cppcheck-suppress unusedFunction */
>> +int vu_send(struct vu_dev *vdev, const void *buf, size_t size)
>> +{
>> +	size_t hdrlen = vdev->hdrlen;
>> +	struct vu_virtq *vq = &vdev->vq[VHOST_USER_RX_QUEUE];
>> +	struct vu_virtq_element elem[VIRTQUEUE_MAX_SIZE];
>> +	struct iovec in_sg[VIRTQUEUE_MAX_SIZE];
>> +	size_t lens[VIRTQUEUE_MAX_SIZE];
>> +	size_t offset;
>> +	int i, j;
>> +	__virtio16 *num_buffers_ptr;
>> +	int in_sg_count;
> 
> Can those be aligned in the usual way (from longest to shortest)?
> 
>> +
>> +	debug("vu_send size %zu hdrlen %zu", size, hdrlen);
>> +
>> +	if (!vu_queue_enabled(vq) || !vu_queue_started(vq)) {
>> +		err("Got packet, but no available descriptors on RX virtq.");
>> +		return 0;
>> +	}
>> +
>> +	offset = 0;
>> +	i = 0;
>> +	num_buffers_ptr = NULL;
>> +	in_sg_count = 0;
> 
> Could those be initialised when you declare them?
> 
>> +	while (offset < size) {
>> +		size_t len;
>> +		int total;
>> +		int ret;
>> +
>> +		total = 0;
>> +
>> +		if (i == ARRAY_SIZE(elem) ||
>> +		    in_sg_count == ARRAY_SIZE(in_sg)) {
>> +			err("virtio-net unexpected long buffer chain");
>> +			goto err;
>> +		}
>> +
>> +		elem[i].out_num = 0;
>> +		elem[i].out_sg = NULL;
>> +		elem[i].in_num = ARRAY_SIZE(in_sg) - in_sg_count;
>> +		elem[i].in_sg = &in_sg[in_sg_count];
>> +
>> +		ret = vu_queue_pop(vdev, vq, &elem[i]);
>> +		if (ret < 0) {
>> +			if (vu_wait_queue(vq) != -1)
>> +				continue;
>> +			if (i) {
>> +				err("virtio-net unexpected empty queue: "
>> +				    "i %d mergeable %d offset %zd, size %zd, "
>> +				    "features 0x%" PRIx64,
>> +				    i, vu_has_feature(vdev,
>> +						      VIRTIO_NET_F_MRG_RXBUF),
>> +				    offset, size, vdev->features);
>> +			}
>> +			offset = -1;
>> +			goto err;
>> +		}
>> +		in_sg_count += elem[i].in_num;
>> +
>> +		if (elem[i].in_num < 1) {
>> +			err("virtio-net receive queue contains no in buffers");
>> +			vu_queue_detach_element(vdev, vq, elem[i].index, 0);
>> +			offset = -1;
>> +			goto err;
>> +		}
>> +
>> +		if (i == 0) {
>> +			struct virtio_net_hdr hdr = {
>> +				.flags = VIRTIO_NET_HDR_F_DATA_VALID,
>> +				.gso_type = VIRTIO_NET_HDR_GSO_NONE,
>> +			};
>> +
>> +			ASSERT(offset == 0);
>> +			ASSERT(elem[i].in_sg[0].iov_len >= hdrlen);
>> +
>> +			len = iov_from_buf(elem[i].in_sg, elem[i].in_num, 0,
>> +					   &hdr, sizeof(hdr));
>> +
>> +			num_buffers_ptr = (__virtio16 *)((char *)elem[i].in_sg[0].iov_base +
>> +							 len);
>> +
>> +			total += hdrlen;
> 
> Shouldn't this be 'total += len' or, alternatively, shouldn't there be
> a check that len == hdrlen?

len is sizeof(virtio_net_hdr) but hdrlen can be either sizeof(struct virtio_net_hdr) or 
sizeof(struct virtio_net_hdr_mrg_rxbuf). It depends on VIRTIO_NET_F_MRG_RXBUF.
We actually want to add hdrlen to total.

struct virtio_net_hdr_mrg_rxbuf {
         struct virtio_net_hdr hdr;
         __virtio16 num_buffers; /* Number of merged rx buffers */
};

At this point we initialize hdr, num_buffers will be set later only if hdrlen is 
sizeof(struct virtio_net_hdr_mrg_rxbuf).

Thanks,
Laurent