netdev.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH 0/4] move pci_assivned_vfs() check (while disabling VFs) to pci sub-system
@ 2014-11-10 11:53 Sathya Perla
  2014-11-10 11:53 ` [PATCH 1/4] pci: move pci_assivned_vfs() check while disabling VFs " Sathya Perla
                   ` (5 more replies)
  0 siblings, 6 replies; 12+ messages in thread
From: Sathya Perla @ 2014-11-10 11:53 UTC (permalink / raw)
  To: linux-pci, netdev; +Cc: ariel.elior, linux.nics, shahed.shaikh, ddutile

A user must not be allowed to disable VFs while they are already assigned to
a guest. This check is being made in each individual driver that implements
the sriov_configure PCI method.
This patch-set fixes this code duplication by moving this check from
drivers to the sriov_nuvfs_store() routine just before invoking
sriov_configure() when num_vfs is equal to 0.

Vasundhara Volam (4):
  pci: move pci_assivned_vfs() check while disabling VFs to pci
    sub-system
  bnx2x: remove pci_assigned_vfs() check while disabling VFs
  i40e: remove pci_assigned_vfs() check while disabling VFs
  qlcnic: remove pci_assigned_vfs() check while disabling VFs

 drivers/net/ethernet/broadcom/bnx2x/bnx2x_sriov.c  |    2 +-
 drivers/net/ethernet/intel/i40e/i40e_virtchnl_pf.c |    7 +------
 .../net/ethernet/qlogic/qlcnic/qlcnic_sriov_pf.c   |   10 ----------
 drivers/pci/pci-sysfs.c                            |    5 +++++
 4 files changed, 7 insertions(+), 17 deletions(-)

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

* [PATCH 1/4] pci: move pci_assivned_vfs() check while disabling VFs to pci sub-system
  2014-11-10 11:53 [PATCH 0/4] move pci_assivned_vfs() check (while disabling VFs) to pci sub-system Sathya Perla
@ 2014-11-10 11:53 ` Sathya Perla
  2014-11-10 11:53 ` [PATCH 2/4] bnx2x: remove pci_assigned_vfs() check while disabling VFs Sathya Perla
                   ` (4 subsequent siblings)
  5 siblings, 0 replies; 12+ messages in thread
From: Sathya Perla @ 2014-11-10 11:53 UTC (permalink / raw)
  To: linux-pci, netdev; +Cc: ariel.elior, linux.nics, shahed.shaikh, ddutile

From: Vasundhara Volam <vasundhara.volam@emulex.com>

A user must not be allowed to disable VFs while they are already assigned to
a guest. This check is being made in each individual driver that implements
the sriov_configure PCI method.
This patch fixes this code duplication by moving this check to the
sriov_nuvfs_store() routine just before invoking sriov_configure() when
num_vfs is equal to 0.

Signed-off-by: Vasundhara Volam <vasundhara.volam@emulex.com>
Signed-off-by: Sathya Perla <sathya.perla@emulex.com>
---
 drivers/pci/pci-sysfs.c |    5 +++++
 1 files changed, 5 insertions(+), 0 deletions(-)

diff --git a/drivers/pci/pci-sysfs.c b/drivers/pci/pci-sysfs.c
index 2c6643f..6e65b47 100644
--- a/drivers/pci/pci-sysfs.c
+++ b/drivers/pci/pci-sysfs.c
@@ -477,6 +477,11 @@ static ssize_t sriov_numvfs_store(struct device *dev,
 	}
 
 	if (num_vfs == 0) {
+		if (pci_vfs_assigned(pdev)) {
+			dev_warn(&pdev->dev, "Cannot disable VFs while they are assigned\n");
+			return -EBUSY;
+		}
+
 		/* disable VFs */
 		ret = pdev->driver->sriov_configure(pdev, 0);
 		if (ret < 0)
-- 
1.7.1

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

* [PATCH 2/4] bnx2x: remove pci_assigned_vfs() check while disabling VFs
  2014-11-10 11:53 [PATCH 0/4] move pci_assivned_vfs() check (while disabling VFs) to pci sub-system Sathya Perla
  2014-11-10 11:53 ` [PATCH 1/4] pci: move pci_assivned_vfs() check while disabling VFs " Sathya Perla
