Page MenuHomeFreeBSD

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

Authored by wanpengqian_gmail.com on Nov 11 2022, 9:12 AM.
Tags
None
Referenced Files
Unknown Object (File)
Sun, Jan 15, 4:38 PM
Unknown Object (File)
Wed, Jan 11, 5:20 AM
Unknown Object (File)
Mon, Jan 2, 7:56 AM
Unknown Object (File)
Dec 14 2022, 11:31 PM
Unknown Object (File)
Dec 4 2022, 11:12 PM

Details

Reviewers
None
Group Reviewers
bhyve
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

Diff Detail

Repository
rG FreeBSD src repository
Lint
Lint Passed
Unit
No Test Coverage
Build Status
Buildable 48307
Build 45193: arc lint + arc unit

Event Timeline

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.

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.

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.

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.

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.

usr.sbin/bhyve/pci_nvme.c
327

When should the nvme controller create the queues?

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.