Add driver for the VMware Paravirtual SCSI (pvscsi) controller
ClosedPublic
Actions

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

Details

Reviewers

mp
bcr
imp
scottl
vbhakta_vmware.com

Group Reviewers

manpages
Core Team

Commits

rS354715: Add the pvscsi driver to the tree.

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

Repository

rS FreeBSD src repository - subversion

Lint

Lint Skipped

Unit

Tests Skipped

Event Timeline

vbhakta_vmware.com created this revision.Dec 20 2018, 12:26 AM

Herald added a reviewer: manpages. · View Herald TranscriptDec 20 2018, 12:26 AM

Herald added a subscriber: imp. · View Herald Transcript

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

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

Herald added a reviewer: Core Team. · View Herald TranscriptMar 5 2019, 11:31 PM

This revision now requires review to proceed.Mar 5 2019, 11:31 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
4	(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: scottl.Jun 20 2019, 5:22 PM

allanjude added a reviewer: jpaetzel.

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.

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!

In D18613#447692, @scottl wrote:
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?

In D18613#462036, @jpaetzel wrote:

@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.

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.

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.