linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [pci PATCH v5 0/4] Series short description
@ 2018-03-12 17:20 Alexander Duyck
  2018-03-12 17:21 ` [pci PATCH v5 1/4] pci: Add pci_sriov_configure_simple for PFs that don't manage VF resources Alexander Duyck
                   ` (3 more replies)
  0 siblings, 4 replies; 18+ messages in thread
From: Alexander Duyck @ 2018-03-12 17:20 UTC (permalink / raw)
  To: bhelgaas, alexander.h.duyck, linux-pci
  Cc: virtio-dev, kvm, netdev, dan.daly, linux-kernel, linux-nvme,
	keith.busch, netanel, mheyne, liang-min.wang, mark.d.rustad,
	dwmw2, hch, dwmw

This series is meant to add support for SR-IOV on devices when the VFs are
not managed by the kernel. Examples of recent patches attempting to do this
include:
virto - https://patchwork.kernel.org/patch/10241225/
pci-stub - https://patchwork.kernel.org/patch/10109935/
vfio - https://patchwork.kernel.org/patch/10103353/
uio - https://patchwork.kernel.org/patch/9974031/

Since this is quickly blowing up into a multi-driver problem it is probably
best to implement this solution as generically as possible.

This series is an attempt to do that. What we do with this patch set is
provide a generic framework to enable SR-IOV in the case that the PF driver
doesn't support managing the VFs itself.

I based my patch set originally on the patch by Mark Rustad but there isn't
much left after going through and cleaning out the bits that were no longer
needed, and after incorporating the feedback from David Miller. At this point
the only items to be fully reused was his patch description which is now
present in patch 3 of the set.

This solution is limited in scope to just adding support for devices that
provide no functionality for SR-IOV other than allocating the VFs by
calling pci_enable_sriov. Previous sets had included patches for VFIO, but
for now I am dropping that as the scope of that work is larger then I
think I can take on at this time.

v2: Reduced scope back to just virtio_pci and vfio-pci
    Broke into 3 patch set from single patch
    Changed autoprobe behavior to always set when num_vfs is set non-zero
v3: Updated Documentation to clarify when sriov_unmanaged_autoprobe is used
    Wrapped vfio_pci_sriov_configure to fix build errors w/o SR-IOV in kernel
v4: Dropped vfio-pci patch
    Added ena and nvme to drivers now using pci_sriov_configure_unmanaged
    Dropped pci_disable_sriov call in virtio_pci to be consistent with ena
v5: Dropped sriov_unmanaged_autoprobe and pci_sriov_conifgure_unmanaged
    Added new patch that enables pci_sriov_configure_simple
    Updated drivers to use pci_sriov_configure_simple

Cc: Mark Rustad <mark.d.rustad@intel.com>
Cc: Maximilian Heyne <mheyne@amazon.de>
Cc: Liang-Min Wang <liang-min.wang@intel.com>
Cc: David Woodhouse <dwmw@amazon.co.uk>

---

Alexander Duyck (4):
      pci: Add pci_sriov_configure_simple for PFs that don't manage VF resources
      virtio_pci: Add support for unmanaged SR-IOV on virtio_pci devices
      ena: Migrate over to unmanaged SR-IOV support
      nvme: Migrate over to unmanaged SR-IOV support


 drivers/net/ethernet/amazon/ena/ena_netdev.c |   30 ++----------------------
 drivers/nvme/host/pci.c                      |   22 ++----------------
 drivers/pci/iov.c                            |   32 ++++++++++++++++++++++++++
 drivers/virtio/virtio_pci_common.c           |    3 ++
 include/linux/pci.h                          |    1 +
 5 files changed, 42 insertions(+), 46 deletions(-)

--

^ permalink raw reply	[flat|nested] 18+ messages in thread

* [pci PATCH v5 1/4] pci: Add pci_sriov_configure_simple for PFs that don't manage VF resources
  2018-03-12 17:20 [pci PATCH v5 0/4] Series short description Alexander Duyck
@ 2018-03-12 17:21 ` Alexander Duyck
  2018-03-12 17:40   ` Keith Busch
  2018-03-12 17:23 ` [pci PATCH v5 2/4] virtio_pci: Add support for unmanaged SR-IOV on virtio_pci devices Alexander Duyck
                   ` (2 subsequent siblings)
  3 siblings, 1 reply; 18+ messages in thread
From: Alexander Duyck @ 2018-03-12 17:21 UTC (permalink / raw)
  To: bhelgaas, alexander.h.duyck, linux-pci
  Cc: virtio-dev, kvm, netdev, dan.daly, linux-kernel, linux-nvme,
	keith.busch, netanel, mheyne, liang-min.wang, mark.d.rustad,
	dwmw2, hch, dwmw

From: Alexander Duyck <alexander.h.duyck@intel.com>

This patch adds a common configuration function called
pci_sriov_configure_simple that will allow for managing VFs on devices
where the PF is not capable of managing VF resources.

Signed-off-by: Alexander Duyck <alexander.h.duyck@intel.com>
---

v5: New patch replacing pci_sriov_configure_unmanaged with
      pci_sriov_configure_simple
    Dropped bits related to autoprobe changes

 drivers/pci/iov.c   |   32 ++++++++++++++++++++++++++++++++
 include/linux/pci.h |    1 +
 2 files changed, 33 insertions(+)

