Re: [PATCH 12/15] hw/nvme: Initialize capability structures for primary/secondary controllers

[Łukasz Gieryk]

On Thu, Nov 04, 2021 at 04:48:43PM +0100, Łukasz Gieryk wrote:
> On Wed, Nov 03, 2021 at 01:07:31PM +0100, Klaus Jensen wrote:
> > On Oct  7 18:24, Lukasz Maniak wrote:
> > > From: Łukasz Gieryk <lukasz.gieryk@linux.intel.com>
> > > 
> > > With two new properties (sriov_max_vi_per_vf, sriov_max_vq_per_vf) one
> > > can configure the maximum number of virtual queues and interrupts
> > > assignable to a single virtual device. The primary and secondary
> > > controller capability structures are initialized accordingly.
> > > 
> > > Since the number of available queues (interrupts) now varies between
> > > VF/PF, BAR size calculation is also adjusted.
> > > 
> > 
> > While this patch allows configuring the VQFRSM and VIFRSM fields, it
> > implicitly sets VQFRT and VIFRT (i.e. by setting them to the product of
> > sriov_max_vi_pervf and max_vfs). Which is just setting it to an upper
> > bound and this removes a testable case for host software (e.g.
> > requesting more flexible resources than what is currently available).
> > 
> > This patch also requires that these parameters are set if sriov_max_vfs
> > is. I think we can provide better defaults.
> > 
> 
> Originally I considered more params, but ended up coding the simplest,
> user-friendly solution, because I did not like the mess with so many
> parameters, and the flexibility wasn't needed for my use cases. But I do
> agree: others may need the flexibility. Case (FRT < max_vfs * FRSM) is
> valid and resembles an actual device.
> 
> > How about,
> > 
> > 1. if only sriov_max_vfs is set, then all VFs get private resources
> >    equal to max_ioqpairs. Like before this patch. This limits the number
> >    of parameters required to get a basic setup going.
> > 
> > 2. if sriov_v{q,i}_private is set (I suggested this parameter in patch
> >    10), the difference between that and max_ioqpairs become flexible
> >    resources. Also, I'd be just fine with having sriov_v{q,i}_flexible
> >    instead and just make the difference become private resources.
> >    Potato/potato.
> > 
> >    a. in the absence of sriov_max_v{q,i}_per_vf, set them to the number
> >       of calculated flexible resources.
> > 
> > This probably smells a bit like bikeshedding, but I think this gives
> > more flexibility and better defaults, which helps with verifying host
> > software.
> > 
> > If we can't agree on this now, I suggest we could go ahead and merge the
> > base functionality (i.e. private resources only) and ruminate some more
> > about these parameters.
> 
> The problem is that the spec allows VFs to support either only private,
> or only flexible resources.
> 
> At this point I have to admit, that since my use cases for
> QEMU/Nvme/SRIOV require flexible resources, I haven’t paid much
> attention to the case with VFs having private resources. So this SR/IOV
> implementation doesn’t even support such case (max_vX_per_vf != 0).
> 
> Let me summarize the possible config space, and how the current
> parameters (could) map to these (interrupt-related ones omitted):
> 
> Flexible resources not supported (not implemented):
>  - Private resources for PF     = max_ioqpairs
>  - Private resources per VF     = ?
>  - (error if flexible resources are configured)
> 
> With flexible resources:
>  - VQPRT, private resources for PF      = max_ioqpairs
>  - VQFRT, total flexible resources      = max_vq_per_vf * num_vfs
>  - VQFRSM, maximum assignable per VF    = max_vq_per_vf
>  - VQGRAN, granularity                  = #define constant
>  - (error if private resources per VF are configured)
> 
> Since I don’t want to misunderstand your suggestion: could you provide a
> similar map with your parameters, formulas, and explain how to determine
> if flexible resources are active? I want to be sure we are on the
> same page.
> 

I’ve just re-read through my email and decided that some bits need
clarification.

This implementation supports the “Flexible”-resources-only flavor of
SR/IOV, while the “Private” also could be supported. Some effort is
required to support both, and I cannot afford that (at least I cannot
commit today, neither the other Lukasz).

While I’m ready to rework the Flexible config and prepare it to be
extended later to handle the Private variant, the 2nd version of these
patches will still support the Flexible flavor only.

I will include appropriate TODO/open in the next cover letter.