[v3,01/15] hw/block/nvme: Define 64 bit cqe.result

Commit Message

Dmitry Fomichev Sept. 13, 2020, 10:14 p.m. UTC

From: Ajay Joshi <[e-mail]>

A new write command, Zone Append, is added as a part of Zoned
Namespace Command Set. Upon successful completion of this command,
the controller returns the start LBA of the performed write operation
in cqe.result field. Therefore, the maximum size of this variable
needs to be changed from 32 to 64 bit, consuming the reserved 32 bit
field that follows the result in CQE struct. Since the existing
commands are expected to return a 32 bit LE value, two separate
variables, result32 and result64, are now kept in a union.

Signed-off-by: Ajay Joshi <[e-mail]>
Signed-off-by: Dmitry Fomichev <[e-mail]>
Reviewed-by: Klaus Jensen <[e-mail]>
---
 block/nvme.c         |  2 +-
 block/trace-events   |  2 +-
 hw/block/nvme.c      | 10 +++++-----
 include/block/nvme.h |  6 ++++--
 4 files changed, 11 insertions(+), 9 deletions(-)

Comments

Klaus Jensen Sept. 15, 2020, 7:37 a.m. UTC | #1

On Sep 14 07:14, Dmitry Fomichev wrote:
> From: Ajay Joshi <[e-mail]>
> 
> A new write command, Zone Append, is added as a part of Zoned
> Namespace Command Set. Upon successful completion of this command,
> the controller returns the start LBA of the performed write operation
> in cqe.result field. Therefore, the maximum size of this variable
> needs to be changed from 32 to 64 bit, consuming the reserved 32 bit
> field that follows the result in CQE struct. Since the existing
> commands are expected to return a 32 bit LE value, two separate
> variables, result32 and result64, are now kept in a union.
> 
> Signed-off-by: Ajay Joshi <[e-mail]>
> Signed-off-by: Dmitry Fomichev <[e-mail]>
> Reviewed-by: Klaus Jensen <[e-mail]>

I know that I R-b'ed this, but can this be moved to the namespace types
patch, since that is the TP that changes this.

Also, I don't think we should touch the tracing in the block driver
since it is not aware of namespace types.

Dmitry Fomichev Sept. 15, 2020, 6:56 p.m. UTC | #2

> -----Original Message-----
> From: Klaus Jensen <[e-mail]>
> Sent: Tuesday, September 15, 2020 3:37 AM
> To: Dmitry Fomichev <[e-mail]>
> Cc: Keith Busch <[e-mail]>; Klaus Jensen
> <[e-mail]>; Kevin Wolf <[e-mail]>; Philippe
> Mathieu-Daudé <[e-mail]>; Maxim Levitsky
> <[e-mail]>; Fam Zheng <[e-mail]>; Niklas Cassel
> <[e-mail]>; Damien Le Moal <[e-mail]>;
> [e-mail]; [e-mail]; Alistair Francis
> <[e-mail]>; Matias Bjorling <[e-mail]>
> Subject: Re: [PATCH v3 01/15] hw/block/nvme: Define 64 bit cqe.result
> 
> On Sep 14 07:14, Dmitry Fomichev wrote:
> > From: Ajay Joshi <[e-mail]>
> >
> > A new write command, Zone Append, is added as a part of Zoned
> > Namespace Command Set. Upon successful completion of this command,
> > the controller returns the start LBA of the performed write operation
> > in cqe.result field. Therefore, the maximum size of this variable
> > needs to be changed from 32 to 64 bit, consuming the reserved 32 bit
> > field that follows the result in CQE struct. Since the existing
> > commands are expected to return a 32 bit LE value, two separate
> > variables, result32 and result64, are now kept in a union.
> >
> > Signed-off-by: Ajay Joshi <[e-mail]>
> > Signed-off-by: Dmitry Fomichev <[e-mail]>
> > Reviewed-by: Klaus Jensen <[e-mail]>
> 
> I know that I R-b'ed this, but can this be moved to the namespace types
> patch, since that is the TP that changes this.

You probably meant the ZNS patch since result64 is first used there to return
ZA starting data LBA. Sure, I can move this stuff to that patch.

