[PATCH v5 00/15] block/nvme: Various cleanups required to use multiple queues

[PATCH v5 00/15] block/nvme: Various cleanups required to use multiple queues

[Philippe Mathieu-Daudé]

Hi Kevin,

This series is mostly code rearrangement (cleanups) to be
able to split the hardware code from the block driver code,
to be able to use multiple queues on the same hardware, or
multiple block drivers on the same hardware.

All this series is reviewed.

Since v4:
- added 'block/nvme: Use an array of EventNotifier' patch

Since v3:
- renamed QUEUE_INDEX_{ADMIN/IO} -> INDEX{ADMIN/IO}
- added stefanha tags

Since v2:
- addressed stefanha review comments
- added 4 trivial patches (to simplify the last one)
- register IRQ notifier for each queuepair (admin and io)

Since v1:
- rebased
- use SCALE_MS definition
- added Stefan's R-b
- addressed Stefan's review comments
  - use union { NvmeIdCtrl / NvmeIdNs }
  - move irq_notifier to NVMeQueuePair
  - removed patches depending on "a tracable hardware stateo
    object instead of BDRVNVMeState".

Phil.

Philippe Mathieu-Daudé (15):
  block/nvme: Replace magic value by SCALE_MS definition
  block/nvme: Avoid further processing if trace event not enabled
  block/nvme: Let nvme_create_queue_pair() fail gracefully
  block/nvme: Define INDEX macros to ease code review
  block/nvme: Improve error message when IO queue creation failed
  block/nvme: Use common error path in nvme_add_io_queue()
  block/nvme: Rename local variable
  block/nvme: Use union of NvmeIdCtrl / NvmeIdNs structures
  block/nvme: Replace qemu_try_blockalign0 by qemu_try_blockalign/memset
  block/nvme: Replace qemu_try_blockalign(bs) by
    qemu_try_memalign(pg_sz)
  block/nvme: Simplify nvme_init_queue() arguments
  block/nvme: Replace BDRV_POLL_WHILE by AIO_WAIT_WHILE
  block/nvme: Simplify nvme_create_queue_pair() arguments
  block/nvme: Extract nvme_poll_queue()
  block/nvme: Use an array of EventNotifier

 block/nvme.c | 211 ++++++++++++++++++++++++++++++---------------------
 1 file changed, 125 insertions(+), 86 deletions(-)

-- 
2.26.2

[PATCH v5 01/15] block/nvme: Replace magic value by SCALE_MS definition

[Philippe Mathieu-Daudé]

Use self-explicit SCALE_MS definition instead of magic value.

Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>
Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
---
 block/nvme.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/block/nvme.c b/block/nvme.c
