bhyve: fix the number of queue feature setting timing for nvme controller.

[wanpengqian_gmail.com]

Summary

This is a discussion revision. The fix is not clear yet.

NVMe Specification writes:

This Feature indicates the number of queues that the host requests for this controller. This feature shall
only be issued during initialization prior to creation of any I/O Submission and/or Completion Queues. If a
Set Features command is issued for this feature after creation of any I/O Submission and/or I/O Completion
Queues, then the Set Features command shall fail with status code of Command Sequence Error

current implementation did not obey this description.

Test Plan

under consideration

[corvink]

You've just renamed the variable and inverted the logic from is_set to is_setable. That's unneccessary. Please keep the old name.

The only thing which really changed is that the reset of the variable is done in nvme_init_queues instead of nvme_reset.

So, the question is: Do we have to reset the variable on nvme_reset. I don't know. Your quote of the specification isn't clear on that.

[wanpengqian_gmail.com]

address reviewer feedback.

[wanpengqian_gmail.com]

In D37356#848742, @corvink wrote:

You've just renamed the variable and inverted the logic from is_set to is_setable. That's unneccessary. Please keep the old name.

The only thing which really changed is that the reset of the variable is done in nvme_init_queues instead of nvme_reset.

So, the question is: Do we have to reset the variable on nvme_reset. I don't know. Your quote of the specification isn't clear on that.

Here is what the specification says.

The controller shall not change the value allocated between resets.

I think once this value is set, it will remain a during reset.

and I think it is very hard to call this feature. since the setting time window is so critical.

This Feature indicates the number of queues that the host requests for this controller. This feature shall
only be issued during initialization prior to creation of any I/O Submission and/or Completion Queues.

current nvme code, the queue is init before this feature is init, so it is hard for hosts to call this feature successfully.
I don't know how to fix it.

[corvink]

Btw: qemu resets num_q_is_set on ctrl reset.

See https://github.com/qemu/qemu/blob/f21f1cfeb94b94ce044726856c291bed9391e3a4/hw/nvme/ctrl.c#L6373

usr.sbin/bhyve/pci_nvme.c

327

Unneccessary change in my opinion. The old comment fits.

479

That's the wrong place for setting it.

The host allocates some buffers which can be used as queues. The real allocation of queues is done in nvme_feature_num_queues. So, the old place is correct.

1805–1806

Your new comment sounds like an informational message not like a warning which it is. The old comment is better, leave it as it is.

[wanpengqian_gmail.com]

usr.sbin/bhyve/pci_nvme.c

327

the specification says

This feature shall only be issued during initialization prior to creation of any I/O Submission and/or Completion Queues

It did not say this value can only be set once. Before the controller creates the queues, this feature can be set many times, in my understanding. so we want a flag that the queue can be set or not. That is why I renamed this value at first. num_q_is_setable.

This flag indicates whether the queue can be set or not. Even if the host did not call this feature, after the controller allocates the queue with default values, it cannot be set later.

[corvink]

usr.sbin/bhyve/pci_nvme.c

327

When should the nvme controller create the queues?

[wanpengqian_gmail.com]

usr.sbin/bhyve/pci_nvme.c

327

Existing code, the queue is init before feature init, so this feature is not setable at all if implemented by spec.

pci_nvme_init_queues(sc, sc->max_queues, sc->max_queues);
/*
* Controller data depends on Namespace data so initialize Namespace
* data first.
*/
pci_nvme_init_nsdata(sc, &sc->nsdata, 1, &sc->nvstore);
pci_nvme_init_ctrldata(sc);
pci_nvme_init_logpages(sc);
pci_nvme_init_features(sc);

An ideal fix is the init queue after the host does the first I/O operation. Before queues are created, it can be set by this feature. I don't know if this approach is OK or not, maybe chuck can give some ideas.