> 
> Also, I don't think we should touch the tracing in the block driver
> since it is not aware of namespace types.

Ok

Klaus Jensen Sept. 15, 2020, 7:55 p.m. UTC | #3

On Sep 15 18:56, Dmitry Fomichev wrote:
> > -----Original Message-----
> > From: Klaus Jensen <[e-mail]>
> > Sent: Tuesday, September 15, 2020 3:37 AM
> > To: Dmitry Fomichev <[e-mail]>
> > Cc: Keith Busch <[e-mail]>; Klaus Jensen
> > <[e-mail]>; Kevin Wolf <[e-mail]>; Philippe
> > Mathieu-Daudé <[e-mail]>; Maxim Levitsky
> > <[e-mail]>; Fam Zheng <[e-mail]>; Niklas Cassel
> > <[e-mail]>; Damien Le Moal <[e-mail]>;
> > [e-mail]; [e-mail]; Alistair Francis
> > <[e-mail]>; Matias Bjorling <[e-mail]>
> > Subject: Re: [PATCH v3 01/15] hw/block/nvme: Define 64 bit cqe.result
> > 
> > On Sep 14 07:14, Dmitry Fomichev wrote:
> > > From: Ajay Joshi <[e-mail]>
> > >
> > > A new write command, Zone Append, is added as a part of Zoned
> > > Namespace Command Set. Upon successful completion of this command,
> > > the controller returns the start LBA of the performed write operation
> > > in cqe.result field. Therefore, the maximum size of this variable
> > > needs to be changed from 32 to 64 bit, consuming the reserved 32 bit
> > > field that follows the result in CQE struct. Since the existing
> > > commands are expected to return a 32 bit LE value, two separate
> > > variables, result32 and result64, are now kept in a union.
> > >
> > > Signed-off-by: Ajay Joshi <[e-mail]>
> > > Signed-off-by: Dmitry Fomichev <[e-mail]>
> > > Reviewed-by: Klaus Jensen <[e-mail]>
> > 
> > I know that I R-b'ed this, but can this be moved to the namespace types
> > patch, since that is the TP that changes this.
> 
> You probably meant the ZNS patch since result64 is first used there to return
> ZA starting data LBA. Sure, I can move this stuff to that patch.
> 

No, I actually did mean the NST patch since TP 4056 is the TP that
"unreserves" dw1 in the CQE.

Dmitry Fomichev Sept. 15, 2020, 8:44 p.m. UTC | #4

> -----Original Message-----
> From: Klaus Jensen <[e-mail]>
> Sent: Tuesday, September 15, 2020 3:56 PM
> To: Dmitry Fomichev <[e-mail]>
> Cc: Fam Zheng <[e-mail]>; Kevin Wolf <[e-mail]>;
> Damien Le Moal <[e-mail]>; [e-mail];
> Niklas Cassel <[e-mail]>; Klaus Jensen
> <[e-mail]>; [e-mail]; Alistair Francis
> <[e-mail]>; Keith Busch <[e-mail]>; Philippe
> Mathieu-Daudé <[e-mail]>; Matias Bjorling
> <[e-mail]>
> Subject: Re: [PATCH v3 01/15] hw/block/nvme: Define 64 bit cqe.result
> 
> On Sep 15 18:56, Dmitry Fomichev wrote:
> > > -----Original Message-----
> > > From: Klaus Jensen <[e-mail]>
> > > Sent: Tuesday, September 15, 2020 3:37 AM
> > > To: Dmitry Fomichev <[e-mail]>
> > > Cc: Keith Busch <[e-mail]>; Klaus Jensen
> > > <[e-mail]>; Kevin Wolf <[e-mail]>; Philippe
> > > Mathieu-Daudé <[e-mail]>; Maxim Levitsky
> > > <[e-mail]>; Fam Zheng <[e-mail]>; Niklas Cassel
> > > <[e-mail]>; Damien Le Moal
> <[e-mail]>;
> > > [e-mail]; [e-mail]; Alistair Francis
> > > <[e-mail]>; Matias Bjorling <[e-mail]>
> > > Subject: Re: [PATCH v3 01/15] hw/block/nvme: Define 64 bit cqe.result
> > >
> > > On Sep 14 07:14, Dmitry Fomichev wrote:
> > > > From: Ajay Joshi <[e-mail]>
> > > >
> > > > A new write command, Zone Append, is added as a part of Zoned
> > > > Namespace Command Set. Upon successful completion of this
> command,
> > > > the controller returns the start LBA of the performed write operation
> > > > in cqe.result field. Therefore, the maximum size of this variable
> > > > needs to be changed from 32 to 64 bit, consuming the reserved 32 bit
> > > > field that follows the result in CQE struct. Since the existing
> > > > commands are expected to return a 32 bit LE value, two separate
> > > > variables, result32 and result64, are now kept in a union.
> > > >
> > > > Signed-off-by: Ajay Joshi <[e-mail]>
> > > > Signed-off-by: Dmitry Fomichev <[e-mail]>
> > > > Reviewed-by: Klaus Jensen <[e-mail]>
> > >
> > > I know that I R-b'ed this, but can this be moved to the namespace types
> > > patch, since that is the TP that changes this.
> >
> > You probably meant the ZNS patch since result64 is first used there to
> return
> > ZA starting data LBA. Sure, I can move this stuff to that patch.
> >
> 
> No, I actually did mean the NST patch since TP 4056 is the TP that
> "unreserves" dw1 in the CQE.

