Details

Reviewers

chuck
jhb
imp
corvink

Group Reviewers

bhyve

Commits

rG5ebc003a2882: bhyve: abort and return FEATURE_NOT_SAVEABLE while set feature with a save flag…
rG8ab99dbea167: bhyve: abort and return FEATURE_NOT_SAVEABLE while set feature with a save flag…

Summary

Currently bhyve's NVMe controller cannot save feature values cross reboot.
It should return a FEATURE_NOT_SAVEABLE error when the command specifies a save flag.

Quote from NVMe specification, page 205:

https://nvmexpress.org/wp-content/uploads/NVM-Express-1_4-2019.06.10-Ratified.pdf

If the Feature Identifier specified in the Set Features command is not saveable by the controller and the controller receives a Set Features command with the Save bit set to one, then the command shall be aborted with a status of Feature Identifier Not Saveable.

Test Plan

After apply this patch, under linux guest, fire command
nvme set-feature -f 4 -v 0x157 -s /dev/nvme0
it return an error message of
NVMe status: FEATURE_NOT_SAVEABLE: The Feature Identifier specified does not support a saveable value(0x10d)

Diff Detail

Repository

rG FreeBSD src repository

Lint

Lint Not Applicable

Unit

Tests Not Applicable

Event Timeline

wanpengqian_gmail.com created this revision.Nov 1 2021, 4:23 AM

Herald added a reviewer: bhyve. · View Herald TranscriptNov 1 2021, 4:23 AM

Herald added subscribers: dab, bcran, rgrimes. · View Herald Transcript

wanpengqian_gmail.com requested review of this revision.Nov 1 2021, 4:23 AM

Harbormaster completed remote builds in B42479: Diff 97783.Nov 1 2021, 4:23 AM

use function-like macro.

Harbormaster completed remote builds in B42665: Diff 98220.Nov 9 2021, 12:31 AM

Any comments?

wanpengqian_gmail.com planned changes to this revision.Aug 26 2022, 12:08 AM

wanpengqian_gmail.com requested review of this revision.Oct 19 2022, 5:48 AM

wanpengqian_gmail.com retitled this revision from bhyve: abort and return FEATURE_NOT_SAVEABLE while set feature with save flag. to bhyve: abort and return FEATURE_NOT_SAVEABLE while set feature with a save flag for NVMe controller..

wanpengqian_gmail.com edited the summary of this revision. (Show Details)

wanpengqian_gmail.com edited the test plan for this revision. (Show Details)

wanpengqian_gmail.com removed a reviewer: grehan.

catch up with the latest source.

Harbormaster completed remote builds in B47896: Diff 111956.Oct 19 2022, 5:56 AM

wanpengqian_gmail.com edited the summary of this revision. (Show Details)Oct 19 2022, 6:00 AM

wanpengqian_gmail.com edited the test plan for this revision. (Show Details)

LGTM @manu

usr.sbin/bhyve/pci_nvme.c
122–132	Not sure if we should add these defines to `sys/dev/nvme/nvme.h`

This revision is now accepted and ready to land.Nov 9 2022, 12:35 PM

In D32767#847825, @corvink wrote:

LGTM @manu

Approved.

wanpengqian_gmail.com added inline comments.Nov 9 2022, 1:55 PM

usr.sbin/bhyve/pci_nvme.c
122–132	Yes, it is better move to nvme.h I will update this patch soon. also make a macro to make it more simple.

chuck accepted this revision as: chuck.Nov 9 2022, 3:59 PM

chuck added inline comments.

usr.sbin/bhyve/pci_nvme.c
1863	Nit but I'd drop this debug message as it doesn't add much value

imp added inline comments.Nov 9 2022, 11:50 PM

usr.sbin/bhyve/pci_nvme.c
122–132	thanks! I was going to suggest this as well.

Address reviews' feedback.

This revision now requires review to proceed.Nov 10 2022, 12:47 AM

Harbormaster completed remote builds in B48250: Diff 112822.Nov 10 2022, 12:47 AM

Address reviewers' feedback.

usr.sbin/bhyve/pci_nvme.c
1854	Inspired by Chuck's NVMEB macro, I create an NVMEV macro that helps extract values. And we don't need to do the following defines now. save a lot of code work and potential typo #define NVME_FEAT_SET_FID(x) \ ((x) >> NVME_FEAT_SET_FID_SHIFT & NVME_FEAT_SET_FID_MASK)