| Message ID | 20221208082955.51732-2-its@irrelevant.dk |
|---|---|
| State | New |
| Headers | show |
| Series | hw/nvme: shadow doorbells broken on riscv64 | expand |
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); > } >
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); > > } >
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); }