Re: [PATCH 05/16] hw/block/nvme: refactor dma read/write

[Klaus Jensen]

On Jul 29 20:35, Maxim Levitsky wrote:
> On Mon, 2020-07-20 at 13:37 +0200, Klaus Jensen wrote:
> > From: Klaus Jensen <k.jensen@samsung.com>
> > 
> > Refactor the nvme_dma_{read,write}_prp functions into a common function
> > taking a DMADirection parameter.
> > 
> > Signed-off-by: Klaus Jensen <k.jensen@samsung.com>
> > Reviewed-by: Maxim Levitsky <mlevitsk@redhat.com>
> > ---
> >  hw/block/nvme.c | 88 ++++++++++++++++++++++++-------------------------
> >  1 file changed, 43 insertions(+), 45 deletions(-)
> > 
> > diff --git a/hw/block/nvme.c b/hw/block/nvme.c
> > index 6a1a1626b87b..d314a604db81 100644
> > --- a/hw/block/nvme.c
> > +++ b/hw/block/nvme.c
> > @@ -361,55 +361,50 @@ unmap:
> >      return status;
> >  }
> >  
> > -static uint16_t nvme_dma_write_prp(NvmeCtrl *n, uint8_t *ptr, uint32_t len,
> > -                                   uint64_t prp1, uint64_t prp2)
> > +static uint16_t nvme_dma_prp(NvmeCtrl *n, uint8_t *ptr, uint32_t len,
> > +                             uint64_t prp1, uint64_t prp2, DMADirection 
> > dir)
> >  {
> >      QEMUSGList qsg;
> >      QEMUIOVector iov;
> >      uint16_t status = NVME_SUCCESS;
> >  
> > -    if (nvme_map_prp(&qsg, &iov, prp1, prp2, len, n)) {
> > -        return NVME_INVALID_FIELD | NVME_DNR;
> > +    status = nvme_map_prp(&qsg, &iov, prp1, prp2, len, n);
> > +    if (status) {
> > +        return status;
> >      }
> > +
> >      if (qsg.nsg > 0) {
> > -        if (dma_buf_write(ptr, len, &qsg)) {
> > -            status = NVME_INVALID_FIELD | NVME_DNR;
> > +        uint64_t residual;
> > +
> > +        if (dir == DMA_DIRECTION_TO_DEVICE) {
> > +            residual = dma_buf_write(ptr, len, &qsg);
> > +        } else {
> > +            residual = dma_buf_read(ptr, len, &qsg);
> >          }
> > -        qemu_sglist_destroy(&qsg);
> > -    } else {
> > -        if (qemu_iovec_to_buf(&iov, 0, ptr, len) != len) {
> > -            status = NVME_INVALID_FIELD | NVME_DNR;
> > -        }
> > -        qemu_iovec_destroy(&iov);
> > -    }
> > -    return status;
> > -}
> >  
> > -static uint16_t nvme_dma_read_prp(NvmeCtrl *n, uint8_t *ptr, uint32_t len,
> > -    uint64_t prp1, uint64_t prp2)
> > -{
> > -    QEMUSGList qsg;
> > -    QEMUIOVector iov;
> > -    uint16_t status = NVME_SUCCESS;
> > -
> > -    trace_pci_nvme_dma_read(prp1, prp2);
> > -
> > -    if (nvme_map_prp(&qsg, &iov, prp1, prp2, len, n)) {
> > -        return NVME_INVALID_FIELD | NVME_DNR;
> > -    }
> > -    if (qsg.nsg > 0) {
> > -        if (unlikely(dma_buf_read(ptr, len, &qsg))) {
> > +        if (unlikely(residual)) {
> >              trace_pci_nvme_err_invalid_dma();
> >              status = NVME_INVALID_FIELD | NVME_DNR;
> >          }
> > +
> >          qemu_sglist_destroy(&qsg);
> >      } else {
> > -        if (unlikely(qemu_iovec_from_buf(&iov, 0, ptr, len) != len)) {
> > +        size_t bytes;
> > +
> > +        if (dir == DMA_DIRECTION_TO_DEVICE) {
> > +            bytes = qemu_iovec_to_buf(&iov, 0, ptr, len);
> > +        } else {
> > +            bytes = qemu_iovec_from_buf(&iov, 0, ptr, len);
> > +        }
> > +
> > +        if (unlikely(bytes != len)) {
> >              trace_pci_nvme_err_invalid_dma();
> >              status = NVME_INVALID_FIELD | NVME_DNR;
> >          }
> > +
> >          qemu_iovec_destroy(&iov);
> >      }
> > +
> I know I reviewed this, but thinking now, why not to add an assert here
> that we don't have both iov and qsg with data.
> 

Good point. I added it after the nvme_map_prp call.