Page MenuHomeFreeBSD

nvme: Alternate approach to restoring API
ClosedPublic

Authored by chuck on Jul 23 2018, 2:04 PM.

Details

Summary

Make NVMe compatible with the original API

The original NVMe API used bit-fields to represent fields in data
structures defined by the specification (e.g. the op-code in the command
data structure). The implementation targeted x86_64 processors and
defined the bit fields for little endian dwords (i.e. 32 bits).

This approach does not work as-is for big endian architectures and was
changed to use a combination of bit shifts and masks to support PowerPC.
Unfortunately, this changed the NVMe API and forces #ifdef's based on
the OS revision level in user space code.

This change reverts to something that looks like the original API, but
it uses bytes instead of bit-fields inside the packed command structure.
As a bonus, this works as-is for both big and little endian CPU
architectures.

Test Plan

Kernel changes have been tested on x86_64 hardware and via Qemu PPC64 emulator
User space changes have been tested on x86_64

Diff Detail

Repository
rS FreeBSD src repository
Lint
Lint Skipped
Unit
Unit Tests Skipped
Build Status
Buildable 18850

Event Timeline

chuck created this revision.Jul 23 2018, 2:04 PM
chuck updated this revision to Diff 45725.Jul 23 2018, 2:05 PM

Removed Linux stuff that crept into the patch

imp added a comment.Aug 9 2018, 2:32 PM

This looks OK, module the endian issue I highlighted. But that trick only works for fields that fall on even byte boundaries.

sys/dev/nvme/nvme.h
427–428

You'll need a #ifdef big/little endian here.

mav added a comment.Aug 9 2018, 2:36 PM

I have no objections against this, other then it is one more API change. I don't know why those bitfields were merged into uint16_t, while uint8_t for opc is indeed more convenient.

I like this. It cleans things up, and drops some ugly macros.

sys/dev/nvme/nvme.h
427–428

I don't think so. opc is the lowest end byte, so is in the right position here.

imp added inline comments.Aug 9 2018, 3:22 PM
sys/dev/nvme/nvme.h
427–428

Doh! Right. char a; char b; will always be right.

chuck added a comment.Aug 12 2018, 2:13 PM
In D16404#353695, @mav wrote:

I have no objections against this, other then it is one more API change. I don't know why those bitfields were merged into uint16_t, while uint8_t for opc is indeed more convenient.

For what it's worth, the API flail has been a problem for me as well. With this change, the API between 11.x and 12.x will match, and hopefully, make our lives a little bit easier when trying to MFC NVMe changes.

chuck marked 3 inline comments as done.Aug 12 2018, 2:13 PM
chuck updated this revision to Diff 46775.Aug 16 2018, 1:53 PM
chuck edited the summary of this revision. (Show Details)
chuck edited the test plan for this revision. (Show Details)

Add the user space changes and commit message

imp accepted this revision.Aug 16 2018, 2:30 PM

I like this. Make sure that smartmontools is updated too. It likely adapted to the crazy API that we had for a while...

Other than that, I love this change.

This revision is now accepted and ready to land.Aug 16 2018, 2:30 PM
smh requested changes to this revision.Aug 16 2018, 2:42 PM
smh added a subscriber: smh.

Few little style nits but apart from that much prefer this approach.

sbin/nvmecontrol/firmware.c
128

missing space before =

sbin/nvmecontrol/power.c
107

missing space before =

This revision now requires changes to proceed.Aug 16 2018, 2:42 PM
kbowling accepted this revision.Aug 18 2018, 8:33 AM
kbowling added a subscriber: kbowling.

I've tested basic functionality (nvmecontrol, mount, bulk data transfers) on ppc64

imp added a comment.Aug 18 2018, 1:58 PM

This looks ready to land. We need to get it in in the next week.

chuck added a comment.Aug 19 2018, 2:04 PM

I've tested basic functionality (nvmecontrol, mount, bulk data transfers) on ppc64

Thank you!

chuck updated this revision to Diff 46924.Aug 19 2018, 2:05 PM

Fixed style issues

chuck marked 2 inline comments as done.Aug 19 2018, 2:07 PM
smh accepted this revision.Aug 21 2018, 3:10 PM
This revision is now accepted and ready to land.Aug 21 2018, 3:10 PM
imp accepted this revision.Aug 21 2018, 3:34 PM
mav accepted this revision.Aug 21 2018, 6:57 PM

Don't forget to bump __FreeBSD_version.

This revision was automatically updated to reflect the committed changes.
chuck added a comment.Aug 22 2018, 4:31 AM

@mav thanks for the reminder. version bumped to 1200081