Re: [PATCH v6 14/42] nvme: add missing mandatory features

[Klaus Birkelund Jensen]

On Mar 31 12:39, Maxim Levitsky wrote:
> On Tue, 2020-03-31 at 07:41 +0200, Klaus Birkelund Jensen wrote:
> > On Mar 25 12:41, Maxim Levitsky wrote:
> > > BTW the user of the device doesn't have to have 1:1 mapping between qid 
> > > and msi interrupt index,
> > > in fact when MSI is not used, all the queues will map to the same vector, 
> > > which will be interrupt 0
> > > from point of view of the device IMHO.
> > > So it kind of makes sense IMHO to have num_irqs or something, even if it 
> > > technically equals to number of queues.
> > > 
> > 
> > Yeah, but the device will still *support* the N IVs, so they can still
> > be configured even though they will not be used. So I don't think we
> > need to introduce an additional parameter?
> 
> Yes and no.
> I wasn't thinking to add a new parameter for number of supporter interrupt 
> vectors,
> but just to have an internal variable to represent it so that we could 
> support in future
> case where these are not equal.
> 
> Also from point of view of validating the users of this virtual nvme drive, I 
> think it kind
> of makes sense to allow having less supported IRQ vectors than IO queues, so 
> to check
> how userspace copes with it. It is valid after all to have same interrupt 
> vector shared between
> multiple queues.
> 

I see that this could be useful for testing, but I think we can defer
that to a later patch. Would you be okay with that for now?

> In fact in theory (but that would complicate the implementation greatly) we 
> should even support
> case when number of submission queues is not equal to number of completion 
> queues. Yes nobody does in real hardware,
> and at least Linux nvme driver hard assumes 1:1 SQ/CQ mapping but still.
> 

It is not the hardware that decides this and I believe that there
definitely are applications that chooses to associate multiple SQs with
a single CQ. The CQ is an attribute of the SQ and the IV of the CQ is
also specified in the create command. I believe this is already
supported.

> My nvme-mdev doesn't make this assumpiton (and neither any assumptions on 
> interrupt vector counts) 
> and allows the user to have any SQ/CQ mapping as far as the spec allows
> (but it does hardcode maximum number of SQ/CQ supported)
> 
> BTW, I haven't looked at that but we should check that the virtual nvme drive 
> can cope with using legacy
> interrupt (that is MSI disabled) - nvme-mdev does support this and was tested 
> with it.
> 

Yes, this is definitely not very well tested.

If you insist on all of the above being implemented, then I will do it,
but I would rather defer this to later patches as this series is already
pretty large ;)