diff --git a/drivers/pci/iov.c b/drivers/pci/iov.c
index 677924ae0350..bd7021491fdb 100644
--- a/drivers/pci/iov.c
+++ b/drivers/pci/iov.c
@@ -807,3 +807,35 @@ int pci_sriov_get_totalvfs(struct pci_dev *dev)
 	return dev->sriov->total_VFs;
 }
 EXPORT_SYMBOL_GPL(pci_sriov_get_totalvfs);
+
+/**
+ * pci_sriov_configure_simple - helper to configure unmanaged SR-IOV
+ * @dev: the PCI device
+ * @nr_virtfn: number of virtual functions to enable, 0 to disable
+ *
+ * Used to provide generic enable/disable SR-IOV option for devices
+ * that do not manage the VFs generated by their driver
+ */
+int pci_sriov_configure_simple(struct pci_dev *dev, int nr_virtfn)
+{
+	int err = -EINVAL;
+
+	might_sleep();
+
+	if (!dev->is_physfn)
+		return -ENODEV;
+
+	if (pci_vfs_assigned(dev)) {
+		pci_warn(dev,
+			 "Cannot modify SR-IOV while VFs are assigned\n");
+		err = -EPERM;
+	} else if (!nr_virtfn) {
+		sriov_disable(dev);
+		err = 0;
+	} else if (!dev->sriov->num_VFs) {
+		err = sriov_enable(dev, nr_virtfn);
+	}
+
+	return err ? err : nr_virtfn;
+}
+EXPORT_SYMBOL_GPL(pci_sriov_configure_simple);
diff --git a/include/linux/pci.h b/include/linux/pci.h
index 024a1beda008..9cab9d0d51dc 100644
--- a/include/linux/pci.h
+++ b/include/linux/pci.h
@@ -1953,6 +1953,7 @@ static inline void pci_mmcfg_late_init(void) { }
 int pci_vfs_assigned(struct pci_dev *dev);
 int pci_sriov_set_totalvfs(struct pci_dev *dev, u16 numvfs);
 int pci_sriov_get_totalvfs(struct pci_dev *dev);
+int pci_sriov_configure_simple(struct pci_dev *dev, int nr_virtfn);
 resource_size_t pci_iov_resource_size(struct pci_dev *dev, int resno);
 void pci_vf_drivers_autoprobe(struct pci_dev *dev, bool probe);
 #else

^ permalink raw reply related	[flat|nested] 18+ messages in thread

* [pci PATCH v5 2/4] virtio_pci: Add support for unmanaged SR-IOV on virtio_pci devices
  2018-03-12 17:20 [pci PATCH v5 0/4] Series short description Alexander Duyck
  2018-03-12 17:21 ` [pci PATCH v5 1/4] pci: Add pci_sriov_configure_simple for PFs that don't manage VF resources Alexander Duyck
@ 2018-03-12 17:23 ` Alexander Duyck
  2018-03-12 17:23 ` [pci PATCH v5 3/4] ena: Migrate over to unmanaged SR-IOV support Alexander Duyck
  2018-03-12 17:23 ` [pci PATCH v5 4/4] nvme: " Alexander Duyck
  3 siblings, 0 replies; 18+ messages in thread
From: Alexander Duyck @ 2018-03-12 17:23 UTC (permalink / raw)
  To: bhelgaas, alexander.h.duyck, linux-pci
  Cc: virtio-dev, kvm, netdev, dan.daly, linux-kernel, linux-nvme,
	keith.busch, netanel, mheyne, liang-min.wang, mark.d.rustad,
	dwmw2, hch, dwmw

From: Alexander Duyck <alexander.h.duyck@intel.com>

Hardware-realized virtio_pci devices can implement SR-IOV, so this
patch enables its use. The device in question is an upcoming Intel
NIC that implements both a virtio_net PF and virtio_net VFs. These
are hardware realizations of what has been up to now been a software
interface.

The device in question has the following 4-part PCI IDs:

PF: vendor: 1af4 device: 1041 subvendor: 8086 subdevice: 15fe
VF: vendor: 1af4 device: 1041 subvendor: 8086 subdevice: 05fe

The patch currently needs no check for device ID, because the callback
will never be made for devices that do not assert the capability or
when run on a platform incapable of SR-IOV.

One reason for this patch is because the hardware requires the
vendor ID of a VF to be the same as the vendor ID of the PF that
created it. So it seemed logical to simply have a fully-functioning
virtio_net PF create the VFs. This patch makes that possible.

Signed-off-by: Mark Rustad <mark.d.rustad@intel.com>
Signed-off-by: Alexander Duyck <alexander.h.duyck@intel.com>
---

v4: Dropped call to pci_disable_sriov in virtio_pci_remove function
v5: Replaced call to pci_sriov_configure_unmanaged with
        pci_sriov_configure_simple

 drivers/virtio/virtio_pci_common.c |    3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/virtio/virtio_pci_common.c b/drivers/virtio/virtio_pci_common.c
index 48d4d1cf1cb6..41938d36b0cb 100644
--- a/drivers/virtio/virtio_pci_common.c
+++ b/drivers/virtio/virtio_pci_common.c
@@ -596,6 +596,9 @@ static void virtio_pci_remove(struct pci_dev *pci_dev)
 #ifdef CONFIG_PM_SLEEP
 	.driver.pm	= &virtio_pci_pm_ops,
 #endif
+#ifdef CONFIG_PCI_IOV
+	.sriov_configure = pci_sriov_configure_simple,
+#endif
 };
 
 module_pci_driver(virtio_pci_driver);

^ permalink raw reply related	[flat|nested] 18+ messages in thread

