| Message ID | 1563857495-26692-4-git-send-email-hao.wu@intel.com (mailing list archive) |
|---|---|
| State | Superseded, archived |
| Headers | show |
| Series | FPGA DFL updates | expand |
On Tue, Jul 23, 2019 at 12:51:26PM +0800, Wu Hao wrote: > This patch enables the standard sriov support. It allows user to > enable SRIOV (and VFs), then user could pass through accelerators > (VFs) into virtual machine or use VFs directly in host. > > Signed-off-by: Zhang Yi Z <yi.z.zhang@intel.com> > Signed-off-by: Xu Yilun <yilun.xu@intel.com> > Signed-off-by: Wu Hao <hao.wu@intel.com> > Acked-by: Alan Tull <atull@kernel.org> > Acked-by: Moritz Fischer <mdf@kernel.org> > Signed-off-by: Moritz Fischer <mdf@kernel.org> > --- > v2: remove DRV/MODULE_VERSION modifications. > --- > drivers/fpga/dfl-pci.c | 39 +++++++++++++++++++++++++++++++++++++++ > drivers/fpga/dfl.c | 41 +++++++++++++++++++++++++++++++++++++++++ > drivers/fpga/dfl.h | 1 + > 3 files changed, 81 insertions(+) > > diff --git a/drivers/fpga/dfl-pci.c b/drivers/fpga/dfl-pci.c > index 66b5720..0e65d81 100644 > --- a/drivers/fpga/dfl-pci.c > +++ b/drivers/fpga/dfl-pci.c > @@ -223,8 +223,46 @@ int cci_pci_probe(struct pci_dev *pcidev, const struct pci_device_id *pcidevid) > return ret; > } > > +static int cci_pci_sriov_configure(struct pci_dev *pcidev, int num_vfs) > +{ > + struct cci_drvdata *drvdata = pci_get_drvdata(pcidev); > + struct dfl_fpga_cdev *cdev = drvdata->cdev; > + int ret = 0; > + > + mutex_lock(&cdev->lock); > + > + if (!num_vfs) { > + /* > + * disable SRIOV and then put released ports back to default > + * PF access mode. > + */ > + pci_disable_sriov(pcidev); > + > + __dfl_fpga_cdev_config_port_vf(cdev, false); > + > + } else if (cdev->released_port_num == num_vfs) { > + /* > + * only enable SRIOV if cdev has matched released ports, put > + * released ports into VF access mode firstly. > + */ > + __dfl_fpga_cdev_config_port_vf(cdev, true); > + > + ret = pci_enable_sriov(pcidev, num_vfs); > + if (ret) > + __dfl_fpga_cdev_config_port_vf(cdev, false); > + } else { > + ret = -EINVAL; > + } > + > + mutex_unlock(&cdev->lock); > + return ret; > +} > + > static void cci_pci_remove(struct pci_dev *pcidev) > { > + if (dev_is_pf(&pcidev->dev)) > + cci_pci_sriov_configure(pcidev, 0); > + > cci_remove_feature_devs(pcidev); > pci_disable_pcie_error_reporting(pcidev); > } > @@ -234,6 +272,7 @@ static void cci_pci_remove(struct pci_dev *pcidev) > .id_table = cci_pcie_id_tbl, > .probe = cci_pci_probe, > .remove = cci_pci_remove, > + .sriov_configure = cci_pci_sriov_configure, > }; > > module_pci_driver(cci_pci_driver); > diff --git a/drivers/fpga/dfl.c b/drivers/fpga/dfl.c > index e04ed45..c3a8e1d 100644 > --- a/drivers/fpga/dfl.c > +++ b/drivers/fpga/dfl.c > @@ -1112,6 +1112,47 @@ int dfl_fpga_cdev_config_port(struct dfl_fpga_cdev *cdev, int port_id, > } > EXPORT_SYMBOL_GPL(dfl_fpga_cdev_config_port); > > +static void config_port_vf(struct device *fme_dev, int port_id, bool is_vf) > +{ > + void __iomem *base; > + u64 v; > + > + base = dfl_get_feature_ioaddr_by_id(fme_dev, FME_FEATURE_ID_HEADER); > + > + v = readq(base + FME_HDR_PORT_OFST(port_id)); > + > + v &= ~FME_PORT_OFST_ACC_CTRL; > + v |= FIELD_PREP(FME_PORT_OFST_ACC_CTRL, > + is_vf ? FME_PORT_OFST_ACC_VF : FME_PORT_OFST_ACC_PF); > + > + writeq(v, base + FME_HDR_PORT_OFST(port_id)); > +} > + > +/** > + * __dfl_fpga_cdev_config_port_vf - configure port to VF access mode > + * > + * @cdev: parent container device. > + * @if_vf: true for VF access mode, and false for PF access mode > + * > + * Return: 0 on success, negative error code otherwise. > + * > + * This function is needed in sriov configuration routine. It could be used to > + * configures the released ports access mode to VF or PF. > + * The caller needs to hold lock for protection. > + */ > +void __dfl_fpga_cdev_config_port_vf(struct dfl_fpga_cdev *cdev, bool is_vf) > +{ > + struct dfl_feature_platform_data *pdata; > + > + list_for_each_entry(pdata, &cdev->port_dev_list, node) { > + if (device_is_registered(&pdata->dev->dev)) > + continue; > + > + config_port_vf(cdev->fme_dev, pdata->id, is_vf); > + } > +} > +EXPORT_SYMBOL_GPL(__dfl_fpga_cdev_config_port_vf); Why are you exporting a function with a leading __? You are expecting someone else, in who knows what code, to do locking correctly? If so, and the caller always has to have a local lock, then it's not a big deal, just drop the '__', otherwise if you have to have a specific lock for a specific device, then you have a really complex and probably broken api here :( thanks, greg k-h
On Wed, Jul 24, 2019 at 11:37:44AM +0200, Greg KH wrote: > On Tue, Jul 23, 2019 at 12:51:26PM +0800, Wu Hao wrote: > > This patch enables the standard sriov support. It allows user to > > enable SRIOV (and VFs), then user could pass through accelerators > > (VFs) into virtual machine or use VFs directly in host. > > > > Signed-off-by: Zhang Yi Z <yi.z.zhang@intel.com> > > Signed-off-by: Xu Yilun <yilun.xu@intel.com> > > Signed-off-by: Wu Hao <hao.wu@intel.com> > > Acked-by: Alan Tull <atull@kernel.org> > > Acked-by: Moritz Fischer <mdf@kernel.org> > > Signed-off-by: Moritz Fischer <mdf@kernel.org> > > --- > > v2: remove DRV/MODULE_VERSION modifications. > > --- > > drivers/fpga/dfl-pci.c | 39 +++++++++++++++++++++++++++++++++++++++ > > drivers/fpga/dfl.c | 41 +++++++++++++++++++++++++++++++++++++++++ > > drivers/fpga/dfl.h | 1 + > > 3 files changed, 81 insertions(+) > > > > diff --git a/drivers/fpga/dfl-pci.c b/drivers/fpga/dfl-pci.c > > index 66b5720..0e65d81 100644 > > --- a/drivers/fpga/dfl-pci.c > > +++ b/drivers/fpga/dfl-pci.c > > @@ -223,8 +223,46 @@ int cci_pci_probe(struct pci_dev *pcidev, const struct pci_device_id *pcidevid) > > return ret; > > } > > > > +static int cci_pci_sriov_configure(struct pci_dev *pcidev, int num_vfs) > > +{ > > + struct cci_drvdata *drvdata = pci_get_drvdata(pcidev); > > + struct dfl_fpga_cdev *cdev = drvdata->cdev; > > + int ret = 0; > > + > > + mutex_lock(&cdev->lock); > > + > > + if (!num_vfs) { > > + /* > > + * disable SRIOV and then put released ports back to default > > + * PF access mode. > > + */ > > + pci_disable_sriov(pcidev); > > + > > + __dfl_fpga_cdev_config_port_vf(cdev, false); > > + > > + } else if (cdev->released_port_num == num_vfs) { > > + /* > > + * only enable SRIOV if cdev has matched released ports, put > > + * released ports into VF access mode firstly. > > + */ > > + __dfl_fpga_cdev_config_port_vf(cdev, true); > > + > > + ret = pci_enable_sriov(pcidev, num_vfs); > > + if (ret) > > + __dfl_fpga_cdev_config_port_vf(cdev, false); > > + } else { > > + ret = -EINVAL; > > + } > > + > > + mutex_unlock(&cdev->lock); > > + return ret; > > +} > > + > > static void cci_pci_remove(struct pci_dev *pcidev) > > { > > + if (dev_is_pf(&pcidev->dev)) > > + cci_pci_sriov_configure(pcidev, 0); > > + > > cci_remove_feature_devs(pcidev); > > pci_disable_pcie_error_reporting(pcidev); > > } > > @@ -234,6 +272,7 @@ static void cci_pci_remove(struct pci_dev *pcidev) > > .id_table = cci_pcie_id_tbl, > > .probe = cci_pci_probe, > > .remove = cci_pci_remove, > > + .sriov_configure = cci_pci_sriov_configure, > > }; > > > > module_pci_driver(cci_pci_driver); > > diff --git a/drivers/fpga/dfl.c b/drivers/fpga/dfl.c > > index e04ed45..c3a8e1d 100644 > > --- a/drivers/fpga/dfl.c > > +++ b/drivers/fpga/dfl.c > > @@ -1112,6 +1112,47 @@ int dfl_fpga_cdev_config_port(struct dfl_fpga_cdev *cdev, int port_id, > > } > > EXPORT_SYMBOL_GPL(dfl_fpga_cdev_config_port); > > > > +static void config_port_vf(struct device *fme_dev, int port_id, bool is_vf) > > +{ > > + void __iomem *base; > > + u64 v; > > + > > + base = dfl_get_feature_ioaddr_by_id(fme_dev, FME_FEATURE_ID_HEADER); > > + > > + v = readq(base + FME_HDR_PORT_OFST(port_id)); > > + > > + v &= ~FME_PORT_OFST_ACC_CTRL; > > + v |= FIELD_PREP(FME_PORT_OFST_ACC_CTRL, > > + is_vf ? FME_PORT_OFST_ACC_VF : FME_PORT_OFST_ACC_PF); > > + > > + writeq(v, base + FME_HDR_PORT_OFST(port_id)); > > +} > > + > > +/** > > + * __dfl_fpga_cdev_config_port_vf - configure port to VF access mode > > + * > > + * @cdev: parent container device. > > + * @if_vf: true for VF access mode, and false for PF access mode > > + * > > + * Return: 0 on success, negative error code otherwise. > > + * > > + * This function is needed in sriov configuration routine. It could be used to > > + * configures the released ports access mode to VF or PF. > > + * The caller needs to hold lock for protection. > > + */ > > +void __dfl_fpga_cdev_config_port_vf(struct dfl_fpga_cdev *cdev, bool is_vf) > > +{ > > + struct dfl_feature_platform_data *pdata; > > + > > + list_for_each_entry(pdata, &cdev->port_dev_list, node) { > > + if (device_is_registered(&pdata->dev->dev)) > > + continue; > > + > > + config_port_vf(cdev->fme_dev, pdata->id, is_vf); > > + } > > +} > > +EXPORT_SYMBOL_GPL(__dfl_fpga_cdev_config_port_vf); > > Why are you exporting a function with a leading __? > > You are expecting someone else, in who knows what code, to do locking > correctly? If so, and the caller always has to have a local lock, then > it's not a big deal, just drop the '__', otherwise if you have to have a > specific lock for a specific device, then you have a really complex and > probably broken api here :( Yes, I just want to remind the user of this API, caller needs to hold the lock to protect the list. I fully agree, it does make sense to make the APIs easy to use. I will try to improve this, maybe move the lock inside this function, then API user doesn't need to know the details of locking. Thanks a lot for the comments, it really helps. Hao > > thanks, > > greg k-h
diff --git a/drivers/fpga/dfl-pci.c b/drivers/fpga/dfl-pci.c index 66b5720..0e65d81 100644 --- a/drivers/fpga/dfl-pci.c +++ b/drivers/fpga/dfl-pci.c @@ -223,8 +223,46 @@ int cci_pci_probe(struct pci_dev *pcidev, const struct pci_device_id *pcidevid) return ret; } +static int cci_pci_sriov_configure(struct pci_dev *pcidev, int num_vfs) +{ + struct cci_drvdata *drvdata = pci_get_drvdata(pcidev); + struct dfl_fpga_cdev *cdev = drvdata->cdev; + int ret = 0; + + mutex_lock(&cdev->lock); + + if (!num_vfs) { + /* + * disable SRIOV and then put released ports back to default + * PF access mode. + */ + pci_disable_sriov(pcidev); + + __dfl_fpga_cdev_config_port_vf(cdev, false); + + } else if (cdev->released_port_num == num_vfs) { + /* + * only enable SRIOV if cdev has matched released ports, put + * released ports into VF access mode firstly. + */ + __dfl_fpga_cdev_config_port_vf(cdev, true); + + ret = pci_enable_sriov(pcidev, num_vfs); + if (ret) + __dfl_fpga_cdev_config_port_vf(cdev, false); + } else { + ret = -EINVAL; + } + + mutex_unlock(&cdev->lock); + return ret; +} + static void cci_pci_remove(struct pci_dev *pcidev) { + if (dev_is_pf(&pcidev->dev)) + cci_pci_sriov_configure(pcidev, 0); + cci_remove_feature_devs(pcidev); pci_disable_pcie_error_reporting(pcidev); } @@ -234,6 +272,7 @@ static void cci_pci_remove(struct pci_dev *pcidev) .id_table = cci_pcie_id_tbl, .probe = cci_pci_probe, .remove = cci_pci_remove, + .sriov_configure = cci_pci_sriov_configure, }; module_pci_driver(cci_pci_driver); diff --git a/drivers/fpga/dfl.c b/drivers/fpga/dfl.c index e04ed45..c3a8e1d 100644 --- a/drivers/fpga/dfl.c +++ b/drivers/fpga/dfl.c @@ -1112,6 +1112,47 @@ int dfl_fpga_cdev_config_port(struct dfl_fpga_cdev *cdev, int port_id, } EXPORT_SYMBOL_GPL(dfl_fpga_cdev_config_port); +static void config_port_vf(struct device *fme_dev, int port_id, bool is_vf) +{ + void __iomem *base; + u64 v; + + base = dfl_get_feature_ioaddr_by_id(fme_dev, FME_FEATURE_ID_HEADER); + + v = readq(base + FME_HDR_PORT_OFST(port_id)); + + v &= ~FME_PORT_OFST_ACC_CTRL; + v |= FIELD_PREP(FME_PORT_OFST_ACC_CTRL, + is_vf ? FME_PORT_OFST_ACC_VF : FME_PORT_OFST_ACC_PF); + + writeq(v, base + FME_HDR_PORT_OFST(port_id)); +} + +/** + * __dfl_fpga_cdev_config_port_vf - configure port to VF access mode + * + * @cdev: parent container device. + * @if_vf: true for VF access mode, and false for PF access mode + * + * Return: 0 on success, negative error code otherwise. + * + * This function is needed in sriov configuration routine. It could be used to + * configures the released ports access mode to VF or PF. + * The caller needs to hold lock for protection. + */ +void __dfl_fpga_cdev_config_port_vf(struct dfl_fpga_cdev *cdev, bool is_vf) +{ + struct dfl_feature_platform_data *pdata; + + list_for_each_entry(pdata, &cdev->port_dev_list, node) { + if (device_is_registered(&pdata->dev->dev)) + continue; + + config_port_vf(cdev->fme_dev, pdata->id, is_vf); + } +} +EXPORT_SYMBOL_GPL(__dfl_fpga_cdev_config_port_vf); + static int __init dfl_fpga_init(void) { int ret; diff --git a/drivers/fpga/dfl.h b/drivers/fpga/dfl.h index d700ee9..061ccd4 100644 --- a/drivers/fpga/dfl.h +++ b/drivers/fpga/dfl.h @@ -421,5 +421,6 @@ struct platform_device * int dfl_fpga_cdev_config_port(struct dfl_fpga_cdev *cdev, int port_id, bool release); +void __dfl_fpga_cdev_config_port_vf(struct dfl_fpga_cdev *cdev, bool is_vf); #endif /* __FPGA_DFL_H */