Re: [PATCH 5/5] vhost-user: add vhost-user

[Laurent Vivier <lvivier@redhat.com>]

On 24/06/2024 07:05, David Gibson wrote:
> On Fri, Jun 21, 2024 at 04:56:40PM +0200, Laurent Vivier wrote:
>> add virtio and vhost-user functions to connect with QEMU.
>>
>>    $ ./passt --vhost-user
>>
>> and
>>
>>    # qemu-system-x86_64 ... -m 4G \
>>          -object memory-backend-memfd,id=memfd0,share=on,size=4G \
>>          -numa node,memdev=memfd0 \
>>          -chardev socket,id=chr0,path=/tmp/passt_1.socket \
>>          -netdev vhost-user,id=netdev0,chardev=chr0 \
>>          -device virtio-net,mac=9a:2b:2c:2d:2e:2f,netdev=netdev0 \
>>          ...
>>
>> Signed-off-by: Laurent Vivier <lvivier@redhat.com>
>> ---
>>   Makefile       |   4 +-
>>   checksum.c     |   1 -
>>   conf.c         |  18 +-
>>   iov.c          |   1 -
>>   packet.c       |   6 +
>>   packet.h       |   2 +
>>   passt.c        |  12 +-
>>   passt.h        |   2 +
>>   pcap.c         |   1 -
>>   tap.c          |  87 ++++++--
>>   tap.h          |   3 +-
>>   tcp.c          |  17 +-
>>   tcp_vu.c       | 547 +++++++++++++++++++++++++++++++++++++++++++++++++
>>   tcp_vu.h       |   9 +
>>   udp.c          |  54 +++--
>>   udp_internal.h |  39 ++++
>>   udp_vu.c       | 237 +++++++++++++++++++++
>>   udp_vu.h       |   8 +
>>   vhost_user.c   |   6 -
>>   virtio.c       |   1 -
>>   20 files changed, 988 insertions(+), 67 deletions(-)
>>   create mode 100644 tcp_vu.c
>>   create mode 100644 tcp_vu.h
>>   create mode 100644 udp_internal.h
>>   create mode 100644 udp_vu.c
>>   create mode 100644 udp_vu.h
>>
...
>> diff --git a/tcp_vu.c b/tcp_vu.c
>> new file mode 100644
>> index 000000000000..f27890f63c0e
>> --- /dev/null
>> +++ b/tcp_vu.c
>> @@ -0,0 +1,547 @@
>> +// SPDX-License-Identifier: GPL-2.0-or-later
> 
> Needs Copyright notice, author information and general description here.
> 
>> +
>> +#include <errno.h>
>> +#include <stddef.h>
>> +#include <stdint.h>
>> +
>> +#include <netinet/ip.h>
>> +
>> +#include <sys/socket.h>
>> +
>> +#include <linux/tcp.h>
>> +#include <linux/virtio_net.h>
>> +
>> +#include "util.h"
>> +#include "ip.h"
>> +#include "passt.h"
>> +#include "siphash.h"
>> +#include "inany.h"
>> +#include "vhost_user.h"
>> +#include "tcp.h"
>> +#include "pcap.h"
>> +#include "flow.h"
>> +#include "tcp_conn.h"
>> +#include "flow_table.h"
>> +#include "tcp_vu.h"
>> +#include "tcp_internal.h"
>> +#include "checksum.h"
>> +
>> +#define CONN_V4(conn)		(!!inany_v4(&(conn)->faddr))
>> +#define CONN_V6(conn)		(!CONN_V4(conn))
>> +
>> +/**
>> + * struct tcp_payload_t - TCP header and data to send segments with payload
>> + * @th:		TCP header
>> + * @data:	TCP data
>> + */
>> +struct tcp_payload_t {
>> +	struct tcphdr th;
>> +	uint8_t data[IP_MAX_MTU - sizeof(struct tcphdr)];
>> +};
> 
> This could be common with tcp_buf.c, couldn't it?

In fact, no: because we cannot have the ((aligned)) attribute as the address is provided 
by the guest.

> 
>> +
>> +/**
>> + * struct tcp_flags_t - TCP header and data to send zero-length
>> + *                      segments (flags)
>> + * @th:		TCP header
>> + * @opts	TCP options
>> + */
>> +struct tcp_flags_t {
>> +	struct tcphdr th;
>> +	char opts[OPT_MSS_LEN + OPT_WS_LEN + 1];
>> +};
> 
> Likewise here.
> 
>> +
>> +/* vhost-user */
>> +static const struct virtio_net_hdr vu_header = {
>> +	.flags = VIRTIO_NET_HDR_F_DATA_VALID,
>> +	.gso_type = VIRTIO_NET_HDR_GSO_NONE,
>> +};
>> +
>> +int tcp_vu_send_flag(struct ctx *c, struct tcp_tap_conn *conn, int flags)
>> +{
>> +	VuDev *vdev = (VuDev *)&c->vdev;
>> +	VuVirtq *vq = &vdev->vq[VHOST_USER_RX_QUEUE];
>> +	size_t tlen, vnet_hdrlen, l4len, optlen;
>> +	struct virtio_net_hdr_mrg_rxbuf *vh;
>> +	struct iovec l2_iov[TCP_NUM_IOVS];
>> +	VuVirtqElement elem;
>> +	struct iovec in_sg;
>> +	struct ethhdr *eh;
>> +	int nb_ack;
>> +	int ret;
>> +
>> +	elem.out_num = 0;
>> +	elem.out_sg = NULL;
>> +	elem.in_num = 1;
>> +	elem.in_sg = &in_sg;
>> +	ret = vu_queue_pop(vdev, vq, &elem);
>> +	if (ret < 0)
>> +		return 0;
>> +
>> +	if (elem.in_num < 1) {
>> +		err("virtio-net receive queue contains no in buffers");
>> +		vu_queue_rewind(vdev, vq, 1);
>> +		return 0;
>> +	}
>> +
>> +	vh = elem.in_sg[0].iov_base;
> 
> AFAICT, the code below requires that in_sg[0] be large enough to
> contain the frame, plus a virtio_net_hdr_mrg_rxbuf.  Seems like that
> we should ASSERT() that somewhere.

I agree

> 
> If I'm understanding correctly that the virtio_net_hdr_mrg_rxbuf is a
> kind of pseudo-header you need for each frame, I'm wondering if it
> could be integrated into the tap_hdr mechanisms.

I tried, but the we use a contiguous buffer rather than the TCP iovec so it's simpler to 
initialize the header in place.

Thanks,
Laurent