Page MenuHomeFreeBSD
Differential D37356 Diff 112941 usr.sbin/bhyve/pci_nvme.c

Changeset View

Standalone View

View Options

usr.sbin/bhyve/pci_nvme.c

Show First 20 Lines • Show All 318 Lines • ▼ Show 20 Linesstruct pci_nvme_softc {
struct nvme_ns_list ns_log; struct nvme_ns_list ns_log;
struct pci_nvme_blockstore nvstore; struct pci_nvme_blockstore nvstore;
uint16_t max_qentries; /* max entries per queue */ uint16_t max_qentries; /* max entries per queue */
uint32_t max_queues; /* max number of IO SQ's or CQ's */ uint32_t max_queues; /* max number of IO SQ's or CQ's */
uint32_t num_cqueues; uint32_t num_cqueues;
uint32_t num_squeues; uint32_t num_squeues;
bool num_q_is_set; /* Has host set Number of Queues */ bool num_q_is_set; /* Number of Queues is set */
corvinkUnsubmitted
Not Done
Inline Actions

Unneccessary change in my opinion. The old comment fits.

corvink: Unneccessary change in my opinion. The old comment fits.
wanpengqian_gmail.comAuthorUnsubmitted
Done
Inline Actions

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.

wanpengqian_gmail.com: the specification says > This feature shall only be issued during initialization prior to…
corvinkUnsubmitted
Not Done
Inline Actions

When should the nvme controller create the queues?

corvink: When should the nvme controller create the queues?
wanpengqian_gmail.comAuthorUnsubmitted
Done
Inline Actions

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.

wanpengqian_gmail.com: Existing code, the queue is init before feature init, so this feature is not setable at all if…
struct pci_nvme_ioreq *ioreqs; struct pci_nvme_ioreq *ioreqs;
STAILQ_HEAD(, pci_nvme_ioreq) ioreqs_free; /* free list of ioreqs */ STAILQ_HEAD(, pci_nvme_ioreq) ioreqs_free; /* free list of ioreqs */
uint32_t pending_ios; uint32_t pending_ios;
uint32_t ioslots; uint32_t ioslots;
sem_t iosemlock; sem_t iosemlock;
/* /*
▲ Show 20 Lines • Show All 135 Lines • ▼ Show 20 Linespci_nvme_init_queues(struct pci_nvme_softc *sc, uint32_t nsq, uint32_t ncq)
* Allocate and initialize the Submission Queues * Allocate and initialize the Submission Queues
*/ */
if (nsq > NVME_QUEUES) { if (nsq > NVME_QUEUES) {
WPRINTF("%s: clamping number of SQ from %u to %u", WPRINTF("%s: clamping number of SQ from %u to %u",
__func__, nsq, NVME_QUEUES); __func__, nsq, NVME_QUEUES);
nsq = NVME_QUEUES; nsq = NVME_QUEUES;
} }
sc->num_q_is_set = true;
corvinkUnsubmitted
Not Done
Inline Actions

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.

corvink: That's the wrong place for setting it. The host allocates some buffers which can be used as…
sc->num_squeues = nsq; sc->num_squeues = nsq;
sc->submit_queues = calloc(sc->num_squeues + 1, sc->submit_queues = calloc(sc->num_squeues + 1,
sizeof(struct nvme_submission_queue)); sizeof(struct nvme_submission_queue));
if (sc->submit_queues == NULL) { if (sc->submit_queues == NULL) {
WPRINTF("%s: SQ allocation failed", __func__); WPRINTF("%s: SQ allocation failed", __func__);
sc->num_squeues = 0; sc->num_squeues = 0;
} else { } else {
▲ Show 20 Lines • Show All 585 Lines • ▼ Show 20 Linespci_nvme_reset_locked(struct pci_nvme_softc *sc)
for (i = 0; i < sc->num_cqueues + 1; i++) { for (i = 0; i < sc->num_cqueues + 1; i++) {
sc->compl_queues[i].qbase = NULL; sc->compl_queues[i].qbase = NULL;
sc->compl_queues[i].size = 0; sc->compl_queues[i].size = 0;
sc->compl_queues[i].tail = 0; sc->compl_queues[i].tail = 0;
sc->compl_queues[i].head = 0; sc->compl_queues[i].head = 0;
} }
sc->num_q_is_set = false;
pci_nvme_aer_destroy(sc); pci_nvme_aer_destroy(sc);
pci_nvme_aen_destroy(sc); pci_nvme_aen_destroy(sc);
/* /*
* Clear CSTS.RDY last to prevent the host from enabling Controller * Clear CSTS.RDY last to prevent the host from enabling Controller
* before cleanup completes * before cleanup completes
*/ */
sc->regs.csts = 0; sc->regs.csts = 0;
▲ Show 20 Lines • Show All 56 Lines • ▼ Show 20 Lines if (sc->compl_queues[0].qbase == NULL) {
sc->regs.csts |= NVME_CSTS_CFS; sc->regs.csts |= NVME_CSTS_CFS;
return (-1); return (-1);
} }
sc->compl_queues[0].intr_en = NVME_CQ_INTEN; sc->compl_queues[0].intr_en = NVME_CQ_INTEN;
DPRINTF("%s mapping Admin-CQ guest 0x%lx, host: %p", DPRINTF("%s mapping Admin-CQ guest 0x%lx, host: %p",
__func__, sc->regs.acq, sc->compl_queues[0].qbase); __func__, sc->regs.acq, sc->compl_queues[0].qbase);
sc->num_q_is_set = false;
return (0); return (0);
} }
static int static int
nvme_prp_memcpy(struct vmctx *ctx, uint64_t prp1, uint64_t prp2, uint8_t *b, nvme_prp_memcpy(struct vmctx *ctx, uint64_t prp1, uint64_t prp2, uint8_t *b,
size_t len, enum nvme_copy_dir dir) size_t len, enum nvme_copy_dir dir)
{ {
uint8_t *p; uint8_t *p;
▲ Show 20 Lines • Show All 635 Lines • ▼ Show 20 Lines
nvme_feature_num_queues(struct pci_nvme_softc *sc, nvme_feature_num_queues(struct pci_nvme_softc *sc,
struct nvme_feature_obj *feat __unused, struct nvme_feature_obj *feat __unused,
struct nvme_command *command, struct nvme_command *command,
struct nvme_completion *compl) struct nvme_completion *compl)
{ {
uint16_t nqr; /* Number of Queues Requested */ uint16_t nqr; /* Number of Queues Requested */
if (sc->num_q_is_set) { if (sc->num_q_is_set) {
WPRINTF("%s: Number of Queues already set", __func__); WPRINTF("%s: Number of Queues has been initialized", __func__);
corvinkUnsubmitted
Not Done
Inline Actions

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.

corvink: Your new comment sounds like an informational message not like a warning which it is. The old…
pci_nvme_status_genc(&compl->status, pci_nvme_status_genc(&compl->status,
NVME_SC_COMMAND_SEQUENCE_ERROR); NVME_SC_COMMAND_SEQUENCE_ERROR);
return; return;
} }
nqr = command->cdw11 & 0xFFFF; nqr = command->cdw11 & 0xFFFF;
if (nqr == 0xffff) { if (nqr == 0xffff) {
WPRINTF("%s: Illegal NSQR value %#x", __func__, nqr); WPRINTF("%s: Illegal NSQR value %#x", __func__, nqr);
Show All 20 Lines if (sc->num_cqueues > sc->max_queues) {
DPRINTF("NCQR=%u is greater than max %u", sc->num_cqueues, DPRINTF("NCQR=%u is greater than max %u", sc->num_cqueues,
sc->max_queues); sc->max_queues);
sc->num_cqueues = sc->max_queues; sc->num_cqueues = sc->max_queues;
} }
/* Patch the command value which will be saved on callback's return */ /* Patch the command value which will be saved on callback's return */
command->cdw11 = NVME_FEATURE_NUM_QUEUES(sc); command->cdw11 = NVME_FEATURE_NUM_QUEUES(sc);
compl->cdw0 = NVME_FEATURE_NUM_QUEUES(sc); compl->cdw0 = NVME_FEATURE_NUM_QUEUES(sc);
sc->num_q_is_set = true;
} }
static int static int
nvme_opc_set_features(struct pci_nvme_softc *sc, struct nvme_command *command, nvme_opc_set_features(struct pci_nvme_softc *sc, struct nvme_command *command,
struct nvme_completion *compl) struct nvme_completion *compl)
{ {
struct nvme_feature_obj *feat; struct nvme_feature_obj *feat;
uint32_t nsid = command->nsid; uint32_t nsid = command->nsid;
▲ Show 20 Lines • Show All 1,522 Lines • Show Last 20 Lines