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 B7AEB58DF for ; Wed, 3 Sep 2014 07:34:56 +0200 (CEST) Received: from fmsmga001.fm.intel.com ([10.253.24.23]) by fmsmga101.fm.intel.com with ESMTP; 02 Sep 2014 22:39:29 -0700 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.04,455,1406617200"; d="scan'208";a="585462159" Received: from fmsmsx107.amr.corp.intel.com ([10.18.124.205]) by fmsmga001.fm.intel.com with ESMTP; 02 Sep 2014 22:39:28 -0700 Received: from fmsmsx158.amr.corp.intel.com (10.18.116.75) by fmsmsx107.amr.corp.intel.com (10.18.124.205) with Microsoft SMTP Server (TLS) id 14.3.195.1; Tue, 2 Sep 2014 22:39:28 -0700 Received: from shsmsx103.ccr.corp.intel.com (10.239.4.69) by fmsmsx158.amr.corp.intel.com (10.18.116.75) with Microsoft SMTP Server (TLS) id 14.3.195.1; Tue, 2 Sep 2014 22:39:28 -0700 Received: from shsmsx101.ccr.corp.intel.com ([169.254.1.198]) by SHSMSX103.ccr.corp.intel.com ([169.254.4.219]) with mapi id 14.03.0195.001; Wed, 3 Sep 2014 13:39:25 +0800 From: "Xie, Huawei" To: Tetsuya.Mukawa , "dev@dpdk.org" Thread-Topic: [dpdk-dev] [PATCH 2/3] lib/librte_vhost: vhost library support to facilitate integration with DPDK accelerated vswitch Thread-Index: AQHPxyib+p4Q6rQgG0yUrSqO7EYjaJvu2twg Date: Wed, 3 Sep 2014 05:39:24 +0000 Message-ID: References: <1409648131-4301-1-git-send-email-huawei.xie@intel.com> <1409648131-4301-3-git-send-email-huawei.xie@intel.com> <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 to 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 List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Wed, 03 Sep 2014 05:34:57 -0000 Thanks Tetsuya: Some of them are due to 80 character limitation. Is it ok to break the lim= itation for better indentation? > -----Original Message----- > From: Tetsuya.Mukawa [mailto:mukawa@igel.co.jp] > Sent: Wednesday, September 03, 2014 11:39 AM > To: Xie, Huawei; dev@dpdk.org > Subject: Re: [dpdk-dev] [PATCH 2/3] lib/librte_vhost: vhost library suppo= rt to > facilitate integration with DPDK accelerated vswitch >=20 > Hi Huawei, >=20 > I added few comments. Mainly those comments are about source code indent. >=20 > (2014/09/02 17:55), Huawei Xie wrote: > > + > > +/** > > + * Structure contains variables relevant to RX/TX virtqueues. > > + */ > > +struct vhost_virtqueue { > > + /**< descriptor ring. */ > > + struct vring_desc *desc; > > + /**< available ring. */ > > + struct vring_avail *avail; > > + /**< used ring. */ > > + struct vring_used *used; > > + /**< Size of descriptor ring. */ > > + uint32_t size; > > + /**< Backend value to determine if device should be started/stopped. = */ > > + uint32_t backend; > > + /**< Vhost header length (varies depending on RX merge buffers. */ > > + uint16_t vhost_hlen; > > + /**< Last index used on the available ring. */ > > + volatile uint16_t last_used_idx; > > + /**< Used for multiple devices reserving buffers. */ > > + volatile uint16_t last_used_idx_res; > > + /**< Currently unused as polling mode is enabled. */ > > + eventfd_t callfd; > > + /**< Used to notify the guest (trigger interrupt). */ > > + eventfd_t kickfd; > > +} __rte_cache_aligned; > > + > > +/** > > + * Information relating to memory regions including offsets to > > + * addresses in QEMUs memory file. > > + */ > > +struct virtio_memory_regions { > > + /**< Base guest physical address of region. */ > > + uint64_t guest_phys_address; > > + /**< End guest physical address of region. */ > > + uint64_t guest_phys_address_end; > > + /**< Size of region. */ > > + uint64_t memory_size; > > + /**< Base userspace address of region. */ > > + uint64_t userspace_address; > > + /**< Offset of region for address translation. */ > > + uint64_t address_offset; > > +}; > > + > > + > > +/** > > + * Memory structure includes region and mapping information. > > + */ > > +struct virtio_memory { > > + /**< Base QEMU userspace address of the memory file. */ > > + uint64_t base_address; > > + /**< Mapped address of memory file in this process's memory space. */ > > + uint64_t mapped_address; > > + /**< Total size of memory file. */ > > + uint64_t mapped_size; > > + /**< Number of memory regions. */ > > + uint32_t nregions; > > + /**< Memory region information. */ > > + struct virtio_memory_regions regions[0]; > > +}; > > + > > +/** > > + * Device structure contains all configuration information relating to= the > device. > > + */ > > +struct virtio_net { > > + /**< Contains all virtqueue information. */ > > + struct vhost_virtqueue *virtqueue[VIRTIO_QNUM]; > > + /**< QEMU memory and memory region information. */ > > + struct virtio_memory *mem; > > + /**< Negotiated feature set. */ > > + uint64_t features; > > + /**< Device identifier. */ > > + uint64_t device_fh; > > + /**< Device flags, used to check if device is running on data core. *= / > > + uint32_t flags; > > + void *priv; > > +} __rte_cache_aligned; > > + > Above comments of 'vhost_virtqueue', 'struct virtio_memory_regions', > 'struct virtio-memory' > and 'struct virtio-net' will break API documentation created by Doxygen. > Not to break, I guess those comment need to be placed after variable. Thanks. I will check doxgen rule. Moved the comment above to avoid 80 chara= cter limitation warning. >=20 > > +/** > > + * cuse_info is populated and used to register the cuse device. > > + * vhost_net_device_ops is passed when the device is registered in mai= n.c. > > + */ > > +int > > +rte_vhost_driver_register(const char *dev_name) > > +{ > > + struct cuse_info cuse_info; > > + char device_name[PATH_MAX] =3D ""; > > + char char_device_name[PATH_MAX] =3D ""; > > + const char *device_argv[] =3D { device_name }; > > + > > + char fuse_opt_dummy[] =3D FUSE_OPT_DUMMY; > > + char fuse_opt_fore[] =3D FUSE_OPT_FORE; > > + char fuse_opt_nomulti[] =3D FUSE_OPT_NOMULTI; > > + char *fuse_argv[] =3D {fuse_opt_dummy, fuse_opt_fore, > fuse_opt_nomulti}; > > + > > + if (access(cuse_device_name, R_OK | W_OK) < 0) { > > + RTE_LOG(ERR, VHOST_CONFIG, > > + "Character device %s can't be accessed, maybe not exist\n", > > + cuse_device_name); > Need one more indent? Again to avoid 80 char warning for some lengthy RTE_LOG. >=20 > > +/** > > + * Locate the file containing QEMU's memory space and map it to our > address space. > > + */ > > +static int > > +host_memory_map(struct virtio_net *dev, struct virtio_memory *mem, pid= _t > pid, > > + uint64_t addr) > > +{ > > + struct dirent *dptr =3D NULL; > > + struct procmap procmap; > > + DIR *dp =3D NULL; > > + int fd; > > + int i; > > + char memfile[PATH_MAX]; > > + char mapfile[PATH_MAX]; > > + char procdir[PATH_MAX]; > > + char resolved_path[PATH_MAX]; > > + FILE *fmap; > > + void *map; > > + uint8_t found =3D 0; > > + char line[BUFSIZE]; > > + char dlm[] =3D "- : "; > > + char *str, *sp, *in[PROCMAP_SZ]; > > + char *end =3D NULL; > > + > > + /* Path where mem files are located. */ > > + snprintf(procdir, PATH_MAX, "/proc/%u/fd/", pid); > > + /* Maps file used to locate mem file. */ > > + snprintf(mapfile, PATH_MAX, "/proc/%u/maps", pid); > > + > > + fmap =3D fopen(mapfile, "r"); > > + if (fmap =3D=3D NULL) { > > + RTE_LOG(ERR, VHOST_CONFIG, > > + "(%"PRIu64") Failed to open maps file for pid %d\n", > > + dev->device_fh, pid); > > + return -1; > > + } > > + > > + /* Read through maps file until we find out base_address. */ > > + while (fgets(line, BUFSIZE, fmap) !=3D 0) { > > + str =3D line; > > + errno =3D 0; > > + /* Split line in to fields. */ > > + for (i =3D 0; i < PROCMAP_SZ; i++) { > > + in[i] =3D strtok_r(str, &dlm[i], &sp); > > + if ((in[i] =3D=3D NULL) || (errno !=3D 0)) { > > + fclose(fmap); > > + return -1; > > + } > > + str =3D NULL; > > + } > > + > > + /* Convert/Copy each field as needed. */ > > + procmap.va_start =3D strtoull(in[0], &end, 16); > > + if ((in[0] =3D=3D '\0') || (end =3D=3D NULL) || (*end !=3D '\0') || > > + (errno !=3D 0)) { > > + fclose(fmap); > > + return -1; > > + } > > + > > + procmap.len =3D strtoull(in[1], &end, 16); > > + if ((in[1] =3D=3D '\0') || (end =3D=3D NULL) || (*end !=3D '\0') || > > + (errno !=3D 0)) { > > + fclose(fmap); > > + return -1; > > + } > > + > > + procmap.pgoff =3D strtoull(in[3], &end, 16); > > + if ((in[3] =3D=3D '\0') || (end =3D=3D NULL) || (*end !=3D '\0') || > > + (errno !=3D 0)) { > > + fclose(fmap); > > + return -1; > > + } > > + > > + procmap.maj =3D strtoul(in[4], &end, 16); > > + if ((in[4] =3D=3D '\0') || (end =3D=3D NULL) || (*end !=3D '\0') || > > + (errno !=3D 0)) { > > + fclose(fmap); > > + return -1; > > + } > > + > > + procmap.min =3D strtoul(in[5], &end, 16); > > + if ((in[5] =3D=3D '\0') || (end =3D=3D NULL) || (*end !=3D '\0') || > > + (errno !=3D 0)) { > > + fclose(fmap); > > + return -1; > > + } > > + > > + procmap.ino =3D strtoul(in[6], &end, 16); > > + if ((in[6] =3D=3D '\0') || (end =3D=3D NULL) || (*end !=3D '\0') || > > + (errno !=3D 0)) { > > + fclose(fmap); > > + return -1; > > + } > > + > > + memcpy(&procmap.prot, in[2], PROT_SZ); > > + memcpy(&procmap.fname, in[7], PATH_MAX); > > + > > + if (procmap.va_start =3D=3D addr) { > > + procmap.len =3D procmap.len - procmap.va_start; > > + found =3D 1; > > + break; > > + } > > + } > > + fclose(fmap); > > + > > + if (!found) { > > + RTE_LOG(ERR, VHOST_CONFIG, > > + "(%"PRIu64") Failed to find memory file in pid %d maps file\n", > > + dev->device_fh, pid); > Need one more indent? >=20 > > > > +/** > > + * Searches the configuration core linked list and retrieves the devic= e if it > exists. > > + */ > > +static struct virtio_net * > > +get_device(struct vhost_device_ctx ctx) > > +{ > > + struct virtio_net_config_ll *ll_dev; > > + > > + ll_dev =3D get_config_ll_entry(ctx); > > + > > + /* If a matching entry is found in the linked list, return the device > > + * in that entry. */ > Need a blank space at start of comment? >=20 > > + if (ll_dev) > > + return &ll_dev->dev; > > + > > + RTE_LOG(ERR, VHOST_CONFIG, > > + "(%"PRIu64") Device not found in linked list.\n", ctx.fh); > > + return NULL; > > +} > > + > > +/** > > + * Add entry containing a device to the device configuration linked li= st. > > + */ > > +static void > > +add_config_ll_entry(struct virtio_net_config_ll *new_ll_dev) > > +{ > > + struct virtio_net_config_ll *ll_dev =3D ll_root; > > + > > + /* If ll_dev =3D=3D NULL then this is the first device so go to else = */ > > + if (ll_dev) { > > + /* If the 1st device_fh !=3D 0 then we insert our device here. */ > > + if (ll_dev->dev.device_fh !=3D 0) { > > + new_ll_dev->dev.device_fh =3D 0; > > + new_ll_dev->next =3D ll_dev; > > + ll_root =3D new_ll_dev; > > + } else { > > + /* Increment through the ll until we find un unused > > + * device_fh. Insert the device at that entry*/ > Need a blank space at end of comment? Will fix. >=20 > > > > +/** > > + * Function is called from the CUSE release function. This function wi= ll > cleanup > > + * the device and remove it from device configuration linked list. > > + */ > > +static void > > +destroy_device(struct vhost_device_ctx ctx) > > +{ > > + struct virtio_net_config_ll *ll_dev_cur_ctx, *ll_dev_last =3D NULL; > > + struct virtio_net_config_ll *ll_dev_cur =3D ll_root; > > + > > + /* Find the linked list entry for the device to be removed. */ > > + ll_dev_cur_ctx =3D get_config_ll_entry(ctx); > > + while (ll_dev_cur !=3D NULL) { > > + /* If the device is found or a device that doesn't exist is > > + * found then it is removed. */ > Need a blank space? Will fix. >=20 > > +/** > > + * Called from CUSE IOCTL: VHOST_SET_FEATURES > > + * We receive the negotiated set of features supported by us and the v= irtio > > + * device. > > + */ > > +static int > > +set_features(struct vhost_device_ctx ctx, uint64_t *pu) > > +{ > > + struct virtio_net *dev; > > + > > + dev =3D get_device(ctx); > > + if (dev =3D=3D NULL) > > + return -1; > > + if (*pu & ~VHOST_FEATURES) > > + return -1; > > + > > + /* Store the negotiated feature list for the device. */ > > + dev->features =3D *pu; > > + > > + /* Set the vhost_hlen depending on if VIRTIO_NET_F_MRG_RXBUF is set. > */ > > + if (dev->features & (1 << VIRTIO_NET_F_MRG_RXBUF)) { > > + LOG_DEBUG(VHOST_CONFIG, > > + "(%"PRIu64") Mergeable RX buffers enabled\n", dev->device_fh); > > + dev->virtqueue[VIRTIO_RXQ]->vhost_hlen =3D > > + sizeof(struct virtio_net_hdr_mrg_rxbuf); > > + dev->virtqueue[VIRTIO_TXQ]->vhost_hlen =3D > > + sizeof(struct virtio_net_hdr_mrg_rxbuf); > One more indent? >=20 > > > > +/** > > + * Called from CUSE IOCTL: VHOST_SET_MEM_TABLE > > + * This function creates and populates the memory structure for the de= vice. > > + * This includes storing offsets used to translate buffer addresses. > > + */ > > +static int > > +set_mem_table(struct vhost_device_ctx ctx, const void *mem_regions_add= r, > > + uint32_t nregions) > > +{ > > + struct virtio_net *dev; > > + struct vhost_memory_region *mem_regions; > > + struct virtio_memory *mem; > > + uint64_t size =3D offsetof(struct vhost_memory, regions); > > + uint32_t regionidx, valid_regions; > > + > > + dev =3D get_device(ctx); > > + if (dev =3D=3D NULL) > > + return -1; > > + > > + if (dev->mem) { > > + munmap((void *)(uintptr_t)dev->mem->mapped_address, > > + (size_t)dev->mem->mapped_size); > > + free(dev->mem); > > + } > > + > > + /* Malloc the memory structure depending on the number of regions. */ > > + mem =3D calloc(1, sizeof(struct virtio_memory) + > > + (sizeof(struct virtio_memory_regions) * nregions)); > > + if (mem =3D=3D NULL) { > > + RTE_LOG(ERR, VHOST_CONFIG, > > + "(%"PRIu64") Failed to allocate memory for dev- > >mem.\n", > > + dev->device_fh); > > + return -1; > > + } > > + > > + mem->nregions =3D nregions; > > + > > + mem_regions =3D (void *)(uintptr_t) > > + ((uint64_t)(uintptr_t)mem_regions_addr + size); > > + > > + for (regionidx =3D 0; regionidx < mem->nregions; regionidx++) { > > + /* Populate the region structure for each region. */ > > + mem->regions[regionidx].guest_phys_address =3D > > + mem_regions[regionidx].guest_phys_addr; > > + mem->regions[regionidx].guest_phys_address_end =3D > > + mem->regions[regionidx].guest_phys_address + > > + mem_regions[regionidx].memory_size; > > + mem->regions[regionidx].memory_size =3D > > + mem_regions[regionidx].memory_size; > > + mem->regions[regionidx].userspace_address =3D > > + mem_regions[regionidx].userspace_addr; > > + > > + LOG_DEBUG(VHOST_CONFIG, > > + "(%"PRIu64") REGION: %u - GPA: %p - QEMU VA: %p - > SIZE " > > + "(%"PRIu64")\n", > > + dev->device_fh, regionidx, > > + (void *)(uintptr_t) > > + mem->regions[regionidx].guest_phys_address, > > + (void *)(uintptr_t) > > + mem->regions[regionidx].userspace_address, > > + mem->regions[regionidx].memory_size); > > + > > + /*set the base address mapping*/ > Need a blank space at start and end of comment? Thanks for careful review. >=20 > > + if (mem->regions[regionidx].guest_phys_address =3D=3D 0x0) { > > + mem->base_address =3D > > + mem->regions[regionidx].userspace_address; > > + /* Map VM memory file */ > > + if (host_memory_map(dev, mem, ctx.pid, > > + mem->base_address) !=3D 0) { > > + free(mem); > > + return -1; > > + } > > + } > > + } > > + > > + /* Check that we have a valid base address. */ > > + if (mem->base_address =3D=3D 0) { > > + RTE_LOG(ERR, VHOST_CONFIG, > > + "(%"PRIu64") Failed to find base address of qemu memory " > > + "file.\n", dev->device_fh); > One more indent? >=20 > > + free(mem); > > + return -1; > > + } > > + > > + /* Check if all of our regions have valid mappings. Usually one does > > + * not exist in the QEMU memory file. */ > Need a blank space? >=20 > > + valid_regions =3D mem->nregions; > > + for (regionidx =3D 0; regionidx < mem->nregions; regionidx++) { > > + if ((mem->regions[regionidx].userspace_address < > > + mem->base_address) || > > + (mem->regions[regionidx].userspace_address > > > + (mem->base_address + mem->mapped_size))) > > + valid_regions--; > > + } > > + > > + /* If a region does not have a valid mapping we rebuild our memory > > + * struct to contain only valid entries. */ > Need a blank space? > > + if (valid_regions !=3D mem->nregions) { > > + LOG_DEBUG(VHOST_CONFIG, > > + "(%"PRIu64") Not all memory regions exist in the QEMU mem > file." > > + "Re-populating mem structure\n", > One more indent? >=20 > > + dev->device_fh); > > + > > + /* Re-populate the memory structure with only valid regions. > > + * Invalid regions are over-written with memmove. */ > Need a blank space? >=20 > Thanks, > Tetsuya