* [pci PATCH v5 3/4] ena: Migrate over to unmanaged SR-IOV support
  2018-03-12 17:20 [pci PATCH v5 0/4] Series short description Alexander Duyck
  2018-03-12 17:21 ` [pci PATCH v5 1/4] pci: Add pci_sriov_configure_simple for PFs that don't manage VF resources Alexander Duyck
  2018-03-12 17:23 ` [pci PATCH v5 2/4] virtio_pci: Add support for unmanaged SR-IOV on virtio_pci devices Alexander Duyck
@ 2018-03-12 17:23 ` Alexander Duyck
  2018-03-13  8:12   ` David Woodhouse
  2018-03-12 17:23 ` [pci PATCH v5 4/4] nvme: " Alexander Duyck
  3 siblings, 1 reply; 18+ messages in thread
From: Alexander Duyck @ 2018-03-12 17:23 UTC (permalink / raw)
  To: bhelgaas, alexander.h.duyck, linux-pci
  Cc: virtio-dev, kvm, netdev, dan.daly, linux-kernel, linux-nvme,
	keith.busch, netanel, mheyne, liang-min.wang, mark.d.rustad,
	dwmw2, hch, dwmw

From: Alexander Duyck <alexander.h.duyck@intel.com>

Instead of implementing our own version of a SR-IOV configuration stub in
the ena driver we can just reuse the existing
pci_sriov_configure_simple function.

Signed-off-by: Alexander Duyck <alexander.h.duyck@intel.com>
---

v5: Replaced call to pci_sriov_configure_unmanaged with
        pci_sriov_configure_simple

 drivers/net/ethernet/amazon/ena/ena_netdev.c |   30 +++-----------------------
 1 file changed, 3 insertions(+), 27 deletions(-)

diff --git a/drivers/net/ethernet/amazon/ena/ena_netdev.c b/drivers/net/ethernet/amazon/ena/ena_netdev.c
index 6975150d144e..868069363bdd 100644
--- a/drivers/net/ethernet/amazon/ena/ena_netdev.c
+++ b/drivers/net/ethernet/amazon/ena/ena_netdev.c
@@ -3385,32 +3385,6 @@ static int ena_probe(struct pci_dev *pdev, const struct pci_device_id *ent)
 }
 
 /*****************************************************************************/
-static int ena_sriov_configure(struct pci_dev *dev, int numvfs)
-{
-	int rc;
-
-	if (numvfs > 0) {
-		rc = pci_enable_sriov(dev, numvfs);
-		if (rc != 0) {
-			dev_err(&dev->dev,
-				"pci_enable_sriov failed to enable: %d vfs with the error: %d\n",
-				numvfs, rc);
-			return rc;
-		}
-
-		return numvfs;
-	}
-
-	if (numvfs == 0) {
-		pci_disable_sriov(dev);
-		return 0;
-	}
-
-	return -EINVAL;
-}
-
-/*****************************************************************************/
-/*****************************************************************************/
 
 /* ena_remove - Device Removal Routine
  * @pdev: PCI device information struct
@@ -3525,7 +3499,9 @@ static int ena_resume(struct pci_dev *pdev)
 	.suspend    = ena_suspend,
 	.resume     = ena_resume,
 #endif
-	.sriov_configure = ena_sriov_configure,
+#ifdef CONFIG_PCI_IOV
+	.sriov_configure = pci_sriov_configure_simple,
+#endif
 };
 
 static int __init ena_init(void)

^ permalink raw reply related	[flat|nested] 18+ messages in thread

* [pci PATCH v5 4/4] nvme: Migrate over to unmanaged SR-IOV support
  2018-03-12 17:20 [pci PATCH v5 0/4] Series short description Alexander Duyck
                   ` (2 preceding siblings ...)
  2018-03-12 17:23 ` [pci PATCH v5 3/4] ena: Migrate over to unmanaged SR-IOV support Alexander Duyck
@ 2018-03-12 17:23 ` Alexander Duyck
  3 siblings, 0 replies; 18+ messages in thread
From: Alexander Duyck @ 2018-03-12 17:23 UTC (permalink / raw)
  To: bhelgaas, alexander.h.duyck, linux-pci
  Cc: virtio-dev, kvm, netdev, dan.daly, linux-kernel, linux-nvme,
	keith.busch, netanel, mheyne, liang-min.wang, mark.d.rustad,
	dwmw2, hch, dwmw

From: Alexander Duyck <alexander.h.duyck@intel.com>

Instead of implementing our own version of a SR-IOV configuration stub in
the nvme driver we can just reuse the existing
pci_sriov_configure_simple function.

Signed-off-by: Alexander Duyck <alexander.h.duyck@intel.com>
---

v5: Replaced call to pci_sriov_configure_unmanaged with
        pci_sriov_configure_simple

 drivers/nvme/host/pci.c |   22 +++-------------------
 1 file changed, 3 insertions(+), 19 deletions(-)

diff --git a/drivers/nvme/host/pci.c b/drivers/nvme/host/pci.c
index 5933a5c732e8..3b582f9f8cac 100644
--- a/drivers/nvme/host/pci.c
+++ b/drivers/nvme/host/pci.c
@@ -2580,24 +2580,6 @@ static void nvme_remove(struct pci_dev *pdev)
 	nvme_put_ctrl(&dev->ctrl);
 }
 
