Re: [PATCH 02/16] hw/block/nvme: add mapping helpers

[Andrzej Jakowski <andrzej.jakowski@linux.intel.com>]

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?