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

-----Original Message-----
From: Yuanhan Liu [mailto:yliu@fridaylinux.org]
Sent: Friday, July 7, 2017 8:51 AM
To: Liu, Changpeng <redacted>
Cc: dev@dpdk.org
Subject: Re: [PATCH] examples/vhost: introduce a new vhost-user-scsi sample
application

On Thu, Jun 29, 2017 at 05:28:32PM +0800, Changpeng Liu wrote:

quoted

+.. note::
+    You must check whether you Qemu can support "vhost-user-scsi" or not,
+    the latest Qemu code is recommended.

You should specify the least QEMU version that supports vhost-user scsi here.

Okay, will add Qemu version and  commit ID.

quoted

+
+Common Issues
+-------------
+
+* QEMU fails to allocate memory on hugetlbfs, with an error like the
+  following::
+
+      file_ram_alloc: can't mmap RAM pages: Cannot allocate memory
+
+  When running QEMU the above error indicates that it has failed to allocate
+  memory for the Virtual Machine on the hugetlbfs. This is typically due to
+  insufficient hugepages being free to support the allocation request. The
+  number of free hugepages can be checked as follows:
+
+  .. code-block:: console
+
+      cat /sys/kernel/mm/hugepages/hugepages-<pagesize>/nr_hugepages
+
+  The command above indicates how many hugepages are free to support

QEMU's

quoted

+  allocation request.
+
+* vhost-user will not work with a QEMU version without shared memory

mapping:

quoted

+
+  Make sure ``share=on`` QEMU option is given.

They are common pitfalls of vhost-user examples. You don't have to repeat
it again here. Instead, you could add a link to the vhost-example doc and
let this section only cover issues belong to vhost-user scsi.

quoted

+* vhost_scsi can not start with block size 512 Bytes:
+
+  Currently DPDK vhost library was designed for NET device(althrough the APIs
+  are generic now), for 512 Bytes block device, Qemu BIOS(x86 BIOS Enhanced
+  Disk Device) will enumerate all block device and do some IO to those block
+  devices with 512 Bytes sector size. DPDK vhost library can not process such
+  scenarios(both BIOS and OS will enumerate the block device), so as a
+  workaround, the vhost_scsi example application hardcoded the block size
+  with 512 Bytes.

A bit confused here. You said we can not use 512 bytes while you hardcode it
with 512 bytes?

Typo, hardcoded 4096 now.

quoted

+static void
+scsi_task_build_sense_data(struct vhost_scsi_task *task, int sk,
+			   int asc, int ascq)
+{
+	uint8_t *cp;
+	int resp_code;
+
+	resp_code = 0x70; /* Current + Fixed format */

Could you add macros for those magic numbers? I saw quite a lot of them.

All the numbers are defined by SCSI specifications, I think this is okay, if you looked into kernel scsi module or
kernel LIO scsi target, they also used the specification numbers directly.

BTW, you might want to build it on a 32bit system. I think this patch
won't build successfully on it.

I didn't build it with 32bit system before, I don't know the result.

	--yliu