| Message ID | 20201029093306.1063879-1-philmd@redhat.com |
|---|---|
| Headers | show |
| Series | block/nvme: Fix Aarch64 or big-endian hosts | expand |
On Thu, Oct 29, 2020 at 10:33:06AM +0100, Philippe Mathieu-Daudé wrote: > The "Completion Queue Entry: DW 2" describes it as: > > This identifier is assigned by host software when > the command is submitted to the Submission > > As the is just an opaque cookie, it is pointless to byte-swap it. > > Suggested-by: Keith Busch <kbusch@kernel.org> > Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com> > --- > block/nvme.c | 6 +++--- > 1 file changed, 3 insertions(+), 3 deletions(-) > > diff --git a/block/nvme.c b/block/nvme.c > index a06a188d530..e7723c42a6d 100644 > --- a/block/nvme.c > +++ b/block/nvme.c > @@ -344,7 +344,7 @@ static inline int nvme_translate_error(const NvmeCqe *c) > trace_nvme_error(le32_to_cpu(c->result), > le16_to_cpu(c->sq_head), > le16_to_cpu(c->sq_id), > - le16_to_cpu(c->cid), > + c->cid, > le16_to_cpu(status)); > } > switch (status) { > @@ -401,7 +401,7 @@ static bool nvme_process_completion(NVMeQueuePair *q) > if (!q->cq.head) { > q->cq_phase = !q->cq_phase; > } > - cid = le16_to_cpu(c->cid); > + cid = c->cid; > if (cid == 0 || cid > NVME_QUEUE_SIZE) { > warn_report("NVMe: Unexpected CID in completion queue: %"PRIu32", " > "queue size: %u", cid, NVME_QUEUE_SIZE); > @@ -469,7 +469,7 @@ static void nvme_submit_command(NVMeQueuePair *q, NVMeRequest *req, > assert(!req->cb); > req->cb = cb; > req->opaque = opaque; > - cmd->cid = cpu_to_le16(req->cid); > + cmd->cid = req->cid; > > trace_nvme_submit_command(q->s, q->index, req->cid); > nvme_trace_command(cmd); Eliminating the byteswap is safe but this patch makes the code confusing, as I mentioned previously. Please use a comment or macro to mark this field native endian. It's not obvious to the reader that we can skip the byteswap here. Otherwise it will confuse readers into adding the byteswap back, not using byteswapping in other places where it is needed, etc. Thanks, Stefan
On Thu, Oct 29, 2020 at 10:32:51AM +0100, Philippe Mathieu-Daudé wrote: > Just for consistency, following the example documented since > commit e3fe3988d7 ("error: Document Error API usage rules"), > return a boolean value indicating an error is set or not. > Directly pass errp as the local_err is not requested in our > case. > > Tested-by: Eric Auger <eric.auger@redhat.com> > Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com> > --- > block/nvme.c | 12 +++++++----- > 1 file changed, 7 insertions(+), 5 deletions(-) Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>
On 10/30/20 3:00 PM, Stefan Hajnoczi wrote: > On Thu, Oct 29, 2020 at 10:33:06AM +0100, Philippe Mathieu-Daudé wrote: >> The "Completion Queue Entry: DW 2" describes it as: >> >> This identifier is assigned by host software when >> the command is submitted to the Submission >> >> As the is just an opaque cookie, it is pointless to byte-swap it. >> >> Suggested-by: Keith Busch <kbusch@kernel.org> >> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com> >> --- >> block/nvme.c | 6 +++--- >> 1 file changed, 3 insertions(+), 3 deletions(-) >> >> diff --git a/block/nvme.c b/block/nvme.c >> index a06a188d530..e7723c42a6d 100644 >> --- a/block/nvme.c >> +++ b/block/nvme.c >> @@ -344,7 +344,7 @@ static inline int nvme_translate_error(const NvmeCqe *c) >> trace_nvme_error(le32_to_cpu(c->result), >> le16_to_cpu(c->sq_head), >> le16_to_cpu(c->sq_id), >> - le16_to_cpu(c->cid), >> + c->cid, >> le16_to_cpu(status)); >> } >> switch (status) { >> @@ -401,7 +401,7 @@ static bool nvme_process_completion(NVMeQueuePair *q) >> if (!q->cq.head) { >> q->cq_phase = !q->cq_phase; >> } >> - cid = le16_to_cpu(c->cid); >> + cid = c->cid; >> if (cid == 0 || cid > NVME_QUEUE_SIZE) { >> warn_report("NVMe: Unexpected CID in completion queue: %"PRIu32", " >> "queue size: %u", cid, NVME_QUEUE_SIZE); >> @@ -469,7 +469,7 @@ static void nvme_submit_command(NVMeQueuePair *q, NVMeRequest *req, >> assert(!req->cb); >> req->cb = cb; >> req->opaque = opaque; >> - cmd->cid = cpu_to_le16(req->cid); >> + cmd->cid = req->cid; >> >> trace_nvme_submit_command(q->s, q->index, req->cid); >> nvme_trace_command(cmd); > > Eliminating the byteswap is safe but this patch makes the code > confusing, as I mentioned previously. > > Please use a comment or macro to mark this field native endian. It's not > obvious to the reader that we can skip the byteswap here. > > Otherwise it will confuse readers into adding the byteswap back, not > using byteswapping in other places where it is needed, etc. OK. (This patch is for 6.0 anyway, I included because it was following the previous patch in its previous version). > > Thanks, > Stefan >
On Thu, Oct 29, 2020 at 10:32:41AM +0100, Philippe Mathieu-Daudé wrote: > Add a bit of tracing, clean around to finally fix few bugs. > In particular, restore NVMe on Aarch64 host. > > Since v1: > - addressed Stefan and Eric review comments > - dropped unnecessary patches > - added BE fix reported by Keith > > Patches missing review: #10, #24, #25 > > Supersedes: <20201027135547.374946-1-philmd@redhat.com> > > Eric Auger (4): > block/nvme: Change size and alignment of IDENTIFY response buffer > block/nvme: Change size and alignment of queue > block/nvme: Change size and alignment of prp_list_pages > block/nvme: Align iov's va and size on host page size > > Philippe Mathieu-Daudé (21): > MAINTAINERS: Cover 'block/nvme.h' file > block/nvme: Use hex format to display offset in trace events > block/nvme: Report warning with warn_report() > block/nvme: Trace controller capabilities > block/nvme: Trace nvme_poll_queue() per queue > block/nvme: Improve nvme_free_req_queue_wait() trace information > block/nvme: Trace queue pair creation/deletion > block/nvme: Move definitions before structure declarations > block/nvme: Use unsigned integer for queue counter/size > block/nvme: Make nvme_identify() return boolean indicating error > block/nvme: Make nvme_init_queue() return boolean indicating error > block/nvme: Introduce Completion Queue definitions > block/nvme: Use definitions instead of magic values in add_io_queue() > block/nvme: Correctly initialize Admin Queue Attributes > block/nvme: Simplify ADMIN queue access > block/nvme: Simplify nvme_cmd_sync() > block/nvme: Set request_alignment at initialization > block/nvme: Correct minimum device page size > block/nvme: Fix use of write-only doorbells page on Aarch64 arch > block/nvme: Fix nvme_submit_command() on big-endian host > block/nvme: Simplify Completion Queue Command Identifier field use > > include/block/nvme.h | 18 ++-- > block/nvme.c | 213 ++++++++++++++++++++++++------------------- > MAINTAINERS | 2 + > block/trace-events | 30 +++--- > 4 files changed, 150 insertions(+), 113 deletions(-) > > -- > 2.26.2 > > Thanks, applied the 5.2 patches to my block tree: https://gitlab.com/stefanha/qemu/commits/block Stefan