[PATCH 1/2] pci: Error disabling SR-IOV if in VFs assigned

[PATCH 1/2] pci: Error disabling SR-IOV if in VFs assigned

[Keith Busch]

Every sriov capable driver has to check if any guest is using a virtual
function prior to disabling, so let's make it common code.

Signed-off-by: Keith Busch <keith.busch@intel.com>
---
 drivers/pci/pci-sysfs.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/drivers/pci/pci-sysfs.c b/drivers/pci/pci-sysfs.c
index 342b691..5011fa9 100644
--- a/drivers/pci/pci-sysfs.c
+++ b/drivers/pci/pci-sysfs.c
@@ -487,6 +487,11 @@ static ssize_t sriov_numvfs_store(struct device *dev,
 
 	if (num_vfs == 0) {
 		/* disable VFs */
+		if (pci_vfs_assigned(pdev)) {
+			dev_warn(&pdev->dev,
+				"Cannot disable SR-IOV VFs while assigned\n");
+			return -EPERM;
+		}
 		ret = pdev->driver->sriov_configure(pdev, 0);
 		if (ret < 0)
 			return ret;
-- 
2.7.2

[PATCH 2/2] nvme/pci: Enable SR-IOV capabilities

[Keith Busch]

Registers a standard boiler-plate SR-IOV configure callback for NVMe.

Signed-off-by: Keith Busch <keith.busch@intel.com>
---
 drivers/nvme/host/pci.c | 13 +++++++++++++
 1 file changed, 13 insertions(+)

diff --git a/drivers/nvme/host/pci.c b/drivers/nvme/host/pci.c
index fbe8cc2..8379b9a 100644
--- a/drivers/nvme/host/pci.c
+++ b/drivers/nvme/host/pci.c
@@ -2033,6 +2033,18 @@ static void nvme_remove(struct pci_dev *pdev)
 	nvme_put_ctrl(&dev->ctrl);
 }
 
+static int nvme_sriov_configure(struct pci_dev *pdev, int numvfs)
+{
+	int ret = 0;
+
+	if (numvfs == 0)
+		pci_disable_sriov(pdev);
+	else
+		ret = pci_enable_sriov(pdev, numvfs);
+
+	return ret ? ret : numvfs;
+}
+
 #ifdef CONFIG_PM_SLEEP
 static int nvme_suspend(struct device *dev)
 {
@@ -2127,6 +2139,7 @@ static struct pci_driver nvme_driver = {
 	.driver		= {
 		.pm	= &nvme_dev_pm_ops,
 	},
+	.sriov_configure = nvme_sriov_configure,
 	.err_handler	= &nvme_err_handler,
 };
 
-- 
2.7.2

Re: [PATCH 2/2] nvme/pci: Enable SR-IOV capabilities

[Christoph Hellwig]

Doesn't look fine (because the API is a mess), but the best we can do
for now:

Reviewed-by: Christoph Hellwig <hch@lst.de>

Re: [PATCH 2/2] nvme/pci: Enable SR-IOV capabilities

[Bjorn Helgaas]

On Tue, May 17, 2016 at 03:39:59PM -0600, Keith Busch wrote:
> Registers a standard boiler-plate SR-IOV configure callback for NVMe.
> 
> Signed-off-by: Keith Busch <keith.busch@intel.com>
> ---
>  drivers/nvme/host/pci.c | 13 +++++++++++++
>  1 file changed, 13 insertions(+)
> 
> diff --git a/drivers/nvme/host/pci.c b/drivers/nvme/host/pci.c
> index fbe8cc2..8379b9a 100644
> --- a/drivers/nvme/host/pci.c
> +++ b/drivers/nvme/host/pci.c
> @@ -2033,6 +2033,18 @@ static void nvme_remove(struct pci_dev *pdev)
>  	nvme_put_ctrl(&dev->ctrl);
>  }
>  
> +static int nvme_sriov_configure(struct pci_dev *pdev, int numvfs)
> +{
> +	int ret = 0;
> +
> +	if (numvfs == 0)
> +		pci_disable_sriov(pdev);
> +	else
> +		ret = pci_enable_sriov(pdev, numvfs);
> +
> +	return ret ? ret : numvfs;
> +}

I do not subscribe to the belief that every function should have a
single exit.  In this case, I think it makes the function much harder
to understand than this:

  if (numvfs == 0)
    pci_disable_sriov(pdev);
    return 0;
  }

  return pci_enable_sriov(pdev, numvfs);

