Page MenuHomeFreeBSD

bhyve: Add EUI64 to NVMe device
ClosedPublic

Authored by chuck on Apr 14 2019, 1:30 AM.

Details

Summary

Add the EUI64 field (part of the Identify Namespace data) to NVMe devices to support UEFI drivers.

The implementation will accept an IEEE Extended Unique Identifier (EUI-64) from the command line. If one isn't provided, it will create one based on

  • The IEEE OUI reported in the Identify Controller data
  • The PCI bus, device/slot, function values
  • The Namespace ID
Test Plan

Checked that multiple NVMe devices had unique EUI64 values inside the UEFI driver

Diff Detail

Repository
rS FreeBSD src repository
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

chuck created this revision.Apr 14 2019, 1:30 AM
rgrimes accepted this revision as: rgrimes.Apr 14 2019, 6:30 PM
imp added a comment.Apr 14 2019, 7:06 PM

There's only one issue that I see with this commit, and that's OUI namespace management. It's likely not a huge deal, but I think it's an easy thing to get right. emaste@ or kevans@ have the context / information to get the allocation from the foundation that I think we need. It's a minor tweak to the code to o things that way.

usr.sbin/bhyve/pci_nvme.c
378 ↗(On Diff #56188)

I see we set this to the FreeBSD OUI, so that's good. However...

382 ↗(On Diff #56188)

We partition the namespace so this is not. We need 8 bits for the bus, and another 8 bits for the slot. In bhyve, func will always be 0. So that suggests we need a 16-bit sub-space allocated and we can use that.

See the recent discussion about providing a stable, generated MAC address for context. I can dig that up if you need more than just this vague hint.

And Slot might also always be 0 if we can't get a full 16-bit range.

chuck updated this revision to Diff 57457.May 16 2019, 7:07 PM
chuck edited the test plan for this revision. (Show Details)
chuck added a reviewer: kevans.

Updated patch to define OUI range specifically for NVMe
Fixed typo while I was in ieee_oui.h (reviewed by kevans)
Use NVMe OUI macro when creating the EUI64 value. Feeds the vmname + the PCI BDF into crc16 to make the low 16 bits "unique"

imp accepted this revision.May 17 2019, 2:22 PM

Love the update.
One niggle I pointed out that I'll trust you to fix or ask about if you don't know how.

usr.sbin/bhyve/pci_nvme.c
358 ↗(On Diff #57457)

where does this come from? insert rant on copyright, attribution, license etc :)

chuck updated this revision to Diff 57509.May 18 2019, 1:24 AM

Fixed copyright attribution

Hi Chuck!

This note is for your information only. No reply or action needed.

There are many lookup table driven CRC16 implementations out there. They
all have the same the values. For the purpose of attribution, if you do a
web search by the hex values you will get many hits w/ different authors
and licenses.

This is the earliest I know.

http://cpsr.org/prevsite/cpsr/privacy/crypto/tools/other/crc-16.c/

W. David Schwaderer, 1985

It's a small bit of code.

Regards,
Rick

rgrimes accepted this revision as: rgrimes.May 23 2019, 4:45 PM
araujo accepted this revision.May 23 2019, 4:51 PM

Lgtm

jhb added inline comments.May 23 2019, 5:17 PM
usr.sbin/bhyve/pci_nvme.c
425 ↗(On Diff #57509)

To help this comment be more accurate, I might remove the 'htobe64' when setting the manual EUI64 and instead structure this code as something like:

if (eui64 == 0) {
    ...
}
be64enc(nd->eui64, eui64);
433 ↗(On Diff #57509)

Does NVMe require a specific formula for computing the input to the CRC? If not, I think a single call to asprintf() might be simpler:

uint64_t eui64 = OUI_FREEBSD_NVME_LOW;
char *data;

asprintf(&data, "%s%u%u%u", vmname, sc->nsc_pi->pi_bus,
    sc->nsc_pi->pi_slot, sc->nsc_pi->pi_func);
if (data != NULL) {
    eui64 |= crc16(data, strlen(data));
    free(data);
}

eui64 = (eui64 << 16) | (nsid & 0xffff);
442 ↗(On Diff #57509)

Missing free(data)

445 ↗(On Diff #57509)

be64enc might be a better way to do this:

be64enc(nd->eui64, eui64);

It's a bit easier to read and is also safe on non-x86 architectures with alignment constraints, etc.

chuck marked 4 inline comments as done.Jul 12 2019, 5:18 AM
chuck updated this revision to Diff 59696.Jul 12 2019, 5:42 PM

Updated based on jhb's feedback

jhb accepted this revision.Jul 12 2019, 5:47 PM
This revision is now accepted and ready to land.Jul 12 2019, 5:47 PM
This revision was automatically updated to reflect the committed changes.