On Nov 10 14:42, Lukasz Maniak wrote: > On Mon, Nov 08, 2021 at 08:56:43AM +0100, Klaus Jensen wrote: > > 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 > > > > > > --- > > > > > > 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). > > > > Hi Klaus, > > Literal copying will still be correct and there will be no difference > between PF and VF since by prohibit we mean to disable interaction > between SR-IOV functionality and CMB/PMR for PF as well. > > if (params->sriov_max_vfs) { > if (!n->subsys) { > error_setg(errp, "subsystem is required for the use of SR-IOV"); > return; > } > > if (params->sriov_max_vfs > NVME_MAX_VFS) { > error_setg(errp, "sriov_max_vfs must be between 0 and %d", > NVME_MAX_VFS); > return; > } > > if (params->cmb_size_mb) { > error_setg(errp, "CMB is not supported with SR-IOV"); > return; > } > > if (n->pmr.dev) { > error_setg(errp, "PMR is not supported with SR-IOV"); > return; > } > Right. Understood.