[dpdk-dev] [PATCH v3] examples/vhost: introduce a new vhost-user-scsi sample application

[Wodkowski, PawelX]

Hi, sorry for late review but just spotted this patch.
Please see my comments

> diff --git a/examples/vhost_scsi/scsi.c b/examples/vhost_scsi/scsi.c
> new file mode 100644
> index 0000000..08dfe61
> --- /dev/null
> +++ b/examples/vhost_scsi/scsi.c
> @@ -0,0 +1,507 @@

> +static int
> +vhost_hex2bin(char ch)
> +{
> +	if ((ch >= '0') && (ch <= '9'))
> +		return ch - '0';
> +	ch = tolower(ch);
> +	if ((ch >= 'a') && (ch <= 'f'))
> +		return ch - 'a' + 10;
> +	return (int)ch;
> +}

consider using strtol()

> +
> +int
> +vhost_bdev_process_scsi_commands(struct vhost_block_dev *bdev,
> +				 struct vhost_scsi_task *task)
> +{
> +	int len;
> +	uint8_t *data;
> +	uint64_t fmt_lun = 0;
> +	const uint8_t *lun;
> +	uint8_t *cdb = (uint8_t *)task->req->cdb;
> +
> +	lun = (const uint8_t *)task->req->lun;
> +	/* only 1 LUN supported */
> +	if (lun[0] != 1 || lun[1] >= 1)
> +		return -1;
> +
> +	switch (cdb[0]) {
> +	case SPC_INQUIRY:
> +		len = vhost_bdev_scsi_inquiry_command(bdev, task);
> +		task->data_len = len;
> +		break;
> +	case SPC_REPORT_LUNS:
> +		data = (uint8_t *)task->iovs[0].iov_base;
> +		fmt_lun |= (0x0ULL & 0x00ffULL) << 48;

Please provide a description for this magical formula for non-SCSI proficient community.

> +		to_be64((void *)&data[8], fmt_lun);
> +		to_be32((void *)data, 8);
> +		task->data_len = 16;
> +		scsi_task_set_status(task, SCSI_STATUS_GOOD, 0, 0, 0);
> +		break;
> +	case SPC_MODE_SELECT_6:
> +	case SPC_MODE_SELECT_10:
> +		/* TODO: Add SCSI Commands support */
> +		scsi_task_set_status(task, SCSI_STATUS_GOOD, 0, 0, 0);
> +		break;
> +	case SPC_MODE_SENSE_6:
> +	case SPC_MODE_SENSE_10:
> +		/* TODO: Add SCSI Commands support */
> +		scsi_task_set_status(task, SCSI_STATUS_GOOD, 0, 0, 0);
> +		break;

If those cases are mandatory they should be implemented if not removing TODO would be better than informing that
there is something to do.

> +	case SPC_TEST_UNIT_READY:
> +		scsi_task_set_status(task, SCSI_STATUS_GOOD, 0, 0, 0);
> +		break;
> +	default:
> +		len = vhost_bdev_scsi_process_block(bdev, task);
> +		task->data_len = len;
> +	}
> +
> +	return 0;
> +}
> diff --git a/examples/vhost_scsi/scsi_spec.h b/examples/vhost_scsi/scsi_spec.h
> new file mode 100644
> index 0000000..0474d92
> --- /dev/null
> +++ b/examples/vhost_scsi/scsi_spec.h
> @@ -0,0 +1,488 @@

Most of the scsi_spec.h file is unused, please remove unnecessary parts. 

> diff --git a/examples/vhost_scsi/vhost_scsi.c
> b/examples/vhost_scsi/vhost_scsi.c
> new file mode 100644
> index 0000000..160c2e0
> --- /dev/null
> +++ b/examples/vhost_scsi/vhost_scsi.c
> @@ -0,0 +1,472 @@

> +
> +#define VIRTIO_SCSI_FEATURES ((1 << VIRTIO_F_NOTIFY_ON_EMPTY) |\
> +			      (1 << VIRTIO_RING_F_EVENT_IDX) |\

Am I missing something or this example app doesn't support EVENT_IDX?

