[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [PATCH 02/16] hw/block/nvme: add mapping helpers
From: |
Andrzej Jakowski |
Subject: |
Re: [PATCH 02/16] hw/block/nvme: add mapping helpers |
Date: |
Wed, 29 Jul 2020 14:51:04 -0700 |
User-agent: |
Mozilla/5.0 (X11; Linux x86_64; rv:68.0) Gecko/20100101 Thunderbird/68.10.0 |
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?
[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 03/16] hw/block/nvme: replace dma_acct with blk_acct equivalent, Klaus Jensen, 2020/07/20