Import Isilon port of VMWare PV SCSI driver
AbandonedPublic

Authored by ngie on Nov 9 2015, 6:02 AM.

Details

Reviewers
benno
scottl
darius-dons.net.au
Apply Patch
arc patch D4112
Summary

This is a (lightly) modified version of the driver used at Isilon to speak to the VMWare PV SCSI controller.

Test Plan

Built it as a module, loaded it in an ESXi 6.0 VM with the PV SCSI controller and ran fsx for a while.

# kldload vmw_pvscsi
pvscsi0: <VMware para-virtual SCSI driver v1.1.2.0> port 0x4000-0x4007 mem 0xfd4f8000-0xfd4fffff irq 18 at device 0.0 on pci3
pvscsi0: Maximum number of targets is 16
pvscsi0: retval>6, msix_vecs_needed>1
pvscsi0: Using INTx interrupts
pvscsi0: softc:0xfffff80003b31400, unmasking interrupts w/intr-status 0
da0 at pvscsi0 bus 0 scbus2 target 0 lun 0
da0: <VMware Virtual disk 1.0> Fixed Direct Access SCSI-2 device
da0: 4294967.295MB/s transfers
da0: 8192MB (16777216 512 byte sectors)
da0: quirks=0x40<RETRY_BUSY>
# gpart create -s GPT da0
(da0:pvscsi0:0:0:0): WRITE(6). CDB: 0a 00 00 00 01 00
(da0:pvscsi0:0:0:0): CAM status: SCSI Status Error
(da0:pvscsi0:0:0:0): SCSI status: Busy
(da0:pvscsi0:0:0:0): Retrying command
da0 created
# gpart add -t freebsd-ufs da0
da0p1 added
# newfs /dev/da0p1
/dev/da0p1: 8192.0MB (16777136 sectors) block size 32768, fragment size 4096
	using 14 cylinder groups of 626.09MB, 20035 blks, 80256 inodes.