-static int nvme_pci_sriov_configure(struct pci_dev *pdev, int numvfs)
-{
-	int ret = 0;
-
-	if (numvfs == 0) {
-		if (pci_vfs_assigned(pdev)) {
-			dev_warn(&pdev->dev,
-				"Cannot disable SR-IOV VFs while assigned\n");
-			return -EPERM;
-		}
-		pci_disable_sriov(pdev);
-		return 0;
-	}
-
-	ret = pci_enable_sriov(pdev, numvfs);
-	return ret ? ret : numvfs;
-}
-
 #ifdef CONFIG_PM_SLEEP
 static int nvme_suspend(struct device *dev)
 {
@@ -2716,7 +2698,9 @@ static void nvme_error_resume(struct pci_dev *pdev)
 	.driver		= {
 		.pm	= &nvme_dev_pm_ops,
 	},
-	.sriov_configure = nvme_pci_sriov_configure,
+#ifdef CONFIG_PCI_IOV
+	.sriov_configure = pci_sriov_configure_simple,
+#endif
 	.err_handler	= &nvme_err_handler,
 };
 

^ permalink raw reply related	[flat|nested] 18+ messages in thread

* Re: [pci PATCH v5 1/4] pci: Add pci_sriov_configure_simple for PFs that don't manage VF resources
  2018-03-12 17:21 ` [pci PATCH v5 1/4] pci: Add pci_sriov_configure_simple for PFs that don't manage VF resources Alexander Duyck
@ 2018-03-12 17:40   ` Keith Busch
  2018-03-12 18:09     ` Alexander Duyck
  0 siblings, 1 reply; 18+ messages in thread
From: Keith Busch @ 2018-03-12 17:40 UTC (permalink / raw)
  To: Alexander Duyck
  Cc: bhelgaas, alexander.h.duyck, linux-pci, virtio-dev, kvm, netdev,
	dan.daly, linux-kernel, linux-nvme, netanel, mheyne,
	liang-min.wang, mark.d.rustad, dwmw2, hch, dwmw

On Mon, Mar 12, 2018 at 10:21:29AM -0700, Alexander Duyck wrote:
> diff --git a/include/linux/pci.h b/include/linux/pci.h
> index 024a1beda008..9cab9d0d51dc 100644
> --- a/include/linux/pci.h
> +++ b/include/linux/pci.h
> @@ -1953,6 +1953,7 @@ static inline void pci_mmcfg_late_init(void) { }
>  int pci_vfs_assigned(struct pci_dev *dev);
>  int pci_sriov_set_totalvfs(struct pci_dev *dev, u16 numvfs);
>  int pci_sriov_get_totalvfs(struct pci_dev *dev);
> +int pci_sriov_configure_simple(struct pci_dev *dev, int nr_virtfn);
>  resource_size_t pci_iov_resource_size(struct pci_dev *dev, int resno);
>  void pci_vf_drivers_autoprobe(struct pci_dev *dev, bool probe);
>  #else

I recommend stubbing 'pci_sriov_configure_simple' or defining it to
NULL in the '#else' section here so you don't need to repeat the "#ifdef
CONFIG_PCI_IOV" in each driver wishing to use this function. Otherwise
looks fine to me.

^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [pci PATCH v5 1/4] pci: Add pci_sriov_configure_simple for PFs that don't manage VF resources
  2018-03-12 17:40   ` Keith Busch
@ 2018-03-12 18:09     ` Alexander Duyck
  2018-03-12 18:23       ` Keith Busch
  0 siblings, 1 reply; 18+ messages in thread
From: Alexander Duyck @ 2018-03-12 18:09 UTC (permalink / raw)
  To: Keith Busch
  Cc: Bjorn Helgaas, Duyck, Alexander H, linux-pci, virtio-dev, kvm,
	Netdev, Daly, Dan, LKML, linux-nvme, netanel, Maximilian Heyne,
	Wang, Liang-min, Rustad, Mark D, David Woodhouse,
	Christoph Hellwig, dwmw

On Mon, Mar 12, 2018 at 10:40 AM, Keith Busch <keith.busch@intel.com> wrote:
> On Mon, Mar 12, 2018 at 10:21:29AM -0700, Alexander Duyck wrote:
>> diff --git a/include/linux/pci.h b/include/linux/pci.h
>> index 024a1beda008..9cab9d0d51dc 100644
>> --- a/include/linux/pci.h
>> +++ b/include/linux/pci.h
>> @@ -1953,6 +1953,7 @@ static inline void pci_mmcfg_late_init(void) { }
>>  int pci_vfs_assigned(struct pci_dev *dev);
>>  int pci_sriov_set_totalvfs(struct pci_dev *dev, u16 numvfs);
>>  int pci_sriov_get_totalvfs(struct pci_dev *dev);
>> +int pci_sriov_configure_simple(struct pci_dev *dev, int nr_virtfn);
>>  resource_size_t pci_iov_resource_size(struct pci_dev *dev, int resno);
>>  void pci_vf_drivers_autoprobe(struct pci_dev *dev, bool probe);
>>  #else
>
> I recommend stubbing 'pci_sriov_configure_simple' or defining it to
> NULL in the '#else' section here so you don't need to repeat the "#ifdef
> CONFIG_PCI_IOV" in each driver wishing to use this function. Otherwise
> looks fine to me.

My concern with defining it as NULL is that somebody may end up
calling it in the future directly and that may end up causing issues.
One thought I have been debating is moving it to a different file. I
am just not sure where the best place to put something like this would
be. I could move this function to drivers/pci/pci.c if everyone is
okay with it and then I could just strip the contents out by wrapping
them in a #ifdef instead.

