Page MenuHomeFreeBSD

nvme: Use routines from nvme_util.c to decode opcodes and status codes
ClosedPublic

Authored by jhb on Jun 2 2025, 4:01 PM.
Tags
None
Referenced Files
Unknown Object (File)
Thu, Oct 9, 10:30 PM
Unknown Object (File)
Tue, Oct 7, 6:43 AM
Unknown Object (File)
Mon, Oct 6, 7:21 AM
Unknown Object (File)
Thu, Oct 2, 4:49 AM
Unknown Object (File)
Sat, Sep 27, 9:23 PM
Unknown Object (File)
Thu, Sep 25, 7:23 PM
Unknown Object (File)
Thu, Sep 25, 4:34 PM
Unknown Object (File)
Fri, Sep 19, 12:29 AM
Subscribers

Details

Summary

This reduces the number of duplicate string tables for NVMe opcodes
and status codes.

Adjust the formatting of unknown opcodes and status codes to more
closely match nvme(4).

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.Jun 2 2025, 4:01 PM

Yea, this kinda break kernels that use nvd. Well, it forces all of cam to be drug in, which may or may not be a big deal.
Better to move to a subr_nvme that both dev/nvme and dev/nvmef use, or something similar. We can chat at bsdcan.

But, those routines can move from CAM to subr_nvme.c, in the future so I guess I'm OK with this.

This revision is now accepted and ready to land.Jun 2 2025, 4:04 PM
In D50652#1156269, @imp wrote:

Yea, this kinda break kernels that use nvd. Well, it forces all of cam to be drug in, which may or may not be a big deal.
Better to move to a subr_nvme that both dev/nvme and dev/nvmef use, or something similar. We can chat at bsdcan.

But, those routines can move from CAM to subr_nvme.c, in the future so I guess I'm OK with this.

Kernels that use nvd already have to pull in CAM? Well, nvme.ko at least pulls in nvme_sim.c unconditionally. I guess custom kernels without any SATA or SCSI and only NVMe that are also using nvd instead of nda(4)? How common is that use case?

Note that nvmf doesn't use any of the decoding bits, those all live in CAM (either in nda for the client side, or in ctl for the target side). An analog would be having subr_sata.c and subr_scsi.c. It's not clear to me though that it will be worth having subr_nvme.c. It would either need to be standard, or we'd have to have a subr_nvme.ko that nvme(4), nda(4), and ctl(4) all depend on.

In D50652#1156295, @jhb wrote:
In D50652#1156269, @imp wrote:

Yea, this kinda break kernels that use nvd. Well, it forces all of cam to be drug in, which may or may not be a big deal.
Better to move to a subr_nvme that both dev/nvme and dev/nvmef use, or something similar. We can chat at bsdcan.

But, those routines can move from CAM to subr_nvme.c, in the future so I guess I'm OK with this.

Kernels that use nvd already have to pull in CAM? Well, nvme.ko at least pulls in nvme_sim.c unconditionally. I guess custom kernels without any SATA or SCSI and only NVMe that are also using nvd instead of nda(4)? How common is that use case?

It's a desired case, and people have talked to me about fixing this bug. Or rather wanting it fixed. And NVMe only-systems are quire popular. But the overhead is small in terms of extra memory, so it's not been a bug high on my list of fixing. This makes it just a little more so. It's a non-default case, and we do the right thing when both are in the kernel, so it's fine. And we've been de-emphasizing nvd, which really is only a win for ultra-high transaction rate systems.

In D50652#1156297, @imp wrote:
In D50652#1156295, @jhb wrote:
In D50652#1156269, @imp wrote:

Yea, this kinda break kernels that use nvd. Well, it forces all of cam to be drug in, which may or may not be a big deal.
Better to move to a subr_nvme that both dev/nvme and dev/nvmef use, or something similar. We can chat at bsdcan.

But, those routines can move from CAM to subr_nvme.c, in the future so I guess I'm OK with this.

Kernels that use nvd already have to pull in CAM? Well, nvme.ko at least pulls in nvme_sim.c unconditionally. I guess custom kernels without any SATA or SCSI and only NVMe that are also using nvd instead of nda(4)? How common is that use case?

It's a desired case, and people have talked to me about fixing this bug. Or rather wanting it fixed. And NVMe only-systems are quire popular. But the overhead is small in terms of extra memory, so it's not been a bug high on my list of fixing. This makes it just a little more so. It's a non-default case, and we do the right thing when both are in the kernel, so it's fine. And we've been de-emphasizing nvd, which really is only a win for ultra-high transaction rate systems.