> +			      (1 << VIRTIO_SCSI_F_INOUT) |\
> +			      (1 << VIRTIO_SCSI_F_CHANGE))
> +
> +/* Path to folder where character device will be created. Can be set by user.
> */
> +static char dev_pathname[PATH_MAX] = "";
> +
> +static struct vhost_scsi_ctrlr *g_vhost_ctrlr;
> +static int g_should_stop;
> +static sem_t exit_sem;
> +
> +#define NUM_OF_SCSI_QUEUES 3
> +
> +static struct vhost_scsi_ctrlr *
> +vhost_scsi_ctrlr_find(__rte_unused const char *ctrlr_name)
> +{
> +	/* currently we only support 1 socket file fd */

So remove this function.

> +	return g_vhost_ctrlr;
> +}
> +
> +static uint64_t gpa_to_vva(int vid, uint64_t gpa)
> +{
> +	char path[PATH_MAX];
> +	struct vhost_scsi_ctrlr *ctrlr;
> +	int ret = 0;
> +
> +	ret = rte_vhost_get_ifname(vid, path, PATH_MAX);
> +	if (ret) {
> +		fprintf(stderr, "Cannot get socket name\n");
> +		assert(ret != 0);
> +	}
> +
> +	ctrlr = vhost_scsi_ctrlr_find(path);
> +	if (!ctrlr) {
> +		fprintf(stderr, "Controller is not ready\n");
> +		assert(ctrlr != NULL);
> +	}
> +
> +	assert(ctrlr->mem != NULL);
> +
> +	return rte_vhost_gpa_to_vva(ctrlr->mem, gpa);
> +}
> +

Replace vid in  vhost_block_dev by pointer to struct vhost_scsi_ctrlr
This way you will not need this whole function at all.

> +static struct vring_desc *
> +descriptor_get_next(struct vring_desc *vq_desc, struct vring_desc
> *cur_desc)
> +{
> +	return &vq_desc[cur_desc->next];
> +}
> +
> +static bool
> +descriptor_has_next(struct vring_desc *cur_desc)
> +{
> +	return !!(cur_desc->flags & VRING_DESC_F_NEXT);
> +}
> +
> +static bool
> +descriptor_is_wr(struct vring_desc *cur_desc)
> +{
> +	return !!(cur_desc->flags & VRING_DESC_F_WRITE);
> +}

I have an impression that all those functions should go to library instead of example application.

