[PULL 0/1] hw/nvme fixes

[PULL 0/1] hw/nvme fixes

[Klaus Jensen]

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

Hi,

The following changes since commit 361d5397355276e3007825cc17217c1e4d4320f7:

  Merge tag 'block-pull-request' of https://gitlab.com/stefanha/qemu into staging (2023-07-17 15:49:27 +0100)

are available in the Git repository at:

  https://gitlab.com/birkelund/qemu.git tags/nvme-next-pull-request

for you to fetch changes up to ea3c76f1494d0c75873c3b470e6e048202661ad8:

  hw/nvme: fix endianness issue for shadow doorbells (2023-07-19 09:33:54 +0200)

----------------------------------------------------------------
hw/nvme fixes

* fix shadow doorbell endian issue
-----BEGIN PGP SIGNATURE-----

iQEzBAABCgAdFiEEUigzqnXi3OaiR2bATeGvMW1PDekFAmS3kkAACgkQTeGvMW1P
DenG1ggArIHi1dQQBIG1ubzHx/C+93cybpKwT73/5wfO7BT8CCh1v+qrH/6SsYUT
5O7y1MaCLDV4ocf5dRQseXFK0tpjo7EqDnr25UhcSunQ+d2Tn7MAIuubQOFD+Axh
5gIwOEJbKqw9apJgnVWnInTBd//ManOgh6OyC1uJ+DEJE7ISJzLlJeWaBekiWpAA
hNL1zsR5+eTcwnewDRmMs4FlKBlSfgcNgNYnz8tfpnW0DzXKuiY4ITnk6kX9eMAM
kDlbjFjlgoTPZ8IsYcyhVCJMcH8jqY/LuZcaF7XHHsdX7fa5p17C6rR1hxVyDs+E
rydOtWetQDhXlyakE+Jp2RB3HLcSmg==
=j1TL
-----END PGP SIGNATURE-----

----------------------------------------------------------------

Klaus Jensen (1):
  hw/nvme: fix endianness issue for shadow doorbells

 hw/nvme/ctrl.c | 18 +++++++++++++-----
 1 file changed, 13 insertions(+), 5 deletions(-)

-- 
2.41.0

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

[Klaus Jensen]

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

In commit 2fda0726e514 ("hw/nvme: fix missing endian conversions for
doorbell buffers"), we fixed shadow doorbells for big-endian guests
running on little endian hosts. But I did not fix little-endian guests
on big-endian hosts. Fix this.

Resolves: https://gitlab.com/qemu-project/qemu/-/issues/1765
Fixes: 3f7fe8de3d49 ("hw/nvme: Implement shadow doorbell buffer support")
Cc: qemu-stable@nongnu.org
Reported-by: Thomas Huth <thuth@redhat.com>
Tested-by: Cédric Le Goater <clg@redhat.com>
Tested-by: Thomas Huth <thuth@redhat.com>
Reviewed-by: Keith Busch <kbusch@kernel.org>
Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>
Signed-off-by: Klaus Jensen <k.jensen@samsung.com>
---
 hw/nvme/ctrl.c | 18 +++++++++++++-----
 1 file changed, 13 insertions(+), 5 deletions(-)

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);
-- 
2.41.0

Re: [PULL 0/1] hw/nvme fixes

[Peter Maydell]

On Wed, 19 Jul 2023 at 08:36, Klaus Jensen <its@irrelevant.dk> wrote:
>
> From: Klaus Jensen <k.jensen@samsung.com>
>
> Hi,
>
> The following changes since commit 361d5397355276e3007825cc17217c1e4d4320f7:
>
>   Merge tag 'block-pull-request' of https://gitlab.com/stefanha/qemu into staging (2023-07-17 15:49:27 +0100)
>
> are available in the Git repository at:
>
>   https://gitlab.com/birkelund/qemu.git tags/nvme-next-pull-request
>
> for you to fetch changes up to ea3c76f1494d0c75873c3b470e6e048202661ad8:
>
>   hw/nvme: fix endianness issue for shadow doorbells (2023-07-19 09:33:54 +0200)
>
> ----------------------------------------------------------------
> hw/nvme fixes
>
> * fix shadow doorbell endian issue
> -----BEGIN PGP SIGNATURE-----


Applied, thanks.

Please update the changelog at https://wiki.qemu.org/ChangeLog/8.1
for any user-visible changes.

-- PMM

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));
          }

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

[Peter Maydell]

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

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

[Klaus Jensen]

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

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.

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

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

[Peter Maydell]

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

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

[Klaus Jensen]

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

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.

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