| Message ID | 20230719073605.98222-4-its@irrelevant.dk |
|---|---|
| State | New |
| Headers | show |
| Series | [PULL,1/1] hw/nvme: fix endianness issue for shadow doorbells | expand |
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)); }
On Wed, 19 Jul 2023 at 21:13, Michael Tokarev <mjt@tls.msk.ru> wrote: > > 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: If you're going to change this, better to take the approach Philippe suggested in review of using stl_le_pci_dma(). https://lore.kernel.org/qemu-devel/376e5e45-a3e7-0029-603a-b7ad9673fac4@linaro.org/ thanks -- PMM
On Jul 20 09:43, Peter Maydell wrote: > On Wed, 19 Jul 2023 at 21:13, Michael Tokarev <mjt@tls.msk.ru> wrote: > > > > 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: > > If you're going to change this, better to take the approach > Philippe suggested in review of using stl_le_pci_dma(). > > https://lore.kernel.org/qemu-devel/376e5e45-a3e7-0029-603a-b7ad9673fac4@linaro.org/ > Yup, that was my plan for next. But the original patch was already verified on hardware and mutiple testes, so wanted to go with that for the "fix". But yes, I will refactor into the much nicer stl/ldl api.
On Thu, 20 Jul 2023 at 09:49, Klaus Jensen <its@irrelevant.dk> wrote: > > On Jul 20 09:43, Peter Maydell wrote: > > On Wed, 19 Jul 2023 at 21:13, Michael Tokarev <mjt@tls.msk.ru> wrote: > > > > > > 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: > > > > If you're going to change this, better to take the approach > > Philippe suggested in review of using stl_le_pci_dma(). > > > > https://lore.kernel.org/qemu-devel/376e5e45-a3e7-0029-603a-b7ad9673fac4@linaro.org/ > > > > Yup, that was my plan for next. But the original patch was already > verified on hardware and mutiple testes, so wanted to go with that for > the "fix". > > But yes, I will refactor into the much nicer stl/ldl api. FWIW, I don't think this bug fix was so urgent that we needed to go with a quick fix and a followup -- we're not yet that close to 8.1 release. thanks -- PMM
On Jul 20 09:51, Peter Maydell wrote: > On Thu, 20 Jul 2023 at 09:49, Klaus Jensen <its@irrelevant.dk> wrote: > > > > On Jul 20 09:43, Peter Maydell wrote: > > > On Wed, 19 Jul 2023 at 21:13, Michael Tokarev <mjt@tls.msk.ru> wrote: > > > > > > > > 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: > > > > > > If you're going to change this, better to take the approach > > > Philippe suggested in review of using stl_le_pci_dma(). > > > > > > https://lore.kernel.org/qemu-devel/376e5e45-a3e7-0029-603a-b7ad9673fac4@linaro.org/ > > > > > > > Yup, that was my plan for next. But the original patch was already > > verified on hardware and mutiple testes, so wanted to go with that for > > the "fix". > > > > But yes, I will refactor into the much nicer stl/ldl api. > > FWIW, I don't think this bug fix was so urgent that we > needed to go with a quick fix and a followup -- we're > not yet that close to 8.1 release. > Alright, noted ;) I will spin this into the other fix I have under review.
diff --git a/hw/nvme/ctrl.c b/hw/nvme/ctrl.c index 8e8e870b9a80..dadc2dc7da10 100644 --- a/hw/nvme/ctrl.c +++ b/hw/nvme/ctrl.c @@ -6801,6 +6801,7 @@ static uint16_t nvme_dbbuf_config(NvmeCtrl *n, const NvmeRequest *req) PCIDevice *pci = PCI_DEVICE(n); uint64_t dbs_addr = le64_to_cpu(req->cmd.dptr.prp1); uint64_t eis_addr = le64_to_cpu(req->cmd.dptr.prp2); + uint32_t v; int i; /* Address should be page aligned */ @@ -6818,6 +6819,8 @@ static uint16_t nvme_dbbuf_config(NvmeCtrl *n, const NvmeRequest *req) NvmeCQueue *cq = n->cq[i]; if (sq) { + v = cpu_to_le32(sq->tail); + /* * CAP.DSTRD is 0, so offset of ith sq db_addr is (i<<3) * nvme_process_db() uses this hard-coded way to calculate @@ -6825,7 +6828,7 @@ 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, &sq->tail, sizeof(sq->tail)); + pci_dma_write(pci, sq->db_addr, &v, sizeof(sq->tail)); if (n->params.ioeventfd && sq->sqid != 0) { if (!nvme_init_sq_ioeventfd(sq)) { @@ -6835,10 +6838,12 @@ 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, &cq->head, sizeof(cq->head)); + pci_dma_write(pci, cq->db_addr, &v, sizeof(cq->head)); if (n->params.ioeventfd && cq->cqid != 0) { if (!nvme_init_cq_ioeventfd(cq)) { @@ -7587,7 +7592,7 @@ static uint64_t nvme_mmio_read(void *opaque, hwaddr addr, unsigned size) static void nvme_process_db(NvmeCtrl *n, hwaddr addr, int val) { PCIDevice *pci = PCI_DEVICE(n); - uint32_t qid; + uint32_t qid, v; if (unlikely(addr & ((1 << 2) - 1))) { NVME_GUEST_ERR(pci_nvme_ub_db_wr_misaligned, @@ -7654,7 +7659,8 @@ static void nvme_process_db(NvmeCtrl *n, hwaddr addr, int val) start_sqs = nvme_cq_full(cq) ? 1 : 0; cq->head = new_head; if (!qid && n->dbbuf_enabled) { - pci_dma_write(pci, cq->db_addr, &cq->head, sizeof(cq->head)); + v = cpu_to_le32(cq->head); + pci_dma_write(pci, cq->db_addr, &v, sizeof(cq->head)); } if (start_sqs) { NvmeSQueue *sq; @@ -7714,6 +7720,8 @@ 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 * corresponding doorbell property to match the value of that entry @@ -7727,7 +7735,7 @@ static void nvme_process_db(NvmeCtrl *n, hwaddr addr, int val) * including ones that run on Linux, are not updating Admin Queues, * so we can't trust reading it for an appropriate 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)); } qemu_bh_schedule(sq->bh);