> +
> +static struct vhost_block_dev *
> +vhost_scsi_bdev_construct(const char *bdev_name, const char
> *bdev_serial,
> +			  uint32_t blk_size, uint64_t blk_cnt,
> +			  bool wce_enable)
> +{
> +	struct vhost_block_dev *bdev;
> +
> +	bdev = rte_zmalloc(NULL, sizeof(*bdev), RTE_CACHE_LINE_SIZE);
> +	if (!bdev)
> +		return NULL;
> +
> +	strncpy(bdev->name, bdev_name, sizeof(bdev->name));
> +	strncpy(bdev->product_name, bdev_serial, sizeof(bdev-
> >product_name));
> +	bdev->blocklen = blk_size;
> +	bdev->blockcnt = blk_cnt;
> +	bdev->write_cache = wce_enable;

write_cache is not used and should be removed.

> +
> +	/* use memory as disk storage space */
> +	bdev->data = rte_zmalloc(NULL, blk_cnt * blk_size, 0);
> +	if (!bdev->data) {
> +		fprintf(stderr, "no enough reseverd huge memory for
> disk\n");
> +		return NULL;
> +	}
> +
> +	return bdev;
> +}
> +
> +static void
> +process_requestq(struct vhost_scsi_ctrlr *ctrlr, uint32_t q_idx)
> +{
> +	int ret;
> +	struct vhost_scsi_queue *scsi_vq;
> +	struct rte_vhost_vring *vq;
> +
> +	scsi_vq = &ctrlr->bdev->queues[q_idx];
> +	vq = &scsi_vq->vq;
> +	ret = rte_vhost_get_vhost_vring(ctrlr->bdev->vid, q_idx, vq);
> +	assert(ret == 0);
> +
> +	while (vq->avail->idx != scsi_vq->last_used_idx) {
> +		int req_idx;
> +		uint16_t last_idx;
> +		struct vhost_scsi_task *task;
> +
> +		last_idx = scsi_vq->last_used_idx & (vq->size - 1);
> +		req_idx = vq->avail->ring[last_idx];
> +
> +		task = rte_zmalloc(NULL, sizeof(*task), 0);
> +		assert(task != NULL);
> +
> +		task->ctrlr = ctrlr;
> +		task->bdev = ctrlr->bdev;
> +		task->vq = vq;
> +		task->req_idx = req_idx;
> +		task->desc = &task->vq->desc[task->req_idx];
> +
> +		/* does not support indirect descriptors */
> +		assert((task->desc->flags & VRING_DESC_F_INDIRECT) == 0);
> +		scsi_vq->last_used_idx++;
> +
> +		task->req = (void *)gpa_to_vva(task->bdev->vid,
> +					       task->desc->addr);
> +
> +		task->desc = descriptor_get_next(task->vq->desc, task-
> >desc);
> +		if (!descriptor_has_next(task->desc)) {
> +			task->dxfer_dir = SCSI_DIR_NONE;
> +			task->resp = (void *)gpa_to_vva(task->bdev->vid,
> +							task->desc->addr);
> +
> +		} else if (!descriptor_is_wr(task->desc)) {
> +			task->dxfer_dir = SCSI_DIR_TO_DEV;
> +			vhost_process_write_payload_chain(task);
> +		} else {
> +			task->dxfer_dir = SCSI_DIR_FROM_DEV;
> +			vhost_process_read_payload_chain(task);
> +		}
> +
> +		ret = vhost_bdev_process_scsi_commands(ctrlr->bdev,
> task);
> +		if (ret) {
> +			/* invalid response */
> +			task->resp->response =
> VIRTIO_SCSI_S_BAD_TARGET;
> +		} else {
> +			/* successfully */
> +			task->resp->response = VIRTIO_SCSI_S_OK;
> +			task->resp->status = 0;

You need to fill resp->resid field here.

> +		}
> +		submit_completion(task);
> +		rte_free(task);
> +	}
> +}
> +
> +/* Main framework for processing IOs */
> +static void *
> +ctrlr_worker(void *arg)
> +{
> +	uint32_t idx, num;
> +	struct vhost_scsi_ctrlr *ctrlr = (struct vhost_scsi_ctrlr *)arg;
> +	cpu_set_t cpuset;
> +	pthread_t thread;
> +
> +	thread = pthread_self();
> +	CPU_ZERO(&cpuset);
> +	CPU_SET(0, &cpuset);
> +	pthread_setaffinity_np(thread, sizeof(cpu_set_t), &cpuset);
> +
> +	num =  rte_vhost_get_vring_num(ctrlr->bdev->vid);
> +	fprintf(stdout, "Ctrlr Worker Thread Started with %u Vring\n", num);
> +
> +	if (num != NUM_OF_SCSI_QUEUES) {

Again, how many queues doeas this app support 8 or NUM_OF_SCSI_QUEUES?

> +		fprintf(stderr, "Only 1 IO queue are supported\n");
> +		exit(0);
> +	}
> +
> +	while (!g_should_stop && ctrlr->bdev != NULL) {
> +		/* At least 3 vrings, currently only can support 1 IO queue
> +		 * Queue 2 for IO queue, does not support TMF and hotplug
> +		 * for the example application now
> +		 */
> +		for (idx = 2; idx < num; idx++)
> +			process_requestq(ctrlr, idx);
> +	}
> +
> +	fprintf(stdout, "Ctrlr Worker Thread Exiting\n");
> +	sem_post(&exit_sem);
> +	return NULL;
> +}
> +

> +
> +static struct vhost_scsi_ctrlr *
> +vhost_scsi_ctrlr_construct(const char *ctrlr_name)
> +{
> +	int ret;
> +	struct vhost_scsi_ctrlr *ctrlr;
> +	char *path;
> +	char cwd[PATH_MAX];
> +
> +	/* always use current directory */
> +	path = getcwd(cwd, PATH_MAX);
> +	if (!path) {
> +		fprintf(stderr, "Cannot get current working directory\n");
> +		return NULL;
> +	}
> +	snprintf(dev_pathname, sizeof(dev_pathname), "%s/%s", path,
> ctrlr_name);
> +
> +	if (access(dev_pathname, F_OK) != -1) {
> +		if (unlink(dev_pathname) != 0)
> +			rte_exit(EXIT_FAILURE, "Cannot remove %s.\n",
> +				 dev_pathname);
> +	}
> +
> +	fprintf(stdout, "socket file: %s\n", dev_pathname);
> +
> +	if (rte_vhost_driver_register(dev_pathname, 0) != 0) {
> +		fprintf(stderr, "socket %s already exists\n", dev_pathname);
> +		return NULL;
> +	}
> +
> +	ret = rte_vhost_driver_set_features(dev_pathname,
> VIRTIO_SCSI_FEATURES);
> +	if (ret) {
> +		fprintf(stderr, "Set vhost driver features failed\n");
> +		return NULL;
> +	}
> +
> +	ctrlr = rte_zmalloc(NULL, sizeof(*ctrlr), RTE_CACHE_LINE_SIZE);
> +	if (!ctrlr)
> +		return NULL;
> +
> +	ctrlr->name = strdup(ctrlr_name);

name is never used  and also never free.

> +
> +	rte_vhost_driver_callback_register(dev_pathname,
> +					   &vhost_scsi_device_ops);
> +
> +	return ctrlr;
> +}
> +
> +static void
> +signal_handler(__rte_unused int signum)
> +{

Some message would be good to inform about successful exit.

> +	if (access(dev_pathname, F_OK) == 0)
> +		unlink(dev_pathname);
> +	exit(0);
> +}
> +

> diff --git a/examples/vhost_scsi/vhost_scsi.h b/examples/vhost_scsi/vhost_scsi.h
> new file mode 100644
> index 0000000..b5340cc
> --- /dev/null
> +++ b/examples/vhost_scsi/vhost_scsi.h
> @@ -0,0 +1,250 @@

Do we need this file at all? I think it can got to .c file

> +
> +#include <rte_vhost.h>
> +
> +static inline uint16_t
> +from_be16(const void *ptr)
> +{
> +	const uint8_t *tmp = (const uint8_t *)ptr;
> +
> +	return (((uint16_t)tmp[0] << 8) | tmp[1]);
> +}
> +

Why implementing this instead of using existing macros from rte_byteorder.h?

> +
> +struct vaddr_region {
> +	void *vaddr;
> +	uint64_t len;
> +};

I don't see any usage of this struct.

> +
> +struct vhost_scsi_queue {
> +	struct rte_vhost_vring vq;
> +	uint16_t last_avail_idx;
> +	uint16_t last_used_idx;
> +};
> +
> +struct vhost_block_dev {
> +	/** ID for vhost library. */
> +	int vid;

Don't think this is right place for vid. As mentioned, pointer to struct vhost_scsi_ctrlr would be better here.

> +	/** Queues for the block device */
> +	struct vhost_scsi_queue queues[8];

You define 8 queues here but in vhost_scsi.c NUM_OF_SCSI_QUEUES is 3.
Maybe move definition of NUM_OF_SCSI_QUEUES to .h file and use it here instead of '8'

> +	/** Unique name for this block device. */
> +	char name[64];
> +
> +	/** Unique product name for this kind of block device. */
> +	char product_name[256];
> +
> +	/** Size in bytes of a logical block for the backend */
> +	uint32_t blocklen;
> +
> +	/** Number of blocks */
> +	uint64_t blockcnt;
> +
> +	/** write cache enabled, not used at the moment */
> +	int write_cache;

So can be removed.

> +
> +	/** use memory as disk storage space */
> +	uint8_t *data;
> +};
> +

Thanks
Pawel