From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mga01.intel.com (mga01.intel.com [192.55.52.88]) by dpdk.org (Postfix) with ESMTP id 779713DC for ; Mon, 26 Dec 2016 08:42:51 +0100 (CET) Received: from orsmga001.jf.intel.com ([10.7.209.18]) by fmsmga101.fm.intel.com with ESMTP; 25 Dec 2016 23:42:50 -0800 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.33,404,1477983600"; d="scan'208";a="1076321475" Received: from yliu-dev.sh.intel.com (HELO yliu-dev) ([10.239.67.162]) by orsmga001.jf.intel.com with ESMTP; 25 Dec 2016 23:42:49 -0800 Date: Mon, 26 Dec 2016 15:44:37 +0800 From: Yuanhan Liu To: Jianfeng Tan Cc: dev@dpdk.org, ferruh.yigit@intel.com, cunming.liang@intel.com Message-ID: <20161226074437.GD19288@yliu-dev.sh.intel.com> References: <1480689075-66977-1-git-send-email-jianfeng.tan@intel.com> <1482477266-39199-1-git-send-email-jianfeng.tan@intel.com> <1482477266-39199-6-git-send-email-jianfeng.tan@intel.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <1482477266-39199-6-git-send-email-jianfeng.tan@intel.com> User-Agent: Mutt/1.5.23 (2014-03-12) Subject: Re: [dpdk-dev] [PATCH v2 5/7] net/virtio_user: add vhost kernel support X-BeenThere: dev@dpdk.org X-Mailman-Version: 2.1.15 Precedence: list List-Id: DPDK patches and discussions List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Mon, 26 Dec 2016 07:42:51 -0000 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