[dpdk-dev] [PATCH v2 5/7] net/virtio_user: add vhost kernel support

[Yuanhan Liu]

On Fri, Dec 23, 2016 at 07:14:24AM +0000, Jianfeng Tan wrote:
> + *   Copyright(c) 2010-2016 Intel Corporation. All rights reserved.
                     ^^^^^^^^^

I think it should be 2016.


> +/* By default, vhost kernel module allows 64 regions, but DPDK allows
> + * 256 segments. As a relief, below function merges those virtually
> + * adjacent memsegs into one region.
> + */
> +static struct vhost_memory_kernel *
> +prepare_vhost_memory_kernel(void)
> +{
> +	uint32_t i, j, k = 0;
> +	struct rte_memseg *seg;
> +	struct vhost_memory_region *mr;
> +	struct vhost_memory_kernel *vm;
> +
> +	vm = malloc(sizeof(struct vhost_memory_kernel) +
> +		    VHOST_KERNEL_MAX_REGIONS *
> +		    sizeof(struct vhost_memory_region));
> +
> +	for (i = 0; i < RTE_MAX_MEMSEG; ++i) {
> +		seg = &rte_eal_get_configuration()->mem_config->memseg[i];
> +		if (!seg->addr)
> +			break;
> +
> +		int new_region = 1;
> +
> +		for (j = 0; j < k; ++j) {
> +			mr = &vm->regions[j];
> +
> +			if (mr->userspace_addr + mr->memory_size ==
> +			    (uint64_t)seg->addr) {
> +				mr->memory_size += seg->len;
> +				new_region = 0;
> +				break;
> +			}
> +
> +			if ((uint64_t)seg->addr + seg->len ==
> +			    mr->userspace_addr) {
> +				mr->guest_phys_addr = (uint64_t)seg->addr;

Be careful, such cast would break 32 bit OS build.

> +static int
> +vhost_kernel_ioctl(struct virtio_user_dev *dev,
> +		   enum vhost_user_request req,
> +		   void *arg)
> +{
> +	int i, ret = -1;
> +	uint64_t req_kernel;
> +	struct vhost_memory_kernel *vm = NULL;
> +
> +	req_kernel = vhost_req_user_to_kernel[req];
> +
> +	if (req_kernel == VHOST_SET_MEM_TABLE) {
> +		vm = prepare_vhost_memory_kernel();
> +		if (!vm)
> +			return -1;
> +		arg = (void *)vm;
> +	}
> +
> +	/* Does not work when VIRTIO_F_IOMMU_PLATFORM now, why? */

Because this feature need the vhost IOTLB support from the device
emulation. Patches for QEMU hasn't been merged yet, but it has been
there for a while.

Since we don't have the support yet, for sure it won't work. But
I'm wondering why you have to disable it explicitly?

> +	if (req_kernel == VHOST_SET_FEATURES)
> +		*(uint64_t *)arg &= ~(1ULL << VIRTIO_F_IOMMU_PLATFORM);
> +
> +	for (i = 0; i < VHOST_KERNEL_MAX_QUEUES; ++i) {
> +		if (dev->vhostfds[i] < 0)
> +			continue;
> +
> +		ret = ioctl(dev->vhostfds[i], req_kernel, arg);
> +		if (ret < 0)
> +			break;
> +	}
> +
> +	if (vm)
> +		free(vm);
> +
> +	return ret;
> +}
> +
> +/**
> + * Set up environment to talk with a vhost kernel backend.
> + *
> + * @return
> + *   - (-1) if fail to set up;
> + *   - (>=0) if successful.
> + */
> +static int
> +vhost_kernel_setup(struct virtio_user_dev *dev)
> +{
> +	int vhostfd;
> +	uint32_t q;

Why not simply use 'i'?

> +static int
> +vhost_kernel_enable_queue_pair(struct virtio_user_dev *dev,
> +			       uint16_t pair_idx,
> +			       int enable)
> +{
> +	unsigned int features;

Better to rename it to "tap_features" or something? When I first saw
such name, I thought it was about vhost features.

	--yliu