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

14 Aug 2024

      On 19/07/2024 23:29, Stefano Brivio wrote:
...
On Fri, 12 Jul 2024 17:32:43 +0200
Laurent Vivier  wrote:
...
Add vhost_user.c and vhost_user.h that define the functions needed
to implement vhost-user backend.
Signed-off-by: Laurent Vivier 
---
  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