index 374e2689157..2f5e3c2adfa 100644
--- a/block/nvme.c
+++ b/block/nvme.c
@@ -715,7 +715,7 @@ static int nvme_init(BlockDriverState *bs, const char *device, int namespace,
     /* Reset device to get a clean state. */
     s->regs->cc = cpu_to_le32(le32_to_cpu(s->regs->cc) & 0xFE);
     /* Wait for CSTS.RDY = 0. */
-    deadline = qemu_clock_get_ns(QEMU_CLOCK_REALTIME) + timeout_ms * 1000000ULL;
+    deadline = qemu_clock_get_ns(QEMU_CLOCK_REALTIME) + timeout_ms * SCALE_MS;
     while (le32_to_cpu(s->regs->csts) & 0x1) {
         if (qemu_clock_get_ns(QEMU_CLOCK_REALTIME) > deadline) {
             error_setg(errp, "Timeout while waiting for device to reset (%"
-- 
2.26.2

[PATCH v5 02/15] block/nvme: Avoid further processing if trace event not enabled

[Philippe Mathieu-Daudé]

Avoid further processing if TRACE_NVME_SUBMIT_COMMAND_RAW is
not enabled. This is an untested intend of performance optimization.

Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>
Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
---
 block/nvme.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/block/nvme.c b/block/nvme.c
index 2f5e3c2adfa..8c30a5fee28 100644
--- a/block/nvme.c
+++ b/block/nvme.c
@@ -441,6 +441,9 @@ static void nvme_trace_command(const NvmeCmd *cmd)
 {
     int i;
 
+    if (!trace_event_get_state_backends(TRACE_NVME_SUBMIT_COMMAND_RAW)) {
+        return;
+    }
     for (i = 0; i < 8; ++i) {
         uint8_t *cmdp = (uint8_t *)cmd + i * 8;
         trace_nvme_submit_command_raw(cmdp[0], cmdp[1], cmdp[2], cmdp[3],
-- 
2.26.2

[PATCH v5 03/15] block/nvme: Let nvme_create_queue_pair() fail gracefully

[Philippe Mathieu-Daudé]

As nvme_create_queue_pair() is allowed to fail, replace the
alloc() calls by try_alloc() to avoid aborting QEMU.

Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>
Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
---
 block/nvme.c | 12 ++++++++++--
 1 file changed, 10 insertions(+), 2 deletions(-)

diff --git a/block/nvme.c b/block/nvme.c
index 8c30a5fee28..e1893b4e792 100644
--- a/block/nvme.c
+++ b/block/nvme.c
@@ -213,14 +213,22 @@ static NVMeQueuePair *nvme_create_queue_pair(BlockDriverState *bs,
     int i, r;
     BDRVNVMeState *s = bs->opaque;
     Error *local_err = NULL;
-    NVMeQueuePair *q = g_new0(NVMeQueuePair, 1);
+    NVMeQueuePair *q;
     uint64_t prp_list_iova;
 
+    q = g_try_new0(NVMeQueuePair, 1);
+    if (!q) {
+        return NULL;
+    }
+    q->prp_list_pages = qemu_try_blockalign0(bs,
+                                          s->page_size * NVME_QUEUE_SIZE);
+    if (!q->prp_list_pages) {
+        goto fail;
+    }
     qemu_mutex_init(&q->lock);
     q->s = s;
     q->index = idx;
     qemu_co_queue_init(&q->free_req_queue);
-    q->prp_list_pages = qemu_blockalign0(bs, s->page_size * NVME_NUM_REQS);
     q->completion_bh = aio_bh_new(bdrv_get_aio_context(bs),
                                   nvme_process_completion_bh, q);
     r = qemu_vfio_dma_map(s->vfio, q->prp_list_pages,
-- 
2.26.2

[PATCH v5 04/15] block/nvme: Define INDEX macros to ease code review

[Philippe Mathieu-Daudé]

Use definitions instead of '0' or '1' indexes. Also this will
be useful when using multi-queues later.

Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>
Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
---
 block/nvme.c | 33 +++++++++++++++++++--------------
 1 file changed, 19 insertions(+), 14 deletions(-)

diff --git a/block/nvme.c b/block/nvme.c
index e1893b4e792..003809fbd83 100644
--- a/block/nvme.c
+++ b/block/nvme.c
@@ -103,6 +103,9 @@ typedef volatile struct {
 
 QEMU_BUILD_BUG_ON(offsetof(NVMeRegs, doorbells) != 0x1000);
 
+#define INDEX_ADMIN     0
+#define INDEX_IO(n)     (1 + n)
+
 struct BDRVNVMeState {
     AioContext *aio_context;
     QEMUVFIOState *vfio;
@@ -531,7 +534,7 @@ static void nvme_identify(BlockDriverState *bs, int namespace, Error **errp)
     }
     cmd.prp1 = cpu_to_le64(iova);
 
-    if (nvme_cmd_sync(bs, s->queues[0], &cmd)) {
+    if (nvme_cmd_sync(bs, s->queues[INDEX_ADMIN], &cmd)) {
         error_setg(errp, "Failed to identify controller");
         goto out;
     }
@@ -555,7 +558,7 @@ static void nvme_identify(BlockDriverState *bs, int namespace, Error **errp)
 
     cmd.cdw10 = 0;
     cmd.nsid = cpu_to_le32(namespace);
-    if (nvme_cmd_sync(bs, s->queues[0], &cmd)) {
+    if (nvme_cmd_sync(bs, s->queues[INDEX_ADMIN], &cmd)) {
         error_setg(errp, "Failed to identify namespace");
         goto out;
     }
@@ -644,7 +647,7 @@ static bool nvme_add_io_queue(BlockDriverState *bs, Error **errp)
         .cdw10 = cpu_to_le32(((queue_size - 1) << 16) | (n & 0xFFFF)),
         .cdw11 = cpu_to_le32(0x3),
     };
-    if (nvme_cmd_sync(bs, s->queues[0], &cmd)) {
+    if (nvme_cmd_sync(bs, s->queues[INDEX_ADMIN], &cmd)) {
         error_setg(errp, "Failed to create io queue [%d]", n);
         nvme_free_queue_pair(q);
         return false;
@@ -655,7 +658,7 @@ static bool nvme_add_io_queue(BlockDriverState *bs, Error **errp)
         .cdw10 = cpu_to_le32(((queue_size - 1) << 16) | (n & 0xFFFF)),
         .cdw11 = cpu_to_le32(0x1 | (n << 16)),
     };
-    if (nvme_cmd_sync(bs, s->queues[0], &cmd)) {
+    if (nvme_cmd_sync(bs, s->queues[INDEX_ADMIN], &cmd)) {
         error_setg(errp, "Failed to create io queue [%d]", n);
         nvme_free_queue_pair(q);
         return false;
@@ -739,16 +742,18 @@ static int nvme_init(BlockDriverState *bs, const char *device, int namespace,
 
     /* Set up admin queue. */
     s->queues = g_new(NVMeQueuePair *, 1);
-    s->queues[0] = nvme_create_queue_pair(bs, 0, NVME_QUEUE_SIZE, errp);
-    if (!s->queues[0]) {
+    s->queues[INDEX_ADMIN] = nvme_create_queue_pair(bs, 0,
+                                                          NVME_QUEUE_SIZE,
+                                                          errp);
+    if (!s->queues[INDEX_ADMIN]) {
         ret = -EINVAL;
         goto out;
     }
     s->nr_queues = 1;
     QEMU_BUILD_BUG_ON(NVME_QUEUE_SIZE & 0xF000);
     s->regs->aqa = cpu_to_le32((NVME_QUEUE_SIZE << 16) | NVME_QUEUE_SIZE);
-    s->regs->asq = cpu_to_le64(s->queues[0]->sq.iova);
-    s->regs->acq = cpu_to_le64(s->queues[0]->cq.iova);
+    s->regs->asq = cpu_to_le64(s->queues[INDEX_ADMIN]->sq.iova);
+    s->regs->acq = cpu_to_le64(s->queues[INDEX_ADMIN]->cq.iova);
 
     /* After setting up all control registers we can enable device now. */
     s->regs->cc = cpu_to_le32((ctz32(NVME_CQ_ENTRY_BYTES) << 20) |
@@ -839,7 +844,7 @@ static int nvme_enable_disable_write_cache(BlockDriverState *bs, bool enable,
         .cdw11 = cpu_to_le32(enable ? 0x01 : 0x00),
     };
 
-    ret = nvme_cmd_sync(bs, s->queues[0], &cmd);
+    ret = nvme_cmd_sync(bs, s->queues[INDEX_ADMIN], &cmd);
     if (ret) {
         error_setg(errp, "Failed to configure NVMe write cache");
     }
@@ -1056,7 +1061,7 @@ static coroutine_fn int nvme_co_prw_aligned(BlockDriverState *bs,
 {
     int r;
     BDRVNVMeState *s = bs->opaque;
-    NVMeQueuePair *ioq = s->queues[1];
+    NVMeQueuePair *ioq = s->queues[INDEX_IO(0)];
     NVMeRequest *req;
 
     uint32_t cdw12 = (((bytes >> s->blkshift) - 1) & 0xFFFF) |
@@ -1171,7 +1176,7 @@ static coroutine_fn int nvme_co_pwritev(BlockDriverState *bs,
 static coroutine_fn int nvme_co_flush(BlockDriverState *bs)
 {
     BDRVNVMeState *s = bs->opaque;
-    NVMeQueuePair *ioq = s->queues[1];
+    NVMeQueuePair *ioq = s->queues[INDEX_IO(0)];
     NVMeRequest *req;
     NvmeCmd cmd = {
         .opcode = NVME_CMD_FLUSH,
@@ -1202,7 +1207,7 @@ static coroutine_fn int nvme_co_pwrite_zeroes(BlockDriverState *bs,
                                               BdrvRequestFlags flags)
 {
     BDRVNVMeState *s = bs->opaque;
-    NVMeQueuePair *ioq = s->queues[1];
+    NVMeQueuePair *ioq = s->queues[INDEX_IO(0)];
     NVMeRequest *req;
 
     uint32_t cdw12 = ((bytes >> s->blkshift) - 1) & 0xFFFF;
@@ -1255,7 +1260,7 @@ static int coroutine_fn nvme_co_pdiscard(BlockDriverState *bs,
                                          int bytes)
 {
     BDRVNVMeState *s = bs->opaque;
-    NVMeQueuePair *ioq = s->queues[1];
+    NVMeQueuePair *ioq = s->queues[INDEX_IO(0)];
     NVMeRequest *req;
     NvmeDsmRange *buf;
     QEMUIOVector local_qiov;
@@ -1398,7 +1403,7 @@ static void nvme_aio_unplug(BlockDriverState *bs)
     BDRVNVMeState *s = bs->opaque;
     assert(s->plugged);
     s->plugged = false;
-    for (i = 1; i < s->nr_queues; i++) {
+    for (i = INDEX_IO(0); i < s->nr_queues; i++) {
         NVMeQueuePair *q = s->queues[i];
         qemu_mutex_lock(&q->lock);
         nvme_kick(q);
-- 
2.26.2

[PATCH v5 05/15] block/nvme: Improve error message when IO queue creation failed

[Philippe Mathieu-Daudé]

Do not use the same error message for different failures.
Display a different error whether it is the CQ or the SQ.

Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>
Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
---
 block/nvme.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/block/nvme.c b/block/nvme.c
index 003809fbd83..53448b7d230 100644
--- a/block/nvme.c
+++ b/block/nvme.c
@@ -648,7 +648,7 @@ static bool nvme_add_io_queue(BlockDriverState *bs, Error **errp)
         .cdw11 = cpu_to_le32(0x3),
     };
     if (nvme_cmd_sync(bs, s->queues[INDEX_ADMIN], &cmd)) {
-        error_setg(errp, "Failed to create io queue [%d]", n);
+        error_setg(errp, "Failed to create CQ io queue [%d]", n);
         nvme_free_queue_pair(q);
         return false;
     }
@@ -659,7 +659,7 @@ static bool nvme_add_io_queue(BlockDriverState *bs, Error **errp)
         .cdw11 = cpu_to_le32(0x1 | (n << 16)),
     };
     if (nvme_cmd_sync(bs, s->queues[INDEX_ADMIN], &cmd)) {
-        error_setg(errp, "Failed to create io queue [%d]", n);
+        error_setg(errp, "Failed to create SQ io queue [%d]", n);
         nvme_free_queue_pair(q);
         return false;
     }
-- 
2.26.2

[PATCH v5 06/15] block/nvme: Use common error path in nvme_add_io_queue()

[Philippe Mathieu-Daudé]

Rearrange nvme_add_io_queue() by using a common error path.
This will be proven useful in few commits where we add IRQ
notification to the IO queues.

Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>
Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
---
 block/nvme.c | 9 +++++----
 1 file changed, 5 insertions(+), 4 deletions(-)

diff --git a/block/nvme.c b/block/nvme.c
index 53448b7d230..3101f1ad55d 100644
--- a/block/nvme.c
+++ b/block/nvme.c
@@ -649,8 +649,7 @@ static bool nvme_add_io_queue(BlockDriverState *bs, Error **errp)
     };
     if (nvme_cmd_sync(bs, s->queues[INDEX_ADMIN], &cmd)) {
         error_setg(errp, "Failed to create CQ io queue [%d]", n);
-        nvme_free_queue_pair(q);
-        return false;
+        goto out_error;
     }
     cmd = (NvmeCmd) {
         .opcode = NVME_ADM_CMD_CREATE_SQ,
@@ -660,13 +659,15 @@ static bool nvme_add_io_queue(BlockDriverState *bs, Error **errp)
     };
     if (nvme_cmd_sync(bs, s->queues[INDEX_ADMIN], &cmd)) {
         error_setg(errp, "Failed to create SQ io queue [%d]", n);
-        nvme_free_queue_pair(q);
-        return false;
+        goto out_error;
     }
     s->queues = g_renew(NVMeQueuePair *, s->queues, n + 1);
     s->queues[n] = q;
     s->nr_queues++;
     return true;
+out_error:
+    nvme_free_queue_pair(q);
+    return false;
 }
 
 static bool nvme_poll_cb(void *opaque)
-- 
2.26.2

[PATCH v5 07/15] block/nvme: Rename local variable

[Philippe Mathieu-Daudé]

We are going to modify the code in the next commit. Renaming
the 'resp' variable to 'id' first makes the next commit easier
to review. No logical changes.

Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>
Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
---
 block/nvme.c | 19 +++++++++----------
 1 file changed, 9 insertions(+), 10 deletions(-)

diff --git a/block/nvme.c b/block/nvme.c
index 3101f1ad55d..99822d9fd36 100644
--- a/block/nvme.c
+++ b/block/nvme.c
@@ -510,8 +510,8 @@ static void nvme_identify(BlockDriverState *bs, int namespace, Error **errp)
     BDRVNVMeState *s = bs->opaque;
     NvmeIdCtrl *idctrl;
     NvmeIdNs *idns;
+    uint8_t *id;
     NvmeLBAF *lbaf;
-    uint8_t *resp;
     uint16_t oncs;
     int r;
     uint64_t iova;
@@ -520,14 +520,14 @@ static void nvme_identify(BlockDriverState *bs, int namespace, Error **errp)
         .cdw10 = cpu_to_le32(0x1),
     };
 
-    resp = qemu_try_blockalign0(bs, sizeof(NvmeIdCtrl));
-    if (!resp) {
+    id = qemu_try_blockalign0(bs, sizeof(NvmeIdCtrl));
+    if (!id) {
         error_setg(errp, "Cannot allocate buffer for identify response");
         goto out;
     }
-    idctrl = (NvmeIdCtrl *)resp;
-    idns = (NvmeIdNs *)resp;
-    r = qemu_vfio_dma_map(s->vfio, resp, sizeof(NvmeIdCtrl), true, &iova);
+    idctrl = (NvmeIdCtrl *)id;
+    idns = (NvmeIdNs *)id;
+    r = qemu_vfio_dma_map(s->vfio, id, sizeof(NvmeIdCtrl), true, &iova);
     if (r) {
         error_setg(errp, "Cannot map buffer for DMA");
         goto out;
@@ -554,8 +554,7 @@ static void nvme_identify(BlockDriverState *bs, int namespace, Error **errp)
     s->supports_write_zeroes = !!(oncs & NVME_ONCS_WRITE_ZEROS);
     s->supports_discard = !!(oncs & NVME_ONCS_DSM);
 
-    memset(resp, 0, 4096);
-
+    memset(id, 0, 4096);
     cmd.cdw10 = 0;
     cmd.nsid = cpu_to_le32(namespace);
     if (nvme_cmd_sync(bs, s->queues[INDEX_ADMIN], &cmd)) {
@@ -587,8 +586,8 @@ static void nvme_identify(BlockDriverState *bs, int namespace, Error **errp)
 
     s->blkshift = lbaf->ds;
 out:
-    qemu_vfio_dma_unmap(s->vfio, resp);
-    qemu_vfree(resp);
+    qemu_vfio_dma_unmap(s->vfio, id);
+    qemu_vfree(id);
 }
 
 static bool nvme_poll_queues(BDRVNVMeState *s)
-- 
2.26.2

[PATCH v5 08/15] block/nvme: Use union of NvmeIdCtrl / NvmeIdNs structures

[Philippe Mathieu-Daudé]

We allocate an unique chunk of memory then use it for two
different structures. By using an union, we make it clear
the data is overlapping (and we can remove the casts).

Suggested-by: Stefan Hajnoczi <stefanha@redhat.com>
Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>
Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
---
 block/nvme.c | 31 +++++++++++++++----------------
 1 file changed, 15 insertions(+), 16 deletions(-)

diff --git a/block/nvme.c b/block/nvme.c
index 99822d9fd36..2bd1935f951 100644
--- a/block/nvme.c
+++ b/block/nvme.c
@@ -508,9 +508,10 @@ static int nvme_cmd_sync(BlockDriverState *bs, NVMeQueuePair *q,
 static void nvme_identify(BlockDriverState *bs, int namespace, Error **errp)
 {
     BDRVNVMeState *s = bs->opaque;
-    NvmeIdCtrl *idctrl;
-    NvmeIdNs *idns;
-    uint8_t *id;
+    union {
+        NvmeIdCtrl ctrl;
+        NvmeIdNs ns;
+    } *id;
     NvmeLBAF *lbaf;
     uint16_t oncs;
     int r;
@@ -520,14 +521,12 @@ static void nvme_identify(BlockDriverState *bs, int namespace, Error **errp)
         .cdw10 = cpu_to_le32(0x1),
     };
 
-    id = qemu_try_blockalign0(bs, sizeof(NvmeIdCtrl));
+    id = qemu_try_blockalign0(bs, sizeof(*id));
     if (!id) {
         error_setg(errp, "Cannot allocate buffer for identify response");
         goto out;
     }
-    idctrl = (NvmeIdCtrl *)id;
-    idns = (NvmeIdNs *)id;
-    r = qemu_vfio_dma_map(s->vfio, id, sizeof(NvmeIdCtrl), true, &iova);
+    r = qemu_vfio_dma_map(s->vfio, id, sizeof(*id), true, &iova);
     if (r) {
         error_setg(errp, "Cannot map buffer for DMA");
         goto out;
@@ -539,22 +538,22 @@ static void nvme_identify(BlockDriverState *bs, int namespace, Error **errp)
         goto out;
     }
 
-    if (le32_to_cpu(idctrl->nn) < namespace) {
+    if (le32_to_cpu(id->ctrl.nn) < namespace) {
         error_setg(errp, "Invalid namespace");
         goto out;
     }
-    s->write_cache_supported = le32_to_cpu(idctrl->vwc) & 0x1;
-    s->max_transfer = (idctrl->mdts ? 1 << idctrl->mdts : 0) * s->page_size;
+    s->write_cache_supported = le32_to_cpu(id->ctrl.vwc) & 0x1;
+    s->max_transfer = (id->ctrl.mdts ? 1 << id->ctrl.mdts : 0) * s->page_size;
     /* For now the page list buffer per command is one page, to hold at most
      * s->page_size / sizeof(uint64_t) entries. */
     s->max_transfer = MIN_NON_ZERO(s->max_transfer,
                           s->page_size / sizeof(uint64_t) * s->page_size);
 
-    oncs = le16_to_cpu(idctrl->oncs);
+    oncs = le16_to_cpu(id->ctrl.oncs);
     s->supports_write_zeroes = !!(oncs & NVME_ONCS_WRITE_ZEROS);
     s->supports_discard = !!(oncs & NVME_ONCS_DSM);
 
-    memset(id, 0, 4096);
+    memset(id, 0, sizeof(*id));
     cmd.cdw10 = 0;
     cmd.nsid = cpu_to_le32(namespace);
     if (nvme_cmd_sync(bs, s->queues[INDEX_ADMIN], &cmd)) {
@@ -562,11 +561,11 @@ static void nvme_identify(BlockDriverState *bs, int namespace, Error **errp)
         goto out;
     }
 
-    s->nsze = le64_to_cpu(idns->nsze);
-    lbaf = &idns->lbaf[NVME_ID_NS_FLBAS_INDEX(idns->flbas)];
+    s->nsze = le64_to_cpu(id->ns.nsze);
+    lbaf = &id->ns.lbaf[NVME_ID_NS_FLBAS_INDEX(id->ns.flbas)];
 
-    if (NVME_ID_NS_DLFEAT_WRITE_ZEROES(idns->dlfeat) &&
-            NVME_ID_NS_DLFEAT_READ_BEHAVIOR(idns->dlfeat) ==
+    if (NVME_ID_NS_DLFEAT_WRITE_ZEROES(id->ns.dlfeat) &&
+            NVME_ID_NS_DLFEAT_READ_BEHAVIOR(id->ns.dlfeat) ==
                     NVME_ID_NS_DLFEAT_READ_BEHAVIOR_ZEROES) {
         bs->supported_write_flags |= BDRV_REQ_MAY_UNMAP;
     }
-- 
2.26.2

[PATCH v5 09/15] block/nvme: Replace qemu_try_blockalign0 by qemu_try_blockalign/memset

[Philippe Mathieu-Daudé]

In the next commit we'll get rid of qemu_try_blockalign().
To ease review, first replace qemu_try_blockalign0() by explicit
calls to qemu_try_blockalign() and memset().

Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>
Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
---
 block/nvme.c | 16 +++++++++-------
 1 file changed, 9 insertions(+), 7 deletions(-)

diff --git a/block/nvme.c b/block/nvme.c
index 2bd1935f951..ac6bb52043d 100644
--- a/block/nvme.c
+++ b/block/nvme.c
@@ -174,12 +174,12 @@ static void nvme_init_queue(BlockDriverState *bs, NVMeQueue *q,
 
     bytes = ROUND_UP(nentries * entry_bytes, s->page_size);
     q->head = q->tail = 0;
-    q->queue = qemu_try_blockalign0(bs, bytes);
-
+    q->queue = qemu_try_blockalign(bs, bytes);
     if (!q->queue) {
         error_setg(errp, "Cannot allocate queue");
         return;
     }
+    memset(q->queue, 0, bytes);
     r = qemu_vfio_dma_map(s->vfio, q->queue, bytes, false, &q->iova);
     if (r) {
         error_setg(errp, "Cannot map queue");
@@ -223,11 +223,12 @@ static NVMeQueuePair *nvme_create_queue_pair(BlockDriverState *bs,
     if (!q) {
         return NULL;
     }
-    q->prp_list_pages = qemu_try_blockalign0(bs,
+    q->prp_list_pages = qemu_try_blockalign(bs,
                                           s->page_size * NVME_QUEUE_SIZE);
     if (!q->prp_list_pages) {
         goto fail;
     }
+    memset(q->prp_list_pages, 0, s->page_size * NVME_QUEUE_SIZE);
     qemu_mutex_init(&q->lock);
     q->s = s;
     q->index = idx;
@@ -521,7 +522,7 @@ static void nvme_identify(BlockDriverState *bs, int namespace, Error **errp)
         .cdw10 = cpu_to_le32(0x1),
     };
 
-    id = qemu_try_blockalign0(bs, sizeof(*id));
+    id = qemu_try_blockalign(bs, sizeof(*id));
     if (!id) {
         error_setg(errp, "Cannot allocate buffer for identify response");
         goto out;
@@ -531,8 +532,9 @@ static void nvme_identify(BlockDriverState *bs, int namespace, Error **errp)
         error_setg(errp, "Cannot map buffer for DMA");
         goto out;
     }
-    cmd.prp1 = cpu_to_le64(iova);
 
+    memset(id, 0, sizeof(*id));
+    cmd.prp1 = cpu_to_le64(iova);
     if (nvme_cmd_sync(bs, s->queues[INDEX_ADMIN], &cmd)) {
         error_setg(errp, "Failed to identify controller");
         goto out;
@@ -1283,11 +1285,11 @@ static int coroutine_fn nvme_co_pdiscard(BlockDriverState *bs,
 
     assert(s->nr_queues > 1);
 
-    buf = qemu_try_blockalign0(bs, s->page_size);
+    buf = qemu_try_blockalign(bs, s->page_size);
     if (!buf) {
         return -ENOMEM;
     }
-
+    memset(buf, 0, s->page_size);
     buf->nlb = cpu_to_le32(bytes >> s->blkshift);
     buf->slba = cpu_to_le64(offset >> s->blkshift);
     buf->cattr = 0;
-- 
2.26.2

[PATCH v5 10/15] block/nvme: Replace qemu_try_blockalign(bs) by qemu_try_memalign(pg_sz)

[Philippe Mathieu-Daudé]

qemu_try_blockalign() is a generic API that call back to the
block driver to return its page alignment. As we call from
within the very same driver, we already know to page alignment
stored in our state. Remove indirections and use the value from
BDRVNVMeState.
This change is required to later remove the BlockDriverState
argument, to make nvme_init_queue() per hardware, and not per
block driver.

Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>
Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
---
 block/nvme.c | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/block/nvme.c b/block/nvme.c
index ac6bb52043d..f180078e781 100644
--- a/block/nvme.c
+++ b/block/nvme.c
@@ -174,7 +174,7 @@ static void nvme_init_queue(BlockDriverState *bs, NVMeQueue *q,
 
     bytes = ROUND_UP(nentries * entry_bytes, s->page_size);
     q->head = q->tail = 0;
-    q->queue = qemu_try_blockalign(bs, bytes);
+    q->queue = qemu_try_memalign(s->page_size, bytes);
     if (!q->queue) {
         error_setg(errp, "Cannot allocate queue");
         return;
@@ -223,7 +223,7 @@ static NVMeQueuePair *nvme_create_queue_pair(BlockDriverState *bs,
     if (!q) {
         return NULL;
     }
-    q->prp_list_pages = qemu_try_blockalign(bs,
+    q->prp_list_pages = qemu_try_memalign(s->page_size,
                                           s->page_size * NVME_QUEUE_SIZE);
     if (!q->prp_list_pages) {
         goto fail;
@@ -522,7 +522,7 @@ static void nvme_identify(BlockDriverState *bs, int namespace, Error **errp)
         .cdw10 = cpu_to_le32(0x1),
     };
 
-    id = qemu_try_blockalign(bs, sizeof(*id));
+    id = qemu_try_memalign(s->page_size, sizeof(*id));
     if (!id) {
         error_setg(errp, "Cannot allocate buffer for identify response");
         goto out;
@@ -1141,7 +1141,7 @@ static int nvme_co_prw(BlockDriverState *bs, uint64_t offset, uint64_t bytes,
         return nvme_co_prw_aligned(bs, offset, bytes, qiov, is_write, flags);
     }
     trace_nvme_prw_buffered(s, offset, bytes, qiov->niov, is_write);
-    buf = qemu_try_blockalign(bs, bytes);
+    buf = qemu_try_memalign(s->page_size, bytes);
 
     if (!buf) {
         return -ENOMEM;
@@ -1285,7 +1285,7 @@ static int coroutine_fn nvme_co_pdiscard(BlockDriverState *bs,
 
     assert(s->nr_queues > 1);
 
-    buf = qemu_try_blockalign(bs, s->page_size);
+    buf = qemu_try_memalign(s->page_size, s->page_size);
     if (!buf) {
         return -ENOMEM;
     }
-- 
2.26.2

[PATCH v5 11/15] block/nvme: Simplify nvme_init_queue() arguments

[Philippe Mathieu-Daudé]

nvme_init_queue() doesn't require BlockDriverState anymore.
Replace it by BDRVNVMeState to simplify.

Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>
Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
---
 block/nvme.c | 7 +++----
 1 file changed, 3 insertions(+), 4 deletions(-)

diff --git a/block/nvme.c b/block/nvme.c
index f180078e781..5b69fc75a60 100644
--- a/block/nvme.c
+++ b/block/nvme.c
@@ -165,10 +165,9 @@ static QemuOptsList runtime_opts = {
     },
 };
 
-static void nvme_init_queue(BlockDriverState *bs, NVMeQueue *q,
+static void nvme_init_queue(BDRVNVMeState *s, NVMeQueue *q,
                             int nentries, int entry_bytes, Error **errp)
 {
-    BDRVNVMeState *s = bs->opaque;
     size_t bytes;
     int r;
 
@@ -251,14 +250,14 @@ static NVMeQueuePair *nvme_create_queue_pair(BlockDriverState *bs,
         req->prp_list_iova = prp_list_iova + i * s->page_size;
     }
 
-    nvme_init_queue(bs, &q->sq, size, NVME_SQ_ENTRY_BYTES, &local_err);
+    nvme_init_queue(s, &q->sq, size, NVME_SQ_ENTRY_BYTES, &local_err);
     if (local_err) {
         error_propagate(errp, local_err);
         goto fail;
     }
     q->sq.doorbell = &s->regs->doorbells[idx * 2 * s->doorbell_scale];
 
-    nvme_init_queue(bs, &q->cq, size, NVME_CQ_ENTRY_BYTES, &local_err);
+    nvme_init_queue(s, &q->cq, size, NVME_CQ_ENTRY_BYTES, &local_err);
     if (local_err) {
         error_propagate(errp, local_err);
         goto fail;
-- 
2.26.2

[PATCH v5 12/15] block/nvme: Replace BDRV_POLL_WHILE by AIO_WAIT_WHILE

[Philippe Mathieu-Daudé]

BDRV_POLL_WHILE() is defined as:

  #define BDRV_POLL_WHILE(bs, cond) ({          \
      BlockDriverState *bs_ = (bs);             \
      AIO_WAIT_WHILE(bdrv_get_aio_context(bs_), \
                     cond); })

As we will remove the BlockDriverState use in the next commit,
start by using the exploded version of BDRV_POLL_WHILE().

Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>
Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
---
 block/nvme.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/block/nvme.c b/block/nvme.c
index 5b69fc75a60..456fe61f5ea 100644
--- a/block/nvme.c
+++ b/block/nvme.c
@@ -493,6 +493,7 @@ static void nvme_cmd_sync_cb(void *opaque, int ret)
 static int nvme_cmd_sync(BlockDriverState *bs, NVMeQueuePair *q,
                          NvmeCmd *cmd)
 {
+    AioContext *aio_context = bdrv_get_aio_context(bs);
     NVMeRequest *req;
     int ret = -EINPROGRESS;
     req = nvme_get_free_req(q);
@@ -501,7 +502,7 @@ static int nvme_cmd_sync(BlockDriverState *bs, NVMeQueuePair *q,
     }
     nvme_submit_command(q, req, cmd, nvme_cmd_sync_cb, &ret);
 
-    BDRV_POLL_WHILE(bs, ret == -EINPROGRESS);
+    AIO_WAIT_WHILE(aio_context, ret == -EINPROGRESS);
     return ret;
 }
 
-- 
2.26.2

[PATCH v5 13/15] block/nvme: Simplify nvme_create_queue_pair() arguments

[Philippe Mathieu-Daudé]

nvme_create_queue_pair() doesn't require BlockDriverState anymore.
Replace it by BDRVNVMeState and AioContext to simplify.

Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>
Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
---
 block/nvme.c | 13 +++++++------
 1 file changed, 7 insertions(+), 6 deletions(-)

diff --git a/block/nvme.c b/block/nvme.c
index 456fe61f5ea..1f67e888c84 100644
--- a/block/nvme.c
+++ b/block/nvme.c
@@ -208,12 +208,12 @@ static void nvme_free_req_queue_cb(void *opaque)
     qemu_mutex_unlock(&q->lock);
 }
 
-static NVMeQueuePair *nvme_create_queue_pair(BlockDriverState *bs,
+static NVMeQueuePair *nvme_create_queue_pair(BDRVNVMeState *s,
+                                             AioContext *aio_context,
                                              int idx, int size,
                                              Error **errp)
 {
     int i, r;
-    BDRVNVMeState *s = bs->opaque;
     Error *local_err = NULL;
     NVMeQueuePair *q;
     uint64_t prp_list_iova;
@@ -232,8 +232,7 @@ static NVMeQueuePair *nvme_create_queue_pair(BlockDriverState *bs,
     q->s = s;
     q->index = idx;
     qemu_co_queue_init(&q->free_req_queue);
-    q->completion_bh = aio_bh_new(bdrv_get_aio_context(bs),
-                                  nvme_process_completion_bh, q);
+    q->completion_bh = aio_bh_new(aio_context, nvme_process_completion_bh, q);
     r = qemu_vfio_dma_map(s->vfio, q->prp_list_pages,
                           s->page_size * NVME_NUM_REQS,
                           false, &prp_list_iova);
@@ -637,7 +636,8 @@ static bool nvme_add_io_queue(BlockDriverState *bs, Error **errp)
     NvmeCmd cmd;
     int queue_size = NVME_QUEUE_SIZE;
 
-    q = nvme_create_queue_pair(bs, n, queue_size, errp);
+    q = nvme_create_queue_pair(s, bdrv_get_aio_context(bs),
+                               n, queue_size, errp);
     if (!q) {
         return false;
     }
@@ -683,6 +683,7 @@ static int nvme_init(BlockDriverState *bs, const char *device, int namespace,
                      Error **errp)
 {
     BDRVNVMeState *s = bs->opaque;
+    AioContext *aio_context = bdrv_get_aio_context(bs);
     int ret;
     uint64_t cap;
     uint64_t timeout_ms;
@@ -743,7 +744,7 @@ static int nvme_init(BlockDriverState *bs, const char *device, int namespace,
 
     /* Set up admin queue. */
     s->queues = g_new(NVMeQueuePair *, 1);
-    s->queues[INDEX_ADMIN] = nvme_create_queue_pair(bs, 0,
+    s->queues[INDEX_ADMIN] = nvme_create_queue_pair(s, aio_context, 0,
                                                           NVME_QUEUE_SIZE,
                                                           errp);
     if (!s->queues[INDEX_ADMIN]) {
-- 
2.26.2

[PATCH v5 14/15] block/nvme: Extract nvme_poll_queue()

[Philippe Mathieu-Daudé]

As we want to do per-queue polling, extract the nvme_poll_queue()
method which operates on a single queue.

Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>
Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
---
 block/nvme.c | 44 +++++++++++++++++++++++++++-----------------
 1 file changed, 27 insertions(+), 17 deletions(-)

diff --git a/block/nvme.c b/block/nvme.c
index 1f67e888c84..a61e86a83eb 100644
--- a/block/nvme.c
+++ b/block/nvme.c
@@ -590,31 +590,41 @@ out:
     qemu_vfree(id);
 }
 
+static bool nvme_poll_queue(NVMeQueuePair *q)
+{
+    bool progress = false;
+
+    const size_t cqe_offset = q->cq.head * NVME_CQ_ENTRY_BYTES;
+    NvmeCqe *cqe = (NvmeCqe *)&q->cq.queue[cqe_offset];
+
+    /*
+     * Do an early check for completions. q->lock isn't needed because
+     * nvme_process_completion() only runs in the event loop thread and
+     * cannot race with itself.
+     */
+    if ((le16_to_cpu(cqe->status) & 0x1) == q->cq_phase) {
+        return false;
+    }
+
+    qemu_mutex_lock(&q->lock);
+    while (nvme_process_completion(q)) {
+        /* Keep polling */
+        progress = true;
+    }
+    qemu_mutex_unlock(&q->lock);
+
+    return progress;
+}
+
 static bool nvme_poll_queues(BDRVNVMeState *s)
 {
     bool progress = false;
     int i;
 
     for (i = 0; i < s->nr_queues; i++) {
-        NVMeQueuePair *q = s->queues[i];
-        const size_t cqe_offset = q->cq.head * NVME_CQ_ENTRY_BYTES;
-        NvmeCqe *cqe = (NvmeCqe *)&q->cq.queue[cqe_offset];
-
-        /*
-         * Do an early check for completions. q->lock isn't needed because
-         * nvme_process_completion() only runs in the event loop thread and
-         * cannot race with itself.
-         */
-        if ((le16_to_cpu(cqe->status) & 0x1) == q->cq_phase) {
-            continue;
-        }
-
-        qemu_mutex_lock(&q->lock);
-        while (nvme_process_completion(q)) {
-            /* Keep polling */
+        if (nvme_poll_queue(s->queues[i])) {
             progress = true;
         }
-        qemu_mutex_unlock(&q->lock);
     }
     return progress;
 }
-- 
2.26.2

[PATCH v5 15/15] block/nvme: Use an array of EventNotifier

[Philippe Mathieu-Daudé]

In preparation of using multiple IRQ (thus multiple eventfds)
make BDRVNVMeState::irq_notifier an array (for now of a single
element, the admin queue notifier).

Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>
Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
---
 block/nvme.c | 31 +++++++++++++++++++++----------
 1 file changed, 21 insertions(+), 10 deletions(-)

diff --git a/block/nvme.c b/block/nvme.c
index a61e86a83eb..fe8a40b7ede 100644
--- a/block/nvme.c
+++ b/block/nvme.c
@@ -106,6 +106,12 @@ QEMU_BUILD_BUG_ON(offsetof(NVMeRegs, doorbells) != 0x1000);
 #define INDEX_ADMIN     0
 #define INDEX_IO(n)     (1 + n)
 
+/* This driver shares a single MSIX IRQ for the admin and I/O queues */
+enum {
+    MSIX_SHARED_IRQ_IDX = 0,
+    MSIX_IRQ_COUNT = 1
+};
+
 struct BDRVNVMeState {
     AioContext *aio_context;
     QEMUVFIOState *vfio;
@@ -120,7 +126,7 @@ struct BDRVNVMeState {
     /* How many uint32_t elements does each doorbell entry take. */
     size_t doorbell_scale;
     bool write_cache_supported;
-    EventNotifier irq_notifier;
+    EventNotifier irq_notifier[MSIX_IRQ_COUNT];
 
     uint64_t nsze; /* Namespace size reported by identify command */
     int nsid;      /* The namespace id to read/write data. */
@@ -631,7 +637,8 @@ static bool nvme_poll_queues(BDRVNVMeState *s)
 
 static void nvme_handle_event(EventNotifier *n)
 {
-    BDRVNVMeState *s = container_of(n, BDRVNVMeState, irq_notifier);
+    BDRVNVMeState *s = container_of(n, BDRVNVMeState,
+                                    irq_notifier[MSIX_SHARED_IRQ_IDX]);
 
     trace_nvme_handle_event(s);
     event_notifier_test_and_clear(n);
@@ -683,7 +690,8 @@ out_error:
 static bool nvme_poll_cb(void *opaque)
 {
     EventNotifier *e = opaque;
-    BDRVNVMeState *s = container_of(e, BDRVNVMeState, irq_notifier);
+    BDRVNVMeState *s = container_of(e, BDRVNVMeState,
+                                    irq_notifier[MSIX_SHARED_IRQ_IDX]);
 
     trace_nvme_poll_cb(s);
     return nvme_poll_queues(s);
@@ -705,7 +713,7 @@ static int nvme_init(BlockDriverState *bs, const char *device, int namespace,
     s->device = g_strdup(device);
     s->nsid = namespace;
     s->aio_context = bdrv_get_aio_context(bs);
-    ret = event_notifier_init(&s->irq_notifier, 0);
+    ret = event_notifier_init(&s->irq_notifier[MSIX_SHARED_IRQ_IDX], 0);
     if (ret) {
         error_setg(errp, "Failed to init event notifier");
         return ret;
@@ -784,12 +792,13 @@ static int nvme_init(BlockDriverState *bs, const char *device, int namespace,
         }
     }
 
-    ret = qemu_vfio_pci_init_irq(s->vfio, &s->irq_notifier,
+    ret = qemu_vfio_pci_init_irq(s->vfio, s->irq_notifier,
                                  VFIO_PCI_MSIX_IRQ_INDEX, errp);
     if (ret) {
         goto out;
     }
-    aio_set_event_notifier(bdrv_get_aio_context(bs), &s->irq_notifier,
+    aio_set_event_notifier(bdrv_get_aio_context(bs),
+                           &s->irq_notifier[MSIX_SHARED_IRQ_IDX],
                            false, nvme_handle_event, nvme_poll_cb);
 
     nvme_identify(bs, namespace, &local_err);
@@ -872,9 +881,10 @@ static void nvme_close(BlockDriverState *bs)
         nvme_free_queue_pair(s->queues[i]);
     }
     g_free(s->queues);
-    aio_set_event_notifier(bdrv_get_aio_context(bs), &s->irq_notifier,
+    aio_set_event_notifier(bdrv_get_aio_context(bs),
+                           &s->irq_notifier[MSIX_SHARED_IRQ_IDX],
                            false, NULL, NULL);
-    event_notifier_cleanup(&s->irq_notifier);
+    event_notifier_cleanup(&s->irq_notifier[MSIX_SHARED_IRQ_IDX]);
     qemu_vfio_pci_unmap_bar(s->vfio, 0, (void *)s->regs, 0, NVME_BAR_SIZE);
     qemu_vfio_close(s->vfio);
 
@@ -1381,7 +1391,8 @@ static void nvme_detach_aio_context(BlockDriverState *bs)
         q->completion_bh = NULL;
     }
 
-    aio_set_event_notifier(bdrv_get_aio_context(bs), &s->irq_notifier,
+    aio_set_event_notifier(bdrv_get_aio_context(bs),
+                           &s->irq_notifier[MSIX_SHARED_IRQ_IDX],
                            false, NULL, NULL);
 }
 
@@ -1391,7 +1402,7 @@ static void nvme_attach_aio_context(BlockDriverState *bs,
     BDRVNVMeState *s = bs->opaque;
 
     s->aio_context = new_context;
-    aio_set_event_notifier(new_context, &s->irq_notifier,
+    aio_set_event_notifier(new_context, &s->irq_notifier[MSIX_SHARED_IRQ_IDX],
                            false, nvme_handle_event, nvme_poll_cb);
 
     for (int i = 0; i < s->nr_queues; i++) {
-- 
2.26.2

Re: [PATCH v5 01/15] block/nvme: Replace magic value by SCALE_MS definition

[Stefano Garzarella]

On Thu, Aug 20, 2020 at 06:58:47PM +0200, Philippe Mathieu-Daudé wrote:
> Use self-explicit SCALE_MS definition instead of magic value.
> 
> Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>
> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
> ---
>  block/nvme.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)

Reviewed-by: Stefano Garzarella <sgarzare@redhat.com>

> 
> diff --git a/block/nvme.c b/block/nvme.c
> index 374e2689157..2f5e3c2adfa 100644
> --- a/block/nvme.c
> +++ b/block/nvme.c
> @@ -715,7 +715,7 @@ static int nvme_init(BlockDriverState *bs, const char *device, int namespace,
>      /* Reset device to get a clean state. */
>      s->regs->cc = cpu_to_le32(le32_to_cpu(s->regs->cc) & 0xFE);
>      /* Wait for CSTS.RDY = 0. */
> -    deadline = qemu_clock_get_ns(QEMU_CLOCK_REALTIME) + timeout_ms * 1000000ULL;
> +    deadline = qemu_clock_get_ns(QEMU_CLOCK_REALTIME) + timeout_ms * SCALE_MS;
>      while (le32_to_cpu(s->regs->csts) & 0x1) {
>          if (qemu_clock_get_ns(QEMU_CLOCK_REALTIME) > deadline) {
>              error_setg(errp, "Timeout while waiting for device to reset (%"
> -- 
> 2.26.2
> 
> 

Re: [PATCH v5 03/15] block/nvme: Let nvme_create_queue_pair() fail gracefully

[Stefano Garzarella]

On Thu, Aug 20, 2020 at 06:58:49PM +0200, Philippe Mathieu-Daudé wrote:
> As nvme_create_queue_pair() is allowed to fail, replace the
> alloc() calls by try_alloc() to avoid aborting QEMU.
> 
> Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>
> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
> ---
>  block/nvme.c | 12 ++++++++++--
>  1 file changed, 10 insertions(+), 2 deletions(-)
> 
> diff --git a/block/nvme.c b/block/nvme.c
> index 8c30a5fee28..e1893b4e792 100644
> --- a/block/nvme.c
> +++ b/block/nvme.c
> @@ -213,14 +213,22 @@ static NVMeQueuePair *nvme_create_queue_pair(BlockDriverState *bs,
>      int i, r;
>      BDRVNVMeState *s = bs->opaque;
>      Error *local_err = NULL;
> -    NVMeQueuePair *q = g_new0(NVMeQueuePair, 1);
> +    NVMeQueuePair *q;
>      uint64_t prp_list_iova;
>  
> +    q = g_try_new0(NVMeQueuePair, 1);
> +    if (!q) {
> +        return NULL;
> +    }
> +    q->prp_list_pages = qemu_try_blockalign0(bs,
> +                                          s->page_size * NVME_QUEUE_SIZE);

Here you use NVME_QUEUE_SIZE instead of NVME_NUM_REQS, is that an
intentional change?

Maybe is not an issue, sice NVME_QUEUE_SIZE is bigger than
NVME_NUM_REQS, but we should mention in the commit message.

Thanks,
Stefano

> +    if (!q->prp_list_pages) {
> +        goto fail;
> +    }
>      qemu_mutex_init(&q->lock);
>      q->s = s;
>      q->index = idx;
>      qemu_co_queue_init(&q->free_req_queue);
> -    q->prp_list_pages = qemu_blockalign0(bs, s->page_size * NVME_NUM_REQS);
>      q->completion_bh = aio_bh_new(bdrv_get_aio_context(bs),
>                                    nvme_process_completion_bh, q);
>      r = qemu_vfio_dma_map(s->vfio, q->prp_list_pages,
> -- 
> 2.26.2
> 
> 

Re: [PATCH v5 04/15] block/nvme: Define INDEX macros to ease code review

[Stefano Garzarella]

On Thu, Aug 20, 2020 at 06:58:50PM +0200, Philippe Mathieu-Daudé wrote:
> Use definitions instead of '0' or '1' indexes. Also this will
> be useful when using multi-queues later.
> 
> Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>
> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
> ---
>  block/nvme.c | 33 +++++++++++++++++++--------------
>  1 file changed, 19 insertions(+), 14 deletions(-)

Reviewed-by: Stefano Garzarella <sgarzare@redhat.com>

> 
> diff --git a/block/nvme.c b/block/nvme.c
> index e1893b4e792..003809fbd83 100644
> --- a/block/nvme.c
> +++ b/block/nvme.c
> @@ -103,6 +103,9 @@ typedef volatile struct {
>  
>  QEMU_BUILD_BUG_ON(offsetof(NVMeRegs, doorbells) != 0x1000);
>  
> +#define INDEX_ADMIN     0
> +#define INDEX_IO(n)     (1 + n)
> +
>  struct BDRVNVMeState {
>      AioContext *aio_context;
>      QEMUVFIOState *vfio;
> @@ -531,7 +534,7 @@ static void nvme_identify(BlockDriverState *bs, int namespace, Error **errp)
>      }
>      cmd.prp1 = cpu_to_le64(iova);
>  
> -    if (nvme_cmd_sync(bs, s->queues[0], &cmd)) {
> +    if (nvme_cmd_sync(bs, s->queues[INDEX_ADMIN], &cmd)) {
>          error_setg(errp, "Failed to identify controller");
>          goto out;
>      }
> @@ -555,7 +558,7 @@ static void nvme_identify(BlockDriverState *bs, int namespace, Error **errp)
>  
>      cmd.cdw10 = 0;
>      cmd.nsid = cpu_to_le32(namespace);
> -    if (nvme_cmd_sync(bs, s->queues[0], &cmd)) {
> +    if (nvme_cmd_sync(bs, s->queues[INDEX_ADMIN], &cmd)) {
>          error_setg(errp, "Failed to identify namespace");
>          goto out;
>      }
> @@ -644,7 +647,7 @@ static bool nvme_add_io_queue(BlockDriverState *bs, Error **errp)
>          .cdw10 = cpu_to_le32(((queue_size - 1) << 16) | (n & 0xFFFF)),
>          .cdw11 = cpu_to_le32(0x3),
>      };
> -    if (nvme_cmd_sync(bs, s->queues[0], &cmd)) {
> +    if (nvme_cmd_sync(bs, s->queues[INDEX_ADMIN], &cmd)) {
>          error_setg(errp, "Failed to create io queue [%d]", n);
>          nvme_free_queue_pair(q);
>          return false;
> @@ -655,7 +658,7 @@ static bool nvme_add_io_queue(BlockDriverState *bs, Error **errp)
>          .cdw10 = cpu_to_le32(((queue_size - 1) << 16) | (n & 0xFFFF)),
>          .cdw11 = cpu_to_le32(0x1 | (n << 16)),
>      };
> -    if (nvme_cmd_sync(bs, s->queues[0], &cmd)) {
> +    if (nvme_cmd_sync(bs, s->queues[INDEX_ADMIN], &cmd)) {
>          error_setg(errp, "Failed to create io queue [%d]", n);
>          nvme_free_queue_pair(q);
>          return false;
> @@ -739,16 +742,18 @@ static int nvme_init(BlockDriverState *bs, const char *device, int namespace,
>  
>      /* Set up admin queue. */
>      s->queues = g_new(NVMeQueuePair *, 1);
> -    s->queues[0] = nvme_create_queue_pair(bs, 0, NVME_QUEUE_SIZE, errp);
> -    if (!s->queues[0]) {
> +    s->queues[INDEX_ADMIN] = nvme_create_queue_pair(bs, 0,
> +                                                          NVME_QUEUE_SIZE,
> +                                                          errp);
> +    if (!s->queues[INDEX_ADMIN]) {
>          ret = -EINVAL;
>          goto out;
>      }
>      s->nr_queues = 1;
>      QEMU_BUILD_BUG_ON(NVME_QUEUE_SIZE & 0xF000);
>      s->regs->aqa = cpu_to_le32((NVME_QUEUE_SIZE << 16) | NVME_QUEUE_SIZE);
> -    s->regs->asq = cpu_to_le64(s->queues[0]->sq.iova);
> -    s->regs->acq = cpu_to_le64(s->queues[0]->cq.iova);
> +    s->regs->asq = cpu_to_le64(s->queues[INDEX_ADMIN]->sq.iova);
> +    s->regs->acq = cpu_to_le64(s->queues[INDEX_ADMIN]->cq.iova);
>  
>      /* After setting up all control registers we can enable device now. */
>      s->regs->cc = cpu_to_le32((ctz32(NVME_CQ_ENTRY_BYTES) << 20) |
> @@ -839,7 +844,7 @@ static int nvme_enable_disable_write_cache(BlockDriverState *bs, bool enable,
>          .cdw11 = cpu_to_le32(enable ? 0x01 : 0x00),
>      };
>  
> -    ret = nvme_cmd_sync(bs, s->queues[0], &cmd);
> +    ret = nvme_cmd_sync(bs, s->queues[INDEX_ADMIN], &cmd);
>      if (ret) {
>          error_setg(errp, "Failed to configure NVMe write cache");
>      }
> @@ -1056,7 +1061,7 @@ static coroutine_fn int nvme_co_prw_aligned(BlockDriverState *bs,
>  {
>      int r;
>      BDRVNVMeState *s = bs->opaque;
> -    NVMeQueuePair *ioq = s->queues[1];
> +    NVMeQueuePair *ioq = s->queues[INDEX_IO(0)];
>      NVMeRequest *req;
>  
>      uint32_t cdw12 = (((bytes >> s->blkshift) - 1) & 0xFFFF) |
> @@ -1171,7 +1176,7 @@ static coroutine_fn int nvme_co_pwritev(BlockDriverState *bs,
>  static coroutine_fn int nvme_co_flush(BlockDriverState *bs)
>  {
>      BDRVNVMeState *s = bs->opaque;
> -    NVMeQueuePair *ioq = s->queues[1];
> +    NVMeQueuePair *ioq = s->queues[INDEX_IO(0)];
>      NVMeRequest *req;
>      NvmeCmd cmd = {
>          .opcode = NVME_CMD_FLUSH,
> @@ -1202,7 +1207,7 @@ static coroutine_fn int nvme_co_pwrite_zeroes(BlockDriverState *bs,
>                                                BdrvRequestFlags flags)
>  {
>      BDRVNVMeState *s = bs->opaque;
> -    NVMeQueuePair *ioq = s->queues[1];
> +    NVMeQueuePair *ioq = s->queues[INDEX_IO(0)];
>      NVMeRequest *req;
>  
>      uint32_t cdw12 = ((bytes >> s->blkshift) - 1) & 0xFFFF;
> @@ -1255,7 +1260,7 @@ static int coroutine_fn nvme_co_pdiscard(BlockDriverState *bs,
>                                           int bytes)
>  {
>      BDRVNVMeState *s = bs->opaque;
> -    NVMeQueuePair *ioq = s->queues[1];
> +    NVMeQueuePair *ioq = s->queues[INDEX_IO(0)];
>      NVMeRequest *req;
>      NvmeDsmRange *buf;
>      QEMUIOVector local_qiov;
> @@ -1398,7 +1403,7 @@ static void nvme_aio_unplug(BlockDriverState *bs)
>      BDRVNVMeState *s = bs->opaque;
>      assert(s->plugged);
>      s->plugged = false;
> -    for (i = 1; i < s->nr_queues; i++) {
> +    for (i = INDEX_IO(0); i < s->nr_queues; i++) {
>          NVMeQueuePair *q = s->queues[i];
>          qemu_mutex_lock(&q->lock);
>          nvme_kick(q);
> -- 
> 2.26.2
> 
> 

Re: [PATCH v5 05/15] block/nvme: Improve error message when IO queue creation failed

[Stefano Garzarella]

On Thu, Aug 20, 2020 at 06:58:51PM +0200, Philippe Mathieu-Daudé wrote:
> Do not use the same error message for different failures.
> Display a different error whether it is the CQ or the SQ.
> 
> Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>
> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
> ---
>  block/nvme.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)

Reviewed-by: Stefano Garzarella <sgarzare@redhat.com>

> 
> diff --git a/block/nvme.c b/block/nvme.c
> index 003809fbd83..53448b7d230 100644
> --- a/block/nvme.c
> +++ b/block/nvme.c
> @@ -648,7 +648,7 @@ static bool nvme_add_io_queue(BlockDriverState *bs, Error **errp)
>          .cdw11 = cpu_to_le32(0x3),
>      };
>      if (nvme_cmd_sync(bs, s->queues[INDEX_ADMIN], &cmd)) {
> -        error_setg(errp, "Failed to create io queue [%d]", n);
> +        error_setg(errp, "Failed to create CQ io queue [%d]", n);
>          nvme_free_queue_pair(q);
>          return false;
>      }
> @@ -659,7 +659,7 @@ static bool nvme_add_io_queue(BlockDriverState *bs, Error **errp)
>          .cdw11 = cpu_to_le32(0x1 | (n << 16)),
>      };
>      if (nvme_cmd_sync(bs, s->queues[INDEX_ADMIN], &cmd)) {
> -        error_setg(errp, "Failed to create io queue [%d]", n);
> +        error_setg(errp, "Failed to create SQ io queue [%d]", n);
>          nvme_free_queue_pair(q);
>          return false;
>      }
> -- 
> 2.26.2
> 
> 

Re: [PATCH v5 06/15] block/nvme: Use common error path in nvme_add_io_queue()

[Stefano Garzarella]

On Thu, Aug 20, 2020 at 06:58:52PM +0200, Philippe Mathieu-Daudé wrote:
> Rearrange nvme_add_io_queue() by using a common error path.
> This will be proven useful in few commits where we add IRQ
> notification to the IO queues.
> 
> Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>
> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
> ---
>  block/nvme.c | 9 +++++----
>  1 file changed, 5 insertions(+), 4 deletions(-)

Reviewed-by: Stefano Garzarella <sgarzare@redhat.com>

> 
> diff --git a/block/nvme.c b/block/nvme.c
> index 53448b7d230..3101f1ad55d 100644
> --- a/block/nvme.c
> +++ b/block/nvme.c
> @@ -649,8 +649,7 @@ static bool nvme_add_io_queue(BlockDriverState *bs, Error **errp)
>      };
>      if (nvme_cmd_sync(bs, s->queues[INDEX_ADMIN], &cmd)) {
>          error_setg(errp, "Failed to create CQ io queue [%d]", n);
> -        nvme_free_queue_pair(q);
> -        return false;
> +        goto out_error;
>      }
>      cmd = (NvmeCmd) {
>          .opcode = NVME_ADM_CMD_CREATE_SQ,
> @@ -660,13 +659,15 @@ static bool nvme_add_io_queue(BlockDriverState *bs, Error **errp)
>      };
>      if (nvme_cmd_sync(bs, s->queues[INDEX_ADMIN], &cmd)) {
>          error_setg(errp, "Failed to create SQ io queue [%d]", n);
> -        nvme_free_queue_pair(q);
> -        return false;
> +        goto out_error;
>      }
>      s->queues = g_renew(NVMeQueuePair *, s->queues, n + 1);
>      s->queues[n] = q;
>      s->nr_queues++;
>      return true;
> +out_error:
> +    nvme_free_queue_pair(q);
> +    return false;
>  }
>  
>  static bool nvme_poll_cb(void *opaque)
> -- 
> 2.26.2
> 
> 

Re: [PATCH v5 07/15] block/nvme: Rename local variable

[Stefano Garzarella]

On Thu, Aug 20, 2020 at 06:58:53PM +0200, Philippe Mathieu-Daudé wrote:
> We are going to modify the code in the next commit. Renaming
> the 'resp' variable to 'id' first makes the next commit easier
> to review. No logical changes.
> 
> Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>
> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
> ---
>  block/nvme.c | 19 +++++++++----------
>  1 file changed, 9 insertions(+), 10 deletions(-)

Reviewed-by: Stefano Garzarella <sgarzare@redhat.com>

> 
> diff --git a/block/nvme.c b/block/nvme.c
> index 3101f1ad55d..99822d9fd36 100644
> --- a/block/nvme.c
> +++ b/block/nvme.c
> @@ -510,8 +510,8 @@ static void nvme_identify(BlockDriverState *bs, int namespace, Error **errp)
>      BDRVNVMeState *s = bs->opaque;
>      NvmeIdCtrl *idctrl;
>      NvmeIdNs *idns;
> +    uint8_t *id;
>      NvmeLBAF *lbaf;
> -    uint8_t *resp;
>      uint16_t oncs;
>      int r;
>      uint64_t iova;
> @@ -520,14 +520,14 @@ static void nvme_identify(BlockDriverState *bs, int namespace, Error **errp)
>          .cdw10 = cpu_to_le32(0x1),
>      };
>  
> -    resp = qemu_try_blockalign0(bs, sizeof(NvmeIdCtrl));
> -    if (!resp) {
> +    id = qemu_try_blockalign0(bs, sizeof(NvmeIdCtrl));
> +    if (!id) {
>          error_setg(errp, "Cannot allocate buffer for identify response");
>          goto out;
>      }
> -    idctrl = (NvmeIdCtrl *)resp;
> -    idns = (NvmeIdNs *)resp;
> -    r = qemu_vfio_dma_map(s->vfio, resp, sizeof(NvmeIdCtrl), true, &iova);
> +    idctrl = (NvmeIdCtrl *)id;
> +    idns = (NvmeIdNs *)id;
> +    r = qemu_vfio_dma_map(s->vfio, id, sizeof(NvmeIdCtrl), true, &iova);
>      if (r) {
>          error_setg(errp, "Cannot map buffer for DMA");
>          goto out;
> @@ -554,8 +554,7 @@ static void nvme_identify(BlockDriverState *bs, int namespace, Error **errp)
>      s->supports_write_zeroes = !!(oncs & NVME_ONCS_WRITE_ZEROS);
>      s->supports_discard = !!(oncs & NVME_ONCS_DSM);
>  
> -    memset(resp, 0, 4096);
> -
> +    memset(id, 0, 4096);
>      cmd.cdw10 = 0;
>      cmd.nsid = cpu_to_le32(namespace);
>      if (nvme_cmd_sync(bs, s->queues[INDEX_ADMIN], &cmd)) {
> @@ -587,8 +586,8 @@ static void nvme_identify(BlockDriverState *bs, int namespace, Error **errp)
>  
>      s->blkshift = lbaf->ds;
>  out:
> -    qemu_vfio_dma_unmap(s->vfio, resp);
> -    qemu_vfree(resp);
> +    qemu_vfio_dma_unmap(s->vfio, id);
> +    qemu_vfree(id);
>  }
>  
>  static bool nvme_poll_queues(BDRVNVMeState *s)
> -- 
> 2.26.2
> 
> 

Re: [PATCH v5 08/15] block/nvme: Use union of NvmeIdCtrl / NvmeIdNs structures

[Stefano Garzarella]

On Thu, Aug 20, 2020 at 06:58:54PM +0200, Philippe Mathieu-Daudé wrote:
> We allocate an unique chunk of memory then use it for two
> different structures. By using an union, we make it clear
> the data is overlapping (and we can remove the casts).
> 
> Suggested-by: Stefan Hajnoczi <stefanha@redhat.com>
> Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>
> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
> ---
>  block/nvme.c | 31 +++++++++++++++----------------
>  1 file changed, 15 insertions(+), 16 deletions(-)
> 
> diff --git a/block/nvme.c b/block/nvme.c
> index 99822d9fd36..2bd1935f951 100644
> --- a/block/nvme.c
> +++ b/block/nvme.c
> @@ -508,9 +508,10 @@ static int nvme_cmd_sync(BlockDriverState *bs, NVMeQueuePair *q,
>  static void nvme_identify(BlockDriverState *bs, int namespace, Error **errp)
>  {
>      BDRVNVMeState *s = bs->opaque;
> -    NvmeIdCtrl *idctrl;
> -    NvmeIdNs *idns;
> -    uint8_t *id;
> +    union {
> +        NvmeIdCtrl ctrl;
> +        NvmeIdNs ns;
> +    } *id;

What about defining a new 'NvmeId' type with this union?

>      NvmeLBAF *lbaf;
>      uint16_t oncs;
>      int r;
> @@ -520,14 +521,12 @@ static void nvme_identify(BlockDriverState *bs, int namespace, Error **errp)
>          .cdw10 = cpu_to_le32(0x1),
>      };
>  
> -    id = qemu_try_blockalign0(bs, sizeof(NvmeIdCtrl));
> +    id = qemu_try_blockalign0(bs, sizeof(*id));
>      if (!id) {
>          error_setg(errp, "Cannot allocate buffer for identify response");
>          goto out;
>      }
> -    idctrl = (NvmeIdCtrl *)id;
> -    idns = (NvmeIdNs *)id;
> -    r = qemu_vfio_dma_map(s->vfio, id, sizeof(NvmeIdCtrl), true, &iova);
> +    r = qemu_vfio_dma_map(s->vfio, id, sizeof(*id), true, &iova);
>      if (r) {
>          error_setg(errp, "Cannot map buffer for DMA");
>          goto out;
> @@ -539,22 +538,22 @@ static void nvme_identify(BlockDriverState *bs, int namespace, Error **errp)
>          goto out;
>      }
>  
> -    if (le32_to_cpu(idctrl->nn) < namespace) {
> +    if (le32_to_cpu(id->ctrl.nn) < namespace) {
>          error_setg(errp, "Invalid namespace");
>          goto out;
>      }
> -    s->write_cache_supported = le32_to_cpu(idctrl->vwc) & 0x1;
> -    s->max_transfer = (idctrl->mdts ? 1 << idctrl->mdts : 0) * s->page_size;
> +    s->write_cache_supported = le32_to_cpu(id->ctrl.vwc) & 0x1;
> +    s->max_transfer = (id->ctrl.mdts ? 1 << id->ctrl.mdts : 0) * s->page_size;
>      /* For now the page list buffer per command is one page, to hold at most
>       * s->page_size / sizeof(uint64_t) entries. */
>      s->max_transfer = MIN_NON_ZERO(s->max_transfer,
>                            s->page_size / sizeof(uint64_t) * s->page_size);
>  
> -    oncs = le16_to_cpu(idctrl->oncs);
> +    oncs = le16_to_cpu(id->ctrl.oncs);
>      s->supports_write_zeroes = !!(oncs & NVME_ONCS_WRITE_ZEROS);
>      s->supports_discard = !!(oncs & NVME_ONCS_DSM);
>  
> -    memset(id, 0, 4096);
> +    memset(id, 0, sizeof(*id));
>      cmd.cdw10 = 0;
>      cmd.nsid = cpu_to_le32(namespace);
>      if (nvme_cmd_sync(bs, s->queues[INDEX_ADMIN], &cmd)) {
> @@ -562,11 +561,11 @@ static void nvme_identify(BlockDriverState *bs, int namespace, Error **errp)
>          goto out;
>      }
>  
> -    s->nsze = le64_to_cpu(idns->nsze);
> -    lbaf = &idns->lbaf[NVME_ID_NS_FLBAS_INDEX(idns->flbas)];
> +    s->nsze = le64_to_cpu(id->ns.nsze);
> +    lbaf = &id->ns.lbaf[NVME_ID_NS_FLBAS_INDEX(id->ns.flbas)];
>  
> -    if (NVME_ID_NS_DLFEAT_WRITE_ZEROES(idns->dlfeat) &&
> -            NVME_ID_NS_DLFEAT_READ_BEHAVIOR(idns->dlfeat) ==
> +    if (NVME_ID_NS_DLFEAT_WRITE_ZEROES(id->ns.dlfeat) &&
> +            NVME_ID_NS_DLFEAT_READ_BEHAVIOR(id->ns.dlfeat) ==
>                      NVME_ID_NS_DLFEAT_READ_BEHAVIOR_ZEROES) {
>          bs->supported_write_flags |= BDRV_REQ_MAY_UNMAP;
>      }
> -- 
> 2.26.2
> 
> 

With or without the new tyoe, the patch looks good to me:

Reviewed-by: Stefano Garzarella <sgarzare@redhat.com>

Re: [PATCH v5 09/15] block/nvme: Replace qemu_try_blockalign0 by qemu_try_blockalign/memset

[Stefano Garzarella]

On Thu, Aug 20, 2020 at 06:58:55PM +0200, Philippe Mathieu-Daudé wrote:
> In the next commit we'll get rid of qemu_try_blockalign().
> To ease review, first replace qemu_try_blockalign0() by explicit
> calls to qemu_try_blockalign() and memset().
> 
> Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>
> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
> ---
>  block/nvme.c | 16 +++++++++-------
>  1 file changed, 9 insertions(+), 7 deletions(-)

Reviewed-by: Stefano Garzarella <sgarzare@redhat.com>

> 
> diff --git a/block/nvme.c b/block/nvme.c
> index 2bd1935f951..ac6bb52043d 100644
> --- a/block/nvme.c
> +++ b/block/nvme.c
> @@ -174,12 +174,12 @@ static void nvme_init_queue(BlockDriverState *bs, NVMeQueue *q,
>  
>      bytes = ROUND_UP(nentries * entry_bytes, s->page_size);
>      q->head = q->tail = 0;
> -    q->queue = qemu_try_blockalign0(bs, bytes);
> -
> +    q->queue = qemu_try_blockalign(bs, bytes);
>      if (!q->queue) {
>          error_setg(errp, "Cannot allocate queue");
>          return;
>      }
> +    memset(q->queue, 0, bytes);
>      r = qemu_vfio_dma_map(s->vfio, q->queue, bytes, false, &q->iova);
>      if (r) {
>          error_setg(errp, "Cannot map queue");
> @@ -223,11 +223,12 @@ static NVMeQueuePair *nvme_create_queue_pair(BlockDriverState *bs,
>      if (!q) {
>          return NULL;
>      }
> -    q->prp_list_pages = qemu_try_blockalign0(bs,
> +    q->prp_list_pages = qemu_try_blockalign(bs,
>                                            s->page_size * NVME_QUEUE_SIZE);
>      if (!q->prp_list_pages) {
>          goto fail;
>      }
> +    memset(q->prp_list_pages, 0, s->page_size * NVME_QUEUE_SIZE);
>      qemu_mutex_init(&q->lock);
>      q->s = s;
>      q->index = idx;
> @@ -521,7 +522,7 @@ static void nvme_identify(BlockDriverState *bs, int namespace, Error **errp)
>          .cdw10 = cpu_to_le32(0x1),
>      };
>  
> -    id = qemu_try_blockalign0(bs, sizeof(*id));
> +    id = qemu_try_blockalign(bs, sizeof(*id));
>      if (!id) {
>          error_setg(errp, "Cannot allocate buffer for identify response");
>          goto out;
> @@ -531,8 +532,9 @@ static void nvme_identify(BlockDriverState *bs, int namespace, Error **errp)
>          error_setg(errp, "Cannot map buffer for DMA");
>          goto out;
>      }
> -    cmd.prp1 = cpu_to_le64(iova);
>  
> +    memset(id, 0, sizeof(*id));
> +    cmd.prp1 = cpu_to_le64(iova);
>      if (nvme_cmd_sync(bs, s->queues[INDEX_ADMIN], &cmd)) {
>          error_setg(errp, "Failed to identify controller");
>          goto out;
> @@ -1283,11 +1285,11 @@ static int coroutine_fn nvme_co_pdiscard(BlockDriverState *bs,
>  
>      assert(s->nr_queues > 1);
>  
> -    buf = qemu_try_blockalign0(bs, s->page_size);
> +    buf = qemu_try_blockalign(bs, s->page_size);
>      if (!buf) {
>          return -ENOMEM;
>      }
> -
> +    memset(buf, 0, s->page_size);
>      buf->nlb = cpu_to_le32(bytes >> s->blkshift);
>      buf->slba = cpu_to_le64(offset >> s->blkshift);
>      buf->cattr = 0;
> -- 
> 2.26.2
> 
> 

Re: [PATCH v5 10/15] block/nvme: Replace qemu_try_blockalign(bs) by qemu_try_memalign(pg_sz)

[Stefano Garzarella]

On Thu, Aug 20, 2020 at 06:58:56PM +0200, Philippe Mathieu-Daudé wrote:
> qemu_try_blockalign() is a generic API that call back to the
> block driver to return its page alignment. As we call from
> within the very same driver, we already know to page alignment
> stored in our state. Remove indirections and use the value from
> BDRVNVMeState.
> This change is required to later remove the BlockDriverState
> argument, to make nvme_init_queue() per hardware, and not per
> block driver.
> 
> Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>
> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
> ---
>  block/nvme.c | 10 +++++-----
>  1 file changed, 5 insertions(+), 5 deletions(-)

Reviewed-by: Stefano Garzarella <sgarzare@redhat.com>

> 
> diff --git a/block/nvme.c b/block/nvme.c
> index ac6bb52043d..f180078e781 100644
> --- a/block/nvme.c
> +++ b/block/nvme.c
> @@ -174,7 +174,7 @@ static void nvme_init_queue(BlockDriverState *bs, NVMeQueue *q,
>  
>      bytes = ROUND_UP(nentries * entry_bytes, s->page_size);
>      q->head = q->tail = 0;
> -    q->queue = qemu_try_blockalign(bs, bytes);
> +    q->queue = qemu_try_memalign(s->page_size, bytes);
>      if (!q->queue) {
>          error_setg(errp, "Cannot allocate queue");
>          return;
> @@ -223,7 +223,7 @@ static NVMeQueuePair *nvme_create_queue_pair(BlockDriverState *bs,
>      if (!q) {
>          return NULL;
>      }
> -    q->prp_list_pages = qemu_try_blockalign(bs,
> +    q->prp_list_pages = qemu_try_memalign(s->page_size,
>                                            s->page_size * NVME_QUEUE_SIZE);
>      if (!q->prp_list_pages) {
>          goto fail;
> @@ -522,7 +522,7 @@ static void nvme_identify(BlockDriverState *bs, int namespace, Error **errp)
>          .cdw10 = cpu_to_le32(0x1),
>      };
>  
> -    id = qemu_try_blockalign(bs, sizeof(*id));
> +    id = qemu_try_memalign(s->page_size, sizeof(*id));
>      if (!id) {
>          error_setg(errp, "Cannot allocate buffer for identify response");
>          goto out;
> @@ -1141,7 +1141,7 @@ static int nvme_co_prw(BlockDriverState *bs, uint64_t offset, uint64_t bytes,
>          return nvme_co_prw_aligned(bs, offset, bytes, qiov, is_write, flags);
>      }
>      trace_nvme_prw_buffered(s, offset, bytes, qiov->niov, is_write);
> -    buf = qemu_try_blockalign(bs, bytes);
> +    buf = qemu_try_memalign(s->page_size, bytes);
>  
>      if (!buf) {
>          return -ENOMEM;
> @@ -1285,7 +1285,7 @@ static int coroutine_fn nvme_co_pdiscard(BlockDriverState *bs,
>  
>      assert(s->nr_queues > 1);
>  
> -    buf = qemu_try_blockalign(bs, s->page_size);
> +    buf = qemu_try_memalign(s->page_size, s->page_size);
>      if (!buf) {
>          return -ENOMEM;
>      }
> -- 
> 2.26.2
> 
> 

Re: [PATCH v5 11/15] block/nvme: Simplify nvme_init_queue() arguments

[Stefano Garzarella]

On Thu, Aug 20, 2020 at 06:58:57PM +0200, Philippe Mathieu-Daudé wrote:
> nvme_init_queue() doesn't require BlockDriverState anymore.
> Replace it by BDRVNVMeState to simplify.
> 
> Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>
> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
> ---
>  block/nvme.c | 7 +++----
>  1 file changed, 3 insertions(+), 4 deletions(-)

Reviewed-by: Stefano Garzarella <sgarzare@redhat.com>

> 
> diff --git a/block/nvme.c b/block/nvme.c
> index f180078e781..5b69fc75a60 100644
> --- a/block/nvme.c
> +++ b/block/nvme.c
> @@ -165,10 +165,9 @@ static QemuOptsList runtime_opts = {
>      },
>  };
>  
> -static void nvme_init_queue(BlockDriverState *bs, NVMeQueue *q,
> +static void nvme_init_queue(BDRVNVMeState *s, NVMeQueue *q,
>                              int nentries, int entry_bytes, Error **errp)
>  {
> -    BDRVNVMeState *s = bs->opaque;
>      size_t bytes;
>      int r;
>  
> @@ -251,14 +250,14 @@ static NVMeQueuePair *nvme_create_queue_pair(BlockDriverState *bs,
>          req->prp_list_iova = prp_list_iova + i * s->page_size;
>      }
>  
> -    nvme_init_queue(bs, &q->sq, size, NVME_SQ_ENTRY_BYTES, &local_err);
> +    nvme_init_queue(s, &q->sq, size, NVME_SQ_ENTRY_BYTES, &local_err);
>      if (local_err) {
>          error_propagate(errp, local_err);
>          goto fail;
>      }
>      q->sq.doorbell = &s->regs->doorbells[idx * 2 * s->doorbell_scale];
>  
> -    nvme_init_queue(bs, &q->cq, size, NVME_CQ_ENTRY_BYTES, &local_err);
> +    nvme_init_queue(s, &q->cq, size, NVME_CQ_ENTRY_BYTES, &local_err);
>      if (local_err) {
>          error_propagate(errp, local_err);
>          goto fail;
> -- 
> 2.26.2
> 
> 

Re: [PATCH v5 12/15] block/nvme: Replace BDRV_POLL_WHILE by AIO_WAIT_WHILE

[Stefano Garzarella]

On Thu, Aug 20, 2020 at 06:58:58PM +0200, Philippe Mathieu-Daudé wrote:
> BDRV_POLL_WHILE() is defined as:
> 
>   #define BDRV_POLL_WHILE(bs, cond) ({          \
>       BlockDriverState *bs_ = (bs);             \
>       AIO_WAIT_WHILE(bdrv_get_aio_context(bs_), \
>                      cond); })
> 
> As we will remove the BlockDriverState use in the next commit,
> start by using the exploded version of BDRV_POLL_WHILE().
> 
> Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>
> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
> ---
>  block/nvme.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/block/nvme.c b/block/nvme.c
> index 5b69fc75a60..456fe61f5ea 100644
> --- a/block/nvme.c
> +++ b/block/nvme.c
> @@ -493,6 +493,7 @@ static void nvme_cmd_sync_cb(void *opaque, int ret)
>  static int nvme_cmd_sync(BlockDriverState *bs, NVMeQueuePair *q,
>                           NvmeCmd *cmd)
>  {
> +    AioContext *aio_context = bdrv_get_aio_context(bs);
>      NVMeRequest *req;
>      int ret = -EINPROGRESS;
>      req = nvme_get_free_req(q);
> @@ -501,7 +502,7 @@ static int nvme_cmd_sync(BlockDriverState *bs, NVMeQueuePair *q,
>      }
>      nvme_submit_command(q, req, cmd, nvme_cmd_sync_cb, &ret);
>  
> -    BDRV_POLL_WHILE(bs, ret == -EINPROGRESS);
> +    AIO_WAIT_WHILE(aio_context, ret == -EINPROGRESS);

Maybe I would have:

    AIO_WAIT_WHILE(bdrv_get_aio_context(bs), ret == -EINPROGRESS);

But it doesn't matter, LGTM:

Reviewed-by: Stefano Garzarella <sgarzare@redhat.com>

>      return ret;
>  }
>  
> -- 
> 2.26.2
> 
> 

Re: [PATCH v5 13/15] block/nvme: Simplify nvme_create_queue_pair() arguments

[Stefano Garzarella]

On Thu, Aug 20, 2020 at 06:58:59PM +0200, Philippe Mathieu-Daudé wrote:
> nvme_create_queue_pair() doesn't require BlockDriverState anymore.
> Replace it by BDRVNVMeState and AioContext to simplify.
> 
> Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>
> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
> ---
>  block/nvme.c | 13 +++++++------
>  1 file changed, 7 insertions(+), 6 deletions(-)

Reviewed-by: Stefano Garzarella <sgarzare@redhat.com>

> 
> diff --git a/block/nvme.c b/block/nvme.c
> index 456fe61f5ea..1f67e888c84 100644
> --- a/block/nvme.c
> +++ b/block/nvme.c
> @@ -208,12 +208,12 @@ static void nvme_free_req_queue_cb(void *opaque)
>      qemu_mutex_unlock(&q->lock);
>  }
>  
> -static NVMeQueuePair *nvme_create_queue_pair(BlockDriverState *bs,
> +static NVMeQueuePair *nvme_create_queue_pair(BDRVNVMeState *s,
> +                                             AioContext *aio_context,
>                                               int idx, int size,
>                                               Error **errp)
>  {
>      int i, r;
> -    BDRVNVMeState *s = bs->opaque;
>      Error *local_err = NULL;
>      NVMeQueuePair *q;
>      uint64_t prp_list_iova;
> @@ -232,8 +232,7 @@ static NVMeQueuePair *nvme_create_queue_pair(BlockDriverState *bs,
>      q->s = s;
>      q->index = idx;
>      qemu_co_queue_init(&q->free_req_queue);
> -    q->completion_bh = aio_bh_new(bdrv_get_aio_context(bs),
> -                                  nvme_process_completion_bh, q);
> +    q->completion_bh = aio_bh_new(aio_context, nvme_process_completion_bh, q);
>      r = qemu_vfio_dma_map(s->vfio, q->prp_list_pages,
>                            s->page_size * NVME_NUM_REQS,
>                            false, &prp_list_iova);
> @@ -637,7 +636,8 @@ static bool nvme_add_io_queue(BlockDriverState *bs, Error **errp)
>      NvmeCmd cmd;
>      int queue_size = NVME_QUEUE_SIZE;
>  
> -    q = nvme_create_queue_pair(bs, n, queue_size, errp);
> +    q = nvme_create_queue_pair(s, bdrv_get_aio_context(bs),
> +                               n, queue_size, errp);
>      if (!q) {
>          return false;
>      }
> @@ -683,6 +683,7 @@ static int nvme_init(BlockDriverState *bs, const char *device, int namespace,
>                       Error **errp)
>  {
>      BDRVNVMeState *s = bs->opaque;
> +    AioContext *aio_context = bdrv_get_aio_context(bs);
>      int ret;
>      uint64_t cap;
>      uint64_t timeout_ms;
> @@ -743,7 +744,7 @@ static int nvme_init(BlockDriverState *bs, const char *device, int namespace,
>  
>      /* Set up admin queue. */
>      s->queues = g_new(NVMeQueuePair *, 1);
> -    s->queues[INDEX_ADMIN] = nvme_create_queue_pair(bs, 0,
> +    s->queues[INDEX_ADMIN] = nvme_create_queue_pair(s, aio_context, 0,
>                                                            NVME_QUEUE_SIZE,
>                                                            errp);
>      if (!s->queues[INDEX_ADMIN]) {
> -- 
> 2.26.2
> 
> 

Re: [PATCH v5 14/15] block/nvme: Extract nvme_poll_queue()

[Stefano Garzarella]

On Thu, Aug 20, 2020 at 06:59:00PM +0200, Philippe Mathieu-Daudé wrote:
> As we want to do per-queue polling, extract the nvme_poll_queue()
> method which operates on a single queue.
> 
> Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>
> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
> ---
>  block/nvme.c | 44 +++++++++++++++++++++++++++-----------------
>  1 file changed, 27 insertions(+), 17 deletions(-)

Reviewed-by: Stefano Garzarella <sgarzare@redhat.com>

> 
> diff --git a/block/nvme.c b/block/nvme.c
> index 1f67e888c84..a61e86a83eb 100644
> --- a/block/nvme.c
> +++ b/block/nvme.c
> @@ -590,31 +590,41 @@ out:
>      qemu_vfree(id);
>  }
>  
> +static bool nvme_poll_queue(NVMeQueuePair *q)
> +{
> +    bool progress = false;
> +
> +    const size_t cqe_offset = q->cq.head * NVME_CQ_ENTRY_BYTES;
> +    NvmeCqe *cqe = (NvmeCqe *)&q->cq.queue[cqe_offset];
> +
> +    /*
> +     * Do an early check for completions. q->lock isn't needed because
> +     * nvme_process_completion() only runs in the event loop thread and
> +     * cannot race with itself.
> +     */
> +    if ((le16_to_cpu(cqe->status) & 0x1) == q->cq_phase) {
> +        return false;
> +    }
> +
> +    qemu_mutex_lock(&q->lock);
> +    while (nvme_process_completion(q)) {
> +        /* Keep polling */
> +        progress = true;
> +    }
> +    qemu_mutex_unlock(&q->lock);
> +
> +    return progress;
> +}
> +
>  static bool nvme_poll_queues(BDRVNVMeState *s)
>  {
>      bool progress = false;
>      int i;
>  
>      for (i = 0; i < s->nr_queues; i++) {
> -        NVMeQueuePair *q = s->queues[i];
> -        const size_t cqe_offset = q->cq.head * NVME_CQ_ENTRY_BYTES;
> -        NvmeCqe *cqe = (NvmeCqe *)&q->cq.queue[cqe_offset];
> -
> -        /*
> -         * Do an early check for completions. q->lock isn't needed because
> -         * nvme_process_completion() only runs in the event loop thread and
> -         * cannot race with itself.
> -         */
> -        if ((le16_to_cpu(cqe->status) & 0x1) == q->cq_phase) {
> -            continue;
> -        }
> -
> -        qemu_mutex_lock(&q->lock);
> -        while (nvme_process_completion(q)) {
> -            /* Keep polling */
> +        if (nvme_poll_queue(s->queues[i])) {
>              progress = true;
>          }
> -        qemu_mutex_unlock(&q->lock);
>      }
>      return progress;
>  }
> -- 
> 2.26.2
> 
> 

Re: [PATCH v5 15/15] block/nvme: Use an array of EventNotifier

[Stefano Garzarella]

On Thu, Aug 20, 2020 at 06:59:01PM +0200, Philippe Mathieu-Daudé wrote:
> In preparation of using multiple IRQ (thus multiple eventfds)
> make BDRVNVMeState::irq_notifier an array (for now of a single
> element, the admin queue notifier).
> 
> Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>
> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
> ---
>  block/nvme.c | 31 +++++++++++++++++++++----------
>  1 file changed, 21 insertions(+), 10 deletions(-)
> 
> diff --git a/block/nvme.c b/block/nvme.c
> index a61e86a83eb..fe8a40b7ede 100644
> --- a/block/nvme.c
> +++ b/block/nvme.c
> @@ -106,6 +106,12 @@ QEMU_BUILD_BUG_ON(offsetof(NVMeRegs, doorbells) != 0x1000);
>  #define INDEX_ADMIN     0
>  #define INDEX_IO(n)     (1 + n)
>  
> +/* This driver shares a single MSIX IRQ for the admin and I/O queues */
> +enum {
> +    MSIX_SHARED_IRQ_IDX = 0,
> +    MSIX_IRQ_COUNT = 1
> +};
> +
>  struct BDRVNVMeState {
>      AioContext *aio_context;
>      QEMUVFIOState *vfio;
> @@ -120,7 +126,7 @@ struct BDRVNVMeState {
>      /* How many uint32_t elements does each doorbell entry take. */
>      size_t doorbell_scale;
>      bool write_cache_supported;
> -    EventNotifier irq_notifier;
> +    EventNotifier irq_notifier[MSIX_IRQ_COUNT];
>  
>      uint64_t nsze; /* Namespace size reported by identify command */
>      int nsid;      /* The namespace id to read/write data. */
> @@ -631,7 +637,8 @@ static bool nvme_poll_queues(BDRVNVMeState *s)
>  
>  static void nvme_handle_event(EventNotifier *n)
>  {
> -    BDRVNVMeState *s = container_of(n, BDRVNVMeState, irq_notifier);
> +    BDRVNVMeState *s = container_of(n, BDRVNVMeState,
> +                                    irq_notifier[MSIX_SHARED_IRQ_IDX]);
>  
>      trace_nvme_handle_event(s);
>      event_notifier_test_and_clear(n);
> @@ -683,7 +690,8 @@ out_error:
>  static bool nvme_poll_cb(void *opaque)
>  {
>      EventNotifier *e = opaque;
> -    BDRVNVMeState *s = container_of(e, BDRVNVMeState, irq_notifier);
> +    BDRVNVMeState *s = container_of(e, BDRVNVMeState,
> +                                    irq_notifier[MSIX_SHARED_IRQ_IDX]);
>  
>      trace_nvme_poll_cb(s);
>      return nvme_poll_queues(s);
> @@ -705,7 +713,7 @@ static int nvme_init(BlockDriverState *bs, const char *device, int namespace,
>      s->device = g_strdup(device);
>      s->nsid = namespace;
>      s->aio_context = bdrv_get_aio_context(bs);
> -    ret = event_notifier_init(&s->irq_notifier, 0);
> +    ret = event_notifier_init(&s->irq_notifier[MSIX_SHARED_IRQ_IDX], 0);
>      if (ret) {
>          error_setg(errp, "Failed to init event notifier");
>          return ret;
> @@ -784,12 +792,13 @@ static int nvme_init(BlockDriverState *bs, const char *device, int namespace,
>          }
>      }
>  
> -    ret = qemu_vfio_pci_init_irq(s->vfio, &s->irq_notifier,
> +    ret = qemu_vfio_pci_init_irq(s->vfio, s->irq_notifier,

Maybe we can use '&s->irq_notifier[MSIX_SHARED_IRQ_IDX]' to match the other
changes.

>                                   VFIO_PCI_MSIX_IRQ_INDEX, errp);
>      if (ret) {
>          goto out;
>      }
> -    aio_set_event_notifier(bdrv_get_aio_context(bs), &s->irq_notifier,
> +    aio_set_event_notifier(bdrv_get_aio_context(bs),
> +                           &s->irq_notifier[MSIX_SHARED_IRQ_IDX],
>                             false, nvme_handle_event, nvme_poll_cb);
>  
>      nvme_identify(bs, namespace, &local_err);
> @@ -872,9 +881,10 @@ static void nvme_close(BlockDriverState *bs)
>          nvme_free_queue_pair(s->queues[i]);
>      }
>      g_free(s->queues);
> -    aio_set_event_notifier(bdrv_get_aio_context(bs), &s->irq_notifier,
> +    aio_set_event_notifier(bdrv_get_aio_context(bs),
> +                           &s->irq_notifier[MSIX_SHARED_IRQ_IDX],
>                             false, NULL, NULL);
> -    event_notifier_cleanup(&s->irq_notifier);
> +    event_notifier_cleanup(&s->irq_notifier[MSIX_SHARED_IRQ_IDX]);
>      qemu_vfio_pci_unmap_bar(s->vfio, 0, (void *)s->regs, 0, NVME_BAR_SIZE);
>      qemu_vfio_close(s->vfio);
>  
> @@ -1381,7 +1391,8 @@ static void nvme_detach_aio_context(BlockDriverState *bs)
>          q->completion_bh = NULL;
>      }
>  
> -    aio_set_event_notifier(bdrv_get_aio_context(bs), &s->irq_notifier,
> +    aio_set_event_notifier(bdrv_get_aio_context(bs),
> +                           &s->irq_notifier[MSIX_SHARED_IRQ_IDX],
>                             false, NULL, NULL);
>  }
>  
> @@ -1391,7 +1402,7 @@ static void nvme_attach_aio_context(BlockDriverState *bs,
>      BDRVNVMeState *s = bs->opaque;
>  
>      s->aio_context = new_context;
> -    aio_set_event_notifier(new_context, &s->irq_notifier,
> +    aio_set_event_notifier(new_context, &s->irq_notifier[MSIX_SHARED_IRQ_IDX],
>                             false, nvme_handle_event, nvme_poll_cb);
>  
>      for (int i = 0; i < s->nr_queues; i++) {
> -- 
> 2.26.2
> 
> 

Re: [PATCH v5 15/15] block/nvme: Use an array of EventNotifier

[Philippe Mathieu-Daudé]

On 8/21/20 12:29 PM, Stefano Garzarella wrote:
> On Thu, Aug 20, 2020 at 06:59:01PM +0200, Philippe Mathieu-Daudé wrote:
>> In preparation of using multiple IRQ (thus multiple eventfds)
>> make BDRVNVMeState::irq_notifier an array (for now of a single
>> element, the admin queue notifier).
>>
>> Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>
>> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
>> ---
>>  block/nvme.c | 31 +++++++++++++++++++++----------
>>  1 file changed, 21 insertions(+), 10 deletions(-)
>>
>> diff --git a/block/nvme.c b/block/nvme.c
>> index a61e86a83eb..fe8a40b7ede 100644
>> --- a/block/nvme.c
>> +++ b/block/nvme.c
>> @@ -106,6 +106,12 @@ QEMU_BUILD_BUG_ON(offsetof(NVMeRegs, doorbells) != 0x1000);
>>  #define INDEX_ADMIN     0
>>  #define INDEX_IO(n)     (1 + n)
>>  
>> +/* This driver shares a single MSIX IRQ for the admin and I/O queues */
>> +enum {
>> +    MSIX_SHARED_IRQ_IDX = 0,
>> +    MSIX_IRQ_COUNT = 1
>> +};
>> +
>>  struct BDRVNVMeState {
>>      AioContext *aio_context;
>>      QEMUVFIOState *vfio;
>> @@ -120,7 +126,7 @@ struct BDRVNVMeState {
>>      /* How many uint32_t elements does each doorbell entry take. */
>>      size_t doorbell_scale;
>>      bool write_cache_supported;
>> -    EventNotifier irq_notifier;
>> +    EventNotifier irq_notifier[MSIX_IRQ_COUNT];
>>  
>>      uint64_t nsze; /* Namespace size reported by identify command */
>>      int nsid;      /* The namespace id to read/write data. */
>> @@ -631,7 +637,8 @@ static bool nvme_poll_queues(BDRVNVMeState *s)
>>  
>>  static void nvme_handle_event(EventNotifier *n)
>>  {
>> -    BDRVNVMeState *s = container_of(n, BDRVNVMeState, irq_notifier);
>> +    BDRVNVMeState *s = container_of(n, BDRVNVMeState,
>> +                                    irq_notifier[MSIX_SHARED_IRQ_IDX]);
>>  
>>      trace_nvme_handle_event(s);
>>      event_notifier_test_and_clear(n);
>> @@ -683,7 +690,8 @@ out_error:
>>  static bool nvme_poll_cb(void *opaque)
>>  {
>>      EventNotifier *e = opaque;
>> -    BDRVNVMeState *s = container_of(e, BDRVNVMeState, irq_notifier);
>> +    BDRVNVMeState *s = container_of(e, BDRVNVMeState,
>> +                                    irq_notifier[MSIX_SHARED_IRQ_IDX]);
>>  
>>      trace_nvme_poll_cb(s);
>>      return nvme_poll_queues(s);
>> @@ -705,7 +713,7 @@ static int nvme_init(BlockDriverState *bs, const char *device, int namespace,
>>      s->device = g_strdup(device);
>>      s->nsid = namespace;
>>      s->aio_context = bdrv_get_aio_context(bs);
>> -    ret = event_notifier_init(&s->irq_notifier, 0);
>> +    ret = event_notifier_init(&s->irq_notifier[MSIX_SHARED_IRQ_IDX], 0);
>>      if (ret) {
>>          error_setg(errp, "Failed to init event notifier");
>>          return ret;
>> @@ -784,12 +792,13 @@ static int nvme_init(BlockDriverState *bs, const char *device, int namespace,
>>          }
>>      }
>>  
>> -    ret = qemu_vfio_pci_init_irq(s->vfio, &s->irq_notifier,
>> +    ret = qemu_vfio_pci_init_irq(s->vfio, s->irq_notifier,
> 
> Maybe we can use '&s->irq_notifier[MSIX_SHARED_IRQ_IDX]' to match the other
> changes.

This makes the following patch in the next series (using multiple
queues) simpler, but if you prefer I don't mind using your suggestion
here, then adding another patch to directly use the array address
(instead of the address of the 1st element in that array). As you
wish :)

> 
>>                                   VFIO_PCI_MSIX_IRQ_INDEX, errp);
>>      if (ret) {
>>          goto out;
>>      }
>> -    aio_set_event_notifier(bdrv_get_aio_context(bs), &s->irq_notifier,
>> +    aio_set_event_notifier(bdrv_get_aio_context(bs),
>> +                           &s->irq_notifier[MSIX_SHARED_IRQ_IDX],
>>                             false, nvme_handle_event, nvme_poll_cb);
>>  
>>      nvme_identify(bs, namespace, &local_err);
>> @@ -872,9 +881,10 @@ static void nvme_close(BlockDriverState *bs)
>>          nvme_free_queue_pair(s->queues[i]);
>>      }
>>      g_free(s->queues);
>> -    aio_set_event_notifier(bdrv_get_aio_context(bs), &s->irq_notifier,
>> +    aio_set_event_notifier(bdrv_get_aio_context(bs),
>> +                           &s->irq_notifier[MSIX_SHARED_IRQ_IDX],
>>                             false, NULL, NULL);
>> -    event_notifier_cleanup(&s->irq_notifier);
>> +    event_notifier_cleanup(&s->irq_notifier[MSIX_SHARED_IRQ_IDX]);
>>      qemu_vfio_pci_unmap_bar(s->vfio, 0, (void *)s->regs, 0, NVME_BAR_SIZE);
>>      qemu_vfio_close(s->vfio);
>>  
>> @@ -1381,7 +1391,8 @@ static void nvme_detach_aio_context(BlockDriverState *bs)
>>          q->completion_bh = NULL;
>>      }
>>  
>> -    aio_set_event_notifier(bdrv_get_aio_context(bs), &s->irq_notifier,
>> +    aio_set_event_notifier(bdrv_get_aio_context(bs),
>> +                           &s->irq_notifier[MSIX_SHARED_IRQ_IDX],
>>                             false, NULL, NULL);
>>  }
>>  
>> @@ -1391,7 +1402,7 @@ static void nvme_attach_aio_context(BlockDriverState *bs,
>>      BDRVNVMeState *s = bs->opaque;
>>  
>>      s->aio_context = new_context;
>> -    aio_set_event_notifier(new_context, &s->irq_notifier,
>> +    aio_set_event_notifier(new_context, &s->irq_notifier[MSIX_SHARED_IRQ_IDX],
>>                             false, nvme_handle_event, nvme_poll_cb);
>>  
>>      for (int i = 0; i < s->nr_queues; i++) {
>> -- 
>> 2.26.2
>>
>>
> 

Re: [PATCH v5 12/15] block/nvme: Replace BDRV_POLL_WHILE by AIO_WAIT_WHILE

[Philippe Mathieu-Daudé]

On 8/21/20 12:15 PM, Stefano Garzarella wrote:
> On Thu, Aug 20, 2020 at 06:58:58PM +0200, Philippe Mathieu-Daudé wrote:
>> BDRV_POLL_WHILE() is defined as:
>>
>>   #define BDRV_POLL_WHILE(bs, cond) ({          \
>>       BlockDriverState *bs_ = (bs);             \
>>       AIO_WAIT_WHILE(bdrv_get_aio_context(bs_), \
>>                      cond); })
>>
>> As we will remove the BlockDriverState use in the next commit,
>> start by using the exploded version of BDRV_POLL_WHILE().
>>
>> Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>
>> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
>> ---
>>  block/nvme.c | 3 ++-
>>  1 file changed, 2 insertions(+), 1 deletion(-)
>>
>> diff --git a/block/nvme.c b/block/nvme.c
>> index 5b69fc75a60..456fe61f5ea 100644
>> --- a/block/nvme.c
>> +++ b/block/nvme.c
>> @@ -493,6 +493,7 @@ static void nvme_cmd_sync_cb(void *opaque, int ret)
>>  static int nvme_cmd_sync(BlockDriverState *bs, NVMeQueuePair *q,
>>                           NvmeCmd *cmd)
>>  {
>> +    AioContext *aio_context = bdrv_get_aio_context(bs);
>>      NVMeRequest *req;
>>      int ret = -EINPROGRESS;
>>      req = nvme_get_free_req(q);
>> @@ -501,7 +502,7 @@ static int nvme_cmd_sync(BlockDriverState *bs, NVMeQueuePair *q,
>>      }
>>      nvme_submit_command(q, req, cmd, nvme_cmd_sync_cb, &ret);
>>  
>> -    BDRV_POLL_WHILE(bs, ret == -EINPROGRESS);
>> +    AIO_WAIT_WHILE(aio_context, ret == -EINPROGRESS);
> 
> Maybe I would have:
> 
>     AIO_WAIT_WHILE(bdrv_get_aio_context(bs), ret == -EINPROGRESS);

I extracted aio_context in this patch because in the following
series it is passed by the caller as an argument to nvme_cmd_sync(),
so this makes the next series simpler to review.

> 
> But it doesn't matter, LGTM:
> 
> Reviewed-by: Stefano Garzarella <sgarzare@redhat.com>

Thanks!

> 
>>      return ret;
>>  }
>>  
>> -- 
>> 2.26.2
>>
>>
> 

Re: [PATCH v5 08/15] block/nvme: Use union of NvmeIdCtrl / NvmeIdNs structures

[Philippe Mathieu-Daudé]

On 8/21/20 12:03 PM, Stefano Garzarella wrote:
> On Thu, Aug 20, 2020 at 06:58:54PM +0200, Philippe Mathieu-Daudé wrote:
>> We allocate an unique chunk of memory then use it for two
>> different structures. By using an union, we make it clear
>> the data is overlapping (and we can remove the casts).
>>
>> Suggested-by: Stefan Hajnoczi <stefanha@redhat.com>
>> Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>
>> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
>> ---
>>  block/nvme.c | 31 +++++++++++++++----------------
>>  1 file changed, 15 insertions(+), 16 deletions(-)
>>
>> diff --git a/block/nvme.c b/block/nvme.c
>> index 99822d9fd36..2bd1935f951 100644
>> --- a/block/nvme.c
>> +++ b/block/nvme.c
>> @@ -508,9 +508,10 @@ static int nvme_cmd_sync(BlockDriverState *bs, NVMeQueuePair *q,
>>  static void nvme_identify(BlockDriverState *bs, int namespace, Error **errp)
>>  {
>>      BDRVNVMeState *s = bs->opaque;
>> -    NvmeIdCtrl *idctrl;
>> -    NvmeIdNs *idns;
>> -    uint8_t *id;
>> +    union {
>> +        NvmeIdCtrl ctrl;
>> +        NvmeIdNs ns;
>> +    } *id;
> 
> What about defining a new 'NvmeId' type with this union?

I'd rather not, these are different command responses, it
just happens to make this code simpler as the same response
packet is used for the 2 requests.

See previous discussion:
https://www.mail-archive.com/qemu-devel@nongnu.org/msg716858.html

> 
>>      NvmeLBAF *lbaf;
>>      uint16_t oncs;
>>      int r;
>> @@ -520,14 +521,12 @@ static void nvme_identify(BlockDriverState *bs, int namespace, Error **errp)
>>          .cdw10 = cpu_to_le32(0x1),
>>      };
>>  
>> -    id = qemu_try_blockalign0(bs, sizeof(NvmeIdCtrl));
>> +    id = qemu_try_blockalign0(bs, sizeof(*id));
>>      if (!id) {
>>          error_setg(errp, "Cannot allocate buffer for identify response");
>>          goto out;
>>      }
>> -    idctrl = (NvmeIdCtrl *)id;
>> -    idns = (NvmeIdNs *)id;
>> -    r = qemu_vfio_dma_map(s->vfio, id, sizeof(NvmeIdCtrl), true, &iova);
>> +    r = qemu_vfio_dma_map(s->vfio, id, sizeof(*id), true, &iova);
>>      if (r) {
>>          error_setg(errp, "Cannot map buffer for DMA");
>>          goto out;
>> @@ -539,22 +538,22 @@ static void nvme_identify(BlockDriverState *bs, int namespace, Error **errp)
>>          goto out;
>>      }
>>  
>> -    if (le32_to_cpu(idctrl->nn) < namespace) {
>> +    if (le32_to_cpu(id->ctrl.nn) < namespace) {
>>          error_setg(errp, "Invalid namespace");
>>          goto out;
>>      }
>> -    s->write_cache_supported = le32_to_cpu(idctrl->vwc) & 0x1;
>> -    s->max_transfer = (idctrl->mdts ? 1 << idctrl->mdts : 0) * s->page_size;
>> +    s->write_cache_supported = le32_to_cpu(id->ctrl.vwc) & 0x1;
>> +    s->max_transfer = (id->ctrl.mdts ? 1 << id->ctrl.mdts : 0) * s->page_size;
>>      /* For now the page list buffer per command is one page, to hold at most
>>       * s->page_size / sizeof(uint64_t) entries. */
>>      s->max_transfer = MIN_NON_ZERO(s->max_transfer,
>>                            s->page_size / sizeof(uint64_t) * s->page_size);
>>  
>> -    oncs = le16_to_cpu(idctrl->oncs);
>> +    oncs = le16_to_cpu(id->ctrl.oncs);
>>      s->supports_write_zeroes = !!(oncs & NVME_ONCS_WRITE_ZEROS);
>>      s->supports_discard = !!(oncs & NVME_ONCS_DSM);
>>  
>> -    memset(id, 0, 4096);
>> +    memset(id, 0, sizeof(*id));
>>      cmd.cdw10 = 0;
>>      cmd.nsid = cpu_to_le32(namespace);
>>      if (nvme_cmd_sync(bs, s->queues[INDEX_ADMIN], &cmd)) {
>> @@ -562,11 +561,11 @@ static void nvme_identify(BlockDriverState *bs, int namespace, Error **errp)
>>          goto out;
>>      }
>>  
>> -    s->nsze = le64_to_cpu(idns->nsze);
>> -    lbaf = &idns->lbaf[NVME_ID_NS_FLBAS_INDEX(idns->flbas)];
>> +    s->nsze = le64_to_cpu(id->ns.nsze);
>> +    lbaf = &id->ns.lbaf[NVME_ID_NS_FLBAS_INDEX(id->ns.flbas)];
>>  
>> -    if (NVME_ID_NS_DLFEAT_WRITE_ZEROES(idns->dlfeat) &&
>> -            NVME_ID_NS_DLFEAT_READ_BEHAVIOR(idns->dlfeat) ==
>> +    if (NVME_ID_NS_DLFEAT_WRITE_ZEROES(id->ns.dlfeat) &&
>> +            NVME_ID_NS_DLFEAT_READ_BEHAVIOR(id->ns.dlfeat) ==
>>                      NVME_ID_NS_DLFEAT_READ_BEHAVIOR_ZEROES) {
>>          bs->supported_write_flags |= BDRV_REQ_MAY_UNMAP;
>>      }
>> -- 
>> 2.26.2
>>
>>
> 
> With or without the new tyoe, the patch looks good to me:
> 
> Reviewed-by: Stefano Garzarella <sgarzare@redhat.com>

Thanks!

Phil.

Re: [PATCH v5 03/15] block/nvme: Let nvme_create_queue_pair() fail gracefully

[Philippe Mathieu-Daudé]

On 8/21/20 11:44 AM, Stefano Garzarella wrote:
> On Thu, Aug 20, 2020 at 06:58:49PM +0200, Philippe Mathieu-Daudé wrote:
>> As nvme_create_queue_pair() is allowed to fail, replace the
>> alloc() calls by try_alloc() to avoid aborting QEMU.
>>
>> Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>
>> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
>> ---
>>  block/nvme.c | 12 ++++++++++--
>>  1 file changed, 10 insertions(+), 2 deletions(-)
>>
>> diff --git a/block/nvme.c b/block/nvme.c
>> index 8c30a5fee28..e1893b4e792 100644
>> --- a/block/nvme.c
>> +++ b/block/nvme.c
>> @@ -213,14 +213,22 @@ static NVMeQueuePair *nvme_create_queue_pair(BlockDriverState *bs,
>>      int i, r;
>>      BDRVNVMeState *s = bs->opaque;
>>      Error *local_err = NULL;
>> -    NVMeQueuePair *q = g_new0(NVMeQueuePair, 1);
>> +    NVMeQueuePair *q;
>>      uint64_t prp_list_iova;
>>  
>> +    q = g_try_new0(NVMeQueuePair, 1);
>> +    if (!q) {
>> +        return NULL;
>> +    }
>> +    q->prp_list_pages = qemu_try_blockalign0(bs,
>> +                                          s->page_size * NVME_QUEUE_SIZE);
> 
> Here you use NVME_QUEUE_SIZE instead of NVME_NUM_REQS, is that an
> intentional change?

No... Thanks for spotting this, I missed it because git didn't
emit any warning while rebasing on top of "block/nvme: support nested
aio_poll".
This value has been changed in 1086e95da17 ("block/nvme: switch to a
NVMeRequest freelist").

Good catch!
I'll respin (after reviewing the 'nested aio_poll' changes).

> 
> Maybe is not an issue, sice NVME_QUEUE_SIZE is bigger than
> NVME_NUM_REQS, but we should mention in the commit message.
> 
> Thanks,
> Stefano
> 
>> +    if (!q->prp_list_pages) {
>> +        goto fail;
>> +    }
>>      qemu_mutex_init(&q->lock);
>>      q->s = s;
>>      q->index = idx;
>>      qemu_co_queue_init(&q->free_req_queue);
>> -    q->prp_list_pages = qemu_blockalign0(bs, s->page_size * NVME_NUM_REQS);
>>      q->completion_bh = aio_bh_new(bdrv_get_aio_context(bs),
>>                                    nvme_process_completion_bh, q);
>>      r = qemu_vfio_dma_map(s->vfio, q->prp_list_pages,
>> -- 
>> 2.26.2
>>
>>
> 

Re: [PATCH v5 15/15] block/nvme: Use an array of EventNotifier

[Stefano Garzarella]

On Fri, Aug 21, 2020 at 03:09:13PM +0200, Philippe Mathieu-Daudé wrote:
> On 8/21/20 12:29 PM, Stefano Garzarella wrote:
> > On Thu, Aug 20, 2020 at 06:59:01PM +0200, Philippe Mathieu-Daudé wrote:
> >> In preparation of using multiple IRQ (thus multiple eventfds)
> >> make BDRVNVMeState::irq_notifier an array (for now of a single
> >> element, the admin queue notifier).
> >>
> >> Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>
> >> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
> >> ---
> >>  block/nvme.c | 31 +++++++++++++++++++++----------
> >>  1 file changed, 21 insertions(+), 10 deletions(-)
> >>
> >> diff --git a/block/nvme.c b/block/nvme.c
> >> index a61e86a83eb..fe8a40b7ede 100644
> >> --- a/block/nvme.c
> >> +++ b/block/nvme.c
> >> @@ -106,6 +106,12 @@ QEMU_BUILD_BUG_ON(offsetof(NVMeRegs, doorbells) != 0x1000);
> >>  #define INDEX_ADMIN     0
> >>  #define INDEX_IO(n)     (1 + n)
> >>  
> >> +/* This driver shares a single MSIX IRQ for the admin and I/O queues */
> >> +enum {
> >> +    MSIX_SHARED_IRQ_IDX = 0,
> >> +    MSIX_IRQ_COUNT = 1
> >> +};
> >> +
> >>  struct BDRVNVMeState {
> >>      AioContext *aio_context;
> >>      QEMUVFIOState *vfio;
> >> @@ -120,7 +126,7 @@ struct BDRVNVMeState {
> >>      /* How many uint32_t elements does each doorbell entry take. */
> >>      size_t doorbell_scale;
> >>      bool write_cache_supported;
> >> -    EventNotifier irq_notifier;
> >> +    EventNotifier irq_notifier[MSIX_IRQ_COUNT];
> >>  
> >>      uint64_t nsze; /* Namespace size reported by identify command */
> >>      int nsid;      /* The namespace id to read/write data. */
> >> @@ -631,7 +637,8 @@ static bool nvme_poll_queues(BDRVNVMeState *s)
> >>  
> >>  static void nvme_handle_event(EventNotifier *n)
> >>  {
> >> -    BDRVNVMeState *s = container_of(n, BDRVNVMeState, irq_notifier);
> >> +    BDRVNVMeState *s = container_of(n, BDRVNVMeState,
> >> +                                    irq_notifier[MSIX_SHARED_IRQ_IDX]);
> >>  
> >>      trace_nvme_handle_event(s);
> >>      event_notifier_test_and_clear(n);
> >> @@ -683,7 +690,8 @@ out_error:
> >>  static bool nvme_poll_cb(void *opaque)
> >>  {
> >>      EventNotifier *e = opaque;
> >> -    BDRVNVMeState *s = container_of(e, BDRVNVMeState, irq_notifier);
> >> +    BDRVNVMeState *s = container_of(e, BDRVNVMeState,
> >> +                                    irq_notifier[MSIX_SHARED_IRQ_IDX]);
> >>  
> >>      trace_nvme_poll_cb(s);
> >>      return nvme_poll_queues(s);
> >> @@ -705,7 +713,7 @@ static int nvme_init(BlockDriverState *bs, const char *device, int namespace,
> >>      s->device = g_strdup(device);
> >>      s->nsid = namespace;
> >>      s->aio_context = bdrv_get_aio_context(bs);
> >> -    ret = event_notifier_init(&s->irq_notifier, 0);
> >> +    ret = event_notifier_init(&s->irq_notifier[MSIX_SHARED_IRQ_IDX], 0);
> >>      if (ret) {
> >>          error_setg(errp, "Failed to init event notifier");
> >>          return ret;
> >> @@ -784,12 +792,13 @@ static int nvme_init(BlockDriverState *bs, const char *device, int namespace,
> >>          }
> >>      }
> >>  
> >> -    ret = qemu_vfio_pci_init_irq(s->vfio, &s->irq_notifier,
> >> +    ret = qemu_vfio_pci_init_irq(s->vfio, s->irq_notifier,
> > 
> > Maybe we can use '&s->irq_notifier[MSIX_SHARED_IRQ_IDX]' to match the other
> > changes.
> 
> This makes the following patch in the next series (using multiple
> queues) simpler, but if you prefer I don't mind using your suggestion
> here, then adding another patch to directly use the array address
> (instead of the address of the 1st element in that array). As you
> wish :)

If it simplifies the next patches, it's fine for me ;-)

Reviewed-by: Stefano Garzarella <sgarzare@redhat.com>

> 
> > 
> >>                                   VFIO_PCI_MSIX_IRQ_INDEX, errp);
> >>      if (ret) {
> >>          goto out;
> >>      }
> >> -    aio_set_event_notifier(bdrv_get_aio_context(bs), &s->irq_notifier,
> >> +    aio_set_event_notifier(bdrv_get_aio_context(bs),
> >> +                           &s->irq_notifier[MSIX_SHARED_IRQ_IDX],
> >>                             false, nvme_handle_event, nvme_poll_cb);
> >>  
> >>      nvme_identify(bs, namespace, &local_err);
> >> @@ -872,9 +881,10 @@ static void nvme_close(BlockDriverState *bs)
> >>          nvme_free_queue_pair(s->queues[i]);
> >>      }
> >>      g_free(s->queues);
> >> -    aio_set_event_notifier(bdrv_get_aio_context(bs), &s->irq_notifier,
> >> +    aio_set_event_notifier(bdrv_get_aio_context(bs),
> >> +                           &s->irq_notifier[MSIX_SHARED_IRQ_IDX],
> >>                             false, NULL, NULL);
> >> -    event_notifier_cleanup(&s->irq_notifier);
> >> +    event_notifier_cleanup(&s->irq_notifier[MSIX_SHARED_IRQ_IDX]);
> >>      qemu_vfio_pci_unmap_bar(s->vfio, 0, (void *)s->regs, 0, NVME_BAR_SIZE);
> >>      qemu_vfio_close(s->vfio);
> >>  
> >> @@ -1381,7 +1391,8 @@ static void nvme_detach_aio_context(BlockDriverState *bs)
> >>          q->completion_bh = NULL;
> >>      }
> >>  
> >> -    aio_set_event_notifier(bdrv_get_aio_context(bs), &s->irq_notifier,
> >> +    aio_set_event_notifier(bdrv_get_aio_context(bs),
> >> +                           &s->irq_notifier[MSIX_SHARED_IRQ_IDX],
> >>                             false, NULL, NULL);
> >>  }
> >>  
> >> @@ -1391,7 +1402,7 @@ static void nvme_attach_aio_context(BlockDriverState *bs,
> >>      BDRVNVMeState *s = bs->opaque;
> >>  
> >>      s->aio_context = new_context;
> >> -    aio_set_event_notifier(new_context, &s->irq_notifier,
> >> +    aio_set_event_notifier(new_context, &s->irq_notifier[MSIX_SHARED_IRQ_IDX],
> >>                             false, nvme_handle_event, nvme_poll_cb);
> >>  
> >>      for (int i = 0; i < s->nr_queues; i++) {
> >> -- 
> >> 2.26.2
> >>
> >>
> > 
> 

Re: [PATCH v5 12/15] block/nvme: Replace BDRV_POLL_WHILE by AIO_WAIT_WHILE

[Stefano Garzarella]

On Fri, Aug 21, 2020 at 03:15:58PM +0200, Philippe Mathieu-Daudé wrote:
> On 8/21/20 12:15 PM, Stefano Garzarella wrote:
> > On Thu, Aug 20, 2020 at 06:58:58PM +0200, Philippe Mathieu-Daudé wrote:
> >> BDRV_POLL_WHILE() is defined as:
> >>
> >>   #define BDRV_POLL_WHILE(bs, cond) ({          \
> >>       BlockDriverState *bs_ = (bs);             \
> >>       AIO_WAIT_WHILE(bdrv_get_aio_context(bs_), \
> >>                      cond); })
> >>
> >> As we will remove the BlockDriverState use in the next commit,
> >> start by using the exploded version of BDRV_POLL_WHILE().
> >>
> >> Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>
> >> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
> >> ---
> >>  block/nvme.c | 3 ++-
> >>  1 file changed, 2 insertions(+), 1 deletion(-)
> >>
> >> diff --git a/block/nvme.c b/block/nvme.c
> >> index 5b69fc75a60..456fe61f5ea 100644
> >> --- a/block/nvme.c
> >> +++ b/block/nvme.c
> >> @@ -493,6 +493,7 @@ static void nvme_cmd_sync_cb(void *opaque, int ret)
> >>  static int nvme_cmd_sync(BlockDriverState *bs, NVMeQueuePair *q,
> >>                           NvmeCmd *cmd)
> >>  {
> >> +    AioContext *aio_context = bdrv_get_aio_context(bs);
> >>      NVMeRequest *req;
> >>      int ret = -EINPROGRESS;
> >>      req = nvme_get_free_req(q);
> >> @@ -501,7 +502,7 @@ static int nvme_cmd_sync(BlockDriverState *bs, NVMeQueuePair *q,
> >>      }
> >>      nvme_submit_command(q, req, cmd, nvme_cmd_sync_cb, &ret);
> >>  
> >> -    BDRV_POLL_WHILE(bs, ret == -EINPROGRESS);
> >> +    AIO_WAIT_WHILE(aio_context, ret == -EINPROGRESS);
> > 
> > Maybe I would have:
> > 
> >     AIO_WAIT_WHILE(bdrv_get_aio_context(bs), ret == -EINPROGRESS);
> 
> I extracted aio_context in this patch because in the following
> series it is passed by the caller as an argument to nvme_cmd_sync(),
> so this makes the next series simpler to review.

Make sense!

> 
> > 
> > But it doesn't matter, LGTM:
> > 
> > Reviewed-by: Stefano Garzarella <sgarzare@redhat.com>
> 
> Thanks!
> 
> > 
> >>      return ret;
> >>  }
> >>  
> >> -- 
> >> 2.26.2
> >>
> >>
> > 
> 

Re: [PATCH v5 08/15] block/nvme: Use union of NvmeIdCtrl / NvmeIdNs structures

[Stefano Garzarella]

On Fri, Aug 21, 2020 at 03:27:15PM +0200, Philippe Mathieu-Daudé wrote:
> On 8/21/20 12:03 PM, Stefano Garzarella wrote:
> > On Thu, Aug 20, 2020 at 06:58:54PM +0200, Philippe Mathieu-Daudé wrote:
> >> We allocate an unique chunk of memory then use it for two
> >> different structures. By using an union, we make it clear
> >> the data is overlapping (and we can remove the casts).
> >>
> >> Suggested-by: Stefan Hajnoczi <stefanha@redhat.com>
> >> Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>
> >> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
> >> ---
> >>  block/nvme.c | 31 +++++++++++++++----------------
> >>  1 file changed, 15 insertions(+), 16 deletions(-)
> >>
> >> diff --git a/block/nvme.c b/block/nvme.c
> >> index 99822d9fd36..2bd1935f951 100644
> >> --- a/block/nvme.c
> >> +++ b/block/nvme.c
> >> @@ -508,9 +508,10 @@ static int nvme_cmd_sync(BlockDriverState *bs, NVMeQueuePair *q,
> >>  static void nvme_identify(BlockDriverState *bs, int namespace, Error **errp)
> >>  {
> >>      BDRVNVMeState *s = bs->opaque;
> >> -    NvmeIdCtrl *idctrl;
> >> -    NvmeIdNs *idns;
> >> -    uint8_t *id;
> >> +    union {
> >> +        NvmeIdCtrl ctrl;
> >> +        NvmeIdNs ns;
> >> +    } *id;
> > 
> > What about defining a new 'NvmeId' type with this union?
> 
> I'd rather not, these are different command responses, it
> just happens to make this code simpler as the same response
> packet is used for the 2 requests.
> 
> See previous discussion:
> https://www.mail-archive.com/qemu-devel@nongnu.org/msg716858.html

Yeah, if it is useful only in this part of the code, never mind defining a
new type.

> 
> > 
> >>      NvmeLBAF *lbaf;
> >>      uint16_t oncs;
> >>      int r;
> >> @@ -520,14 +521,12 @@ static void nvme_identify(BlockDriverState *bs, int namespace, Error **errp)
> >>          .cdw10 = cpu_to_le32(0x1),
> >>      };
> >>  
> >> -    id = qemu_try_blockalign0(bs, sizeof(NvmeIdCtrl));
> >> +    id = qemu_try_blockalign0(bs, sizeof(*id));
> >>      if (!id) {
> >>          error_setg(errp, "Cannot allocate buffer for identify response");
> >>          goto out;
> >>      }
> >> -    idctrl = (NvmeIdCtrl *)id;
> >> -    idns = (NvmeIdNs *)id;
> >> -    r = qemu_vfio_dma_map(s->vfio, id, sizeof(NvmeIdCtrl), true, &iova);
> >> +    r = qemu_vfio_dma_map(s->vfio, id, sizeof(*id), true, &iova);
> >>      if (r) {
> >>          error_setg(errp, "Cannot map buffer for DMA");
> >>          goto out;
> >> @@ -539,22 +538,22 @@ static void nvme_identify(BlockDriverState *bs, int namespace, Error **errp)
> >>          goto out;
> >>      }
> >>  
> >> -    if (le32_to_cpu(idctrl->nn) < namespace) {
> >> +    if (le32_to_cpu(id->ctrl.nn) < namespace) {
> >>          error_setg(errp, "Invalid namespace");
> >>          goto out;
> >>      }
> >> -    s->write_cache_supported = le32_to_cpu(idctrl->vwc) & 0x1;
> >> -    s->max_transfer = (idctrl->mdts ? 1 << idctrl->mdts : 0) * s->page_size;
> >> +    s->write_cache_supported = le32_to_cpu(id->ctrl.vwc) & 0x1;
> >> +    s->max_transfer = (id->ctrl.mdts ? 1 << id->ctrl.mdts : 0) * s->page_size;
> >>      /* For now the page list buffer per command is one page, to hold at most
> >>       * s->page_size / sizeof(uint64_t) entries. */
> >>      s->max_transfer = MIN_NON_ZERO(s->max_transfer,
> >>                            s->page_size / sizeof(uint64_t) * s->page_size);
> >>  
> >> -    oncs = le16_to_cpu(idctrl->oncs);
> >> +    oncs = le16_to_cpu(id->ctrl.oncs);
> >>      s->supports_write_zeroes = !!(oncs & NVME_ONCS_WRITE_ZEROS);
> >>      s->supports_discard = !!(oncs & NVME_ONCS_DSM);
> >>  
> >> -    memset(id, 0, 4096);
> >> +    memset(id, 0, sizeof(*id));
> >>      cmd.cdw10 = 0;
> >>      cmd.nsid = cpu_to_le32(namespace);
> >>      if (nvme_cmd_sync(bs, s->queues[INDEX_ADMIN], &cmd)) {
> >> @@ -562,11 +561,11 @@ static void nvme_identify(BlockDriverState *bs, int namespace, Error **errp)
> >>          goto out;
> >>      }
> >>  
> >> -    s->nsze = le64_to_cpu(idns->nsze);
> >> -    lbaf = &idns->lbaf[NVME_ID_NS_FLBAS_INDEX(idns->flbas)];
> >> +    s->nsze = le64_to_cpu(id->ns.nsze);
> >> +    lbaf = &id->ns.lbaf[NVME_ID_NS_FLBAS_INDEX(id->ns.flbas)];
> >>  
> >> -    if (NVME_ID_NS_DLFEAT_WRITE_ZEROES(idns->dlfeat) &&
> >> -            NVME_ID_NS_DLFEAT_READ_BEHAVIOR(idns->dlfeat) ==
> >> +    if (NVME_ID_NS_DLFEAT_WRITE_ZEROES(id->ns.dlfeat) &&
> >> +            NVME_ID_NS_DLFEAT_READ_BEHAVIOR(id->ns.dlfeat) ==
> >>                      NVME_ID_NS_DLFEAT_READ_BEHAVIOR_ZEROES) {
> >>          bs->supported_write_flags |= BDRV_REQ_MAY_UNMAP;
> >>      }
> >> -- 
> >> 2.26.2
> >>
> >>
> > 
> > With or without the new tyoe, the patch looks good to me:
> > 
> > Reviewed-by: Stefano Garzarella <sgarzare@redhat.com>
> 
> Thanks!
> 
> Phil.
> 

Re: [PATCH v5 03/15] block/nvme: Let nvme_create_queue_pair() fail gracefully

[Stefano Garzarella]

On Fri, Aug 21, 2020 at 03:36:47PM +0200, Philippe Mathieu-Daudé wrote:
> On 8/21/20 11:44 AM, Stefano Garzarella wrote:
> > On Thu, Aug 20, 2020 at 06:58:49PM +0200, Philippe Mathieu-Daudé wrote:
> >> As nvme_create_queue_pair() is allowed to fail, replace the
> >> alloc() calls by try_alloc() to avoid aborting QEMU.
> >>
> >> Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>
> >> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
> >> ---
> >>  block/nvme.c | 12 ++++++++++--
> >>  1 file changed, 10 insertions(+), 2 deletions(-)
> >>
> >> diff --git a/block/nvme.c b/block/nvme.c
> >> index 8c30a5fee28..e1893b4e792 100644
> >> --- a/block/nvme.c
> >> +++ b/block/nvme.c
> >> @@ -213,14 +213,22 @@ static NVMeQueuePair *nvme_create_queue_pair(BlockDriverState *bs,
> >>      int i, r;
> >>      BDRVNVMeState *s = bs->opaque;
> >>      Error *local_err = NULL;
> >> -    NVMeQueuePair *q = g_new0(NVMeQueuePair, 1);
> >> +    NVMeQueuePair *q;
> >>      uint64_t prp_list_iova;
> >>  
> >> +    q = g_try_new0(NVMeQueuePair, 1);
> >> +    if (!q) {
> >> +        return NULL;
> >> +    }
> >> +    q->prp_list_pages = qemu_try_blockalign0(bs,
> >> +                                          s->page_size * NVME_QUEUE_SIZE);
> > 
> > Here you use NVME_QUEUE_SIZE instead of NVME_NUM_REQS, is that an
> > intentional change?
> 
> No... Thanks for spotting this, I missed it because git didn't
> emit any warning while rebasing on top of "block/nvme: support nested
> aio_poll".
> This value has been changed in 1086e95da17 ("block/nvme: switch to a
> NVMeRequest freelist").
> 
> Good catch!
> I'll respin (after reviewing the 'nested aio_poll' changes).

Cool, with that fixed the patch LGTM:

Reviewed-by: Stefano Garzarella <sgarzare@redhat.com>

> 
> > 
> > Maybe is not an issue, sice NVME_QUEUE_SIZE is bigger than
> > NVME_NUM_REQS, but we should mention in the commit message.
> > 
> > Thanks,
> > Stefano
> > 
> >> +    if (!q->prp_list_pages) {
> >> +        goto fail;
> >> +    }
> >>      qemu_mutex_init(&q->lock);
> >>      q->s = s;
> >>      q->index = idx;
> >>      qemu_co_queue_init(&q->free_req_queue);
> >> -    q->prp_list_pages = qemu_blockalign0(bs, s->page_size * NVME_NUM_REQS);
> >>      q->completion_bh = aio_bh_new(bdrv_get_aio_context(bs),
> >>                                    nvme_process_completion_bh, q);
> >>      r = qemu_vfio_dma_map(s->vfio, q->prp_list_pages,
> >> -- 
> >> 2.26.2
> >>
> >>
> > 
>