Differential D59536

camcontrol: Add an nvfeature subcommand for NVMe Get/Set Features
Needs ReviewPublic
Actions

Authored by ken on Wed, Sep 9, 3:35 PM.

Details

Reviewers

None

Group Reviewers

cam

Summary

Add NVMe Get Features / Set Features support to camcontrol(8):

camcontrol nvfeature ndaX -a <get|set> -f feature [-d data]
    [-s num] [-U]

Currently, only the hostid feature is fully implemented. The
hostid value for set is specified with -d as a UUID or an integer
in decimal, hex, or octal, and -U outputs the hostid as a UUID on
get.

The -s argument exposes the feature select / save fields from the
NVMe specification. If bit 4 of the Optional NVM Command Support
(ONCS) field of the Identify Controller data is set, each feature
has a default, saved and current value, plus a capabilities mask:

For get, -s selects which to display: 0 (current), 1 (default), 2 (saved) or 3 (capabilities).
For set, -s is the save flag; a value of 1 sets the saved value as well as the current value, so the feature persists across an NVMe reset. Setting the save flag only works if the feature's capability mask includes saveable.

sbin/camcontrol/nvfeature.c:

New file, the camcontrol(8) nvfeature subcommand for getting
and setting NVMe features.

sbin/camcontrol/camcontrol.c:

Add the nvfeature subcommand.

sbin/camcontrol/camcontrol.h:

Add the nvmefeature() prototype.

sbin/camcontrol/Makefile:

Add nvfeature.c to the build.

sbin/camcontrol/camcontrol.8:

Document the nvfeature subcommand.

Co-authored-by: Reid Linnemann <reidl@spectralogic.com>
Co-authored-by: Dave Baukus <daveb@spectralogic.com>
Sponsored by: Spectra Logic

Depends on D59535

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>

Diff Detail

Repository

rG FreeBSD src repository

Lint

Lint Passed

Unit

No Test Coverage

Build Status

Buildable 76701
Build 73584: arc lint + arc unit

Event Timeline

ken created this revision.Wed, Sep 9, 3:35 PM

Herald added subscribers: ziaee, imp. · View Herald TranscriptWed, Sep 9, 3:35 PM

ken requested review of this revision.Wed, Sep 9, 3:35 PM

Harbormaster completed remote builds in B76701: Diff 186279.Wed, Sep 9, 3:35 PM

Fix spelling of the changeable capability name

Harbormaster completed remote builds in B76702: Diff 186281.Wed, Sep 9, 3:36 PM

ken edited the summary of this revision. (Show Details)Wed, Sep 9, 3:41 PM

imp added inline comments.Wed, Sep 9, 8:05 PM

sbin/camcontrol/camcontrol.8
421	same comment about nv prefix as before. Why not just feature.
3029	a spot check shows these are all in the tables below. How are they not implemented?
sbin/camcontrol/nvfeature.c
229	This is weird and likely deserves a comment.

Adapt to the wire-byte-order hostid convention from D59531: drop the pre-print byte swapping (which also fixes a bug where the 8-byte hostid was truncated through a uint8_t on big-endian), decode the UUID display with uuid_dec_be() to match the uuid_enc_be() used on set, and use le64enc() to build the 8-byte set payload.

Harbormaster completed remote builds in B76717: Diff 186301.Wed, Sep 9, 8:31 PM

ken mentioned this in D59531: nvme: Add reservation, format and features command building support.Wed, Sep 9, 8:43 PM

linnemannr_gmail.com added a subscriber: linnemannr_gmail.com.Wed, Sep 9, 10:24 PM

linnemannr_gmail.com added inline comments.Wed, Sep 9, 10:44 PM

sbin/camcontrol/camcontrol.8
421	I chose the nv prefix to make it obvious that a subcommand was for nvme devices only. Honestly I think camcontrol is in need of some reorganization at this point to add one or more subcommand layers and split the documentation out to the top layer subcommands. e.g. if we had a 'camcontrol nvme' subcommand, it would be obvious that 'camcontrol nvme feature' is specific to nvme devices.
3029	The constants are defined, yes, but the nvfeature_print_, nvfeature_get_, and nvfeature_set_* transforms between a varargs string and the feature value type aren't implemented for anything but hostid.

linnemannr_gmail.com added inline comments.Wed, Sep 9, 10:50 PM

sbin/camcontrol/nvfeature.c
229	Can you elaborate on what is weird to you here? Are you referring to the entire uuid stringification or the set-error/goto pattern for error handling, or something else?

imp added inline comments.Mon, Sep 21, 3:15 PM

sbin/camcontrol/camcontrol.8
421	Yes. It does need some reorg, and has for a while. But we have the paradigm we have today and the 'nv' prefix is at odds with that. But this introduces a lot of transport specific prefixes when they mostly aren't needed. camcontrol started scsi, but as ata was added, generally the protocol differences were papered over. Adding an extra 'nvme' would be even more confusing. is it camcontrol format, camcontrol nvformat, camcontrol scsiformat, camcontrol scsi format, etc. I guess why do I need to know it's specific to an nvme device? We have lots of scsi commands we don't call out as specific to the scsi device. And we have functions to find the type of the device, or the protocols it supports / uses so we can know the command isn't for that type of device / protocol and give an appropriate error message. I can ask for an identify on a scsi device, but I get an error. It works on the nda and da devices though.
3029	OK. I need volatile write cache.... And being able to send it to the ndaX device rather than needing to translate the ndaX to nvmeY would be super helpful.