[PATCH 0/1] hw/nvme: shadow doorbells broken on riscv64

[PATCH 0/1] hw/nvme: shadow doorbells broken on riscv64

[Klaus Jensen]

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

Guenter reports[1] that hw/nvme is broken on riscv64.

This is a regression since 7.1, so this does not warrent an rc5 for 7.2.
I'm sure Guenter can carry this patch in his tree, and maybe we can get
this out in a stable release.

I really wonder why this issue only shows up on riscv64. We have not
observed this on other platforms (yet).

  [1]: https://lore.kernel.org/qemu-devel/20221207174918.GA1151796@roeck-us.net/

Klaus Jensen (1):
  hw/nvme: fix missing cq eventidx update

 hw/nvme/ctrl.c | 8 ++++++++
 1 file changed, 8 insertions(+)

-- 
2.38.1

[PATCH 1/1] hw/nvme: fix missing cq eventidx update

[Klaus Jensen]

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

Prior to reading the shadow doorbell cq head, we have to update the
eventidx. Otherwise, we risk that the driver will skip an mmio doorbell
write. This happens on riscv64, as reported by Guenter.

Adding the missing update to the cq eventidx fixes the issue.

Fixes: 3f7fe8de3d49 ("hw/nvme: Implement shadow doorbell buffer support")
Reported-by: Guenter Roeck <linux@roeck-us.net>
Signed-off-by: Klaus Jensen <k.jensen@samsung.com>
---
 hw/nvme/ctrl.c | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/hw/nvme/ctrl.c b/hw/nvme/ctrl.c
index e54276dc1dc7..1192919b4869 100644
--- a/hw/nvme/ctrl.c
+++ b/hw/nvme/ctrl.c
@@ -1331,6 +1331,13 @@ static inline void nvme_blk_write(BlockBackend *blk, int64_t offset,
     }
 }
 
