nvme lightnvm comments

nvme lightnvm comments

[Christoph Hellwig]

Hi Matias,

two small comments on the NVME lighnvm support:

 1) please don't claim the existing Qemu NVMe PCI ID for lightnvm
    support.  We might need the actual vendor specific data for
    qemu purposes.  Please allocate a different PCI ID for a qemu
    emulated lightnvm nvme device
 2) please build the lightnvm.o object conditionally in the makefile

Thanks!

nvme lightnvm comments

[Matias Bjørling]

Hi Christoph,

On 11/20/2015 11:50 AM, Christoph Hellwig wrote:
> Hi Matias,
>
> two small comments on the NVME lighnvm support:
>
>   1) please don't claim the existing Qemu NVMe PCI ID for lightnvm
>      support.  We might need the actual vendor specific data for
>      qemu purposes.  Please allocate a different PCI ID for a qemu
>      emulated lightnvm nvme device

I'll look into it.

>   2) please build the lightnvm.o object conditionally in the makefile

Then the functions

nvme_nvm_register
nvme_nvm_unregister
nvme_nvm_register

have to be #ifdef'ed in the pci file. Do you want that instead? It was 
on purpose the other way around, so that the nvme_nvm_* functions was 
null functions if lightnvm isn't compiled in.

>
> Thanks!
>

nvme lightnvm comments

[Christoph Hellwig]

On Fri, Nov 20, 2015@12:06:36PM +0100, Matias Bj?rling wrote:
> >  2) please build the lightnvm.o object conditionally in the makefile
> 
> Then the functions
> 
> nvme_nvm_register
> nvme_nvm_unregister
> nvme_nvm_register
> 
> have to be #ifdef'ed in the pci file. Do you want that instead? It was on
> purpose the other way around, so that the nvme_nvm_* functions was null
> functions if lightnvm isn't compiled in.

Just keep the stubs as inlines in the header.

nvme lightnvm comments

[Matias Bjørling]

On 11/20/2015 05:47 PM, Christoph Hellwig wrote:
> On Fri, Nov 20, 2015@12:06:36PM +0100, Matias Bj?rling wrote:
>>>   2) please build the lightnvm.o object conditionally in the makefile
>>
>> Then the functions
>>
>> nvme_nvm_register
>> nvme_nvm_unregister
>> nvme_nvm_register
>>
>> have to be #ifdef'ed in the pci file. Do you want that instead? It was on
>> purpose the other way around, so that the nvme_nvm_* functions was null
>> functions if lightnvm isn't compiled in.
>
> Just keep the stubs as inlines in the header.
>


That'll do.

Do you have any plans for exposing the completion_entry on 
req_completion in the driver? I'm interested in the fields ->result and 
->rsvd in struct nvme_completion. They contain the completion bits for 
each of the PPA entries. Which I'll need higher up in the stack to 
perform correct retries and so forth.

nvme lightnvm comments

[Christoph Hellwig]

On Fri, Nov 20, 2015@10:26:27PM +0100, Matias Bj?rling wrote:
> Do you have any plans for exposing the completion_entry on req_completion in
> the driver? I'm interested in the fields ->result and ->rsvd in struct
> nvme_completion. They contain the completion bits for each of the PPA
> entries. Which I'll need higher up in the stack to perform correct retries
> and so forth.

->result is available already for passthrough commands.  But for Fabrics
I actually need the full CQE as well, and I'm working on that at the
moment.  Stay tuned!

nvme lightnvm comments

[Christoph Hellwig]

Btw, another one: it seems like LightNVM devices still claim to
use the NVM I/O set per CAP and CC, which seems a little odd.

nvme lightnvm comments

[Matias Bjørling]

On 11/21/2015 12:12 PM, Christoph Hellwig wrote:
> Btw, another one: it seems like LightNVM devices still claim to
> use the NVM I/O set per CAP and CC, which seems a little odd.
>

Oh, where have you seen it? Thought I had it changed everywhere.

nvme lightnvm comments

[Christoph Hellwig]

On Sun, Nov 22, 2015@07:17:39PM +0100, Matias Bj?rling wrote:
> On 11/21/2015 12:12 PM, Christoph Hellwig wrote:
> >Btw, another one: it seems like LightNVM devices still claim to
> >use the NVM I/O set per CAP and CC, which seems a little odd.
> >
> 
> Oh, where have you seen it? Thought I had it changed everywhere.

nvme_configure_admin_queue still hardcodes then NVM comman set when
enabling the controller.

nvme lightnvm comments

[Keith Busch]

On Mon, Nov 23, 2015@07:45:57AM -0800, Christoph Hellwig wrote:
> On Sun, Nov 22, 2015@07:17:39PM +0100, Matias Bj?rling wrote:
> > On 11/21/2015 12:12 PM, Christoph Hellwig wrote:
> > >Btw, another one: it seems like LightNVM devices still claim to
> > >use the NVM I/O set per CAP and CC, which seems a little odd.
> > >
> > 
> > Oh, where have you seen it? Thought I had it changed everywhere.
> 
> nvme_configure_admin_queue still hardcodes then NVM comman set when
> enabling the controller.

We also thought it'd have been cleaner to have a different command set,
but the committee hasn't taken on assigning these reserved bits for any
new purposes yet. LightNVM detection is done with the vendor + device
id at the moment.

nvme lightnvm comments

[Christoph Hellwig]

On Mon, Nov 23, 2015@04:03:20PM +0000, Keith Busch wrote:
> We also thought it'd have been cleaner to have a different command set,
> but the committee hasn't taken on assigning these reserved bits for any
> new purposes yet. LightNVM detection is done with the vendor + device
> id at the moment.

I saw the vendor + device, but even with that I'd rather see a reserved
value in the command set in addition.  Claiming that we're NVM if we're
not is a bad idea.

While we're at it - the NVMe driver really should check for these
reserved bits and reject a PCIe device that doesn't claim to implement
the NVM command set.

nvme lightnvm comments

[Matias Bjørling]

On 11/23/2015 08:53 PM, Christoph Hellwig wrote:
> On Mon, Nov 23, 2015@04:03:20PM +0000, Keith Busch wrote:
>> We also thought it'd have been cleaner to have a different command set,
>> but the committee hasn't taken on assigning these reserved bits for any
>> new purposes yet. LightNVM detection is done with the vendor + device
>> id at the moment.
>
> I saw the vendor + device, but even with that I'd rather see a reserved
> value in the command set in addition.  Claiming that we're NVM if we're
> not is a bad idea.
>

Well, I would too. However, we can't add a new mode without breaking 
compatibility at some other point in time. We're working on the 
standardization process.

nvme lightnvm comments

[Matias Bjørling]

On 11/23/2015 04:45 PM, Christoph Hellwig wrote:
> On Sun, Nov 22, 2015@07:17:39PM +0100, Matias Bj?rling wrote:
>> On 11/21/2015 12:12 PM, Christoph Hellwig wrote:
>>> Btw, another one: it seems like LightNVM devices still claim to
>>> use the NVM I/O set per CAP and CC, which seems a little odd.
>>>
>>
>> Oh, where have you seen it? Thought I had it changed everywhere.
>
> nvme_configure_admin_queue still hardcodes then NVM comman set when
> enabling the controller.
>

I might be wrong. That is the normal nvme command set. It isn't the 
lightnvm command set. In the previous implementation, it was called 
NVME_CC_CSS_LIGHTNVM.