* Re: [PATCH] PCI: allow drivers to limit the number of VFs to 0
2018-05-25 14:02 ` Bjorn Helgaas
@ 2018-05-25 17:01 ` Bjorn Helgaas
2018-05-25 19:27 ` Don Dutile
` (2 subsequent siblings)
3 siblings, 0 replies; 23+ messages in thread
From: Bjorn Helgaas @ 2018-05-25 17:01 UTC (permalink / raw)
To: Jakub Kicinski
Cc: Bjorn Helgaas, linux-pci, netdev, Sathya Perla, Felix Manlunas,
alexander.duyck, john.fastabend, Jacob Keller, Donald Dutile,
oss-drivers, Christoph Hellwig, Derek Chickles, Satanand Burla,
Raghu Vatsavayi, Ajit Khaparde, Sriharsha Basavapatna,
Somnath Kotur, Jeff Kirsher, intel-wired-lan
[+cc liquidio, benet, fm10k maintainers:
The patch below will affect you if your driver calls
pci_sriov_set_totalvfs(dev, 0);
Previously that caused a subsequent pci_sriov_get_totalvfs() to return
the totalVFs value from the SR-IOV capability. After this patch, it will
return 0, which has implications for VF enablement via the sysfs
"sriov_numvfs" file.]
On Fri, May 25, 2018 at 09:02:23AM -0500, Bjorn Helgaas wrote:
> On Thu, May 24, 2018 at 06:20:15PM -0700, Jakub Kicinski wrote:
> > Hi Bjorn!
> >
> > On Thu, 24 May 2018 18:57:48 -0500, Bjorn Helgaas wrote:
> > > On Mon, Apr 02, 2018 at 03:46:52PM -0700, Jakub Kicinski wrote:
> > > > Some user space depends on enabling sriov_totalvfs number of VFs
> > > > to not fail, e.g.:
> > > >
> > > > $ cat .../sriov_totalvfs > .../sriov_numvfs
> > > >
> > > > For devices which VF support depends on loaded FW we have the
> > > > pci_sriov_{g,s}et_totalvfs() API. However, this API uses 0 as
> > > > a special "unset" value, meaning drivers can't limit sriov_totalvfs
> > > > to 0. Remove the special values completely and simply initialize
> > > > driver_max_VFs to total_VFs. Then always use driver_max_VFs.
> > > > Add a helper for drivers to reset the VF limit back to total.
> > >
> > > I still can't really make sense out of the changelog.
> > >
> > > I think part of the reason it's confusing is because there are two
> > > things going on:
> > >
> > > 1) You want this:
> > >
> > > pci_sriov_set_totalvfs(dev, 0);
> > > x = pci_sriov_get_totalvfs(dev)
> > >
> > > to return 0 instead of total_VFs. That seems to connect with
> > > your subject line. It means "sriov_totalvfs" in sysfs could be
> > > 0, but I don't know how that is useful (I'm sure it is; just
> > > educate me :))
> >
> > Let me just quote the bug report that got filed on our internal bug
> > tracker :)
> >
> > When testing Juju Openstack with Ubuntu 18.04, enabling SR-IOV causes
> > errors because Juju gets the sriov_totalvfs for SR-IOV-capable device
> > then tries to set that as the sriov_numvfs parameter.
> >
> > For SR-IOV incapable FW, the sriov_totalvfs parameter should be 0,
> > but it's set to max. When FW is switched to flower*, the correct
> > sriov_totalvfs value is presented.
> >
> > * flower is a project name
>
> From the point of view of the PCI core (which knows nothing about
> device firmware and relies on the architected config space described
> by the PCIe spec), this sounds like an erratum: with some firmware
> installed, the device is not capable of SR-IOV, but still advertises
> an SR-IOV capability with "TotalVFs > 0".
>
> Regardless of whether that's an erratum, we do allow PF drivers to use
> pci_sriov_set_totalvfs() to limit the number of VFs that may be
> enabled by writing to the PF's "sriov_numvfs" sysfs file.
>
> But the current implementation does not allow a PF driver to limit VFs
> to 0, and that does seem nonsensical.
>
> > My understanding is OpenStack uses sriov_totalvfs to determine how many
> > VFs can be enabled, looks like this is the code:
> >
> > http://git.openstack.org/cgit/openstack/charm-neutron-openvswitch/tree/hooks/neutron_ovs_utils.py#n464
> >
> > > 2) You're adding the pci_sriov_reset_totalvfs() interface. I'm not
> > > sure what you intend for this. Is *every* driver supposed to
> > > call it in .remove()? Could/should this be done in the core
> > > somehow instead of depending on every driver?
> >
> > Good question, I was just thinking yesterday we may want to call it
> > from the core, but I don't think it's strictly necessary nor always
> > sufficient (we may reload FW without re-probing).
> >
> > We have a device which supports different number of VFs based on the FW
> > loaded. Some legacy FWs does not inform the driver how many VFs it can
> > support, because it supports max. So the flow in our driver is this:
> >
> > load_fw(dev);
> > ...
> > max_vfs = ask_fw_for_max_vfs(dev);
> > if (max_vfs >= 0)
> > return pci_sriov_set_totalvfs(dev, max_vfs);
> > else /* FW didn't tell us, assume max */
> > return pci_sriov_reset_totalvfs(dev);
> >
> > We also reset the max on device remove, but that's not strictly
> > necessary.
> >
> > Other users of pci_sriov_set_totalvfs() always know the value to set
> > the total to (either always get it from FW or it's a constant).
> >
> > If you prefer we can work out the correct max for those legacy cases in
> > the driver as well, although it seemed cleaner to just ask the core,
> > since it already has total_VFs value handy :)
> >
> > > I'm also having a hard time connecting your user-space command example
> > > with the rest of this. Maybe it will make more sense to me tomorrow
> > > after some coffee.
> >
> > OpenStack assumes it will always be able to set sriov_numvfs to
> > sriov_totalvfs, see this 'if':
> >
> > http://git.openstack.org/cgit/openstack/charm-neutron-openvswitch/tree/hooks/neutron_ovs_utils.py#n512
>
> Thanks for educating me. I think there are two issues here that we
> can separate. I extracted the patch below for the first.
>
> The second is the question of resetting driver_max_VFs. I think we
> currently have a general issue in the core:
>
> - load PF driver 1
> - driver calls pci_sriov_set_totalvfs() to reduce driver_max_VFs
> - unload PF driver 1
> - load PF driver 2
>
> Now driver_max_VFs is still stuck at the lower value set by driver 1.
> I don't think that's the way this should work.
>
> I guess this is partly a consequence of setting driver_max_VFs in
> sriov_init(), which is called before driver attach and should only
> depend on hardware characteristics, so it is related to the patch
> below. But I think we should fix it in general, not just for
> netronome.
>
>
> commit 4a338bc6f94b9ad824ac944f5dfc249d6838719c
> Author: Jakub Kicinski <jakub.kicinski@netronome.com>
> Date: Fri May 25 08:18:34 2018 -0500
>
> PCI/IOV: Allow PF drivers to limit total_VFs to 0
>
> Some SR-IOV PF drivers implement .sriov_configure(), which allows
> user-space to enable VFs by writing the desired number of VFs to the sysfs
> "sriov_numvfs" file (see sriov_numvfs_store()).
>
> The PCI core limits the number of VFs to the TotalVFs advertised by the
> device in its SR-IOV capability. The PF driver can limit the number of VFs
> to even fewer (it may have pre-allocated data structures or knowledge of
> device limitations) by calling pci_sriov_set_totalvfs(), but previously it
> could not limit the VFs to 0.
>
> Change pci_sriov_get_totalvfs() so it always respects the VF limit imposed
> by the PF driver, even if the limit is 0.
>
> This sequence:
>
> pci_sriov_set_totalvfs(dev, 0);
> x = pci_sriov_get_totalvfs(dev);
>
> previously set "x" to TotalVFs from the SR-IOV capability. Now it will set
> "x" to 0.
>
> Signed-off-by: Jakub Kicinski <jakub.kicinski@netronome.com>
> Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>
>
> diff --git a/drivers/pci/iov.c b/drivers/pci/iov.c
> index 192b82898a38..d0d73dbbd5ca 100644
> --- a/drivers/pci/iov.c
> +++ b/drivers/pci/iov.c
> @@ -469,6 +469,7 @@ static int sriov_init(struct pci_dev *dev, int pos)
> iov->nres = nres;
> iov->ctrl = ctrl;
> iov->total_VFs = total;
> + iov->driver_max_VFs = total;
> pci_read_config_word(dev, pos + PCI_SRIOV_VF_DID, &iov->vf_device);
> iov->pgsz = pgsz;
> iov->self = dev;
> @@ -827,10 +828,7 @@ int pci_sriov_get_totalvfs(struct pci_dev *dev)
> if (!dev->is_physfn)
> return 0;
>
> - if (dev->sriov->driver_max_VFs)
> - return dev->sriov->driver_max_VFs;
> -
> - return dev->sriov->total_VFs;
> + return dev->sriov->driver_max_VFs;
> }
> EXPORT_SYMBOL_GPL(pci_sriov_get_totalvfs);
>
^ permalink raw reply [flat|nested] 23+ messages in thread* [Intel-wired-lan] [PATCH] PCI: allow drivers to limit the number of VFs to 0
@ 2018-05-25 17:01 ` Bjorn Helgaas
0 siblings, 0 replies; 23+ messages in thread
From: Bjorn Helgaas @ 2018-05-25 17:01 UTC (permalink / raw)
To: intel-wired-lan
[+cc liquidio, benet, fm10k maintainers:
The patch below will affect you if your driver calls
pci_sriov_set_totalvfs(dev, 0);
Previously that caused a subsequent pci_sriov_get_totalvfs() to return
the totalVFs value from the SR-IOV capability. After this patch, it will
return 0, which has implications for VF enablement via the sysfs
"sriov_numvfs" file.]
On Fri, May 25, 2018 at 09:02:23AM -0500, Bjorn Helgaas wrote:
> On Thu, May 24, 2018 at 06:20:15PM -0700, Jakub Kicinski wrote:
> > Hi Bjorn!
> >
> > On Thu, 24 May 2018 18:57:48 -0500, Bjorn Helgaas wrote:
> > > On Mon, Apr 02, 2018 at 03:46:52PM -0700, Jakub Kicinski wrote:
> > > > Some user space depends on enabling sriov_totalvfs number of VFs
> > > > to not fail, e.g.:
> > > >
> > > > $ cat .../sriov_totalvfs > .../sriov_numvfs
> > > >
> > > > For devices which VF support depends on loaded FW we have the
> > > > pci_sriov_{g,s}et_totalvfs() API. However, this API uses 0 as
> > > > a special "unset" value, meaning drivers can't limit sriov_totalvfs
> > > > to 0. Remove the special values completely and simply initialize
> > > > driver_max_VFs to total_VFs. Then always use driver_max_VFs.
> > > > Add a helper for drivers to reset the VF limit back to total.
> > >
> > > I still can't really make sense out of the changelog.
> > >
> > > I think part of the reason it's confusing is because there are two
> > > things going on:
> > >
> > > 1) You want this:
> > >
> > > pci_sriov_set_totalvfs(dev, 0);
> > > x = pci_sriov_get_totalvfs(dev)
> > >
> > > to return 0 instead of total_VFs. That seems to connect with
> > > your subject line. It means "sriov_totalvfs" in sysfs could be
> > > 0, but I don't know how that is useful (I'm sure it is; just
> > > educate me :))
> >
> > Let me just quote the bug report that got filed on our internal bug
> > tracker :)
> >
> > When testing Juju Openstack with Ubuntu 18.04, enabling SR-IOV causes
> > errors because Juju gets the sriov_totalvfs for SR-IOV-capable device
> > then tries to set that as the sriov_numvfs parameter.
> >
> > For SR-IOV incapable FW, the sriov_totalvfs parameter should be 0,
> > but it's set to max. When FW is switched to flower*, the correct
> > sriov_totalvfs value is presented.
> >
> > * flower is a project name
>
> From the point of view of the PCI core (which knows nothing about
> device firmware and relies on the architected config space described
> by the PCIe spec), this sounds like an erratum: with some firmware
> installed, the device is not capable of SR-IOV, but still advertises
> an SR-IOV capability with "TotalVFs > 0".
>
> Regardless of whether that's an erratum, we do allow PF drivers to use
> pci_sriov_set_totalvfs() to limit the number of VFs that may be
> enabled by writing to the PF's "sriov_numvfs" sysfs file.
>
> But the current implementation does not allow a PF driver to limit VFs
> to 0, and that does seem nonsensical.
>
> > My understanding is OpenStack uses sriov_totalvfs to determine how many
> > VFs can be enabled, looks like this is the code:
> >
> > http://git.openstack.org/cgit/openstack/charm-neutron-openvswitch/tree/hooks/neutron_ovs_utils.py#n464
> >
> > > 2) You're adding the pci_sriov_reset_totalvfs() interface. I'm not
> > > sure what you intend for this. Is *every* driver supposed to
> > > call it in .remove()? Could/should this be done in the core
> > > somehow instead of depending on every driver?
> >
> > Good question, I was just thinking yesterday we may want to call it
> > from the core, but I don't think it's strictly necessary nor always
> > sufficient (we may reload FW without re-probing).
> >
> > We have a device which supports different number of VFs based on the FW
> > loaded. Some legacy FWs does not inform the driver how many VFs it can
> > support, because it supports max. So the flow in our driver is this:
> >
> > load_fw(dev);
> > ...
> > max_vfs = ask_fw_for_max_vfs(dev);
> > if (max_vfs >= 0)
> > return pci_sriov_set_totalvfs(dev, max_vfs);
> > else /* FW didn't tell us, assume max */
> > return pci_sriov_reset_totalvfs(dev);
> >
> > We also reset the max on device remove, but that's not strictly
> > necessary.
> >
> > Other users of pci_sriov_set_totalvfs() always know the value to set
> > the total to (either always get it from FW or it's a constant).
> >
> > If you prefer we can work out the correct max for those legacy cases in
> > the driver as well, although it seemed cleaner to just ask the core,
> > since it already has total_VFs value handy :)
> >
> > > I'm also having a hard time connecting your user-space command example
> > > with the rest of this. Maybe it will make more sense to me tomorrow
> > > after some coffee.
> >
> > OpenStack assumes it will always be able to set sriov_numvfs to
> > sriov_totalvfs, see this 'if':
> >
> > http://git.openstack.org/cgit/openstack/charm-neutron-openvswitch/tree/hooks/neutron_ovs_utils.py#n512
>
> Thanks for educating me. I think there are two issues here that we
> can separate. I extracted the patch below for the first.
>
> The second is the question of resetting driver_max_VFs. I think we
> currently have a general issue in the core:
>
> - load PF driver 1
> - driver calls pci_sriov_set_totalvfs() to reduce driver_max_VFs
> - unload PF driver 1
> - load PF driver 2
>
> Now driver_max_VFs is still stuck at the lower value set by driver 1.
> I don't think that's the way this should work.
>
> I guess this is partly a consequence of setting driver_max_VFs in
> sriov_init(), which is called before driver attach and should only
> depend on hardware characteristics, so it is related to the patch
> below. But I think we should fix it in general, not just for
> netronome.
>
>
> commit 4a338bc6f94b9ad824ac944f5dfc249d6838719c
> Author: Jakub Kicinski <jakub.kicinski@netronome.com>
> Date: Fri May 25 08:18:34 2018 -0500
>
> PCI/IOV: Allow PF drivers to limit total_VFs to 0
>
> Some SR-IOV PF drivers implement .sriov_configure(), which allows
> user-space to enable VFs by writing the desired number of VFs to the sysfs
> "sriov_numvfs" file (see sriov_numvfs_store()).
>
> The PCI core limits the number of VFs to the TotalVFs advertised by the
> device in its SR-IOV capability. The PF driver can limit the number of VFs
> to even fewer (it may have pre-allocated data structures or knowledge of
> device limitations) by calling pci_sriov_set_totalvfs(), but previously it
> could not limit the VFs to 0.
>
> Change pci_sriov_get_totalvfs() so it always respects the VF limit imposed
> by the PF driver, even if the limit is 0.
>
> This sequence:
>
> pci_sriov_set_totalvfs(dev, 0);
> x = pci_sriov_get_totalvfs(dev);
>
> previously set "x" to TotalVFs from the SR-IOV capability. Now it will set
> "x" to 0.
>
> Signed-off-by: Jakub Kicinski <jakub.kicinski@netronome.com>
> Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>
>
> diff --git a/drivers/pci/iov.c b/drivers/pci/iov.c
> index 192b82898a38..d0d73dbbd5ca 100644
> --- a/drivers/pci/iov.c
> +++ b/drivers/pci/iov.c
> @@ -469,6 +469,7 @@ static int sriov_init(struct pci_dev *dev, int pos)
> iov->nres = nres;
> iov->ctrl = ctrl;
> iov->total_VFs = total;
> + iov->driver_max_VFs = total;
> pci_read_config_word(dev, pos + PCI_SRIOV_VF_DID, &iov->vf_device);
> iov->pgsz = pgsz;
> iov->self = dev;
> @@ -827,10 +828,7 @@ int pci_sriov_get_totalvfs(struct pci_dev *dev)
> if (!dev->is_physfn)
> return 0;
>
> - if (dev->sriov->driver_max_VFs)
> - return dev->sriov->driver_max_VFs;
> -
> - return dev->sriov->total_VFs;
> + return dev->sriov->driver_max_VFs;
> }
> EXPORT_SYMBOL_GPL(pci_sriov_get_totalvfs);
>
^ permalink raw reply [flat|nested] 23+ messages in thread* RE: [PATCH] PCI: allow drivers to limit the number of VFs to 0
2018-05-25 17:01 ` [Intel-wired-lan] " Bjorn Helgaas
(?)
@ 2018-05-25 17:46 ` Keller, Jacob E
-1 siblings, 0 replies; 23+ messages in thread
From: Keller, Jacob E @ 2018-05-25 17:46 UTC (permalink / raw)
To: Bjorn Helgaas, Jakub Kicinski
Cc: Bjorn Helgaas, linux-pci, netdev, Sathya Perla, Felix Manlunas,
alexander.duyck, john.fastabend, Donald Dutile, oss-drivers,
Christoph Hellwig, Derek Chickles, Satanand Burla,
Raghu Vatsavayi, Ajit Khaparde, Sriharsha Basavapatna,
Somnath Kotur
> -----Original Message-----
> From: Bjorn Helgaas [mailto:helgaas@kernel.org]
> Sent: Friday, May 25, 2018 10:01 AM
> To: Jakub Kicinski <jakub.kicinski@netronome.com>
> Cc: Bjorn Helgaas <bhelgaas@google.com>; linux-pci@vger.kernel.org;
> netdev@vger.kernel.org; Sathya Perla <sathya.perla@broadcom.com>; Felix
> Manlunas <felix.manlunas@caviumnetworks.com>;
> alexander.duyck@gmail.com; john.fastabend@gmail.com; Keller, Jacob E
> <jacob.e.keller@intel.com>; Donald Dutile <ddutile@redhat.com>; oss-
> drivers@netronome.com; Christoph Hellwig <hch@infradead.org>; Derek
> Chickles <derek.chickles@caviumnetworks.com>; Satanand Burla
> <satananda.burla@caviumnetworks.com>; Raghu Vatsavayi
> <raghu.vatsavayi@caviumnetworks.com>; Ajit Khaparde
> <ajit.khaparde@broadcom.com>; Sriharsha Basavapatna
> <sriharsha.basavapatna@broadcom.com>; Somnath Kotur
> <somnath.kotur@broadcom.com>; Kirsher, Jeffrey T
> <jeffrey.t.kirsher@intel.com>; intel-wired-lan@lists.osuosl.org
> Subject: Re: [PATCH] PCI: allow drivers to limit the number of VFs to 0
>
> [+cc liquidio, benet, fm10k maintainers:
>
> The patch below will affect you if your driver calls
> pci_sriov_set_totalvfs(dev, 0);
>
> Previously that caused a subsequent pci_sriov_get_totalvfs() to return
> the totalVFs value from the SR-IOV capability. After this patch, it will
> return 0, which has implications for VF enablement via the sysfs
> "sriov_numvfs" file.]
>
Thanks. I don't foresee any issues with fm10k regarding this..
Thanks,
Jake
^ permalink raw reply [flat|nested] 23+ messages in thread
* [Intel-wired-lan] [PATCH] PCI: allow drivers to limit the number of VFs to 0
@ 2018-05-25 17:46 ` Keller, Jacob E
0 siblings, 0 replies; 23+ messages in thread
From: Keller, Jacob E @ 2018-05-25 17:46 UTC (permalink / raw)
To: intel-wired-lan
> -----Original Message-----
> From: Bjorn Helgaas [mailto:helgaas at kernel.org]
> Sent: Friday, May 25, 2018 10:01 AM
> To: Jakub Kicinski <jakub.kicinski@netronome.com>
> Cc: Bjorn Helgaas <bhelgaas@google.com>; linux-pci at vger.kernel.org;
> netdev at vger.kernel.org; Sathya Perla <sathya.perla@broadcom.com>; Felix
> Manlunas <felix.manlunas@caviumnetworks.com>;
> alexander.duyck at gmail.com; john.fastabend at gmail.com; Keller, Jacob E
> <jacob.e.keller@intel.com>; Donald Dutile <ddutile@redhat.com>; oss-
> drivers at netronome.com; Christoph Hellwig <hch@infradead.org>; Derek
> Chickles <derek.chickles@caviumnetworks.com>; Satanand Burla
> <satananda.burla@caviumnetworks.com>; Raghu Vatsavayi
> <raghu.vatsavayi@caviumnetworks.com>; Ajit Khaparde
> <ajit.khaparde@broadcom.com>; Sriharsha Basavapatna
> <sriharsha.basavapatna@broadcom.com>; Somnath Kotur
> <somnath.kotur@broadcom.com>; Kirsher, Jeffrey T
> <jeffrey.t.kirsher@intel.com>; intel-wired-lan at lists.osuosl.org
> Subject: Re: [PATCH] PCI: allow drivers to limit the number of VFs to 0
>
> [+cc liquidio, benet, fm10k maintainers:
>
> The patch below will affect you if your driver calls
> pci_sriov_set_totalvfs(dev, 0);
>
> Previously that caused a subsequent pci_sriov_get_totalvfs() to return
> the totalVFs value from the SR-IOV capability. After this patch, it will
> return 0, which has implications for VF enablement via the sysfs
> "sriov_numvfs" file.]
>
Thanks. I don't foresee any issues with fm10k regarding this..
Thanks,
Jake
^ permalink raw reply [flat|nested] 23+ messages in thread
* RE: [PATCH] PCI: allow drivers to limit the number of VFs to 0
@ 2018-05-25 17:46 ` Keller, Jacob E
0 siblings, 0 replies; 23+ messages in thread
From: Keller, Jacob E @ 2018-05-25 17:46 UTC (permalink / raw)
To: Bjorn Helgaas, Jakub Kicinski
Cc: Bjorn Helgaas, linux-pci, netdev, Sathya Perla, Felix Manlunas,
alexander.duyck, john.fastabend, Donald Dutile, oss-drivers,
Christoph Hellwig, Derek Chickles, Satanand Burla,
Raghu Vatsavayi, Ajit Khaparde, Sriharsha Basavapatna,
Somnath Kotur, Kirsher, Jeffrey T, intel-wired-lan
> -----Original Message-----
> From: Bjorn Helgaas [mailto:helgaas@kernel.org]
> Sent: Friday, May 25, 2018 10:01 AM
> To: Jakub Kicinski <jakub.kicinski@netronome.com>
> Cc: Bjorn Helgaas <bhelgaas@google.com>; linux-pci@vger.kernel.org;
> netdev@vger.kernel.org; Sathya Perla <sathya.perla@broadcom.com>; Felix
> Manlunas <felix.manlunas@caviumnetworks.com>;
> alexander.duyck@gmail.com; john.fastabend@gmail.com; Keller, Jacob E
> <jacob.e.keller@intel.com>; Donald Dutile <ddutile@redhat.com>; oss-
> drivers@netronome.com; Christoph Hellwig <hch@infradead.org>; Derek
> Chickles <derek.chickles@caviumnetworks.com>; Satanand Burla
> <satananda.burla@caviumnetworks.com>; Raghu Vatsavayi
> <raghu.vatsavayi@caviumnetworks.com>; Ajit Khaparde
> <ajit.khaparde@broadcom.com>; Sriharsha Basavapatna
> <sriharsha.basavapatna@broadcom.com>; Somnath Kotur
> <somnath.kotur@broadcom.com>; Kirsher, Jeffrey T
> <jeffrey.t.kirsher@intel.com>; intel-wired-lan@lists.osuosl.org
> Subject: Re: [PATCH] PCI: allow drivers to limit the number of VFs to 0
>=20
> [+cc liquidio, benet, fm10k maintainers:
>=20
> The patch below will affect you if your driver calls
> pci_sriov_set_totalvfs(dev, 0);
>=20
> Previously that caused a subsequent pci_sriov_get_totalvfs() to return
> the totalVFs value from the SR-IOV capability. After this patch, it wi=
ll
> return 0, which has implications for VF enablement via the sysfs
> "sriov_numvfs" file.]
>=20
Thanks. I don't foresee any issues with fm10k regarding this..
Thanks,
Jake
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH] PCI: allow drivers to limit the number of VFs to 0
2018-05-25 14:02 ` Bjorn Helgaas
2018-05-25 17:01 ` [Intel-wired-lan] " Bjorn Helgaas
@ 2018-05-25 19:27 ` Don Dutile
2018-05-25 20:46 ` Bjorn Helgaas
2018-05-25 21:05 ` Jakub Kicinski
2018-06-19 21:37 ` Bjorn Helgaas
3 siblings, 1 reply; 23+ messages in thread
From: Don Dutile @ 2018-05-25 19:27 UTC (permalink / raw)
To: Bjorn Helgaas, Jakub Kicinski
Cc: Bjorn Helgaas, linux-pci, netdev, Sathya Perla, Felix Manlunas,
alexander.duyck, john.fastabend, Jacob Keller, oss-drivers,
Christoph Hellwig
On 05/25/2018 10:02 AM, Bjorn Helgaas wrote:
> On Thu, May 24, 2018 at 06:20:15PM -0700, Jakub Kicinski wrote:
>> Hi Bjorn!
>>
>> On Thu, 24 May 2018 18:57:48 -0500, Bjorn Helgaas wrote:
>>> On Mon, Apr 02, 2018 at 03:46:52PM -0700, Jakub Kicinski wrote:
>>>> Some user space depends on enabling sriov_totalvfs number of VFs
>>>> to not fail, e.g.:
>>>>
>>>> $ cat .../sriov_totalvfs > .../sriov_numvfs
>>>>
>>>> For devices which VF support depends on loaded FW we have the
>>>> pci_sriov_{g,s}et_totalvfs() API. However, this API uses 0 as
>>>> a special "unset" value, meaning drivers can't limit sriov_totalvfs
>>>> to 0. Remove the special values completely and simply initialize
>>>> driver_max_VFs to total_VFs. Then always use driver_max_VFs.
>>>> Add a helper for drivers to reset the VF limit back to total.
>>>
>>> I still can't really make sense out of the changelog.
>>>
>>> I think part of the reason it's confusing is because there are two
>>> things going on:
>>>
>>> 1) You want this:
>>>
>>> pci_sriov_set_totalvfs(dev, 0);
>>> x = pci_sriov_get_totalvfs(dev)
>>>
>>> to return 0 instead of total_VFs. That seems to connect with
>>> your subject line. It means "sriov_totalvfs" in sysfs could be
>>> 0, but I don't know how that is useful (I'm sure it is; just
>>> educate me :))
>>
>> Let me just quote the bug report that got filed on our internal bug
>> tracker :)
>>
>> When testing Juju Openstack with Ubuntu 18.04, enabling SR-IOV causes
>> errors because Juju gets the sriov_totalvfs for SR-IOV-capable device
>> then tries to set that as the sriov_numvfs parameter.
>>
>> For SR-IOV incapable FW, the sriov_totalvfs parameter should be 0,
>> but it's set to max. When FW is switched to flower*, the correct
>> sriov_totalvfs value is presented.
>>
>> * flower is a project name
>
> From the point of view of the PCI core (which knows nothing about
> device firmware and relies on the architected config space described
> by the PCIe spec), this sounds like an erratum: with some firmware
> installed, the device is not capable of SR-IOV, but still advertises
> an SR-IOV capability with "TotalVFs > 0".
>
> Regardless of whether that's an erratum, we do allow PF drivers to use
> pci_sriov_set_totalvfs() to limit the number of VFs that may be
> enabled by writing to the PF's "sriov_numvfs" sysfs file.
>
+1.
> But the current implementation does not allow a PF driver to limit VFs
> to 0, and that does seem nonsensical.
>
Well, not really -- claiming to support VFs, and then wanting it to be 0...
I could certainly argue is non-sensical.
From a sw perspective, sure, see if we can set VFs to 0 (and reset to another value later).
/me wishes that implementers would follow the architecture vs torquing it into strange shapes.
>> My understanding is OpenStack uses sriov_totalvfs to determine how many
>> VFs can be enabled, looks like this is the code:
>>
>> http://git.openstack.org/cgit/openstack/charm-neutron-openvswitch/tree/hooks/neutron_ovs_utils.py#n464
>>
>>> 2) You're adding the pci_sriov_reset_totalvfs() interface. I'm not
>>> sure what you intend for this. Is *every* driver supposed to
>>> call it in .remove()? Could/should this be done in the core
>>> somehow instead of depending on every driver?
>>
>> Good question, I was just thinking yesterday we may want to call it
>> from the core, but I don't think it's strictly necessary nor always
>> sufficient (we may reload FW without re-probing).
>>
>> We have a device which supports different number of VFs based on the FW
>> loaded. Some legacy FWs does not inform the driver how many VFs it can
>> support, because it supports max. So the flow in our driver is this:
>>
>> load_fw(dev);
>> ...
>> max_vfs = ask_fw_for_max_vfs(dev);
>> if (max_vfs >= 0)
>> return pci_sriov_set_totalvfs(dev, max_vfs);
>> else /* FW didn't tell us, assume max */
>> return pci_sriov_reset_totalvfs(dev);
>>
>> We also reset the max on device remove, but that's not strictly
>> necessary.
>>
>> Other users of pci_sriov_set_totalvfs() always know the value to set
>> the total to (either always get it from FW or it's a constant).
>>
>> If you prefer we can work out the correct max for those legacy cases in
>> the driver as well, although it seemed cleaner to just ask the core,
>> since it already has total_VFs value handy :)
>>
>>> I'm also having a hard time connecting your user-space command example
>>> with the rest of this. Maybe it will make more sense to me tomorrow
>>> after some coffee.
>>
>> OpenStack assumes it will always be able to set sriov_numvfs to
>> sriov_totalvfs, see this 'if':
>>
>> http://git.openstack.org/cgit/openstack/charm-neutron-openvswitch/tree/hooks/neutron_ovs_utils.py#n512
>
> Thanks for educating me. I think there are two issues here that we
> can separate. I extracted the patch below for the first.
>
> The second is the question of resetting driver_max_VFs. I think we
> currently have a general issue in the core:
>
> - load PF driver 1
> - driver calls pci_sriov_set_totalvfs() to reduce driver_max_VFs
> - unload PF driver 1
> - load PF driver 2
>
> Now driver_max_VFs is still stuck at the lower value set by driver 1.
> I don't think that's the way this should work.
>
> I guess this is partly a consequence of setting driver_max_VFs in
> sriov_init(), which is called before driver attach and should only
um, if it's at sriov_init() how is max changed by a PF driver?
or am I missing something subtle (a new sysfs param) as to what is being changed?
> depend on hardware characteristics, so it is related to the patch
> below. But I think we should fix it in general, not just for
> netronome.
>
>
> commit 4a338bc6f94b9ad824ac944f5dfc249d6838719c
> Author: Jakub Kicinski <jakub.kicinski@netronome.com>
> Date: Fri May 25 08:18:34 2018 -0500
>
> PCI/IOV: Allow PF drivers to limit total_VFs to 0
>
> Some SR-IOV PF drivers implement .sriov_configure(), which allows
> user-space to enable VFs by writing the desired number of VFs to the sysfs
> "sriov_numvfs" file (see sriov_numvfs_store()).
>
> The PCI core limits the number of VFs to the TotalVFs advertised by the
> device in its SR-IOV capability. The PF driver can limit the number of VFs
> to even fewer (it may have pre-allocated data structures or knowledge of
> device limitations) by calling pci_sriov_set_totalvfs(), but previously it
> could not limit the VFs to 0.
>
> Change pci_sriov_get_totalvfs() so it always respects the VF limit imposed
> by the PF driver, even if the limit is 0.
>
> This sequence:
>
> pci_sriov_set_totalvfs(dev, 0);
> x = pci_sriov_get_totalvfs(dev);
>
> previously set "x" to TotalVFs from the SR-IOV capability. Now it will set
> "x" to 0.
>
> Signed-off-by: Jakub Kicinski <jakub.kicinski@netronome.com>
> Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>
>
> diff --git a/drivers/pci/iov.c b/drivers/pci/iov.c
> index 192b82898a38..d0d73dbbd5ca 100644
> --- a/drivers/pci/iov.c
> +++ b/drivers/pci/iov.c
> @@ -469,6 +469,7 @@ static int sriov_init(struct pci_dev *dev, int pos)
> iov->nres = nres;
> iov->ctrl = ctrl;
> iov->total_VFs = total;
> + iov->driver_max_VFs = total;
> pci_read_config_word(dev, pos + PCI_SRIOV_VF_DID, &iov->vf_device);
> iov->pgsz = pgsz;
> iov->self = dev;
> @@ -827,10 +828,7 @@ int pci_sriov_get_totalvfs(struct pci_dev *dev)
> if (!dev->is_physfn)
> return 0;
>
> - if (dev->sriov->driver_max_VFs)
> - return dev->sriov->driver_max_VFs;
> -
> - return dev->sriov->total_VFs;
> + return dev->sriov->driver_max_VFs;
> }
> EXPORT_SYMBOL_GPL(pci_sriov_get_totalvfs);
>
>
^ permalink raw reply [flat|nested] 23+ messages in thread* Re: [PATCH] PCI: allow drivers to limit the number of VFs to 0
2018-05-25 19:27 ` Don Dutile
@ 2018-05-25 20:46 ` Bjorn Helgaas
2018-05-29 14:29 ` Don Dutile
0 siblings, 1 reply; 23+ messages in thread
From: Bjorn Helgaas @ 2018-05-25 20:46 UTC (permalink / raw)
To: Don Dutile
Cc: Jakub Kicinski, Bjorn Helgaas, linux-pci, netdev, Sathya Perla,
Felix Manlunas, alexander.duyck, john.fastabend, Jacob Keller,
oss-drivers, Christoph Hellwig
On Fri, May 25, 2018 at 03:27:52PM -0400, Don Dutile wrote:
> On 05/25/2018 10:02 AM, Bjorn Helgaas wrote:
> > On Thu, May 24, 2018 at 06:20:15PM -0700, Jakub Kicinski wrote:
> > > Hi Bjorn!
> > >
> > > On Thu, 24 May 2018 18:57:48 -0500, Bjorn Helgaas wrote:
> > > > On Mon, Apr 02, 2018 at 03:46:52PM -0700, Jakub Kicinski wrote:
> > > > > Some user space depends on enabling sriov_totalvfs number of VFs
> > > > > to not fail, e.g.:
> > > > >
> > > > > $ cat .../sriov_totalvfs > .../sriov_numvfs
> > > > >
> > > > > For devices which VF support depends on loaded FW we have the
> > > > > pci_sriov_{g,s}et_totalvfs() API. However, this API uses 0 as
> > > > > a special "unset" value, meaning drivers can't limit sriov_totalvfs
> > > > > to 0. Remove the special values completely and simply initialize
> > > > > driver_max_VFs to total_VFs. Then always use driver_max_VFs.
> > > > > Add a helper for drivers to reset the VF limit back to total.
> > > >
> > > > I still can't really make sense out of the changelog.
> > > >
> > > > I think part of the reason it's confusing is because there are two
> > > > things going on:
> > > >
> > > > 1) You want this:
> > > > pci_sriov_set_totalvfs(dev, 0);
> > > > x = pci_sriov_get_totalvfs(dev)
> > > >
> > > > to return 0 instead of total_VFs. That seems to connect with
> > > > your subject line. It means "sriov_totalvfs" in sysfs could be
> > > > 0, but I don't know how that is useful (I'm sure it is; just
> > > > educate me :))
> > >
> > > Let me just quote the bug report that got filed on our internal bug
> > > tracker :)
> > >
> > > When testing Juju Openstack with Ubuntu 18.04, enabling SR-IOV causes
> > > errors because Juju gets the sriov_totalvfs for SR-IOV-capable device
> > > then tries to set that as the sriov_numvfs parameter.
> > >
> > > For SR-IOV incapable FW, the sriov_totalvfs parameter should be 0,
> > > but it's set to max. When FW is switched to flower*, the correct
> > > sriov_totalvfs value is presented.
> > >
> > > * flower is a project name
> >
> > From the point of view of the PCI core (which knows nothing about
> > device firmware and relies on the architected config space described
> > by the PCIe spec), this sounds like an erratum: with some firmware
> > installed, the device is not capable of SR-IOV, but still advertises
> > an SR-IOV capability with "TotalVFs > 0".
> >
> > Regardless of whether that's an erratum, we do allow PF drivers to use
> > pci_sriov_set_totalvfs() to limit the number of VFs that may be
> > enabled by writing to the PF's "sriov_numvfs" sysfs file.
> >
> +1.
>
> > But the current implementation does not allow a PF driver to limit VFs
> > to 0, and that does seem nonsensical.
> >
> Well, not really -- claiming to support VFs, and then wanting it to be 0...
> I could certainly argue is non-sensical.
> From a sw perspective, sure, see if we can set VFs to 0 (and reset to another value later).
>
> /me wishes that implementers would follow the architecture vs torquing it into strange shapes.
>
> > > My understanding is OpenStack uses sriov_totalvfs to determine how many
> > > VFs can be enabled, looks like this is the code:
> > >
> > > http://git.openstack.org/cgit/openstack/charm-neutron-openvswitch/tree/hooks/neutron_ovs_utils.py#n464
> > >
> > > > 2) You're adding the pci_sriov_reset_totalvfs() interface. I'm not
> > > > sure what you intend for this. Is *every* driver supposed to
> > > > call it in .remove()? Could/should this be done in the core
> > > > somehow instead of depending on every driver?
> > >
> > > Good question, I was just thinking yesterday we may want to call it
> > > from the core, but I don't think it's strictly necessary nor always
> > > sufficient (we may reload FW without re-probing).
> > >
> > > We have a device which supports different number of VFs based on the FW
> > > loaded. Some legacy FWs does not inform the driver how many VFs it can
> > > support, because it supports max. So the flow in our driver is this:
> > >
> > > load_fw(dev);
> > > ...
> > > max_vfs = ask_fw_for_max_vfs(dev);
> > > if (max_vfs >= 0)
> > > return pci_sriov_set_totalvfs(dev, max_vfs);
> > > else /* FW didn't tell us, assume max */
> > > return pci_sriov_reset_totalvfs(dev);
> > >
> > > We also reset the max on device remove, but that's not strictly
> > > necessary.
> > >
> > > Other users of pci_sriov_set_totalvfs() always know the value to set
> > > the total to (either always get it from FW or it's a constant).
> > >
> > > If you prefer we can work out the correct max for those legacy cases in
> > > the driver as well, although it seemed cleaner to just ask the core,
> > > since it already has total_VFs value handy :)
> > >
> > > > I'm also having a hard time connecting your user-space command example
> > > > with the rest of this. Maybe it will make more sense to me tomorrow
> > > > after some coffee.
> > >
> > > OpenStack assumes it will always be able to set sriov_numvfs to
> > > sriov_totalvfs, see this 'if':
> > >
> > > http://git.openstack.org/cgit/openstack/charm-neutron-openvswitch/tree/hooks/neutron_ovs_utils.py#n512
> >
> > Thanks for educating me. I think there are two issues here that we
> > can separate. I extracted the patch below for the first.
> >
> > The second is the question of resetting driver_max_VFs. I think we
> > currently have a general issue in the core:
> >
> > - load PF driver 1
> > - driver calls pci_sriov_set_totalvfs() to reduce driver_max_VFs
> > - unload PF driver 1
> > - load PF driver 2
> >
> > Now driver_max_VFs is still stuck at the lower value set by driver 1.
> > I don't think that's the way this should work.
> >
> > I guess this is partly a consequence of setting driver_max_VFs in
> > sriov_init(), which is called before driver attach and should only
> um, if it's at sriov_init() how is max changed by a PF driver?
> or am I missing something subtle (a new sysfs param) as to what is being changed?
sriov_init() basically just sets the default driver_max_VFs to Total_VFs.
If the PF driver later calls pci_sriov_set_totalvfs(), it can reduce
driver_max_VFs.
My concern is that there's nothing that resets driver_max_VFs back to
Total_VFs if we unload and reload the PF driver.
> > depend on hardware characteristics, so it is related to the patch
> > below. But I think we should fix it in general, not just for
> > netronome.
> >
> >
> > commit 4a338bc6f94b9ad824ac944f5dfc249d6838719c
> > Author: Jakub Kicinski <jakub.kicinski@netronome.com>
> > Date: Fri May 25 08:18:34 2018 -0500
> >
> > PCI/IOV: Allow PF drivers to limit total_VFs to 0
> > Some SR-IOV PF drivers implement .sriov_configure(), which allows
> > user-space to enable VFs by writing the desired number of VFs to the sysfs
> > "sriov_numvfs" file (see sriov_numvfs_store()).
> > The PCI core limits the number of VFs to the TotalVFs advertised by the
> > device in its SR-IOV capability. The PF driver can limit the number of VFs
> > to even fewer (it may have pre-allocated data structures or knowledge of
> > device limitations) by calling pci_sriov_set_totalvfs(), but previously it
> > could not limit the VFs to 0.
> > Change pci_sriov_get_totalvfs() so it always respects the VF limit imposed
> > by the PF driver, even if the limit is 0.
> > This sequence:
> > pci_sriov_set_totalvfs(dev, 0);
> > x = pci_sriov_get_totalvfs(dev);
> > previously set "x" to TotalVFs from the SR-IOV capability. Now it will set
> > "x" to 0.
> > Signed-off-by: Jakub Kicinski <jakub.kicinski@netronome.com>
> > Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>
> >
> > diff --git a/drivers/pci/iov.c b/drivers/pci/iov.c
> > index 192b82898a38..d0d73dbbd5ca 100644
> > --- a/drivers/pci/iov.c
> > +++ b/drivers/pci/iov.c
> > @@ -469,6 +469,7 @@ static int sriov_init(struct pci_dev *dev, int pos)
> > iov->nres = nres;
> > iov->ctrl = ctrl;
> > iov->total_VFs = total;
> > + iov->driver_max_VFs = total;
> > pci_read_config_word(dev, pos + PCI_SRIOV_VF_DID, &iov->vf_device);
> > iov->pgsz = pgsz;
> > iov->self = dev;
> > @@ -827,10 +828,7 @@ int pci_sriov_get_totalvfs(struct pci_dev *dev)
> > if (!dev->is_physfn)
> > return 0;
> > - if (dev->sriov->driver_max_VFs)
> > - return dev->sriov->driver_max_VFs;
> > -
> > - return dev->sriov->total_VFs;
> > + return dev->sriov->driver_max_VFs;
> > }
> > EXPORT_SYMBOL_GPL(pci_sriov_get_totalvfs);
> >
>
^ permalink raw reply [flat|nested] 23+ messages in thread* Re: [PATCH] PCI: allow drivers to limit the number of VFs to 0
2018-05-25 20:46 ` Bjorn Helgaas
@ 2018-05-29 14:29 ` Don Dutile
0 siblings, 0 replies; 23+ messages in thread
From: Don Dutile @ 2018-05-29 14:29 UTC (permalink / raw)
To: Bjorn Helgaas
Cc: Jakub Kicinski, Bjorn Helgaas, linux-pci, netdev, Sathya Perla,
Felix Manlunas, alexander.duyck, john.fastabend, Jacob Keller,
oss-drivers, Christoph Hellwig
On 05/25/2018 04:46 PM, Bjorn Helgaas wrote:
> On Fri, May 25, 2018 at 03:27:52PM -0400, Don Dutile wrote:
>> On 05/25/2018 10:02 AM, Bjorn Helgaas wrote:
>>> On Thu, May 24, 2018 at 06:20:15PM -0700, Jakub Kicinski wrote:
>>>> Hi Bjorn!
>>>>
>>>> On Thu, 24 May 2018 18:57:48 -0500, Bjorn Helgaas wrote:
>>>>> On Mon, Apr 02, 2018 at 03:46:52PM -0700, Jakub Kicinski wrote:
>>>>>> Some user space depends on enabling sriov_totalvfs number of VFs
>>>>>> to not fail, e.g.:
>>>>>>
>>>>>> $ cat .../sriov_totalvfs > .../sriov_numvfs
>>>>>>
>>>>>> For devices which VF support depends on loaded FW we have the
>>>>>> pci_sriov_{g,s}et_totalvfs() API. However, this API uses 0 as
>>>>>> a special "unset" value, meaning drivers can't limit sriov_totalvfs
>>>>>> to 0. Remove the special values completely and simply initialize
>>>>>> driver_max_VFs to total_VFs. Then always use driver_max_VFs.
>>>>>> Add a helper for drivers to reset the VF limit back to total.
>>>>>
>>>>> I still can't really make sense out of the changelog.
>>>>>
>>>>> I think part of the reason it's confusing is because there are two
>>>>> things going on:
>>>>>
>>>>> 1) You want this:
>>>>> pci_sriov_set_totalvfs(dev, 0);
>>>>> x = pci_sriov_get_totalvfs(dev)
>>>>>
>>>>> to return 0 instead of total_VFs. That seems to connect with
>>>>> your subject line. It means "sriov_totalvfs" in sysfs could be
>>>>> 0, but I don't know how that is useful (I'm sure it is; just
>>>>> educate me :))
>>>>
>>>> Let me just quote the bug report that got filed on our internal bug
>>>> tracker :)
>>>>
>>>> When testing Juju Openstack with Ubuntu 18.04, enabling SR-IOV causes
>>>> errors because Juju gets the sriov_totalvfs for SR-IOV-capable device
>>>> then tries to set that as the sriov_numvfs parameter.
>>>>
>>>> For SR-IOV incapable FW, the sriov_totalvfs parameter should be 0,
>>>> but it's set to max. When FW is switched to flower*, the correct
>>>> sriov_totalvfs value is presented.
>>>>
>>>> * flower is a project name
>>>
>>> From the point of view of the PCI core (which knows nothing about
>>> device firmware and relies on the architected config space described
>>> by the PCIe spec), this sounds like an erratum: with some firmware
>>> installed, the device is not capable of SR-IOV, but still advertises
>>> an SR-IOV capability with "TotalVFs > 0".
>>>
>>> Regardless of whether that's an erratum, we do allow PF drivers to use
>>> pci_sriov_set_totalvfs() to limit the number of VFs that may be
>>> enabled by writing to the PF's "sriov_numvfs" sysfs file.
>>>
>> +1.
>>
>>> But the current implementation does not allow a PF driver to limit VFs
>>> to 0, and that does seem nonsensical.
>>>
>> Well, not really -- claiming to support VFs, and then wanting it to be 0...
>> I could certainly argue is non-sensical.
>> From a sw perspective, sure, see if we can set VFs to 0 (and reset to another value later).
>>
>> /me wishes that implementers would follow the architecture vs torquing it into strange shapes.
>>
>>>> My understanding is OpenStack uses sriov_totalvfs to determine how many
>>>> VFs can be enabled, looks like this is the code:
>>>>
>>>> http://git.openstack.org/cgit/openstack/charm-neutron-openvswitch/tree/hooks/neutron_ovs_utils.py#n464
>>>>
>>>>> 2) You're adding the pci_sriov_reset_totalvfs() interface. I'm not
>>>>> sure what you intend for this. Is *every* driver supposed to
>>>>> call it in .remove()? Could/should this be done in the core
>>>>> somehow instead of depending on every driver?
>>>>
>>>> Good question, I was just thinking yesterday we may want to call it
>>>> from the core, but I don't think it's strictly necessary nor always
>>>> sufficient (we may reload FW without re-probing).
>>>>
>>>> We have a device which supports different number of VFs based on the FW
>>>> loaded. Some legacy FWs does not inform the driver how many VFs it can
>>>> support, because it supports max. So the flow in our driver is this:
>>>>
>>>> load_fw(dev);
>>>> ...
>>>> max_vfs = ask_fw_for_max_vfs(dev);
>>>> if (max_vfs >= 0)
>>>> return pci_sriov_set_totalvfs(dev, max_vfs);
>>>> else /* FW didn't tell us, assume max */
>>>> return pci_sriov_reset_totalvfs(dev);
>>>>
>>>> We also reset the max on device remove, but that's not strictly
>>>> necessary.
>>>>
>>>> Other users of pci_sriov_set_totalvfs() always know the value to set
>>>> the total to (either always get it from FW or it's a constant).
>>>>
>>>> If you prefer we can work out the correct max for those legacy cases in
>>>> the driver as well, although it seemed cleaner to just ask the core,
>>>> since it already has total_VFs value handy :)
>>>>
>>>>> I'm also having a hard time connecting your user-space command example
>>>>> with the rest of this. Maybe it will make more sense to me tomorrow
>>>>> after some coffee.
>>>>
>>>> OpenStack assumes it will always be able to set sriov_numvfs to
>>>> sriov_totalvfs, see this 'if':
>>>>
>>>> http://git.openstack.org/cgit/openstack/charm-neutron-openvswitch/tree/hooks/neutron_ovs_utils.py#n512
>>>
>>> Thanks for educating me. I think there are two issues here that we
>>> can separate. I extracted the patch below for the first.
>>>
>>> The second is the question of resetting driver_max_VFs. I think we
>>> currently have a general issue in the core:
>>>
>>> - load PF driver 1
>>> - driver calls pci_sriov_set_totalvfs() to reduce driver_max_VFs
>>> - unload PF driver 1
>>> - load PF driver 2
>>>
>>> Now driver_max_VFs is still stuck at the lower value set by driver 1.
>>> I don't think that's the way this should work.
>>>
>>> I guess this is partly a consequence of setting driver_max_VFs in
>>> sriov_init(), which is called before driver attach and should only
>> um, if it's at sriov_init() how is max changed by a PF driver?
>> or am I missing something subtle (a new sysfs param) as to what is being changed?
>
> sriov_init() basically just sets the default driver_max_VFs to Total_VFs.
>
> If the PF driver later calls pci_sriov_set_totalvfs(), it can reduce
> driver_max_VFs.
>
> My concern is that there's nothing that resets driver_max_VFs back to
> Total_VFs if we unload and reload the PF driver.
>
ok, gotcha.
any complication of this non-arch quirk. :-/
>>> depend on hardware characteristics, so it is related to the patch
>>> below. But I think we should fix it in general, not just for
>>> netronome.
>>>
>>>
>>> commit 4a338bc6f94b9ad824ac944f5dfc249d6838719c
>>> Author: Jakub Kicinski <jakub.kicinski@netronome.com>
>>> Date: Fri May 25 08:18:34 2018 -0500
>>>
>>> PCI/IOV: Allow PF drivers to limit total_VFs to 0
>>> Some SR-IOV PF drivers implement .sriov_configure(), which allows
>>> user-space to enable VFs by writing the desired number of VFs to the sysfs
>>> "sriov_numvfs" file (see sriov_numvfs_store()).
>>> The PCI core limits the number of VFs to the TotalVFs advertised by the
>>> device in its SR-IOV capability. The PF driver can limit the number of VFs
>>> to even fewer (it may have pre-allocated data structures or knowledge of
>>> device limitations) by calling pci_sriov_set_totalvfs(), but previously it
>>> could not limit the VFs to 0.
>>> Change pci_sriov_get_totalvfs() so it always respects the VF limit imposed
>>> by the PF driver, even if the limit is 0.
>>> This sequence:
>>> pci_sriov_set_totalvfs(dev, 0);
>>> x = pci_sriov_get_totalvfs(dev);
>>> previously set "x" to TotalVFs from the SR-IOV capability. Now it will set
>>> "x" to 0.
>>> Signed-off-by: Jakub Kicinski <jakub.kicinski@netronome.com>
>>> Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>
>>>
>>> diff --git a/drivers/pci/iov.c b/drivers/pci/iov.c
>>> index 192b82898a38..d0d73dbbd5ca 100644
>>> --- a/drivers/pci/iov.c
>>> +++ b/drivers/pci/iov.c
>>> @@ -469,6 +469,7 @@ static int sriov_init(struct pci_dev *dev, int pos)
>>> iov->nres = nres;
>>> iov->ctrl = ctrl;
>>> iov->total_VFs = total;
>>> + iov->driver_max_VFs = total;
>>> pci_read_config_word(dev, pos + PCI_SRIOV_VF_DID, &iov->vf_device);
>>> iov->pgsz = pgsz;
>>> iov->self = dev;
>>> @@ -827,10 +828,7 @@ int pci_sriov_get_totalvfs(struct pci_dev *dev)
>>> if (!dev->is_physfn)
>>> return 0;
>>> - if (dev->sriov->driver_max_VFs)
>>> - return dev->sriov->driver_max_VFs;
>>> -
>>> - return dev->sriov->total_VFs;
>>> + return dev->sriov->driver_max_VFs;
>>> }
>>> EXPORT_SYMBOL_GPL(pci_sriov_get_totalvfs);
>>>
>>
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH] PCI: allow drivers to limit the number of VFs to 0
2018-05-25 14:02 ` Bjorn Helgaas
2018-05-25 17:01 ` [Intel-wired-lan] " Bjorn Helgaas
2018-05-25 19:27 ` Don Dutile
@ 2018-05-25 21:05 ` Jakub Kicinski
2018-05-25 21:45 ` Bjorn Helgaas
2018-05-29 14:34 ` [PATCH] PCI: allow drivers to limit the number of VFs to 0 Don Dutile
2018-06-19 21:37 ` Bjorn Helgaas
3 siblings, 2 replies; 23+ messages in thread
From: Jakub Kicinski @ 2018-05-25 21:05 UTC (permalink / raw)
To: Bjorn Helgaas
Cc: Bjorn Helgaas, linux-pci, netdev, Sathya Perla, Felix Manlunas,
alexander.duyck, john.fastabend, Jacob Keller, Donald Dutile,
oss-drivers, Christoph Hellwig
On Fri, 25 May 2018 09:02:23 -0500, Bjorn Helgaas wrote:
> On Thu, May 24, 2018 at 06:20:15PM -0700, Jakub Kicinski wrote:
> > On Thu, 24 May 2018 18:57:48 -0500, Bjorn Helgaas wrote:
> > > On Mon, Apr 02, 2018 at 03:46:52PM -0700, Jakub Kicinski wrote:
> > > > Some user space depends on enabling sriov_totalvfs number of VFs
> > > > to not fail, e.g.:
> > > >
> > > > $ cat .../sriov_totalvfs > .../sriov_numvfs
> > > >
> > > > For devices which VF support depends on loaded FW we have the
> > > > pci_sriov_{g,s}et_totalvfs() API. However, this API uses 0 as
> > > > a special "unset" value, meaning drivers can't limit sriov_totalvfs
> > > > to 0. Remove the special values completely and simply initialize
> > > > driver_max_VFs to total_VFs. Then always use driver_max_VFs.
> > > > Add a helper for drivers to reset the VF limit back to total.
> > >
> > > I still can't really make sense out of the changelog.
> > >
> > > I think part of the reason it's confusing is because there are two
> > > things going on:
> > >
> > > 1) You want this:
> > >
> > > pci_sriov_set_totalvfs(dev, 0);
> > > x = pci_sriov_get_totalvfs(dev)
> > >
> > > to return 0 instead of total_VFs. That seems to connect with
> > > your subject line. It means "sriov_totalvfs" in sysfs could be
> > > 0, but I don't know how that is useful (I'm sure it is; just
> > > educate me :))
> >
> > Let me just quote the bug report that got filed on our internal bug
> > tracker :)
> >
> > When testing Juju Openstack with Ubuntu 18.04, enabling SR-IOV causes
> > errors because Juju gets the sriov_totalvfs for SR-IOV-capable device
> > then tries to set that as the sriov_numvfs parameter.
> >
> > For SR-IOV incapable FW, the sriov_totalvfs parameter should be 0,
> > but it's set to max. When FW is switched to flower*, the correct
> > sriov_totalvfs value is presented.
> >
> > * flower is a project name
>
> From the point of view of the PCI core (which knows nothing about
> device firmware and relies on the architected config space described
> by the PCIe spec), this sounds like an erratum: with some firmware
> installed, the device is not capable of SR-IOV, but still advertises
> an SR-IOV capability with "TotalVFs > 0".
>
> Regardless of whether that's an erratum, we do allow PF drivers to use
> pci_sriov_set_totalvfs() to limit the number of VFs that may be
> enabled by writing to the PF's "sriov_numvfs" sysfs file.
Think more of an FPGA which can be reprogrammed at runtime to have
different capabilities than an erratum. Some FWs simply have no use
for VFs and save resources (and validation time) by not supporting it.
> But the current implementation does not allow a PF driver to limit VFs
> to 0, and that does seem nonsensical.
>
> > My understanding is OpenStack uses sriov_totalvfs to determine how many
> > VFs can be enabled, looks like this is the code:
> >
> > http://git.openstack.org/cgit/openstack/charm-neutron-openvswitch/tree/hooks/neutron_ovs_utils.py#n464
> >
> > > 2) You're adding the pci_sriov_reset_totalvfs() interface. I'm not
> > > sure what you intend for this. Is *every* driver supposed to
> > > call it in .remove()? Could/should this be done in the core
> > > somehow instead of depending on every driver?
> >
> > Good question, I was just thinking yesterday we may want to call it
> > from the core, but I don't think it's strictly necessary nor always
> > sufficient (we may reload FW without re-probing).
> >
> > We have a device which supports different number of VFs based on the FW
> > loaded. Some legacy FWs does not inform the driver how many VFs it can
> > support, because it supports max. So the flow in our driver is this:
> >
> > load_fw(dev);
> > ...
> > max_vfs = ask_fw_for_max_vfs(dev);
> > if (max_vfs >= 0)
> > return pci_sriov_set_totalvfs(dev, max_vfs);
> > else /* FW didn't tell us, assume max */
> > return pci_sriov_reset_totalvfs(dev);
> >
> > We also reset the max on device remove, but that's not strictly
> > necessary.
> >
> > Other users of pci_sriov_set_totalvfs() always know the value to set
> > the total to (either always get it from FW or it's a constant).
> >
> > If you prefer we can work out the correct max for those legacy cases in
> > the driver as well, although it seemed cleaner to just ask the core,
> > since it already has total_VFs value handy :)
> >
> > > I'm also having a hard time connecting your user-space command example
> > > with the rest of this. Maybe it will make more sense to me tomorrow
> > > after some coffee.
> >
> > OpenStack assumes it will always be able to set sriov_numvfs to
> > sriov_totalvfs, see this 'if':
> >
> > http://git.openstack.org/cgit/openstack/charm-neutron-openvswitch/tree/hooks/neutron_ovs_utils.py#n512
>
> Thanks for educating me. I think there are two issues here that we
> can separate. I extracted the patch below for the first.
>
> The second is the question of resetting driver_max_VFs. I think we
> currently have a general issue in the core:
>
> - load PF driver 1
> - driver calls pci_sriov_set_totalvfs() to reduce driver_max_VFs
> - unload PF driver 1
> - load PF driver 2
>
> Now driver_max_VFs is still stuck at the lower value set by driver 1.
> I don't think that's the way this should work.
>
> I guess this is partly a consequence of setting driver_max_VFs in
> sriov_init(), which is called before driver attach and should only
> depend on hardware characteristics, so it is related to the patch
> below. But I think we should fix it in general, not just for
> netronome.
Okay, perfect. That makes sense. The patch below certainly fixes the
first issue for us. Thank you!
As far as the second issue goes - agreed, having the core reset the
number of VFs to total_VFs definitely makes sense. It doesn't cater to
the case where FW is reloaded without reprobing, but we don't do this
today anyway.
Should I try to come up with a patch to reset total_VFs after detach?
> commit 4a338bc6f94b9ad824ac944f5dfc249d6838719c
> Author: Jakub Kicinski <jakub.kicinski@netronome.com>
> Date: Fri May 25 08:18:34 2018 -0500
>
> PCI/IOV: Allow PF drivers to limit total_VFs to 0
>
> Some SR-IOV PF drivers implement .sriov_configure(), which allows
> user-space to enable VFs by writing the desired number of VFs to the sysfs
> "sriov_numvfs" file (see sriov_numvfs_store()).
>
> The PCI core limits the number of VFs to the TotalVFs advertised by the
> device in its SR-IOV capability. The PF driver can limit the number of VFs
> to even fewer (it may have pre-allocated data structures or knowledge of
> device limitations) by calling pci_sriov_set_totalvfs(), but previously it
> could not limit the VFs to 0.
>
> Change pci_sriov_get_totalvfs() so it always respects the VF limit imposed
> by the PF driver, even if the limit is 0.
>
> This sequence:
>
> pci_sriov_set_totalvfs(dev, 0);
> x = pci_sriov_get_totalvfs(dev);
>
> previously set "x" to TotalVFs from the SR-IOV capability. Now it will set
> "x" to 0.
>
> Signed-off-by: Jakub Kicinski <jakub.kicinski@netronome.com>
> Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>
>
> diff --git a/drivers/pci/iov.c b/drivers/pci/iov.c
> index 192b82898a38..d0d73dbbd5ca 100644
> --- a/drivers/pci/iov.c
> +++ b/drivers/pci/iov.c
> @@ -469,6 +469,7 @@ static int sriov_init(struct pci_dev *dev, int pos)
> iov->nres = nres;
> iov->ctrl = ctrl;
> iov->total_VFs = total;
> + iov->driver_max_VFs = total;
> pci_read_config_word(dev, pos + PCI_SRIOV_VF_DID, &iov->vf_device);
> iov->pgsz = pgsz;
> iov->self = dev;
> @@ -827,10 +828,7 @@ int pci_sriov_get_totalvfs(struct pci_dev *dev)
> if (!dev->is_physfn)
> return 0;
>
> - if (dev->sriov->driver_max_VFs)
> - return dev->sriov->driver_max_VFs;
> -
> - return dev->sriov->total_VFs;
> + return dev->sriov->driver_max_VFs;
> }
> EXPORT_SYMBOL_GPL(pci_sriov_get_totalvfs);
>
^ permalink raw reply [flat|nested] 23+ messages in thread* Re: [PATCH] PCI: allow drivers to limit the number of VFs to 0
2018-05-25 21:05 ` Jakub Kicinski
@ 2018-05-25 21:45 ` Bjorn Helgaas
2018-05-26 3:00 ` [PATCH] PCI: reset driver SR-IOV state after remove Jakub Kicinski
2018-05-29 14:34 ` [PATCH] PCI: allow drivers to limit the number of VFs to 0 Don Dutile
1 sibling, 1 reply; 23+ messages in thread
From: Bjorn Helgaas @ 2018-05-25 21:45 UTC (permalink / raw)
To: Jakub Kicinski
Cc: Bjorn Helgaas, linux-pci, netdev, Sathya Perla, Felix Manlunas,
alexander.duyck, john.fastabend, Jacob Keller, Donald Dutile,
oss-drivers, Christoph Hellwig
On Fri, May 25, 2018 at 02:05:21PM -0700, Jakub Kicinski wrote:
> On Fri, 25 May 2018 09:02:23 -0500, Bjorn Helgaas wrote:
> > On Thu, May 24, 2018 at 06:20:15PM -0700, Jakub Kicinski wrote:
> > > On Thu, 24 May 2018 18:57:48 -0500, Bjorn Helgaas wrote:
> > > > On Mon, Apr 02, 2018 at 03:46:52PM -0700, Jakub Kicinski wrote:
> > > > > Some user space depends on enabling sriov_totalvfs number of VFs
> > > > > to not fail, e.g.:
> > > > >
> > > > > $ cat .../sriov_totalvfs > .../sriov_numvfs
> > > > >
> > > > > For devices which VF support depends on loaded FW we have the
> > > > > pci_sriov_{g,s}et_totalvfs() API. However, this API uses 0 as
> > > > > a special "unset" value, meaning drivers can't limit sriov_totalvfs
> > > > > to 0. Remove the special values completely and simply initialize
> > > > > driver_max_VFs to total_VFs. Then always use driver_max_VFs.
> > > > > Add a helper for drivers to reset the VF limit back to total.
> > > >
> > > > I still can't really make sense out of the changelog.
> > > >
> > > > I think part of the reason it's confusing is because there are two
> > > > things going on:
> > > >
> > > > 1) You want this:
> > > >
> > > > pci_sriov_set_totalvfs(dev, 0);
> > > > x = pci_sriov_get_totalvfs(dev)
> > > >
> > > > to return 0 instead of total_VFs. That seems to connect with
> > > > your subject line. It means "sriov_totalvfs" in sysfs could be
> > > > 0, but I don't know how that is useful (I'm sure it is; just
> > > > educate me :))
> > >
> > > Let me just quote the bug report that got filed on our internal bug
> > > tracker :)
> > >
> > > When testing Juju Openstack with Ubuntu 18.04, enabling SR-IOV causes
> > > errors because Juju gets the sriov_totalvfs for SR-IOV-capable device
> > > then tries to set that as the sriov_numvfs parameter.
> > >
> > > For SR-IOV incapable FW, the sriov_totalvfs parameter should be 0,
> > > but it's set to max. When FW is switched to flower*, the correct
> > > sriov_totalvfs value is presented.
> > >
> > > * flower is a project name
> >
> > From the point of view of the PCI core (which knows nothing about
> > device firmware and relies on the architected config space described
> > by the PCIe spec), this sounds like an erratum: with some firmware
> > installed, the device is not capable of SR-IOV, but still advertises
> > an SR-IOV capability with "TotalVFs > 0".
> >
> > Regardless of whether that's an erratum, we do allow PF drivers to use
> > pci_sriov_set_totalvfs() to limit the number of VFs that may be
> > enabled by writing to the PF's "sriov_numvfs" sysfs file.
>
> Think more of an FPGA which can be reprogrammed at runtime to have
> different capabilities than an erratum. Some FWs simply have no use
> for VFs and save resources (and validation time) by not supporting it.
This is a bit of a gray area. Reloading firmware or reprogramming an
FPGA has the potential to create a new and different device than we
had before, but the PCI core doesn't know that. The typical sequence
is:
- PCI core enumerates device
- driver binds to device (we call .probe())
- driver loads new firmware to device
- driver resets device with pci_reset_function() or similar
- pci_reset_function() saves config space
- pci_reset_function() resets device
- device uses new firmware when it comes out of reset
- pci_reset_function() restores config space
Loading the new firmware might change what the device looks like in
config space -- it could change the number or size of BARs, the
capabilities advertised, etc. We currently sweep that under the rug
and blindly restore the old config space.
It looks like your driver does the reset differently, so maybe it
keeps the original config space setup.
But all that said, I agree that we should allow a PF driver to prevent
VF enablement, whether because the firmware doesn't support it or the
PF driver just wants to prevent use of VFs for whatever reason (maybe
we don't have enough MMIO resources, we don't need the VFs, etc.)
> Okay, perfect. That makes sense. The patch below certainly fixes the
> first issue for us. Thank you!
>
> As far as the second issue goes - agreed, having the core reset the
> number of VFs to total_VFs definitely makes sense. It doesn't cater to
> the case where FW is reloaded without reprobing, but we don't do this
> today anyway.
>
> Should I try to come up with a patch to reset total_VFs after detach?
Yes, please.
Bjorn
^ permalink raw reply [flat|nested] 23+ messages in thread* [PATCH] PCI: reset driver SR-IOV state after remove
2018-05-25 21:45 ` Bjorn Helgaas
@ 2018-05-26 3:00 ` Jakub Kicinski
0 siblings, 0 replies; 23+ messages in thread
From: Jakub Kicinski @ 2018-05-26 3:00 UTC (permalink / raw)
To: Bjorn Helgaas
Cc: linux-pci, netdev, Sathya Perla, Felix Manlunas, alexander.duyck,
Jacob Keller, Donald Dutile, oss-drivers, Christoph Hellwig,
Jakub Kicinski
Bjorn points out that currently core and most of the drivers don't
clean up dev->sriov->driver_max_VFs settings on .remove(). This
means that if a different driver is bound afterwards it will
inherit the old setting:
- load PF driver 1
- driver calls pci_sriov_set_totalvfs() to reduce driver_max_VFs
- unload PF driver 1
- load PF driver 2
Reset driver_max_VFs back to total_VFs after device remove.
Signed-off-by: Jakub Kicinski <jakub.kicinski@netronome.com>
---
I gave into the temptation and also added a warning about SR-IOV
being on after remove :) Please let me know if this is anywhere
close to what you had in mind!
drivers/pci/iov.c | 16 ++++++++++++++++
drivers/pci/pci-driver.c | 1 +
drivers/pci/pci.h | 4 ++++
3 files changed, 21 insertions(+)
diff --git a/drivers/pci/iov.c b/drivers/pci/iov.c
index db86fd26f8e1..5d0f560a1e28 100644
--- a/drivers/pci/iov.c
+++ b/drivers/pci/iov.c
@@ -574,6 +574,22 @@ void pci_iov_release(struct pci_dev *dev)
sriov_release(dev);
}
+/**
+ * pci_sriov_drv_cleanup - clean up SR-IOV state after PF driver is detached
+ * @dev: the PCI device
+ */
+void pci_sriov_drv_cleanup(struct pci_dev *dev)
+{
+ struct pci_sriov *iov = dev->sriov;
+
+ if (!dev->is_physfn)
+ return;
+ iov->driver_max_VFs = iov->total_VFs;
+ if (iov->num_VFs)
+ dev_warn(&dev->dev,
+ "driver left SR-IOV enabled after remove\n");
+}
+
/**
* pci_iov_update_resource - update a VF BAR
* @dev: the PCI device
diff --git a/drivers/pci/pci-driver.c b/drivers/pci/pci-driver.c
index b9a131137e64..932a1acf7b1b 100644
--- a/drivers/pci/pci-driver.c
+++ b/drivers/pci/pci-driver.c
@@ -443,6 +443,7 @@ static int pci_device_remove(struct device *dev)
}
pcibios_free_irq(pci_dev);
pci_dev->driver = NULL;
+ pci_sriov_drv_cleanup(pci_dev);
}
/* Undo the runtime PM settings in local_pci_probe() */
diff --git a/drivers/pci/pci.h b/drivers/pci/pci.h
index 023f7cf25bff..5fa6d19762bd 100644
--- a/drivers/pci/pci.h
+++ b/drivers/pci/pci.h
@@ -311,6 +311,7 @@ static inline void pci_restore_ats_state(struct pci_dev *dev)
#ifdef CONFIG_PCI_IOV
int pci_iov_init(struct pci_dev *dev);
void pci_iov_release(struct pci_dev *dev);
+void pci_sriov_drv_cleanup(struct pci_dev *dev);
void pci_iov_update_resource(struct pci_dev *dev, int resno);
resource_size_t pci_sriov_resource_alignment(struct pci_dev *dev, int resno);
void pci_restore_iov_state(struct pci_dev *dev);
@@ -323,6 +324,9 @@ static inline int pci_iov_init(struct pci_dev *dev)
}
static inline void pci_iov_release(struct pci_dev *dev)
+{
+}
+static inline void pci_sriov_drv_cleanup(struct pci_dev *dev)
{
}
static inline void pci_restore_iov_state(struct pci_dev *dev)
--
2.17.0
^ permalink raw reply related [flat|nested] 23+ messages in thread
* Re: [PATCH] PCI: allow drivers to limit the number of VFs to 0
2018-05-25 21:05 ` Jakub Kicinski
2018-05-25 21:45 ` Bjorn Helgaas
@ 2018-05-29 14:34 ` Don Dutile
1 sibling, 0 replies; 23+ messages in thread
From: Don Dutile @ 2018-05-29 14:34 UTC (permalink / raw)
To: Jakub Kicinski, Bjorn Helgaas
Cc: Bjorn Helgaas, linux-pci, netdev, Sathya Perla, Felix Manlunas,
alexander.duyck, john.fastabend, Jacob Keller, oss-drivers,
Christoph Hellwig
On 05/25/2018 05:05 PM, Jakub Kicinski wrote:
> On Fri, 25 May 2018 09:02:23 -0500, Bjorn Helgaas wrote:
>> On Thu, May 24, 2018 at 06:20:15PM -0700, Jakub Kicinski wrote:
>>> On Thu, 24 May 2018 18:57:48 -0500, Bjorn Helgaas wrote:
>>>> On Mon, Apr 02, 2018 at 03:46:52PM -0700, Jakub Kicinski wrote:
>>>>> Some user space depends on enabling sriov_totalvfs number of VFs
>>>>> to not fail, e.g.:
>>>>>
>>>>> $ cat .../sriov_totalvfs > .../sriov_numvfs
>>>>>
>>>>> For devices which VF support depends on loaded FW we have the
>>>>> pci_sriov_{g,s}et_totalvfs() API. However, this API uses 0 as
>>>>> a special "unset" value, meaning drivers can't limit sriov_totalvfs
>>>>> to 0. Remove the special values completely and simply initialize
>>>>> driver_max_VFs to total_VFs. Then always use driver_max_VFs.
>>>>> Add a helper for drivers to reset the VF limit back to total.
>>>>
>>>> I still can't really make sense out of the changelog.
>>>>
>>>> I think part of the reason it's confusing is because there are two
>>>> things going on:
>>>>
>>>> 1) You want this:
>>>>
>>>> pci_sriov_set_totalvfs(dev, 0);
>>>> x = pci_sriov_get_totalvfs(dev)
>>>>
>>>> to return 0 instead of total_VFs. That seems to connect with
>>>> your subject line. It means "sriov_totalvfs" in sysfs could be
>>>> 0, but I don't know how that is useful (I'm sure it is; just
>>>> educate me :))
>>>
>>> Let me just quote the bug report that got filed on our internal bug
>>> tracker :)
>>>
>>> When testing Juju Openstack with Ubuntu 18.04, enabling SR-IOV causes
>>> errors because Juju gets the sriov_totalvfs for SR-IOV-capable device
>>> then tries to set that as the sriov_numvfs parameter.
>>>
>>> For SR-IOV incapable FW, the sriov_totalvfs parameter should be 0,
>>> but it's set to max. When FW is switched to flower*, the correct
>>> sriov_totalvfs value is presented.
>>>
>>> * flower is a project name
>>
>> From the point of view of the PCI core (which knows nothing about
>> device firmware and relies on the architected config space described
>> by the PCIe spec), this sounds like an erratum: with some firmware
>> installed, the device is not capable of SR-IOV, but still advertises
>> an SR-IOV capability with "TotalVFs > 0".
>>
>> Regardless of whether that's an erratum, we do allow PF drivers to use
>> pci_sriov_set_totalvfs() to limit the number of VFs that may be
>> enabled by writing to the PF's "sriov_numvfs" sysfs file.
>
> Think more of an FPGA which can be reprogrammed at runtime to have
> different capabilities than an erratum. Some FWs simply have no use
> for VFs and save resources (and validation time) by not supporting it.
>
Sure, then the steps should be:
a) (re-)program FPGA
b) invoke hot-plug for new device.
-- by default, VFs aren't configured(enabled) in a Linux kernel;
-- some drivers provide boot-time enablement, but that becomes
system-wide, and can cause major config issues when multiples of a device are installed in the system.
-- otherwise, configure via sysfs
-- this should clear/reset the VF values too.
>> But the current implementation does not allow a PF driver to limit VFs
>> to 0, and that does seem nonsensical.
>>
>>> My understanding is OpenStack uses sriov_totalvfs to determine how many
>>> VFs can be enabled, looks like this is the code:
>>>
>>> http://git.openstack.org/cgit/openstack/charm-neutron-openvswitch/tree/hooks/neutron_ovs_utils.py#n464
>>>
>>>> 2) You're adding the pci_sriov_reset_totalvfs() interface. I'm not
>>>> sure what you intend for this. Is *every* driver supposed to
>>>> call it in .remove()? Could/should this be done in the core
>>>> somehow instead of depending on every driver?
>>>
>>> Good question, I was just thinking yesterday we may want to call it
>>> from the core, but I don't think it's strictly necessary nor always
>>> sufficient (we may reload FW without re-probing).
>>>
>>> We have a device which supports different number of VFs based on the FW
>>> loaded. Some legacy FWs does not inform the driver how many VFs it can
>>> support, because it supports max. So the flow in our driver is this:
>>>
>>> load_fw(dev);
>>> ...
>>> max_vfs = ask_fw_for_max_vfs(dev);
>>> if (max_vfs >= 0)
>>> return pci_sriov_set_totalvfs(dev, max_vfs);
>>> else /* FW didn't tell us, assume max */
>>> return pci_sriov_reset_totalvfs(dev);
>>>
>>> We also reset the max on device remove, but that's not strictly
>>> necessary.
>>>
>>> Other users of pci_sriov_set_totalvfs() always know the value to set
>>> the total to (either always get it from FW or it's a constant).
>>>
>>> If you prefer we can work out the correct max for those legacy cases in
>>> the driver as well, although it seemed cleaner to just ask the core,
>>> since it already has total_VFs value handy :)
>>>
>>>> I'm also having a hard time connecting your user-space command example
>>>> with the rest of this. Maybe it will make more sense to me tomorrow
>>>> after some coffee.
>>>
>>> OpenStack assumes it will always be able to set sriov_numvfs to
>>> sriov_totalvfs, see this 'if':
>>>
>>> http://git.openstack.org/cgit/openstack/charm-neutron-openvswitch/tree/hooks/neutron_ovs_utils.py#n512
>>
>> Thanks for educating me. I think there are two issues here that we
>> can separate. I extracted the patch below for the first.
>>
>> The second is the question of resetting driver_max_VFs. I think we
>> currently have a general issue in the core:
>>
>> - load PF driver 1
>> - driver calls pci_sriov_set_totalvfs() to reduce driver_max_VFs
>> - unload PF driver 1
>> - load PF driver 2
>>
>> Now driver_max_VFs is still stuck at the lower value set by driver 1.
>> I don't think that's the way this should work.
>>
>> I guess this is partly a consequence of setting driver_max_VFs in
>> sriov_init(), which is called before driver attach and should only
>> depend on hardware characteristics, so it is related to the patch
>> below. But I think we should fix it in general, not just for
>> netronome.
>
> Okay, perfect. That makes sense. The patch below certainly fixes the
> first issue for us. Thank you!
>
> As far as the second issue goes - agreed, having the core reset the
> number of VFs to total_VFs definitely makes sense. It doesn't cater to
> the case where FW is reloaded without reprobing, but we don't do this
> today anyway.
>
> Should I try to come up with a patch to reset total_VFs after detach?
>
>> commit 4a338bc6f94b9ad824ac944f5dfc249d6838719c
>> Author: Jakub Kicinski <jakub.kicinski@netronome.com>
>> Date: Fri May 25 08:18:34 2018 -0500
>>
>> PCI/IOV: Allow PF drivers to limit total_VFs to 0
>>
>> Some SR-IOV PF drivers implement .sriov_configure(), which allows
>> user-space to enable VFs by writing the desired number of VFs to the sysfs
>> "sriov_numvfs" file (see sriov_numvfs_store()).
>>
>> The PCI core limits the number of VFs to the TotalVFs advertised by the
>> device in its SR-IOV capability. The PF driver can limit the number of VFs
>> to even fewer (it may have pre-allocated data structures or knowledge of
>> device limitations) by calling pci_sriov_set_totalvfs(), but previously it
>> could not limit the VFs to 0.
>>
>> Change pci_sriov_get_totalvfs() so it always respects the VF limit imposed
>> by the PF driver, even if the limit is 0.
>>
>> This sequence:
>>
>> pci_sriov_set_totalvfs(dev, 0);
>> x = pci_sriov_get_totalvfs(dev);
>>
>> previously set "x" to TotalVFs from the SR-IOV capability. Now it will set
>> "x" to 0.
>>
>> Signed-off-by: Jakub Kicinski <jakub.kicinski@netronome.com>
>> Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>
>>
>> diff --git a/drivers/pci/iov.c b/drivers/pci/iov.c
>> index 192b82898a38..d0d73dbbd5ca 100644
>> --- a/drivers/pci/iov.c
>> +++ b/drivers/pci/iov.c
>> @@ -469,6 +469,7 @@ static int sriov_init(struct pci_dev *dev, int pos)
>> iov->nres = nres;
>> iov->ctrl = ctrl;
>> iov->total_VFs = total;
>> + iov->driver_max_VFs = total;
>> pci_read_config_word(dev, pos + PCI_SRIOV_VF_DID, &iov->vf_device);
>> iov->pgsz = pgsz;
>> iov->self = dev;
>> @@ -827,10 +828,7 @@ int pci_sriov_get_totalvfs(struct pci_dev *dev)
>> if (!dev->is_physfn)
>> return 0;
>>
>> - if (dev->sriov->driver_max_VFs)
>> - return dev->sriov->driver_max_VFs;
>> -
>> - return dev->sriov->total_VFs;
>> + return dev->sriov->driver_max_VFs;
>> }
>> EXPORT_SYMBOL_GPL(pci_sriov_get_totalvfs);
>>
>
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH] PCI: allow drivers to limit the number of VFs to 0
2018-05-25 14:02 ` Bjorn Helgaas
` (2 preceding siblings ...)
2018-05-25 21:05 ` Jakub Kicinski
@ 2018-06-19 21:37 ` Bjorn Helgaas
2018-06-20 2:56 ` Jakub Kicinski
3 siblings, 1 reply; 23+ messages in thread
From: Bjorn Helgaas @ 2018-06-19 21:37 UTC (permalink / raw)
To: Jakub Kicinski
Cc: Bjorn Helgaas, linux-pci, netdev, Sathya Perla, Felix Manlunas,
alexander.duyck, john.fastabend, Jacob Keller, Donald Dutile,
oss-drivers, Christoph Hellwig
On Fri, May 25, 2018 at 09:02:23AM -0500, Bjorn Helgaas wrote:
> On Thu, May 24, 2018 at 06:20:15PM -0700, Jakub Kicinski wrote:
> > Hi Bjorn!
> >
> > On Thu, 24 May 2018 18:57:48 -0500, Bjorn Helgaas wrote:
> > > On Mon, Apr 02, 2018 at 03:46:52PM -0700, Jakub Kicinski wrote:
> > > > Some user space depends on enabling sriov_totalvfs number of VFs
> > > > to not fail, e.g.:
> > > >
> > > > $ cat .../sriov_totalvfs > .../sriov_numvfs
> > > >
> > > > For devices which VF support depends on loaded FW we have the
> > > > pci_sriov_{g,s}et_totalvfs() API. However, this API uses 0 as
> > > > a special "unset" value, meaning drivers can't limit sriov_totalvfs
> > > > to 0. Remove the special values completely and simply initialize
> > > > driver_max_VFs to total_VFs. Then always use driver_max_VFs.
> > > > Add a helper for drivers to reset the VF limit back to total.
> > >
> > > I still can't really make sense out of the changelog.
> > >
> > > I think part of the reason it's confusing is because there are two
> > > things going on:
> > >
> > > 1) You want this:
> > >
> > > pci_sriov_set_totalvfs(dev, 0);
> > > x = pci_sriov_get_totalvfs(dev)
> > >
> > > to return 0 instead of total_VFs. That seems to connect with
> > > your subject line. It means "sriov_totalvfs" in sysfs could be
> > > 0, but I don't know how that is useful (I'm sure it is; just
> > > educate me :))
> >
> > Let me just quote the bug report that got filed on our internal bug
> > tracker :)
> >
> > When testing Juju Openstack with Ubuntu 18.04, enabling SR-IOV causes
> > errors because Juju gets the sriov_totalvfs for SR-IOV-capable device
> > then tries to set that as the sriov_numvfs parameter.
> >
> > For SR-IOV incapable FW, the sriov_totalvfs parameter should be 0,
> > but it's set to max. When FW is switched to flower*, the correct
> > sriov_totalvfs value is presented.
> >
> > * flower is a project name
>
> From the point of view of the PCI core (which knows nothing about
> device firmware and relies on the architected config space described
> by the PCIe spec), this sounds like an erratum: with some firmware
> installed, the device is not capable of SR-IOV, but still advertises
> an SR-IOV capability with "TotalVFs > 0".
>
> Regardless of whether that's an erratum, we do allow PF drivers to use
> pci_sriov_set_totalvfs() to limit the number of VFs that may be
> enabled by writing to the PF's "sriov_numvfs" sysfs file.
>
> But the current implementation does not allow a PF driver to limit VFs
> to 0, and that does seem nonsensical.
>
> > My understanding is OpenStack uses sriov_totalvfs to determine how many
> > VFs can be enabled, looks like this is the code:
> >
> > http://git.openstack.org/cgit/openstack/charm-neutron-openvswitch/tree/hooks/neutron_ovs_utils.py#n464
> >
> > > 2) You're adding the pci_sriov_reset_totalvfs() interface. I'm not
> > > sure what you intend for this. Is *every* driver supposed to
> > > call it in .remove()? Could/should this be done in the core
> > > somehow instead of depending on every driver?
> >
> > Good question, I was just thinking yesterday we may want to call it
> > from the core, but I don't think it's strictly necessary nor always
> > sufficient (we may reload FW without re-probing).
> >
> > We have a device which supports different number of VFs based on the FW
> > loaded. Some legacy FWs does not inform the driver how many VFs it can
> > support, because it supports max. So the flow in our driver is this:
> >
> > load_fw(dev);
> > ...
> > max_vfs = ask_fw_for_max_vfs(dev);
> > if (max_vfs >= 0)
> > return pci_sriov_set_totalvfs(dev, max_vfs);
> > else /* FW didn't tell us, assume max */
> > return pci_sriov_reset_totalvfs(dev);
> >
> > We also reset the max on device remove, but that's not strictly
> > necessary.
> >
> > Other users of pci_sriov_set_totalvfs() always know the value to set
> > the total to (either always get it from FW or it's a constant).
> >
> > If you prefer we can work out the correct max for those legacy cases in
> > the driver as well, although it seemed cleaner to just ask the core,
> > since it already has total_VFs value handy :)
> >
> > > I'm also having a hard time connecting your user-space command example
> > > with the rest of this. Maybe it will make more sense to me tomorrow
> > > after some coffee.
> >
> > OpenStack assumes it will always be able to set sriov_numvfs to
> > sriov_totalvfs, see this 'if':
> >
> > http://git.openstack.org/cgit/openstack/charm-neutron-openvswitch/tree/hooks/neutron_ovs_utils.py#n512
>
> Thanks for educating me. I think there are two issues here that we
> can separate. I extracted the patch below for the first.
>
> The second is the question of resetting driver_max_VFs. I think we
> currently have a general issue in the core:
>
> - load PF driver 1
> - driver calls pci_sriov_set_totalvfs() to reduce driver_max_VFs
> - unload PF driver 1
> - load PF driver 2
>
> Now driver_max_VFs is still stuck at the lower value set by driver 1.
> I don't think that's the way this should work.
>
> I guess this is partly a consequence of setting driver_max_VFs in
> sriov_init(), which is called before driver attach and should only
> depend on hardware characteristics, so it is related to the patch
> below. But I think we should fix it in general, not just for
> netronome.
Hi Jakub, the patch below is in v4.18-rc1 as 8d85a7a4f2c9 ("PCI/IOV:
Allow PF drivers to limit total_VFs to 0"). If there's more we need
to do here, would you mind rebasing what's left to v4.18-rc1 and
reposting it?
> commit 4a338bc6f94b9ad824ac944f5dfc249d6838719c
> Author: Jakub Kicinski <jakub.kicinski@netronome.com>
> Date: Fri May 25 08:18:34 2018 -0500
>
> PCI/IOV: Allow PF drivers to limit total_VFs to 0
>
> Some SR-IOV PF drivers implement .sriov_configure(), which allows
> user-space to enable VFs by writing the desired number of VFs to the sysfs
> "sriov_numvfs" file (see sriov_numvfs_store()).
>
> The PCI core limits the number of VFs to the TotalVFs advertised by the
> device in its SR-IOV capability. The PF driver can limit the number of VFs
> to even fewer (it may have pre-allocated data structures or knowledge of
> device limitations) by calling pci_sriov_set_totalvfs(), but previously it
> could not limit the VFs to 0.
>
> Change pci_sriov_get_totalvfs() so it always respects the VF limit imposed
> by the PF driver, even if the limit is 0.
>
> This sequence:
>
> pci_sriov_set_totalvfs(dev, 0);
> x = pci_sriov_get_totalvfs(dev);
>
> previously set "x" to TotalVFs from the SR-IOV capability. Now it will set
> "x" to 0.
>
> Signed-off-by: Jakub Kicinski <jakub.kicinski@netronome.com>
> Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>
>
> diff --git a/drivers/pci/iov.c b/drivers/pci/iov.c
> index 192b82898a38..d0d73dbbd5ca 100644
> --- a/drivers/pci/iov.c
> +++ b/drivers/pci/iov.c
> @@ -469,6 +469,7 @@ static int sriov_init(struct pci_dev *dev, int pos)
> iov->nres = nres;
> iov->ctrl = ctrl;
> iov->total_VFs = total;
> + iov->driver_max_VFs = total;
> pci_read_config_word(dev, pos + PCI_SRIOV_VF_DID, &iov->vf_device);
> iov->pgsz = pgsz;
> iov->self = dev;
> @@ -827,10 +828,7 @@ int pci_sriov_get_totalvfs(struct pci_dev *dev)
> if (!dev->is_physfn)
> return 0;
>
> - if (dev->sriov->driver_max_VFs)
> - return dev->sriov->driver_max_VFs;
> -
> - return dev->sriov->total_VFs;
> + return dev->sriov->driver_max_VFs;
> }
> EXPORT_SYMBOL_GPL(pci_sriov_get_totalvfs);
>
^ permalink raw reply [flat|nested] 23+ messages in thread* Re: [PATCH] PCI: allow drivers to limit the number of VFs to 0
2018-06-19 21:37 ` Bjorn Helgaas
@ 2018-06-20 2:56 ` Jakub Kicinski
0 siblings, 0 replies; 23+ messages in thread
From: Jakub Kicinski @ 2018-06-20 2:56 UTC (permalink / raw)
To: Bjorn Helgaas
Cc: Bjorn Helgaas, linux-pci, netdev, Sathya Perla, Felix Manlunas,
alexander.duyck, john.fastabend, Jacob Keller, Donald Dutile,
oss-drivers, Christoph Hellwig
On Tue, 19 Jun 2018 16:37:15 -0500, Bjorn Helgaas wrote:
> On Fri, May 25, 2018 at 09:02:23AM -0500, Bjorn Helgaas wrote:
> > On Thu, May 24, 2018 at 06:20:15PM -0700, Jakub Kicinski wrote:
> > > Hi Bjorn!
> > >
> > > On Thu, 24 May 2018 18:57:48 -0500, Bjorn Helgaas wrote:
> > > > On Mon, Apr 02, 2018 at 03:46:52PM -0700, Jakub Kicinski wrote:
> > > > > Some user space depends on enabling sriov_totalvfs number of VFs
> > > > > to not fail, e.g.:
> > > > >
> > > > > $ cat .../sriov_totalvfs > .../sriov_numvfs
> > > > >
> > > > > For devices which VF support depends on loaded FW we have the
> > > > > pci_sriov_{g,s}et_totalvfs() API. However, this API uses 0 as
> > > > > a special "unset" value, meaning drivers can't limit sriov_totalvfs
> > > > > to 0. Remove the special values completely and simply initialize
> > > > > driver_max_VFs to total_VFs. Then always use driver_max_VFs.
> > > > > Add a helper for drivers to reset the VF limit back to total.
> > > >
> > > > I still can't really make sense out of the changelog.
> > > >
> > > > I think part of the reason it's confusing is because there are two
> > > > things going on:
> > > >
> > > > 1) You want this:
> > > >
> > > > pci_sriov_set_totalvfs(dev, 0);
> > > > x = pci_sriov_get_totalvfs(dev)
> > > >
> > > > to return 0 instead of total_VFs. That seems to connect with
> > > > your subject line. It means "sriov_totalvfs" in sysfs could be
> > > > 0, but I don't know how that is useful (I'm sure it is; just
> > > > educate me :))
> > >
> > > Let me just quote the bug report that got filed on our internal bug
> > > tracker :)
> > >
> > > When testing Juju Openstack with Ubuntu 18.04, enabling SR-IOV causes
> > > errors because Juju gets the sriov_totalvfs for SR-IOV-capable device
> > > then tries to set that as the sriov_numvfs parameter.
> > >
> > > For SR-IOV incapable FW, the sriov_totalvfs parameter should be 0,
> > > but it's set to max. When FW is switched to flower*, the correct
> > > sriov_totalvfs value is presented.
> > >
> > > * flower is a project name
> >
> > From the point of view of the PCI core (which knows nothing about
> > device firmware and relies on the architected config space described
> > by the PCIe spec), this sounds like an erratum: with some firmware
> > installed, the device is not capable of SR-IOV, but still advertises
> > an SR-IOV capability with "TotalVFs > 0".
> >
> > Regardless of whether that's an erratum, we do allow PF drivers to use
> > pci_sriov_set_totalvfs() to limit the number of VFs that may be
> > enabled by writing to the PF's "sriov_numvfs" sysfs file.
> >
> > But the current implementation does not allow a PF driver to limit VFs
> > to 0, and that does seem nonsensical.
> >
> > > My understanding is OpenStack uses sriov_totalvfs to determine how many
> > > VFs can be enabled, looks like this is the code:
> > >
> > > http://git.openstack.org/cgit/openstack/charm-neutron-openvswitch/tree/hooks/neutron_ovs_utils.py#n464
> > >
> > > > 2) You're adding the pci_sriov_reset_totalvfs() interface. I'm not
> > > > sure what you intend for this. Is *every* driver supposed to
> > > > call it in .remove()? Could/should this be done in the core
> > > > somehow instead of depending on every driver?
> > >
> > > Good question, I was just thinking yesterday we may want to call it
> > > from the core, but I don't think it's strictly necessary nor always
> > > sufficient (we may reload FW without re-probing).
> > >
> > > We have a device which supports different number of VFs based on the FW
> > > loaded. Some legacy FWs does not inform the driver how many VFs it can
> > > support, because it supports max. So the flow in our driver is this:
> > >
> > > load_fw(dev);
> > > ...
> > > max_vfs = ask_fw_for_max_vfs(dev);
> > > if (max_vfs >= 0)
> > > return pci_sriov_set_totalvfs(dev, max_vfs);
> > > else /* FW didn't tell us, assume max */
> > > return pci_sriov_reset_totalvfs(dev);
> > >
> > > We also reset the max on device remove, but that's not strictly
> > > necessary.
> > >
> > > Other users of pci_sriov_set_totalvfs() always know the value to set
> > > the total to (either always get it from FW or it's a constant).
> > >
> > > If you prefer we can work out the correct max for those legacy cases in
> > > the driver as well, although it seemed cleaner to just ask the core,
> > > since it already has total_VFs value handy :)
> > >
> > > > I'm also having a hard time connecting your user-space command example
> > > > with the rest of this. Maybe it will make more sense to me tomorrow
> > > > after some coffee.
> > >
> > > OpenStack assumes it will always be able to set sriov_numvfs to
> > > sriov_totalvfs, see this 'if':
> > >
> > > http://git.openstack.org/cgit/openstack/charm-neutron-openvswitch/tree/hooks/neutron_ovs_utils.py#n512
> >
> > Thanks for educating me. I think there are two issues here that we
> > can separate. I extracted the patch below for the first.
> >
> > The second is the question of resetting driver_max_VFs. I think we
> > currently have a general issue in the core:
> >
> > - load PF driver 1
> > - driver calls pci_sriov_set_totalvfs() to reduce driver_max_VFs
> > - unload PF driver 1
> > - load PF driver 2
> >
> > Now driver_max_VFs is still stuck at the lower value set by driver 1.
> > I don't think that's the way this should work.
> >
> > I guess this is partly a consequence of setting driver_max_VFs in
> > sriov_init(), which is called before driver attach and should only
> > depend on hardware characteristics, so it is related to the patch
> > below. But I think we should fix it in general, not just for
> > netronome.
>
> Hi Jakub, the patch below is in v4.18-rc1 as 8d85a7a4f2c9 ("PCI/IOV:
> Allow PF drivers to limit total_VFs to 0"). If there's more we need
> to do here, would you mind rebasing what's left to v4.18-rc1 and
> reposting it?
Hi Bjorn!
Thanks a lot for looking into this! My understanding is that we have
two ways forward:
- add a pci_sriov_reset_totalvfs() helper for drivers to call;
- make the core reset the totalVFs after driver is detached.
IMHO second option is better. I went ahead and posted:
https://patchwork.ozlabs.org/patch/931210/
This works very well for nfp driver (modulo minor clean ups but I'd
rather route those via networking trees to avoid conflicts).
> > commit 4a338bc6f94b9ad824ac944f5dfc249d6838719c
> > Author: Jakub Kicinski <jakub.kicinski@netronome.com>
> > Date: Fri May 25 08:18:34 2018 -0500
> >
> > PCI/IOV: Allow PF drivers to limit total_VFs to 0
> >
> > Some SR-IOV PF drivers implement .sriov_configure(), which allows
> > user-space to enable VFs by writing the desired number of VFs to the sysfs
> > "sriov_numvfs" file (see sriov_numvfs_store()).
> >
> > The PCI core limits the number of VFs to the TotalVFs advertised by the
> > device in its SR-IOV capability. The PF driver can limit the number of VFs
> > to even fewer (it may have pre-allocated data structures or knowledge of
> > device limitations) by calling pci_sriov_set_totalvfs(), but previously it
> > could not limit the VFs to 0.
> >
> > Change pci_sriov_get_totalvfs() so it always respects the VF limit imposed
> > by the PF driver, even if the limit is 0.
> >
> > This sequence:
> >
> > pci_sriov_set_totalvfs(dev, 0);
> > x = pci_sriov_get_totalvfs(dev);
> >
> > previously set "x" to TotalVFs from the SR-IOV capability. Now it will set
> > "x" to 0.
> >
> > Signed-off-by: Jakub Kicinski <jakub.kicinski@netronome.com>
> > Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>
> >
> > diff --git a/drivers/pci/iov.c b/drivers/pci/iov.c
> > index 192b82898a38..d0d73dbbd5ca 100644
> > --- a/drivers/pci/iov.c
> > +++ b/drivers/pci/iov.c
> > @@ -469,6 +469,7 @@ static int sriov_init(struct pci_dev *dev, int pos)
> > iov->nres = nres;
> > iov->ctrl = ctrl;
> > iov->total_VFs = total;
> > + iov->driver_max_VFs = total;
> > pci_read_config_word(dev, pos + PCI_SRIOV_VF_DID, &iov->vf_device);
> > iov->pgsz = pgsz;
> > iov->self = dev;
> > @@ -827,10 +828,7 @@ int pci_sriov_get_totalvfs(struct pci_dev *dev)
> > if (!dev->is_physfn)
> > return 0;
> >
> > - if (dev->sriov->driver_max_VFs)
> > - return dev->sriov->driver_max_VFs;
> > -
> > - return dev->sriov->total_VFs;
> > + return dev->sriov->driver_max_VFs;
> > }
> > EXPORT_SYMBOL_GPL(pci_sriov_get_totalvfs);
> >
^ permalink raw reply [flat|nested] 23+ messages in thread