Re: [PATCH 2/3] nvme: Pass pointers, not dma addresses, to nvme_get/set_features()

[Andy Lutomirski]

On Mon, Aug 29, 2016 at 11:36 PM, Christoph Hellwig <hch@xxxxxx> wrote:
> On Mon, Aug 29, 2016 at 04:20:43PM -0700, Andy Lutomirski wrote:
>> The "Set Features" command (section 5.15) Figure 103 says:
>>
>> If using PRPs, this field shall not be a pointer to a PRP List as the
>> data buffer may not cross more than one page boundary. If no data
>> structure is used as part of the specified feature, then this field is
>> not used.
>>
>> Does the Linux driver use PRPs?
>
> The Linux PCIe driver always uses PRPs - and for admin command only
> Fabrics can use SGLs anyway.
>
>> Do we need to worry about kmalloc
>> returning a buffer that spans a 4k boundary but does not span a Linux
>> page boundary?
>
> Isn't kmalloc supposed to return naturally aligned buffers?

>From brief inspection of the code, it looks like kmalloc always
returns a pointer aligned to a biggest power of two that can hold the
allocation except when it uses 96-byte or 192-byte alignment.  96 and
192 don't divide 4k.

However, I think this is all moot because I misunderstood the spec.  It says:

Data Pointer (DPTR): This field specifies the start of the data
buffer. Refer to Figure 11 for the
definition of this field. If using PRPs, this field shall not be a
pointer to a PRP List as the data buffer
may not cross more than one page boundary. If no data structure is
used as part of the specified
feature, then this field is not used.

It doesn't say "may not cross a page boundary" -- it says it may not
cross *more than one* page boundary.  I think that all it's trying to
say is that there aren't any features that have buffers larger than a
page, so no matter how they're aligned there are at most two PRP
entries, and two PRP entries can be expressed without a PRP List.

So I'm just going to remove the warning.

--Andy