| Message ID | 20180312172309.3487.76690.stgit@localhost.localdomain (mailing list archive) |
|---|---|
| State | New, archived |
| Delegated to: | Bjorn Helgaas |
| Headers | show |
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.
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.
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?
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.
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.
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
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.
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. >
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)