On Jul 29 13:40, Andrzej Jakowski wrote:
> On 7/20/20 4:37 AM, Klaus Jensen wrote:
> > From: Klaus Jensen <k.jen...@samsung.com>
> >
> > Add nvme_map_addr, nvme_map_addr_cmb and nvme_addr_to_cmb helpers and
> > use them in nvme_map_prp.
> >
> > This fixes a bug where in the case of a CMB transfer, the device would
> > map to the buffer with a wrong length.
> >
> > Fixes: b2b2b67a00574 ("nvme: Add support for Read Data and Write Data in
> > CMBs.")
> > Signed-off-by: Klaus Jensen <k.jen...@samsung.com>
> > ---
> > hw/block/nvme.c | 109 +++++++++++++++++++++++++++++++++++-------
> > hw/block/trace-events | 2 +
> > 2 files changed, 94 insertions(+), 17 deletions(-)
> >
> > diff --git a/hw/block/nvme.c b/hw/block/nvme.c
> > index 4d7b730a62b6..9b1a080cdc70 100644
> > --- a/hw/block/nvme.c
> > +++ b/hw/block/nvme.c
> > @@ -270,20 +338,27 @@ static uint16_t nvme_map_prp(QEMUSGList *qsg,
> > QEMUIOVector *iov, uint64_t prp1,
> > } else {
> > if (unlikely(prp2 & (n->page_size - 1))) {
> > trace_pci_nvme_err_invalid_prp2_align(prp2);
> > + status = NVME_INVALID_FIELD | NVME_DNR;
> > goto unmap;
> > }
> > - if (qsg->nsg) {
> > - qemu_sglist_add(qsg, prp2, len);
> > - } else {
> > - qemu_iovec_add(iov, (void *)&n->cmbuf[prp2 -
> > n->ctrl_mem.addr], trans_len);
> > + status = nvme_map_addr(n, qsg, iov, prp2, len);
> > + if (status) {
> > + goto unmap;
> > }
> > }
> > }
> > return NVME_SUCCESS;
> >
> > - unmap:
> > - qemu_sglist_destroy(qsg);
> > - return NVME_INVALID_FIELD | NVME_DNR;
> > +unmap:
> > + if (iov && iov->iov) {
> > + qemu_iovec_destroy(iov);
> > + }
> > +
> > + if (qsg && qsg->sg) {
> > + qemu_sglist_destroy(qsg);
> > + }
> > +
> > + return status;
>
> I think it would make sense to move whole unmap block to a separate function.
> That function could be called from here and after completing IO and would
> contain
> unified deinitialization block - so no code repetitions would be necessary.
> Other than that it looks good to me. Thx!
>
> Reviewed-by: Andrzej Jakowski <andrzej.jakow...@linux.intel.com>
>
Hi Andrzej,
Thanks for the review :)
Yes, this is done in a later patch ("hw/block/nvme: consolidate qsg/iov
clearing"), but kept here to reduce churn.
