[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [PATCH 02/16] hw/block/nvme: add mapping helpers
From: |
Klaus Jensen |
Subject: |
Re: [PATCH 02/16] hw/block/nvme: add mapping helpers |
Date: |
Wed, 29 Jul 2020 23:53:45 +0200 |
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.jensen@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.jensen@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.jakowski@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 ;)
- Re: [PATCH 05/16] hw/block/nvme: refactor dma read/write, (continued)
[PATCH 04/16] hw/block/nvme: remove redundant has_sg member, Klaus Jensen, 2020/07/20
[PATCH 06/16] hw/block/nvme: pass request along for tracing, Klaus Jensen, 2020/07/20
[PATCH 10/16] hw/block/nvme: add check for mdts, Klaus Jensen, 2020/07/20