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
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

Removed Linux stuff that crept into the patch

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
429 ↗(On Diff #45725)

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

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
429 ↗(On Diff #45725)

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

sys/dev/nvme/nvme.h
429 ↗(On Diff #45725)

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

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 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

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 ↗(On Diff #46775)

missing space before =

sbin/nvmecontrol/power.c
107 ↗(On Diff #46775)

missing space before =

This revision now requires changes to proceed.Aug 16 2018, 2:42 PM
kbowling added a subscriber: kbowling.

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

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

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

Thank you!

This revision is now accepted and ready to land.Aug 21 2018, 3:10 PM

Don't forget to bump __FreeBSD_version.

This revision was automatically updated to reflect the committed changes.

@mav thanks for the reminder. version bumped to 1200081