On Jul 29 14:51, Andrzej Jakowski wrote: > On 7/29/20 2:24 PM, Klaus Jensen wrote: > > 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. > > > Yep, noticed that after sending email :) > Do you plan to submit second version of these patches incorporating some > of the feedback? >
Yes, so you can defer your reviews for v2 ;)