Details

Reviewers

None

Group Reviewers

Summary

Add a feature subcommand for the NVMe Get Features and Set Features
commands:

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

There is no transport-specific prefix in the name: SCSI has no
feature command (the closest equivalents are mode pages), and the
ATA SET FEATURES subcommands are already exposed through dedicated
camcontrol subcommands (apm, aam, epc and the power commands), so
the name is unclaimed. The subcommand returns an error for
non-NVMe devices.

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) feature subcommand for getting
and setting NVMe features.

sbin/camcontrol/camcontrol.c:

Add the feature subcommand, dispatched only to NVMe devices.

sbin/camcontrol/camcontrol.h:

Add the nvmefeature() prototype.

sbin/camcontrol/Makefile:

Add nvfeature.c to the build.

sbin/camcontrol/camcontrol.8:

Document the feature 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 76702
Build 73585: 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
228	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
228	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.

Rebase on the persist rework in D59534 (the nvres subcommand is gone; no changes to nvfeature itself).

Harbormaster completed remote builds in B77146: Diff 187359.Mon, Sep 21, 9:32 PM

imp added inline comments.Fri, Sep 25, 7:59 PM

sbin/camcontrol/nvfeature.c
381	So this only sets 8 of the bytes, yet we always send all 16. We either need to bzero them here, or only send 8 bytes below.
491	This is a core dump of -a isn't specified. Suggest adding a sanity check that says if (action == NULL) bail("-a required");
494	This is a core dump of -f isn't specified. Suggest adding a sanity check that says if (feature == NULL) bail("-f required"); above.

Address review feedback: error out instead of crashing when -a or -f is missing, and zero the hostid set buffer and only transfer 8 bytes for the non-extended host identifier.

Harbormaster completed remote builds in B77427: Diff 187926.Mon, Sep 28, 7:34 PM

Rework per the naming discussion: rename the subcommand from nvfeature to feature. SCSI has no feature command and the ATA SET FEATURES subcommands already have dedicated camcontrol subcommands (apm, aam, epc), so the name is unclaimed; the subcommand errors cleanly on non-NVMe devices. Also fix a being/bing typo in the man page.

Harbormaster completed remote builds in B77463: Diff 188083.Tue, Sep 29, 6:49 PM

camcontrol: Add a feature subcommand for NVMe Get/Set Features
Needs ReviewPublic
Actions

Details

Diff Detail

Event Timeline

Revision Contents
Changeset List

Diff 186281

sbin/camcontrol/Makefile

sbin/camcontrol/camcontrol.h

sbin/camcontrol/camcontrol.8

sbin/camcontrol/camcontrol.c

sbin/camcontrol/nvfeature.c

camcontrol: Add a feature subcommand for NVMe Get/Set FeaturesNeeds ReviewPublicActions

Details

Diff Detail

Event Timeline

Revision ContentsChangeset List

Diff 186281

sbin/camcontrol/Makefile

sbin/camcontrol/camcontrol.h

sbin/camcontrol/camcontrol.8

sbin/camcontrol/camcontrol.c

sbin/camcontrol/nvfeature.c

camcontrol: Add a feature subcommand for NVMe Get/Set Features
Needs ReviewPublic
Actions

Revision Contents
Changeset List