Re: [PATCH 15/16] hw/block/nvme: remove NvmeCmd parameter
On Jul 29 21:25, Maxim Levitsky wrote: > On Mon, 2020-07-20 at 13:37 +0200, Klaus Jensen wrote: > > From: Klaus Jensen > > > > Keep a copy of the raw nvme command in the NvmeRequest and remove the > > now redundant NvmeCmd parameter. > > Shouldn't you clear the req->cmd in nvme_req_clear too for consistency? It always gets unconditionally overwritten with a memcpy in nvme_process_sq, so we are not leaving anything dangling (like we would do with the namespace reference because it's usually not initialized for Admin commands)
Re: [PATCH 15/16] hw/block/nvme: remove NvmeCmd parameter
On Jul 30 01:10, Minwoo Im wrote: > On 20-07-20 13:37:47, Klaus Jensen wrote: > > From: Klaus Jensen > > > > Keep a copy of the raw nvme command in the NvmeRequest and remove the > > now redundant NvmeCmd parameter. > > > > Signed-off-by: Klaus Jensen > > I would really have suggested this change from 13th patch! > > Reviewed-by: Minwoo Im > I squashed the two patches. If don't think it makes the match much harder to review since the added namespace reference is a pretty small change.
Re: [PATCH 15/16] hw/block/nvme: remove NvmeCmd parameter
On Mon, 2020-07-20 at 13:37 +0200, Klaus Jensen wrote:
> From: Klaus Jensen
>
> Keep a copy of the raw nvme command in the NvmeRequest and remove the
> now redundant NvmeCmd parameter.
Shouldn't you clear the req->cmd in nvme_req_clear too for consistency?
Other than that looks OK, but I might have missed something.
Best regards,
Maxim Levitsky
>
> Signed-off-by: Klaus Jensen
> ---
> hw/block/nvme.c | 177 +---
> hw/block/nvme.h | 1 +
> 2 files changed, 93 insertions(+), 85 deletions(-)
>
> diff --git a/hw/block/nvme.c b/hw/block/nvme.c
> index b53afdeb3fb6..0b3dceccc89b 100644
> --- a/hw/block/nvme.c
> +++ b/hw/block/nvme.c
> @@ -425,9 +425,9 @@ static uint16_t nvme_dma_prp(NvmeCtrl *n, uint8_t *ptr,
> uint32_t len,
> return status;
> }
>
> -static uint16_t nvme_map(NvmeCtrl *n, NvmeCmd *cmd, size_t len,
> - NvmeRequest *req)
> +static uint16_t nvme_map(NvmeCtrl *n, size_t len, NvmeRequest *req)
> {
> +NvmeCmd *cmd = >cmd;
> uint64_t prp1 = le64_to_cpu(cmd->dptr.prp1);
> uint64_t prp2 = le64_to_cpu(cmd->dptr.prp2);
>
> @@ -597,7 +597,7 @@ static void nvme_rw_cb(void *opaque, int ret)
> nvme_enqueue_req_completion(cq, req);
> }
>
> -static uint16_t nvme_flush(NvmeCtrl *n, NvmeCmd *cmd, NvmeRequest *req)
> +static uint16_t nvme_flush(NvmeCtrl *n, NvmeRequest *req)
> {
> block_acct_start(blk_get_stats(n->conf.blk), >acct, 0,
> BLOCK_ACCT_FLUSH);
> @@ -606,9 +606,9 @@ static uint16_t nvme_flush(NvmeCtrl *n, NvmeCmd *cmd,
> NvmeRequest *req)
> return NVME_NO_COMPLETE;
> }
>
> -static uint16_t nvme_write_zeroes(NvmeCtrl *n, NvmeCmd *cmd, NvmeRequest
> *req)
> +static uint16_t nvme_write_zeroes(NvmeCtrl *n, NvmeRequest *req)
> {
> -NvmeRwCmd *rw = (NvmeRwCmd *)cmd;
> +NvmeRwCmd *rw = (NvmeRwCmd *)>cmd;
> NvmeNamespace *ns = req->ns;
> const uint8_t lba_index = NVME_ID_NS_FLBAS_INDEX(ns->id_ns.flbas);
> const uint8_t data_shift = ns->id_ns.lbaf[lba_index].ds;
> @@ -633,9 +633,9 @@ static uint16_t nvme_write_zeroes(NvmeCtrl *n, NvmeCmd
> *cmd, NvmeRequest *req)
> return NVME_NO_COMPLETE;
> }
>
> -static uint16_t nvme_rw(NvmeCtrl *n, NvmeCmd *cmd, NvmeRequest *req)
> +static uint16_t nvme_rw(NvmeCtrl *n, NvmeRequest *req)
> {
> -NvmeRwCmd *rw = (NvmeRwCmd *)cmd;
> +NvmeRwCmd *rw = (NvmeRwCmd *)>cmd;
> NvmeNamespace *ns = req->ns;
> uint32_t nlb = le32_to_cpu(rw->nlb) + 1;
> uint64_t slba = le64_to_cpu(rw->slba);
> @@ -664,7 +664,7 @@ static uint16_t nvme_rw(NvmeCtrl *n, NvmeCmd *cmd,
> NvmeRequest *req)
> return status;
> }
>
> -if (nvme_map(n, cmd, data_size, req)) {
> +if (nvme_map(n, data_size, req)) {
> block_acct_invalid(blk_get_stats(n->conf.blk), acct);
> return NVME_INVALID_FIELD | NVME_DNR;
> }
> @@ -690,11 +690,12 @@ static uint16_t nvme_rw(NvmeCtrl *n, NvmeCmd *cmd,
> NvmeRequest *req)
> return NVME_NO_COMPLETE;
> }
>
> -static uint16_t nvme_io_cmd(NvmeCtrl *n, NvmeCmd *cmd, NvmeRequest *req)
> +static uint16_t nvme_io_cmd(NvmeCtrl *n, NvmeRequest *req)
> {
> -uint32_t nsid = le32_to_cpu(cmd->nsid);
> +uint32_t nsid = le32_to_cpu(req->cmd.nsid);
>
> -trace_pci_nvme_io_cmd(nvme_cid(req), nsid, nvme_sqid(req), cmd->opcode);
> +trace_pci_nvme_io_cmd(nvme_cid(req), nsid, nvme_sqid(req),
> + req->cmd.opcode);
>
> if (unlikely(nsid == 0 || nsid > n->num_namespaces)) {
> trace_pci_nvme_err_invalid_ns(nsid, n->num_namespaces);
> @@ -702,16 +703,16 @@ static uint16_t nvme_io_cmd(NvmeCtrl *n, NvmeCmd *cmd,
> NvmeRequest *req)
> }
>
> req->ns = >namespaces[nsid - 1];
> -switch (cmd->opcode) {
> +switch (req->cmd.opcode) {
> case NVME_CMD_FLUSH:
> -return nvme_flush(n, cmd, req);
> +return nvme_flush(n, req);
> case NVME_CMD_WRITE_ZEROES:
> -return nvme_write_zeroes(n, cmd, req);
> +return nvme_write_zeroes(n, req);
> case NVME_CMD_WRITE:
> case NVME_CMD_READ:
> -return nvme_rw(n, cmd, req);
> +return nvme_rw(n, req);
> default:
> -trace_pci_nvme_err_invalid_opc(cmd->opcode);
> +trace_pci_nvme_err_invalid_opc(req->cmd.opcode);
> return NVME_INVALID_OPCODE | NVME_DNR;
> }
> }
> @@ -727,10 +728,10 @@ static void nvme_free_sq(NvmeSQueue *sq, NvmeCtrl *n)
> }
> }
>
> -static uint16_t nvme_del_sq(NvmeCtrl *n, NvmeCmd *cmd)
> +static uint16_t nvme_del_sq(NvmeCtrl *n, NvmeRequest *req)
> {
> -NvmeDeleteQ *c = (NvmeDeleteQ *)cmd;
> -NvmeRequest *req, *next;
> +NvmeDeleteQ *c = (NvmeDeleteQ *)>cmd;
> +NvmeRequest *r, *next;
> NvmeSQueue *sq;
> NvmeCQueue *cq;
> uint16_t qid = le16_to_cpu(c->qid);
> @@ -744,19 +745,19 @@ static uint16_t nvme_del_sq(NvmeCtrl *n, NvmeCmd *cmd)
>
> sq = n->sq[qid];
> while (!QTAILQ_EMPTY(>out_req_list))
Re: [PATCH 15/16] hw/block/nvme: remove NvmeCmd parameter
On 20-07-20 13:37:47, Klaus Jensen wrote: > From: Klaus Jensen > > Keep a copy of the raw nvme command in the NvmeRequest and remove the > now redundant NvmeCmd parameter. > > Signed-off-by: Klaus Jensen I would really have suggested this change from 13th patch! Reviewed-by: Minwoo Im Thanks,
[PATCH 15/16] hw/block/nvme: remove NvmeCmd parameter
From: Klaus Jensen
Keep a copy of the raw nvme command in the NvmeRequest and remove the
now redundant NvmeCmd parameter.
Signed-off-by: Klaus Jensen
---
hw/block/nvme.c | 177 +---
hw/block/nvme.h | 1 +
2 files changed, 93 insertions(+), 85 deletions(-)
diff --git a/hw/block/nvme.c b/hw/block/nvme.c
index b53afdeb3fb6..0b3dceccc89b 100644
--- a/hw/block/nvme.c
+++ b/hw/block/nvme.c
@@ -425,9 +425,9 @@ static uint16_t nvme_dma_prp(NvmeCtrl *n, uint8_t *ptr,
uint32_t len,
return status;
}
-static uint16_t nvme_map(NvmeCtrl *n, NvmeCmd *cmd, size_t len,
- NvmeRequest *req)
+static uint16_t nvme_map(NvmeCtrl *n, size_t len, NvmeRequest *req)
{
+NvmeCmd *cmd = >cmd;
uint64_t prp1 = le64_to_cpu(cmd->dptr.prp1);
uint64_t prp2 = le64_to_cpu(cmd->dptr.prp2);
@@ -597,7 +597,7 @@ static void nvme_rw_cb(void *opaque, int ret)
nvme_enqueue_req_completion(cq, req);
}
-static uint16_t nvme_flush(NvmeCtrl *n, NvmeCmd *cmd, NvmeRequest *req)
+static uint16_t nvme_flush(NvmeCtrl *n, NvmeRequest *req)
{
block_acct_start(blk_get_stats(n->conf.blk), >acct, 0,
BLOCK_ACCT_FLUSH);
@@ -606,9 +606,9 @@ static uint16_t nvme_flush(NvmeCtrl *n, NvmeCmd *cmd,
NvmeRequest *req)
return NVME_NO_COMPLETE;
}
-static uint16_t nvme_write_zeroes(NvmeCtrl *n, NvmeCmd *cmd, NvmeRequest *req)
+static uint16_t nvme_write_zeroes(NvmeCtrl *n, NvmeRequest *req)
{
-NvmeRwCmd *rw = (NvmeRwCmd *)cmd;
+NvmeRwCmd *rw = (NvmeRwCmd *)>cmd;
NvmeNamespace *ns = req->ns;
const uint8_t lba_index = NVME_ID_NS_FLBAS_INDEX(ns->id_ns.flbas);
const uint8_t data_shift = ns->id_ns.lbaf[lba_index].ds;
@@ -633,9 +633,9 @@ static uint16_t nvme_write_zeroes(NvmeCtrl *n, NvmeCmd
*cmd, NvmeRequest *req)
return NVME_NO_COMPLETE;
}
-static uint16_t nvme_rw(NvmeCtrl *n, NvmeCmd *cmd, NvmeRequest *req)
+static uint16_t nvme_rw(NvmeCtrl *n, NvmeRequest *req)
{
-NvmeRwCmd *rw = (NvmeRwCmd *)cmd;
+NvmeRwCmd *rw = (NvmeRwCmd *)>cmd;
NvmeNamespace *ns = req->ns;
uint32_t nlb = le32_to_cpu(rw->nlb) + 1;
uint64_t slba = le64_to_cpu(rw->slba);
@@ -664,7 +664,7 @@ static uint16_t nvme_rw(NvmeCtrl *n, NvmeCmd *cmd,
NvmeRequest *req)
return status;
}
-if (nvme_map(n, cmd, data_size, req)) {
+if (nvme_map(n, data_size, req)) {
block_acct_invalid(blk_get_stats(n->conf.blk), acct);
return NVME_INVALID_FIELD | NVME_DNR;
}
@@ -690,11 +690,12 @@ static uint16_t nvme_rw(NvmeCtrl *n, NvmeCmd *cmd,
NvmeRequest *req)
return NVME_NO_COMPLETE;
}
-static uint16_t nvme_io_cmd(NvmeCtrl *n, NvmeCmd *cmd, NvmeRequest *req)
+static uint16_t nvme_io_cmd(NvmeCtrl *n, NvmeRequest *req)
{
-uint32_t nsid = le32_to_cpu(cmd->nsid);
+uint32_t nsid = le32_to_cpu(req->cmd.nsid);
-trace_pci_nvme_io_cmd(nvme_cid(req), nsid, nvme_sqid(req), cmd->opcode);
+trace_pci_nvme_io_cmd(nvme_cid(req), nsid, nvme_sqid(req),
+ req->cmd.opcode);
if (unlikely(nsid == 0 || nsid > n->num_namespaces)) {
trace_pci_nvme_err_invalid_ns(nsid, n->num_namespaces);
@@ -702,16 +703,16 @@ static uint16_t nvme_io_cmd(NvmeCtrl *n, NvmeCmd *cmd,
NvmeRequest *req)
}
req->ns = >namespaces[nsid - 1];
-switch (cmd->opcode) {
+switch (req->cmd.opcode) {
case NVME_CMD_FLUSH:
-return nvme_flush(n, cmd, req);
+return nvme_flush(n, req);
case NVME_CMD_WRITE_ZEROES:
-return nvme_write_zeroes(n, cmd, req);
+return nvme_write_zeroes(n, req);
case NVME_CMD_WRITE:
case NVME_CMD_READ:
-return nvme_rw(n, cmd, req);
+return nvme_rw(n, req);
default:
-trace_pci_nvme_err_invalid_opc(cmd->opcode);
+trace_pci_nvme_err_invalid_opc(req->cmd.opcode);
return NVME_INVALID_OPCODE | NVME_DNR;
}
}
@@ -727,10 +728,10 @@ static void nvme_free_sq(NvmeSQueue *sq, NvmeCtrl *n)
}
}
-static uint16_t nvme_del_sq(NvmeCtrl *n, NvmeCmd *cmd)
+static uint16_t nvme_del_sq(NvmeCtrl *n, NvmeRequest *req)
{
-NvmeDeleteQ *c = (NvmeDeleteQ *)cmd;
-NvmeRequest *req, *next;
+NvmeDeleteQ *c = (NvmeDeleteQ *)>cmd;
+NvmeRequest *r, *next;
NvmeSQueue *sq;
NvmeCQueue *cq;
uint16_t qid = le16_to_cpu(c->qid);
@@ -744,19 +745,19 @@ static uint16_t nvme_del_sq(NvmeCtrl *n, NvmeCmd *cmd)
sq = n->sq[qid];
while (!QTAILQ_EMPTY(>out_req_list)) {
-req = QTAILQ_FIRST(>out_req_list);
-assert(req->aiocb);
-blk_aio_cancel(req->aiocb);
+r = QTAILQ_FIRST(>out_req_list);
+assert(r->aiocb);
+blk_aio_cancel(r->aiocb);
}
if (!nvme_check_cqid(n, sq->cqid)) {
cq = n->cq[sq->cqid];
QTAILQ_REMOVE(>sq_list, sq, entry);
nvme_post_cqes(cq);
-QTAILQ_FOREACH_SAFE(req, >req_list, entry, next) {
-if (req->sq == sq) {
-
