Patch Comment List
List patch comments
GET /api/patches/295/comments/?format=api&order=id
[ { "id": 635, "web_url": "https://patches.dpdk.org/comment/635/", "msgid": "<54068D47.4090401@igel.co.jp>", "list_archive_url": "https://inbox.dpdk.org/dev/54068D47.4090401@igel.co.jp", "date": "2014-09-03T03:38:47", "subject": "Re: [dpdk-dev] [PATCH 2/3] lib/librte_vhost: vhost library support\n\tto facilitate integration with DPDK accelerated vswitch", "submitter": { "id": 64, "url": "https://patches.dpdk.org/api/people/64/?format=api", "name": "Tetsuya Mukawa", "email": "mukawa@igel.co.jp" }, "content": "Hi Huawei,\n\nI added few comments. Mainly those comments are about source code indent.\n\n(2014/09/02 17:55), Huawei Xie wrote:\n> +\n> +/**\n> + * Structure contains variables relevant to RX/TX virtqueues.\n> + */\n> +struct vhost_virtqueue {\n> +\t/**< descriptor ring. */\n> +\tstruct vring_desc *desc;\n> +\t/**< available ring. */\n> +\tstruct vring_avail *avail;\n> +\t/**< used ring. */\n> +\tstruct vring_used *used;\n> +\t/**< Size of descriptor ring. */\n> +\tuint32_t size;\n> +\t/**< Backend value to determine if device should be started/stopped. */\n> +\tuint32_t backend;\n> +\t/**< Vhost header length (varies depending on RX merge buffers. */\n> +\tuint16_t vhost_hlen;\n> +\t/**< Last index used on the available ring. */\n> +\tvolatile uint16_t last_used_idx;\n> +\t/**< Used for multiple devices reserving buffers. */\n> +\tvolatile uint16_t last_used_idx_res;\n> +\t/**< Currently unused as polling mode is enabled. */\n> +\teventfd_t callfd;\n> +\t/**< Used to notify the guest (trigger interrupt). */\n> +\teventfd_t kickfd;\n> +} __rte_cache_aligned;\n> +\n> +/**\n> + * Information relating to memory regions including offsets to\n> + * addresses in QEMUs memory file.\n> + */\n> +struct virtio_memory_regions {\n> +\t/**< Base guest physical address of region. */\n> +\tuint64_t guest_phys_address;\n> +\t/**< End guest physical address of region. */\n> +\tuint64_t guest_phys_address_end;\n> +\t/**< Size of region. */\n> +\tuint64_t memory_size;\n> +\t/**< Base userspace address of region. */\n> +\tuint64_t userspace_address;\n> +\t/**< Offset of region for address translation. */\n> +\tuint64_t address_offset;\n> +};\n> +\n> +\n> +/**\n> + * Memory structure includes region and mapping information.\n> + */\n> +struct virtio_memory {\n> +\t/**< Base QEMU userspace address of the memory file. */\n> +\tuint64_t base_address;\n> +\t/**< Mapped address of memory file in this process's memory space. */\n> +\tuint64_t mapped_address;\n> +\t/**< Total size of memory file. */\n> +\tuint64_t mapped_size;\n> +\t/**< Number of memory regions. */\n> +\tuint32_t nregions;\n> +\t/**< Memory region information. */\n> +\tstruct virtio_memory_regions regions[0];\n> +};\n> +\n> +/**\n> + * Device structure contains all configuration information relating to the device.\n> + */\n> +struct virtio_net {\n> +\t/**< Contains all virtqueue information. */\n> +\tstruct vhost_virtqueue *virtqueue[VIRTIO_QNUM];\n> +\t/**< QEMU memory and memory region information. */\n> +\tstruct virtio_memory *mem;\n> +\t/**< Negotiated feature set. */\n> +\tuint64_t features;\n> +\t/**< Device identifier. */\n> +\tuint64_t device_fh;\n> +\t/**< Device flags, used to check if device is running on data core. */\n> +\tuint32_t flags;\n> +\tvoid *priv;\n> +} __rte_cache_aligned;\n> +\nAbove comments of 'vhost_virtqueue', 'struct virtio_memory_regions',\n'struct virtio-memory'\nand 'struct virtio-net' will break API documentation created by Doxygen.\nNot to break, I guess those comment need to be placed after variable.\n\n> +/**\n> + * cuse_info is populated and used to register the cuse device.\n> + * vhost_net_device_ops is passed when the device is registered in main.c.\n> + */\n> +int\n> +rte_vhost_driver_register(const char *dev_name)\n> +{\n> +\tstruct cuse_info cuse_info;\n> +\tchar device_name[PATH_MAX] = \"\";\n> +\tchar char_device_name[PATH_MAX] = \"\";\n> +\tconst char *device_argv[] = { device_name };\n> +\n> +\tchar fuse_opt_dummy[] = FUSE_OPT_DUMMY;\n> +\tchar fuse_opt_fore[] = FUSE_OPT_FORE;\n> +\tchar fuse_opt_nomulti[] = FUSE_OPT_NOMULTI;\n> +\tchar *fuse_argv[] = {fuse_opt_dummy, fuse_opt_fore, fuse_opt_nomulti};\n> +\n> +\tif (access(cuse_device_name, R_OK | W_OK) < 0) {\n> +\t\tRTE_LOG(ERR, VHOST_CONFIG,\n> +\t\t\"Character device %s can't be accessed, maybe not exist\\n\",\n> +\t\tcuse_device_name);\nNeed one more indent?\n\n> +/**\n> + * Locate the file containing QEMU's memory space and map it to our address space.\n> + */\n> +static int\n> +host_memory_map(struct virtio_net *dev, struct virtio_memory *mem, pid_t pid,\n> +\t\tuint64_t addr)\n> +{\n> +\tstruct dirent *dptr = NULL;\n> +\tstruct procmap procmap;\n> +\tDIR *dp = NULL;\n> +\tint fd;\n> +\tint i;\n> +\tchar memfile[PATH_MAX];\n> +\tchar mapfile[PATH_MAX];\n> +\tchar procdir[PATH_MAX];\n> +\tchar resolved_path[PATH_MAX];\n> +\tFILE *fmap;\n> +\tvoid *map;\n> +\tuint8_t\tfound = 0;\n> +\tchar line[BUFSIZE];\n> +\tchar dlm[] = \"- : \";\n> +\tchar *str, *sp, *in[PROCMAP_SZ];\n> +\tchar *end = NULL;\n> +\n> +\t/* Path where mem files are located. */\n> +\tsnprintf(procdir, PATH_MAX, \"/proc/%u/fd/\", pid);\n> +\t/* Maps file used to locate mem file. */\n> +\tsnprintf(mapfile, PATH_MAX, \"/proc/%u/maps\", pid);\n> +\n> +\tfmap = fopen(mapfile, \"r\");\n> +\tif (fmap == NULL) {\n> +\t\tRTE_LOG(ERR, VHOST_CONFIG,\n> +\t\t\t\"(%\"PRIu64\") Failed to open maps file for pid %d\\n\",\n> +\t\tdev->device_fh, pid);\n> +\t\treturn -1;\n> +\t}\n> +\n> +\t/* Read through maps file until we find out base_address. */\n> +\twhile (fgets(line, BUFSIZE, fmap) != 0) {\n> +\t\tstr = line;\n> +\t\terrno = 0;\n> +\t\t/* Split line in to fields. */\n> +\t\tfor (i = 0; i < PROCMAP_SZ; i++) {\n> +\t\t\tin[i] = strtok_r(str, &dlm[i], &sp);\n> +\t\t\tif ((in[i] == NULL) || (errno != 0)) {\n> +\t\t\t\tfclose(fmap);\n> +\t\t\t\treturn -1;\n> +\t\t\t}\n> +\t\t\tstr = NULL;\n> +\t\t}\n> +\n> +\t\t/* Convert/Copy each field as needed. */\n> +\t\tprocmap.va_start = strtoull(in[0], &end, 16);\n> +\t\tif ((in[0] == '\\0') || (end == NULL) || (*end != '\\0') ||\n> +\t\t\t(errno != 0)) {\n> +\t\t\tfclose(fmap);\n> +\t\t\treturn -1;\n> +\t\t}\n> +\n> +\t\tprocmap.len = strtoull(in[1], &end, 16);\n> +\t\tif ((in[1] == '\\0') || (end == NULL) || (*end != '\\0') ||\n> +\t\t\t(errno != 0)) {\n> +\t\t\tfclose(fmap);\n> +\t\t\treturn -1;\n> +\t\t}\n> +\n> +\t\tprocmap.pgoff = strtoull(in[3], &end, 16);\n> +\t\tif ((in[3] == '\\0') || (end == NULL) || (*end != '\\0') ||\n> +\t\t\t(errno != 0)) {\n> +\t\t\tfclose(fmap);\n> +\t\t\treturn -1;\n> +\t\t}\n> +\n> +\t\tprocmap.maj = strtoul(in[4], &end, 16);\n> +\t\tif ((in[4] == '\\0') || (end == NULL) || (*end != '\\0') ||\n> +\t\t\t(errno != 0)) {\n> +\t\t\tfclose(fmap);\n> +\t\t\treturn -1;\n> +\t\t}\n> +\n> +\t\tprocmap.min = strtoul(in[5], &end, 16);\n> +\t\tif ((in[5] == '\\0') || (end == NULL) || (*end != '\\0') ||\n> +\t\t\t(errno != 0)) {\n> +\t\t\tfclose(fmap);\n> +\t\t\treturn -1;\n> +\t\t}\n> +\n> +\t\tprocmap.ino = strtoul(in[6], &end, 16);\n> +\t\tif ((in[6] == '\\0') || (end == NULL) || (*end != '\\0') ||\n> +\t\t\t(errno != 0)) {\n> +\t\t\tfclose(fmap);\n> +\t\t\treturn -1;\n> +\t\t}\n> +\n> +\t\tmemcpy(&procmap.prot, in[2], PROT_SZ);\n> +\t\tmemcpy(&procmap.fname, in[7], PATH_MAX);\n> +\n> +\t\tif (procmap.va_start == addr) {\n> +\t\t\tprocmap.len = procmap.len - procmap.va_start;\n> +\t\t\tfound = 1;\n> +\t\t\tbreak;\n> +\t\t}\n> +\t}\n> +\tfclose(fmap);\n> +\n> +\tif (!found) {\n> +\t\tRTE_LOG(ERR, VHOST_CONFIG,\n> +\t\t\"(%\"PRIu64\") Failed to find memory file in pid %d maps file\\n\",\n> +\t\tdev->device_fh, pid);\nNeed one more indent?\n\n>\n> +/**\n> + * Searches the configuration core linked list and retrieves the device if it exists.\n> + */\n> +static struct virtio_net *\n> +get_device(struct vhost_device_ctx ctx)\n> +{\n> +\tstruct virtio_net_config_ll *ll_dev;\n> +\n> +\tll_dev = get_config_ll_entry(ctx);\n> +\n> +\t/* If a matching entry is found in the linked list, return the device\n> +\t* in that entry. */\nNeed a blank space at start of comment?\n\n> +\tif (ll_dev)\n> +\t\treturn &ll_dev->dev;\n> +\n> +\tRTE_LOG(ERR, VHOST_CONFIG,\n> +\t\t\"(%\"PRIu64\") Device not found in linked list.\\n\", ctx.fh);\n> +\treturn NULL;\n> +}\n> +\n> +/**\n> + * Add entry containing a device to the device configuration linked list.\n> + */\n> +static void\n> +add_config_ll_entry(struct virtio_net_config_ll *new_ll_dev)\n> +{\n> +\tstruct virtio_net_config_ll *ll_dev = ll_root;\n> +\n> +\t/* If ll_dev == NULL then this is the first device so go to else */\n> +\tif (ll_dev) {\n> +\t\t/* If the 1st device_fh != 0 then we insert our device here. */\n> +\t\tif (ll_dev->dev.device_fh != 0)\t{\n> +\t\t\tnew_ll_dev->dev.device_fh = 0;\n> +\t\t\tnew_ll_dev->next = ll_dev;\n> +\t\t\tll_root = new_ll_dev;\n> +\t\t} else {\n> +\t\t\t/* Increment through the ll until we find un unused\n> +\t\t\t* device_fh. Insert the device at that entry*/\nNeed a blank space at end of comment?\n\n>\n> +/**\n> + * Function is called from the CUSE release function. This function will cleanup\n> + * the device and remove it from device configuration linked list.\n> + */\n> +static void\n> +destroy_device(struct vhost_device_ctx ctx)\n> +{\n> +\tstruct virtio_net_config_ll *ll_dev_cur_ctx, *ll_dev_last = NULL;\n> +\tstruct virtio_net_config_ll *ll_dev_cur = ll_root;\n> +\n> +\t/* Find the linked list entry for the device to be removed. */\n> +\tll_dev_cur_ctx = get_config_ll_entry(ctx);\n> +\twhile (ll_dev_cur != NULL) {\n> +\t\t/* If the device is found or a device that doesn't exist is\n> +\t\t* found then it is removed. */\nNeed a blank space?\n\n> +/**\n> + * Called from CUSE IOCTL: VHOST_SET_FEATURES\n> + * We receive the negotiated set of features supported by us and the virtio\n> + * device.\n> + */\n> +static int\n> +set_features(struct vhost_device_ctx ctx, uint64_t *pu)\n> +{\n> +\tstruct virtio_net *dev;\n> +\n> +\tdev = get_device(ctx);\n> +\tif (dev == NULL)\n> +\t\treturn -1;\n> +\tif (*pu & ~VHOST_FEATURES)\n> +\t\treturn -1;\n> +\n> +\t/* Store the negotiated feature list for the device. */\n> +\tdev->features = *pu;\n> +\n> +\t/* Set the vhost_hlen depending on if VIRTIO_NET_F_MRG_RXBUF is set. */\n> +\tif (dev->features & (1 << VIRTIO_NET_F_MRG_RXBUF)) {\n> +\t\tLOG_DEBUG(VHOST_CONFIG,\n> +\t\t\"(%\"PRIu64\") Mergeable RX buffers enabled\\n\", dev->device_fh);\n> +\t\tdev->virtqueue[VIRTIO_RXQ]->vhost_hlen =\n> +\t\t\tsizeof(struct virtio_net_hdr_mrg_rxbuf);\n> +\t\tdev->virtqueue[VIRTIO_TXQ]->vhost_hlen =\n> +\t\t\tsizeof(struct virtio_net_hdr_mrg_rxbuf);\nOne more indent?\n\n>\n> +/**\n> + * Called from CUSE IOCTL: VHOST_SET_MEM_TABLE\n> + * This function creates and populates the memory structure for the device.\n> + * This includes storing offsets used to translate buffer addresses.\n> + */\n> +static int\n> +set_mem_table(struct vhost_device_ctx ctx, const void *mem_regions_addr,\n> +\tuint32_t nregions)\n> +{\n> +\tstruct virtio_net *dev;\n> +\tstruct vhost_memory_region *mem_regions;\n> +\tstruct virtio_memory *mem;\n> +\tuint64_t size = offsetof(struct vhost_memory, regions);\n> +\tuint32_t regionidx, valid_regions;\n> +\n> +\tdev = get_device(ctx);\n> +\tif (dev == NULL)\n> +\t\treturn -1;\n> +\n> +\tif (dev->mem) {\n> +\t\tmunmap((void *)(uintptr_t)dev->mem->mapped_address,\n> +\t\t\t(size_t)dev->mem->mapped_size);\n> +\t\tfree(dev->mem);\n> +\t}\n> +\n> +\t/* Malloc the memory structure depending on the number of regions. */\n> +\tmem = calloc(1, sizeof(struct virtio_memory) +\n> +\t\t(sizeof(struct virtio_memory_regions) * nregions));\n> +\tif (mem == NULL) {\n> +\t\tRTE_LOG(ERR, VHOST_CONFIG,\n> +\t\t\t\"(%\"PRIu64\") Failed to allocate memory for dev->mem.\\n\",\n> +\t\t\tdev->device_fh);\n> +\t\treturn -1;\n> +\t}\n> +\n> +\tmem->nregions = nregions;\n> +\n> +\tmem_regions = (void *)(uintptr_t)\n> +\t\t((uint64_t)(uintptr_t)mem_regions_addr + size);\n> +\n> +\tfor (regionidx = 0; regionidx < mem->nregions; regionidx++) {\n> +\t\t/* Populate the region structure for each region. */\n> +\t\tmem->regions[regionidx].guest_phys_address =\n> +\t\t\tmem_regions[regionidx].guest_phys_addr;\n> +\t\tmem->regions[regionidx].guest_phys_address_end =\n> +\t\t\tmem->regions[regionidx].guest_phys_address +\n> +\t\t\tmem_regions[regionidx].memory_size;\n> +\t\tmem->regions[regionidx].memory_size =\n> +\t\t\tmem_regions[regionidx].memory_size;\n> +\t\tmem->regions[regionidx].userspace_address =\n> +\t\t\tmem_regions[regionidx].userspace_addr;\n> +\n> +\t\tLOG_DEBUG(VHOST_CONFIG,\n> +\t\t\t\"(%\"PRIu64\") REGION: %u - GPA: %p - QEMU VA: %p - SIZE \"\n> +\t\t\t\"(%\"PRIu64\")\\n\",\n> +\t\t\tdev->device_fh, regionidx,\n> +\t\t\t(void *)(uintptr_t)\n> +\t\t\t\tmem->regions[regionidx].guest_phys_address,\n> +\t\t\t(void *)(uintptr_t)\n> +\t\t\t\tmem->regions[regionidx].userspace_address,\n> +\t\t\tmem->regions[regionidx].memory_size);\n> +\n> +\t\t/*set the base address mapping*/\nNeed a blank space at start and end of comment?\n\n> +\t\tif (mem->regions[regionidx].guest_phys_address == 0x0) {\n> +\t\t\tmem->base_address =\n> +\t\t\t\tmem->regions[regionidx].userspace_address;\n> +\t\t\t/* Map VM memory file */\n> +\t\t\tif (host_memory_map(dev, mem, ctx.pid,\n> +\t\t\t\tmem->base_address) != 0) {\n> +\t\t\t\tfree(mem);\n> +\t\t\t\treturn -1;\n> +\t\t\t}\n> +\t\t}\n> +\t}\n> +\n> +\t/* Check that we have a valid base address. */\n> +\tif (mem->base_address == 0) {\n> +\t\tRTE_LOG(ERR, VHOST_CONFIG,\n> +\t\t\"(%\"PRIu64\") Failed to find base address of qemu memory \"\n> +\t\t\"file.\\n\", dev->device_fh);\nOne more indent?\n\n> +\t\tfree(mem);\n> +\t\treturn -1;\n> +\t}\n> +\n> +\t/* Check if all of our regions have valid mappings. Usually one does\n> +\t* not exist in the QEMU memory file. */\nNeed a blank space?\n\n> +\tvalid_regions = mem->nregions;\n> +\tfor (regionidx = 0; regionidx < mem->nregions; regionidx++) {\n> +\t\tif ((mem->regions[regionidx].userspace_address <\n> +\t\t\tmem->base_address) ||\n> +\t\t\t(mem->regions[regionidx].userspace_address >\n> +\t\t\t\t(mem->base_address + mem->mapped_size)))\n> +\t\t\t\tvalid_regions--;\n> +\t}\n> +\n> +\t/* If a region does not have a valid mapping we rebuild our memory\n> +\t* struct to contain only valid entries. */\nNeed a blank space?\n> +\tif (valid_regions != mem->nregions) {\n> +\t\tLOG_DEBUG(VHOST_CONFIG,\n> +\t\t\"(%\"PRIu64\") Not all memory regions exist in the QEMU mem file.\"\n> +\t\t\"Re-populating mem structure\\n\",\nOne more indent?\n\n> +\t\t\tdev->device_fh);\n> +\n> +\t\t/* Re-populate the memory structure with only valid regions.\n> +\t\t* Invalid regions are over-written with memmove. */\nNeed a blank space?\n\nThanks,\nTetsuya", "headers": { "Return-Path": "<mukawa@igel.co.jp>", "Received": [ "from mail-pd0-f173.google.com (mail-pd0-f173.google.com\n\t[209.85.192.173]) by dpdk.org (Postfix) with ESMTP id 00B005913\n\tfor <dev@dpdk.org>; Wed, 3 Sep 2014 05:34:14 +0200 (CEST)", "by mail-pd0-f173.google.com with SMTP id p10so10022331pdj.4\n\tfor <dev@dpdk.org>; Tue, 02 Sep 2014 20:38:49 -0700 (PDT)", "from [10.16.129.101] (napt.igel.co.jp. [219.106.231.132])\n\tby mx.google.com with ESMTPSA id\n\typ10sm15857469pac.18.2014.09.02.20.38.47 for <multiple recipients>\n\t(version=TLSv1 cipher=ECDHE-RSA-RC4-SHA bits=128/128);\n\tTue, 02 Sep 2014 20:38:48 -0700 (PDT)" ], "X-Google-DKIM-Signature": "v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20130820;\n\th=x-gm-message-state:message-id:date:from:user-agent:mime-version:to\n\t:subject:references:in-reply-to:content-type\n\t:content-transfer-encoding;\n\tbh=FKj49HuAjxgyG0RrsS7KgqOKWwN7J2pjMa9jMx23b8I=;\n\tb=RYnZTXFnkg04afqCSetG/K9aJAyl2Uk5CePtGxw2F9RqMYc+O7pVyqLiSIry1p5hcJ\n\teg7cDVDMPUxf9Rz9esYILfYd5T6wXVkVfrS/JJwBrhEYN8O8iwHx+wKjiOSIsCtL1uE0\n\tTYu1py+EGDxajJS9u5/jMfoloudRiLr7rIZDPjuqex0W/1NMFVHkz9EGBDH+lJzAnx9o\n\t5FkcAWdpsXAk4svgPR7HTuX1/+Q5IJ3vX2fEy6Fw6nzILPNKNBW4EVnsmlDtJVNAQWWB\n\to5SvMk2NfVpUuf1OihqNM5lNjzGx8V8kWdbl7K4HYky1Go7fbpYX0sYs5MAWYWJB/ksM\n\tn9xQ==", "X-Gm-Message-State": "ALoCoQlPCMlMJOEVboTJ9GcoGgC5pw3yH4dCEjCp9497eBiGdJK2WoTu1+3/wbzsTo+cNM/YcFa2", "X-Received": "by 10.66.160.233 with SMTP id xn9mr53884745pab.29.1409715528897; \n\tTue, 02 Sep 2014 20:38:48 -0700 (PDT)", "Message-ID": "<54068D47.4090401@igel.co.jp>", "Date": "Wed, 03 Sep 2014 12:38:47 +0900", "From": "\"Tetsuya.Mukawa\" <mukawa@igel.co.jp>", "User-Agent": "Mozilla/5.0 (Windows NT 6.1; WOW64;\n\trv:24.0) Gecko/20100101 Thunderbird/24.6.0", "MIME-Version": "1.0", "To": "Huawei Xie <huawei.xie@intel.com>, dev@dpdk.org", "References": "<1409648131-4301-1-git-send-email-huawei.xie@intel.com>\n\t<1409648131-4301-3-git-send-email-huawei.xie@intel.com>", "In-Reply-To": "<1409648131-4301-3-git-send-email-huawei.xie@intel.com>", "Content-Type": "text/plain; charset=ISO-2022-JP", "Content-Transfer-Encoding": "7bit", "Subject": "Re: [dpdk-dev] [PATCH 2/3] lib/librte_vhost: vhost library support\n\tto facilitate integration with DPDK accelerated vswitch", "X-BeenThere": "dev@dpdk.org", "X-Mailman-Version": "2.1.15", "Precedence": "list", "List-Id": "patches and discussions about DPDK <dev.dpdk.org>", "List-Unsubscribe": "<http://dpdk.org/ml/options/dev>,\n\t<mailto:dev-request@dpdk.org?subject=unsubscribe>", "List-Archive": "<http://dpdk.org/ml/archives/dev/>", "List-Post": "<mailto:dev@dpdk.org>", "List-Help": "<mailto:dev-request@dpdk.org?subject=help>", "List-Subscribe": "<http://dpdk.org/ml/listinfo/dev>,\n\t<mailto:dev-request@dpdk.org?subject=subscribe>", "X-List-Received-Date": "Wed, 03 Sep 2014 03:34:15 -0000" }, "addressed": null }, { "id": 636, "web_url": "https://patches.dpdk.org/comment/636/", "msgid": "<C37D651A908B024F974696C65296B57B0F2847A0@SHSMSX101.ccr.corp.intel.com>", "list_archive_url": "https://inbox.dpdk.org/dev/C37D651A908B024F974696C65296B57B0F2847A0@SHSMSX101.ccr.corp.intel.com", "date": "2014-09-03T05:39:24", "subject": "Re: [dpdk-dev] [PATCH 2/3] lib/librte_vhost: vhost library support\n\tto facilitate integration with DPDK accelerated vswitch", "submitter": { "id": 16, "url": "https://patches.dpdk.org/api/people/16/?format=api", "name": "Huawei Xie", "email": "huawei.xie@intel.com" }, "content": "Thanks Tetsuya:\n\tSome of them are due to 80 character limitation. Is it ok to break the limitation for better indentation?\n\n> -----Original Message-----\n> From: Tetsuya.Mukawa [mailto:mukawa@igel.co.jp]\n> Sent: Wednesday, September 03, 2014 11:39 AM\n> To: Xie, Huawei; dev@dpdk.org\n> Subject: Re: [dpdk-dev] [PATCH 2/3] lib/librte_vhost: vhost library support to\n> facilitate integration with DPDK accelerated vswitch\n> \n> Hi Huawei,\n> \n> I added few comments. Mainly those comments are about source code indent.\n> \n> (2014/09/02 17:55), Huawei Xie wrote:\n> > +\n> > +/**\n> > + * Structure contains variables relevant to RX/TX virtqueues.\n> > + */\n> > +struct vhost_virtqueue {\n> > +\t/**< descriptor ring. */\n> > +\tstruct vring_desc *desc;\n> > +\t/**< available ring. */\n> > +\tstruct vring_avail *avail;\n> > +\t/**< used ring. */\n> > +\tstruct vring_used *used;\n> > +\t/**< Size of descriptor ring. */\n> > +\tuint32_t size;\n> > +\t/**< Backend value to determine if device should be started/stopped. */\n> > +\tuint32_t backend;\n> > +\t/**< Vhost header length (varies depending on RX merge buffers. */\n> > +\tuint16_t vhost_hlen;\n> > +\t/**< Last index used on the available ring. */\n> > +\tvolatile uint16_t last_used_idx;\n> > +\t/**< Used for multiple devices reserving buffers. */\n> > +\tvolatile uint16_t last_used_idx_res;\n> > +\t/**< Currently unused as polling mode is enabled. */\n> > +\teventfd_t callfd;\n> > +\t/**< Used to notify the guest (trigger interrupt). */\n> > +\teventfd_t kickfd;\n> > +} __rte_cache_aligned;\n> > +\n> > +/**\n> > + * Information relating to memory regions including offsets to\n> > + * addresses in QEMUs memory file.\n> > + */\n> > +struct virtio_memory_regions {\n> > +\t/**< Base guest physical address of region. */\n> > +\tuint64_t guest_phys_address;\n> > +\t/**< End guest physical address of region. */\n> > +\tuint64_t guest_phys_address_end;\n> > +\t/**< Size of region. */\n> > +\tuint64_t memory_size;\n> > +\t/**< Base userspace address of region. */\n> > +\tuint64_t userspace_address;\n> > +\t/**< Offset of region for address translation. */\n> > +\tuint64_t address_offset;\n> > +};\n> > +\n> > +\n> > +/**\n> > + * Memory structure includes region and mapping information.\n> > + */\n> > +struct virtio_memory {\n> > +\t/**< Base QEMU userspace address of the memory file. */\n> > +\tuint64_t base_address;\n> > +\t/**< Mapped address of memory file in this process's memory space. */\n> > +\tuint64_t mapped_address;\n> > +\t/**< Total size of memory file. */\n> > +\tuint64_t mapped_size;\n> > +\t/**< Number of memory regions. */\n> > +\tuint32_t nregions;\n> > +\t/**< Memory region information. */\n> > +\tstruct virtio_memory_regions regions[0];\n> > +};\n> > +\n> > +/**\n> > + * Device structure contains all configuration information relating to the\n> device.\n> > + */\n> > +struct virtio_net {\n> > +\t/**< Contains all virtqueue information. */\n> > +\tstruct vhost_virtqueue *virtqueue[VIRTIO_QNUM];\n> > +\t/**< QEMU memory and memory region information. */\n> > +\tstruct virtio_memory *mem;\n> > +\t/**< Negotiated feature set. */\n> > +\tuint64_t features;\n> > +\t/**< Device identifier. */\n> > +\tuint64_t device_fh;\n> > +\t/**< Device flags, used to check if device is running on data core. */\n> > +\tuint32_t flags;\n> > +\tvoid *priv;\n> > +} __rte_cache_aligned;\n> > +\n> Above comments of 'vhost_virtqueue', 'struct virtio_memory_regions',\n> 'struct virtio-memory'\n> and 'struct virtio-net' will break API documentation created by Doxygen.\n> Not to break, I guess those comment need to be placed after variable.\nThanks. I will check doxgen rule. Moved the comment above to avoid 80 character limitation warning.\n> \n> > +/**\n> > + * cuse_info is populated and used to register the cuse device.\n> > + * vhost_net_device_ops is passed when the device is registered in main.c.\n> > + */\n> > +int\n> > +rte_vhost_driver_register(const char *dev_name)\n> > +{\n> > +\tstruct cuse_info cuse_info;\n> > +\tchar device_name[PATH_MAX] = \"\";\n> > +\tchar char_device_name[PATH_MAX] = \"\";\n> > +\tconst char *device_argv[] = { device_name };\n> > +\n> > +\tchar fuse_opt_dummy[] = FUSE_OPT_DUMMY;\n> > +\tchar fuse_opt_fore[] = FUSE_OPT_FORE;\n> > +\tchar fuse_opt_nomulti[] = FUSE_OPT_NOMULTI;\n> > +\tchar *fuse_argv[] = {fuse_opt_dummy, fuse_opt_fore,\n> fuse_opt_nomulti};\n> > +\n> > +\tif (access(cuse_device_name, R_OK | W_OK) < 0) {\n> > +\t\tRTE_LOG(ERR, VHOST_CONFIG,\n> > +\t\t\"Character device %s can't be accessed, maybe not exist\\n\",\n> > +\t\tcuse_device_name);\n> Need one more indent?\nAgain to avoid 80 char warning for some lengthy RTE_LOG.\n> \n> > +/**\n> > + * Locate the file containing QEMU's memory space and map it to our\n> address space.\n> > + */\n> > +static int\n> > +host_memory_map(struct virtio_net *dev, struct virtio_memory *mem, pid_t\n> pid,\n> > +\t\tuint64_t addr)\n> > +{\n> > +\tstruct dirent *dptr = NULL;\n> > +\tstruct procmap procmap;\n> > +\tDIR *dp = NULL;\n> > +\tint fd;\n> > +\tint i;\n> > +\tchar memfile[PATH_MAX];\n> > +\tchar mapfile[PATH_MAX];\n> > +\tchar procdir[PATH_MAX];\n> > +\tchar resolved_path[PATH_MAX];\n> > +\tFILE *fmap;\n> > +\tvoid *map;\n> > +\tuint8_t\tfound = 0;\n> > +\tchar line[BUFSIZE];\n> > +\tchar dlm[] = \"- : \";\n> > +\tchar *str, *sp, *in[PROCMAP_SZ];\n> > +\tchar *end = NULL;\n> > +\n> > +\t/* Path where mem files are located. */\n> > +\tsnprintf(procdir, PATH_MAX, \"/proc/%u/fd/\", pid);\n> > +\t/* Maps file used to locate mem file. */\n> > +\tsnprintf(mapfile, PATH_MAX, \"/proc/%u/maps\", pid);\n> > +\n> > +\tfmap = fopen(mapfile, \"r\");\n> > +\tif (fmap == NULL) {\n> > +\t\tRTE_LOG(ERR, VHOST_CONFIG,\n> > +\t\t\t\"(%\"PRIu64\") Failed to open maps file for pid %d\\n\",\n> > +\t\tdev->device_fh, pid);\n> > +\t\treturn -1;\n> > +\t}\n> > +\n> > +\t/* Read through maps file until we find out base_address. */\n> > +\twhile (fgets(line, BUFSIZE, fmap) != 0) {\n> > +\t\tstr = line;\n> > +\t\terrno = 0;\n> > +\t\t/* Split line in to fields. */\n> > +\t\tfor (i = 0; i < PROCMAP_SZ; i++) {\n> > +\t\t\tin[i] = strtok_r(str, &dlm[i], &sp);\n> > +\t\t\tif ((in[i] == NULL) || (errno != 0)) {\n> > +\t\t\t\tfclose(fmap);\n> > +\t\t\t\treturn -1;\n> > +\t\t\t}\n> > +\t\t\tstr = NULL;\n> > +\t\t}\n> > +\n> > +\t\t/* Convert/Copy each field as needed. */\n> > +\t\tprocmap.va_start = strtoull(in[0], &end, 16);\n> > +\t\tif ((in[0] == '\\0') || (end == NULL) || (*end != '\\0') ||\n> > +\t\t\t(errno != 0)) {\n> > +\t\t\tfclose(fmap);\n> > +\t\t\treturn -1;\n> > +\t\t}\n> > +\n> > +\t\tprocmap.len = strtoull(in[1], &end, 16);\n> > +\t\tif ((in[1] == '\\0') || (end == NULL) || (*end != '\\0') ||\n> > +\t\t\t(errno != 0)) {\n> > +\t\t\tfclose(fmap);\n> > +\t\t\treturn -1;\n> > +\t\t}\n> > +\n> > +\t\tprocmap.pgoff = strtoull(in[3], &end, 16);\n> > +\t\tif ((in[3] == '\\0') || (end == NULL) || (*end != '\\0') ||\n> > +\t\t\t(errno != 0)) {\n> > +\t\t\tfclose(fmap);\n> > +\t\t\treturn -1;\n> > +\t\t}\n> > +\n> > +\t\tprocmap.maj = strtoul(in[4], &end, 16);\n> > +\t\tif ((in[4] == '\\0') || (end == NULL) || (*end != '\\0') ||\n> > +\t\t\t(errno != 0)) {\n> > +\t\t\tfclose(fmap);\n> > +\t\t\treturn -1;\n> > +\t\t}\n> > +\n> > +\t\tprocmap.min = strtoul(in[5], &end, 16);\n> > +\t\tif ((in[5] == '\\0') || (end == NULL) || (*end != '\\0') ||\n> > +\t\t\t(errno != 0)) {\n> > +\t\t\tfclose(fmap);\n> > +\t\t\treturn -1;\n> > +\t\t}\n> > +\n> > +\t\tprocmap.ino = strtoul(in[6], &end, 16);\n> > +\t\tif ((in[6] == '\\0') || (end == NULL) || (*end != '\\0') ||\n> > +\t\t\t(errno != 0)) {\n> > +\t\t\tfclose(fmap);\n> > +\t\t\treturn -1;\n> > +\t\t}\n> > +\n> > +\t\tmemcpy(&procmap.prot, in[2], PROT_SZ);\n> > +\t\tmemcpy(&procmap.fname, in[7], PATH_MAX);\n> > +\n> > +\t\tif (procmap.va_start == addr) {\n> > +\t\t\tprocmap.len = procmap.len - procmap.va_start;\n> > +\t\t\tfound = 1;\n> > +\t\t\tbreak;\n> > +\t\t}\n> > +\t}\n> > +\tfclose(fmap);\n> > +\n> > +\tif (!found) {\n> > +\t\tRTE_LOG(ERR, VHOST_CONFIG,\n> > +\t\t\"(%\"PRIu64\") Failed to find memory file in pid %d maps file\\n\",\n> > +\t\tdev->device_fh, pid);\n> Need one more indent?\n> \n> >\n> > +/**\n> > + * Searches the configuration core linked list and retrieves the device if it\n> exists.\n> > + */\n> > +static struct virtio_net *\n> > +get_device(struct vhost_device_ctx ctx)\n> > +{\n> > +\tstruct virtio_net_config_ll *ll_dev;\n> > +\n> > +\tll_dev = get_config_ll_entry(ctx);\n> > +\n> > +\t/* If a matching entry is found in the linked list, return the device\n> > +\t* in that entry. */\n> Need a blank space at start of comment?\n> \n> > +\tif (ll_dev)\n> > +\t\treturn &ll_dev->dev;\n> > +\n> > +\tRTE_LOG(ERR, VHOST_CONFIG,\n> > +\t\t\"(%\"PRIu64\") Device not found in linked list.\\n\", ctx.fh);\n> > +\treturn NULL;\n> > +}\n> > +\n> > +/**\n> > + * Add entry containing a device to the device configuration linked list.\n> > + */\n> > +static void\n> > +add_config_ll_entry(struct virtio_net_config_ll *new_ll_dev)\n> > +{\n> > +\tstruct virtio_net_config_ll *ll_dev = ll_root;\n> > +\n> > +\t/* If ll_dev == NULL then this is the first device so go to else */\n> > +\tif (ll_dev) {\n> > +\t\t/* If the 1st device_fh != 0 then we insert our device here. */\n> > +\t\tif (ll_dev->dev.device_fh != 0)\t{\n> > +\t\t\tnew_ll_dev->dev.device_fh = 0;\n> > +\t\t\tnew_ll_dev->next = ll_dev;\n> > +\t\t\tll_root = new_ll_dev;\n> > +\t\t} else {\n> > +\t\t\t/* Increment through the ll until we find un unused\n> > +\t\t\t* device_fh. Insert the device at that entry*/\n> Need a blank space at end of comment?\nWill fix.\n> \n> >\n> > +/**\n> > + * Function is called from the CUSE release function. This function will\n> cleanup\n> > + * the device and remove it from device configuration linked list.\n> > + */\n> > +static void\n> > +destroy_device(struct vhost_device_ctx ctx)\n> > +{\n> > +\tstruct virtio_net_config_ll *ll_dev_cur_ctx, *ll_dev_last = NULL;\n> > +\tstruct virtio_net_config_ll *ll_dev_cur = ll_root;\n> > +\n> > +\t/* Find the linked list entry for the device to be removed. */\n> > +\tll_dev_cur_ctx = get_config_ll_entry(ctx);\n> > +\twhile (ll_dev_cur != NULL) {\n> > +\t\t/* If the device is found or a device that doesn't exist is\n> > +\t\t* found then it is removed. */\n> Need a blank space?\nWill fix.\n> \n> > +/**\n> > + * Called from CUSE IOCTL: VHOST_SET_FEATURES\n> > + * We receive the negotiated set of features supported by us and the virtio\n> > + * device.\n> > + */\n> > +static int\n> > +set_features(struct vhost_device_ctx ctx, uint64_t *pu)\n> > +{\n> > +\tstruct virtio_net *dev;\n> > +\n> > +\tdev = get_device(ctx);\n> > +\tif (dev == NULL)\n> > +\t\treturn -1;\n> > +\tif (*pu & ~VHOST_FEATURES)\n> > +\t\treturn -1;\n> > +\n> > +\t/* Store the negotiated feature list for the device. */\n> > +\tdev->features = *pu;\n> > +\n> > +\t/* Set the vhost_hlen depending on if VIRTIO_NET_F_MRG_RXBUF is set.\n> */\n> > +\tif (dev->features & (1 << VIRTIO_NET_F_MRG_RXBUF)) {\n> > +\t\tLOG_DEBUG(VHOST_CONFIG,\n> > +\t\t\"(%\"PRIu64\") Mergeable RX buffers enabled\\n\", dev->device_fh);\n> > +\t\tdev->virtqueue[VIRTIO_RXQ]->vhost_hlen =\n> > +\t\t\tsizeof(struct virtio_net_hdr_mrg_rxbuf);\n> > +\t\tdev->virtqueue[VIRTIO_TXQ]->vhost_hlen =\n> > +\t\t\tsizeof(struct virtio_net_hdr_mrg_rxbuf);\n> One more indent?\n> \n> >\n> > +/**\n> > + * Called from CUSE IOCTL: VHOST_SET_MEM_TABLE\n> > + * This function creates and populates the memory structure for the device.\n> > + * This includes storing offsets used to translate buffer addresses.\n> > + */\n> > +static int\n> > +set_mem_table(struct vhost_device_ctx ctx, const void *mem_regions_addr,\n> > +\tuint32_t nregions)\n> > +{\n> > +\tstruct virtio_net *dev;\n> > +\tstruct vhost_memory_region *mem_regions;\n> > +\tstruct virtio_memory *mem;\n> > +\tuint64_t size = offsetof(struct vhost_memory, regions);\n> > +\tuint32_t regionidx, valid_regions;\n> > +\n> > +\tdev = get_device(ctx);\n> > +\tif (dev == NULL)\n> > +\t\treturn -1;\n> > +\n> > +\tif (dev->mem) {\n> > +\t\tmunmap((void *)(uintptr_t)dev->mem->mapped_address,\n> > +\t\t\t(size_t)dev->mem->mapped_size);\n> > +\t\tfree(dev->mem);\n> > +\t}\n> > +\n> > +\t/* Malloc the memory structure depending on the number of regions. */\n> > +\tmem = calloc(1, sizeof(struct virtio_memory) +\n> > +\t\t(sizeof(struct virtio_memory_regions) * nregions));\n> > +\tif (mem == NULL) {\n> > +\t\tRTE_LOG(ERR, VHOST_CONFIG,\n> > +\t\t\t\"(%\"PRIu64\") Failed to allocate memory for dev-\n> >mem.\\n\",\n> > +\t\t\tdev->device_fh);\n> > +\t\treturn -1;\n> > +\t}\n> > +\n> > +\tmem->nregions = nregions;\n> > +\n> > +\tmem_regions = (void *)(uintptr_t)\n> > +\t\t((uint64_t)(uintptr_t)mem_regions_addr + size);\n> > +\n> > +\tfor (regionidx = 0; regionidx < mem->nregions; regionidx++) {\n> > +\t\t/* Populate the region structure for each region. */\n> > +\t\tmem->regions[regionidx].guest_phys_address =\n> > +\t\t\tmem_regions[regionidx].guest_phys_addr;\n> > +\t\tmem->regions[regionidx].guest_phys_address_end =\n> > +\t\t\tmem->regions[regionidx].guest_phys_address +\n> > +\t\t\tmem_regions[regionidx].memory_size;\n> > +\t\tmem->regions[regionidx].memory_size =\n> > +\t\t\tmem_regions[regionidx].memory_size;\n> > +\t\tmem->regions[regionidx].userspace_address =\n> > +\t\t\tmem_regions[regionidx].userspace_addr;\n> > +\n> > +\t\tLOG_DEBUG(VHOST_CONFIG,\n> > +\t\t\t\"(%\"PRIu64\") REGION: %u - GPA: %p - QEMU VA: %p -\n> SIZE \"\n> > +\t\t\t\"(%\"PRIu64\")\\n\",\n> > +\t\t\tdev->device_fh, regionidx,\n> > +\t\t\t(void *)(uintptr_t)\n> > +\t\t\t\tmem->regions[regionidx].guest_phys_address,\n> > +\t\t\t(void *)(uintptr_t)\n> > +\t\t\t\tmem->regions[regionidx].userspace_address,\n> > +\t\t\tmem->regions[regionidx].memory_size);\n> > +\n> > +\t\t/*set the base address mapping*/\n> Need a blank space at start and end of comment?\nThanks for careful review.\n> \n> > +\t\tif (mem->regions[regionidx].guest_phys_address == 0x0) {\n> > +\t\t\tmem->base_address =\n> > +\t\t\t\tmem->regions[regionidx].userspace_address;\n> > +\t\t\t/* Map VM memory file */\n> > +\t\t\tif (host_memory_map(dev, mem, ctx.pid,\n> > +\t\t\t\tmem->base_address) != 0) {\n> > +\t\t\t\tfree(mem);\n> > +\t\t\t\treturn -1;\n> > +\t\t\t}\n> > +\t\t}\n> > +\t}\n> > +\n> > +\t/* Check that we have a valid base address. */\n> > +\tif (mem->base_address == 0) {\n> > +\t\tRTE_LOG(ERR, VHOST_CONFIG,\n> > +\t\t\"(%\"PRIu64\") Failed to find base address of qemu memory \"\n> > +\t\t\"file.\\n\", dev->device_fh);\n> One more indent?\n> \n> > +\t\tfree(mem);\n> > +\t\treturn -1;\n> > +\t}\n> > +\n> > +\t/* Check if all of our regions have valid mappings. Usually one does\n> > +\t* not exist in the QEMU memory file. */\n> Need a blank space?\n> \n> > +\tvalid_regions = mem->nregions;\n> > +\tfor (regionidx = 0; regionidx < mem->nregions; regionidx++) {\n> > +\t\tif ((mem->regions[regionidx].userspace_address <\n> > +\t\t\tmem->base_address) ||\n> > +\t\t\t(mem->regions[regionidx].userspace_address >\n> > +\t\t\t\t(mem->base_address + mem->mapped_size)))\n> > +\t\t\t\tvalid_regions--;\n> > +\t}\n> > +\n> > +\t/* If a region does not have a valid mapping we rebuild our memory\n> > +\t* struct to contain only valid entries. */\n> Need a blank space?\n> > +\tif (valid_regions != mem->nregions) {\n> > +\t\tLOG_DEBUG(VHOST_CONFIG,\n> > +\t\t\"(%\"PRIu64\") Not all memory regions exist in the QEMU mem\n> file.\"\n> > +\t\t\"Re-populating mem structure\\n\",\n> One more indent?\n> \n> > +\t\t\tdev->device_fh);\n> > +\n> > +\t\t/* Re-populate the memory structure with only valid regions.\n> > +\t\t* Invalid regions are over-written with memmove. */\n> Need a blank space?\n> \n> Thanks,\n> Tetsuya", "headers": { "Return-Path": "<huawei.xie@intel.com>", "Received": [ "from mga01.intel.com (mga01.intel.com [192.55.52.88])\n\tby dpdk.org (Postfix) with ESMTP id B7AEB58DF\n\tfor <dev@dpdk.org>; Wed, 3 Sep 2014 07:34:56 +0200 (CEST)", "from fmsmga001.fm.intel.com ([10.253.24.23])\n\tby fmsmga101.fm.intel.com with ESMTP; 02 Sep 2014 22:39:29 -0700", "from fmsmsx107.amr.corp.intel.com ([10.18.124.205])\n\tby fmsmga001.fm.intel.com with ESMTP; 02 Sep 2014 22:39:28 -0700", "from fmsmsx158.amr.corp.intel.com (10.18.116.75) by\n\tfmsmsx107.amr.corp.intel.com (10.18.124.205) with Microsoft SMTP\n\tServer (TLS) id 14.3.195.1; Tue, 2 Sep 2014 22:39:28 -0700", "from shsmsx103.ccr.corp.intel.com (10.239.4.69) by\n\tfmsmsx158.amr.corp.intel.com (10.18.116.75) with Microsoft SMTP\n\tServer (TLS) id 14.3.195.1; Tue, 2 Sep 2014 22:39:28 -0700", "from shsmsx101.ccr.corp.intel.com ([169.254.1.198]) by\n\tSHSMSX103.ccr.corp.intel.com ([169.254.4.219]) with mapi id\n\t14.03.0195.001; Wed, 3 Sep 2014 13:39:25 +0800" ], "X-ExtLoop1": "1", "X-IronPort-AV": "E=Sophos;i=\"5.04,455,1406617200\"; d=\"scan'208\";a=\"585462159\"", "From": "\"Xie, Huawei\" <huawei.xie@intel.com>", "To": "Tetsuya.Mukawa <mukawa@igel.co.jp>, \"dev@dpdk.org\" <dev@dpdk.org>", "Thread-Topic": "[dpdk-dev] [PATCH 2/3] lib/librte_vhost: vhost library support\n\tto facilitate integration with DPDK accelerated vswitch", "Thread-Index": "AQHPxyib+p4Q6rQgG0yUrSqO7EYjaJvu2twg", "Date": "Wed, 3 Sep 2014 05:39:24 +0000", "Message-ID": "<C37D651A908B024F974696C65296B57B0F2847A0@SHSMSX101.ccr.corp.intel.com>", "References": "<1409648131-4301-1-git-send-email-huawei.xie@intel.com>\n\t<1409648131-4301-3-git-send-email-huawei.xie@intel.com>\n\t<54068D47.4090401@igel.co.jp>", "In-Reply-To": "<54068D47.4090401@igel.co.jp>", "Accept-Language": "en-US", "Content-Language": "en-US", "X-MS-Has-Attach": "", "X-MS-TNEF-Correlator": "", "x-originating-ip": "[10.239.127.40]", "Content-Type": "text/plain; charset=\"us-ascii\"", "Content-Transfer-Encoding": "quoted-printable", "MIME-Version": "1.0", "Subject": "Re: [dpdk-dev] [PATCH 2/3] lib/librte_vhost: vhost library support\n\tto facilitate integration with DPDK accelerated vswitch", "X-BeenThere": "dev@dpdk.org", "X-Mailman-Version": "2.1.15", "Precedence": "list", "List-Id": "patches and discussions about DPDK <dev.dpdk.org>", "List-Unsubscribe": "<http://dpdk.org/ml/options/dev>,\n\t<mailto:dev-request@dpdk.org?subject=unsubscribe>", "List-Archive": "<http://dpdk.org/ml/archives/dev/>", "List-Post": "<mailto:dev@dpdk.org>", "List-Help": "<mailto:dev-request@dpdk.org?subject=help>", "List-Subscribe": "<http://dpdk.org/ml/listinfo/dev>,\n\t<mailto:dev-request@dpdk.org?subject=subscribe>", "X-List-Received-Date": "Wed, 03 Sep 2014 05:34:57 -0000" }, "addressed": null }, { "id": 637, "web_url": "https://patches.dpdk.org/comment/637/", "msgid": "<20140902230153.0f4f4375@urahara>", "list_archive_url": "https://inbox.dpdk.org/dev/20140902230153.0f4f4375@urahara", "date": "2014-09-03T06:01:53", "subject": "Re: [dpdk-dev] [PATCH 2/3] lib/librte_vhost: vhost library support\n\tto facilitate integration with DPDK accelerated vswitch", "submitter": { "id": 27, "url": "https://patches.dpdk.org/api/people/27/?format=api", "name": "Stephen Hemminger", "email": "stephen@networkplumber.org" }, "content": "On Wed, 3 Sep 2014 05:39:24 +0000\n\"Xie, Huawei\" <huawei.xie@intel.com> wrote:\n\n> Thanks Tetsuya:\n> \tSome of them are due to 80 character limitation. Is it ok to break the limitation for better indentation?\n\nMany of these comments could just be removed or shortened.\nOften removing comments makes code clearer and less buggy, because\nthere is no risk that comment does not match code.", "headers": { "Return-Path": "<stephen@networkplumber.org>", "Received": [ "from mail-pd0-f182.google.com (mail-pd0-f182.google.com\n\t[209.85.192.182]) by dpdk.org (Postfix) with ESMTP id E6ABF58DF\n\tfor <dev@dpdk.org>; Wed, 3 Sep 2014 07:57:29 +0200 (CEST)", "by mail-pd0-f182.google.com with SMTP id fp1so10358822pdb.27\n\tfor <dev@dpdk.org>; Tue, 02 Sep 2014 23:02:04 -0700 (PDT)", "from urahara (static-50-53-65-80.bvtn.or.frontiernet.net.\n\t[50.53.65.80]) by mx.google.com with ESMTPSA id\n\tz4sm7995783pda.23.2014.09.02.23.02.03 for <multiple recipients>\n\t(version=TLSv1.2 cipher=ECDHE-RSA-AES128-GCM-SHA256 bits=128/128);\n\tTue, 02 Sep 2014 23:02:03 -0700 (PDT)" ], "X-Google-DKIM-Signature": "v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20130820;\n\th=x-gm-message-state:date:from:to:cc:subject:message-id:in-reply-to\n\t:references:mime-version:content-type:content-transfer-encoding;\n\tbh=HcLDbpTyE5VpfO3+EiJ5vFPuQlw4+zMy5H2HLbI4eiw=;\n\tb=LetZxuyrvFDl5pOwzBjngRl0p88EDxn8ZuCdTLK/4Kp1Rycu2DSEug7xGMM/6dKHny\n\tv7oiQsZPmlWxVN1PhQVT7kiJreL32jm3KoeVAYJjEficC11682cWQl3pgXxwP7RisvpN\n\tcmFiiVKe3dny6Yt60HyV4xPVNoazroTdf6hfcgncZrtKys48RjCT5duwdEAtkGyFP9RB\n\t5JY5LcJDs9rw3C6rG9o1UEQS06BOF3j9VwInBd0YFWod3UWz5C1fYNe/4cgJJZ5NgIYN\n\tj/aJcp+DD26P+WckQNOn+u2PwZ9llRqfPGpqML1XaKxWtIjLnQ/FM0gUzMBQ+ziJN/3t\n\twCnw==", "X-Gm-Message-State": "ALoCoQnB1aryTjpuX93Gh2zTQJWQgH5C1Q7xSgxjLHmtZhqAtyi9kPQVPwrVLlIpoWEKs4Shj483", "X-Received": "by 10.66.168.204 with SMTP id zy12mr53320644pab.19.1409724124280;\n\tTue, 02 Sep 2014 23:02:04 -0700 (PDT)", "Date": "Tue, 2 Sep 2014 23:01:53 -0700", "From": "Stephen Hemminger <stephen@networkplumber.org>", "To": "\"Xie, Huawei\" <huawei.xie@intel.com>", "Message-ID": "<20140902230153.0f4f4375@urahara>", "In-Reply-To": "<C37D651A908B024F974696C65296B57B0F2847A0@SHSMSX101.ccr.corp.intel.com>", "References": "<1409648131-4301-1-git-send-email-huawei.xie@intel.com>\n\t<1409648131-4301-3-git-send-email-huawei.xie@intel.com>\n\t<54068D47.4090401@igel.co.jp>\n\t<C37D651A908B024F974696C65296B57B0F2847A0@SHSMSX101.ccr.corp.intel.com>", "MIME-Version": "1.0", "Content-Type": "text/plain; charset=US-ASCII", "Content-Transfer-Encoding": "7bit", "Cc": "\"dev@dpdk.org\" <dev@dpdk.org>", "Subject": "Re: [dpdk-dev] [PATCH 2/3] lib/librte_vhost: vhost library support\n\tto facilitate integration with DPDK accelerated vswitch", "X-BeenThere": "dev@dpdk.org", "X-Mailman-Version": "2.1.15", "Precedence": "list", "List-Id": "patches and discussions about DPDK <dev.dpdk.org>", "List-Unsubscribe": "<http://dpdk.org/ml/options/dev>,\n\t<mailto:dev-request@dpdk.org?subject=unsubscribe>", "List-Archive": "<http://dpdk.org/ml/archives/dev/>", "List-Post": "<mailto:dev@dpdk.org>", "List-Help": "<mailto:dev-request@dpdk.org?subject=help>", "List-Subscribe": "<http://dpdk.org/ml/listinfo/dev>,\n\t<mailto:dev-request@dpdk.org?subject=subscribe>", "X-List-Received-Date": "Wed, 03 Sep 2014 05:57:30 -0000" }, "addressed": null }, { "id": 640, "web_url": "https://patches.dpdk.org/comment/640/", "msgid": "<5406B87D.9070301@igel.co.jp>", "list_archive_url": "https://inbox.dpdk.org/dev/5406B87D.9070301@igel.co.jp", "date": "2014-09-03T06:43:09", "subject": "Re: [dpdk-dev] [PATCH 2/3] lib/librte_vhost: vhost library support\n\tto facilitate integration with DPDK accelerated vswitch", "submitter": { "id": 64, "url": "https://patches.dpdk.org/api/people/64/?format=api", "name": "Tetsuya Mukawa", "email": "mukawa@igel.co.jp" }, "content": "(2014/09/03 14:39), Xie, Huawei wrote:\n> Thanks Tetsuya:\n> \tSome of them are due to 80 character limitation. Is it ok to break the limitation for better indentation?\nIt's is up to the cording style of dpdk. But, there may be no strict\nrule in dpdk.\nSo please use your original cord. :)\n\nThanks,\nTetsuya\n\n>> -----Original Message-----\n>> From: Tetsuya.Mukawa [mailto:mukawa@igel.co.jp]\n>> Sent: Wednesday, September 03, 2014 11:39 AM\n>> To: Xie, Huawei; dev@dpdk.org\n>> Subject: Re: [dpdk-dev] [PATCH 2/3] lib/librte_vhost: vhost library support to\n>> facilitate integration with DPDK accelerated vswitch\n>>\n>> Hi Huawei,\n>>\n>> I added few comments. Mainly those comments are about source code indent.\n>>\n>> (2014/09/02 17:55), Huawei Xie wrote:\n>>> +\n>>> +/**\n>>> + * Structure contains variables relevant to RX/TX virtqueues.\n>>> + */\n>>> +struct vhost_virtqueue {\n>>> +\t/**< descriptor ring. */\n>>> +\tstruct vring_desc *desc;\n>>> +\t/**< available ring. */\n>>> +\tstruct vring_avail *avail;\n>>> +\t/**< used ring. */\n>>> +\tstruct vring_used *used;\n>>> +\t/**< Size of descriptor ring. */\n>>> +\tuint32_t size;\n>>> +\t/**< Backend value to determine if device should be started/stopped. */\n>>> +\tuint32_t backend;\n>>> +\t/**< Vhost header length (varies depending on RX merge buffers. */\n>>> +\tuint16_t vhost_hlen;\n>>> +\t/**< Last index used on the available ring. */\n>>> +\tvolatile uint16_t last_used_idx;\n>>> +\t/**< Used for multiple devices reserving buffers. */\n>>> +\tvolatile uint16_t last_used_idx_res;\n>>> +\t/**< Currently unused as polling mode is enabled. */\n>>> +\teventfd_t callfd;\n>>> +\t/**< Used to notify the guest (trigger interrupt). */\n>>> +\teventfd_t kickfd;\n>>> +} __rte_cache_aligned;\n>>> +\n>>> +/**\n>>> + * Information relating to memory regions including offsets to\n>>> + * addresses in QEMUs memory file.\n>>> + */\n>>> +struct virtio_memory_regions {\n>>> +\t/**< Base guest physical address of region. */\n>>> +\tuint64_t guest_phys_address;\n>>> +\t/**< End guest physical address of region. */\n>>> +\tuint64_t guest_phys_address_end;\n>>> +\t/**< Size of region. */\n>>> +\tuint64_t memory_size;\n>>> +\t/**< Base userspace address of region. */\n>>> +\tuint64_t userspace_address;\n>>> +\t/**< Offset of region for address translation. */\n>>> +\tuint64_t address_offset;\n>>> +};\n>>> +\n>>> +\n>>> +/**\n>>> + * Memory structure includes region and mapping information.\n>>> + */\n>>> +struct virtio_memory {\n>>> +\t/**< Base QEMU userspace address of the memory file. */\n>>> +\tuint64_t base_address;\n>>> +\t/**< Mapped address of memory file in this process's memory space. */\n>>> +\tuint64_t mapped_address;\n>>> +\t/**< Total size of memory file. */\n>>> +\tuint64_t mapped_size;\n>>> +\t/**< Number of memory regions. */\n>>> +\tuint32_t nregions;\n>>> +\t/**< Memory region information. */\n>>> +\tstruct virtio_memory_regions regions[0];\n>>> +};\n>>> +\n>>> +/**\n>>> + * Device structure contains all configuration information relating to the\n>> device.\n>>> + */\n>>> +struct virtio_net {\n>>> +\t/**< Contains all virtqueue information. */\n>>> +\tstruct vhost_virtqueue *virtqueue[VIRTIO_QNUM];\n>>> +\t/**< QEMU memory and memory region information. */\n>>> +\tstruct virtio_memory *mem;\n>>> +\t/**< Negotiated feature set. */\n>>> +\tuint64_t features;\n>>> +\t/**< Device identifier. */\n>>> +\tuint64_t device_fh;\n>>> +\t/**< Device flags, used to check if device is running on data core. */\n>>> +\tuint32_t flags;\n>>> +\tvoid *priv;\n>>> +} __rte_cache_aligned;\n>>> +\n>> Above comments of 'vhost_virtqueue', 'struct virtio_memory_regions',\n>> 'struct virtio-memory'\n>> and 'struct virtio-net' will break API documentation created by Doxygen.\n>> Not to break, I guess those comment need to be placed after variable.\n> Thanks. I will check doxgen rule. Moved the comment above to avoid 80 character limitation warning.\n>>> +/**\n>>> + * cuse_info is populated and used to register the cuse device.\n>>> + * vhost_net_device_ops is passed when the device is registered in main.c.\n>>> + */\n>>> +int\n>>> +rte_vhost_driver_register(const char *dev_name)\n>>> +{\n>>> +\tstruct cuse_info cuse_info;\n>>> +\tchar device_name[PATH_MAX] = \"\";\n>>> +\tchar char_device_name[PATH_MAX] = \"\";\n>>> +\tconst char *device_argv[] = { device_name };\n>>> +\n>>> +\tchar fuse_opt_dummy[] = FUSE_OPT_DUMMY;\n>>> +\tchar fuse_opt_fore[] = FUSE_OPT_FORE;\n>>> +\tchar fuse_opt_nomulti[] = FUSE_OPT_NOMULTI;\n>>> +\tchar *fuse_argv[] = {fuse_opt_dummy, fuse_opt_fore,\n>> fuse_opt_nomulti};\n>>> +\n>>> +\tif (access(cuse_device_name, R_OK | W_OK) < 0) {\n>>> +\t\tRTE_LOG(ERR, VHOST_CONFIG,\n>>> +\t\t\"Character device %s can't be accessed, maybe not exist\\n\",\n>>> +\t\tcuse_device_name);\n>> Need one more indent?\n> Again to avoid 80 char warning for some lengthy RTE_LOG.\n>>> +/**\n>>> + * Locate the file containing QEMU's memory space and map it to our\n>> address space.\n>>> + */\n>>> +static int\n>>> +host_memory_map(struct virtio_net *dev, struct virtio_memory *mem, pid_t\n>> pid,\n>>> +\t\tuint64_t addr)\n>>> +{\n>>> +\tstruct dirent *dptr = NULL;\n>>> +\tstruct procmap procmap;\n>>> +\tDIR *dp = NULL;\n>>> +\tint fd;\n>>> +\tint i;\n>>> +\tchar memfile[PATH_MAX];\n>>> +\tchar mapfile[PATH_MAX];\n>>> +\tchar procdir[PATH_MAX];\n>>> +\tchar resolved_path[PATH_MAX];\n>>> +\tFILE *fmap;\n>>> +\tvoid *map;\n>>> +\tuint8_t\tfound = 0;\n>>> +\tchar line[BUFSIZE];\n>>> +\tchar dlm[] = \"- : \";\n>>> +\tchar *str, *sp, *in[PROCMAP_SZ];\n>>> +\tchar *end = NULL;\n>>> +\n>>> +\t/* Path where mem files are located. */\n>>> +\tsnprintf(procdir, PATH_MAX, \"/proc/%u/fd/\", pid);\n>>> +\t/* Maps file used to locate mem file. */\n>>> +\tsnprintf(mapfile, PATH_MAX, \"/proc/%u/maps\", pid);\n>>> +\n>>> +\tfmap = fopen(mapfile, \"r\");\n>>> +\tif (fmap == NULL) {\n>>> +\t\tRTE_LOG(ERR, VHOST_CONFIG,\n>>> +\t\t\t\"(%\"PRIu64\") Failed to open maps file for pid %d\\n\",\n>>> +\t\tdev->device_fh, pid);\n>>> +\t\treturn -1;\n>>> +\t}\n>>> +\n>>> +\t/* Read through maps file until we find out base_address. */\n>>> +\twhile (fgets(line, BUFSIZE, fmap) != 0) {\n>>> +\t\tstr = line;\n>>> +\t\terrno = 0;\n>>> +\t\t/* Split line in to fields. */\n>>> +\t\tfor (i = 0; i < PROCMAP_SZ; i++) {\n>>> +\t\t\tin[i] = strtok_r(str, &dlm[i], &sp);\n>>> +\t\t\tif ((in[i] == NULL) || (errno != 0)) {\n>>> +\t\t\t\tfclose(fmap);\n>>> +\t\t\t\treturn -1;\n>>> +\t\t\t}\n>>> +\t\t\tstr = NULL;\n>>> +\t\t}\n>>> +\n>>> +\t\t/* Convert/Copy each field as needed. */\n>>> +\t\tprocmap.va_start = strtoull(in[0], &end, 16);\n>>> +\t\tif ((in[0] == '\\0') || (end == NULL) || (*end != '\\0') ||\n>>> +\t\t\t(errno != 0)) {\n>>> +\t\t\tfclose(fmap);\n>>> +\t\t\treturn -1;\n>>> +\t\t}\n>>> +\n>>> +\t\tprocmap.len = strtoull(in[1], &end, 16);\n>>> +\t\tif ((in[1] == '\\0') || (end == NULL) || (*end != '\\0') ||\n>>> +\t\t\t(errno != 0)) {\n>>> +\t\t\tfclose(fmap);\n>>> +\t\t\treturn -1;\n>>> +\t\t}\n>>> +\n>>> +\t\tprocmap.pgoff = strtoull(in[3], &end, 16);\n>>> +\t\tif ((in[3] == '\\0') || (end == NULL) || (*end != '\\0') ||\n>>> +\t\t\t(errno != 0)) {\n>>> +\t\t\tfclose(fmap);\n>>> +\t\t\treturn -1;\n>>> +\t\t}\n>>> +\n>>> +\t\tprocmap.maj = strtoul(in[4], &end, 16);\n>>> +\t\tif ((in[4] == '\\0') || (end == NULL) || (*end != '\\0') ||\n>>> +\t\t\t(errno != 0)) {\n>>> +\t\t\tfclose(fmap);\n>>> +\t\t\treturn -1;\n>>> +\t\t}\n>>> +\n>>> +\t\tprocmap.min = strtoul(in[5], &end, 16);\n>>> +\t\tif ((in[5] == '\\0') || (end == NULL) || (*end != '\\0') ||\n>>> +\t\t\t(errno != 0)) {\n>>> +\t\t\tfclose(fmap);\n>>> +\t\t\treturn -1;\n>>> +\t\t}\n>>> +\n>>> +\t\tprocmap.ino = strtoul(in[6], &end, 16);\n>>> +\t\tif ((in[6] == '\\0') || (end == NULL) || (*end != '\\0') ||\n>>> +\t\t\t(errno != 0)) {\n>>> +\t\t\tfclose(fmap);\n>>> +\t\t\treturn -1;\n>>> +\t\t}\n>>> +\n>>> +\t\tmemcpy(&procmap.prot, in[2], PROT_SZ);\n>>> +\t\tmemcpy(&procmap.fname, in[7], PATH_MAX);\n>>> +\n>>> +\t\tif (procmap.va_start == addr) {\n>>> +\t\t\tprocmap.len = procmap.len - procmap.va_start;\n>>> +\t\t\tfound = 1;\n>>> +\t\t\tbreak;\n>>> +\t\t}\n>>> +\t}\n>>> +\tfclose(fmap);\n>>> +\n>>> +\tif (!found) {\n>>> +\t\tRTE_LOG(ERR, VHOST_CONFIG,\n>>> +\t\t\"(%\"PRIu64\") Failed to find memory file in pid %d maps file\\n\",\n>>> +\t\tdev->device_fh, pid);\n>> Need one more indent?\n>>\n>>> +/**\n>>> + * Searches the configuration core linked list and retrieves the device if it\n>> exists.\n>>> + */\n>>> +static struct virtio_net *\n>>> +get_device(struct vhost_device_ctx ctx)\n>>> +{\n>>> +\tstruct virtio_net_config_ll *ll_dev;\n>>> +\n>>> +\tll_dev = get_config_ll_entry(ctx);\n>>> +\n>>> +\t/* If a matching entry is found in the linked list, return the device\n>>> +\t* in that entry. */\n>> Need a blank space at start of comment?\n>>\n>>> +\tif (ll_dev)\n>>> +\t\treturn &ll_dev->dev;\n>>> +\n>>> +\tRTE_LOG(ERR, VHOST_CONFIG,\n>>> +\t\t\"(%\"PRIu64\") Device not found in linked list.\\n\", ctx.fh);\n>>> +\treturn NULL;\n>>> +}\n>>> +\n>>> +/**\n>>> + * Add entry containing a device to the device configuration linked list.\n>>> + */\n>>> +static void\n>>> +add_config_ll_entry(struct virtio_net_config_ll *new_ll_dev)\n>>> +{\n>>> +\tstruct virtio_net_config_ll *ll_dev = ll_root;\n>>> +\n>>> +\t/* If ll_dev == NULL then this is the first device so go to else */\n>>> +\tif (ll_dev) {\n>>> +\t\t/* If the 1st device_fh != 0 then we insert our device here. */\n>>> +\t\tif (ll_dev->dev.device_fh != 0)\t{\n>>> +\t\t\tnew_ll_dev->dev.device_fh = 0;\n>>> +\t\t\tnew_ll_dev->next = ll_dev;\n>>> +\t\t\tll_root = new_ll_dev;\n>>> +\t\t} else {\n>>> +\t\t\t/* Increment through the ll until we find un unused\n>>> +\t\t\t* device_fh. Insert the device at that entry*/\n>> Need a blank space at end of comment?\n> Will fix.\n>>> +/**\n>>> + * Function is called from the CUSE release function. This function will\n>> cleanup\n>>> + * the device and remove it from device configuration linked list.\n>>> + */\n>>> +static void\n>>> +destroy_device(struct vhost_device_ctx ctx)\n>>> +{\n>>> +\tstruct virtio_net_config_ll *ll_dev_cur_ctx, *ll_dev_last = NULL;\n>>> +\tstruct virtio_net_config_ll *ll_dev_cur = ll_root;\n>>> +\n>>> +\t/* Find the linked list entry for the device to be removed. */\n>>> +\tll_dev_cur_ctx = get_config_ll_entry(ctx);\n>>> +\twhile (ll_dev_cur != NULL) {\n>>> +\t\t/* If the device is found or a device that doesn't exist is\n>>> +\t\t* found then it is removed. */\n>> Need a blank space?\n> Will fix.\n>>> +/**\n>>> + * Called from CUSE IOCTL: VHOST_SET_FEATURES\n>>> + * We receive the negotiated set of features supported by us and the virtio\n>>> + * device.\n>>> + */\n>>> +static int\n>>> +set_features(struct vhost_device_ctx ctx, uint64_t *pu)\n>>> +{\n>>> +\tstruct virtio_net *dev;\n>>> +\n>>> +\tdev = get_device(ctx);\n>>> +\tif (dev == NULL)\n>>> +\t\treturn -1;\n>>> +\tif (*pu & ~VHOST_FEATURES)\n>>> +\t\treturn -1;\n>>> +\n>>> +\t/* Store the negotiated feature list for the device. */\n>>> +\tdev->features = *pu;\n>>> +\n>>> +\t/* Set the vhost_hlen depending on if VIRTIO_NET_F_MRG_RXBUF is set.\n>> */\n>>> +\tif (dev->features & (1 << VIRTIO_NET_F_MRG_RXBUF)) {\n>>> +\t\tLOG_DEBUG(VHOST_CONFIG,\n>>> +\t\t\"(%\"PRIu64\") Mergeable RX buffers enabled\\n\", dev->device_fh);\n>>> +\t\tdev->virtqueue[VIRTIO_RXQ]->vhost_hlen =\n>>> +\t\t\tsizeof(struct virtio_net_hdr_mrg_rxbuf);\n>>> +\t\tdev->virtqueue[VIRTIO_TXQ]->vhost_hlen =\n>>> +\t\t\tsizeof(struct virtio_net_hdr_mrg_rxbuf);\n>> One more indent?\n>>\n>>> +/**\n>>> + * Called from CUSE IOCTL: VHOST_SET_MEM_TABLE\n>>> + * This function creates and populates the memory structure for the device.\n>>> + * This includes storing offsets used to translate buffer addresses.\n>>> + */\n>>> +static int\n>>> +set_mem_table(struct vhost_device_ctx ctx, const void *mem_regions_addr,\n>>> +\tuint32_t nregions)\n>>> +{\n>>> +\tstruct virtio_net *dev;\n>>> +\tstruct vhost_memory_region *mem_regions;\n>>> +\tstruct virtio_memory *mem;\n>>> +\tuint64_t size = offsetof(struct vhost_memory, regions);\n>>> +\tuint32_t regionidx, valid_regions;\n>>> +\n>>> +\tdev = get_device(ctx);\n>>> +\tif (dev == NULL)\n>>> +\t\treturn -1;\n>>> +\n>>> +\tif (dev->mem) {\n>>> +\t\tmunmap((void *)(uintptr_t)dev->mem->mapped_address,\n>>> +\t\t\t(size_t)dev->mem->mapped_size);\n>>> +\t\tfree(dev->mem);\n>>> +\t}\n>>> +\n>>> +\t/* Malloc the memory structure depending on the number of regions. */\n>>> +\tmem = calloc(1, sizeof(struct virtio_memory) +\n>>> +\t\t(sizeof(struct virtio_memory_regions) * nregions));\n>>> +\tif (mem == NULL) {\n>>> +\t\tRTE_LOG(ERR, VHOST_CONFIG,\n>>> +\t\t\t\"(%\"PRIu64\") Failed to allocate memory for dev-\n>>> mem.\\n\",\n>>> +\t\t\tdev->device_fh);\n>>> +\t\treturn -1;\n>>> +\t}\n>>> +\n>>> +\tmem->nregions = nregions;\n>>> +\n>>> +\tmem_regions = (void *)(uintptr_t)\n>>> +\t\t((uint64_t)(uintptr_t)mem_regions_addr + size);\n>>> +\n>>> +\tfor (regionidx = 0; regionidx < mem->nregions; regionidx++) {\n>>> +\t\t/* Populate the region structure for each region. */\n>>> +\t\tmem->regions[regionidx].guest_phys_address =\n>>> +\t\t\tmem_regions[regionidx].guest_phys_addr;\n>>> +\t\tmem->regions[regionidx].guest_phys_address_end =\n>>> +\t\t\tmem->regions[regionidx].guest_phys_address +\n>>> +\t\t\tmem_regions[regionidx].memory_size;\n>>> +\t\tmem->regions[regionidx].memory_size =\n>>> +\t\t\tmem_regions[regionidx].memory_size;\n>>> +\t\tmem->regions[regionidx].userspace_address =\n>>> +\t\t\tmem_regions[regionidx].userspace_addr;\n>>> +\n>>> +\t\tLOG_DEBUG(VHOST_CONFIG,\n>>> +\t\t\t\"(%\"PRIu64\") REGION: %u - GPA: %p - QEMU VA: %p -\n>> SIZE \"\n>>> +\t\t\t\"(%\"PRIu64\")\\n\",\n>>> +\t\t\tdev->device_fh, regionidx,\n>>> +\t\t\t(void *)(uintptr_t)\n>>> +\t\t\t\tmem->regions[regionidx].guest_phys_address,\n>>> +\t\t\t(void *)(uintptr_t)\n>>> +\t\t\t\tmem->regions[regionidx].userspace_address,\n>>> +\t\t\tmem->regions[regionidx].memory_size);\n>>> +\n>>> +\t\t/*set the base address mapping*/\n>> Need a blank space at start and end of comment?\n> Thanks for careful review.\n>>> +\t\tif (mem->regions[regionidx].guest_phys_address == 0x0) {\n>>> +\t\t\tmem->base_address =\n>>> +\t\t\t\tmem->regions[regionidx].userspace_address;\n>>> +\t\t\t/* Map VM memory file */\n>>> +\t\t\tif (host_memory_map(dev, mem, ctx.pid,\n>>> +\t\t\t\tmem->base_address) != 0) {\n>>> +\t\t\t\tfree(mem);\n>>> +\t\t\t\treturn -1;\n>>> +\t\t\t}\n>>> +\t\t}\n>>> +\t}\n>>> +\n>>> +\t/* Check that we have a valid base address. */\n>>> +\tif (mem->base_address == 0) {\n>>> +\t\tRTE_LOG(ERR, VHOST_CONFIG,\n>>> +\t\t\"(%\"PRIu64\") Failed to find base address of qemu memory \"\n>>> +\t\t\"file.\\n\", dev->device_fh);\n>> One more indent?\n>>\n>>> +\t\tfree(mem);\n>>> +\t\treturn -1;\n>>> +\t}\n>>> +\n>>> +\t/* Check if all of our regions have valid mappings. Usually one does\n>>> +\t* not exist in the QEMU memory file. */\n>> Need a blank space?\n>>\n>>> +\tvalid_regions = mem->nregions;\n>>> +\tfor (regionidx = 0; regionidx < mem->nregions; regionidx++) {\n>>> +\t\tif ((mem->regions[regionidx].userspace_address <\n>>> +\t\t\tmem->base_address) ||\n>>> +\t\t\t(mem->regions[regionidx].userspace_address >\n>>> +\t\t\t\t(mem->base_address + mem->mapped_size)))\n>>> +\t\t\t\tvalid_regions--;\n>>> +\t}\n>>> +\n>>> +\t/* If a region does not have a valid mapping we rebuild our memory\n>>> +\t* struct to contain only valid entries. */\n>> Need a blank space?\n>>> +\tif (valid_regions != mem->nregions) {\n>>> +\t\tLOG_DEBUG(VHOST_CONFIG,\n>>> +\t\t\"(%\"PRIu64\") Not all memory regions exist in the QEMU mem\n>> file.\"\n>>> +\t\t\"Re-populating mem structure\\n\",\n>> One more indent?\n>>\n>>> +\t\t\tdev->device_fh);\n>>> +\n>>> +\t\t/* Re-populate the memory structure with only valid regions.\n>>> +\t\t* Invalid regions are over-written with memmove. */\n>> Need a blank space?\n>>\n>> Thanks,\n>> Tetsuya", "headers": { "Return-Path": "<mukawa@igel.co.jp>", "Received": [ "from mail-pa0-f46.google.com (mail-pa0-f46.google.com\n\t[209.85.220.46]) by dpdk.org (Postfix) with ESMTP id 7886E58DF\n\tfor <dev@dpdk.org>; Wed, 3 Sep 2014 08:38:43 +0200 (CEST)", "by mail-pa0-f46.google.com with SMTP id eu11so16506329pac.5\n\tfor <dev@dpdk.org>; Tue, 02 Sep 2014 23:43:14 -0700 (PDT)", "from [10.16.129.101] (napt.igel.co.jp. [219.106.231.132])\n\tby mx.google.com with ESMTPSA id\n\th12sm8088043pdk.48.2014.09.02.23.43.12 for <multiple recipients>\n\t(version=TLSv1 cipher=ECDHE-RSA-RC4-SHA bits=128/128);\n\tTue, 02 Sep 2014 23:43:12 -0700 (PDT)" ], "X-Google-DKIM-Signature": "v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20130820;\n\th=x-gm-message-state:message-id:date:from:user-agent:mime-version:to\n\t:subject:references:in-reply-to:content-type\n\t:content-transfer-encoding;\n\tbh=GN+WVv6OyMgVpKy4SKIwV5dgc6zj9+LktMa2su7tmxQ=;\n\tb=D8udRMbGf0XTqskRPjHTdRLPNUl5eeCf95Ix18SOs/nXnjXlMXK6REeJ6xS0e/d8bq\n\tQSfRHTGw5+Oy+eywkAG/008J7J5svv9qSSSozluvrDOpNIID9zEjtLUbYqIeFJPmxrKl\n\tf1yzXes7e0yIoO21z6xr/V4HBzaz2VCV/q/CDHYJPOwqWI9ueu6RfXRXb42wwCs1q2FZ\n\tvEbfyUY+i/fu2Mvacjq2W15SyGzJtTcaKJ7SfEvGyOvkdNXD42sai3fF7zbUWodfdxGl\n\tgQVRXoTfG+UeRhEhajdTnAomczq9rK+QiW8dxxvEImpgrueooox+a1LaXjtAZv7Ul1fW\n\tmz/Q==", "X-Gm-Message-State": "ALoCoQkDsTOKiettYOL49cmZaDPq1HXciSiAcDgHUOBgYeDda5SSdw+O4G2ho/XuiwYzjBGP5unD", "X-Received": "by 10.70.130.35 with SMTP id ob3mr11623028pdb.149.1409726593480; \n\tTue, 02 Sep 2014 23:43:13 -0700 (PDT)", "Message-ID": "<5406B87D.9070301@igel.co.jp>", "Date": "Wed, 03 Sep 2014 15:43:09 +0900", "From": "\"Tetsuya.Mukawa\" <mukawa@igel.co.jp>", "User-Agent": "Mozilla/5.0 (Windows NT 6.1; WOW64;\n\trv:24.0) Gecko/20100101 Thunderbird/24.6.0", "MIME-Version": "1.0", "To": "\"Xie, Huawei\" <huawei.xie@intel.com>, \"dev@dpdk.org\" <dev@dpdk.org>", "References": "<1409648131-4301-1-git-send-email-huawei.xie@intel.com>\n\t<1409648131-4301-3-git-send-email-huawei.xie@intel.com>\n\t<54068D47.4090401@igel.co.jp>\n\t<C37D651A908B024F974696C65296B57B0F2847A0@SHSMSX101.ccr.corp.intel.com>", "In-Reply-To": "<C37D651A908B024F974696C65296B57B0F2847A0@SHSMSX101.ccr.corp.intel.com>", "Content-Type": "text/plain; charset=ISO-8859-1", "Content-Transfer-Encoding": "7bit", "Subject": "Re: [dpdk-dev] [PATCH 2/3] lib/librte_vhost: vhost library support\n\tto facilitate integration with DPDK accelerated vswitch", "X-BeenThere": "dev@dpdk.org", "X-Mailman-Version": "2.1.15", "Precedence": "list", "List-Id": "patches and discussions about DPDK <dev.dpdk.org>", "List-Unsubscribe": "<http://dpdk.org/ml/options/dev>,\n\t<mailto:dev-request@dpdk.org?subject=unsubscribe>", "List-Archive": "<http://dpdk.org/ml/archives/dev/>", "List-Post": "<mailto:dev@dpdk.org>", "List-Help": "<mailto:dev-request@dpdk.org?subject=help>", "List-Subscribe": "<http://dpdk.org/ml/listinfo/dev>,\n\t<mailto:dev-request@dpdk.org?subject=subscribe>", "X-List-Received-Date": "Wed, 03 Sep 2014 06:38:44 -0000" }, "addressed": null }, { "id": 641, "web_url": "https://patches.dpdk.org/comment/641/", "msgid": "<2601191342CEEE43887BDE71AB97725821360229@IRSMSX105.ger.corp.intel.com>", "list_archive_url": "https://inbox.dpdk.org/dev/2601191342CEEE43887BDE71AB97725821360229@IRSMSX105.ger.corp.intel.com", "date": "2014-09-03T09:43:43", "subject": "Re: [dpdk-dev] [PATCH 2/3] lib/librte_vhost: vhost library support\n\tto facilitate integration with DPDK accelerated vswitch", "submitter": { "id": 33, "url": "https://patches.dpdk.org/api/people/33/?format=api", "name": "Ananyev, Konstantin", "email": "konstantin.ananyev@intel.com" }, "content": "> From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Stephen Hemminger\n> Sent: Wednesday, September 03, 2014 7:02 AM\n> To: Xie, Huawei\n> Cc: dev@dpdk.org\n> Subject: Re: [dpdk-dev] [PATCH 2/3] lib/librte_vhost: vhost library support to facilitate integration with DPDK accelerated vswitch\n> \n> On Wed, 3 Sep 2014 05:39:24 +0000\n> \"Xie, Huawei\" <huawei.xie@intel.com> wrote:\n> \n> > Thanks Tetsuya:\n> > \tSome of them are due to 80 character limitation. Is it ok to break the limitation for better indentation?\n> \n> Many of these comments could just be removed or shortened.\n> Often removing comments makes code clearer and less buggy, because\n> there is no risk that comment does not match code.\n\nIf comment string is longer than 80 characters, why just not split comment string into multiline?", "headers": { "Return-Path": "<konstantin.ananyev@intel.com>", "Received": [ "from mga01.intel.com (mga01.intel.com [192.55.52.88])\n\tby dpdk.org (Postfix) with ESMTP id 127962E8B\n\tfor <dev@dpdk.org>; Wed, 3 Sep 2014 11:39:23 +0200 (CEST)", "from fmsmga001.fm.intel.com ([10.253.24.23])\n\tby fmsmga101.fm.intel.com with ESMTP; 03 Sep 2014 02:43:58 -0700", "from irsmsx104.ger.corp.intel.com ([163.33.3.159])\n\tby fmsmga001.fm.intel.com with ESMTP; 03 Sep 2014 02:43:57 -0700", "from irsmsx152.ger.corp.intel.com (163.33.192.66) by\n\tIRSMSX104.ger.corp.intel.com (163.33.3.159) with Microsoft SMTP\n\tServer (TLS) id 14.3.195.1; Wed, 3 Sep 2014 10:43:44 +0100", "from irsmsx105.ger.corp.intel.com ([169.254.7.158]) by\n\tIRSMSX152.ger.corp.intel.com ([169.254.6.48]) with mapi id\n\t14.03.0195.001; Wed, 3 Sep 2014 10:43:44 +0100" ], "X-ExtLoop1": "1", "X-IronPort-AV": "E=Sophos;i=\"5.04,456,1406617200\"; d=\"scan'208\";a=\"585543211\"", "From": "\"Ananyev, Konstantin\" <konstantin.ananyev@intel.com>", "To": "Stephen Hemminger <stephen@networkplumber.org>, \"Xie, Huawei\"\n\t<huawei.xie@intel.com>", "Thread-Topic": "[dpdk-dev] [PATCH 2/3] lib/librte_vhost: vhost library support\n\tto facilitate integration with DPDK accelerated vswitch", "Thread-Index": "AQHPxyikCIAElc+oAk2v4Mb7fkbgiZvu08EAgAAGSICAAE3/YA==", "Date": "Wed, 3 Sep 2014 09:43:43 +0000", "Message-ID": "<2601191342CEEE43887BDE71AB97725821360229@IRSMSX105.ger.corp.intel.com>", "References": "<1409648131-4301-1-git-send-email-huawei.xie@intel.com>\n\t<1409648131-4301-3-git-send-email-huawei.xie@intel.com>\n\t<54068D47.4090401@igel.co.jp>\n\t<C37D651A908B024F974696C65296B57B0F2847A0@SHSMSX101.ccr.corp.intel.com>\n\t<20140902230153.0f4f4375@urahara>", "In-Reply-To": "<20140902230153.0f4f4375@urahara>", "Accept-Language": "en-IE, en-US", "Content-Language": "en-US", "X-MS-Has-Attach": "", "X-MS-TNEF-Correlator": "", "x-originating-ip": "[163.33.239.181]", "Content-Type": "text/plain; charset=\"us-ascii\"", "Content-Transfer-Encoding": "quoted-printable", "MIME-Version": "1.0", "Cc": "\"dev@dpdk.org\" <dev@dpdk.org>", "Subject": "Re: [dpdk-dev] [PATCH 2/3] lib/librte_vhost: vhost library support\n\tto facilitate integration with DPDK accelerated vswitch", "X-BeenThere": "dev@dpdk.org", "X-Mailman-Version": "2.1.15", "Precedence": "list", "List-Id": "patches and discussions about DPDK <dev.dpdk.org>", "List-Unsubscribe": "<http://dpdk.org/ml/options/dev>,\n\t<mailto:dev-request@dpdk.org?subject=unsubscribe>", "List-Archive": "<http://dpdk.org/ml/archives/dev/>", "List-Post": "<mailto:dev@dpdk.org>", "List-Help": "<mailto:dev-request@dpdk.org?subject=help>", "List-Subscribe": "<http://dpdk.org/ml/listinfo/dev>,\n\t<mailto:dev-request@dpdk.org?subject=subscribe>", "X-List-Received-Date": "Wed, 03 Sep 2014 09:39:24 -0000" }, "addressed": null } ]