Page MenuHomeFreeBSD

ctl: Support for NVMe commands
ClosedPublic

Authored by jhb on Apr 9 2024, 11:04 PM.
Tags
None
Referenced Files
F102150338: D44720.id.diff
Fri, Nov 8, 6:09 AM
Unknown Object (File)
Mon, Oct 21, 1:51 PM
Unknown Object (File)
Thu, Oct 17, 3:17 AM
Unknown Object (File)
Mon, Oct 14, 8:00 PM
Unknown Object (File)
Mon, Oct 14, 8:00 PM
Unknown Object (File)
Sun, Oct 13, 3:56 PM
Unknown Object (File)
Sat, Oct 12, 5:32 PM
Unknown Object (File)
Fri, Oct 11, 9:10 AM
Subscribers

Details

Reviewers
imp
ken
Group Reviewers
cam
Commits
rG0c4ee619dff8: ctl: Support for NVMe commands
Summary
  • Add support for queueing and executing NVMe admin and NVM commands via ctl_run and ctl_queue. This requires fixing a few places that were SCSI-specific to add NVME logic.
  • NVMe has much simpler command ordering requirements than SCSI. In particular, the HBA is not required to enforce any specific ordering for requests with overlapping LBAs. The host is required to manage that ordering. However, fused commands (currently only COMPARE and WRITE NVM commands can be fused) are required to be executed atomically.

    To support fused commands, make the second half of a fused command block on the first half, and have commands submitted after a fused command pair block on the second half.
  • Add handlers and command tables for admin and NVM commands that operate on individual namespaces and will be passed down from an NVMe over Fabrics controller to a CTL LUN.

Sponsored by: Chelsio Communications

Diff Detail

Repository
rG FreeBSD src repository
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

jhb requested review of this revision.Apr 9 2024, 11:04 PM

Note: I did not try to create a loopback NVMe sim to expose CTL LUNs as NVMe namespaces on the local host. Presumably that wouldn't be too hard to add. I have not tried to update any of the HA bits to support NVMe.

I think this is OK. I have a few suggestions
But this is kinda large to review. There's also several change sets colocated here

  1. the scsi type asserts
  2. the accesor functions
  3. adding nvme
sys/cam/ctl/ctl.c
643

thought in passing... Do we still need this #if 0?

10664

This likely was copied from elsewhere. A #define wouldn't be bad for this, though it's a minor issue.

10891

Although this is a simple test, it's repeated multiple times and has many places to have a fencepost.
Maybe make it an inline?

11159

#define?

11321

Sometimes I wish we had a good library of 'print this command' for NVME

11592

this repetition suggests a macro might be in order

14114

unsure why you're updating the #if 0 stuff...

This revision is now accepted and ready to land.Apr 12 2024, 12:28 AM

Looks good, thank you for doing this! I'm glad to finally see CTL NVMe support going in.

sys/cam/ctl/ctl.c
14114

See the comment above. It really does help if you're trying to track down duplicate completion issues, but it isn't something we need to enable in the general case.

sys/cam/ctl/ctl.c
14114

#ifdef CTL_DEBUG_SPECIAL then?

I could indeed split this up a bit further if that is helpful. Adding wrapper routines is certainly easy to split out. I slightly worry that adding the assertions is kind of odd without the context for seeing which places were updated to handle both and which were instead changed to assert SCSI-only.

sys/cam/ctl/ctl.c
10891

Hmm, it's also repeated a bunch of times up in the SCSI handlers now (well, a similar idiom that uses slightly different spelling). Maybe I can add a similar inline for SCSI first.

11159

I would need to add one for 0x01 as well over in nvme.h for the fused fields.

11592

CTL_ASSERT_SCSI?

Update after splitting into 3 commits and adding FUSE constants

This revision now requires review to proceed.Apr 18 2024, 5:56 PM

Update for CTL_IO_ASSERT macro

This revision was not accepted when it landed; it landed in state Needs Review.May 3 2024, 12:16 AM
This revision was automatically updated to reflect the committed changes.