From mboxrd@z Thu Jan 1 00:00:00 1970 From: "Wodkowski, PawelX" Subject: Re: [PATCH v3] examples/vhost: introduce a new vhost-user-scsi sample application Date: Wed, 12 Jul 2017 14:49:38 +0000 Message-ID: References: <1499487291-17053-1-git-send-email-changpeng.liu@intel.com> <1499491297-17800-1-git-send-email-changpeng.liu@intel.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: quoted-printable Cc: "Liu, Changpeng" , "Harris, James R" , "Zedlewski, Piotr" To: "Liu, Changpeng" , "dev@dpdk.org" Return-path: Received: from mga14.intel.com (mga14.intel.com [192.55.52.115]) by dpdk.org (Postfix) with ESMTP id 49C702C54 for ; Wed, 12 Jul 2017 16:49:42 +0200 (CEST) In-Reply-To: <1499491297-17800-1-git-send-email-changpeng.liu@intel.com> Content-Language: en-US List-Id: DPDK patches and discussions List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dev-bounces@dpdk.org Sender: "dev" 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 >=3D '0') && (ch <=3D '9')) > + return ch - '0'; > + ch =3D tolower(ch); > + if ((ch >=3D 'a') && (ch <=3D '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 =3D 0; > + const uint8_t *lun; > + uint8_t *cdb =3D (uint8_t *)task->req->cdb; > + > + lun =3D (const uint8_t *)task->req->lun; > + /* only 1 LUN supported */ > + if (lun[0] !=3D 1 || lun[1] >=3D 1) > + return -1; > + > + switch (cdb[0]) { > + case SPC_INQUIRY: > + len =3D vhost_bdev_scsi_inquiry_command(bdev, task); > + task->data_len =3D len; > + break; > + case SPC_REPORT_LUNS: > + data =3D (uint8_t *)task->iovs[0].iov_base; > + fmt_lun |=3D (0x0ULL & 0x00ffULL) << 48; Please provide a description for this magical formula for non-SCSI proficie= nt community. > + to_be64((void *)&data[8], fmt_lun); > + to_be32((void *)data, 8); > + task->data_len =3D 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 TOD= O 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 =3D vhost_bdev_scsi_process_block(bdev, task); > + task->data_len =3D len; > + } > + > + return 0; > +} > diff --git a/examples/vhost_scsi/scsi_spec.h b/examples/vhost_scsi/scsi_s= pec.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.=20 > 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] =3D ""; > + > +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 =3D 0; > + > + ret =3D rte_vhost_get_ifname(vid, path, PATH_MAX); > + if (ret) { > + fprintf(stderr, "Cannot get socket name\n"); > + assert(ret !=3D 0); > + } > + > + ctrlr =3D vhost_scsi_ctrlr_find(path); > + if (!ctrlr) { > + fprintf(stderr, "Controller is not ready\n"); > + assert(ctrlr !=3D NULL); > + } > + > + assert(ctrlr->mem !=3D 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 =3D 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 =3D blk_size; > + bdev->blockcnt =3D blk_cnt; > + bdev->write_cache =3D wce_enable; write_cache is not used and should be removed. > + > + /* use memory as disk storage space */ > + bdev->data =3D 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 =3D &ctrlr->bdev->queues[q_idx]; > + vq =3D &scsi_vq->vq; > + ret =3D rte_vhost_get_vhost_vring(ctrlr->bdev->vid, q_idx, vq); > + assert(ret =3D=3D 0); > + > + while (vq->avail->idx !=3D scsi_vq->last_used_idx) { > + int req_idx; > + uint16_t last_idx; > + struct vhost_scsi_task *task; > + > + last_idx =3D scsi_vq->last_used_idx & (vq->size - 1); > + req_idx =3D vq->avail->ring[last_idx]; > + > + task =3D rte_zmalloc(NULL, sizeof(*task), 0); > + assert(task !=3D NULL); > + > + task->ctrlr =3D ctrlr; > + task->bdev =3D ctrlr->bdev; > + task->vq =3D vq; > + task->req_idx =3D req_idx; > + task->desc =3D &task->vq->desc[task->req_idx]; > + > + /* does not support indirect descriptors */ > + assert((task->desc->flags & VRING_DESC_F_INDIRECT) =3D=3D 0); > + scsi_vq->last_used_idx++; > + > + task->req =3D (void *)gpa_to_vva(task->bdev->vid, > + task->desc->addr); > + > + task->desc =3D descriptor_get_next(task->vq->desc, task- > >desc); > + if (!descriptor_has_next(task->desc)) { > + task->dxfer_dir =3D SCSI_DIR_NONE; > + task->resp =3D (void *)gpa_to_vva(task->bdev->vid, > + task->desc->addr); > + > + } else if (!descriptor_is_wr(task->desc)) { > + task->dxfer_dir =3D SCSI_DIR_TO_DEV; > + vhost_process_write_payload_chain(task); > + } else { > + task->dxfer_dir =3D SCSI_DIR_FROM_DEV; > + vhost_process_read_payload_chain(task); > + } > + > + ret =3D vhost_bdev_process_scsi_commands(ctrlr->bdev, > task); > + if (ret) { > + /* invalid response */ > + task->resp->response =3D > VIRTIO_SCSI_S_BAD_TARGET; > + } else { > + /* successfully */ > + task->resp->response =3D VIRTIO_SCSI_S_OK; > + task->resp->status =3D 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 =3D (struct vhost_scsi_ctrlr *)arg; > + cpu_set_t cpuset; > + pthread_t thread; > + > + thread =3D pthread_self(); > + CPU_ZERO(&cpuset); > + CPU_SET(0, &cpuset); > + pthread_setaffinity_np(thread, sizeof(cpu_set_t), &cpuset); > + > + num =3D rte_vhost_get_vring_num(ctrlr->bdev->vid); > + fprintf(stdout, "Ctrlr Worker Thread Started with %u Vring\n", num); > + > + if (num !=3D 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 !=3D 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 =3D 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 =3D 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) !=3D -1) { > + if (unlink(dev_pathname) !=3D 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) !=3D 0) { > + fprintf(stderr, "socket %s already exists\n", dev_pathname); > + return NULL; > + } > + > + ret =3D rte_vhost_driver_set_features(dev_pathname, > VIRTIO_SCSI_FEATURES); > + if (ret) { > + fprintf(stderr, "Set vhost driver features failed\n"); > + return NULL; > + } > + > + ctrlr =3D rte_zmalloc(NULL, sizeof(*ctrlr), RTE_CACHE_LINE_SIZE); > + if (!ctrlr) > + return NULL; > + > + ctrlr->name =3D 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) =3D=3D 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 > + > +static inline uint16_t > +from_be16(const void *ptr) > +{ > + const uint8_t *tmp =3D (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 vh= ost_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 inst= ead 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