^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [pci PATCH v5 1/4] pci: Add pci_sriov_configure_simple for PFs that don't manage VF resources
  2018-03-12 18:09     ` Alexander Duyck
@ 2018-03-12 18:23       ` Keith Busch
  2018-03-12 20:17         ` Alexander Duyck
  0 siblings, 1 reply; 18+ messages in thread
From: Keith Busch @ 2018-03-12 18:23 UTC (permalink / raw)
  To: Alexander Duyck
  Cc: Bjorn Helgaas, Duyck, Alexander H, linux-pci, virtio-dev, kvm,
	Netdev, Daly, Dan, LKML, linux-nvme, netanel, Maximilian Heyne,
	Wang, Liang-min, Rustad, Mark D, David Woodhouse,
	Christoph Hellwig, dwmw

On Mon, Mar 12, 2018 at 11:09:34AM -0700, Alexander Duyck wrote:
> On Mon, Mar 12, 2018 at 10:40 AM, Keith Busch <keith.busch@intel.com> wrote:
> > On Mon, Mar 12, 2018 at 10:21:29AM -0700, Alexander Duyck wrote:
> >> diff --git a/include/linux/pci.h b/include/linux/pci.h
> >> index 024a1beda008..9cab9d0d51dc 100644
> >> --- a/include/linux/pci.h
> >> +++ b/include/linux/pci.h
> >> @@ -1953,6 +1953,7 @@ static inline void pci_mmcfg_late_init(void) { }
> >>  int pci_vfs_assigned(struct pci_dev *dev);
> >>  int pci_sriov_set_totalvfs(struct pci_dev *dev, u16 numvfs);
> >>  int pci_sriov_get_totalvfs(struct pci_dev *dev);
> >> +int pci_sriov_configure_simple(struct pci_dev *dev, int nr_virtfn);
> >>  resource_size_t pci_iov_resource_size(struct pci_dev *dev, int resno);
> >>  void pci_vf_drivers_autoprobe(struct pci_dev *dev, bool probe);
> >>  #else
> >
> > I recommend stubbing 'pci_sriov_configure_simple' or defining it to
> > NULL in the '#else' section here so you don't need to repeat the "#ifdef
> > CONFIG_PCI_IOV" in each driver wishing to use this function. Otherwise
> > looks fine to me.
> 
> My concern with defining it as NULL is that somebody may end up
> calling it in the future directly and that may end up causing issues.
> One thought I have been debating is moving it to a different file. I
> am just not sure where the best place to put something like this would
> be. I could move this function to drivers/pci/pci.c if everyone is
> okay with it and then I could just strip the contents out by wrapping
> them in a #ifdef instead.

Okay, instead of NULL, a stub implementation in the header file may
suffice when CONFIG_PCI_IOV is not defined:

static inline int pci_sriov_configure_simple(struct pci_dev *dev, int nr_virtfn)
{
	return -ENOSYS;
}

See pci_iov_virtfn_bus, pci_iov_virtfn_devfn, pci_iov_add_virtfn, or
pci_enable_sriov for other examples.

^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [pci PATCH v5 1/4] pci: Add pci_sriov_configure_simple for PFs that don't manage VF resources
  2018-03-12 18:23       ` Keith Busch
@ 2018-03-12 20:17         ` Alexander Duyck
  2018-03-13  7:44           ` Christoph Hellwig
  0 siblings, 1 reply; 18+ messages in thread
From: Alexander Duyck @ 2018-03-12 20:17 UTC (permalink / raw)
  To: Keith Busch
  Cc: Bjorn Helgaas, Duyck, Alexander H, linux-pci, virtio-dev, kvm,
	Netdev, Daly, Dan, LKML, linux-nvme, netanel, Maximilian Heyne,
	Wang, Liang-min, Rustad, Mark D, David Woodhouse,
	Christoph Hellwig, dwmw

On Mon, Mar 12, 2018 at 11:23 AM, Keith Busch <keith.busch@intel.com> wrote:
> On Mon, Mar 12, 2018 at 11:09:34AM -0700, Alexander Duyck wrote:
>> On Mon, Mar 12, 2018 at 10:40 AM, Keith Busch <keith.busch@intel.com> wrote:
>> > On Mon, Mar 12, 2018 at 10:21:29AM -0700, Alexander Duyck wrote:
>> >> diff --git a/include/linux/pci.h b/include/linux/pci.h
>> >> index 024a1beda008..9cab9d0d51dc 100644
>> >> --- a/include/linux/pci.h
>> >> +++ b/include/linux/pci.h
>> >> @@ -1953,6 +1953,7 @@ static inline void pci_mmcfg_late_init(void) { }
>> >>  int pci_vfs_assigned(struct pci_dev *dev);
>> >>  int pci_sriov_set_totalvfs(struct pci_dev *dev, u16 numvfs);
>> >>  int pci_sriov_get_totalvfs(struct pci_dev *dev);
>> >> +int pci_sriov_configure_simple(struct pci_dev *dev, int nr_virtfn);
>> >>  resource_size_t pci_iov_resource_size(struct pci_dev *dev, int resno);
>> >>  void pci_vf_drivers_autoprobe(struct pci_dev *dev, bool probe);
>> >>  #else
>> >
>> > I recommend stubbing 'pci_sriov_configure_simple' or defining it to
>> > NULL in the '#else' section here so you don't need to repeat the "#ifdef
>> > CONFIG_PCI_IOV" in each driver wishing to use this function. Otherwise
>> > looks fine to me.
>>
>> My concern with defining it as NULL is that somebody may end up
>> calling it in the future directly and that may end up causing issues.
>> One thought I have been debating is moving it to a different file. I
>> am just not sure where the best place to put something like this would
>> be. I could move this function to drivers/pci/pci.c if everyone is
>> okay with it and then I could just strip the contents out by wrapping
>> them in a #ifdef instead.
>
> Okay, instead of NULL, a stub implementation in the header file may
> suffice when CONFIG_PCI_IOV is not defined:
>
> static inline int pci_sriov_configure_simple(struct pci_dev *dev, int nr_virtfn)
> {
>         return -ENOSYS;
> }
>
> See pci_iov_virtfn_bus, pci_iov_virtfn_devfn, pci_iov_add_virtfn, or
> pci_enable_sriov for other examples.

