[PATCH 0/3] block/nvme: Use NvmeBar structure from "block/nvme.h"

[PATCH 0/3] block/nvme: Use NvmeBar structure from "block/nvme.h"

[Philippe Mathieu-Daudé]

Cleanups in the NVMeRegs structure:
- Use the already existing NvmeBar structure from "block/nvme.h"
- Pair doorbell registers

Based-on: <20200903122803.405265-1-philmd@redhat.com>

Philippe Mathieu-Daudé (3):
  block/nvme: Group controller registers in NVMeRegs structure
  block/nvme: Use generic NvmeBar structure
  block/nvme: Pair doorbell registers

 block/nvme.c | 43 +++++++++++++++----------------------------
 1 file changed, 15 insertions(+), 28 deletions(-)

-- 
2.26.2

[PATCH 1/3] block/nvme: Group controller registers in NVMeRegs structure

[Philippe Mathieu-Daudé]

We want to use the NvmeBar structure from "block/nvme.h" in the
next commit. As a preliminary step, group all the NVMe controller
registers in the 'ctrl' field, keeping the doorbells registers
out of it.

Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
---
 block/nvme.c | 48 +++++++++++++++++++++++++-----------------------
 1 file changed, 25 insertions(+), 23 deletions(-)

diff --git a/block/nvme.c b/block/nvme.c
index 24e6e7f0866..c9c3fc02fed 100644
--- a/block/nvme.c
+++ b/block/nvme.c
@@ -83,21 +83,23 @@ typedef struct {
 
 /* Memory mapped registers */
 typedef volatile struct {
-    uint64_t cap;
-    uint32_t vs;
-    uint32_t intms;
-    uint32_t intmc;
-    uint32_t cc;
-    uint32_t reserved0;
-    uint32_t csts;
-    uint32_t nssr;
-    uint32_t aqa;
-    uint64_t asq;
-    uint64_t acq;
-    uint32_t cmbloc;
-    uint32_t cmbsz;
-    uint8_t  reserved1[0xec0];
-    uint8_t  cmd_set_specfic[0x100];
+    struct {
+        uint64_t cap;
+        uint32_t vs;
+        uint32_t intms;
+        uint32_t intmc;
+        uint32_t cc;
+        uint32_t reserved0;
+        uint32_t csts;
+        uint32_t nssr;
+        uint32_t aqa;
+        uint64_t asq;
+        uint64_t acq;
+        uint32_t cmbloc;
+        uint32_t cmbsz;
+        uint8_t  reserved1[0xec0];
+        uint8_t  cmd_set_specfic[0x100];
+    } ctrl;
     uint32_t doorbells[];
 } NVMeRegs;
 
@@ -734,7 +736,7 @@ static int nvme_init(BlockDriverState *bs, const char *device, int namespace,
     /* Perform initialize sequence as described in NVMe spec "7.6.1
      * Initialization". */
 
-    cap = le64_to_cpu(s->regs->cap);
+    cap = le64_to_cpu(s->regs->ctrl.cap);
     if (!(cap & (1ULL << 37))) {
         error_setg(errp, "Device doesn't support NVMe command set");
         ret = -EINVAL;
@@ -747,10 +749,10 @@ static int nvme_init(BlockDriverState *bs, const char *device, int namespace,
     timeout_ms = MIN(500 * ((cap >> 24) & 0xFF), 30000);
 
     /* Reset device to get a clean state. */
-    s->regs->cc = cpu_to_le32(le32_to_cpu(s->regs->cc) & 0xFE);
+    s->regs->ctrl.cc = cpu_to_le32(le32_to_cpu(s->regs->ctrl.cc) & 0xFE);
     /* Wait for CSTS.RDY = 0. */
     deadline = qemu_clock_get_ns(QEMU_CLOCK_REALTIME) + timeout_ms * SCALE_MS;
-    while (le32_to_cpu(s->regs->csts) & 0x1) {
+    while (le32_to_cpu(s->regs->ctrl.csts) & 0x1) {
         if (qemu_clock_get_ns(QEMU_CLOCK_REALTIME) > deadline) {
             error_setg(errp, "Timeout while waiting for device to reset (%"
                              PRId64 " ms)",
@@ -771,18 +773,18 @@ static int nvme_init(BlockDriverState *bs, const char *device, int namespace,
     }
     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[INDEX_ADMIN]->sq.iova);
-    s->regs->acq = cpu_to_le64(s->queues[INDEX_ADMIN]->cq.iova);
+    s->regs->ctrl.aqa = cpu_to_le32((NVME_QUEUE_SIZE << 16) | NVME_QUEUE_SIZE);
+    s->regs->ctrl.asq = cpu_to_le64(s->queues[INDEX_ADMIN]->sq.iova);
+    s->regs->ctrl.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) |
+    s->regs->ctrl.cc = cpu_to_le32((ctz32(NVME_CQ_ENTRY_BYTES) << 20) |
                               (ctz32(NVME_SQ_ENTRY_BYTES) << 16) |
                               0x1);
     /* Wait for CSTS.RDY = 1. */
     now = qemu_clock_get_ns(QEMU_CLOCK_REALTIME);
     deadline = now + timeout_ms * 1000000;
-    while (!(le32_to_cpu(s->regs->csts) & 0x1)) {
+    while (!(le32_to_cpu(s->regs->ctrl.csts) & 0x1)) {
         if (qemu_clock_get_ns(QEMU_CLOCK_REALTIME) > deadline) {
             error_setg(errp, "Timeout while waiting for device to start (%"
                              PRId64 " ms)",
-- 
2.26.2

[PATCH 2/3] block/nvme: Use generic NvmeBar structure

[Philippe Mathieu-Daudé]

Commit f3c507adcd7 ("NVMe: Initial commit for new storage interface")
introduced the NvmeBar structure. Unfortunately in commit bdd6a90a9e5
("block: Add VFIO based NVMe driver") we duplicated it.

Apparently in commit a3d9a352d48 ("block: Move NVMe constants to
a separate header") we tried to unify headers but forgot to remove
the structure declared in the block/nvme.c source file.

Do it now, and remove the structure size check which is redundant
with the header check added in commit 74e18435c0e ("hw/block/nvme:
Align I/O BAR to 4 KiB").

Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
---
 block/nvme.c | 20 +-------------------
 1 file changed, 1 insertion(+), 19 deletions(-)

diff --git a/block/nvme.c b/block/nvme.c
index c9c3fc02fed..a216cc407f6 100644
--- a/block/nvme.c
+++ b/block/nvme.c
@@ -83,28 +83,10 @@ typedef struct {
 
 /* Memory mapped registers */
 typedef volatile struct {
-    struct {
-        uint64_t cap;
-        uint32_t vs;
-        uint32_t intms;
-        uint32_t intmc;
-        uint32_t cc;
-        uint32_t reserved0;
-        uint32_t csts;
-        uint32_t nssr;
-        uint32_t aqa;
-        uint64_t asq;
-        uint64_t acq;
-        uint32_t cmbloc;
-        uint32_t cmbsz;
-        uint8_t  reserved1[0xec0];
-        uint8_t  cmd_set_specfic[0x100];
-    } ctrl;
+    NvmeBar ctrl;
     uint32_t doorbells[];
 } NVMeRegs;
 
-QEMU_BUILD_BUG_ON(offsetof(NVMeRegs, doorbells) != 0x1000);
-
 #define INDEX_ADMIN     0
 #define INDEX_IO(n)     (1 + n)
 
-- 
2.26.2

[PATCH 3/3] block/nvme: Pair doorbell registers

[Philippe Mathieu-Daudé]

For each queue doorbell registers are paired as:
- Submission Queue Tail Doorbell
- Completion Queue Head Doorbell

Reflect that in the NVMeRegs structure, and adapt
nvme_create_queue_pair() accordingly.

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

diff --git a/block/nvme.c b/block/nvme.c
index a216cc407f6..f4f27b6da7d 100644
--- a/block/nvme.c
+++ b/block/nvme.c
@@ -84,7 +84,10 @@ typedef struct {
 /* Memory mapped registers */
 typedef volatile struct {
     NvmeBar ctrl;
-    uint32_t doorbells[];
+    struct {
+        uint32_t sq_tail;
+        uint32_t cq_head;
+    } doorbells[];
 } NVMeRegs;
 
 #define INDEX_ADMIN     0
@@ -244,14 +247,14 @@ static NVMeQueuePair *nvme_create_queue_pair(BDRVNVMeState *s,
         error_propagate(errp, local_err);
         goto fail;
     }
-    q->sq.doorbell = &s->regs->doorbells[idx * 2 * s->doorbell_scale];
+    q->sq.doorbell = &s->regs->doorbells[idx * s->doorbell_scale].sq_tail;
 
     nvme_init_queue(s, &q->cq, size, NVME_CQ_ENTRY_BYTES, &local_err);
     if (local_err) {
         error_propagate(errp, local_err);
         goto fail;
     }
-    q->cq.doorbell = &s->regs->doorbells[(idx * 2 + 1) * s->doorbell_scale];
+    q->cq.doorbell = &s->regs->doorbells[idx * s->doorbell_scale].cq_head;
 
     return q;
 fail:
-- 
2.26.2

Re: [PATCH 0/3] block/nvme: Use NvmeBar structure from "block/nvme.h"

[Fam Zheng]

On 2020-09-04 14:41, Philippe Mathieu-Daudé wrote:
> Cleanups in the NVMeRegs structure:
> - Use the already existing NvmeBar structure from "block/nvme.h"
> - Pair doorbell registers
> 
> Based-on: <20200903122803.405265-1-philmd@redhat.com>
> 
> Philippe Mathieu-Daudé (3):
>   block/nvme: Group controller registers in NVMeRegs structure
>   block/nvme: Use generic NvmeBar structure
>   block/nvme: Pair doorbell registers
> 
>  block/nvme.c | 43 +++++++++++++++----------------------------
>  1 file changed, 15 insertions(+), 28 deletions(-)
> 
> -- 
> 2.26.2
> 
> 

Reviewed-by: Fam Zheng <fam@euphon.net>

Re: [PATCH 0/3] block/nvme: Use NvmeBar structure from "block/nvme.h"

[Klaus Jensen]

[-- Attachment #1: Type: text/plain, Size: 616 bytes --]

On Sep  4 14:41, Philippe Mathieu-Daudé wrote:
> Cleanups in the NVMeRegs structure:
> - Use the already existing NvmeBar structure from "block/nvme.h"
> - Pair doorbell registers
> 
> Based-on: <20200903122803.405265-1-philmd@redhat.com>
> 
> Philippe Mathieu-Daudé (3):
>   block/nvme: Group controller registers in NVMeRegs structure
>   block/nvme: Use generic NvmeBar structure
>   block/nvme: Pair doorbell registers
> 
>  block/nvme.c | 43 +++++++++++++++----------------------------
>  1 file changed, 15 insertions(+), 28 deletions(-)
> 

Reviewed-by: Klaus Jensen <k.jensen@samsung.com>

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH 0/3] block/nvme: Use NvmeBar structure from "block/nvme.h"

[Kevin Wolf]

Am 04.09.2020 um 14:41 hat Philippe Mathieu-Daudé geschrieben:
> Cleanups in the NVMeRegs structure:
> - Use the already existing NvmeBar structure from "block/nvme.h"
> - Pair doorbell registers
> 
> Based-on: <20200903122803.405265-1-philmd@redhat.com>

Thanks, applied to the block branch.

Kevin