Page MenuHomeFreeBSD

Add driver for the VMware Paravirtual SCSI (pvscsi) controller
Needs RevisionPublic

Authored by vbhakta_vmware.com on Dec 20 2018, 12:26 AM.

Details

Reviewers
mp
bcr
imp
scottl
jpaetzel
Group Reviewers
manpages
Core Team
Summary

This is a CAM SIM driver that provides support for the VMware Paravirtual SCSI controller (pvscsi).

Test Plan

The following tests were run for amd64 and most for i386:
Built both in-kernel and as loadable module.
Hot-add/remove of the controller (see known issues) and disks.
Various I/O workloads on different ESX backends, including with VM operations (snapshot, suspend/resume, vMotion, fault tolerance) and with ESX path failover.
Stressed with I/O for over 24 hours and to a large number of disks.
Created ISO and installed FreeBSD on a pvscsi disk.

Known Issues:
Hot-add of the controller works but is followed by a scheduled hot-remove. This affects other VM pci devices as well.
MSI-X is blacklisted, also affects other VM pci devices (https://bugs.freebsd.org/bugzilla/show_bug.cgi?id=203874)
Single thread, single outstanding I/O performance can be ~10% worse than lsisas (mpt) - the pvscsi test appears to spend more time in acpi_cpu_c1 vs sched_idletd. The difference goes away with machdep.idle=spin.

Diff Detail

Lint
Lint Skipped
Unit
Unit Tests Skipped

Event Timeline

vbhakta_vmware.com changed the visibility from "Public (No Login Required)" to "No One".Dec 20 2018, 6:42 AM
vbhakta_vmware.com changed the edit policy from "All Users" to "No One".
vbhakta_vmware.com changed the visibility from "No One" to "Public (No Login Required)".Dec 24 2018, 10:42 PM
vbhakta_vmware.com changed the edit policy from "No One" to "All Users".
bcr accepted this revision.Dec 25 2018, 11:38 AM
bcr added a subscriber: bcr.

OK from manpages.

This revision is now accepted and ready to land.Dec 25 2018, 11:38 AM
imp added a reviewer: imp.Mar 5 2019, 11:31 PM
This revision now requires review to proceed.Mar 5 2019, 11:31 PM
imp added a comment.Mar 5 2019, 11:34 PM

Added license concern, but it shouldn't be a huge deal. haven't looked at the CAM integration yet.... That will come in time.
And in case it isn't clear, I'll drive the License issue inside of core.

sys/dev/vmware/pvscsi/pvscsi.h
5

(picking this at random) Core needs to get its act together and finalize the 'indirection' for license policy and guidelines. I like how this is done, generally, but will have tweaks to suggest for this silly mechanical issue to make sure this indirection is clear and unambiguous.

Updated the man page license header to use or instead of and. No changes to the code.

And updates on this review?

allanjude added a reviewer: jpaetzel.
imp added a comment.Jun 20 2019, 5:47 PM
In D18613#416846, @imp wrote:

Added license concern, but it shouldn't be a huge deal. haven't looked at the CAM integration yet.... That will come in time.
And in case it isn't clear, I'll drive the License issue inside of core.

I've not made progress with the license issue. Worst case, we add the full license to sys/dev/vmware/pvscsi/pvscsi.h and sys/dev/vmware/pvscsi/pvscsi.c and I'll use changes to this as an example for the 'out of file' license stuff that I'll work to established as policy.

scottl requested changes to this revision.Jun 20 2019, 6:07 PM

Overall this looks very good, but error handoff in pvscsi_process_completion() is not correct. Generally if you're going to return a status that isn't CAM_REQ_CMP, you need to do the following before calling xpt_done():

if (mprsas_get_ccbstatus(ccb) != CAM_REQ_CMP) {
         ccb->ccb_h.status |= CAM_DEV_QFRZN;
         xpt_freeze_devq(ccb->ccb_h.path, /*count*/ 1);
 }

I'm also not sure what's going on here:

	if (sc->frozen) {
		ccb->ccb_h.status |= CAM_RELEASE_SIMQ;
		sc->frozen = 0;
	}
This revision now requires changes to proceed.Jun 20 2019, 6:07 PM

Thanks for taking a look!

Overall this looks very good, but error handoff in pvscsi_process_completion() is not correct. Generally if you're going to return a status that isn't CAM_REQ_CMP, you need to do the following before calling xpt_done():

if (mprsas_get_ccbstatus(ccb) != CAM_REQ_CMP) {
         ccb->ccb_h.status |= CAM_DEV_QFRZN;
         xpt_freeze_devq(ccb->ccb_h.path, /*count*/ 1);
 }

I'm also not sure what's going on here:

	if (sc->frozen) {
		ccb->ccb_h.status |= CAM_RELEASE_SIMQ;
		sc->frozen = 0;
	}

The SCSI IO action code freezes the queue if there are no request or completion entries available. This is supposed to unfreeze it once a completion comes in that would make an entry available. But I'll revisit how this works with SCSI error handling and how the completion path handles errors generally.

@vbhakta_vmware.com is there anything I can do to facilitate the changes this driver needs to get it into the tree?

imp added a comment.Aug 13 2019, 7:13 PM

@vbhakta_vmware.com is there anything I can do to facilitate the changes this driver needs to get it into the tree?

I'd add the license file to the .c and .h, and then any core block would be gone.

imp added a comment.Aug 13 2019, 7:15 PM

Alternatively, you could add a explicit reference to the location of the license file in FreeBSD's repo in the .c and .h. That would be fine enough.

mp added a comment.Aug 13 2019, 7:15 PM

Thanks @jpaetzel for the ping. Over the weekend I updated my build systems to facilitate a build/test of this code. One outstanding issue, I've been meaning to circle back with @imp on how we should implement the licensing. I'll send a PM to work it out. I'll let @vbhakta_vmware.com comment on any remaining code changes.

imp added a comment.Aug 13 2019, 7:16 PM

P,S, I don't know the clicky button to say this is no longer blocked by core@, so don't let that stop you from committing it.

cem added a subscriber: cem.Fri, Sep 20, 9:46 PM

Just want to chime and share that Isilon has a worse version of this driver. As far as I'm concerned, we'd prefer to have this one and delete ours.