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

[Klaus Jensen]

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

signature.asc
Description: PGP signature