Page MenuHomeFreeBSD

nvme: Alternate approach to restoring API
ClosedPublic

Authored by chuck on Jul 23 2018, 2:04 PM.
Tags
None
Referenced Files
Unknown Object (File)
Sat, Apr 6, 1:40 PM
Unknown Object (File)
Sat, Apr 6, 11:17 AM
Unknown Object (File)
Sat, Apr 6, 10:20 AM
Unknown Object (File)
Sat, Apr 6, 8:35 AM
Unknown Object (File)
Sat, Apr 6, 4:34 AM
Unknown Object (File)
Dec 22 2023, 11:03 PM
Unknown Object (File)
Dec 21 2023, 9:16 PM
Unknown Object (File)
Dec 13 2023, 6:13 AM

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 - subversion
Lint
Lint Skipped
Unit
Tests Skipped
Build Status
Buildable 18266

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

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

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

sys/dev/nvme/nvme.h
429

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