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/[email protected]/t/#u
signature.asc
Description: PGP signature