>  #ifdef CONFIG_PM_SLEEP
>  static int nvme_suspend(struct device *dev)
>  {
> @@ -2127,6 +2139,7 @@ static struct pci_driver nvme_driver = {
>  	.driver		= {
>  		.pm	= &nvme_dev_pm_ops,
>  	},
> +	.sriov_configure = nvme_sriov_configure,
>  	.err_handler	= &nvme_err_handler,
>  };
>  
> -- 
> 2.7.2
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-pci" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 2/2] nvme/pci: Enable SR-IOV capabilities

[Christoph Hellwig]

On Mon, May 23, 2016 at 12:06:52PM -0500, Bjorn Helgaas wrote:
> > +static int nvme_sriov_configure(struct pci_dev *pdev, int numvfs)
> > +{
> > +	int ret = 0;
> > +
> > +	if (numvfs == 0)
> > +		pci_disable_sriov(pdev);
> > +	else
> > +		ret = pci_enable_sriov(pdev, numvfs);
> > +
> > +	return ret ? ret : numvfs;
> > +}
> 
> I do not subscribe to the belief that every function should have a
> single exit.  In this case, I think it makes the function much harder
> to understand than this:
> 
>   if (numvfs == 0)
>     pci_disable_sriov(pdev);
>     return 0;
>   }
> 
>   return pci_enable_sriov(pdev, numvfs);

I'd tend to agree.  But then again the real issue here is that we should
have two methods instead, and useful defaults.  I.e. if the SR-IOV API
was properly designed NVMe wouldn't even need this callout at all,
and drivers that need it should have one method each for enable /
disable.

Re: [PATCH 2/2] nvme/pci: Enable SR-IOV capabilities

[Keith Busch]

On Mon, May 23, 2016 at 12:06:52PM -0500, Bjorn Helgaas wrote:
> I do not subscribe to the belief that every function should have a
> single exit.  In this case, I think it makes the function much harder
> to understand than this:

No problem with multiple exists. 
 
>   if (numvfs == 0)
>     pci_disable_sriov(pdev);
>     return 0;
>   }
> 
>   return pci_enable_sriov(pdev, numvfs);

Slight change to the above: pci_enable_sriov returns 0 on success,
but a driver's .sriov_configure is supposed to return the number of
VF's successfully configured, so need to return 'numvfs' on success.

Re: [PATCH 2/2] nvme/pci: Enable SR-IOV capabilities

[Bjorn Helgaas]

On Mon, May 23, 2016 at 01:21:44PM -0400, Keith Busch wrote:
> On Mon, May 23, 2016 at 12:06:52PM -0500, Bjorn Helgaas wrote:
> > I do not subscribe to the belief that every function should have a
> > single exit.  In this case, I think it makes the function much harder
> > to understand than this:
> 
> No problem with multiple exists. 
>  
> >   if (numvfs == 0)
> >     pci_disable_sriov(pdev);
> >     return 0;
> >   }
> > 
> >   return pci_enable_sriov(pdev, numvfs);
> 
> Slight change to the above: pci_enable_sriov returns 0 on success,
> but a driver's .sriov_configure is supposed to return the number of
> VF's successfully configured, so need to return 'numvfs' on success.

Oops, sorry, excuse me while I wipe the egg off my face :)

Re: [PATCH 1/2] pci: Error disabling SR-IOV if in VFs assigned

[Alex Williamson]

On Tue, 17 May 2016 15:39:58 -0600
Keith Busch <keith.busch@intel.com> wrote:

> Every sriov capable driver has to check if any guest is using a virtual
> function prior to disabling, so let's make it common code.

This is not true, the PCI_DEV_FLAGS_ASSIGNED flag is inherently racy,
so checking it is really only a courtesy for broken drivers that
still make use of it.  I don't object to adding it here, though I
wish the entire interface was deprecated, but it's only a minimal amount
of insurance as a VF might get assigned immediately following your added
test or might not participate in the assigned device flagging at all.  I
believe the better way to handle this is with proper host drivers for
assigned devices that manage the driver .remove callback properly while
devices are in use.  The only reason to handle assigned devices
specially in this case is when they don't have proper host drivers
managing them, which is a problem that we've fixed.  Thanks,

Alex

