Page MenuHomeFreeBSD

nvme: Add reservation, format and features command building support
Needs ReviewPublic

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

Details

Reviewers
None
Group Reviewers
cam
Summary

Add command (struct nvme_command) building functions for the four
NVMe reservation opcodes (Report, Register, Acquire and Release),
Format NVM, and Get Features / Set Features, along with the
enumerations and data structures that go with them. These are
shared between the kernel and userland, and are the underpinnings
for new camcontrol(8) subcommands (nvres, nvformat, nvfeature) that
will be added separately.

The reservation type names are intentionally similar to the SCSI
names for the same reservation types.

sys/dev/nvme/nvme.h:

Add NVMe command building functions for the four NVMe
reservation opcodes: nvme_ns_resv_report_cmd(),
nvme_ns_resv_register_cmd(), nvme_ns_resv_acquire_cmd() and
nvme_ns_resv_release_cmd().

Add enumerated types for the various NVMe reservation
subcommand actions (acquire, register, release), reservation
types, and Change Persist Through Power Loss State values.

Add struct nvme_resv_acq_data and struct nvme_resv_reg_data,
the data payloads for reservation Acquire and Register.

Add a new NVMe Format NVM command building function,
nvme_admin_format_cmd(), and an enumeration for the Secure
Erase Setting parameter for format.

Add nvme_admin_get_feature_cmd() and
nvme_admin_set_feature_cmd() functions to fill a struct
nvme_command for Get Features / Set Features.

Add macros for bitfields in the ctratt and mic dwords of the
NVMe Identify Controller data structure.

Add a prototype for nvme_hostid_sbuf().

Stop byte swapping the hostid fields in
nvme_resv_status_swapbytes() and
nvme_resv_status_ext_swapbytes(), and document the fields as
little endian.  The hostid is an opaque 8 or 16 byte value;
keep it in the byte order the device returns it in instead of
interpreting it at this level.

Widen the nvme_ctrlr_cmd_get_feature() and
nvme_ctrlr_cmd_set_feature() prototypes as described below.

sys/dev/nvme/nvme_util.c:

Add nvme_hostid_sbuf(), which formats an 8 or 16 byte hostid
(from the Get Features hostid feature, in the little-endian
byte order the device returns it in) into an sbuf as an
integer value in base 16, most significant byte first.

sbin/nvmecontrol/resv.c:

Decode the now little-endian hostid fields explicitly when
printing, and zero pad the printed 64-bit values.  Previously
the extended hostid printed in a different byte order on big
and little endian machines, and without zero padding between
the two halves.

sys/dev/nvme/nvme_ctrlr_cmd.c:

Refactor nvme_ctrlr_cmd_get_feature() and
nvme_ctrlr_cmd_set_feature() to use the new command building
functions from nvme.h and accept a namespace ID parameter.

Refactor nvme_ctrlr_cmd_set_feature() to also accept the sv
(save) parameter to persist a feature value across reset.

sys/dev/nvme/nvme_ctrlr.c:

Update the temperature threshold, autonomous power state
transition and host memory buffer callers for the new
nvme_ctrlr_cmd_get_feature() and nvme_ctrlr_cmd_set_feature()
arguments.

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

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 76696
Build 73579: arc lint + arc unit

Event Timeline

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

This is fine, but I'll note that this is just on the edge of reviewability and it would be best if you broke up the commits per command in the future.

sys/dev/nvme/nvme_util.c
274

Why assume it's in host byte order? In general, we've been trying to undo the magic twiddling of structures to host order except when needed. The original big endian support erred when it did this, and as standards expand old structures, too much high level interpretive knowledge lives at the lowest levels...

In addition, if it was always in device byte order, we wouldn't need an ifdef here.

276

exhid should be a bool

ken edited the summary of this revision. (Show Details)

Keep the hostid in the little-endian byte order the device returns it in, per review feedback: no more byte swapping in the resv status swapbytes functions or ifdefs in nvme_hostid_sbuf(). exhid is now bool. Drop the nvme_le128tobe()/nvme_le128toh() split, no longer needed. Update nvmecontrol resv print for the wire-order hostid fields (also fixes zero padding and inconsistent LE/BE output).

The rest of this series is already one commit per camcontrol
subcommand (D59533 through D59536); this revision is the shared nvme.h
definitions they build on. I can split it further (reservations / format /
features) if you'd like.

Both inline comments are addressed in the updated diff. exhid is now bool.
I think you're right about the byte order. The hostid now stays in the
little-endian byte order the device returns it in. nvme_hostid_sbuf() just
prints it most significant byte first, without ifdefs, and the resv status
swapbytes functions no longer convert the hostid fields (they're
documented as little endian now). That last part changes existing behavior,
so this revision now also touches nvmecontrol resv's printing to decode
the fields explicitly — which fixes the extended hostid printing in a
different byte order on big and little endian machines, and adds the
missing zero padding between the two halves.

I can split the swapbytes/nvmecontrol piece into its own review if want.

ken marked 2 inline comments as done.Wed, Sep 9, 8:44 PM