Thanks Tetsuya:
Some of them are due to 80 character limitation. Is it ok to break the
limitation for better indentation?
> -----Original Message-----
> From: Tetsuya.Mukawa [mailto:mukawa at igel.co.jp]
> Sent: Wednesday, September 03, 2014 11:39 AM
> To: Xie, Huawei; dev at dpdk.org
> Subject: Re: [dpdk-dev] [PATCH 2/3] lib/librte_vhost: vhost library support to
> facilitate integration with DPDK accelerated vswitch
>
> Hi Huawei,
>
> I added few comments. Mainly those comments are about source code indent.
>
> (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 character
limitation warning.
>
> > +/**
> > + * cuse_info is populated and used to register the cuse device.
> > + * vhost_net_device_ops is passed when the device is registered in main.c.
> > + */
> > +int
> > +rte_vhost_driver_register(const char *dev_name)
> > +{
> > + struct cuse_info cuse_info;
> > + char device_name[PATH_MAX] = "";
> > + char char_device_name[PATH_MAX] = "";
> > + const char *device_argv[] = { device_name };
> > +
> > + char fuse_opt_dummy[] = FUSE_OPT_DUMMY;
> > + char fuse_opt_fore[] = FUSE_OPT_FORE;
> > + char fuse_opt_nomulti[] = FUSE_OPT_NOMULTI;
> > + char *fuse_argv[] = {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.
>
> > +/**
> > + * 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 = NULL;
> > + struct procmap procmap;
> > + DIR *dp = 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 = 0;
> > + char line[BUFSIZE];
> > + char dlm[] = "- : ";
> > + char *str, *sp, *in[PROCMAP_SZ];
> > + char *end = 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 = fopen(mapfile, "r");
> > + if (fmap == 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) != 0) {
> > + str = line;
> > + errno = 0;
> > + /* Split line in to fields. */
> > + for (i = 0; i < PROCMAP_SZ; i++) {
> > + in[i] = strtok_r(str, &dlm[i], &sp);
> > + if ((in[i] == NULL) || (errno != 0)) {
> > + fclose(fmap);
> > + return -1;
> > + }
> > + str = NULL;
> > + }
> > +
> > + /* Convert/Copy each field as needed. */
> > + procmap.va_start = strtoull(in[0], &end, 16);
> > + if ((in[0] == '\0') || (end == NULL) || (*end != '\0') ||
> > + (errno != 0)) {
> > + fclose(fmap);
> > + return -1;
> > + }
> > +
> > + procmap.len = strtoull(in[1], &end, 16);
> > + if ((in[1] == '\0') || (end == NULL) || (*end != '\0') ||
> > + (errno != 0)) {
> > + fclose(fmap);
> > + return -1;
> > + }
> > +
> > + procmap.pgoff = strtoull(in[3], &end, 16);
> > + if ((in[3] == '\0') || (end == NULL) || (*end != '\0') ||
> > + (errno != 0)) {
> > + fclose(fmap);
> > + return -1;
> > + }
> > +
> > + procmap.maj = strtoul(in[4], &end, 16);
> > + if ((in[4] == '\0') || (end == NULL) || (*end != '\0') ||
> > + (errno != 0)) {
> > + fclose(fmap);
> > + return -1;
> > + }
> > +
> > + procmap.min = strtoul(in[5], &end, 16);
> > + if ((in[5] == '\0') || (end == NULL) || (*end != '\0') ||
> > + (errno != 0)) {
> > + fclose(fmap);
> > + return -1;
> > + }
> > +
> > + procmap.ino = strtoul(in[6], &end, 16);
> > + if ((in[6] == '\0') || (end == NULL) || (*end != '\0') ||
> > + (errno != 0)) {
> > + fclose(fmap);
> > + return -1;
> > + }
> > +
> > + memcpy(&procmap.prot, in[2], PROT_SZ);
> > + memcpy(&procmap.fname, in[7], PATH_MAX);
> > +
> > + if (procmap.va_start == addr) {
> > + procmap.len = procmap.len - procmap.va_start;
> > + found = 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?
>
> >
> > +/**
> > + * Searches the configuration core linked list and retrieves the device if
> > it
> exists.
> > + */
> > +static struct virtio_net *
> > +get_device(struct vhost_device_ctx ctx)
> > +{
> > + struct virtio_net_config_ll *ll_dev;
> > +
> > + ll_dev = 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?
>
> > + 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 list.
> > + */
> > +static void
> > +add_config_ll_entry(struct virtio_net_config_ll *new_ll_dev)
> > +{
> > + struct virtio_net_config_ll *ll_dev = ll_root;
> > +
> > + /* If ll_dev == NULL then this is the first device so go to else */
> > + if (ll_dev) {
> > + /* If the 1st device_fh != 0 then we insert our device here. */
> > + if (ll_dev->dev.device_fh != 0) {
> > + new_ll_dev->dev.device_fh = 0;
> > + new_ll_dev->next = ll_dev;
> > + ll_root = 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.
>
> >
> > +/**
> > + * Function is called from the CUSE release function. This function will
> 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 = NULL;
> > + struct virtio_net_config_ll *ll_dev_cur = ll_root;
> > +
> > + /* Find the linked list entry for the device to be removed. */
> > + ll_dev_cur_ctx = get_config_ll_entry(ctx);
> > + while (ll_dev_cur != 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.
>
> > +/**
> > + * Called from CUSE IOCTL: VHOST_SET_FEATURES
> > + * We receive the negotiated set of features supported by us and the virtio
> > + * device.
> > + */
> > +static int
> > +set_features(struct vhost_device_ctx ctx, uint64_t *pu)
> > +{
> > + struct virtio_net *dev;
> > +
> > + dev = get_device(ctx);
> > + if (dev == NULL)
> > + return -1;
> > + if (*pu & ~VHOST_FEATURES)
> > + return -1;
> > +
> > + /* Store the negotiated feature list for the device. */
> > + dev->features = *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 =
> > + sizeof(struct virtio_net_hdr_mrg_rxbuf);
> > + dev->virtqueue[VIRTIO_TXQ]->vhost_hlen =
> > + sizeof(struct virtio_net_hdr_mrg_rxbuf);
> One more indent?
>
> >
> > +/**
> > + * Called from CUSE IOCTL: VHOST_SET_MEM_TABLE
> > + * This function creates and populates the memory structure for the device.
> > + * This includes storing offsets used to translate buffer addresses.
> > + */
> > +static int
> > +set_mem_table(struct vhost_device_ctx ctx, const void *mem_regions_addr,
> > + uint32_t nregions)
> > +{
> > + struct virtio_net *dev;
> > + struct vhost_memory_region *mem_regions;
> > + struct virtio_memory *mem;
> > + uint64_t size = offsetof(struct vhost_memory, regions);
> > + uint32_t regionidx, valid_regions;
> > +
> > + dev = get_device(ctx);
> > + if (dev == 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 = calloc(1, sizeof(struct virtio_memory) +
> > + (sizeof(struct virtio_memory_regions) * nregions));
> > + if (mem == NULL) {
> > + RTE_LOG(ERR, VHOST_CONFIG,
> > + "(%"PRIu64") Failed to allocate memory for dev-
> >mem.\n",
> > + dev->device_fh);
> > + return -1;
> > + }
> > +
> > + mem->nregions = nregions;
> > +
> > + mem_regions = (void *)(uintptr_t)
> > + ((uint64_t)(uintptr_t)mem_regions_addr + size);
> > +
> > + for (regionidx = 0; regionidx < mem->nregions; regionidx++) {
> > + /* Populate the region structure for each region. */
> > + mem->regions[regionidx].guest_phys_address =
> > + mem_regions[regionidx].guest_phys_addr;
> > + mem->regions[regionidx].guest_phys_address_end =
> > + mem->regions[regionidx].guest_phys_address +
> > + mem_regions[regionidx].memory_size;
> > + mem->regions[regionidx].memory_size =
> > + mem_regions[regionidx].memory_size;
> > + mem->regions[regionidx].userspace_address =
> > + 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.
>
> > + if (mem->regions[regionidx].guest_phys_address == 0x0) {
> > + mem->base_address =
> > + mem->regions[regionidx].userspace_address;
> > + /* Map VM memory file */
> > + if (host_memory_map(dev, mem, ctx.pid,
> > + mem->base_address) != 0) {
> > + free(mem);
> > + return -1;
> > + }
> > + }
> > + }
> > +
> > + /* Check that we have a valid base address. */
> > + if (mem->base_address == 0) {
> > + RTE_LOG(ERR, VHOST_CONFIG,
> > + "(%"PRIu64") Failed to find base address of qemu memory "
> > + "file.\n", dev->device_fh);
> One more indent?
>
> > + 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?
>
> > + valid_regions = mem->nregions;
> > + for (regionidx = 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 != 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?
>
> > + dev->device_fh);
> > +
> > + /* Re-populate the memory structure with only valid regions.
> > + * Invalid regions are over-written with memmove. */
> Need a blank space?
>
> Thanks,
> Tetsuya