+static void nvme_update_cq_eventidx(const NvmeCQueue *cq)
+{
+    pci_dma_write(&cq->ctrl->parent_obj, cq->ei_addr, &cq->head,
+                  sizeof(cq->head));
+    trace_pci_nvme_eventidx_cq(cq->cqid, cq->head);
+}
+
 static void nvme_update_cq_head(NvmeCQueue *cq)
 {
     pci_dma_read(&cq->ctrl->parent_obj, cq->db_addr, &cq->head,
@@ -1351,6 +1358,7 @@ static void nvme_post_cqes(void *opaque)
         hwaddr addr;
 
         if (n->dbbuf_enabled) {
+            nvme_update_cq_eventidx(cq);
             nvme_update_cq_head(cq);
         }
 
-- 
2.38.1

Re: [PATCH 1/1] hw/nvme: fix missing cq eventidx update

[Philippe Mathieu-Daudé]

On 8/12/22 09:29, Klaus Jensen wrote:
> From: Klaus Jensen <k.jensen@samsung.com>
> 
> Prior to reading the shadow doorbell cq head, we have to update the
> eventidx. Otherwise, we risk that the driver will skip an mmio doorbell
> write. This happens on riscv64, as reported by Guenter.
> 
> Adding the missing update to the cq eventidx fixes the issue.
> 
> Fixes: 3f7fe8de3d49 ("hw/nvme: Implement shadow doorbell buffer support")
> Reported-by: Guenter Roeck <linux@roeck-us.net>
> Signed-off-by: Klaus Jensen <k.jensen@samsung.com>
> ---
>   hw/nvme/ctrl.c | 8 ++++++++
>   1 file changed, 8 insertions(+)
> 
> diff --git a/hw/nvme/ctrl.c b/hw/nvme/ctrl.c
> index e54276dc1dc7..1192919b4869 100644
> --- a/hw/nvme/ctrl.c
> +++ b/hw/nvme/ctrl.c
> @@ -1331,6 +1331,13 @@ static inline void nvme_blk_write(BlockBackend *blk, int64_t offset,
>       }
>   }
>   
> +static void nvme_update_cq_eventidx(const NvmeCQueue *cq)
> +{
> +    pci_dma_write(&cq->ctrl->parent_obj, cq->ei_addr, &cq->head,

'parent_obj' is a private field. Better to use the QOM accessor:

        pci_dma_write(PCI_DEVICE(&cq->ctrl), cq->ei_addr, &cq->head,

> +                  sizeof(cq->head));
> +    trace_pci_nvme_eventidx_cq(cq->cqid, cq->head);

Surprisingly the trace event format was already present in Jinhao respin...
https://lore.kernel.org/all/YqsIh+OUcWnHU3zp@apples/T/

Could we move the event before the call?

> +}
> +
>   static void nvme_update_cq_head(NvmeCQueue *cq)
>   {
>       pci_dma_read(&cq->ctrl->parent_obj, cq->db_addr, &cq->head,
> @@ -1351,6 +1358,7 @@ static void nvme_post_cqes(void *opaque)
>           hwaddr addr;
>   
>           if (n->dbbuf_enabled) {
> +            nvme_update_cq_eventidx(cq);
>               nvme_update_cq_head(cq);
>           }
>   

Re: [PATCH 1/1] hw/nvme: fix missing cq eventidx update

[Klaus Jensen]

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

On Dec  8 11:14, Philippe Mathieu-Daudé wrote:
> On 8/12/22 09:29, Klaus Jensen wrote:
> > From: Klaus Jensen <k.jensen@samsung.com>
> > 
> > Prior to reading the shadow doorbell cq head, we have to update the
> > eventidx. Otherwise, we risk that the driver will skip an mmio doorbell
> > write. This happens on riscv64, as reported by Guenter.
> > 
> > Adding the missing update to the cq eventidx fixes the issue.
> > 
> > Fixes: 3f7fe8de3d49 ("hw/nvme: Implement shadow doorbell buffer support")
> > Reported-by: Guenter Roeck <linux@roeck-us.net>
> > Signed-off-by: Klaus Jensen <k.jensen@samsung.com>
> > ---
> >   hw/nvme/ctrl.c | 8 ++++++++
> >   1 file changed, 8 insertions(+)
> > 
> > diff --git a/hw/nvme/ctrl.c b/hw/nvme/ctrl.c
> > index e54276dc1dc7..1192919b4869 100644
> > --- a/hw/nvme/ctrl.c
> > +++ b/hw/nvme/ctrl.c
> > @@ -1331,6 +1331,13 @@ static inline void nvme_blk_write(BlockBackend *blk, int64_t offset,
> >       }
> >   }
> > +static void nvme_update_cq_eventidx(const NvmeCQueue *cq)
> > +{
> > +    pci_dma_write(&cq->ctrl->parent_obj, cq->ei_addr, &cq->head,
> 
> 'parent_obj' is a private field. Better to use the QOM accessor:
> 
>        pci_dma_write(PCI_DEVICE(&cq->ctrl), cq->ei_addr, &cq->head,

Ah yes, of course. I think we have a couple of other ->parent_obj
accesses that we should fix also.

> 
> > +                  sizeof(cq->head));
> > +    trace_pci_nvme_eventidx_cq(cq->cqid, cq->head);
> 
> Surprisingly the trace event format was already present in Jinhao respin...
> https://lore.kernel.org/all/YqsIh+OUcWnHU3zp@apples/T/
> 
> Could we move the event before the call?
> 

Makes sense.

> > +}
> > +
> >   static void nvme_update_cq_head(NvmeCQueue *cq)
> >   {
> >       pci_dma_read(&cq->ctrl->parent_obj, cq->db_addr, &cq->head,
> > @@ -1351,6 +1358,7 @@ static void nvme_post_cqes(void *opaque)
> >           hwaddr addr;
> >           if (n->dbbuf_enabled) {
> > +            nvme_update_cq_eventidx(cq);
> >               nvme_update_cq_head(cq);
> >           }
> 

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

Re: [PATCH 0/1] hw/nvme: shadow doorbells broken on riscv64

[Philippe Mathieu-Daudé]

On 8/12/22 09:29, Klaus Jensen wrote:
> From: Klaus Jensen <k.jensen@samsung.com>
> 
> Guenter reports[1] that hw/nvme is broken on riscv64.
> 
> This is a regression since 7.1, so this does not warrent an rc5 for 7.2.
> I'm sure Guenter can carry this patch in his tree, and maybe we can get
> this out in a stable release.

Delaying to 8.0 and adding a 'Cc: qemu-stable@' tag seems wise.

Cc'ing riscv list in case.

> I really wonder why this issue only shows up on riscv64. We have not
> observed this on other platforms (yet).
> 
>    [1]: https://lore.kernel.org/qemu-devel/20221207174918.GA1151796@roeck-us.net/
> 
> Klaus Jensen (1):
>    hw/nvme: fix missing cq eventidx update
> 
>   hw/nvme/ctrl.c | 8 ++++++++
>   1 file changed, 8 insertions(+)
>