Add virtio-scsi block storage backend support

[araujo]

Summary

The virtio block scsi interface allows guest to recognize
scsi target disks.

Test Plan

ctl.conf:

portal-group pg0 {
	discovery-auth-group no-authentication
	listen 0.0.0.0
	listen [::]
}

target iqn.2012-06.com.example:target0 {
	auth-group no-authentication
	portal-group pg0
	port ioctl/5/3

	lun 0 {
		path /z/test.img
		size 8G
	}
	lun 1 {
		path /z/test1.img
		size 8G
	}
}

bhyve:

bhyve -A -W -H -P -c 6 -m 8098M -s 0:0,hostbridge -s 31,lpc -l com1,/dev/nmdm1A -l bootrom,/usr/local/share/uefi-firmware/BHYVE_UEFI.fd
-s 29,fbuf,tcp=10.20.21.40:6032,w=800,h=600,, -s 30,xhci,tablet -s 3,virtio-blk,/z/disk.img -s 4,virtio-scsi,dev=/dev/cam/ctl5.3,iid=3 -
s 5,virtio-net,tap0,mac=00:a0:98:2c:d5:9f FreeBSD

Note: The virtio-blk is the bootable disk, the virtio-scsi are the additional disks.

From inside the guest:

root@:~ # zpool list
NAME    SIZE  ALLOC   FREE  CKPOINT  EXPANDSZ   FRAG    CAP  DEDUP  HEALTH  ALTROOT
test     15G   789M  14.2G        -         -     0%     5%  1.00x  ONLINE  -
zroot  5.50G   778M  4.74G        -         -     0%    13%  1.00x  ONLINE  -

root@:~ # zpool status test
  pool: test
 state: ONLINE
  scan: none requested
config:

	NAME        STATE     READ WRITE CKSUM
	test        ONLINE       0     0     0
	  da0       ONLINE       0     0     0
	  da1       ONLINE       0     0     0

dmesg:

da0 at vtscsi0 bus 0 scbus0 target 0 lun 0
da0: <FREEBSD CTLDISK 0001> Fixed Direct Access SPC-5 SCSI device
da0: Serial Number MYSERIAL0000
da0: 300.000MB/s transfers
da0: Command Queueing enabled
da0: 8192MB (16777216 512 byte sectors)
da1 at vtscsi0 bus 0 scbus0 target 0 lun 1
da1: <FREEBSD CTLDISK 0001> Fixed Direct Access SPC-5 SCSI device
da1: Serial Number MYSERIAL0001
da1: 300.000MB/s transfers
da1: Command Queueing enabled
da1: 8192MB (16777216 512 byte sectors)

Note: It depends of review: https://reviews.freebsd.org/D9299

[avg]

I wonder if this can be made to work with pass devices.

[avg]

usr.sbin/bhyve/pci_virtio_scsi.c

674 ↗

(On Diff #42087)

Why strdup is needed here? I think that strtoul can be called on opt just fine.
Besides, free(optvalue) below can be called on a stale pointer if there happens to be another option after "iid".

[rgrimes]

usr.sbin/bhyve/Makefile

8 ↗

(On Diff #42087)

The new adopted convention is to use ${SRCTOP} and not ${CURDIR}/../..
This is also a direct cross from userland to kernel headers, not so sure we should be doing that, isnt there a published interface to these cam components in /usr/include? I see that /usr/include/cam exists, but I do not see a ctl directory. Do additional headers need to be exported to /usr/include?

usr.sbin/bhyve/bhyve.8

294 ↗

(On Diff #42087)

This is a rather terse man page update. No description of either the path or optional argument. Also should probably now .Xr to appropriate cam pages Also port_and_initiatpr_id is spelled iid= in the code.

usr.sbin/bhyve/iov.h

3 ↗

(On Diff #42087)

Can you ask Jakub Klama to remove "All rights reserved" from these files, it is an obsolete convention from the Buenos Aires Convention of 1910, it no longer has any legal effect in any jurisdiction as of 2000. It would also be nice to SPDX tag these files at this time.

usr.sbin/bhyve/pci_virtio_scsi.c

353 ↗

(On Diff #42087)

Missing () around return value.

650 ↗

(On Diff #42087)

extra white space

674 ↗

(On Diff #42087)

Should not the free be inside the if? the optvalue is only allocated inside the if, free it inside the if too makes since.

[bcr]

s/scsi/SCSI/ suggestion.

usr.sbin/bhyve/bhyve.8

220 ↗

(On Diff #42087)

I think it would be better to capitalize "scsi" here. On the lines above, we have the same for PCI and below for RNG.

292 ↗

(On Diff #42087)

same here: s/scsi/SCSI/

[araujo]

In D15276#322033, @avg wrote:

I wonder if this can be made to work with pass devices.

Was not my intention, and It is very tight to ctl and cam.

[araujo]

Address avg@, @bcr and @rgrimes suggestions.

[araujo]

Turn off debugs on pci_virtio_scsi.c, updated the review with this option on by mistake.

[rgrimes]

usr.sbin/bhyve/Makefile

8 ↗

(On Diff #42087)

I still have concerns here about bhyve(8) reaching into kernel header files as an undocumented interface.

usr.sbin/bhyve/iov.h

5 ↗

(On Diff #42133)

This got marked as done, but I still see Al Rights Reserved?

[mav]

It looks good enough for me. I see some rough edges in details of SCSI interface implementation (for example, there seem to be a race window between regular and task management requests processing, submitted via different queues), but those may be addressed/improved later.

usr.sbin/bhyve/bhyve.8

220 ↗

(On Diff #42133)

This interface is in no way "block" interface. It is raw/generic SCSI. The fact that CTL now supports only disks and CDs does not mean it can't support others some day. Also not a fact that virtio-scsi will always be backed only by CTL.

292 ↗

(On Diff #42133)

Again "Block".

[araujo]

usr.sbin/bhyve/iov.h

5 ↗

(On Diff #42133)

Most of the time you don't add anything good in your reviews, just come and complain about other peoples work.

I didn't see you come to this revision and say the same:
https://svnweb.freebsd.org/changeset/base/333460

'All right reserved' still can be used, althought there is no legal de fato in any other country that used to support it. So it is not mandatory and people are still using that, for different reasons.

The best way would be write something or start a campaign to ask everybody to remove that.

Although I still believe, even you are still seeing this here, it will be here, and unfortunately I won't remove that, just because you kind of demand.

[araujo]

• Fix virtio-scsi uefi-edk2 boot problem: uefi-edk2 VirtioScsi expects that MaxSectors must not be less than 2 at the step 4a -- retrieve and validate features.

NOTE: Also a patch for uefi-edk2-bhyve that enables virtio-scsi: https://github.com/freebsd/uefi-edk2/pull/4

[araujo]

Restore files iov.h, iov.c and pci_virtio_scsi.c removed by mistake. Also fix the last bits in the manpage.