@ 2014-11-10 11:53 ` Sathya Perla
  2014-11-10 11:53 ` [PATCH 3/4] i40e: " Sathya Perla
                   ` (3 subsequent siblings)
  5 siblings, 0 replies; 12+ messages in thread
From: Sathya Perla @ 2014-11-10 11:53 UTC (permalink / raw)
  To: linux-pci, netdev; +Cc: ariel.elior, linux.nics, shahed.shaikh, ddutile

From: Vasundhara Volam <vasundhara.volam@emulex.com>

The pci_assigned_vfs() check (while disabling VFs) is being moved to the
pci-sysfs.c file and will be done before invoking sriov_configure().

Signed-off-by: Vasundhara Volam <vasundhara.volam@emulex.com>
Signed-off-by: Sathya Perla <sathya.perla@emulex.com>
---
 drivers/net/ethernet/broadcom/bnx2x/bnx2x_sriov.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/drivers/net/ethernet/broadcom/bnx2x/bnx2x_sriov.c b/drivers/net/ethernet/broadcom/bnx2x/bnx2x_sriov.c
index c88b20a..e90a10b 100644
--- a/drivers/net/ethernet/broadcom/bnx2x/bnx2x_sriov.c
+++ b/drivers/net/ethernet/broadcom/bnx2x/bnx2x_sriov.c
@@ -2481,7 +2481,7 @@ int bnx2x_sriov_configure(struct pci_dev *dev, int num_vfs_param)
 	bp->requested_nr_virtfn = num_vfs_param;
 	if (num_vfs_param == 0) {
 		bnx2x_set_pf_tx_switching(bp, false);
-		bnx2x_disable_sriov(bp);
+		pci_disable_sriov(bp->pdev);
 		return 0;
 	} else {
 		return bnx2x_enable_sriov(bp);
-- 
1.7.1

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

* [PATCH 3/4] i40e: remove pci_assigned_vfs() check while disabling VFs
  2014-11-10 11:53 [PATCH 0/4] move pci_assivned_vfs() check (while disabling VFs) to pci sub-system Sathya Perla
  2014-11-10 11:53 ` [PATCH 1/4] pci: move pci_assivned_vfs() check while disabling VFs " Sathya Perla
  2014-11-10 11:53 ` [PATCH 2/4] bnx2x: remove pci_assigned_vfs() check while disabling VFs Sathya Perla
@ 2014-11-10 11:53 ` Sathya Perla
  2014-11-11 13:52   ` Jeff Kirsher
  2014-11-10 11:53 ` [PATCH 4/4] qlcnic: " Sathya Perla
                   ` (2 subsequent siblings)
  5 siblings, 1 reply; 12+ messages in thread
From: Sathya Perla @ 2014-11-10 11:53 UTC (permalink / raw)
  To: linux-pci, netdev; +Cc: ariel.elior, linux.nics, shahed.shaikh, ddutile

From: Vasundhara Volam <vasundhara.volam@emulex.com>

The pci_assigned_vfs() check (while disabling VFs) is being moved to the
pci-sysfs.c file and will be done before invoking sriov_configure().

Signed-off-by: Vasundhara Volam <vasundhara.volam@emulex.com>
Signed-off-by: Sathya Perla <sathya.perla@emulex.com>
---
 drivers/net/ethernet/intel/i40e/i40e_virtchnl_pf.c |    7 +------
 1 files changed, 1 insertions(+), 6 deletions(-)

diff --git a/drivers/net/ethernet/intel/i40e/i40e_virtchnl_pf.c b/drivers/net/ethernet/intel/i40e/i40e_virtchnl_pf.c
index fff3c27..0028a9a 100644
--- a/drivers/net/ethernet/intel/i40e/i40e_virtchnl_pf.c
+++ b/drivers/net/ethernet/intel/i40e/i40e_virtchnl_pf.c
@@ -951,12 +951,7 @@ int i40e_pci_sriov_configure(struct pci_dev *pdev, int num_vfs)
 	if (num_vfs)
 		return i40e_pci_sriov_enable(pdev, num_vfs);
 
-	if (!pci_vfs_assigned(pf->pdev)) {
-		i40e_free_vfs(pf);
-	} else {
-		dev_warn(&pdev->dev, "Unable to free VFs because some are assigned to VMs.\n");
-		return -EINVAL;
-	}
+	i40e_free_vfs(pf);
 	return 0;
 }
 
-- 
1.7.1

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

* [PATCH 4/4] qlcnic: remove pci_assigned_vfs() check while disabling VFs
  2014-11-10 11:53 [PATCH 0/4] move pci_assivned_vfs() check (while disabling VFs) to pci sub-system Sathya Perla
                   ` (2 preceding siblings ...)
  2014-11-10 11:53 ` [PATCH 3/4] i40e: " Sathya Perla
@ 2014-11-10 11:53 ` Sathya Perla
  2014-11-11 19:09 ` [PATCH 0/4] move pci_assivned_vfs() check (while disabling VFs) to pci sub-system Don Dutile
  2014-11-20 22:21 ` Bjorn Helgaas
  5 siblings, 0 replies; 12+ messages in thread