> Signed-off-by: Keith Busch <keith.busch@intel.com>
> ---
>  drivers/pci/pci-sysfs.c | 5 +++++
>  1 file changed, 5 insertions(+)
> 
> diff --git a/drivers/pci/pci-sysfs.c b/drivers/pci/pci-sysfs.c
> index 342b691..5011fa9 100644
> --- a/drivers/pci/pci-sysfs.c
> +++ b/drivers/pci/pci-sysfs.c
> @@ -487,6 +487,11 @@ static ssize_t sriov_numvfs_store(struct device *dev,
>  
>  	if (num_vfs == 0) {
>  		/* disable VFs */
> +		if (pci_vfs_assigned(pdev)) {
> +			dev_warn(&pdev->dev,
> +				"Cannot disable SR-IOV VFs while assigned\n");
> +			return -EPERM;
> +		}
>  		ret = pdev->driver->sriov_configure(pdev, 0);
>  		if (ret < 0)
>  			return ret;

Re: [PATCH 1/2] pci: Error disabling SR-IOV if in VFs assigned

[Christoph Hellwig]

On Tue, May 17, 2016 at 04:08:32PM -0600, Alex Williamson wrote:
> On Tue, 17 May 2016 15:39:58 -0600
> Keith Busch <keith.busch@intel.com> wrote:
> 
> > Every sriov capable driver has to check if any guest is using a virtual
> > function prior to disabling, so let's make it common code.
> 
> This is not true, the PCI_DEV_FLAGS_ASSIGNED flag is inherently racy,
> so checking it is really only a courtesy for broken drivers that
> still make use of it.  I don't object to adding it here, though I
> wish the entire interface was deprecated, but it's only a minimal amount
> of insurance as a VF might get assigned immediately following your added
> test or might not participate in the assigned device flagging at all.

Si should we just kill it? As far as I can tell it's only used in these
kinds of boilerplate checks.

> I
> believe the better way to handle this is with proper host drivers for
> assigned devices that manage the driver .remove callback properly while
> devices are in use.  The only reason to handle assigned devices
> specially in this case is when they don't have proper host drivers
> managing them, which is a problem that we've fixed.  Thanks,

We always use pci-stub now, don't we?

Re: [PATCH 1/2] pci: Error disabling SR-IOV if in VFs assigned

[Alex Williamson]

On Mon, 23 May 2016 03:55:32 -0700
Christoph Hellwig <hch@infradead.org> wrote:

> On Tue, May 17, 2016 at 04:08:32PM -0600, Alex Williamson wrote:
> > On Tue, 17 May 2016 15:39:58 -0600
> > Keith Busch <keith.busch@intel.com> wrote:
> >   
> > > Every sriov capable driver has to check if any guest is using a virtual
> > > function prior to disabling, so let's make it common code.  
> > 
> > This is not true, the PCI_DEV_FLAGS_ASSIGNED flag is inherently racy,
> > so checking it is really only a courtesy for broken drivers that
> > still make use of it.  I don't object to adding it here, though I
> > wish the entire interface was deprecated, but it's only a minimal amount
> > of insurance as a VF might get assigned immediately following your added
> > test or might not participate in the assigned device flagging at all.  
> 
> Si should we just kill it? As far as I can tell it's only used in these
> kinds of boilerplate checks.

Long term, yes, but perhaps KVM legacy PCI device assignment needs to
be not only deprecated, but removed first.

> > I
> > believe the better way to handle this is with proper host drivers for
> > assigned devices that manage the driver .remove callback properly while
> > devices are in use.  The only reason to handle assigned devices
> > specially in this case is when they don't have proper host drivers
> > managing them, which is a problem that we've fixed.  Thanks,  
> 
> We always use pci-stub now, don't we?

pci-stub's only purpose is to prevent other drivers from binding to a
device, such as while it's in used by legacy KVM device assignment.
pci-stub has no visibility whether a device is in use or not, and will
happily unbind the device at any point in time.  Thus legacy KVM device
assignment with pci-stub makes use of this horrible flag in a vain
attempt to prevent devices from disappearing, littering every possible
remove path with these sorts of checks when really the driver holding
the assigned device should block or maybe allow an error return from
the .remove callback.  Xen pci-back also sets this flag, but I would
hope there's a sensible solution available there and they just adopted
use of this flag without really questioning that it works or makes
sense. Thanks,

Alex

Re: [PATCH 1/2] pci: Error disabling SR-IOV if in VFs assigned

[Christoph Hellwig]

On Mon, May 23, 2016 at 09:07:11AM -0600, Alex Williamson wrote:
> On Mon, 23 May 2016 03:55:32 -0700
> Christoph Hellwig <hch@infradead.org> wrote:
> 
> > On Tue, May 17, 2016 at 04:08:32PM -0600, Alex Williamson wrote:
> > > On Tue, 17 May 2016 15:39:58 -0600
> > > Keith Busch <keith.busch@intel.com> wrote:
> > >   
> > > > Every sriov capable driver has to check if any guest is using a virtual
> > > > function prior to disabling, so let's make it common code.  
> > > 
> > > This is not true, the PCI_DEV_FLAGS_ASSIGNED flag is inherently racy,
> > > so checking it is really only a courtesy for broken drivers that
> > > still make use of it.  I don't object to adding it here, though I
> > > wish the entire interface was deprecated, but it's only a minimal amount
> > > of insurance as a VF might get assigned immediately following your added
> > > test or might not participate in the assigned device flagging at all.  
> > 
> > Si should we just kill it? As far as I can tell it's only used in these
> > kinds of boilerplate checks.
> 
> Long term, yes, but perhaps KVM legacy PCI device assignment needs to
> be not only deprecated, but removed first.

Oh well.  In that case I'd like to at least move it to the PCI core
so that drivers don't have to care about it.

Re: [PATCH 1/2] pci: Error disabling SR-IOV if in VFs assigned

[Bjorn Helgaas]

On Tue, May 17, 2016 at 03:39:58PM -0600, Keith Busch wrote:
> Every sriov capable driver has to check if any guest is using a virtual
> function prior to disabling, so let's make it common code.
> 
> Signed-off-by: Keith Busch <keith.busch@intel.com>

If I understand the discussion correctly, this is still racy but
nobody objects to adding this until we have a better, non-racy
solution.

However, you added this in common code and took advantage of it in
nvme.  Good so far.  But we have about a dozen other drivers that call
pci_vfs_assigned().  I assume some of those places could be changed so
they take advantage of this check in the core instead?

Can we do that at the same time?  If we add good new stuff and only
use it one place, there's not as much overall goodness as there would
be if we updated everybody to do it similarly.

> ---
>  drivers/pci/pci-sysfs.c | 5 +++++
>  1 file changed, 5 insertions(+)
> 
> diff --git a/drivers/pci/pci-sysfs.c b/drivers/pci/pci-sysfs.c
> index 342b691..5011fa9 100644
> --- a/drivers/pci/pci-sysfs.c
> +++ b/drivers/pci/pci-sysfs.c
> @@ -487,6 +487,11 @@ static ssize_t sriov_numvfs_store(struct device *dev,
>  
>  	if (num_vfs == 0) {
>  		/* disable VFs */
> +		if (pci_vfs_assigned(pdev)) {
> +			dev_warn(&pdev->dev,
> +				"Cannot disable SR-IOV VFs while assigned\n");
> +			return -EPERM;
> +		}
>  		ret = pdev->driver->sriov_configure(pdev, 0);
>  		if (ret < 0)
>  			return ret;
> -- 
> 2.7.2
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-pci" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 1/2] pci: Error disabling SR-IOV if in VFs assigned

[Keith Busch]

On Mon, Jun 13, 2016 at 04:14:11PM -0500, Bjorn Helgaas wrote:
> On Tue, May 17, 2016 at 03:39:58PM -0600, Keith Busch wrote:
> > Every sriov capable driver has to check if any guest is using a virtual
> > function prior to disabling, so let's make it common code.
> > 
> > Signed-off-by: Keith Busch <keith.busch@intel.com>
> 
> If I understand the discussion correctly, this is still racy but
> nobody objects to adding this until we have a better, non-racy
> solution.

My understanding as well: there's a potential race, but no different
than what exists with today.
 
> However, you added this in common code and took advantage of it in
> nvme.  Good so far.  But we have about a dozen other drivers that call
> pci_vfs_assigned().  I assume some of those places could be changed so
> they take advantage of this check in the core instead?

> Can we do that at the same time?  If we add good new stuff and only
> use it one place, there's not as much overall goodness as there would
> be if we updated everybody to do it similarly.

Sounds good, I'll send a series taking advantage of this for all the
other PF drivers duplicating this check in their sriov_configure.

Re: [PATCH 1/2] pci: Error disabling SR-IOV if in VFs assigned

[Keith Busch]

On Mon, Jun 13, 2016 at 05:28:10PM -0400, Keith Busch wrote:
> On Mon, Jun 13, 2016 at 04:14:11PM -0500, Bjorn Helgaas wrote:
> > Can we do that at the same time?  If we add good new stuff and only
> > use it one place, there's not as much overall goodness as there would
> > be if we updated everybody to do it similarly.
> 
> Sounds good, I'll send a series taking advantage of this for all the
> other PF drivers duplicating this check in their sriov_configure.

Heh, I thought "no big deal", thinking all use was similar to
NVMe's. However, most network drivers have multiple paths to sriov
configuration, or have other requirements to changing the live
count! There's only two drivers I find that can immediately use the
simplification safely. I'll send those updates to just those ones.

Re: [PATCH 1/2] pci: Error disabling SR-IOV if in VFs assigned

[Bjorn Helgaas]

On Mon, Jun 13, 2016 at 05:57:37PM -0400, Keith Busch wrote:
> On Mon, Jun 13, 2016 at 05:28:10PM -0400, Keith Busch wrote:
> > On Mon, Jun 13, 2016 at 04:14:11PM -0500, Bjorn Helgaas wrote:
> > > Can we do that at the same time?  If we add good new stuff and only
> > > use it one place, there's not as much overall goodness as there would
> > > be if we updated everybody to do it similarly.
> > 
> > Sounds good, I'll send a series taking advantage of this for all the
> > other PF drivers duplicating this check in their sriov_configure.
> 
> Heh, I thought "no big deal", thinking all use was similar to
> NVMe's. However, most network drivers have multiple paths to sriov
> configuration, or have other requirements to changing the live
> count! There's only two drivers I find that can immediately use the
> simplification safely. I'll send those updates to just those ones.

My take on that is, "it's a pretty trival check; just put it directly
in nvme and don't bother doing anything in the PCI core."  Maybe
someday we'll come up with a way to make things more common.  It
doesn't really seem worth it to add something that only helps three
drivers.

Bjorn

Re: [PATCH 1/2] pci: Error disabling SR-IOV if in VFs assigned

[Keith Busch]

On Mon, Jun 13, 2016 at 05:26:30PM -0500, Bjorn Helgaas wrote:
> My take on that is, "it's a pretty trival check; just put it directly
> in nvme and don't bother doing anything in the PCI core."  Maybe
> someday we'll come up with a way to make things more common.  It
> doesn't really seem worth it to add something that only helps three
> drivers.

Fair enough. Will withdraw the check in pci and resend nvme.

Re: [PATCH 1/2] pci: Error disabling SR-IOV if in VFs assigned

[Christoph Hellwig]

On Mon, Jun 13, 2016 at 06:35:52PM -0400, Keith Busch wrote:
> On Mon, Jun 13, 2016 at 05:26:30PM -0500, Bjorn Helgaas wrote:
> > My take on that is, "it's a pretty trival check; just put it directly
> > in nvme and don't bother doing anything in the PCI core."  Maybe
> > someday we'll come up with a way to make things more common.  It
> > doesn't really seem worth it to add something that only helps three
> > drivers.
> 
> Fair enough. Will withdraw the check in pci and resend nvme.

Sorry for leading you down this path..

Re: [PATCH 1/2] pci: Error disabling SR-IOV if in VFs assigned

[Bjorn Helgaas]

On Wed, Jun 15, 2016 at 5:26 AM, Christoph Hellwig <hch@infradead.org> wrote:
> On Mon, Jun 13, 2016 at 06:35:52PM -0400, Keith Busch wrote:
>> On Mon, Jun 13, 2016 at 05:26:30PM -0500, Bjorn Helgaas wrote:
>> > My take on that is, "it's a pretty trival check; just put it directly
>> > in nvme and don't bother doing anything in the PCI core."  Maybe
>> > someday we'll come up with a way to make things more common.  It
>> > doesn't really seem worth it to add something that only helps three
>> > drivers.
>>
>> Fair enough. Will withdraw the check in pci and resend nvme.
>
> Sorry for leading you down this path..

No problem, I think it would be awesome if we could unify these
drivers somehow, and unification always involves lots of dead-ends :)