nvme lightnvm comments

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!

Quoted 1 line

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.

Quoted 8 lines · Matias Bj?rling

Quoted 1 line

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

Then the functions

nvme_nvm_register
nvme_nvm_unregister
nvme_nvm_register

Quoted 3 lines

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.

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.

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.

Quoted 3 lines · 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.

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

Quoted 6 lines · Matias Bj?rling

Quoted 3 lines · 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.

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.

Quoted 4 lines · Keith Busch

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.

Quoted 6 lines · Matias Bj?rling

Quoted 3 lines · 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.

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.