From: Sathya Perla @ 2014-11-10 11:53 UTC (permalink / raw)
  To: linux-pci, netdev; +Cc: ariel.elior, linux.nics, shahed.shaikh, ddutile

From: Vasundhara Volam <vasundhara.volam@emulex.com>

The pci_assigned_vfs() check (while disabling VFs) is being moved to the
pci-sysfs.c file and will be done before invoking sriov_configure().

Signed-off-by: Vasundhara Volam <vasundhara.volam@emulex.com>
Signed-off-by: Sathya Perla <sathya.perla@emulex.com>
---
 .../net/ethernet/qlogic/qlcnic/qlcnic_sriov_pf.c   |   10 ----------
 1 files changed, 0 insertions(+), 10 deletions(-)

diff --git a/drivers/net/ethernet/qlogic/qlcnic/qlcnic_sriov_pf.c b/drivers/net/ethernet/qlogic/qlcnic/qlcnic_sriov_pf.c
index a29538b..9802914 100644
--- a/drivers/net/ethernet/qlogic/qlcnic/qlcnic_sriov_pf.c
+++ b/drivers/net/ethernet/qlogic/qlcnic/qlcnic_sriov_pf.c
@@ -465,16 +465,6 @@ static int qlcnic_pci_sriov_disable(struct qlcnic_adapter *adapter)
 {
 	struct net_device *netdev = adapter->netdev;
 
-	if (pci_vfs_assigned(adapter->pdev)) {
-		netdev_err(adapter->netdev,
-			   "SR-IOV VFs belonging to port %d are assigned to VMs. SR-IOV can not be disabled on this port\n",
-			   adapter->portnum);
-		netdev_info(adapter->netdev,
-			    "Please detach SR-IOV VFs belonging to port %d from VMs, and then try to disable SR-IOV on this port\n",
-			    adapter->portnum);
-		return -EPERM;
-	}
-
 	qlcnic_sriov_pf_disable(adapter);
 
 	rtnl_lock();
-- 
1.7.1

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

* Re: [PATCH 3/4] i40e: remove pci_assigned_vfs() check while disabling VFs
  2014-11-10 11:53 ` [PATCH 3/4] i40e: " Sathya Perla
@ 2014-11-11 13:52   ` Jeff Kirsher
  0 siblings, 0 replies; 12+ messages in thread
From: Jeff Kirsher @ 2014-11-11 13:52 UTC (permalink / raw)
  To: Sathya Perla
  Cc: linux-pci@vger.kernel.org, netdev, ariel.elior, linux.nics,
	shahed.shaikh, ddutile

On Mon, Nov 10, 2014 at 3:53 AM, Sathya Perla <sathya.perla@emulex.com> wrote:
> From: Vasundhara Volam <vasundhara.volam@emulex.com>
>
> The pci_assigned_vfs() check (while disabling VFs) is being moved to the
> pci-sysfs.c file and will be done before invoking sriov_configure().
>
> Signed-off-by: Vasundhara Volam <vasundhara.volam@emulex.com>
> Signed-off-by: Sathya Perla <sathya.perla@emulex.com>
> ---
>  drivers/net/ethernet/intel/i40e/i40e_virtchnl_pf.c |    7 +------
>  1 files changed, 1 insertions(+), 6 deletions(-)

Thanks I will add your patch to my queue.

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

* Re: [PATCH 0/4] move pci_assivned_vfs() check (while disabling VFs) to pci sub-system
  2014-11-10 11:53 [PATCH 0/4] move pci_assivned_vfs() check (while disabling VFs) to pci sub-system Sathya Perla
                   ` (3 preceding siblings ...)
  2014-11-10 11:53 ` [PATCH 4/4] qlcnic: " Sathya Perla
@ 2014-11-11 19:09 ` Don Dutile
  2014-11-11 20:39   ` Alex Williamson
  2014-11-20 22:21 ` Bjorn Helgaas
  5 siblings, 1 reply; 12+ messages in thread
From: Don Dutile @ 2014-11-11 19:09 UTC (permalink / raw)
  To: Sathya Perla, linux-pci, netdev; +Cc: ariel.elior, linux.nics, shahed.shaikh

On 11/10/2014 06:53 AM, Sathya Perla wrote:
> A user must not be allowed to disable VFs while they are already assigned to
> a guest. This check is being made in each individual driver that implements
> the sriov_configure PCI method.
> This patch-set fixes this code duplication by moving this check from
> drivers to the sriov_nuvfs_store() routine just before invoking
> sriov_configure() when num_vfs is equal to 0.
>
> Vasundhara Volam (4):
>    pci: move pci_assivned_vfs() check while disabling VFs to pci
>      sub-system
>    bnx2x: remove pci_assigned_vfs() check while disabling VFs
>    i40e: remove pci_assigned_vfs() check while disabling VFs
>    qlcnic: remove pci_assigned_vfs() check while disabling VFs
>
>   drivers/net/ethernet/broadcom/bnx2x/bnx2x_sriov.c  |    2 +-
>   drivers/net/ethernet/intel/i40e/i40e_virtchnl_pf.c |    7 +------
>   .../net/ethernet/qlogic/qlcnic/qlcnic_sriov_pf.c   |   10 ----------
>   drivers/pci/pci-sysfs.c                            |    5 +++++
>   4 files changed, 7 insertions(+), 17 deletions(-)
>
I have had a side conversation with Alex Williamson, VFIO author.

VFIO is the upstream method that device-assignment is managed/handled on kvm now.
It does not set the PCI_DEV_FLAGS_ASSIGNED pci dev-flags, and thus,
this check will not work when VFIO is used.

This patch set will only work for the former, kvm-managed, device-assignment method,
which is currently being deprecated in qemu as well.

So, yes, it works for kvm managed device-assignment, but not the
newer, VFIO-based device-assignment.

Note, also, that the pci_assigned_vfs() check in the drivers will
always return 0 when VFIO is used for device assignment, so keeping
these checks in the drivers doesn't do what they imply either.

So, taking in the patch solves old, kvm-managed, device assignment,
but a new method is needed when VFIO is involved.

- Don

ps -- Note: just adding the flag setting in vfio-pci does not necessarily
       solve this problem.  VFIO does not know if a device is assigned to a guest;
       it only knows a caller of the ioctl requesting the device to be assigned
       to vfio, and to be dma-mapped for a region of memory, has been requested.
       So, a new PF<->VF mechanism needs to be put in place to
       determine the equivalent information.

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

* Re: [PATCH 0/4] move pci_assivned_vfs() check (while disabling VFs) to pci sub-system
  2014-11-11 19:09 ` [PATCH 0/4] move pci_assivned_vfs() check (while disabling VFs) to pci sub-system Don Dutile
@ 2014-11-11 20:39   ` Alex Williamson
  2014-11-13  7:04     ` Sathya Perla
  0 siblings, 1 reply; 12+ messages in thread
From: Alex Williamson @ 2014-11-11 20:39 UTC (permalink / raw)
  To: Don Dutile
  Cc: Sathya Perla, linux-pci, netdev, ariel.elior, linux.nics,
	shahed.shaikh

On Tue, 2014-11-11 at 14:09 -0500, Don Dutile wrote:
> On 11/10/2014 06:53 AM, Sathya Perla wrote:
> > A user must not be allowed to disable VFs while they are already assigned to
> > a guest. This check is being made in each individual driver that implements
> > the sriov_configure PCI method.
> > This patch-set fixes this code duplication by moving this check from
> > drivers to the sriov_nuvfs_store() routine just before invoking
> > sriov_configure() when num_vfs is equal to 0.
> >
> > Vasundhara Volam (4):
> >    pci: move pci_assivned_vfs() check while disabling VFs to pci
> >      sub-system
> >    bnx2x: remove pci_assigned_vfs() check while disabling VFs
> >    i40e: remove pci_assigned_vfs() check while disabling VFs
> >    qlcnic: remove pci_assigned_vfs() check while disabling VFs
> >
> >   drivers/net/ethernet/broadcom/bnx2x/bnx2x_sriov.c  |    2 +-
> >   drivers/net/ethernet/intel/i40e/i40e_virtchnl_pf.c |    7 +------
> >   .../net/ethernet/qlogic/qlcnic/qlcnic_sriov_pf.c   |   10 ----------
> >   drivers/pci/pci-sysfs.c                            |    5 +++++
> >   4 files changed, 7 insertions(+), 17 deletions(-)
> >
> I have had a side conversation with Alex Williamson, VFIO author.
> 
> VFIO is the upstream method that device-assignment is managed/handled on kvm now.
> It does not set the PCI_DEV_FLAGS_ASSIGNED pci dev-flags, and thus,
> this check will not work when VFIO is used.
> 
> This patch set will only work for the former, kvm-managed, device-assignment method,
> which is currently being deprecated in qemu as well.
> 
> So, yes, it works for kvm managed device-assignment, but not the
> newer, VFIO-based device-assignment.
> 
> Note, also, that the pci_assigned_vfs() check in the drivers will
> always return 0 when VFIO is used for device assignment, so keeping
> these checks in the drivers doesn't do what they imply either.
> 
> So, taking in the patch solves old, kvm-managed, device assignment,
> but a new method is needed when VFIO is involved.
> 
> - Don
> 
> ps -- Note: just adding the flag setting in vfio-pci does not necessarily
>        solve this problem.  VFIO does not know if a device is assigned to a guest;
>        it only knows a caller of the ioctl requesting the device to be assigned
>        to vfio, and to be dma-mapped for a region of memory, has been requested.
>        So, a new PF<->VF mechanism needs to be put in place to
>        determine the equivalent information.

pps -- Note: testing pci_assivned_vfs() is racy, nothing prevents the flag
	being added to a device between your check and removing the VF device.
	This is one of the reasons that vfio-pci doesn't use it and that this
	interface should be discouraged in the kernel.

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

* RE: [PATCH 0/4] move pci_assivned_vfs() check (while disabling VFs) to pci sub-system
  2014-11-11 20:39   ` Alex Williamson
@ 2014-11-13  7:04     ` Sathya Perla
  2014-11-13 21:36       ` Don Dutile
  0 siblings, 1 reply; 12+ messages in thread
From: Sathya Perla @ 2014-11-13  7:04 UTC (permalink / raw)
  To: Alex Williamson, Don Dutile
  Cc: linux-pci@vger.kernel.org, netdev@vger.kernel.org,
	ariel.elior@qlogic.com, linux.nics@intel.com,
	shahed.shaikh@qlogic.com

> -----Original Message-----
> From: Alex Williamson [mailto:alex.williamson@redhat.com]
> 
> On Tue, 2014-11-11 at 14:09 -0500, Don Dutile wrote:
> > On 11/10/2014 06:53 AM, Sathya Perla wrote:
> > > A user must not be allowed to disable VFs while they are already assigned
> to
> > > a guest. This check is being made in each individual driver that
> implements
> > > the sriov_configure PCI method.
> > > This patch-set fixes this code duplication by moving this check from
> > > drivers to the sriov_nuvfs_store() routine just before invoking
> > > sriov_configure() when num_vfs is equal to 0.
> > >
> > > Vasundhara Volam (4):
> > >    pci: move pci_assivned_vfs() check while disabling VFs to pci
> > >      sub-system
> > >    bnx2x: remove pci_assigned_vfs() check while disabling VFs
> > >    i40e: remove pci_assigned_vfs() check while disabling VFs
> > >    qlcnic: remove pci_assigned_vfs() check while disabling VFs
> > >
> > >   drivers/net/ethernet/broadcom/bnx2x/bnx2x_sriov.c  |    2 +-
> > >   drivers/net/ethernet/intel/i40e/i40e_virtchnl_pf.c |    7 +------
> > >   .../net/ethernet/qlogic/qlcnic/qlcnic_sriov_pf.c   |   10 ----------
> > >   drivers/pci/pci-sysfs.c                            |    5 +++++
> > >   4 files changed, 7 insertions(+), 17 deletions(-)
> > >
> > I have had a side conversation with Alex Williamson, VFIO author.
> >
> > VFIO is the upstream method that device-assignment is managed/handled
> on kvm now.
> > It does not set the PCI_DEV_FLAGS_ASSIGNED pci dev-flags, and thus,
> > this check will not work when VFIO is used.
> >
> > This patch set will only work for the former, kvm-managed, device-
> assignment method,
> > which is currently being deprecated in qemu as well.
> >
> > So, yes, it works for kvm managed device-assignment, but not the
> > newer, VFIO-based device-assignment.
> >
> > Note, also, that the pci_assigned_vfs() check in the drivers will
> > always return 0 when VFIO is used for device assignment, so keeping
> > these checks in the drivers doesn't do what they imply either.
> >
> > So, taking in the patch solves old, kvm-managed, device assignment,
> > but a new method is needed when VFIO is involved.
> >
> > - Don
> >
> > ps -- Note: just adding the flag setting in vfio-pci does not necessarily
> >        solve this problem.  VFIO does not know if a device is assigned to a
> guest;
> >        it only knows a caller of the ioctl requesting the device to be assigned
> >        to vfio, and to be dma-mapped for a region of memory, has been
> requested.
> >        So, a new PF<->VF mechanism needs to be put in place to
> >        determine the equivalent information.
> 
> pps -- Note: testing pci_assivned_vfs() is racy, nothing prevents the flag
> 	being added to a device between your check and removing the VF
> device.
> 	This is one of the reasons that vfio-pci doesn't use it and that this
> 	interface should be discouraged in the kernel.

Alex/Don, I agree with the points you've raised.
But, I'd like to know whether you think this patch-set should be accepted or not.
Even though this patch-set doesn't fix any of the pending issues raised here,
it's a small step forward as it reduces the number of invocations of pci_assigned_vfs()
check which is a good thing.

thanks,
-Sathya

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

* Re: [PATCH 0/4] move pci_assivned_vfs() check (while disabling VFs) to pci sub-system
  2014-11-13  7:04     ` Sathya Perla
@ 2014-11-13 21:36       ` Don Dutile
  0 siblings, 0 replies; 12+ messages in thread
From: Don Dutile @ 2014-11-13 21:36 UTC (permalink / raw)
  To: Sathya Perla, Alex Williamson
  Cc: linux-pci@vger.kernel.org, netdev@vger.kernel.org,
	ariel.elior@qlogic.com, linux.nics@intel.com,
	shahed.shaikh@qlogic.com

On 11/13/2014 02:04 AM, Sathya Perla wrote:
>> -----Original Message-----
>> From: Alex Williamson [mailto:alex.williamson@redhat.com]
>>
>> On Tue, 2014-11-11 at 14:09 -0500, Don Dutile wrote:
>>> On 11/10/2014 06:53 AM, Sathya Perla wrote:
>>>> A user must not be allowed to disable VFs while they are already assigned
>> to
>>>> a guest. This check is being made in each individual driver that
>> implements
>>>> the sriov_configure PCI method.
>>>> This patch-set fixes this code duplication by moving this check from
>>>> drivers to the sriov_nuvfs_store() routine just before invoking
>>>> sriov_configure() when num_vfs is equal to 0.
>>>>
>>>> Vasundhara Volam (4):
>>>>     pci: move pci_assivned_vfs() check while disabling VFs to pci
>>>>       sub-system
>>>>     bnx2x: remove pci_assigned_vfs() check while disabling VFs
>>>>     i40e: remove pci_assigned_vfs() check while disabling VFs
>>>>     qlcnic: remove pci_assigned_vfs() check while disabling VFs
>>>>
>>>>    drivers/net/ethernet/broadcom/bnx2x/bnx2x_sriov.c  |    2 +-
>>>>    drivers/net/ethernet/intel/i40e/i40e_virtchnl_pf.c |    7 +------
>>>>    .../net/ethernet/qlogic/qlcnic/qlcnic_sriov_pf.c   |   10 ----------
>>>>    drivers/pci/pci-sysfs.c                            |    5 +++++
>>>>    4 files changed, 7 insertions(+), 17 deletions(-)
>>>>
>>> I have had a side conversation with Alex Williamson, VFIO author.
>>>
>>> VFIO is the upstream method that device-assignment is managed/handled
>> on kvm now.
>>> It does not set the PCI_DEV_FLAGS_ASSIGNED pci dev-flags, and thus,
>>> this check will not work when VFIO is used.
>>>
>>> This patch set will only work for the former, kvm-managed, device-
>> assignment method,
>>> which is currently being deprecated in qemu as well.
>>>
>>> So, yes, it works for kvm managed device-assignment, but not the
>>> newer, VFIO-based device-assignment.
>>>
>>> Note, also, that the pci_assigned_vfs() check in the drivers will
>>> always return 0 when VFIO is used for device assignment, so keeping
>>> these checks in the drivers doesn't do what they imply either.
>>>
>>> So, taking in the patch solves old, kvm-managed, device assignment,
>>> but a new method is needed when VFIO is involved.
>>>
>>> - Don
>>>
>>> ps -- Note: just adding the flag setting in vfio-pci does not necessarily
>>>         solve this problem.  VFIO does not know if a device is assigned to a
>> guest;
>>>         it only knows a caller of the ioctl requesting the device to be assigned
>>>         to vfio, and to be dma-mapped for a region of memory, has been
>> requested.
>>>         So, a new PF<->VF mechanism needs to be put in place to
>>>         determine the equivalent information.
>>
>> pps -- Note: testing pci_assivned_vfs() is racy, nothing prevents the flag
>> 	being added to a device between your check and removing the VF
>> device.
>> 	This is one of the reasons that vfio-pci doesn't use it and that this
>> 	interface should be discouraged in the kernel.
>
> Alex/Don, I agree with the points you've raised.
> But, I'd like to know whether you think this patch-set should be accepted or not.
> Even though this patch-set doesn't fix any of the pending issues raised here,
> it's a small step forward as it reduces the number of invocations of pci_assigned_vfs()
> check which is a good thing.
>
> thanks,
> -Sathya
>

IMO, it's only a fix for XEN.  Upstream has moved on to VFIO-based device assignment,
and these patches do not fix the issue.  They don't make it any worse, either,
but I don't want someone scanning the patch list thinking it's been fixed either.

So, it's ok (but racy, as it is today) for kvm-managed device-assignment & Xen
pci passthrough, but does zip for VFIO-based assignment.

We need a new api - maybe another/new state in sysfs, so userspace can set it
(like qemu &/or libvirt), as well as (xen) kernel ... and meeting non-racy condition(s).

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

* Re: [PATCH 0/4] move pci_assivned_vfs() check (while disabling VFs) to pci sub-system
  2014-11-10 11:53 [PATCH 0/4] move pci_assivned_vfs() check (while disabling VFs) to pci sub-system Sathya Perla
                   ` (4 preceding siblings ...)
  2014-11-11 19:09 ` [PATCH 0/4] move pci_assivned_vfs() check (while disabling VFs) to pci sub-system Don Dutile
@ 2014-11-20 22:21 ` Bjorn Helgaas
  2014-11-20 22:42   ` [linux-nics] " Jeff Kirsher
  5 siblings, 1 reply; 12+ messages in thread
From: Bjorn Helgaas @ 2014-11-20 22:21 UTC (permalink / raw)
  To: Sathya Perla
  Cc: linux-pci, netdev, ariel.elior, linux.nics, shahed.shaikh,
	ddutile, Jeff Kirsher

[+cc Jeff]

On Mon, Nov 10, 2014 at 05:23:26PM +0530, Sathya Perla wrote:
> A user must not be allowed to disable VFs while they are already assigned to
> a guest. This check is being made in each individual driver that implements
> the sriov_configure PCI method.
> This patch-set fixes this code duplication by moving this check from
> drivers to the sriov_nuvfs_store() routine just before invoking
> sriov_configure() when num_vfs is equal to 0.
> 
> Vasundhara Volam (4):
>   pci: move pci_assivned_vfs() check while disabling VFs to pci
>     sub-system
>   bnx2x: remove pci_assigned_vfs() check while disabling VFs
>   i40e: remove pci_assigned_vfs() check while disabling VFs
>   qlcnic: remove pci_assigned_vfs() check while disabling VFs
> 
>  drivers/net/ethernet/broadcom/bnx2x/bnx2x_sriov.c  |    2 +-
>  drivers/net/ethernet/intel/i40e/i40e_virtchnl_pf.c |    7 +------
>  .../net/ethernet/qlogic/qlcnic/qlcnic_sriov_pf.c   |   10 ----------
>  drivers/pci/pci-sysfs.c                            |    5 +++++
>  4 files changed, 7 insertions(+), 17 deletions(-)

I'm dropping these for the reasons Don & Alex outlined -- they don't fix
the problem for VFIO, so this amounts to shuffling around code that's known
to be broken, which seems more confusing than worthwhile.

Jeff, if I were you I would drop the i40e patch.  I don't think it makes
sense to remove the check from i40e before adding it to the PCI core.

Bjorn

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

* Re: [linux-nics] [PATCH 0/4] move pci_assivned_vfs() check (while disabling VFs) to pci sub-system
  2014-11-20 22:21 ` Bjorn Helgaas
@ 2014-11-20 22:42   ` Jeff Kirsher
  0 siblings, 0 replies; 12+ messages in thread
From: Jeff Kirsher @ 2014-11-20 22:42 UTC (permalink / raw)
  To: Bjorn Helgaas
  Cc: Sathya Perla, linux.nics, shahed.shaikh, linux-pci, ddutile,
	ariel.elior, netdev

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

On Thu, 2014-11-20 at 15:21 -0700, Bjorn Helgaas wrote:
> [+cc Jeff]
> 
> On Mon, Nov 10, 2014 at 05:23:26PM +0530, Sathya Perla wrote:
> > A user must not be allowed to disable VFs while they are already assigned to
> > a guest. This check is being made in each individual driver that implements
> > the sriov_configure PCI method.
> > This patch-set fixes this code duplication by moving this check from
> > drivers to the sriov_nuvfs_store() routine just before invoking
> > sriov_configure() when num_vfs is equal to 0.
> > 
> > Vasundhara Volam (4):
> >   pci: move pci_assivned_vfs() check while disabling VFs to pci
> >     sub-system
> >   bnx2x: remove pci_assigned_vfs() check while disabling VFs
> >   i40e: remove pci_assigned_vfs() check while disabling VFs
> >   qlcnic: remove pci_assigned_vfs() check while disabling VFs
> > 
> >  drivers/net/ethernet/broadcom/bnx2x/bnx2x_sriov.c  |    2 +-
> >  drivers/net/ethernet/intel/i40e/i40e_virtchnl_pf.c |    7 +------
> >  .../net/ethernet/qlogic/qlcnic/qlcnic_sriov_pf.c   |   10 ----------
> >  drivers/pci/pci-sysfs.c                            |    5 +++++
> >  4 files changed, 7 insertions(+), 17 deletions(-)
> 
> I'm dropping these for the reasons Don & Alex outlined -- they don't fix
> the problem for VFIO, so this amounts to shuffling around code that's known
> to be broken, which seems more confusing than worthwhile.
> 
> Jeff, if I were you I would drop the i40e patch.  I don't think it makes
> sense to remove the check from i40e before adding it to the PCI core.
> 

Thanks for the heads up, I will drop the i40e patch as well.

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

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

end of thread, other threads:[~2014-11-20 22:42 UTC | newest]

Thread overview: 12+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2014-11-10 11:53 [PATCH 0/4] move pci_assivned_vfs() check (while disabling VFs) to pci sub-system Sathya Perla
2014-11-10 11:53 ` [PATCH 1/4] pci: move pci_assivned_vfs() check while disabling VFs " Sathya Perla
2014-11-10 11:53 ` [PATCH 2/4] bnx2x: remove pci_assigned_vfs() check while disabling VFs Sathya Perla
2014-11-10 11:53 ` [PATCH 3/4] i40e: " Sathya Perla
2014-11-11 13:52   ` Jeff Kirsher
2014-11-10 11:53 ` [PATCH 4/4] qlcnic: " Sathya Perla
2014-11-11 19:09 ` [PATCH 0/4] move pci_assivned_vfs() check (while disabling VFs) to pci sub-system Don Dutile
2014-11-11 20:39   ` Alex Williamson
2014-11-13  7:04     ` Sathya Perla
2014-11-13 21:36       ` Don Dutile
2014-11-20 22:21 ` Bjorn Helgaas
2014-11-20 22:42   ` [linux-nics] " Jeff Kirsher

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).