super-block backups (for fsck_ffs -b #) at:
 192, 1282432, 2564672, 3846912, 5129152, 6411392, 7693632, 8975872, 10258112,
 11540352, 12822592, 14104832, 15387072, 16669312
# mount /dev/da0p1 /mnt
# /fsx /mnt/foo
truncating to largest ever: 0x13e76
truncating to largest ever: 0x2e52c
truncating to largest ever: 0x3c2c2
truncating to largest ever: 0x3f15f
truncating to largest ever: 0x3fcb9
truncating to largest ever: 0x3fe96
truncating to largest ever: 0x3ff9d
truncating to largest ever: 0x3ffff

Diff Detail

Lint
Lint Skipped
Unit
Unit Tests Skipped
darius-dons.net.au added reviewers: scottl, benno.

Spell ISILON correctly and fix a typo.

benno added a subscriber: adrian.Nov 9 2015, 4:30 PM
emaste added a subscriber: emaste.Nov 9 2015, 4:43 PM
emaste added inline comments.
sys/dev/vmware/pvscsi/compat_freebsd.h
1–4

Can you give this a standard license header?

sys/dev/vmware/pvscsi/vmw_pvscsi.c
1–3

Suggest adding this copyright into the existing block

cem added a subscriber: cem.Nov 9 2015, 10:20 PM

pvscsi0: retval>6, msix_vecs_needed>1
pvscsi0: Using INTx interrupts

Any idea why it didn't succeed (ENXIO) in using MSI-X interrupts?

sys/dev/vmware/pvscsi/compat_freebsd.h
79–84

This is completely redundant — vtophys() already does this.

sys/dev/vmware/pvscsi/fbsd_list.h
4–23

These macros could be restated in terms of queue(3) pretty easily.

sys/dev/vmware/pvscsi/vmw_pvscsi.h
526

Probably remove this comment or figure out what should be "seen".

In D4112#86515, @cem wrote:

pvscsi0: retval>6, msix_vecs_needed>1
pvscsi0: Using INTx interrupts

Any idea why it didn't succeed (ENXIO) in using MSI-X interrupts?

The ESXi PCI-e chipset is blacklisted for MSI(-x), the comment says..

  • MSI-X allocation doesn't work properly for devices passed through
  • by VMware up to at least ESXi 5.1.

I haven't tried device passthrough, but on ESXi 6.0 MSI-x works with vmw_pvscsi (I set the flag to disable the blacklist to check).

Any chance this will be reviewed and committed for the 11 release? Seems like it would be a pretty useful addition.

A couple of key things:

  1. Licenses must be clarified. This can't go into the freebsd tree without that.
  2. It looks like this is a mix of linux and freebsd. I get that the original code was linux, and effort was made to shim in convenient places. However, it's a mixed up mess now. Either keep the linux sources as unaltered as possible and put all of the shimming into separate files, or let's work on making this use native APIs. Keeping it in-between makes it hard to maintain and hard to adopt future changes from VMWare. Do you expect this code to stay alive and evolve? The last thing I want is for the code to be tossed in and then abandoned, leaving it hard to maintain and ultimately prone to rot.
  3. Can the ISILON blocks be turned into something more descriptive? Do they encapsulate functionality that is completely unique to Isilon that could never be used by others? If so, then why put such code into FreeBSD? Again, it's hard for someone else to review and maintain this code when there are blocks of mystery functionality.

There's also a OpenBSD VMware PVSCSI driver. If there are licensing concerns with this driver, perhaps the other could be ported instead ...

https://github.com/Bluerise/OpenBSD-src/blob/master/sys/dev/pci/vmwpvs.c

There's also a OpenBSD VMware PVSCSI driver. If there are licensing concerns with this driver, perhaps the other could be ported instead ...

Presumably Isilon can BSD license this one?

benno added a subscriber: ngie.May 17 2016, 8:16 PM
benno added a subscriber: mp.
ngie commandeered this revision.May 18 2016, 1:00 AM
ngie added a reviewer: darius-dons.net.au.

Per discussion between EMC/Isilon and VMware, I'll be driving the work for getting the initial pvscsi driver committed into FreeBSD.

ngie added a comment.EditedMay 18 2016, 1:03 AM

A couple of key things:

  1. Licenses must be clarified. This can't go into the freebsd tree without that.

Will do.

  1. It looks like this is a mix of linux and freebsd. I get that the original code was linux, and effort was made to shim in convenient places. However, it's a mixed up mess now. Either keep the linux sources as unaltered as possible and put all of the shimming into separate files, or let's work on making this use native APIs. Keeping it in-between makes it hard to maintain and hard to adopt future changes from VMWare. Do you expect this code to stay alive and evolve? The last thing I want is for the code to be tossed in and then abandoned, leaving it hard to maintain and ultimately prone to rot.

This is the first revision of the driver that will hit the tree. The VMware team (mp@, etc) will be leading the way with future updates in the driver, which will involve reconciling the FreeBSD and Linux drivers. I'd prefer not deviating style(9) too much from the Linux driver for the time being -- and we can revisit making this more FreeBSD-centric.

  1. Can the ISILON blocks be turned into something more descriptive? Do they encapsulate functionality that is completely unique to Isilon that could never be used by others? If so, then why put such code into FreeBSD? Again, it's hard for someone else to review and maintain this code when there are blocks of mystery functionality.

I'm going to remove them as it requires interfaces that aren't necessarily upstreamable. I'm not sure why the CR was posted with these still in the diff; basically these were local modifications made to make functionality was desire in the product work, which might not match what upstream wants for the general case.

ngie added inline comments.May 18 2016, 1:19 AM
sys/dev/vmware/pvscsi/compat_freebsd.h
6–8

This doesn't help anyone externally as far as the lineage is concerned. Besides... this directory went away after Riptide.

61–68

Oy. bde won't be happy with these type names...

259–272

Remove from upstream driver.

sys/dev/vmware/pvscsi/vmw_pvscsi.c
522–542

Remove from upstream driver.

554

Spurious newline.

685–721

Remove from upstream driver.

887–889

This behavior seems specific to how Isilon wants things (note verbiage, i.e. "Node-wide"). Also, it seems like panic(9) might be more appropriate?

941

Spurious newline.

1030–1033

Remove from upstream driver.

1080–1083

Remove from upstream driver.

1086–1098

Remove from upstream driver.

1156

Magic number

1340–1341

Copy-paste mistake? Looks like it should be deleted, but I need to look at the Linux driver first.

1595

Remove (unnecessary return;)

1721–1728

Remove this from the upstream driver.

1771–1773

Remove this from the upstream driver.

1811
  1. Is there value in printing out the memory address for the adapter?
  2. "w/" should be spelled out like "with ".
  3. A more descriptive name than "intr-status" is desired.
1828–1832

Remove this from the upstream driver.

1842

Probably should fix this return code to work with FreeBSD (make positive instead of negative).

1864

DEVMETHOD_END here.

sys/dev/vmware/pvscsi/vmw_pvscsi.h
28–32

Remove from upstream driver.

600–602

Remove from upstream driver.

scottl added inline comments.May 18 2016, 2:01 PM
sys/dev/vmware/pvscsi/vmw_pvscsi.c
371

Don't use BUS_DMA_NOWAIT this with storage drivers. The rest of this function should go into the callback, and EINPROGRESS should be checked on the return.

Let's break this down into manageable tasks:

  1. Eliminate the use of vtophys, pci_alloc_consistent/pci_map_single, and contigmalloc/contigfree. Everything should be done with busdma. For the sgl and sense buffers, they should be allocated in a block via bus_dmamem_alloc() and then split out to the ctx objects, with their physaddr pre-computed.
  2. Switch linked lists from linux macros to *BSD queue.h macros.
  3. Get rid of the use of the rest of the linux shims, like create_workqueue/destroy_workqueue.
  4. Get rid of the linux typedefs. Is there an intention to share vow_pvscsi.h across OS's? If so then let's see if we can keep the types isolated to that.
  5. Everything else that Ngie said.

I have this driver working in a VMWare Fusion host, but it's using INTx interrupts instead of MSIx. Fusion gives no knobs for controlling any of this (I had to take a guess at how to get it to create a pvscsi device), any idea on how I can tell it to enable MSI/MSIx?

I think this driver is a great start, and I apologize if I sound harsh about it. Since it sounds like there's a long-term intention of keeping this driver developed and moving towards native interfaces, I'd like to help with it. Would anyone object if I uploaded changes to this review?

Unfortunately I'm no longer working at Isilon so I'm not in a position to push this along any further :(

I have this driver working in a VMWare Fusion host, but it's using INTx interrupts instead of MSIx. Fusion gives no knobs for controlling any of this (I had to take a guess at how to get it to create a pvscsi device), any idea on how I can tell it to enable MSI/MSIx?

The PCI code black lists MSI on certain chipsets , ESXi being one of them, wouldn't surprise me if Fusion is in the same boat.

I think this driver is a great start, and I apologize if I sound harsh about it.  Since it sounds like there's a long-term intention of keeping this driver developed and moving towards native interfaces, I'd like to help with it.  Would anyone object if I uploaded changes to this review?

Be my guest :)

ngie marked 22 inline comments as done.May 22 2016, 7:13 AM
ngie added inline comments.
sys/dev/vmware/pvscsi/vmw_pvscsi.h
526

This is Isilon specific; I've removed it.

ngie abandoned this revision.May 25 2016, 10:13 PM
ngie marked 3 inline comments as done.

Benno informed me that VMware has reviewed the driver and will be implementing a new version, specifically for FreeBSD.

I'm abandoning this CR and will not commit any more changes to ^/projects/vmware_pvscsi .