Re: [Qemu-devel] [Qemu-block] [PATCH] nvme: add Get/Set Feature Timestamp support

["Heitke, Kenneth" <kenneth.heitke@intel.com>]

On 5/17/2019 12:24 AM, Klaus Birkelund wrote:
> On Fri, May 17, 2019 at 07:35:04AM +0200, Klaus Birkelund wrote:
>> Hi Kenneth,
>>
>> On Thu, May 16, 2019 at 05:24:47PM -0600, Heitke, Kenneth wrote:
>>> Hi Klaus, thank you for you review. I have one comment inline
>>>
>>> On 5/14/2019 12:02 AM, Klaus Birkelund wrote:
>>>> On Fri, Apr 05, 2019 at 03:41:17PM -0600, Kenneth Heitke wrote:
>>>>> Signed-off-by: Kenneth Heitke <kenneth.heitke@intel.com>
>>>>> ---
>>>>>    hw/block/nvme.c       | 120 +++++++++++++++++++++++++++++++++++++++++-
>>>>>    hw/block/nvme.h       |   3 ++
>>>>>    hw/block/trace-events |   2 +
>>>>>    include/block/nvme.h  |   2 +
>>>>>    4 files changed, 125 insertions(+), 2 deletions(-)
>>>>>
>>>>> diff --git a/hw/block/nvme.c b/hw/block/nvme.c
>>>>> index 7caf92532a..e775e89299 100644
>>>>> --- a/hw/block/nvme.c
>>>>> +++ b/hw/block/nvme.c
>>>>> @@ -219,6 +219,30 @@ static uint16_t nvme_map_prp(QEMUSGList *qsg, QEMUIOVector *iov, uint64_t prp1,
>>>>>        return NVME_INVALID_FIELD | NVME_DNR;
>>>>>    }
>>>>> +static uint16_t nvme_dma_write_prp(NvmeCtrl *n, uint8_t *ptr, uint32_t len,
>>>>> +                                   uint64_t prp1, uint64_t prp2)
>>>>> +{
>>>>> +    QEMUSGList qsg;
>>>>> +    QEMUIOVector iov;
>>>>> +    uint16_t status = NVME_SUCCESS;
>>>>> +
>>>>> +    if (nvme_map_prp(&qsg, &iov, prp1, prp2, len, n)) {
>>>>> +        return NVME_INVALID_FIELD | NVME_DNR;
>>>>> +    }
>>>>> +    if (qsg.nsg > 0) {
>>>>> +        if (dma_buf_write(ptr, len, &qsg)) {
>>>>> +            status = NVME_INVALID_FIELD | NVME_DNR;
>>>>> +        }
>>>>> +        qemu_sglist_destroy(&qsg);
>>>>> +    } else {
>>>>> +        if (qemu_iovec_from_buf(&iov, 0, ptr, len) != len) {
>>>>
>>>> This should be `qemu_iovec_to_buf`.
>>>>
>>>
>>> This function is transferring data from the "host" to the device so I
>>> believe I am using the correct function.
>>>
>>
>> Exactly, but this means that you need to populate `ptr` with data
>> described by the prps, hence dma_buf_*write* and qemu_iovec_*to*_buf. In
>> this case `ptr` is set to the address of the uint64_t timestamp, and
>> that is what we need to write to.
>>
> 
> I was going to argue with the fact that nvme_dma_read_prp uses
> qemu_iovec_from_buf. But it uses _to_buf which as far as I can tell is
> also wrong.
> 

I think the iovec_to/from_buf is only used in the case of the controller 
memory buffer. I'm trying to exercise that path to verify if the correct 
functions are being used but I'm running into issues (io is being 
directed to bar0 instead of bar2).