It is not necessary to change it in NST patch since result64 field is not used
in that patch. But if you insist, I can move it to NST patch :)

Klaus Jensen Sept. 15, 2020, 8:48 p.m. UTC | #5

On Sep 15 20:44, Dmitry Fomichev wrote:
> > -----Original Message-----
> > From: Klaus Jensen <[e-mail]>
> > Sent: Tuesday, September 15, 2020 3:56 PM
> > To: Dmitry Fomichev <[e-mail]>
> > Cc: Fam Zheng <[e-mail]>; Kevin Wolf <[e-mail]>;
> > Damien Le Moal <[e-mail]>; [e-mail];
> > Niklas Cassel <[e-mail]>; Klaus Jensen
> > <[e-mail]>; [e-mail]; Alistair Francis
> > <[e-mail]>; Keith Busch <[e-mail]>; Philippe
> > Mathieu-Daudé <[e-mail]>; Matias Bjorling
> > <[e-mail]>
> > Subject: Re: [PATCH v3 01/15] hw/block/nvme: Define 64 bit cqe.result
> > 
> > On Sep 15 18:56, Dmitry Fomichev wrote:
> > > > -----Original Message-----
> > > > From: Klaus Jensen <[e-mail]>
> > > > Sent: Tuesday, September 15, 2020 3:37 AM
> > > > To: Dmitry Fomichev <[e-mail]>
> > > > Cc: Keith Busch <[e-mail]>; Klaus Jensen
> > > > <[e-mail]>; Kevin Wolf <[e-mail]>; Philippe
> > > > Mathieu-Daudé <[e-mail]>; Maxim Levitsky
> > > > <[e-mail]>; Fam Zheng <[e-mail]>; Niklas Cassel
> > > > <[e-mail]>; Damien Le Moal
> > <[e-mail]>;
> > > > [e-mail]; [e-mail]; Alistair Francis
> > > > <[e-mail]>; Matias Bjorling <[e-mail]>
> > > > Subject: Re: [PATCH v3 01/15] hw/block/nvme: Define 64 bit cqe.result
> > > >
> > > > On Sep 14 07:14, Dmitry Fomichev wrote:
> > > > > From: Ajay Joshi <[e-mail]>
> > > > >
> > > > > A new write command, Zone Append, is added as a part of Zoned
> > > > > Namespace Command Set. Upon successful completion of this
> > command,
> > > > > the controller returns the start LBA of the performed write operation
> > > > > in cqe.result field. Therefore, the maximum size of this variable
> > > > > needs to be changed from 32 to 64 bit, consuming the reserved 32 bit
> > > > > field that follows the result in CQE struct. Since the existing
> > > > > commands are expected to return a 32 bit LE value, two separate
> > > > > variables, result32 and result64, are now kept in a union.
> > > > >
> > > > > Signed-off-by: Ajay Joshi <[e-mail]>
> > > > > Signed-off-by: Dmitry Fomichev <[e-mail]>
> > > > > Reviewed-by: Klaus Jensen <[e-mail]>
> > > >
> > > > I know that I R-b'ed this, but can this be moved to the namespace types
> > > > patch, since that is the TP that changes this.
> > >
> > > You probably meant the ZNS patch since result64 is first used there to
> > return
> > > ZA starting data LBA. Sure, I can move this stuff to that patch.
> > >
> > 
> > No, I actually did mean the NST patch since TP 4056 is the TP that
> > "unreserves" dw1 in the CQE.
> 
> It is not necessary to change it in NST patch since result64 field is not used
> in that patch. But if you insist, I can move it to NST patch :)

