[PATCH 0/2] hw/block/nvme: two fixes for create sq/cq

[PATCH 0/2] hw/block/nvme: two fixes for create sq/cq

[Klaus Jensen]

From: Klaus Jensen <k.jensen@samsung.com>

The first patch is a follow up to "hw/block/nvme: fix prp mapping status
codes" and fixes some status codes in the nvme_create_{sq,cq} functions.

The second patch fixes a faulty check on the given queue identifier.

Gollu Appalanaidu (2):
  nvme: fix create IO SQ/CQ status codes
  nvme: fix queue identifer validation

 hw/block/nvme.c | 16 +++++++++-------
 1 file changed, 9 insertions(+), 7 deletions(-)

-- 
2.28.0

[PATCH 1/2] nvme: fix create IO SQ/CQ status codes

[Klaus Jensen]

From: Gollu Appalanaidu <anaidu.gollu@samsung.com>

Replace the Invalid Field in Command with the Invalid PRP Offset status
code in the nvme_create_{cq,sq} functions. Also, allow PRP1 to be
address 0x0.

Also replace the Completion Queue Invalid status code returned in
nvme_create_cq when the the queue identifier is invalid with the Invalid
Queue Identifier. The Completion Queue Invalid status code is
exclusively for indicating that the completion queue identifer given
when creating a submission queue is invalid.

See NVM Express v1.3d, Section 5.3 ("Create I/O Completion Queue
command") and 5.4("Create I/O Submission Queue command").

Signed-off-by: Gollu Appalanaidu <anaidu.gollu@samsung.com>
Signed-off-by: Klaus Jensen <k.jensen@samsung.com>
---
 hw/block/nvme.c | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/hw/block/nvme.c b/hw/block/nvme.c
index 2896bb49b9c0..5dfef0204c2c 100644
--- a/hw/block/nvme.c
+++ b/hw/block/nvme.c
@@ -1151,9 +1151,9 @@ static uint16_t nvme_create_sq(NvmeCtrl *n, NvmeRequest *req)
         trace_pci_nvme_err_invalid_create_sq_size(qsize);
         return NVME_MAX_QSIZE_EXCEEDED | NVME_DNR;
     }
-    if (unlikely(!prp1 || prp1 & (n->page_size - 1))) {
+    if (unlikely(prp1 & (n->page_size - 1))) {
         trace_pci_nvme_err_invalid_create_sq_addr(prp1);
-        return NVME_INVALID_FIELD | NVME_DNR;
+        return NVME_INVALID_PRP_OFFSET | NVME_DNR;
     }
     if (unlikely(!(NVME_SQ_FLAGS_PC(qflags)))) {
         trace_pci_nvme_err_invalid_create_sq_qflags(NVME_SQ_FLAGS_PC(qflags));
@@ -1400,15 +1400,15 @@ static uint16_t nvme_create_cq(NvmeCtrl *n, NvmeRequest *req)
 
     if (unlikely(!cqid || !nvme_check_cqid(n, cqid))) {
         trace_pci_nvme_err_invalid_create_cq_cqid(cqid);
-        return NVME_INVALID_CQID | NVME_DNR;
+        return NVME_INVALID_QID | NVME_DNR;
     }
     if (unlikely(!qsize || qsize > NVME_CAP_MQES(n->bar.cap))) {
         trace_pci_nvme_err_invalid_create_cq_size(qsize);
         return NVME_MAX_QSIZE_EXCEEDED | NVME_DNR;
     }
-    if (unlikely(!prp1)) {
+    if (unlikely(prp1 & (n->page_size - 1))) {
         trace_pci_nvme_err_invalid_create_cq_addr(prp1);
-        return NVME_INVALID_FIELD | NVME_DNR;
+        return NVME_INVALID_PRP_OFFSET | NVME_DNR;
     }
     if (unlikely(!msix_enabled(&n->parent_obj) && vector)) {
         trace_pci_nvme_err_invalid_create_cq_vector(vector);
-- 
2.28.0

[PATCH 2/2] nvme: fix queue identifer validation

[Klaus Jensen]

From: Gollu Appalanaidu <anaidu.gollu@samsung.com>

The nvme_check_{sq,cq} functions check if the given queue identifer is
valid *and* that the queue exists. Thus, the function return value
cannot simply be inverted to check if the identifer is valid and that
the queue does *not* exist.

Replace the call with an OR'ed version of the checks.

Signed-off-by: Gollu Appalanaidu <anaidu.gollu@samsung.com>
Signed-off-by: Klaus Jensen <k.jensen@samsung.com>
---
 hw/block/nvme.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/hw/block/nvme.c b/hw/block/nvme.c
index 5dfef0204c2c..fa2cba744b57 100644
--- a/hw/block/nvme.c
+++ b/hw/block/nvme.c
@@ -1143,7 +1143,8 @@ static uint16_t nvme_create_sq(NvmeCtrl *n, NvmeRequest *req)
         trace_pci_nvme_err_invalid_create_sq_cqid(cqid);
         return NVME_INVALID_CQID | NVME_DNR;
     }
-    if (unlikely(!sqid || !nvme_check_sqid(n, sqid))) {
+    if (unlikely(!sqid || sqid > n->params.max_ioqpairs ||
+        n->sq[sqid] != NULL)) {
         trace_pci_nvme_err_invalid_create_sq_sqid(sqid);
         return NVME_INVALID_QID | NVME_DNR;
     }
@@ -1398,7 +1399,8 @@ static uint16_t nvme_create_cq(NvmeCtrl *n, NvmeRequest *req)
     trace_pci_nvme_create_cq(prp1, cqid, vector, qsize, qflags,
                              NVME_CQ_FLAGS_IEN(qflags) != 0);
 
-    if (unlikely(!cqid || !nvme_check_cqid(n, cqid))) {
+    if (unlikely(!cqid || cqid > n->params.max_ioqpairs ||
+        n->cq[cqid] != NULL)) {
         trace_pci_nvme_err_invalid_create_cq_cqid(cqid);
         return NVME_INVALID_QID | NVME_DNR;
     }
-- 
2.28.0

Re: [PATCH 0/2] hw/block/nvme: two fixes for create sq/cq

[Keith Busch]

On Thu, Oct 22, 2020 at 03:24:02PM +0200, Klaus Jensen wrote:
> From: Klaus Jensen <k.jensen@samsung.com>
> 
> The first patch is a follow up to "hw/block/nvme: fix prp mapping status
> codes" and fixes some status codes in the nvme_create_{sq,cq} functions.
> 
> The second patch fixes a faulty check on the given queue identifier.

Looks good.

Reviewed-by: Keith Busch <kbusch@kernel.org>

Re: [PATCH 0/2] hw/block/nvme: two fixes for create sq/cq

[Klaus Jensen]

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

On Oct 22 08:20, Keith Busch wrote:
> On Thu, Oct 22, 2020 at 03:24:02PM +0200, Klaus Jensen wrote:
> > From: Klaus Jensen <k.jensen@samsung.com>
> > 
> > The first patch is a follow up to "hw/block/nvme: fix prp mapping status
> > codes" and fixes some status codes in the nvme_create_{sq,cq} functions.
> > 
> > The second patch fixes a faulty check on the given queue identifier.
> 
> Looks good.
> 
> Reviewed-by: Keith Busch <kbusch@kernel.org>

Thanks! Applied to nvme-next.

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