Re: [PATCH v2 00/15] hw/nvme: SR-IOV with Virtualization Enhancements

[Klaus Jensen <its@irrelevant.dk>]

[-- Attachment #1: Type: text/plain, Size: 3618 bytes --]

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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]