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

[Philippe Mathieu-Daudé]

On 18/7/23 13:34, Klaus Jensen wrote:
> On Jul 18 13:18, Philippe Mathieu-Daudé wrote:
> > On 18/7/23 12:35, Klaus Jensen wrote:
> > > 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.
> > >
> > > Solves issue #1765.
> > >
> > > Fixes: 3f7fe8de3d49 ("hw/nvme: Implement shadow doorbell buffer support")
> > > Cc: qemu-stable@nongnu.org
> > > Reported-by: Thomas Huth <thuth@redhat.com>
> > > 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));
> >
> > Or use the PCI DMA ldst API which does the swapping for you:
> >
> >    stl_le_pci_dma(pci, sq->db_addr, sq->tail, MEMTXATTRS_UNSPECIFIED);
>
> Nice! That's definitely preferable. I'll queue that up for next, but
> leave this patch as-is since it's been tested on a real big-endian host
> and gotten its tags ;)

OK, then:

Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>