* [PATCH v2 0/2] hw/nvme: Add shadow doorbell buffer support
@ 2022-06-15 14:49 Jinhao Fan
2022-06-15 14:49 ` [PATCH v2 1/2] hw/nvme: Implement " Jinhao Fan
2022-06-15 14:49 ` [PATCH v2 2/2] hw/nvme: Add trace events for shadow doorbell buffer Jinhao Fan
0 siblings, 2 replies; 5+ messages in thread
From: Jinhao Fan @ 2022-06-15 14:49 UTC (permalink / raw)
To: qemu-devel; +Cc: its, kbusch, Jinhao Fan
This patch adds shadow doorbell buffer support in NVMe 1.3 to QEMU
NVMe. The Doorbell Buffer Config admin command is implemented for the
guest to enable shadow doobell buffer. When this feature is enabled, each
SQ/CQ is associated with two buffers, i.e., Shadow Doorbell buffer and
EventIdx buffer. According to the Spec, each queue's doorbell register
is only updated when the Shadow Doorbell buffer value changes from being
less than or equal to the value of the corresponding EventIdx buffer
entry to being greater than that value. Therefore, the number of MMIO's
on the doorbell registers is greatly reduced.
This patch is adapted from Huaicheng Li's patch[1] in 2018.
[1] https://patchwork.kernel.org/project/qemu-devel/patch/20180305194906.GA3630@gmail.com/
IOPS comparison with FIO:
iodepth 1 2 4 8
QEMU 25.1k 25.9k 24.5k 24.0k
+dbbuf 29.1k 60.1k 99.8k 82.5k
MMIO's per IO measured by perf-kvm:
iodepth 1 2 4 8
QEMU 2.01 1.99 1.99 1.99
+dbbuf 1.00 0.52 0.27 0.46
The tests are done on Ubuntu 22.04 with 5.15.0-33 kernel with Intel(R)
Xeon(R) Gold 6248R CPU @ 3.00GHz.
QEMU set up:
bin/x86_64-softmmu/qemu-system-x86_64 \
-name "nvme-test" \
-machine accel=kvm \
-cpu host \
-smp 4 \
-m 8G \
-daemonize \
-device virtio-scsi-pci,id=scsi0 \
-device scsi-hd,drive=hd0 \
-drive file=$OSIMGF,if=none,aio=native,cache=none,format=qcow2,id=hd0,snapshot=on \
-drive "id=nvm,if=none,file=null-co://,file.read-zeroes=on,format=raw" \
-device nvme,serial=deadbeef,drive=nvm \
-net user,hostfwd=tcp::8080-:22 \
-net nic,model=virtio
FIO configuration:
[global]
ioengine=libaio
filename=/dev/nvme0n1
thread=1
group_reporting=1
direct=1
verify=0
time_based=1
ramp_time=0
runtime=30
;size=1G
;iodepth=1
rw=randread
bs=4k
[test]
numjobs=1
Changes since v1:
- Add compatibility with hosts that do not use admin queue shadow doorbell
Jinhao Fan (2):
hw/nvme: Implement shadow doorbell buffer support
hw/nvme: Add trace events for shadow doorbell buffer
hw/nvme/ctrl.c | 117 ++++++++++++++++++++++++++++++++++++++++++-
hw/nvme/nvme.h | 8 +++
hw/nvme/trace-events | 5 ++
include/block/nvme.h | 2 +
4 files changed, 131 insertions(+), 1 deletion(-)
--
2.25.1
^ permalink raw reply [flat|nested] 5+ messages in thread
* [PATCH v2 1/2] hw/nvme: Implement shadow doorbell buffer support
2022-06-15 14:49 [PATCH v2 0/2] hw/nvme: Add shadow doorbell buffer support Jinhao Fan
@ 2022-06-15 14:49 ` Jinhao Fan
2022-06-16 10:40 ` Klaus Jensen
2022-06-15 14:49 ` [PATCH v2 2/2] hw/nvme: Add trace events for shadow doorbell buffer Jinhao Fan
1 sibling, 1 reply; 5+ messages in thread
From: Jinhao Fan @ 2022-06-15 14:49 UTC (permalink / raw)
To: qemu-devel; +Cc: its, kbusch, Jinhao Fan
Implement Doorbel Buffer Config command (Section 5.7 in NVMe Spec 1.3)
and Shadow Doorbel buffer & EventIdx buffer handling logic (Section 7.13
in NVMe Spec 1.3). For queues created before the Doorbell Buffer Config
command, the nvme_dbbuf_config function tries to associate each existing
SQ and CQ with its Shadow Doorbel buffer and EventIdx buffer address.
Queues created after the Doorbell Buffer Config command will have the
doorbell buffers associated with them when they are initialized.
In nvme_process_sq and nvme_post_cqe, proactively check for Shadow
Doorbell buffer changes instead of wait for doorbell register changes.
This reduces the number of MMIOs.
In nvme_process_db(), update the shadow doorbell buffer value with
the doorbell register value if it is the admin queue. This is a hack
since hosts like Linux NVMe driver and SPDK do not use shadow
doorbell buffer for the admin queue. Copying the doorbell register
value to the shadow doorbell buffer allows us to support these hosts
as well as spec-compliant hosts that use shadow doorbell buffer for
the admin queue.
Signed-off-by: Jinhao Fan <fanjinhao21s@ict.ac.cn>
---
hw/nvme/ctrl.c | 112 ++++++++++++++++++++++++++++++++++++++++++-
hw/nvme/nvme.h | 8 ++++
include/block/nvme.h | 2 +
3 files changed, 121 insertions(+), 1 deletion(-)
diff --git a/hw/nvme/ctrl.c b/hw/nvme/ctrl.c
index 03760ddeae..7be2e43f52 100644
--- a/hw/nvme/ctrl.c
+++ b/hw/nvme/ctrl.c
@@ -223,6 +223,7 @@ static const uint32_t nvme_cse_acs[256] = {
[NVME_ADM_CMD_GET_FEATURES] = NVME_CMD_EFF_CSUPP,
[NVME_ADM_CMD_ASYNC_EV_REQ] = NVME_CMD_EFF_CSUPP,
[NVME_ADM_CMD_NS_ATTACHMENT] = NVME_CMD_EFF_CSUPP | NVME_CMD_EFF_NIC,
+ [NVME_ADM_CMD_DBBUF_CONFIG] = NVME_CMD_EFF_CSUPP,
[NVME_ADM_CMD_FORMAT_NVM] = NVME_CMD_EFF_CSUPP | NVME_CMD_EFF_LBCC,
};
@@ -1304,6 +1305,12 @@ static inline void nvme_blk_write(BlockBackend *blk, int64_t offset,
}
}
+static void nvme_update_cq_head(NvmeCQueue *cq)
+{
+ pci_dma_read(&cq->ctrl->parent_obj, cq->db_addr, &cq->head,
+ sizeof(cq->head));
+}
+
static void nvme_post_cqes(void *opaque)
{
NvmeCQueue *cq = opaque;
@@ -1316,6 +1323,10 @@ static void nvme_post_cqes(void *opaque)
NvmeSQueue *sq;
hwaddr addr;
+ if (cq->cqid && n->dbbuf_enabled) {
+ nvme_update_cq_head(cq);
+ }
+
if (nvme_cq_full(cq)) {
break;
}
@@ -4237,6 +4248,7 @@ static uint16_t nvme_del_sq(NvmeCtrl *n, NvmeRequest *req)
static void nvme_init_sq(NvmeSQueue *sq, NvmeCtrl *n, uint64_t dma_addr,
uint16_t sqid, uint16_t cqid, uint16_t size)
{
+ uint32_t stride = 4 << NVME_CAP_DSTRD(n->bar.cap);
int i;
NvmeCQueue *cq;
@@ -4256,6 +4268,11 @@ static void nvme_init_sq(NvmeSQueue *sq, NvmeCtrl *n, uint64_t dma_addr,
}
sq->timer = timer_new_ns(QEMU_CLOCK_VIRTUAL, nvme_process_sq, sq);
+ if (n->dbbuf_enabled) {
+ sq->db_addr = n->dbbuf_dbs + 2 * sqid * stride;
+ sq->ei_addr = n->dbbuf_eis + 2 * sqid * stride;
+ }
+
assert(n->cq[cqid]);
cq = n->cq[cqid];
QTAILQ_INSERT_TAIL(&(cq->sq_list), sq, entry);
@@ -4599,6 +4616,7 @@ static void nvme_init_cq(NvmeCQueue *cq, NvmeCtrl *n, uint64_t dma_addr,
uint16_t cqid, uint16_t vector, uint16_t size,
uint16_t irq_enabled)
{
+ uint32_t stride = 4 << NVME_CAP_DSTRD(n->bar.cap);
int ret;
if (msix_enabled(&n->parent_obj)) {
@@ -4615,6 +4633,10 @@ static void nvme_init_cq(NvmeCQueue *cq, NvmeCtrl *n, uint64_t dma_addr,
cq->head = cq->tail = 0;
QTAILQ_INIT(&cq->req_list);
QTAILQ_INIT(&cq->sq_list);
+ if (n->dbbuf_enabled) {
+ cq->db_addr = n->dbbuf_dbs + (2 * cqid + 1) * stride;
+ cq->ei_addr = n->dbbuf_eis + (2 * cqid + 1) * stride;
+ }
n->cq[cqid] = cq;
cq->timer = timer_new_ns(QEMU_CLOCK_VIRTUAL, nvme_post_cqes, cq);
}
@@ -5767,6 +5789,47 @@ out:
return status;
}
+static uint16_t nvme_dbbuf_config(NvmeCtrl *n, const NvmeRequest *req)
+{
+ uint32_t stride = 4 << NVME_CAP_DSTRD(n->bar.cap);
+ uint64_t dbs_addr = le64_to_cpu(req->cmd.dptr.prp1);
+ uint64_t eis_addr = le64_to_cpu(req->cmd.dptr.prp2);
+ int i;
+
+ /* Address should be page aligned */
+ if (dbs_addr & (n->page_size - 1) || eis_addr & (n->page_size - 1)) {
+ return NVME_INVALID_FIELD | NVME_DNR;
+ }
+
+ /* Save shadow buffer base addr for use during queue creation */
+ n->dbbuf_dbs = dbs_addr;
+ n->dbbuf_eis = eis_addr;
+ n->dbbuf_enabled = true;
+
+ for (i = 0; i < n->params.max_ioqpairs + 1; i++) {
+ NvmeSQueue *sq = n->sq[i];
+ NvmeCQueue *cq = n->cq[i];
+
+ if (sq) {
+ /* Submission queue tail pointer location, 2 * QID * stride */
+ sq->db_addr = dbs_addr + 2 * i * stride;
+ sq->ei_addr = eis_addr + 2 * i * stride;
+ pci_dma_write(&n->parent_obj, sq->db_addr, &sq->tail,
+ sizeof(sq->tail));
+ }
+
+ if (cq) {
+ /* Completion queue head pointer location, (2 * QID + 1) * stride */
+ cq->db_addr = dbs_addr + (2 * i + 1) * stride;
+ cq->ei_addr = eis_addr + (2 * i + 1) * stride;
+ pci_dma_write(&n->parent_obj, cq->db_addr, &cq->head,
+ sizeof(cq->head));
+ }
+ }
+
+ return NVME_SUCCESS;
+}
+
static uint16_t nvme_admin_cmd(NvmeCtrl *n, NvmeRequest *req)
{
trace_pci_nvme_admin_cmd(nvme_cid(req), nvme_sqid(req), req->cmd.opcode,
@@ -5809,6 +5872,8 @@ static uint16_t nvme_admin_cmd(NvmeCtrl *n, NvmeRequest *req)
return nvme_aer(n, req);
case NVME_ADM_CMD_NS_ATTACHMENT:
return nvme_ns_attachment(n, req);
+ case NVME_ADM_CMD_DBBUF_CONFIG:
+ return nvme_dbbuf_config(n, req);
case NVME_ADM_CMD_FORMAT_NVM:
return nvme_format(n, req);
default:
@@ -5818,6 +5883,18 @@ static uint16_t nvme_admin_cmd(NvmeCtrl *n, NvmeRequest *req)
return NVME_INVALID_OPCODE | NVME_DNR;
}
+static void nvme_update_sq_eventidx(const NvmeSQueue *sq)
+{
+ pci_dma_write(&sq->ctrl->parent_obj, sq->ei_addr, &sq->tail,
+ sizeof(sq->tail));
+}
+
+static void nvme_update_sq_tail(NvmeSQueue *sq)
+{
+ pci_dma_read(&sq->ctrl->parent_obj, sq->db_addr, &sq->tail,
+ sizeof(sq->tail));
+}
+
static void nvme_process_sq(void *opaque)
{
NvmeSQueue *sq = opaque;
@@ -5829,6 +5906,10 @@ static void nvme_process_sq(void *opaque)
NvmeCmd cmd;
NvmeRequest *req;
+ if (sq->sqid && n->dbbuf_enabled) {
+ nvme_update_sq_tail(sq);
+ }
+
while (!(nvme_sq_empty(sq) || QTAILQ_EMPTY(&sq->req_list))) {
addr = sq->dma_addr + sq->head * n->sqe_size;
if (nvme_addr_read(n, addr, (void *)&cmd, sizeof(cmd))) {
@@ -5852,6 +5933,11 @@ static void nvme_process_sq(void *opaque)
req->status = status;
nvme_enqueue_req_completion(cq, req);
}
+
+ if (sq->sqid && n->dbbuf_enabled) {
+ nvme_update_sq_eventidx(sq);
+ nvme_update_sq_tail(sq);
+ }
}
}
@@ -5889,6 +5975,9 @@ static void nvme_ctrl_reset(NvmeCtrl *n)
n->aer_queued = 0;
n->outstanding_aers = 0;
n->qs_created = false;
+ n->dbbuf_dbs = 0;
+ n->dbbuf_eis = 0;
+ n->dbbuf_enabled = false;
}
static void nvme_ctrl_shutdown(NvmeCtrl *n)
@@ -6398,6 +6487,10 @@ static void nvme_process_db(NvmeCtrl *n, hwaddr addr, int val)
start_sqs = nvme_cq_full(cq) ? 1 : 0;
cq->head = new_head;
+ if (!qid && n->dbbuf_enabled) {
+ pci_dma_write(&n->parent_obj, cq->db_addr, &cq->head,
+ sizeof(cq->head));
+ }
if (start_sqs) {
NvmeSQueue *sq;
QTAILQ_FOREACH(sq, &cq->sq_list, entry) {
@@ -6455,6 +6548,23 @@ static void nvme_process_db(NvmeCtrl *n, hwaddr addr, int val)
trace_pci_nvme_mmio_doorbell_sq(sq->sqid, new_tail);
sq->tail = new_tail;
+ if (!qid && n->dbbuf_enabled) {
+ /*
+ * The spec states "the host shall also update the controller's
+ * corresponding doorbell property to match the value of that entry
+ * in the Shadow Doorbell buffer."
+ *
+ * Since this context is currently a VM trap, we can safely enforce
+ * the requirement from the device side in case the host is
+ * misbehaving.
+ *
+ * Note, we shouldn't have to do this, but various drivers
+ * including ones that run on Linux, are not updating Admin Queues,
+ * so we can't trust reading it for an appropriate sq tail.
+ */
+ pci_dma_write(&n->parent_obj, sq->db_addr, &sq->tail,
+ sizeof(sq->tail));
+ }
timer_mod(sq->timer, qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL) + 500);
}
}
@@ -6733,7 +6843,7 @@ static void nvme_init_ctrl(NvmeCtrl *n, PCIDevice *pci_dev)
id->mdts = n->params.mdts;
id->ver = cpu_to_le32(NVME_SPEC_VER);
- id->oacs = cpu_to_le16(NVME_OACS_NS_MGMT | NVME_OACS_FORMAT);
+ id->oacs = cpu_to_le16(NVME_OACS_NS_MGMT | NVME_OACS_FORMAT | NVME_OACS_DBBUF);
id->cntrltype = 0x1;
/*
diff --git a/hw/nvme/nvme.h b/hw/nvme/nvme.h
index 6773819325..4452e4b1bf 100644
--- a/hw/nvme/nvme.h
+++ b/hw/nvme/nvme.h
@@ -334,6 +334,7 @@ static inline const char *nvme_adm_opc_str(uint8_t opc)
case NVME_ADM_CMD_GET_FEATURES: return "NVME_ADM_CMD_GET_FEATURES";
case NVME_ADM_CMD_ASYNC_EV_REQ: return "NVME_ADM_CMD_ASYNC_EV_REQ";
case NVME_ADM_CMD_NS_ATTACHMENT: return "NVME_ADM_CMD_NS_ATTACHMENT";
+ case NVME_ADM_CMD_DBBUF_CONFIG: return "NVME_ADM_CMD_DBBUF_CONFIG";
case NVME_ADM_CMD_FORMAT_NVM: return "NVME_ADM_CMD_FORMAT_NVM";
default: return "NVME_ADM_CMD_UNKNOWN";
}
@@ -365,6 +366,8 @@ typedef struct NvmeSQueue {
uint32_t tail;
uint32_t size;
uint64_t dma_addr;
+ uint64_t db_addr;
+ uint64_t ei_addr;
QEMUTimer *timer;
NvmeRequest *io_req;
QTAILQ_HEAD(, NvmeRequest) req_list;
@@ -382,6 +385,8 @@ typedef struct NvmeCQueue {
uint32_t vector;
uint32_t size;
uint64_t dma_addr;
+ uint64_t db_addr;
+ uint64_t ei_addr;
QEMUTimer *timer;
QTAILQ_HEAD(, NvmeSQueue) sq_list;
QTAILQ_HEAD(, NvmeRequest) req_list;
@@ -432,6 +437,9 @@ typedef struct NvmeCtrl {
uint64_t starttime_ms;
uint16_t temperature;
uint8_t smart_critical_warning;
+ uint64_t dbbuf_dbs;
+ uint64_t dbbuf_eis;
+ bool dbbuf_enabled;
struct {
MemoryRegion mem;
diff --git a/include/block/nvme.h b/include/block/nvme.h
index 3737351cc8..5b522d7b0e 100644
--- a/include/block/nvme.h
+++ b/include/block/nvme.h
@@ -595,6 +595,7 @@ enum NvmeAdminCommands {
NVME_ADM_CMD_ACTIVATE_FW = 0x10,
NVME_ADM_CMD_DOWNLOAD_FW = 0x11,
NVME_ADM_CMD_NS_ATTACHMENT = 0x15,
+ NVME_ADM_CMD_DBBUF_CONFIG = 0x7c,
NVME_ADM_CMD_FORMAT_NVM = 0x80,
NVME_ADM_CMD_SECURITY_SEND = 0x81,
NVME_ADM_CMD_SECURITY_RECV = 0x82,
@@ -1134,6 +1135,7 @@ enum NvmeIdCtrlOacs {
NVME_OACS_FORMAT = 1 << 1,
NVME_OACS_FW = 1 << 2,
NVME_OACS_NS_MGMT = 1 << 3,
+ NVME_OACS_DBBUF = 1 << 8,
};
enum NvmeIdCtrlOncs {
--
2.25.1
^ permalink raw reply related [flat|nested] 5+ messages in thread
* [PATCH v2 2/2] hw/nvme: Add trace events for shadow doorbell buffer
2022-06-15 14:49 [PATCH v2 0/2] hw/nvme: Add shadow doorbell buffer support Jinhao Fan
2022-06-15 14:49 ` [PATCH v2 1/2] hw/nvme: Implement " Jinhao Fan
@ 2022-06-15 14:49 ` Jinhao Fan
1 sibling, 0 replies; 5+ messages in thread
From: Jinhao Fan @ 2022-06-15 14:49 UTC (permalink / raw)
To: qemu-devel; +Cc: its, kbusch, Jinhao Fan
When shadow doorbell buffer is enabled, doorbell registers are lazily
updated. The actual queue head and tail pointers are stored in Shadow
Doorbell buffers.
Add trace events for updates on the Shadow Doorbell buffers and EventIdx
buffers. Also add trace event for the Doorbell Buffer Config command.
Signed-off-by: Jinhao Fan <fanjinhao21s@ict.ac.cn>
---
hw/nvme/ctrl.c | 5 +++++
hw/nvme/trace-events | 5 +++++
2 files changed, 10 insertions(+)
diff --git a/hw/nvme/ctrl.c b/hw/nvme/ctrl.c
index 7be2e43f52..77fa79143d 100644
--- a/hw/nvme/ctrl.c
+++ b/hw/nvme/ctrl.c
@@ -1309,6 +1309,7 @@ static void nvme_update_cq_head(NvmeCQueue *cq)
{
pci_dma_read(&cq->ctrl->parent_obj, cq->db_addr, &cq->head,
sizeof(cq->head));
+ trace_pci_nvme_shadow_doorbell_cq(cq->cqid, cq->head);
}
static void nvme_post_cqes(void *opaque)
@@ -5827,6 +5828,8 @@ static uint16_t nvme_dbbuf_config(NvmeCtrl *n, const NvmeRequest *req)
}
}
+ trace_pci_nvme_dbbuf_config(dbs_addr, eis_addr);
+
return NVME_SUCCESS;
}
@@ -5887,12 +5890,14 @@ static void nvme_update_sq_eventidx(const NvmeSQueue *sq)
{
pci_dma_write(&sq->ctrl->parent_obj, sq->ei_addr, &sq->tail,
sizeof(sq->tail));
+ trace_pci_nvme_eventidx_sq(sq->sqid, sq->tail);
}
static void nvme_update_sq_tail(NvmeSQueue *sq)
{
pci_dma_read(&sq->ctrl->parent_obj, sq->db_addr, &sq->tail,
sizeof(sq->tail));
+ trace_pci_nvme_shadow_doorbell_sq(sq->sqid, sq->tail);
}
static void nvme_process_sq(void *opaque)
diff --git a/hw/nvme/trace-events b/hw/nvme/trace-events
index ff1b458969..00ee42f475 100644
--- a/hw/nvme/trace-events
+++ b/hw/nvme/trace-events
@@ -3,6 +3,7 @@ pci_nvme_irq_msix(uint32_t vector) "raising MSI-X IRQ vector %u"
pci_nvme_irq_pin(void) "pulsing IRQ pin"
pci_nvme_irq_masked(void) "IRQ is masked"
pci_nvme_dma_read(uint64_t prp1, uint64_t prp2) "DMA read, prp1=0x%"PRIx64" prp2=0x%"PRIx64""
+pci_nvme_dbbuf_config(uint64_t dbs_addr, uint64_t eis_addr) "dbs_addr=0x%"PRIx64" eis_addr=0x%"PRIx64""
pci_nvme_map_addr(uint64_t addr, uint64_t len) "addr 0x%"PRIx64" len %"PRIu64""
pci_nvme_map_addr_cmb(uint64_t addr, uint64_t len) "addr 0x%"PRIx64" len %"PRIu64""
pci_nvme_map_prp(uint64_t trans_len, uint32_t len, uint64_t prp1, uint64_t prp2, int num_prps) "trans_len %"PRIu64" len %"PRIu32" prp1 0x%"PRIx64" prp2 0x%"PRIx64" num_prps %d"
@@ -81,6 +82,8 @@ pci_nvme_enqueue_event_noqueue(int queued) "queued %d"
pci_nvme_enqueue_event_masked(uint8_t typ) "type 0x%"PRIx8""
pci_nvme_no_outstanding_aers(void) "ignoring event; no outstanding AERs"
pci_nvme_enqueue_req_completion(uint16_t cid, uint16_t cqid, uint32_t dw0, uint32_t dw1, uint16_t status) "cid %"PRIu16" cqid %"PRIu16" dw0 0x%"PRIx32" dw1 0x%"PRIx32" status 0x%"PRIx16""
+pci_nvme_eventidx_cq(uint16_t cqid, uint16_t new_eventidx) "cqid %"PRIu16" new_eventidx %"PRIu16""
+pci_nvme_eventidx_sq(uint16_t sqid, uint16_t new_eventidx) "sqid %"PRIu16" new_eventidx %"PRIu16""
pci_nvme_mmio_read(uint64_t addr, unsigned size) "addr 0x%"PRIx64" size %d"
pci_nvme_mmio_write(uint64_t addr, uint64_t data, unsigned size) "addr 0x%"PRIx64" data 0x%"PRIx64" size %d"
pci_nvme_mmio_doorbell_cq(uint16_t cqid, uint16_t new_head) "cqid %"PRIu16" new_head %"PRIu16""
@@ -97,6 +100,8 @@ pci_nvme_mmio_start_success(void) "setting controller enable bit succeeded"
pci_nvme_mmio_stopped(void) "cleared controller enable bit"
pci_nvme_mmio_shutdown_set(void) "shutdown bit set"
pci_nvme_mmio_shutdown_cleared(void) "shutdown bit cleared"
+pci_nvme_shadow_doorbell_cq(uint16_t cqid, uint16_t new_shadow_doorbell) "cqid %"PRIu16" new_shadow_doorbell %"PRIu16""
+pci_nvme_shadow_doorbell_sq(uint16_t sqid, uint16_t new_shadow_doorbell) "sqid %"PRIu16" new_shadow_doorbell %"PRIu16""
pci_nvme_open_zone(uint64_t slba, uint32_t zone_idx, int all) "open zone, slba=%"PRIu64", idx=%"PRIu32", all=%"PRIi32""
pci_nvme_close_zone(uint64_t slba, uint32_t zone_idx, int all) "close zone, slba=%"PRIu64", idx=%"PRIu32", all=%"PRIi32""
pci_nvme_finish_zone(uint64_t slba, uint32_t zone_idx, int all) "finish zone, slba=%"PRIu64", idx=%"PRIu32", all=%"PRIi32""
--
2.25.1
^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH v2 1/2] hw/nvme: Implement shadow doorbell buffer support
2022-06-15 14:49 ` [PATCH v2 1/2] hw/nvme: Implement " Jinhao Fan
@ 2022-06-16 10:40 ` Klaus Jensen
2022-06-16 11:37 ` Jinhao Fan
0 siblings, 1 reply; 5+ messages in thread
From: Klaus Jensen @ 2022-06-16 10:40 UTC (permalink / raw)
To: Jinhao Fan; +Cc: qemu-devel, kbusch
[-- Attachment #1: Type: text/plain, Size: 12980 bytes --]
On Jun 15 22:49, Jinhao Fan wrote:
> Implement Doorbel Buffer Config command (Section 5.7 in NVMe Spec 1.3)
> and Shadow Doorbel buffer & EventIdx buffer handling logic (Section 7.13
> in NVMe Spec 1.3). For queues created before the Doorbell Buffer Config
> command, the nvme_dbbuf_config function tries to associate each existing
> SQ and CQ with its Shadow Doorbel buffer and EventIdx buffer address.
> Queues created after the Doorbell Buffer Config command will have the
> doorbell buffers associated with them when they are initialized.
>
> In nvme_process_sq and nvme_post_cqe, proactively check for Shadow
> Doorbell buffer changes instead of wait for doorbell register changes.
> This reduces the number of MMIOs.
>
> In nvme_process_db(), update the shadow doorbell buffer value with
> the doorbell register value if it is the admin queue. This is a hack
> since hosts like Linux NVMe driver and SPDK do not use shadow
> doorbell buffer for the admin queue. Copying the doorbell register
> value to the shadow doorbell buffer allows us to support these hosts
> as well as spec-compliant hosts that use shadow doorbell buffer for
> the admin queue.
>
> Signed-off-by: Jinhao Fan <fanjinhao21s@ict.ac.cn>
> ---
> hw/nvme/ctrl.c | 112 ++++++++++++++++++++++++++++++++++++++++++-
> hw/nvme/nvme.h | 8 ++++
> include/block/nvme.h | 2 +
> 3 files changed, 121 insertions(+), 1 deletion(-)
>
> diff --git a/hw/nvme/ctrl.c b/hw/nvme/ctrl.c
> index 03760ddeae..7be2e43f52 100644
> --- a/hw/nvme/ctrl.c
> +++ b/hw/nvme/ctrl.c
> @@ -223,6 +223,7 @@ static const uint32_t nvme_cse_acs[256] = {
> [NVME_ADM_CMD_GET_FEATURES] = NVME_CMD_EFF_CSUPP,
> [NVME_ADM_CMD_ASYNC_EV_REQ] = NVME_CMD_EFF_CSUPP,
> [NVME_ADM_CMD_NS_ATTACHMENT] = NVME_CMD_EFF_CSUPP | NVME_CMD_EFF_NIC,
> + [NVME_ADM_CMD_DBBUF_CONFIG] = NVME_CMD_EFF_CSUPP,
> [NVME_ADM_CMD_FORMAT_NVM] = NVME_CMD_EFF_CSUPP | NVME_CMD_EFF_LBCC,
> };
>
> @@ -1304,6 +1305,12 @@ static inline void nvme_blk_write(BlockBackend *blk, int64_t offset,
> }
> }
>
> +static void nvme_update_cq_head(NvmeCQueue *cq)
> +{
> + pci_dma_read(&cq->ctrl->parent_obj, cq->db_addr, &cq->head,
> + sizeof(cq->head));
> +}
> +
> static void nvme_post_cqes(void *opaque)
> {
> NvmeCQueue *cq = opaque;
> @@ -1316,6 +1323,10 @@ static void nvme_post_cqes(void *opaque)
> NvmeSQueue *sq;
> hwaddr addr;
>
> + if (cq->cqid && n->dbbuf_enabled) {
> + nvme_update_cq_head(cq);
> + }
> +
This wont work for drivers that *do* rely on updating the buffer for
admin queues, so we should read it regardless of the value of the queue
id (since we are now updating it through the "Keith Hack^TM").
> if (nvme_cq_full(cq)) {
> break;
> }
> @@ -4237,6 +4248,7 @@ static uint16_t nvme_del_sq(NvmeCtrl *n, NvmeRequest *req)
> static void nvme_init_sq(NvmeSQueue *sq, NvmeCtrl *n, uint64_t dma_addr,
> uint16_t sqid, uint16_t cqid, uint16_t size)
> {
> + uint32_t stride = 4 << NVME_CAP_DSTRD(n->bar.cap);
You need to load the little endian value with ldq_le_p(&n->bar.cap).
Sorry, didn't catch this in v1.
> int i;
> NvmeCQueue *cq;
>
> @@ -4256,6 +4268,11 @@ static void nvme_init_sq(NvmeSQueue *sq, NvmeCtrl *n, uint64_t dma_addr,
> }
> sq->timer = timer_new_ns(QEMU_CLOCK_VIRTUAL, nvme_process_sq, sq);
>
> + if (n->dbbuf_enabled) {
> + sq->db_addr = n->dbbuf_dbs + 2 * sqid * stride;
> + sq->ei_addr = n->dbbuf_eis + 2 * sqid * stride;
> + }
> +
> assert(n->cq[cqid]);
> cq = n->cq[cqid];
> QTAILQ_INSERT_TAIL(&(cq->sq_list), sq, entry);
> @@ -4599,6 +4616,7 @@ static void nvme_init_cq(NvmeCQueue *cq, NvmeCtrl *n, uint64_t dma_addr,
> uint16_t cqid, uint16_t vector, uint16_t size,
> uint16_t irq_enabled)
> {
> + uint32_t stride = 4 << NVME_CAP_DSTRD(n->bar.cap);
Same as above.
> int ret;
>
> if (msix_enabled(&n->parent_obj)) {
> @@ -4615,6 +4633,10 @@ static void nvme_init_cq(NvmeCQueue *cq, NvmeCtrl *n, uint64_t dma_addr,
> cq->head = cq->tail = 0;
> QTAILQ_INIT(&cq->req_list);
> QTAILQ_INIT(&cq->sq_list);
> + if (n->dbbuf_enabled) {
> + cq->db_addr = n->dbbuf_dbs + (2 * cqid + 1) * stride;
> + cq->ei_addr = n->dbbuf_eis + (2 * cqid + 1) * stride;
> + }
> n->cq[cqid] = cq;
> cq->timer = timer_new_ns(QEMU_CLOCK_VIRTUAL, nvme_post_cqes, cq);
> }
> @@ -5767,6 +5789,47 @@ out:
> return status;
> }
>
> +static uint16_t nvme_dbbuf_config(NvmeCtrl *n, const NvmeRequest *req)
> +{
> + uint32_t stride = 4 << NVME_CAP_DSTRD(n->bar.cap);
> + uint64_t dbs_addr = le64_to_cpu(req->cmd.dptr.prp1);
> + uint64_t eis_addr = le64_to_cpu(req->cmd.dptr.prp2);
> + int i;
> +
> + /* Address should be page aligned */
> + if (dbs_addr & (n->page_size - 1) || eis_addr & (n->page_size - 1)) {
> + return NVME_INVALID_FIELD | NVME_DNR;
> + }
> +
> + /* Save shadow buffer base addr for use during queue creation */
> + n->dbbuf_dbs = dbs_addr;
> + n->dbbuf_eis = eis_addr;
> + n->dbbuf_enabled = true;
> +
> + for (i = 0; i < n->params.max_ioqpairs + 1; i++) {
> + NvmeSQueue *sq = n->sq[i];
> + NvmeCQueue *cq = n->cq[i];
> +
> + if (sq) {
> + /* Submission queue tail pointer location, 2 * QID * stride */
> + sq->db_addr = dbs_addr + 2 * i * stride;
> + sq->ei_addr = eis_addr + 2 * i * stride;
> + pci_dma_write(&n->parent_obj, sq->db_addr, &sq->tail,
> + sizeof(sq->tail));
> + }
> +
> + if (cq) {
> + /* Completion queue head pointer location, (2 * QID + 1) * stride */
> + cq->db_addr = dbs_addr + (2 * i + 1) * stride;
> + cq->ei_addr = eis_addr + (2 * i + 1) * stride;
> + pci_dma_write(&n->parent_obj, cq->db_addr, &cq->head,
> + sizeof(cq->head));
> + }
> + }
> +
> + return NVME_SUCCESS;
> +}
> +
> static uint16_t nvme_admin_cmd(NvmeCtrl *n, NvmeRequest *req)
> {
> trace_pci_nvme_admin_cmd(nvme_cid(req), nvme_sqid(req), req->cmd.opcode,
> @@ -5809,6 +5872,8 @@ static uint16_t nvme_admin_cmd(NvmeCtrl *n, NvmeRequest *req)
> return nvme_aer(n, req);
> case NVME_ADM_CMD_NS_ATTACHMENT:
> return nvme_ns_attachment(n, req);
> + case NVME_ADM_CMD_DBBUF_CONFIG:
> + return nvme_dbbuf_config(n, req);
> case NVME_ADM_CMD_FORMAT_NVM:
> return nvme_format(n, req);
> default:
> @@ -5818,6 +5883,18 @@ static uint16_t nvme_admin_cmd(NvmeCtrl *n, NvmeRequest *req)
> return NVME_INVALID_OPCODE | NVME_DNR;
> }
>
> +static void nvme_update_sq_eventidx(const NvmeSQueue *sq)
> +{
> + pci_dma_write(&sq->ctrl->parent_obj, sq->ei_addr, &sq->tail,
> + sizeof(sq->tail));
> +}
> +
> +static void nvme_update_sq_tail(NvmeSQueue *sq)
> +{
> + pci_dma_read(&sq->ctrl->parent_obj, sq->db_addr, &sq->tail,
> + sizeof(sq->tail));
> +}
> +
> static void nvme_process_sq(void *opaque)
> {
> NvmeSQueue *sq = opaque;
> @@ -5829,6 +5906,10 @@ static void nvme_process_sq(void *opaque)
> NvmeCmd cmd;
> NvmeRequest *req;
>
> + if (sq->sqid && n->dbbuf_enabled) {
> + nvme_update_sq_tail(sq);
> + }
Remove the check on queue id as above with the completion queue.
> +
> while (!(nvme_sq_empty(sq) || QTAILQ_EMPTY(&sq->req_list))) {
> addr = sq->dma_addr + sq->head * n->sqe_size;
> if (nvme_addr_read(n, addr, (void *)&cmd, sizeof(cmd))) {
> @@ -5852,6 +5933,11 @@ static void nvme_process_sq(void *opaque)
> req->status = status;
> nvme_enqueue_req_completion(cq, req);
> }
> +
> + if (sq->sqid && n->dbbuf_enabled) {
> + nvme_update_sq_eventidx(sq);
> + nvme_update_sq_tail(sq);
> + }
> }
> }
>
> @@ -5889,6 +5975,9 @@ static void nvme_ctrl_reset(NvmeCtrl *n)
> n->aer_queued = 0;
> n->outstanding_aers = 0;
> n->qs_created = false;
> + n->dbbuf_dbs = 0;
> + n->dbbuf_eis = 0;
> + n->dbbuf_enabled = false;
> }
>
> static void nvme_ctrl_shutdown(NvmeCtrl *n)
> @@ -6398,6 +6487,10 @@ static void nvme_process_db(NvmeCtrl *n, hwaddr addr, int val)
>
> start_sqs = nvme_cq_full(cq) ? 1 : 0;
> cq->head = new_head;
> + if (!qid && n->dbbuf_enabled) {
> + pci_dma_write(&n->parent_obj, cq->db_addr, &cq->head,
> + sizeof(cq->head));
> + }
> if (start_sqs) {
> NvmeSQueue *sq;
> QTAILQ_FOREACH(sq, &cq->sq_list, entry) {
> @@ -6455,6 +6548,23 @@ static void nvme_process_db(NvmeCtrl *n, hwaddr addr, int val)
> trace_pci_nvme_mmio_doorbell_sq(sq->sqid, new_tail);
>
> sq->tail = new_tail;
> + if (!qid && n->dbbuf_enabled) {
> + /*
> + * The spec states "the host shall also update the controller's
> + * corresponding doorbell property to match the value of that entry
> + * in the Shadow Doorbell buffer."
> + *
> + * Since this context is currently a VM trap, we can safely enforce
> + * the requirement from the device side in case the host is
> + * misbehaving.
> + *
> + * Note, we shouldn't have to do this, but various drivers
> + * including ones that run on Linux, are not updating Admin Queues,
> + * so we can't trust reading it for an appropriate sq tail.
> + */
> + pci_dma_write(&n->parent_obj, sq->db_addr, &sq->tail,
> + sizeof(sq->tail));
> + }
> timer_mod(sq->timer, qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL) + 500);
> }
> }
> @@ -6733,7 +6843,7 @@ static void nvme_init_ctrl(NvmeCtrl *n, PCIDevice *pci_dev)
>
> id->mdts = n->params.mdts;
> id->ver = cpu_to_le32(NVME_SPEC_VER);
> - id->oacs = cpu_to_le16(NVME_OACS_NS_MGMT | NVME_OACS_FORMAT);
> + id->oacs = cpu_to_le16(NVME_OACS_NS_MGMT | NVME_OACS_FORMAT | NVME_OACS_DBBUF);
> id->cntrltype = 0x1;
>
> /*
> diff --git a/hw/nvme/nvme.h b/hw/nvme/nvme.h
> index 6773819325..4452e4b1bf 100644
> --- a/hw/nvme/nvme.h
> +++ b/hw/nvme/nvme.h
> @@ -334,6 +334,7 @@ static inline const char *nvme_adm_opc_str(uint8_t opc)
> case NVME_ADM_CMD_GET_FEATURES: return "NVME_ADM_CMD_GET_FEATURES";
> case NVME_ADM_CMD_ASYNC_EV_REQ: return "NVME_ADM_CMD_ASYNC_EV_REQ";
> case NVME_ADM_CMD_NS_ATTACHMENT: return "NVME_ADM_CMD_NS_ATTACHMENT";
> + case NVME_ADM_CMD_DBBUF_CONFIG: return "NVME_ADM_CMD_DBBUF_CONFIG";
> case NVME_ADM_CMD_FORMAT_NVM: return "NVME_ADM_CMD_FORMAT_NVM";
> default: return "NVME_ADM_CMD_UNKNOWN";
> }
> @@ -365,6 +366,8 @@ typedef struct NvmeSQueue {
> uint32_t tail;
> uint32_t size;
> uint64_t dma_addr;
> + uint64_t db_addr;
> + uint64_t ei_addr;
> QEMUTimer *timer;
> NvmeRequest *io_req;
> QTAILQ_HEAD(, NvmeRequest) req_list;
> @@ -382,6 +385,8 @@ typedef struct NvmeCQueue {
> uint32_t vector;
> uint32_t size;
> uint64_t dma_addr;
> + uint64_t db_addr;
> + uint64_t ei_addr;
> QEMUTimer *timer;
> QTAILQ_HEAD(, NvmeSQueue) sq_list;
> QTAILQ_HEAD(, NvmeRequest) req_list;
> @@ -432,6 +437,9 @@ typedef struct NvmeCtrl {
> uint64_t starttime_ms;
> uint16_t temperature;
> uint8_t smart_critical_warning;
> + uint64_t dbbuf_dbs;
> + uint64_t dbbuf_eis;
> + bool dbbuf_enabled;
>
> struct {
> MemoryRegion mem;
> diff --git a/include/block/nvme.h b/include/block/nvme.h
> index 3737351cc8..5b522d7b0e 100644
> --- a/include/block/nvme.h
> +++ b/include/block/nvme.h
> @@ -595,6 +595,7 @@ enum NvmeAdminCommands {
> NVME_ADM_CMD_ACTIVATE_FW = 0x10,
> NVME_ADM_CMD_DOWNLOAD_FW = 0x11,
> NVME_ADM_CMD_NS_ATTACHMENT = 0x15,
> + NVME_ADM_CMD_DBBUF_CONFIG = 0x7c,
> NVME_ADM_CMD_FORMAT_NVM = 0x80,
> NVME_ADM_CMD_SECURITY_SEND = 0x81,
> NVME_ADM_CMD_SECURITY_RECV = 0x82,
> @@ -1134,6 +1135,7 @@ enum NvmeIdCtrlOacs {
> NVME_OACS_FORMAT = 1 << 1,
> NVME_OACS_FW = 1 << 2,
> NVME_OACS_NS_MGMT = 1 << 3,
> + NVME_OACS_DBBUF = 1 << 8,
> };
>
> enum NvmeIdCtrlOncs {
> --
> 2.25.1
>
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH v2 1/2] hw/nvme: Implement shadow doorbell buffer support
2022-06-16 10:40 ` Klaus Jensen
@ 2022-06-16 11:37 ` Jinhao Fan
0 siblings, 0 replies; 5+ messages in thread
From: Jinhao Fan @ 2022-06-16 11:37 UTC (permalink / raw)
To: Klaus Jensen; +Cc: qemu-devel, Keith Busch
> On Jun 16, 2022, at 6:40 PM, Klaus Jensen <its@irrelevant.dk> wrote:
>
> This wont work for drivers that *do* rely on updating the buffer for
> admin queues, so we should read it regardless of the value of the queue
> id (since we are now updating it through the "Keith Hack^TM").
Sure. Sorry I forgot to update this.
>
>> if (nvme_cq_full(cq)) {
>> break;
>> }
>> @@ -4237,6 +4248,7 @@ static uint16_t nvme_del_sq(NvmeCtrl *n, NvmeRequest *req)
>> static void nvme_init_sq(NvmeSQueue *sq, NvmeCtrl *n, uint64_t dma_addr,
>> uint16_t sqid, uint16_t cqid, uint16_t size)
>> {
>> + uint32_t stride = 4 << NVME_CAP_DSTRD(n->bar.cap);
>
> You need to load the little endian value with ldq_le_p(&n->bar.cap).
> Sorry, didn't catch this in v1.
Thanks for pointing this out. Will send v3 soon.
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2022-06-16 11:52 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2022-06-15 14:49 [PATCH v2 0/2] hw/nvme: Add shadow doorbell buffer support Jinhao Fan
2022-06-15 14:49 ` [PATCH v2 1/2] hw/nvme: Implement " Jinhao Fan
2022-06-16 10:40 ` Klaus Jensen
2022-06-16 11:37 ` Jinhao Fan
2022-06-15 14:49 ` [PATCH v2 2/2] hw/nvme: Add trace events for shadow doorbell buffer Jinhao Fan
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.