This is a (lightly) modified version of the driver used at Isilon to speak to the VMWare PV SCSI controller.
Details
Details
- Reviewers
scottl benno darius-dons.net.au
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
Diff Detail
- Lint
Lint Skipped - Unit
Tests Skipped
Event Timeline
Comment Actions
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". | |
Comment Actions
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).
Comment Actions
Any chance this will be reviewed and committed for the 11 release? Seems like it would be a pretty useful addition.
Comment Actions
A couple of key things:
- Licenses must be clarified. This can't go into the freebsd tree without that.
- 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.
- 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.
Comment Actions
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
Comment Actions
Per discussion between EMC/Isilon and VMware, I'll be driving the work for getting the initial pvscsi driver committed into FreeBSD.
Comment Actions
Will do.
- 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.
- 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.
| 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 |
| |
| 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. | |
| 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. | |
Comment Actions
Let's break this down into manageable tasks:
- 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.
- Switch linked lists from linux macros to *BSD queue.h macros.
- Get rid of the use of the rest of the linux shims, like create_workqueue/destroy_workqueue.
- 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.
- 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?
Comment Actions
Unfortunately I'm no longer working at Isilon so I'm not in a position to push this along any further :(
Comment Actions
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 :)
| sys/dev/vmware/pvscsi/vmw_pvscsi.h | ||
|---|---|---|
| 526 | This is Isilon specific; I've removed it. | |
Comment Actions
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 .
Comment Actions
Sorry for chiming in, but 1.5 years later there is still no driver in base, probably this work could be resurrected?