You are right of course, but since it is a change to the "spec" related
data structures that go into include/block/nvme.h, I think it belongs in
"hw/block/nvme: Introduce the Namespace Types definitions".

Keith Busch Sept. 18, 2020, 9:10 p.m. UTC | #6

On Tue, Sep 15, 2020 at 10:48:49PM +0200, Klaus Jensen wrote:
> On Sep 15 20:44, Dmitry Fomichev wrote:
> > 
> > It is not necessary to change it in NST patch since result64 field is not used
> > in that patch. But if you insist, I can move it to NST patch :)
> 
> You are right of course, but since it is a change to the "spec" related
> data structures that go into include/block/nvme.h, I think it belongs in
> "hw/block/nvme: Introduce the Namespace Types definitions".

Just my $.02, unless you're making exernal APIs, I really find it easier
to review internal changes inline with the patches that use it.

Another example, there are patches in this series that introduce trace
points, but the patch that use them come later. I find that harder to
review since I need to look at two different patches to understand its
value.

I realize this particular patch is implementing a spec feature, but I'd
prefer to see how it's used over of making a round trip to the spec.

Dmitry Fomichev Sept. 21, 2020, 9:39 p.m. UTC | #7

> -----Original Message-----
> From: Keith Busch <[e-mail]>
> Sent: Friday, September 18, 2020 5:10 PM
> To: Klaus Jensen <[e-mail]>
> Cc: Dmitry Fomichev <[e-mail]>; Fam Zheng
> <[e-mail]>; Kevin Wolf <[e-mail]>; Damien Le Moal
> <[e-mail]>; [e-mail]; Niklas Cassel
> <[e-mail]>; Klaus Jensen <[e-mail]>; qemu-
> [e-mail]; Alistair Francis <[e-mail]>; Philippe
> Mathieu-Daudé <[e-mail]>; Matias Bjorling
> <[e-mail]>
> Subject: Re: [PATCH v3 01/15] hw/block/nvme: Define 64 bit cqe.result
> 
> On Tue, Sep 15, 2020 at 10:48:49PM +0200, Klaus Jensen wrote:
> > On Sep 15 20:44, Dmitry Fomichev wrote:
> > >
> > > It is not necessary to change it in NST patch since result64 field is not used
> > > in that patch. But if you insist, I can move it to NST patch :)
> >
> > You are right of course, but since it is a change to the "spec" related
> > data structures that go into include/block/nvme.h, I think it belongs in
> > "hw/block/nvme: Introduce the Namespace Types definitions".
> 
> Just my $.02, unless you're making exernal APIs, I really find it easier
> to review internal changes inline with the patches that use it.
> 
> Another example, there are patches in this series that introduce trace
> points, but the patch that use them come later. I find that harder to
> review since I need to look at two different patches to understand its
> value.
> 

I decided to split the trace events to be separate because I felt that it
could make the reviewing process simpler. Since the majority of the patches
in this series are on the large side, I thought it would be easier to open
them in separate sessions rather than to review a large single diff.
Let me know if you'd like me to fold the tracing stuff together with
the code...

DF

> I realize this particular patch is implementing a spec feature, but I'd
> prefer to see how it's used over of making a round trip to the spec.

Patch