No, I am aware of those. The problem is they aren't accessed as
function pointers. As such converting them to static inline functions
is easy. As I am sure you are aware an "inline" function doesn't
normally generate a function pointer.

Actually my original idea has been complicated further by the fact
that I realized my code is accessing functions that are static in the
iov.c file. I'll need to think about how to come up with a better
solution for this.

^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [pci PATCH v5 1/4] pci: Add pci_sriov_configure_simple for PFs that don't manage VF resources
  2018-03-12 20:17         ` Alexander Duyck
@ 2018-03-13  7:44           ` Christoph Hellwig
  0 siblings, 0 replies; 18+ messages in thread
From: Christoph Hellwig @ 2018-03-13  7:44 UTC (permalink / raw)
  To: Alexander Duyck
  Cc: Keith Busch, Bjorn Helgaas, Duyck, Alexander H, linux-pci,
	virtio-dev, kvm, Netdev, Daly, Dan, LKML, linux-nvme, netanel,
	Maximilian Heyne, Wang, Liang-min, Rustad, Mark D,
	David Woodhouse, Christoph Hellwig, dwmw

On Mon, Mar 12, 2018 at 01:17:00PM -0700, Alexander Duyck wrote:
> No, I am aware of those. The problem is they aren't accessed as
> function pointers. As such converting them to static inline functions
> is easy. As I am sure you are aware an "inline" function doesn't
> normally generate a function pointer.

I think Keith's original idea of defining them to NULL is right.  That
takes care of all the current trivial assign to struct cases.

If someone wants to call these functions they'll still need the ifdef
around the call as those won't otherwise compile, but they probably
want the ifdef around the whole caller anyway.

^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [pci PATCH v5 3/4] ena: Migrate over to unmanaged SR-IOV support
  2018-03-12 17:23 ` [pci PATCH v5 3/4] ena: Migrate over to unmanaged SR-IOV support Alexander Duyck
@ 2018-03-13  8:12   ` David Woodhouse
  2018-03-13  8:16     ` Christoph Hellwig
  2018-03-13 14:51     ` Alexander Duyck
  0 siblings, 2 replies; 18+ messages in thread
From: David Woodhouse @ 2018-03-13  8:12 UTC (permalink / raw)
  To: Alexander Duyck, bhelgaas, alexander.h.duyck, linux-pci
  Cc: virtio-dev, kvm, netdev, dan.daly, linux-kernel, linux-nvme,
	keith.busch, netanel, mheyne, liang-min.wang, mark.d.rustad, hch

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

On Mon, 2018-03-12 at 10:23 -0700, Alexander Duyck wrote:
> 
> -       .sriov_configure = ena_sriov_configure,
> +#ifdef CONFIG_PCI_IOV
> +       .sriov_configure = pci_sriov_configure_simple,
> +#endif
>  };

I'd like to see that ifdef go away, as discussed. I agree that just
#define pci_sriov_configure_simple NULL
should suffice. As Christoph points out, it's not going to compile if
people try to just invoke it directly.

