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

[Jason Wang <jasowang@redhat.com>]

On 2017年01月10日 14:11, Tan, Jianfeng wrote:
>
> Hi Jason,
>
>
> On 1/9/2017 12:39 PM, Jason Wang wrote:
>>
>>
>> On 2016年12月23日 15:14, Jianfeng Tan wrote:
>>> This patch add support vhost kernel as the backend for virtio_user.
>>> Three main hook functions are added:
>>>    - vhost_kernel_setup() to open char device, each vq pair needs one
>>>      vhostfd;
>>>    - vhost_kernel_ioctl() to communicate control messages with vhost
>>>      kernel module;
>>>    - vhost_kernel_enable_queue_pair() to open tap device and set it
>>>      as the backend of corresonding vhost fd (that is to say, vq pair).
>>>
>>> Signed-off-by: Jianfeng Tan <jianfeng.tan@intel.com>
>>> ---
>>>
> [...]
>>> +/* TUNSETIFF ifr flags */
>>> +#define IFF_TAP          0x0002
>>> +#define IFF_NO_PI        0x1000
>>> +#define IFF_ONE_QUEUE    0x2000
>>> +#define IFF_VNET_HDR     0x4000
>>> +#define IFF_MULTI_QUEUE  0x0100
>>> +#define IFF_ATTACH_QUEUE 0x0200
>>> +#define IFF_DETACH_QUEUE 0x0400
>>
>> Do we really want to duplicate those things which has been exposed by 
>> uapi here?
>
> You mean those defined in <linux/if_tun.h>? Redefine those common 
> macros, or include standard header file, with respective pros and 
> cons. DPDK prefers the redefinition way as far as I understand, 
> doesn't it?
>

Well, if you really want to do this, you may want to use an independent 
file. Then you can sync it with linux headers with a bash script.

>>
>>> +
>>> +/* Constants */
>>> +#define TUN_DEF_SNDBUF    (1ull << 20)
>>> +#define PATH_NET_TUN    "/dev/net/tun"
>>> +#define VHOST_KERNEL_MAX_REGIONS    64
>>
>> Unfortunate not a constant any more since c9ce42f72fd0 vhost: add 
>> max_mem_regions module parameter.
>
> Yes, I was considering to ignore this in the initial release. But it's 
> not a big effort, I'll try to fix it in latest version.
>
>>
>>> +
>>> +static uint64_t vhost_req_user_to_kernel[] = {
>>> +    [VHOST_USER_SET_OWNER] = VHOST_SET_OWNER,
>>> +    [VHOST_USER_RESET_OWNER] = VHOST_RESET_OWNER,
>>> +    [VHOST_USER_SET_FEATURES] = VHOST_SET_FEATURES,
>>> +    [VHOST_USER_GET_FEATURES] = VHOST_GET_FEATURES,
>>> +    [VHOST_USER_SET_VRING_CALL] = VHOST_SET_VRING_CALL,
>>> +    [VHOST_USER_SET_VRING_NUM] = VHOST_SET_VRING_NUM,
>>> +    [VHOST_USER_SET_VRING_BASE] = VHOST_SET_VRING_BASE,
>>> +    [VHOST_USER_GET_VRING_BASE] = VHOST_GET_VRING_BASE,
>>> +    [VHOST_USER_SET_VRING_ADDR] = VHOST_SET_VRING_ADDR,
>>> +    [VHOST_USER_SET_VRING_KICK] = VHOST_SET_VRING_KICK,
>>> +    [VHOST_USER_SET_MEM_TABLE] = VHOST_SET_MEM_TABLE,
>>> +};
>>> +
>>> +/* 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;
>>
>> If we're sure the number of regions is less than 64(or the module 
>> parameter read from /sys), can we avoid the iteration here?
>
> The "if" statement under the "for" statement can save us from all 
> RTE_MAX_MEMSEG iteration.

But if we know the number of regions is short than the limit, there's 
even no need for this?

>
>>
>>> +
>>> +        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;
>>> +                mr->userspace_addr = (uint64_t)seg->addr;
>>> +                mr->memory_size += seg->len;
>>> +                new_region = 0;
>>> +                break;
>>> +            }
>>> +        }
>>> +
>>> +        if (new_region == 0)
>>> +            continue;
>>> +
>>> +        mr = &vm->regions[k++];
>>> +        mr->guest_phys_addr = (uint64_t)seg->addr; /* use vaddr 
>>> here! */
>>> +        mr->userspace_addr = (uint64_t)seg->addr;
>>> +        mr->memory_size = seg->len;
>>> +        mr->mmap_offset = 0;
>>> +
>>> +        if (k >= VHOST_KERNEL_MAX_REGIONS) {
>>> +            free(vm);
>>> +            return NULL;
>>> +        }
>>> +    }
>>> +
>>> +    vm->nregions = k;
>>> +    vm->padding = 0;
>>> +    return vm;
>>> +}
>>> +
>>> +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? */
>>
>> I think the reason is when VIRTIO_F_IOMMU_PLATFORM is negotiated, all 
>> address should be iova instead of gpa.
>>
>
> Yes, I agree. As we don't have to do memory protection in a single 
> process, so it's completely useless here, right?

