[pci,v5,3/4] ena: Migrate over to unmanaged SR-IOV support

Message ID 20180312172309.3487.76690.stgit@localhost.localdomain
State Superseded
Delegated to: Bjorn Helgaas
Headers show
Series
  • Series short description
Related show

Commit Message

Alexander Duyck March 12, 2018, 5:23 p.m.
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(-)

Comments

David Woodhouse March 13, 2018, 8:12 a.m. | #1
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.
Christoph Hellwig March 13, 2018, 8:16 a.m. | #2
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.
David Woodhouse March 13, 2018, 8:45 a.m. | #3
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?
Christoph Hellwig March 13, 2018, 8:54 a.m. | #4
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.
David Woodhouse March 13, 2018, 9:14 a.m. | #5
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.
Alexander Duyck March 13, 2018, 2:51 p.m. | #6
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
David Woodhouse March 13, 2018, 3:04 p.m. | #7
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.
Don Dutile March 13, 2018, 4:17 p.m. | #8
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.
>

Patch

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)