I tried making nvme_all.c nvme | scbus in sys/conf/files and it almost worked except it needed cam_strvis_sbuf for some routines in nvme_all.c that aren't needed by nvme(4). I'm not sure what's better/worse, but we could add DEV_SCBUS to sys/conf/options and then use that #ifdef in nvme_all.c to limit the bits used in an static kernel without scbus? sys/modules/cam/Makefile would need to define it always perhaps. Not sure if that is too ugly.

In D50652#1156621, @jhb wrote:
In D50652#1156297, @imp wrote:
In D50652#1156295, @jhb wrote:
In D50652#1156269, @imp wrote:

Yea, this kinda break kernels that use nvd. Well, it forces all of cam to be drug in, which may or may not be a big deal.
Better to move to a subr_nvme that both dev/nvme and dev/nvmef use, or something similar. We can chat at bsdcan.

But, those routines can move from CAM to subr_nvme.c, in the future so I guess I'm OK with this.

Kernels that use nvd already have to pull in CAM? Well, nvme.ko at least pulls in nvme_sim.c unconditionally. I guess custom kernels without any SATA or SCSI and only NVMe that are also using nvd instead of nda(4)? How common is that use case?

It's a desired case, and people have talked to me about fixing this bug. Or rather wanting it fixed. And NVMe only-systems are quire popular. But the overhead is small in terms of extra memory, so it's not been a bug high on my list of fixing. This makes it just a little more so. It's a non-default case, and we do the right thing when both are in the kernel, so it's fine. And we've been de-emphasizing nvd, which really is only a win for ultra-high transaction rate systems.

I tried making nvme_all.c nvme | scbus in sys/conf/files and it almost worked except it needed cam_strvis_sbuf for some routines in nvme_all.c that aren't needed by nvme(4). I'm not sure what's better/worse, but we could add DEV_SCBUS to sys/conf/options and then use that #ifdef in nvme_all.c to limit the bits used in an static kernel without scbus? sys/modules/cam/Makefile would need to define it always perhaps. Not sure if that is too ugly.

Yea, that's an interesting notion. My first take is that it might be a bit too ugly since we're moving away from DEV_* device defines where possible... and I wonder how that might interact with the cam.ko module. Maybe we should just leave it alone until we start to do more work with CAM to make it more modular...

In D50652#1156810, @imp wrote:
In D50652#1156621, @jhb wrote:
In D50652#1156297, @imp wrote:
In D50652#1156295, @jhb wrote:
In D50652#1156269, @imp wrote:

Yea, this kinda break kernels that use nvd. Well, it forces all of cam to be drug in, which may or may not be a big deal.
Better to move to a subr_nvme that both dev/nvme and dev/nvmef use, or something similar. We can chat at bsdcan.

But, those routines can move from CAM to subr_nvme.c, in the future so I guess I'm OK with this.

Kernels that use nvd already have to pull in CAM? Well, nvme.ko at least pulls in nvme_sim.c unconditionally. I guess custom kernels without any SATA or SCSI and only NVMe that are also using nvd instead of nda(4)? How common is that use case?

It's a desired case, and people have talked to me about fixing this bug. Or rather wanting it fixed. And NVMe only-systems are quire popular. But the overhead is small in terms of extra memory, so it's not been a bug high on my list of fixing. This makes it just a little more so. It's a non-default case, and we do the right thing when both are in the kernel, so it's fine. And we've been de-emphasizing nvd, which really is only a win for ultra-high transaction rate systems.

I tried making nvme_all.c nvme | scbus in sys/conf/files and it almost worked except it needed cam_strvis_sbuf for some routines in nvme_all.c that aren't needed by nvme(4). I'm not sure what's better/worse, but we could add DEV_SCBUS to sys/conf/options and then use that #ifdef in nvme_all.c to limit the bits used in an static kernel without scbus? sys/modules/cam/Makefile would need to define it always perhaps. Not sure if that is too ugly.

Yea, that's an interesting notion. My first take is that it might be a bit too ugly since we're moving away from DEV_* device defines where possible... and I wonder how that might interact with the cam.ko module. Maybe we should just leave it alone until we start to do more work with CAM to make it more modular...

Actually, how about nvme_util.c. That code is already shared between nvme(4) and cam.ko today. I can just put the sbuf routines under #ifdef _KERNEL as user land doesn't need them. I will add a commit to move them first then rebase this one on top of that. I think I prefer this to adding a subr_nvme.c under sys/kern.

jhb retitled this revision from nvme: Use routines from CAM to decode opcodes and status codes to nvme: Use routines from nvme_util.c to decode opcodes and status codes.Jun 4 2025, 4:32 PM
jhb edited the summary of this revision. (Show Details)

Drop extraneous includes

This revision now requires review to proceed.Jun 4 2025, 4:36 PM
This revision is now accepted and ready to land.Jun 4 2025, 4:43 PM