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