DPDK patches and discussions
 help / color / mirror / Atom feed
From: "Tetsuya.Mukawa" <mukawa@igel.co.jp>
To: Huawei Xie <huawei.xie@intel.com>, dev@dpdk.org
Subject: Re: [dpdk-dev] [PATCH 2/3] lib/librte_vhost: vhost library support to facilitate integration with DPDK accelerated vswitch
Date: Wed, 03 Sep 2014 12:38:47 +0900	[thread overview]
Message-ID: <54068D47.4090401@igel.co.jp> (raw)
In-Reply-To: <1409648131-4301-3-git-send-email-huawei.xie@intel.com>

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.

> +/**
> + * 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?

> +/**
> + * 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?

>
> +/**
> + * 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?

> +/**
> + * 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?

> +		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

  reply	other threads:[~2014-09-03  3:34 UTC|newest]

Thread overview: 10+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2014-09-02  8:55 [dpdk-dev] [PATCH 0/3] Transform the vhost example to two parts: vhost library and example Huawei Xie
2014-09-02  8:55 ` [dpdk-dev] [PATCH 1/3] examples/vhost: remove vhost example Huawei Xie
2014-09-02  8:55 ` [dpdk-dev] [PATCH 2/3] lib/librte_vhost: vhost library support to facilitate integration with DPDK accelerated vswitch Huawei Xie
2014-09-03  3:38   ` Tetsuya.Mukawa [this message]
2014-09-03  5:39     ` Xie, Huawei
2014-09-03  6:01       ` Stephen Hemminger
2014-09-03  9:43         ` Ananyev, Konstantin
2014-09-03  6:43       ` Tetsuya.Mukawa
2014-09-02  8:55 ` [dpdk-dev] [PATCH 3/3] examples/vhost: vhost example based on vhost library Huawei Xie
2014-09-02  9:38 ` [dpdk-dev] [PATCH 0/3] Transform the vhost example to two parts: vhost library and example Thomas Monjalon

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=54068D47.4090401@igel.co.jp \
    --to=mukawa@igel.co.jp \
    --cc=dev@dpdk.org \
    --cc=huawei.xie@intel.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).