Re: [PULL 1/1] hw/nvme: fix endianness issue for shadow doorbells

[Michael Tokarev]

19.07.2023 10:36, Klaus Jensen wrote:
pu(req->cmd.dptr.prp2);
> +    uint32_t v;

>           if (sq) {
> +            v = cpu_to_le32(sq->tail);

> -            pci_dma_write(pci, sq->db_addr, &sq->tail, sizeof(sq->tail));
> +            pci_dma_write(pci, sq->db_addr, &v, sizeof(sq->tail));

This and similar cases hurts my eyes.

Why we pass address of v here, but use sizeof(sq->tail) ?

Yes, I know both in theory should be of the same size, but heck,
this is puzzling at best, and confusing in a regular case.

Dunno how it slipped in the review, it instantly catched my eye
in a row of applied patches..

Also, why v is computed a few lines before it is used, with
some expressions between the assignment and usage?

How about the following patch:

From: Michael Tokarev <mjt@tls.msk.ru>
Date: Wed, 19 Jul 2023 23:10:53 +0300
Subject: [PATCH trivial] hw/nvme: fix sizeof() misuse and move endianness 
conversions
 closer to users

Signed-off-by: Michael Tokarev <mjt@tls.msk.ru>
Fixes: ea3c76f1494d0
---
 hw/nvme/ctrl.c | 17 +++++++----------
 1 file changed, 7 insertions(+), 10 deletions(-)

diff --git a/hw/nvme/ctrl.c b/hw/nvme/ctrl.c
index dadc2dc7da..e33b28cf66 100644
--- a/hw/nvme/ctrl.c
+++ b/hw/nvme/ctrl.c
@@ -6820,6 +6820,4 @@ static uint16_t nvme_dbbuf_config(NvmeCtrl *n, const 
NvmeRequest *req)

         if (sq) {
-            v = cpu_to_le32(sq->tail);
-
             /*
              * CAP.DSTRD is 0, so offset of ith sq db_addr is (i<<3)
@@ -6829,5 +6827,6 @@ static uint16_t nvme_dbbuf_config(NvmeCtrl *n, const 
NvmeRequest *req)
             sq->db_addr = dbs_addr + (i << 3);
             sq->ei_addr = eis_addr + (i << 3);
-            pci_dma_write(pci, sq->db_addr, &v, sizeof(sq->tail));
+            v = cpu_to_le32(sq->tail);
+            pci_dma_write(pci, sq->db_addr, &v, sizeof(v));

             if (n->params.ioeventfd && sq->sqid != 0) {
@@ -6839,10 +6838,9 @@ static uint16_t nvme_dbbuf_config(NvmeCtrl *n, const 
NvmeRequest *req)

         if (cq) {
-            v = cpu_to_le32(cq->head);
-
             /* CAP.DSTRD is 0, so offset of ith cq db_addr is (i<<3)+(1<<2) */
             cq->db_addr = dbs_addr + (i << 3) + (1 << 2);
             cq->ei_addr = eis_addr + (i << 3) + (1 << 2);
-            pci_dma_write(pci, cq->db_addr, &v, sizeof(cq->head));
+            v = cpu_to_le32(cq->head);
+            pci_dma_write(pci, cq->db_addr, &v, sizeof(v));

             if (n->params.ioeventfd && cq->cqid != 0) {
@@ -7661,5 +7659,5 @@ static void nvme_process_db(NvmeCtrl *n, hwaddr addr, int 
val)
         if (!qid && n->dbbuf_enabled) {
             v = cpu_to_le32(cq->head);
-            pci_dma_write(pci, cq->db_addr, &v, sizeof(cq->head));
+            pci_dma_write(pci, cq->db_addr, &v, sizeof(v));
         }
         if (start_sqs) {
@@ -7721,6 +7719,4 @@ static void nvme_process_db(NvmeCtrl *n, hwaddr addr, int 
val)
         sq->tail = new_tail;
         if (!qid && n->dbbuf_enabled) {
-            v = cpu_to_le32(sq->tail);
-
             /*
              * The spec states "the host shall also update the controller's
@@ -7736,5 +7732,6 @@ static void nvme_process_db(NvmeCtrl *n, hwaddr addr, int 
val)
              * so we can't trust reading it for an appropriate sq tail.
              */
-            pci_dma_write(pci, sq->db_addr, &v, sizeof(sq->tail));
+            v = cpu_to_le32(sq->tail);
+            pci_dma_write(pci, sq->db_addr, &v, sizeof(v));
         }