Yes if there's no IOMMU concept in this case.

>
>>> +    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;
>>> +
> [...]
>>> +    if (!enable) {
>>> +        if (dev->tapfds[pair_idx]) {
>>> +            close(dev->tapfds[pair_idx]);
>>> +            dev->tapfds[pair_idx] = -1;
>>> +        }
>>> +        return vhost_kernel_set_backend(vhostfd, -1);
>>
>> If this is used to for thing like ethtool -L in guest, we should use 
>> TUNSETQUEUE here.
>
> Oops, I was missing this ioctl operation. Let me fix it in next version.
>
>>
>>> +    } else if (dev->tapfds[pair_idx] >= 0) {
>>> +        return 0;
>>> +    }
>>> +
>>> +    if ((dev->features & (1ULL << VIRTIO_NET_F_MRG_RXBUF)) ||
>>> +        (dev->features & (1ULL << VIRTIO_F_VERSION_1)))
>>> +        hdr_size = sizeof(struct virtio_net_hdr_mrg_rxbuf);
>>> +    else
>>> +        hdr_size = sizeof(struct virtio_net_hdr);
>>> +
>>> +    /* TODO:
>>> +     * 1. verify we can get/set vnet_hdr_len, tap_probe_vnet_hdr_len
>>> +     * 2. get number of memory regions from vhost module parameter
>>> +     * max_mem_regions, supported in newer version linux kernel
>>> +     */
>>> +    tapfd = open(PATH_NET_TUN, O_RDWR);
>>> +    if (tapfd < 0) {
>>> +        PMD_DRV_LOG(ERR, "fail to open %s: %s",
>>> +                PATH_NET_TUN, strerror(errno));
>>> +        return -1;
>>> +    }
>>> +
>>> +    /* Construct ifr */
>>> +    memset(&ifr, 0, sizeof(ifr));
>>> +    ifr.ifr_flags = IFF_TAP | IFF_NO_PI;
>>> +
>>> +    if (ioctl(tapfd, TUNGETFEATURES, &features) == -1) {
>>> +        PMD_DRV_LOG(ERR, "TUNGETFEATURES failed: %s", 
>>> strerror(errno));
>>> +        goto error;
>>> +    }
>>> +    if (features & IFF_ONE_QUEUE)
>>> +        ifr.ifr_flags |= IFF_ONE_QUEUE;
>>> +
>>> +    /* Let tap instead of vhost-net handle vnet header, as the 
>>> latter does
>>> +     * not support offloading. And in this case, we should not set 
>>> feature
>>> +     * bit VHOST_NET_F_VIRTIO_NET_HDR.
>>> +     */
>>> +    if (features & IFF_VNET_HDR) {
>>> +        ifr.ifr_flags |= IFF_VNET_HDR;
>>> +    } else {
>>> +        PMD_DRV_LOG(ERR, "TAP does not support IFF_VNET_HDR");
>>> +        goto error;
>>> +    }
>>> +
>>> +    if (dev->ifname)
>>> +        strncpy(ifr.ifr_name, dev->ifname, IFNAMSIZ);
>>> +    else
>>> +        strncpy(ifr.ifr_name, "tap%d", IFNAMSIZ);
>>> +    if (ioctl(tapfd, TUNSETIFF, (void *)&ifr) == -1) {
>>> +        PMD_DRV_LOG(ERR, "TUNSETIFF failed: %s", strerror(errno));
>>> +        goto error;
>>> +    }
>>
>> This requires CAP_NET_ADMIN, so we should really consider to accept a 
>> pre-created fd here.
>
> It sounds like a future work for me. So far, all DPDK apps are running 
> in privileged mode (including CAP_NET_ADMIN?).
>

That's not safe. Accepting a per-created fd can solve this, and can even 
support macvtap.

>>
>>> +
>>> +    fcntl(tapfd, F_SETFL, O_NONBLOCK);
>>> +
>>> +    if (ioctl(tapfd, TUNSETVNETHDRSZ, &hdr_size) < 0) {
>>> +        PMD_DRV_LOG(ERR, "TUNSETVNETHDRSZ failed: %s", 
>>> strerror(errno));
>>> +        goto error;
>>> +    }
>>> +
>>> +    if (ioctl(tapfd, TUNSETSNDBUF, &sndbuf) < 0) {
>>> +        PMD_DRV_LOG(ERR, "TUNSETSNDBUF failed: %s", strerror(errno));
>>> +        goto error;
>>> +    }
>>
>> Let's use INT_MAX as default here to survive from evil consumer here.
>
> Oh yes, I will fix it.
>
> Thanks,
> Jianfeng