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

[Klaus Jensen <its@irrelevant.dk>]

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

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

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