I'd also *really* like to see a way to enable this for PFs which don't
have (and don't need) a driver. We seem to have lost that along the
way.

[-- Attachment #2: smime.p7s --]
[-- Type: application/x-pkcs7-signature, Size: 5213 bytes --]

^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [pci PATCH v5 3/4] ena: Migrate over to unmanaged SR-IOV support
  2018-03-13  8:12   ` David Woodhouse
@ 2018-03-13  8:16     ` Christoph Hellwig
  2018-03-13  8:45       ` David Woodhouse
  2018-03-13 14:51     ` Alexander Duyck
  1 sibling, 1 reply; 18+ messages in thread
From: Christoph Hellwig @ 2018-03-13  8:16 UTC (permalink / raw)
  To: David Woodhouse
  Cc: Alexander Duyck, bhelgaas, alexander.h.duyck, linux-pci,
	virtio-dev, kvm, netdev, dan.daly, linux-kernel, linux-nvme,
	keith.busch, netanel, mheyne, liang-min.wang, mark.d.rustad, hch

On Tue, Mar 13, 2018 at 08:12:52AM +0000, David Woodhouse wrote:
> I'd also *really* like to see a way to enable this for PFs which don't
> have (and don't need) a driver. We seem to have lost that along the
> way.

We've been forth and back on that.  I agree that not having any driver
just seems dangerous.  If your PF really does nothing we should just
have a trivial pf_stub driver that does nothing but wiring up
pci_sriov_configure_simple.  We can then add PCI IDs to it either
statically, or using the dynamic ids mechanism.

^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [pci PATCH v5 3/4] ena: Migrate over to unmanaged SR-IOV support
  2018-03-13  8:16     ` Christoph Hellwig
@ 2018-03-13  8:45       ` David Woodhouse
  2018-03-13  8:54         ` Christoph Hellwig
  0 siblings, 1 reply; 18+ messages in thread
From: David Woodhouse @ 2018-03-13  8:45 UTC (permalink / raw)
  To: Christoph Hellwig
  Cc: Alexander Duyck, bhelgaas, alexander.h.duyck, linux-pci,
	virtio-dev, kvm, netdev, dan.daly, linux-kernel, linux-nvme,
	keith.busch, netanel, mheyne, liang-min.wang, mark.d.rustad

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



On Tue, 2018-03-13 at 09:16 +0100, Christoph Hellwig wrote:
> On Tue, Mar 13, 2018 at 08:12:52AM +0000, David Woodhouse wrote:
> > 
> > I'd also *really* like to see a way to enable this for PFs which don't
> > have (and don't need) a driver. We seem to have lost that along the
> > way.
> We've been forth and back on that.  I agree that not having any driver
> just seems dangerous.  If your PF really does nothing we should just
> have a trivial pf_stub driver that does nothing but wiring up
> pci_sriov_configure_simple.  We can then add PCI IDs to it either
> statically, or using the dynamic ids mechanism.

Or just add it to the existing pci-stub. What's the point in having a
new driver? 

[-- Attachment #2: smime.p7s --]
[-- Type: application/x-pkcs7-signature, Size: 5213 bytes --]

^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [pci PATCH v5 3/4] ena: Migrate over to unmanaged SR-IOV support
  2018-03-13  8:45       ` David Woodhouse
@ 2018-03-13  8:54         ` Christoph Hellwig
  2018-03-13  9:14           ` David Woodhouse
  0 siblings, 1 reply; 18+ messages in thread
From: Christoph Hellwig @ 2018-03-13  8:54 UTC (permalink / raw)
  To: David Woodhouse
  Cc: Christoph Hellwig, Alexander Duyck, bhelgaas, alexander.h.duyck,
	linux-pci, virtio-dev, kvm, netdev, dan.daly, linux-kernel,
	linux-nvme, keith.busch, netanel, mheyne, liang-min.wang,
	mark.d.rustad

On Tue, Mar 13, 2018 at 08:45:19AM +0000, David Woodhouse wrote:
> 
> 
> On Tue, 2018-03-13 at 09:16 +0100, Christoph Hellwig wrote:
> > On Tue, Mar 13, 2018 at 08:12:52AM +0000, David Woodhouse wrote:
> > > 
> > > I'd also *really* like to see a way to enable this for PFs which don't
> > > have (and don't need) a driver. We seem to have lost that along the
> > > way.
> > We've been forth and back on that.  I agree that not having any driver
> > just seems dangerous.  If your PF really does nothing we should just
> > have a trivial pf_stub driver that does nothing but wiring up
> > pci_sriov_configure_simple.  We can then add PCI IDs to it either
> > statically, or using the dynamic ids mechanism.
> 
> Or just add it to the existing pci-stub. What's the point in having a
> new driver? 

Because binding to pci-stub means that you'd now enable the simple
SR-IOV for any device bound to PCI stub.  Which often might be the wrong
thing.

^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [pci PATCH v5 3/4] ena: Migrate over to unmanaged SR-IOV support
  2018-03-13  8:54         ` Christoph Hellwig
@ 2018-03-13  9:14           ` David Woodhouse
  0 siblings, 0 replies; 18+ messages in thread
From: David Woodhouse @ 2018-03-13  9:14 UTC (permalink / raw)
  To: Christoph Hellwig
  Cc: Alexander Duyck, bhelgaas, alexander.h.duyck, linux-pci,
	virtio-dev, kvm, netdev, dan.daly, linux-kernel, linux-nvme,
	keith.busch, netanel, mheyne, liang-min.wang, mark.d.rustad

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



On Tue, 2018-03-13 at 09:54 +0100, Christoph Hellwig wrote:
> On Tue, Mar 13, 2018 at 08:45:19AM +0000, David Woodhouse wrote:
> Because binding to pci-stub means that you'd now enable the simple
> SR-IOV for any device bound to PCI stub.  Which often might be the wrong
> thing.

No, *using* it would be the wrong thing (bad root; no biscuit).

Except when the PF doesn't have SR-IOV capability anyway, in which case
who cares.

Or when the PF does have SR-IOV capability and root has ensure that
she's doing the right thing.

I understand the arguments about disallowing root from doing bad
things. Not that I agree with them. But simply changing to a
*different* driver seems pointless.


[-- Attachment #2: smime.p7s --]
[-- Type: application/x-pkcs7-signature, Size: 5213 bytes --]

^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [pci PATCH v5 3/4] ena: Migrate over to unmanaged SR-IOV support
  2018-03-13  8:12   ` David Woodhouse
  2018-03-13  8:16     ` Christoph Hellwig
@ 2018-03-13 14:51     ` Alexander Duyck
  2018-03-13 15:04       ` David Woodhouse
  1 sibling, 1 reply; 18+ messages in thread
From: Alexander Duyck @ 2018-03-13 14:51 UTC (permalink / raw)
  To: David Woodhouse, Don Dutile
  Cc: Bjorn Helgaas, Duyck, Alexander H, linux-pci, virtio-dev, kvm,
	Netdev, Daly, Dan, LKML, linux-nvme, Keith Busch, netanel,
	Maximilian Heyne, Wang, Liang-min, Rustad, Mark D,
	Christoph Hellwig

On Tue, Mar 13, 2018 at 1:12 AM, David Woodhouse <dwmw2@infradead.org> wrote:
> On Mon, 2018-03-12 at 10:23 -0700, Alexander Duyck wrote:
>>
>> -       .sriov_configure = ena_sriov_configure,
>> +#ifdef CONFIG_PCI_IOV
>> +       .sriov_configure = pci_sriov_configure_simple,
>> +#endif
>>  };
>
> I'd like to see that ifdef go away, as discussed. I agree that just
> #define pci_sriov_configure_simple NULL
> should suffice. As Christoph points out, it's not going to compile if
> people try to just invoke it directly.
>
> I'd also *really* like to see a way to enable this for PFs which don't
> have (and don't need) a driver. We seem to have lost that along the
> way.

Actually the suggestion I had from Don Dutile was that we should be
looking at creating a pci-stub like driver specifically for those type
of devices, but without the ability to arbitrarily assign devices.
Basically we have to white-list it in one device at a time for those
kind of things.

If you have the device ID of the thing you wanted to have work with
pci-stub before I could look at putting together a quick driver and
adding it to this set.

Thanks.

- Alex

^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [pci PATCH v5 3/4] ena: Migrate over to unmanaged SR-IOV support
  2018-03-13 14:51     ` Alexander Duyck
@ 2018-03-13 15:04       ` David Woodhouse
  2018-03-13 16:17         ` Don Dutile
  0 siblings, 1 reply; 18+ messages in thread
From: David Woodhouse @ 2018-03-13 15:04 UTC (permalink / raw)
  To: Alexander Duyck, Don Dutile
  Cc: Bjorn Helgaas, Duyck, Alexander H, linux-pci, virtio-dev, kvm,
	Netdev, Daly, Dan, LKML, linux-nvme, Keith Busch, netanel,
	Maximilian Heyne, Wang, Liang-min, Rustad, Mark D,
	Christoph Hellwig

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

On Tue, 2018-03-13 at 07:51 -0700, Alexander Duyck wrote:

> Actually the suggestion I had from Don Dutile was that we should be
> looking at creating a pci-stub like driver specifically for those type
> of devices, but without the ability to arbitrarily assign devices.
> Basically we have to white-list it in one device at a time for those
> kind of things.

It's still not clear what the point of that would be.

> If you have the device ID of the thing you wanted to have work with
> pci-stub before I could look at putting together a quick driver and
> adding it to this set.

1d0f:0053 would be an example.

[-- Attachment #2: smime.p7s --]
[-- Type: application/x-pkcs7-signature, Size: 5213 bytes --]

^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [pci PATCH v5 3/4] ena: Migrate over to unmanaged SR-IOV support
  2018-03-13 15:04       ` David Woodhouse
@ 2018-03-13 16:17         ` Don Dutile
  0 siblings, 0 replies; 18+ messages in thread
From: Don Dutile @ 2018-03-13 16:17 UTC (permalink / raw)
  To: David Woodhouse, Alexander Duyck
  Cc: Bjorn Helgaas, Duyck, Alexander H, linux-pci, virtio-dev, kvm,
	Netdev, Daly, Dan, LKML, linux-nvme, Keith Busch, netanel,
	Maximilian Heyne, Wang, Liang-min, Rustad, Mark D,
	Christoph Hellwig

On 03/13/2018 11:04 AM, David Woodhouse wrote:
> On Tue, 2018-03-13 at 07:51 -0700, Alexander Duyck wrote:
> 
>> Actually the suggestion I had from Don Dutile was that we should be
>> looking at creating a pci-stub like driver specifically for those type
>> of devices, but without the ability to arbitrarily assign devices.
>> Basically we have to white-list it in one device at a time for those
>> kind of things.
> 
> It's still not clear what the point of that would be.
> 
A PF-stub to do device-assignment(-like) ops preserves the current security model,
and allows one to add pci-quirks at a device-level as well -- when the usual ACS
structs aren't properly added for a device, which happens quite frequently -- which
retains that common workaround as well.
Yet-another-method for VF assignment w/o even a simple PF driver stub
created multiple failure cases/configs when we were hashing the multiple options
a few weeks ago.
It's just simpler to implement a PF stub w/VF enablement ... b/c it's simple...

>> If you have the device ID of the thing you wanted to have work with
>> pci-stub before I could look at putting together a quick driver and
>> adding it to this set.
> 
> 1d0f:0053 would be an example.
> 

^ permalink raw reply	[flat|nested] 18+ messages in thread

end of thread, other threads:[~2018-03-13 16:17 UTC | newest]

Thread overview: 18+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-03-12 17:20 [pci PATCH v5 0/4] Series short description Alexander Duyck
2018-03-12 17:21 ` [pci PATCH v5 1/4] pci: Add pci_sriov_configure_simple for PFs that don't manage VF resources Alexander Duyck
2018-03-12 17:40   ` Keith Busch
2018-03-12 18:09     ` Alexander Duyck
2018-03-12 18:23       ` Keith Busch
2018-03-12 20:17         ` Alexander Duyck
2018-03-13  7:44           ` Christoph Hellwig
2018-03-12 17:23 ` [pci PATCH v5 2/4] virtio_pci: Add support for unmanaged SR-IOV on virtio_pci devices Alexander Duyck
2018-03-12 17:23 ` [pci PATCH v5 3/4] ena: Migrate over to unmanaged SR-IOV support Alexander Duyck
2018-03-13  8:12   ` David Woodhouse
2018-03-13  8:16     ` Christoph Hellwig
2018-03-13  8:45       ` David Woodhouse
2018-03-13  8:54         ` Christoph Hellwig
2018-03-13  9:14           ` David Woodhouse
2018-03-13 14:51     ` Alexander Duyck
2018-03-13 15:04       ` David Woodhouse
2018-03-13 16:17         ` Don Dutile
2018-03-12 17:23 ` [pci PATCH v5 4/4] nvme: " Alexander Duyck

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).