Details

Reviewers

mav
rgrimes
jhb
grehan
tychon

Group Reviewers

manpages
bhyve

Commits

rS334940: - Add bhyve virtio-scsi storage backend support.

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

Diff Detail

Repository

rS FreeBSD src repository - subversion

Lint

Lint Passed

Unit

No Test Coverage

Build Status

Buildable 16425
Build 16345: arc lint + arc unit

Event Timeline

araujo created this revision.May 3 2018, 7:19 AM

Herald added a reviewer: rgrimes. · View Herald TranscriptMay 3 2018, 7:19 AM

Herald added a reviewer: manpages. · View Herald Transcript

Herald added a subscriber: imp. · View Herald Transcript

Harbormaster completed remote builds in B16409: Diff 42087.May 3 2018, 7:19 AM

araujo removed a reviewer: rgrimes.May 3 2018, 7:20 AM

Herald added a reviewer: rgrimes. · View Herald TranscriptMay 3 2018, 7:20 AM

araujo edited the test plan for this revision. (Show Details)May 3 2018, 7:23 AM

araujo edited the test plan for this revision. (Show Details)

araujo edited reviewers, added: bhyve, mav; removed: rgrimes.

Herald added a reviewer: rgrimes. · View Herald TranscriptMay 3 2018, 7:25 AM

avg added a subscriber: avg.May 3 2018, 8:08 AM

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

avg added inline comments.May 3 2018, 8:17 AM

usr.sbin/bhyve/pci_virtio_scsi.c
675	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".

kbowling added a subscriber: kbowling.May 3 2018, 9:06 AM

rgrimes added a reviewer: jhb.May 3 2018, 1:17 PM

rgrimes added inline comments.

usr.sbin/bhyve/Makefile
8	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	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
4	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
354	Missing () around return value.
651	extra white space
675	Should not the free be inside the if? the optvalue is only allocated inside the if, free it inside the if too makes since.

s/scsi/SCSI/ suggestion.

usr.sbin/bhyve/bhyve.8
220	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	same here: s/scsi/SCSI/

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.

Address avg@, @bcr and @rgrimes suggestions.

Harbormaster completed remote builds in B16425: Diff 42132.May 4 2018, 10:04 AM

araujo marked 8 inline comments as done.May 4 2018, 10:06 AM

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

Harbormaster completed remote builds in B16426: Diff 42133.May 4 2018, 10:07 AM

araujo edited the test plan for this revision. (Show Details)May 4 2018, 10:14 AM

rgrimes added inline comments.May 4 2018, 2:17 PM

usr.sbin/bhyve/Makefile
8	I still have concerns here about bhyve(8) reaching into kernel header files as an undocumented interface.
usr.sbin/bhyve/iov.h
6	This got marked as done, but I still see Al Rights Reserved?

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	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	Again "Block".

This revision is now accepted and ready to land.May 4 2018, 3:48 PM

araujo marked an inline comment as done.May 11 2018, 7:02 PM

araujo added inline comments.

usr.sbin/bhyve/iov.h
6	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.

mgrooms_shrew.net added a subscriber: mgrooms_shrew.net.May 23 2018, 10:20 PM

araujo added reviewers: grehan, tychon.May 30 2018, 10:20 AM

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

This revision now requires review to proceed.Jun 5 2018, 8:47 AM

Harbormaster completed remote builds in B17016: Diff 43329.Jun 5 2018, 8:47 AM

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

Harbormaster completed remote builds in B17017: Diff 43330.Jun 5 2018, 8:54 AM

araujo marked 2 inline comments as done.Jun 5 2018, 9:11 AM

This revision was not accepted when it landed; it landed in state Needs Review.Jun 11 2018, 2:09 AM

Closed by commit rS334940: - Add bhyve virtio-scsi storage backend support. (authored by araujo). · Explain Why

This revision was automatically updated to reflect the committed changes.

Add virtio-scsi block storage backend support
ClosedPublic
Actions

Details

Diff Detail

Event Timeline

Revision Contents
Changeset List

Diff 42132

usr.sbin/bhyve/Makefile

usr.sbin/bhyve/bhyve.8

usr.sbin/bhyve/iov.h

usr.sbin/bhyve/iov.c

usr.sbin/bhyve/pci_virtio_scsi.c

usr.sbin/bhyve/virtio.h

Add virtio-scsi block storage backend supportClosedPublicActions

Details

Diff Detail

Event Timeline

Revision ContentsChangeset List

Diff 42132

usr.sbin/bhyve/Makefile

usr.sbin/bhyve/bhyve.8

usr.sbin/bhyve/iov.h

usr.sbin/bhyve/iov.c

usr.sbin/bhyve/pci_virtio_scsi.c

usr.sbin/bhyve/virtio.h

Add virtio-scsi block storage backend support
ClosedPublic
Actions

Revision Contents
Changeset List