Page MenuHomeFreeBSD

NVMe controller emulator for bhyve.
AbandonedPublic

Authored by grehan on Jan 20 2018, 12:21 PM.

Details

Summary

This emulator is implemented according to NVMe specification 1.0, and didn't covered all. However, I tested using newfs and nvmecontrol perf manually; These ware cleared.

Diff Detail

Lint
Lint Skipped
Unit
Unit Tests Skipped

Event Timeline

sux2mfgj_gmail.com edited the summary of this revision. (Show Details)Jan 20 2018, 12:27 PM
sux2mfgj_gmail.com edited the summary of this revision. (Show Details)
cem added a reviewer: imp.Jan 20 2018, 6:35 PM
cem added a subscriber: cem.

Awesome!

I can't speak to the NVMe emulation itself (I know almost nothing about the protocol and nothing about Bhyve HW emu interfaces). Maybe Warner can.

sys/dev/nvme/nvme.h
127

indentation looks off here. is it just phabricator?

usr.sbin/bhyve/Makefile
65

don't need trailing slash

usr.sbin/bhyve/pci_nvme.c
19

style nit: static FILE *dbg; (asterisk goes on the right side of the space)

21

style nit: Should be tab indents throughout (can't tell if that's just Phab making it look like 4 spaces or actually 4 spaces).

I did a very quick one time scan down the code just to see what was here, these are my comments about it.
None of the functions have block start comments stating what that function does.

usr.sbin/bhyve/pci_nvme.c
1

File is missing a copyright, and a $FreeBSD$, where did this code come from? Who wrote it?

850

style nit, extra blank line

985

Why is this code here and all commented out?
#if 0/#endif would make this more readable.

Leon Dang has also been working on NVMe emulation, and his version works with Linux, Windows and UEFI boot. I'll post that code for review since it is a bit more recent and tested. In the meantime, that version can be seen at www.freebsd.org/~grehan/pci_nvme.c

(There is also a version of UEFI with NVMe support compiled in at www.freebsd.org/~grehan/BHYVE_NVMe.fd.xz )

seanc added a subscriber: seanc.Jan 22 2018, 3:46 AM

The updated version of this code is at https://reviews.freebsd.org/D14022

I've moved the reviewer/subscriber list over as well.

chuck added a subscriber: chuck.Jan 25 2018, 3:49 PM

Overall, it is exciting to see this work being done. I realize the code is in its early stages and has asserts to help catch "the important" code paths, but it might be good to remove some of the asserts and have the commands set standard NVMe errors where appropriate.

usr.sbin/bhyve/pci_nvme.c
371

style(9): Values in return statements should be enclosed in parentheses.

404

Indentation problem here and for several lines

415

It would be good to set the subclass and programming interface values. I.e.:

PCIR_SUBCLASS to 08h and
PCIR_PROGIF to 02h

Note that both the values above have #defines in pcireg.h

566

style(9) (I think) nit; No return needed for void functions.

grehan requested changes to this revision.Jan 25 2018, 3:55 PM

Chuck - the review for this is now in D14022, where I believe your comments have already been addressed.

This revision now requires changes to proceed.Jan 25 2018, 3:55 PM
chuck added a comment.Jan 25 2018, 4:39 PM

@grehan OK. So is the thought that this review be closed and efforts should focus on D14022?

Yep, that's correct. The code in the new review is based on this code so it's really a continuation of the effort.

grehan commandeered this revision.Jan 25 2018, 4:44 PM
grehan abandoned this revision.
grehan edited reviewers, added: sux2mfgj_gmail.com; removed: grehan.

Closing this revision - moving to D14022