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

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

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

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

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

quoted

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-

quoted

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);
+}
+

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