On 06.10.2011 [18:05:37 -0400], Prarit Bhargava wrote:
>
>
> On 10/06/2011 05:03 PM, Nishanth Aravamudan wrote:
> > We have observed the following on Power systems with SR-IOV capable
> > adapters:
> >
> > lpfc 0002:01:00.0: device not available because of BAR 7 [0x000000-0x00ffff] collisions
> >
> > The issue is that on Power systems, PCI BARs cannot be remapped and VF
> > BARs might have values that collide. As far as I can tell, the current
> > SR-IOV code cannot be supported on Power and so it seems like we could
> > provide a hook for an architecture that might set CONFIG_PCI_IOV to
> > disable SR-IOV support (potentially at run-time).
> >
>
> I'd rather see an off/on switch because in a short while when powerpc
> does support SRIOV there maybe no users of the code you've suggested.
Well, I think this is orthogonal to my patch and seems useful
independent of it.
There are two use cases being discussed here, I think.
1) Mis-behaving implementations.
Your patch does resolve this issue by allowing the *user* (not
the kernel itself at run-time) to determine if SR-IOV is the
root cause by trying to disable it.
2) Platforms that are known not to support SR-IOV as currently
implemented.
My patch resolves this issue by not attemping SR-IOV support on
those platforms.
I suppose your hammer is bigger than mine :) But I don't think it's good
to have a flag to workaround a code issue that the user must pass to
ensure functioning adapters.
> -----8<------
>
> [PATCH] Add a kernel parameter that disables SRIOV
>
> Add sriov=[0|1] as a kernel parameter to disable or enable SRIOV. This is
> useful for debugging SRIOV issues vs. bare-metal issues.
>
> Signed-off-by: Prarit Bhargava <prarit@xxxxxxxxxx>
> ---
> Documentation/kernel-parameters.txt | 4 ++++
> drivers/pci/iov.c | 30 +++++++++++++++++++++++++++++-
> 2 files changed, 33 insertions(+), 1 deletions(-)
>
> diff --git a/Documentation/kernel-parameters.txt b/Documentation/kernel-parameters.txt
> index 854ed5c..235b5c5 100644
> --- a/Documentation/kernel-parameters.txt
> +++ b/Documentation/kernel-parameters.txt
> @@ -2385,6 +2385,10 @@ bytes respectively. Such letter suffixes can also be entirely omitted.
> spia_pedr=
> spia_peddr=
>
> + sriov= [HW]
> + Format: [0|1]
> + Enable or disable SRIOV support in kernel.
> +
> stacktrace [FTRACE]
> Enabled the stack tracer on boot up.
>
> diff --git a/drivers/pci/iov.c b/drivers/pci/iov.c
> index 42fae47..da96e3f 100644
> --- a/drivers/pci/iov.c
> +++ b/drivers/pci/iov.c
> @@ -408,6 +408,19 @@ static void sriov_disable(struct pci_dev *dev)
> iov->nr_virtfn = 0;
> }
>
> +static unsigned int sriov_enabled = 1;
> +static int __init do_sriov_enable(char *str)
> +{
> + get_option(&str, &sriov_enabled);
> + if (!sriov_enabled)
> + printk(KERN_INFO "SRIOV: Disabled\n");
> + else
> + printk(KERN_INFO "SRIOV: Enabled\n");
> +
> + return 1;
> +}
> +__setup("sriov=", do_sriov_enable);
Hrm, we only will print the above is sriov= is passed on the kernel
command-line, right? It seems like maybe we should print it always. And
perhaps the prefix should be "SR-IOV"?
> static int sriov_init(struct pci_dev *dev, int pos)
> {
> int i;
> @@ -419,6 +432,12 @@ static int sriov_init(struct pci_dev *dev, int pos)
> struct resource *res;
> struct pci_dev *pdev;
>
> + if (!sriov_enabled) {
> + rc = -EPERM;
> + /* in theory no flags should have been set ... */
> + goto failed;
> + }
> +
Hrm, is this needed if pci_enable_sriov() checks it already? Isn't that
the only caller?
> if (dev->pcie_type != PCI_EXP_TYPE_RC_END &&
> dev->pcie_type != PCI_EXP_TYPE_ENDPOINT)
> return -ENODEV;
> @@ -614,7 +633,10 @@ resource_size_t pci_sriov_resource_alignment(struct pci_dev *dev, int resno)
> struct resource tmp;
> enum pci_bar_type type;
> int reg = pci_iov_resource_bar(dev, resno, &type);
> -
> +
> + if (!sriov_enabled)
> + return 0;
> +
> if (!reg)
> return 0;
>
> @@ -667,6 +689,9 @@ int pci_enable_sriov(struct pci_dev *dev, int nr_virtfn)
> {
> might_sleep();
>
> + if (!sriov_enabled)
> + return -EPERM;
> +
> if (!dev->is_physfn)
> return -ENODEV;
>
> @@ -682,6 +707,9 @@ void pci_disable_sriov(struct pci_dev *dev)
> {
> might_sleep();
>
> + if (!sriov_enabled)
> + return;
> +
> if (!dev->is_physfn)
> return;
Ah my patch needs these three checks as well, FWIW.
Thanks,
Nish
--
Nishanth Aravamudan <nacc@xxxxxxxxxx>
IBM Linux Technology Center
--
To unsubscribe from this list: send the line "unsubscribe linux-pci" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