diff --git a/block/nvme.c b/block/nvme.c
index 05485fdd11..45e1a5dcd1 100644
--- a/block/nvme.c
+++ b/block/nvme.c
@@ -333,7 +333,7 @@  static inline int nvme_translate_error(const NvmeCqe *c)
 {
     uint16_t status = (le16_to_cpu(c->status) >> 1) & 0xFF;
     if (status) {
-        trace_nvme_error(le32_to_cpu(c->result),
+        trace_nvme_error(le64_to_cpu(c->result64),
                          le16_to_cpu(c->sq_head),
                          le16_to_cpu(c->sq_id),
                          le16_to_cpu(c->cid),
diff --git a/block/trace-events b/block/trace-events
index e1c79a910d..55c54a18c3 100644
--- a/block/trace-events
+++ b/block/trace-events
@@ -139,7 +139,7 @@  qed_aio_write_main(void *s, void *acb, int ret, uint64_t offset, size_t len) "s
 # nvme.c
 nvme_kick(void *s, int queue) "s %p queue %d"
 nvme_dma_flush_queue_wait(void *s) "s %p"
-nvme_error(int cmd_specific, int sq_head, int sqid, int cid, int status) "cmd_specific %d sq_head %d sqid %d cid %d status 0x%x"
+nvme_error(uint64_t cmd_specific, int sq_head, int sqid, int cid, int status) "cmd_specific %ld sq_head %d sqid %d cid %d status 0x%x"
 nvme_process_completion(void *s, int index, int inflight) "s %p queue %d inflight %d"
 nvme_process_completion_queue_plugged(void *s, int index) "s %p queue %d"
 nvme_complete_command(void *s, int index, int cid) "s %p queue %d cid %d"
diff --git a/hw/block/nvme.c b/hw/block/nvme.c
index 63078f6009..3a90d80694 100644
--- a/hw/block/nvme.c
+++ b/hw/block/nvme.c
@@ -524,7 +524,7 @@  static void nvme_process_aers(void *opaque)
 
         req = n->aer_reqs[n->outstanding_aers];
 
-        result = (NvmeAerResult *) &req->cqe.result;
+        result = (NvmeAerResult *) &req->cqe.result32;
         result->event_type = event->result.event_type;
         result->event_info = event->result.event_info;
         result->log_page = event->result.log_page;
@@ -1247,7 +1247,7 @@  static uint16_t nvme_abort(NvmeCtrl *n, NvmeRequest *req)
 {
     uint16_t sqid = le32_to_cpu(req->cmd.cdw10) & 0xffff;
 
-    req->cqe.result = 1;
+    req->cqe.result32 = 1;
     if (nvme_check_sqid(n, sqid)) {
         return NVME_INVALID_FIELD | NVME_DNR;
     }
@@ -1425,7 +1425,7 @@  defaults:
     }
 
 out:
-    req->cqe.result = cpu_to_le32(result);
+    req->cqe.result32 = cpu_to_le32(result);
     return NVME_SUCCESS;
 }
 
@@ -1534,8 +1534,8 @@  static uint16_t nvme_set_feature(NvmeCtrl *n, NvmeRequest *req)
                                     ((dw11 >> 16) & 0xFFFF) + 1,
                                     n->params.max_ioqpairs,
                                     n->params.max_ioqpairs);
-        req->cqe.result = cpu_to_le32((n->params.max_ioqpairs - 1) |
-                                      ((n->params.max_ioqpairs - 1) << 16));
+        req->cqe.result32 = cpu_to_le32((n->params.max_ioqpairs - 1) |
+                                        ((n->params.max_ioqpairs - 1) << 16));
         break;
     case NVME_ASYNCHRONOUS_EVENT_CONF:
         n->features.async_config = dw11;
diff --git a/include/block/nvme.h b/include/block/nvme.h
index 65e68a82c8..ac0ccfcb26 100644
--- a/include/block/nvme.h
+++ b/include/block/nvme.h
@@ -617,8 +617,10 @@  typedef struct QEMU_PACKED NvmeAerResult {
 } NvmeAerResult;
 
 typedef struct QEMU_PACKED NvmeCqe {
-    uint32_t    result;
-    uint32_t    rsvd;
+    union {
+        uint64_t     result64;
+        uint32_t     result32;
+    };
     uint16_t    sq_head;
     uint16_t    sq_id;
     uint16_t    cid;