bhyve: return FEATURE_NOT_CHANGEABLE for unimplemented feature of NVMe controller

[wanpengqian_gmail.com]

Summary

Set Feature is a feature specified function.
Currently only some features have the set procedure.
for features that are not handled by the controller, we should return a FEATURE_NOT_CHANGEABLE error message.

Test Plan

Fire a un-handle set feature command to check the response.
it should return a
NVMe status: FEATURE_NOT_CHANGEABLE: The Feature Identifier is not able to be changed(0x10e)
error message within Linux guest.

[chuck]

usr.sbin/bhyve/pci_nvme.c

1927

Doesn't the removal of this break the NVMe requirement that values given in Set Features should be returned in CDW0 of Get Features?

[wanpengqian_gmail.com]

Update code as requested.

[wanpengqian_gmail.com]

usr.sbin/bhyve/pci_nvme.c

1927

You are right. I will keep the change to as small as needed.

[corvink]

LGTM @manu

[manu]

In D32802#847821, @corvink wrote:

LGTM @manu

Approved.

[chuck]

usr.sbin/bhyve/pci_nvme.c

1888

This behavior is not spec compliant. Please review section 5.27.1

[wanpengqian_gmail.com]

usr.sbin/bhyve/pci_nvme.c

1888

Yes, you are correct.
I read section 5.21.1 (not 5.27.1, right?)

https://nvmexpress.org/wp-content/uploads/NVM-Express-1_4c-2021.06.28-Ratified.pdf

If the controller does not support a changeable value for a Feature (e.g., the Feature is not changeable),
and a Set Features command for that Feature is processed, then if that command specifies a Feature value
that:
• is not the same as the existing value for that Feature, then the controller shall abort that command
with a status code of Feature Not Changeable; and
• is the same as the existing value for that Feature, then the controller may:
o complete that command successfully; or
o abort that command with a status code of Feature Not Changeable

I am abandoning this patch.

[corvink]

usr.sbin/bhyve/pci_nvme.c

1888

Hmm, sry don't get it. How I understand the spec: If we there's no feat->set, we do not support a changable value for the feature. So, we should abort the command and return Feature Not Changable.

[wanpengqian_gmail.com]

usr.sbin/bhyve/pci_nvme.c

1888

At first, I understood what you just said. after reading the spec again. I found it means: If the user provides a value that is different to the existing value, and the controller does not support change , it should return Feature Not Changeable.
If a feature does not implement feat->set, I think we should return another error message instead of Feature Not Changeable.

[corvink]

usr.sbin/bhyve/pci_nvme.c

1888

If the user isn't permitted to change the value, you can always return Feature Not Changable. There's a special case: If the value is the same as the existing one, the controller is allowed to succeed but it doesn't have to.
Maybe @chuck could specify in more details why this isn't spec compliant.

Btw: I've checked qemu. It has the same behaviour. If qemu has no feat->set it returns Feature Not Changable:
https://github.com/qemu/qemu/blob/f21f1cfeb94b94ce044726856c291bed9391e3a4/hw/nvme/ctrl.c#L5595-L5596

[chuck]

usr.sbin/bhyve/pci_nvme.c

1888

Yes, you are correct.
I read section 5.21.1 (not 5.27.1, right?)

https://nvmexpress.org/wp-content/uploads/NVM-Express-1_4c-2021.06.28-Ratified.pdf

Apologies, it is section 5.27.1 in the 2.0 spec, but yes, you found the correct section for the 1.4 spec.

1888

The specification has a "shall" that makes this non-compliant (emphasis mine):

If the controller does not support a changeable value for a Feature (e.g., the Feature is not changeable),
and a Set Features command for that Feature is processed, then if that command specifies a Feature value
that:

• is not the same as the existing value for that Feature, then the controller shall abort that command with a status code of Feature Not Changeable; and

The code would also need to check the supplied vs. existing value to be correct.

[corvink]

usr.sbin/bhyve/pci_nvme.c

1888

Read the next statement:

• is the same as the existing value for that Feature, then the controller may:
  • complete that command successfully; or
  • abort that command with a status code of Feature Not Changeable

So, always returning Feature Not Changable should be compliant.

[chuck]

I see what you are saying. Unfortunately, I can't figure out how to remove the "revisions requested" status.

[wanpengqian_gmail.com]

All right, I think for unimplemented features, always return FEATURE NOT CHANGEABLE is compliant to the spec. and for implemented features, it is up to the implementation.so I am reclaiming this patch. thanks.