On Nov 25 15:15, Łukasz Gieryk wrote:
> On Wed, Nov 24, 2021 at 09:03:06AM +0100, Klaus Jensen wrote:
> > Hi Lukasz,
> >
> > I've been through this. I have a couple of review comments, but overall
> > looks good for inclusion in nvme-next. Would be nice to get this in
> > early in the cycle so it can mature there for 7.0.
>
> We (I’m speaking on behalf of the other Lukasz) are really happy to
> read that. We will do our best to make it happen.
>
Keith, do you have any comments on this series - otherwise I'd like to
stage this for nvme-next come January.
> >
> > I'd like that we mark this support experimental, so we can easily do
> > some changes to how parameters work since I'm not sure we completely
> > agree on that yet.
> >
> > By the way, in the future, please add me and Keith as CCs on the entire
> > series so we get CC'ed on replies to the cover-letter ;)
> >
>
> > > List of known gaps and nice-to-haves:
> > >
> > > 1) Interaction of secondary controllers with namespaces is not 100%
> > > following the spec
> > >
> > > The limitation: VF has to be visible on the PCI bus first, and only then
> > > such VF can have a namespace attached.
> > >
> >
> > Looking at the spec I'm not even sure what the expected behavior is
> > supposed to be, can you elaborate? I rebased this on latest, and with
> > Hannes changes, shared namespaces will be attached by default, which
> > seems to be reasonable.
>
> An example flow:
>
> # Release flexible resources from PF (assuming it’s /dev/nvme0)
> nvme virt-mgmt -c 0 -r 0 -n 0 -a 1 /dev/nvme0
> nvme virt-mgmt -c 0 -r 1 -n 0 -a 1 /dev/nvme0
> echo 1 > /sys/class/nvme/nvme0/reset_controller
> # Bind sane minimums to VF1 (cntlid=1) and set it online
> nvme virt-mgmt -c 1 -r 0 -n 5 -a 8 /dev/nvme0
> nvme virt-mgmt -c 1 -r 1 -n 5 -a 8 /dev/nvme0
> nvme virt-mgmt -c 1 -a 9 /dev/nvme0
> # Enable 2 VFs
> echo 2 > /sys/bus/pci/devices/<PF’s id>/sriov_numvfs
> # PF, VF1 and VF2 are visible on PCI
> lspci | grep Non-Volatile
> # The NVMe driver is bound to PF and VF1 (the only online VF)
> nvme list -v
> # VFs shall eventually not support Ns Management/Attachment commands,
> # and namespaces should be attached to VFs (i.e., their secondary
> # controllers) through the PF.
> # A namespace can be attached to VF1, VF2
> nvme attach-ns /dev/nvme0 -c 1 -n X
> nvme attach-ns /dev/nvme0 -c 2 -n X
> # According to the spec this should also succeed, but today it won’t
> nvme attach-ns /dev/nvme0 -c 3 -n X
>
> VF3’s NvmeCtrl object is not yet allocated, so today there’s nothing
> for nvme_subsys_ctrl() to return for cntlid=3, besides NULL (the
> current behavior) or SUBSYS_SLOT_RSVD.
>
> Relevant use cases:
> - admin can configure disabled VFs,
> - information about attached ns persists when VFs are disabled,
> are not that critical, but of course it’s a discrepancy from what a
> real device can handle.
>
> In my opinion, to handle the cases correctly, information about attached
> namespaces could be moved to subsystem. Could you share your thoughts
> whether such approach would make sense?
>
Thanks for the explaination.
I actually already had this sort-of conversation with Hannes[1].
Different issue, but same solution (that is, having a per-CNTLID
namespace list maintained in the subsystem).
I have a refactor series coming up that will address this, so for now, I
don't worry about this not being implemented exactly as the spec defines.
[1]: https://lore.kernel.org/all/20210909094308.122038-1-hare@suse.de/t/#u