Re: [PATCH 05/15] hw/nvme: Add support for SR-IOV

[Klaus Jensen]

On Nov  4 15:30, Lukasz Maniak wrote:
> On Tue, Nov 02, 2021 at 06:33:31PM +0100, Lukasz Maniak wrote:
> > On Tue, Nov 02, 2021 at 03:33:15PM +0100, Klaus Jensen wrote:
> > > On Oct  7 18:23, Lukasz Maniak wrote:
> > > > This patch implements initial support for Single Root I/O Virtualization
> > > > on an NVMe device.
> > > > 
> > > > Essentially, it allows to define the maximum number of virtual functions
> > > > supported by the NVMe controller via sriov_max_vfs parameter.
> > > > 
> > > > Passing a non-zero value to sriov_max_vfs triggers reporting of SR-IOV
> > > > capability by a physical controller and ARI capability by both the
> > > > physical and virtual function devices.
> > > > 
> > > > NVMe controllers created via virtual functions mirror functionally
> > > > the physical controller, which may not entirely be the case, thus
> > > > consideration would be needed on the way to limit the capabilities of
> > > > the VF.
> > > > 
> > > > NVMe subsystem is required for the use of SR-IOV.
> > > > 
> > > > Signed-off-by: Lukasz Maniak <lukasz.maniak@linux.intel.com>
> > > > ---
> > > >  hw/nvme/ctrl.c           | 74 ++++++++++++++++++++++++++++++++++++++--
> > > >  hw/nvme/nvme.h           |  1 +
> > > >  include/hw/pci/pci_ids.h |  1 +
> > > >  3 files changed, 73 insertions(+), 3 deletions(-)
> > > > 
> > > > diff --git a/hw/nvme/ctrl.c b/hw/nvme/ctrl.c
> > > > index 6a571d18cf..ad79ff0c00 100644
> > > > --- a/hw/nvme/ctrl.c
> > > > +++ b/hw/nvme/ctrl.c
> > > > @@ -6361,8 +6406,12 @@ static int nvme_init_pci(NvmeCtrl *n, PCIDevice 
> > > > *pci_dev, Error **errp)
> > > >                            n->reg_size);
> > > >      memory_region_add_subregion(&n->bar0, 0, &n->iomem);
> > > >  
> > > > -    pci_register_bar(pci_dev, 0, PCI_BASE_ADDRESS_SPACE_MEMORY |
> > > > -                     PCI_BASE_ADDRESS_MEM_TYPE_64, &n->bar0);
> > > > +    if (pci_is_vf(pci_dev)) {
> > > > +        pcie_sriov_vf_register_bar(pci_dev, 0, &n->bar0);
> > > > +    } else {
> > > > +        pci_register_bar(pci_dev, 0, PCI_BASE_ADDRESS_SPACE_MEMORY |
> > > > +                         PCI_BASE_ADDRESS_MEM_TYPE_64, &n->bar0);
> > > > +    }
> > > 
> > > I assume that the assert we are seeing means that the pci_register_bars
> > > in nvme_init_cmb and nvme_init_pmr must be changed similarly to this.
> > 
> > Assert will only arise for CMB as VF params are initialized with PF
> > params.
> > 
> > @@ -6532,6 +6585,15 @@ static void nvme_realize(PCIDevice *pci_dev, Error 
> > **errp)
> >      NvmeCtrl *n = NVME(pci_dev);
> >      NvmeNamespace *ns;
> >      Error *local_err = NULL;
> > +    NvmeCtrl *pn = NVME(pcie_sriov_get_pf(pci_dev));
> > +
> > +    if (pci_is_vf(pci_dev)) {
> > +        /* VFs derive settings from the parent. PF's lifespan exceeds
> > +         * that of VF's, so it's safe to share params.serial.
> > +         */
> > +        memcpy(&n->params, &pn->params, sizeof(NvmeParams));
> > +        n->subsys = pn->subsys;
> > +    }
> >  
> >      nvme_check_constraints(n, &local_err);
> >      if (local_err) {
> > 
> > The following simple fix will both fix assert and also allow
> > each VF to have its own CMB of the size defined for PF.
> > 
> > ---
> >  hw/nvme/ctrl.c | 13 +++++++++----
> >  1 file changed, 9 insertions(+), 4 deletions(-)
> > 
> > diff --git a/hw/nvme/ctrl.c b/hw/nvme/ctrl.c
> > index 19b32dd4da..99daa6290c 100644
> > --- a/hw/nvme/ctrl.c
> > +++ b/hw/nvme/ctrl.c
> > @@ -6837,10 +6837,15 @@ static void nvme_init_cmb(NvmeCtrl *n, PCIDevice 
> > *pci_dev)
> >      n->cmb.buf = g_malloc0(cmb_size);
> >      memory_region_init_io(&n->cmb.mem, OBJECT(n), &nvme_cmb_ops, n,
> >                            "nvme-cmb", cmb_size);
> > -    pci_register_bar(pci_dev, NVME_CMB_BIR,
> > -                     PCI_BASE_ADDRESS_SPACE_MEMORY |
> > -                     PCI_BASE_ADDRESS_MEM_TYPE_64 |
> > -                     PCI_BASE_ADDRESS_MEM_PREFETCH, &n->cmb.mem);
> > +
> > +    if (pci_is_vf(pci_dev)) {
> > +        pcie_sriov_vf_register_bar(pci_dev, NVME_CMB_BIR, &n->cmb.mem);
> > +    } else {
> > +        pci_register_bar(pci_dev, NVME_CMB_BIR,
> > +                        PCI_BASE_ADDRESS_SPACE_MEMORY |
> > +                        PCI_BASE_ADDRESS_MEM_TYPE_64 |
> > +                        PCI_BASE_ADDRESS_MEM_PREFETCH, &n->cmb.mem);
> > +    }
> >  
> >      NVME_CAP_SET_CMBS(cap, 1);
> >      stq_le_p(&n->bar.cap, cap);
> > 
> > As for PMR, it is currently only available on PF, as only PF is capable
> > of specifying the memory-backend-file object to use with PMR.
> > Otherwise, either VFs would have to share the PMR with its PF, or there
> > would be a requirement to define a memory-backend-file object for each VF.
> 
> Hi Klaus,
> 
> After some discussion, we decided to prohibit in V2 the use of CMB and
> PMR in combination with SR-IOV.
> 
> While the implementation of CMB with SR-IOV is relatively
> straightforward, PMR is not. We are committed to consistency in CMB and
> PMR design in association with SR-IOV. So we considered it best to
> disable both features and implement them in separate patches.
> 

I am completely fine with that. However, since we are copying the
parameters verbatimly, it would nice that the `info qtree` would reflect
this difference (that the parameters, say, cmb_size_mb is 0 for the
virtual controllers).

signature.asc
Description: PGP signature